From 4fe03031ddc1225c33c6c7eeb10ee7e208dfb879 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sat, 13 Jul 2024 19:05:22 -0300 Subject: [PATCH 1/5] feat(networkmanager): implement WiFi strenght levels Also collect SSID information. Althought nothing is being done with it for now. This was done by interfacing with `NetworkManager.Device.Wifi` and `NetworkManager.AccessPoint` dbus interfaces. --- src/clients/networkmanager/dbus.rs | 22 +++++++++++++ src/clients/networkmanager/mod.rs | 7 ++-- src/clients/networkmanager/state.rs | 31 ++++++++++++++---- src/modules/networkmanager.rs | 50 ++++++++++++++++++++++++++++- 4 files changed, 100 insertions(+), 10 deletions(-) diff --git a/src/clients/networkmanager/dbus.rs b/src/clients/networkmanager/dbus.rs index 5ab3889..25c289a 100644 --- a/src/clients/networkmanager/dbus.rs +++ b/src/clients/networkmanager/dbus.rs @@ -69,6 +69,28 @@ trait DeviceDbus { fn state(&self) -> Result; } +#[dbus_proxy( + default_service = "org.freedesktop.NetworkManager", + interface = "org.freedesktop.NetworkManager.Device.Wireless" +)] +trait DeviceWirelessDbus { + #[dbus_proxy(property)] + fn active_access_point(&self) -> zbus::Result; +} + +// based on code generated by `zbus-xmlgen system org.freedesktop.NetworkManager /org/freedesktop/NetworkManager/AccessPoint/1` +#[dbus_proxy( + default_service = "org.freedesktop.NetworkManager", + interface = "org.freedesktop.NetworkManager.AccessPoint" +)] +trait AccessPointDbus { + #[dbus_proxy(property)] + fn ssid(&self) -> zbus::Result>; + + #[dbus_proxy(property)] + fn strength(&self) -> Result; +} + #[derive(Clone, Debug, OwnedValue, PartialEq)] #[repr(u32)] pub(super) enum DeviceType { diff --git a/src/clients/networkmanager/mod.rs b/src/clients/networkmanager/mod.rs index b0ac6a2..57bed1b 100644 --- a/src/clients/networkmanager/mod.rs +++ b/src/clients/networkmanager/mod.rs @@ -64,7 +64,10 @@ impl Client { ($client:ident) => { $client.state.set(State { wired: determine_wired_state(&read_lock!($client.devices))?, - wifi: determine_wifi_state(&read_lock!($client.devices))?, + wifi: determine_wifi_state( + &$client.dbus_connection, + &read_lock!($client.devices), + )?, cellular: determine_cellular_state(&read_lock!($client.devices))?, vpn: $client.state.get_cloned().vpn, }); @@ -178,7 +181,7 @@ impl Client { }); self.0.state.set(State { wired: determine_wired_state(&read_lock!(self.0.devices))?, - wifi: determine_wifi_state(&read_lock!(self.0.devices))?, + wifi: determine_wifi_state(&self.0.dbus_connection, &read_lock!(self.0.devices))?, cellular: determine_cellular_state(&read_lock!(self.0.devices))?, vpn: determine_vpn_state(&read_lock!(self.0.active_connections))?, }); diff --git a/src/clients/networkmanager/state.rs b/src/clients/networkmanager/state.rs index e5e226e..8bafead 100644 --- a/src/clients/networkmanager/state.rs +++ b/src/clients/networkmanager/state.rs @@ -1,7 +1,8 @@ use color_eyre::Result; use crate::clients::networkmanager::dbus::{ - ActiveConnectionDbusProxyBlocking, DeviceDbusProxyBlocking, DeviceState, DeviceType, + AccessPointDbusProxyBlocking, ActiveConnectionDbusProxyBlocking, DeviceDbusProxyBlocking, + DeviceState, DeviceType, DeviceWirelessDbusProxyBlocking, }; use crate::clients::networkmanager::PathMap; @@ -33,6 +34,8 @@ pub enum WifiState { #[derive(Clone, Debug)] pub struct WifiConnectedState { pub ssid: String, + /// Strength in percentage, from 0 to 100. + pub strength: u8, } #[derive(Clone, Debug)] @@ -82,29 +85,43 @@ pub(super) fn determine_wired_state( } pub(super) fn determine_wifi_state( + dbus_connection: &zbus::blocking::Connection, devices: &PathMap, ) -> Result { let mut present = false; let mut enabled = false; - let mut connected = false; + let mut connected = None; for device in devices.values() { if device.device_type()? == DeviceType::Wifi { present = true; if device.state()?.is_enabled() { enabled = true; - if device.state()? == DeviceState::Activated { - connected = true; + + let wireless_device = DeviceWirelessDbusProxyBlocking::builder(dbus_connection) + .path(device.path().clone())? + .build()?; + let primary_access_point_path = wireless_device.active_access_point()?; + if primary_access_point_path.as_str() != "/" { + connected = Some( + AccessPointDbusProxyBlocking::builder(dbus_connection) + .path(primary_access_point_path)? + .build()?, + ); break; } } } } - if connected { + if let Some(access_point) = connected { + let ssid = access_point + .ssid() + .map(|x| String::from_utf8_lossy(&x).to_string()) + .unwrap_or_else(|_| "unkown".into()); Ok(WifiState::Connected(WifiConnectedState { - // TODO: Implement obtaining SSID - ssid: "unknown".into(), + ssid, + strength: access_point.strength().unwrap_or(0), })) } else if enabled { Ok(WifiState::Disconnected) diff --git a/src/modules/networkmanager.rs b/src/modules/networkmanager.rs index c53f57a..e5be89e 100644 --- a/src/modules/networkmanager.rs +++ b/src/modules/networkmanager.rs @@ -111,7 +111,17 @@ impl Module for NetworkManagerModule { WiredState::NotPresent | WiredState::Unknown => "", }); update_icon!(wifi_icon, wifi, { - WifiState::Connected(_) => "icon:network-wireless-connected-symbolic", + WifiState::Connected(state) => { + let icons = [ + "icon:network-wireless-signal-none-symbolic", + "icon:network-wireless-signal-weak-symbolic", + "icon:network-wireless-signal-ok-symbolic", + "icon:network-wireless-signal-good-symbolic", + "icon:network-wireless-signal-excellent-symbolic", + ]; + let n = strengh_to_level(state.strength, icons.len()); + icons[n] + }, WifiState::Disconnected => "icon:network-wireless-offline-symbolic", WifiState::Disabled => "icon:network-wireless-hardware-disabled-symbolic", WifiState::NotPresent | WifiState::Unknown => "", @@ -133,3 +143,41 @@ impl Module for NetworkManagerModule { module_impl!("networkmanager"); } + +/// Convert strength level (from 0-100), to a level (from 0 to `number_of_levels-1`). +const fn strengh_to_level(strength: u8, number_of_levels: usize) -> usize { + // Strength levels based for the one show by [`nmcli dev wifi list`](https://github.com/NetworkManager/NetworkManager/blob/83a259597000a88217f3ccbdfe71c8114242e7a6/src/libnmc-base/nm-client-utils.c#L700-L727): + // match strength { + // 0..=4 => 0, + // 5..=29 => 1, + // 30..=54 => 2, + // 55..=79 => 3, + // 80.. => 4, + // } + + // to make it work with a custom number of levels, we approach the logic above with the logic + // below (0 for < 5, and a linear interpolation for 5 to 105). + // TODO: if there are more than 20 levels, the last level will be out of scale, and never be + // reach. + if strength < 5 { + return 0; + } + (strength as usize - 5) * (number_of_levels - 1) / 100 + 1 +} + +// Just to make sure my implementation still follow the original logic +#[cfg(test)] +#[test] +fn test_strength_to_level() { + assert_eq!(strengh_to_level(0, 5), 0); + assert_eq!(strengh_to_level(4, 5), 0); + assert_eq!(strengh_to_level(5, 5), 1); + assert_eq!(strengh_to_level(6, 5), 1); + assert_eq!(strengh_to_level(29, 5), 1); + assert_eq!(strengh_to_level(30, 5), 2); + assert_eq!(strengh_to_level(54, 5), 2); + assert_eq!(strengh_to_level(55, 5), 3); + assert_eq!(strengh_to_level(79, 5), 3); + assert_eq!(strengh_to_level(80, 5), 4); + assert_eq!(strengh_to_level(100, 5), 4); +} From 81900c6a293d2708430c254dd2ba97f8cf1080f5 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sat, 13 Jul 2024 20:52:40 -0300 Subject: [PATCH 2/5] feat(networkmanager): add config for icons --- docs/modules/Networkmanager.md | 89 +++++++++++--------- src/modules/networkmanager.rs | 32 ++++--- src/modules/networkmanager/config.rs | 120 +++++++++++++++++++++++++++ 3 files changed, 186 insertions(+), 55 deletions(-) create mode 100644 src/modules/networkmanager/config.rs diff --git a/docs/modules/Networkmanager.md b/docs/modules/Networkmanager.md index bd318d9..e79cfde 100644 --- a/docs/modules/Networkmanager.md +++ b/docs/modules/Networkmanager.md @@ -10,64 +10,77 @@ Supports wired ethernet, wifi, cellular data and VPN connections among others. > Type: `networkmanager` -| Name | Type | Default | Description | -|-------------|-----------|---------|-------------------------| -| `icon_size` | `integer` | `24` | Size to render icon at. | +| Name | Type | Default | Description | +| ----------------------------- | ---------- | ----------------------------------------------------- | ------------------------------------------------- | +| `icon_size` | `integer` | `24` | Size to render icon at. | +| `icons.wired.connected` | `string` | `icon:network-wired-symbolic` | Icon to show when there is a wired connection | +| `icons.wired.disconnected` | `string` | `icon:network-wired-symbolic` | Icon to show when there is no wired connection | +| `icons.wifi.levels` | `string[]` | `["icon:network-wireless-signal-none-symbolic", ...]` | Icon to show when there is no wifi connection | +| `icons.wifi.disconnected` | `string` | `icon:network-wireless-offline-symbolic` | Icon to show when there is no wifi connection | +| `icons.wifi.disabled` | `string` | `icon:network-wireless-hardware-disabled-symbolic` | Icon to show when wifi is disabled | +| `icons.cellular.connected` | `string` | `icon:network-cellular-connected-symbolic` | Icon to show when there is a cellular connection | +| `icons.cellular.disconnected` | `string` | `icon:network-cellular-offline-symbolic` | Icon to show when there is no cellular connection | +| `icons.cellular.disabled` | `string` | `icon:network-cellular-hardware-disabled-symbolic` | Icon to show when cellular connection is disabled | +| `icons.vpn.connected` | `string` | `icon:network-vpn-symbolic` | Icon to show when there is a VPN connection |
- JSON +JSON + +```json +{ + "end": [ + { + "type": "networkmanager", + "icon_size": 32 + } + ] +} +``` - ```json - { - "end": [ - { - "type": "networkmanager", - "icon_size": 32 - } - ] - } - ```
- TOML +TOML + +```toml +[[end]] +type = "networkmanager" +icon_size = 32 +``` - ```toml - [[end]] - type = "networkmanager" - icon_size = 32 - ```
- YAML +YAML + +```yaml +end: + - type: "networkmanager" + icon_size: 32 +``` - ```yaml - end: - - type: "networkmanager" - icon_size: 32 - ```
- Corn +Corn + +```corn +{ + end = [ + { + type = "networkmanager" + icon_size = 32 + } + ] +} +``` - ```corn - { - end = [ - { - type = "networkmanager" - icon_size = 32 - } - ] - } - ```
## Styling | Selector | Description | -|------------------------|----------------------------------| +| ---------------------- | -------------------------------- | | `.networkmanager` | NetworkManager widget container. | | `.networkmanger .icon` | NetworkManager widget icons. | diff --git a/src/modules/networkmanager.rs b/src/modules/networkmanager.rs index e5be89e..2689d90 100644 --- a/src/modules/networkmanager.rs +++ b/src/modules/networkmanager.rs @@ -1,3 +1,5 @@ +mod config; + use color_eyre::Result; use futures_lite::StreamExt; use futures_signals::signal::SignalExt; @@ -22,6 +24,9 @@ pub struct NetworkManagerModule { #[serde(default = "default_icon_size")] icon_size: i32, + #[serde(default)] + icons: config::IconsConfig, + #[serde(flatten)] pub common: Option, } @@ -106,34 +111,27 @@ impl Module for NetworkManagerModule { } update_icon!(wired_icon, wired, { - WiredState::Connected => "icon:network-wired-symbolic", - WiredState::Disconnected => "icon:network-wired-disconnected-symbolic", + WiredState::Connected => &self.icons.wired.connected, + WiredState::Disconnected => &self.icons.wired.disconnected, WiredState::NotPresent | WiredState::Unknown => "", }); update_icon!(wifi_icon, wifi, { WifiState::Connected(state) => { - let icons = [ - "icon:network-wireless-signal-none-symbolic", - "icon:network-wireless-signal-weak-symbolic", - "icon:network-wireless-signal-ok-symbolic", - "icon:network-wireless-signal-good-symbolic", - "icon:network-wireless-signal-excellent-symbolic", - ]; - let n = strengh_to_level(state.strength, icons.len()); - icons[n] + let n = strengh_to_level(state.strength, self.icons.wifi.levels.len()); + &self.icons.wifi.levels[n] }, - WifiState::Disconnected => "icon:network-wireless-offline-symbolic", - WifiState::Disabled => "icon:network-wireless-hardware-disabled-symbolic", + WifiState::Disconnected => &self.icons.wifi.disconnected, + WifiState::Disabled => &self.icons.wifi.disabled, WifiState::NotPresent | WifiState::Unknown => "", }); update_icon!(cellular_icon, cellular, { - CellularState::Connected => "icon:network-cellular-connected-symbolic", - CellularState::Disconnected => "icon:network-cellular-offline-symbolic", - CellularState::Disabled => "icon:network-cellular-hardware-disabled-symbolic", + CellularState::Connected => &self.icons.cellular.connected, + CellularState::Disconnected => &self.icons.cellular.disconnected, + CellularState::Disabled => &self.icons.cellular.disabled, CellularState::NotPresent | CellularState::Unknown => "", }); update_icon!(vpn_icon, vpn, { - VpnState::Connected(_) => "icon:network-vpn-symbolic", + VpnState::Connected(_) => &self.icons.vpn.connected, VpnState::Disconnected | VpnState::Unknown => "", }); }); diff --git a/src/modules/networkmanager/config.rs b/src/modules/networkmanager/config.rs new file mode 100644 index 0000000..c2fd77c --- /dev/null +++ b/src/modules/networkmanager/config.rs @@ -0,0 +1,120 @@ +use serde::Deserialize; + +macro_rules! default_function { + ($(($name:ident, $default:expr),)*) => { + $( + fn $name() -> String { + ($default).to_string() + } + )* + }; +} + +#[derive(Debug, Deserialize, Clone, Default)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct IconsConfig { + #[serde(default)] + pub wired: IconsConfigWired, + #[serde(default)] + pub wifi: IconsConfigWifi, + #[serde(default)] + pub cellular: IconsConfigCellular, + #[serde(default)] + pub vpn: IconsConfigVpn, +} + +#[derive(Debug, Deserialize, Clone)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct IconsConfigWired { + #[serde(default = "default_wired_connected")] + pub connected: String, + #[serde(default = "default_wired_disconnected")] + pub disconnected: String, +} +impl Default for IconsConfigWired { + fn default() -> Self { + Self { + connected: default_wired_connected(), + disconnected: default_wired_disconnected(), + } + } +} + +#[derive(Debug, Deserialize, Clone)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct IconsConfigWifi { + #[serde(default = "default_wifi_levels")] + pub levels: Vec, + #[serde(default = "default_wifi_disconnected")] + pub disconnected: String, + #[serde(default = "default_wifi_disabled")] + pub disabled: String, +} + +impl Default for IconsConfigWifi { + fn default() -> Self { + Self { + levels: default_wifi_levels(), + disconnected: default_wifi_disconnected(), + disabled: default_wifi_disabled(), + } + } +} + +#[derive(Debug, Deserialize, Clone)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct IconsConfigCellular { + #[serde(default = "default_cellular_connected")] + pub connected: String, + #[serde(default = "default_cellular_disconnected")] + pub disconnected: String, + #[serde(default = "default_cellular_disabled")] + pub disabled: String, +} +impl Default for IconsConfigCellular { + fn default() -> Self { + Self { + connected: default_cellular_connected(), + disconnected: default_cellular_disconnected(), + disabled: default_cellular_disabled(), + } + } +} + +#[derive(Debug, Deserialize, Clone)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct IconsConfigVpn { + #[serde(default = "default_vpn_connected")] + pub connected: String, +} +impl Default for IconsConfigVpn { + fn default() -> Self { + Self { + connected: default_vpn_connected(), + } + } +} + +pub fn default_wifi_levels() -> Vec { + vec![ + "icon:network-wireless-signal-none-symbolic".to_string(), + "icon:network-wireless-signal-weak-symbolic".to_string(), + "icon:network-wireless-signal-ok-symbolic".to_string(), + "icon:network-wireless-signal-good-symbolic".to_string(), + "icon:network-wireless-signal-excellent-symbolic".to_string(), + ] +} + +default_function! { + (default_wired_connected, "icon:network-wired-symbolic"), + (default_wired_disconnected, "icon:network-wired-disconnected-symbolic"), + + (default_wifi_disconnected, "icon:network-wireless-offline-symbolic"), + (default_wifi_disabled, "icon:network-wireless-hardware-disabled-symbolic"), + + (default_cellular_connected,"icon:network-cellular-connected-symbolic"), + (default_cellular_disconnected,"icon:network-cellular-offline-symbolic"), + (default_cellular_disabled,"icon:network-cellular-hardware-disabled-symbolic"), + + (default_vpn_connected, "icon:network-vpn-symbolic"), +} From 5c19032e8b70285fc1f60ebd20f7ad4082bf8b29 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sat, 13 Jul 2024 21:51:08 -0300 Subject: [PATCH 3/5] feat(networkmanager): add SSID as tooltip --- src/modules/networkmanager.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/modules/networkmanager.rs b/src/modules/networkmanager.rs index 2689d90..04ec6fe 100644 --- a/src/modules/networkmanager.rs +++ b/src/modules/networkmanager.rs @@ -110,6 +110,15 @@ impl Module for NetworkManagerModule { }; } + match &state.wifi { + WifiState::Connected(state) => { + wifi_icon.set_tooltip_text(Some(&state.ssid)); + }, + _ => { + wifi_icon.set_tooltip_text(None); + }, + } + update_icon!(wired_icon, wired, { WiredState::Connected => &self.icons.wired.connected, WiredState::Disconnected => &self.icons.wired.disconnected, From f74f550db90ea6a3d7ac0de023c62530c3ed23f9 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sat, 13 Jul 2024 23:38:22 -0300 Subject: [PATCH 4/5] feat(networkmanager): add more info to wifi tooltip I was aiming to make it more customizable, but there is currently no support for custom tooltip formatting with module dependent variables. --- src/clients/networkmanager/dbus.rs | 21 +++++++++++++++ src/clients/networkmanager/state.rs | 40 +++++++++++++++++++++++++---- src/modules/networkmanager.rs | 3 ++- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/clients/networkmanager/dbus.rs b/src/clients/networkmanager/dbus.rs index 25c289a..a231dbe 100644 --- a/src/clients/networkmanager/dbus.rs +++ b/src/clients/networkmanager/dbus.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use color_eyre::Result; use zbus::dbus_proxy; use zbus::zvariant::{ObjectPath, OwnedValue, Str}; @@ -67,6 +69,12 @@ trait DeviceDbus { #[dbus_proxy(property)] fn state(&self) -> Result; + + #[dbus_proxy(property)] + fn interface(&self) -> Result; + + #[dbus_proxy(property)] + fn ip4_config(&self) -> Result; } #[dbus_proxy( @@ -89,6 +97,19 @@ trait AccessPointDbus { #[dbus_proxy(property)] fn strength(&self) -> Result; + + #[dbus_proxy(property)] + fn hw_address(&self) -> Result; +} + +// based on code generated by `zbus-xmlgen system org.freedesktop.NetworkManager /org/freedesktop/NetworkManager/AccessPoint/1` +#[dbus_proxy( + default_service = "org.freedesktop.NetworkManager", + interface = "org.freedesktop.NetworkManager.IP4Config" +)] +trait Ip4ConfigDbus { + #[dbus_proxy(property)] + fn address_data(&self) -> Result>>; } #[derive(Clone, Debug, OwnedValue, PartialEq)] diff --git a/src/clients/networkmanager/state.rs b/src/clients/networkmanager/state.rs index 8bafead..5cc1532 100644 --- a/src/clients/networkmanager/state.rs +++ b/src/clients/networkmanager/state.rs @@ -1,8 +1,8 @@ -use color_eyre::Result; +use color_eyre::{Report, Result}; use crate::clients::networkmanager::dbus::{ AccessPointDbusProxyBlocking, ActiveConnectionDbusProxyBlocking, DeviceDbusProxyBlocking, - DeviceState, DeviceType, DeviceWirelessDbusProxyBlocking, + DeviceState, DeviceType, DeviceWirelessDbusProxyBlocking, Ip4ConfigDbusProxyBlocking, }; use crate::clients::networkmanager::PathMap; @@ -33,9 +33,16 @@ pub enum WifiState { #[derive(Clone, Debug)] pub struct WifiConnectedState { + /// The SSID of the access point. pub ssid: String, + /// The MAC address of the access point. + pub bssid: String, /// Strength in percentage, from 0 to 100. pub strength: u8, + /// The IPv4 address. + pub ip4_address: String, + /// The IPv4 prefix, in bits (also known as the subnet mask length). + pub ip4_prefix: u32, } #[derive(Clone, Debug)] @@ -103,24 +110,47 @@ pub(super) fn determine_wifi_state( .build()?; let primary_access_point_path = wireless_device.active_access_point()?; if primary_access_point_path.as_str() != "/" { - connected = Some( + connected = Some(( + device, AccessPointDbusProxyBlocking::builder(dbus_connection) .path(primary_access_point_path)? .build()?, - ); + )); break; } } } } - if let Some(access_point) = connected { + if let Some((device, access_point)) = connected { let ssid = access_point .ssid() .map(|x| String::from_utf8_lossy(&x).to_string()) .unwrap_or_else(|_| "unkown".into()); + let bssid = access_point.hw_address()?.to_string(); + + let ip4config = Ip4ConfigDbusProxyBlocking::builder(dbus_connection) + .path(device.ip4_config()?.clone())? + .build()?; + let address_data = ip4config.address_data()?; + // pick the first address. not sure if there are cases when there are more than one address + // (at least for wifi). + let address = &address_data + .iter() + .next() + .ok_or_else(|| Report::msg("No address in IP4Config"))?; + let ip4_address = address + .get("address") + .ok_or_else(|| Report::msg("IP address data object must have a address"))?; + let ip4_prefix = address + .get("prefix") + .ok_or_else(|| Report::msg("IP address data object must have a prefix"))?; + Ok(WifiState::Connected(WifiConnectedState { ssid, + bssid, + ip4_address: String::try_from(ip4_address.to_owned()).unwrap_or_default(), + ip4_prefix: u32::try_from(ip4_prefix.to_owned()).unwrap_or_default(), strength: access_point.strength().unwrap_or(0), })) } else if enabled { diff --git a/src/modules/networkmanager.rs b/src/modules/networkmanager.rs index 04ec6fe..e2a71ed 100644 --- a/src/modules/networkmanager.rs +++ b/src/modules/networkmanager.rs @@ -112,7 +112,8 @@ impl Module for NetworkManagerModule { match &state.wifi { WifiState::Connected(state) => { - wifi_icon.set_tooltip_text(Some(&state.ssid)); + let tooltip = format!("{}\n{}/{}", state.ssid, state.ip4_address, state.ip4_prefix); + wifi_icon.set_tooltip_text(Some(&tooltip)); }, _ => { wifi_icon.set_tooltip_text(None); From a34c34c5fc51d7cd473cb64bdcb7467490192391 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sun, 14 Jul 2024 21:51:48 -0300 Subject: [PATCH 5/5] fix(networkmanager): update wifi state on strength changes Also fix some bugs with the overall device state change listeners. --- src/clients/networkmanager/mod.rs | 158 ++++++++++++++++------------ src/clients/networkmanager/state.rs | 59 +++++++++-- 2 files changed, 139 insertions(+), 78 deletions(-) diff --git a/src/clients/networkmanager/mod.rs b/src/clients/networkmanager/mod.rs index 57bed1b..1134cf0 100644 --- a/src/clients/networkmanager/mod.rs +++ b/src/clients/networkmanager/mod.rs @@ -8,7 +8,8 @@ use zbus::blocking::Connection; use zbus::zvariant::ObjectPath; use crate::clients::networkmanager::dbus::{ - ActiveConnectionDbusProxyBlocking, DbusProxyBlocking, DeviceDbusProxyBlocking, + AccessPointDbusProxyBlocking, ActiveConnectionDbusProxyBlocking, DbusProxyBlocking, + DeviceDbusProxyBlocking, }; use crate::clients::networkmanager::state::{ determine_cellular_state, determine_vpn_state, determine_wifi_state, determine_wired_state, @@ -32,8 +33,22 @@ struct ClientInner<'l> { root_object: &'l DbusProxyBlocking<'l>, active_connections: RwLock>>, devices: RwLock>>, + access_point: RwLock, AccessPointDbusProxyBlocking<'l>)>>, dbus_connection: Connection, } +impl ClientInner<'static> { + /// Query the state information for each device. This method can fail at random if the + /// connection changes while querying the information. + fn update_state_for_device_change(self: &Arc>) -> Result<()> { + self.state.set(State { + wired: determine_wired_state(&read_lock!(self.devices))?, + wifi: determine_wifi_state(&Client(self.clone()))?, + cellular: determine_cellular_state(&read_lock!(self.devices))?, + vpn: self.state.get_cloned().vpn, + }); + Ok(()) + } +} impl Client { fn new() -> Result { @@ -55,48 +70,12 @@ impl Client { root_object, active_connections: RwLock::new(HashMap::new()), devices: RwLock::new(HashMap::new()), + access_point: RwLock::new(None), dbus_connection, }))) } fn run(&self) -> Result<()> { - macro_rules! update_state_for_device_change { - ($client:ident) => { - $client.state.set(State { - wired: determine_wired_state(&read_lock!($client.devices))?, - wifi: determine_wifi_state( - &$client.dbus_connection, - &read_lock!($client.devices), - )?, - cellular: determine_cellular_state(&read_lock!($client.devices))?, - vpn: $client.state.get_cloned().vpn, - }); - }; - } - - macro_rules! initialise_path_map { - ( - $client:expr, - $path_map:ident, - $proxy_type:ident - $(, |$new_path:ident| $property_watcher:expr)* - ) => { - let new_paths = $client.root_object.$path_map()?; - let mut path_map = HashMap::new(); - for new_path in new_paths { - let new_proxy = $proxy_type::builder(&$client.dbus_connection) - .path(new_path.clone())? - .build()?; - path_map.insert(new_path.clone(), new_proxy); - $({ - let $new_path = &new_path; - $property_watcher; - })* - } - *write_lock!($client.$path_map) = path_map; - }; - } - macro_rules! spawn_path_list_watcher { ( $client:expr, @@ -111,29 +90,33 @@ impl Client { let changes = client.root_object.$property_changes(); for _ in changes { let mut new_path_map = HashMap::new(); + let new_paths = client.root_object.$property()?; { - let new_paths = client.root_object.$property()?; let path_map = read_lock!(client.$property); - for new_path in new_paths { + for new_path in &new_paths { if path_map.contains_key(&new_path) { let proxy = path_map - .get(&new_path) + .get(new_path) .expect("Should contain the key, guarded by runtime check"); - new_path_map.insert(new_path, proxy.to_owned()); + new_path_map.insert(new_path.clone(), proxy.to_owned()); } else { let new_proxy = $proxy_type::builder(&client.dbus_connection) .path(new_path.clone())? .build()?; new_path_map.insert(new_path.clone(), new_proxy); - $({ - let $property_client = &client; - let $new_path = &new_path; - $property_watcher; - })* } } } *write_lock!(client.$property) = new_path_map; + + for _new_path in &new_paths { + $({ + let $property_client = &client; + let $new_path = _new_path; + $property_watcher; + })* + } + let $state_client = &client; $state_update; } @@ -153,12 +136,18 @@ impl Client { let client = $client.clone(); let path = $path.clone(); spawn_blocking_result!({ - let changes = read_lock!(client.$containing_list) - .get(&path) - .expect("Should contain the key upon watcher start") - .$property_changes(); + let changes = { + let path_map = read_lock!(client.$containing_list); + let Some(device) = path_map.get(&path) else { + // this item could have been removed before the watcher was initialized + tracing::warn!("item removed before first iteration"); + return Ok(()); + }; + device.$property_changes() + }; for _ in changes { if !read_lock!(client.$containing_list).contains_key(&path) { + // this item no longer exits break; } let $inner_client = &client; @@ -169,22 +158,50 @@ impl Client { }; } - initialise_path_map!( - self.0, - active_connections, - ActiveConnectionDbusProxyBlocking - ); - initialise_path_map!(self.0, devices, DeviceDbusProxyBlocking, |path| { - spawn_property_watcher!(self.0, path, receive_state_changed, devices, |client| { - update_state_for_device_change!(client); - }); - }); - self.0.state.set(State { - wired: determine_wired_state(&read_lock!(self.0.devices))?, - wifi: determine_wifi_state(&self.0.dbus_connection, &read_lock!(self.0.devices))?, - cellular: determine_cellular_state(&read_lock!(self.0.devices))?, - vpn: determine_vpn_state(&read_lock!(self.0.active_connections))?, - }); + // initialize active_connections proxys + { + let active_connections = HashMap::new(); + for active_connection_path in self.0.root_object.active_connections()? { + let proxy = ActiveConnectionDbusProxyBlocking::builder(&self.0.dbus_connection) + .path(active_connection_path.clone())? + .build()?; + self.0 + .active_connections + .write() + .unwrap() + .insert(active_connection_path, proxy); + } + *write_lock!(self.0.active_connections) = active_connections; + } + + // initialize devices proxys and watchers + { + let devices = self.0.root_object.devices()?; + let mut path_map = HashMap::new(); + for device_path in &devices { + let proxy = DeviceDbusProxyBlocking::builder(&self.0.dbus_connection) + .path(device_path.clone())? + .build()?; + path_map.insert(device_path.clone(), proxy); + } + *write_lock!((self.0).devices) = path_map; + + tracing::debug!("initialize devices: {:?}", devices); + + for device_path in devices { + spawn_property_watcher!( + self.0, + device_path, + receive_state_changed, + devices, + |client| { + let _ = client.update_state_for_device_change(); + } + ); + } + } + + let _ = self.0.update_state_for_device_change(); spawn_path_list_watcher!( self.0, @@ -192,6 +209,7 @@ impl Client { receive_active_connections_changed, ActiveConnectionDbusProxyBlocking, |client| { + tracing::debug!("active connections changed"); client.state.set(State { wired: client.state.get_cloned().wired, wifi: client.state.get_cloned().wifi, @@ -206,11 +224,13 @@ impl Client { receive_devices_changed, DeviceDbusProxyBlocking, |client| { - update_state_for_device_change!(client); + tracing::debug!("devices changed"); + let _ = client.update_state_for_device_change(); }, |client, path| { spawn_property_watcher!(client, path, receive_state_changed, devices, |client| { - update_state_for_device_change!(client); + tracing::debug!("device state changed"); + let _ = client.update_state_for_device_change(); }); } ); diff --git a/src/clients/networkmanager/state.rs b/src/clients/networkmanager/state.rs index 5cc1532..66635ba 100644 --- a/src/clients/networkmanager/state.rs +++ b/src/clients/networkmanager/state.rs @@ -5,6 +5,7 @@ use crate::clients::networkmanager::dbus::{ DeviceState, DeviceType, DeviceWirelessDbusProxyBlocking, Ip4ConfigDbusProxyBlocking, }; use crate::clients::networkmanager::PathMap; +use crate::{error, read_lock, spawn_blocking, spawn_blocking_result, write_lock}; #[derive(Clone, Debug)] pub struct State { @@ -91,10 +92,11 @@ pub(super) fn determine_wired_state( } } -pub(super) fn determine_wifi_state( - dbus_connection: &zbus::blocking::Connection, - devices: &PathMap, -) -> Result { +pub(super) fn determine_wifi_state(client: &super::Client) -> Result { + let dbus_connection = &client.0.dbus_connection; + let devices = &read_lock!(client.0.devices); + let access_point_ = &client.0.access_point; + let mut present = false; let mut enabled = false; let mut connected = None; @@ -133,11 +135,11 @@ pub(super) fn determine_wifi_state( .path(device.ip4_config()?.clone())? .build()?; let address_data = ip4config.address_data()?; - // pick the first address. not sure if there are cases when there are more than one address - // (at least for wifi). + // Pick the first address. Not sure if there are cases when there are more than one + // address. + // These could fail if the access point concurrently changes. let address = &address_data - .iter() - .next() + .first() .ok_or_else(|| Report::msg("No address in IP4Config"))?; let ip4_address = address .get("address") @@ -146,12 +148,51 @@ pub(super) fn determine_wifi_state( .get("prefix") .ok_or_else(|| Report::msg("IP address data object must have a prefix"))?; + let strength = access_point.strength()?; + + 'block: { + if let Some((ref path, _)) = *read_lock!(access_point_) { + if path == access_point.path() { + break 'block; + } + } + + let access_point_path = access_point.path().to_owned(); + + *write_lock!(*access_point_) = Some((access_point_path.clone(), access_point)); + + let client = client.0.clone(); + spawn_blocking_result!({ + let changes = { + let access_point = read_lock!(client.access_point); + let Some((_, access_point)) = access_point.as_ref() else { + return Ok(()); + }; + access_point.receive_strength_changed() + }; + + for _ in changes { + if read_lock!(client.access_point) + .as_ref() + .map_or(false, |(p, _)| *p != access_point_path) + { + break; + } + + tracing::trace!("accesspoint strength changed"); + client.update_state_for_device_change()?; + } + + Ok(()) + }); + } + Ok(WifiState::Connected(WifiConnectedState { ssid, bssid, ip4_address: String::try_from(ip4_address.to_owned()).unwrap_or_default(), ip4_prefix: u32::try_from(ip4_prefix.to_owned()).unwrap_or_default(), - strength: access_point.strength().unwrap_or(0), + strength, })) } else if enabled { Ok(WifiState::Disconnected)