refactor(networkmanager): replace state-based w/ event-based approach

Cargo.toml

@@ -8,6 +8,9 @@ repository = "https://github.com/jakestanger/ironbar"
 categories = ["gui"]
 keywords = ["gtk", "bar", "wayland", "wlroots", "gtk-layer-shell"]
 
+[profile.release]
+lto = true
+
 [features]
 default = [
     "bindmode+all",

src/clients/networkmanager/dbus.rs

@@ -12,46 +12,10 @@ pub(super) trait Dbus {
     fn active_connections(&self) -> Result<Vec<ObjectPath<'_>>>;
 
     #[zbus(property)]
-    fn devices(&self) -> Result<Vec<ObjectPath<'_>>>;
+    fn all_devices(&self) -> Result<Vec<ObjectPath<'_>>>;
-
-    // #[zbus(property)]
-    // fn networking_enabled(&self) -> Result<bool>;
-
-    // #[zbus(property)]
-    // fn primary_connection(&self) -> Result<ObjectPath>;
-
-    // #[zbus(property)]
-    // fn primary_connection_type(&self) -> Result<Str>;
-
-    // #[zbus(property)]
-    // fn wireless_enabled(&self) -> Result<bool>;
-}
-
-#[proxy(
-    default_service = "org.freedesktop.NetworkManager",
-    interface = "org.freedesktop.NetworkManager.Connection.Active"
-)]
-pub(super) trait ActiveConnectionDbus {
-    // #[zbus(property)]
-    // fn connection(&self) -> Result<ObjectPath>;
-
-    // #[zbus(property)]
-    // fn default(&self) -> Result<bool>;
-
-    // #[zbus(property)]
-    // fn default6(&self) -> Result<bool>;
 
     #[zbus(property)]
     fn devices(&self) -> Result<Vec<ObjectPath<'_>>>;
-
-    // #[zbus(property)]
-    // fn id(&self) -> Result<Str>;
-
-    #[zbus(property)]
-    fn type_(&self) -> Result<Str<'_>>;
-
-    // #[zbus(property)]
-    // fn uuid(&self) -> Result<Str>;
 }
 
 #[proxy(
@@ -59,19 +23,20 @@ pub(super) trait ActiveConnectionDbus {
     interface = "org.freedesktop.NetworkManager.Device"
 )]
 pub(super) trait DeviceDbus {
-    // #[zbus(property)]
-    // fn active_connection(&self) -> Result<ObjectPath>;
-
     #[zbus(property)]
     fn device_type(&self) -> Result<DeviceType>;
 
+    #[zbus(property)]
+    fn interface(&self) -> Result<Str<'_>>;
+
     #[zbus(property)]
     fn state(&self) -> Result<DeviceState>;
 }
 
+// For reference: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/e1a7d5ac062f4f23ce3a6b33c62e856056161ad8/src/libnm-core-public/nm-dbus-interface.h#L212-L253
 #[derive(Clone, Debug, Eq, Hash, OwnedValue, PartialEq)]
 #[repr(u32)]
-pub(super) enum DeviceType {
+pub enum DeviceType {
     Unknown = 0,
     Ethernet = 1,
     Wifi = 2,
@@ -105,9 +70,10 @@ pub(super) enum DeviceType {
     Hsr = 33,
 }
 
+// For reference: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/e1a7d5ac062f4f23ce3a6b33c62e856056161ad8/src/libnm-core-public/nm-dbus-interface.h#L501-L538
 #[derive(Clone, Debug, OwnedValue, PartialEq)]
 #[repr(u32)]
-pub(super) enum DeviceState {
+pub enum DeviceState {
     Unknown = 0,
     Unmanaged = 10,
     Unavailable = 20,
@@ -122,18 +88,3 @@ pub(super) enum DeviceState {
     Deactivating = 110,
     Failed = 120,
 }
-
-impl DeviceState {
-    pub(super) fn is_enabled(&self) -> bool {
-        !matches!(
-            self,
-            DeviceState::Unknown | DeviceState::Unmanaged | DeviceState::Unavailable,
-        )
-    }
-}
-
-#[derive(Clone, Debug, Eq, Hash, PartialEq)]
-pub(super) struct Device<'l> {
-    pub object_path: ObjectPath<'l>,
-    pub type_: DeviceType,
-}

src/clients/networkmanager/event.rs

@@ -1,2 +1,14 @@
+use crate::clients::networkmanager::dbus::{DeviceState, DeviceType};
+
 #[derive(Debug, Clone)]
-pub enum Event {}
+pub enum Event {
+    DeviceAdded {
+        interface: String,
+        r#type: DeviceType,
+    },
+    DeviceStateChanged {
+        interface: String,
+        r#type: DeviceType,
+        state: DeviceState,
+    },
+}

src/clients/networkmanager/mod.rs

@@ -1,22 +1,17 @@
 use color_eyre::Result;
-use color_eyre::eyre::Error;
+use color_eyre::eyre::Ok;
 use futures_lite::StreamExt;
 use std::collections::HashSet;
 use std::sync::Arc;
-use std::time::Duration;
+use tokio::sync::broadcast;
-use tokio::join;
-use tokio::sync::{RwLock, broadcast};
-use tokio::time::sleep;
-use tokio_stream::StreamMap;
 use zbus::Connection;
-use zbus::proxy::PropertyStream;
+use zbus::zvariant::{ObjectPath, Str};
-use zbus::zvariant::ObjectPath;
 
-use crate::clients::networkmanager::dbus::{DbusProxy, Device, DeviceDbusProxy, DeviceState};
+use crate::clients::networkmanager::dbus::{DbusProxy, DeviceDbusProxy};
 use crate::clients::networkmanager::event::Event;
 use crate::{register_fallible_client, spawn};
 
-mod dbus;
+pub mod dbus;
 pub mod event;
 
 #[derive(Debug)]
@@ -31,103 +26,98 @@ impl Client {
     }
 
     async fn run(&self) -> Result<()> {
-        let dbus_connection = Connection::system().await?;
+        // TODO: Use glib::clone!()
-        let root_object = DbusProxy::new(&dbus_connection).await?;
 
-        let device_state_changes =
+        let tx = self.tx.clone();
-            RwLock::new(StreamMap::<Device, PropertyStream<DeviceState>>::new());
+        spawn(async move {
+            let dbus_connection = Connection::system().await?;
+            let root = DbusProxy::new(&dbus_connection).await?;
 
-        let _ = join!(
+            // All device types get added to this, but not all types emit events
-            // Handles the addition and removal of device objects
+            let mut devices = HashSet::new();
-            async {
-                let mut devices_changes = root_object.receive_devices_changed().await;
-                while let Some(change) = devices_changes.next().await {
-                    println!("here?");
 
-                    let devices = HashSet::from_iter(
+            let mut devices_changes = root.receive_all_devices_changed().await;
-                        device_state_changes
+            while let Some(devices_change) = devices_changes.next().await {
-                            .read()
+                // The new list of devices from dbus, not to be confused with the added devices below
-                            .await
+                let new_devices = HashSet::from_iter(devices_change.get().await?);
-                            .keys()
-                            .map(|device| &device.object_path)
-                            .cloned(),
-                    );
 
-                    // The new list of devices from dbus, not to be confused with the added devices below
+                let added_devices = new_devices.difference(&devices);
-                    let new_devices_vec = change.get().await?;
+                for added_device in added_devices {
-                    let new_devices = HashSet::<ObjectPath>::from_iter(new_devices_vec);
+                    spawn(watch_device(added_device.to_owned(), tx.clone()));
-                    println!("Existing devices: {:?}", devices);
-                    println!("New devices: {:?}", new_devices);
-
-                    let added_devices = new_devices.difference(&devices);
-                    println!("Added devices: {:?}", added_devices);
-                    for added_device in added_devices {
-                        let device_proxy =
-                            DeviceDbusProxy::new(&dbus_connection, added_device).await?;
-                        let device_type = device_proxy.device_type().await?;
-                        let device_state_stream = device_proxy.receive_state_changed().await;
-                        device_state_changes.write().await.insert(
-                            Device {
-                                object_path: added_device.clone(),
-                                type_: device_type.clone(), // TODO: Remove clone when removing println below
-                            },
-                            device_state_stream,
-                        );
-                        println!("Device added: {} type {:?}", added_device, device_type);
-                    }
-
-                    let removed_devices = devices.difference(&new_devices);
-                    println!("Removed devices: {:?}", removed_devices);
-                    for removed_device in removed_devices {
-                        let device_proxy =
-                            DeviceDbusProxy::new(&dbus_connection, removed_device).await?;
-                        let device_type = device_proxy.device_type().await?;
-                        device_state_changes.write().await.remove(&Device {
-                            object_path: removed_device.clone(),
-                            type_: device_type.clone(), // TODO: Remove clone when removing println below
-                        });
-                        println!("Device removed: {} type {:?}", removed_device, device_type);
-                    }
-                }
-                Ok::<(), Error>(())
-            },
-            // Handles changes to device properties
-            async {
-                sleep(Duration::from_secs(5)).await;
-
-                /*
-                Okay so this causes a deadlock, and we should rewrite all of this with spawn() anyway cause join!() is not multithreaded apparently.
-                In order to not leak memory we could have closures for objects that don't exist anymore check this manually and return.
-                */
-                while let Some((device, property)) = device_state_changes.write().await.next().await
-                {
-                    let property = property.get().await?;
-                    println!(
-                        "Device state changed: {} to {:?}",
-                        device.object_path, property
-                    );
                 }
 
-                println!("Prop loop ended");
+                let removed_devices = devices.difference(&new_devices);
+                // TODO: Cook up some way to notify closures for removed devices to exit
 
-                Ok::<(), Error>(())
+                devices = new_devices;
-            },
+            }
-        );
+
+            Ok(())
+        });
 
         Ok(())
     }
 
     pub fn subscribe(&self) -> broadcast::Receiver<Event> {
+        // Maybe we should pass a direct receiver so that the UI module also gets the events from before it was started
         self.tx.subscribe()
     }
 }
 
 pub async fn create_client() -> Result<Arc<Client>> {
-    // TODO: Use spawn here after all, otherwise we block on creation
-
     let client = Arc::new(Client::new().await?);
     client.run().await?;
     Ok(client)
 }
 
+async fn watch_device(device_path: ObjectPath<'_>, tx: broadcast::Sender<Event>) -> Result<()> {
+    let dbus_connection = Connection::system().await?;
+    let device = DeviceDbusProxy::new(&dbus_connection, device_path.to_owned()).await?;
+
+    let interface = device.interface().await?;
+    let device_type = device.device_type().await?;
+    tx.send(Event::DeviceAdded {
+        interface: interface.to_string(),
+        r#type: device_type,
+    })?;
+
+    spawn(watch_device_state(
+        device_path.to_owned(),
+        interface.to_owned(),
+        tx.clone(),
+    ));
+
+    Ok(())
+}
+
+async fn watch_device_state(
+    device_path: ObjectPath<'_>,
+    interface: Str<'_>,
+    tx: broadcast::Sender<Event>,
+) -> Result<()> {
+    let dbus_connection = Connection::system().await?;
+    let device = DeviceDbusProxy::new(&dbus_connection, &device_path).await?;
+    let r#type = device.device_type().await?;
+
+    // Send an event communicating the initial state
+    let state = device.state().await?;
+    tx.send(Event::DeviceStateChanged {
+        interface: interface.to_string(),
+        r#type: r#type.clone(),
+        state,
+    })?;
+
+    let mut state_changes = device.receive_state_changed().await;
+    while let Some(state_change) = state_changes.next().await {
+        let state = state_change.get().await?;
+        tx.send(Event::DeviceStateChanged {
+            interface: interface.to_string(),
+            r#type: r#type.clone(),
+            state,
+        })?;
+    }
+
+    Ok(())
+}
+
 register_fallible_client!(Client, network_manager);

src/macros.rs

@@ -255,14 +255,3 @@ macro_rules! rc_mut {
         std::rc::Rc::new(std::cell::RefCell::new($val))
     };
 }
-
-#[macro_export]
-macro_rules! spawn_blocking_result {
-    ($body:block) => {
-        spawn_blocking(move || {
-            if let Err(error) = (|| -> Result<()> { $body })() {
-                error!("Error in fallible spawned closure: {}", error);
-            }
-        });
-    };
-}

src/modules/networkmanager.rs

@@ -1,17 +1,18 @@
-use color_eyre::Result;
-use futures_lite::StreamExt;
-use gtk::prelude::{ContainerExt, WidgetExt};
-use gtk::{Box as GtkBox, Image, Orientation};
-use serde::Deserialize;
-use tokio::sync::mpsc::Receiver;
-
-use crate::channels::{AsyncSenderExt, BroadcastReceiverExt};
 use crate::clients::networkmanager::Client;
+use crate::clients::networkmanager::dbus::{DeviceState, DeviceType};
 use crate::clients::networkmanager::event::Event;
 use crate::config::CommonConfig;
 use crate::gtk_helpers::IronbarGtkExt;
-use crate::modules::{Module, ModuleInfo, ModuleParts, ModuleUpdateEvent, WidgetContext};
+use crate::image::Provider;
+use crate::modules::{Module, ModuleInfo, ModuleParts, WidgetContext};
 use crate::{module_impl, spawn};
+use color_eyre::{Result, eyre::Ok};
+use glib::spawn_future_local;
+use gtk::prelude::{ContainerExt, WidgetExt};
+use gtk::{Image, Orientation};
+use serde::Deserialize;
+use std::collections::HashMap;
+use tokio::sync::{broadcast, mpsc};
 
 #[derive(Debug, Deserialize, Clone)]
 #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
@@ -27,7 +28,7 @@ const fn default_icon_size() -> i32 {
     24
 }
 
-impl Module<GtkBox> for NetworkManagerModule {
+impl Module<gtk::Box> for NetworkManagerModule {
     type SendMessage = Event;
     type ReceiveMessage = ();
 
@@ -37,19 +38,19 @@ impl Module<GtkBox> for NetworkManagerModule {
         &self,
         _info: &ModuleInfo,
         context: &WidgetContext<Event, ()>,
-        _rx: Receiver<()>,
+        _rx: mpsc::Receiver<()>,
     ) -> Result<()> {
         let client = context.try_client::<Client>()?;
-        // let mut client_signal = client.subscribe().to_stream();
+        // Should we be using context.tx with ModuleUpdateEvent::Update instead?
-        // let widget_transmitter = context.tx.clone();
+        let tx = context.update_tx.clone();
+        spawn(async move {
+            let mut client_rx = client.subscribe();
+            while let Result::Ok(event) = client_rx.recv().await {
+                tx.send(event)?;
+            }
 
-        // spawn(async move {
+            Ok(())
-        // while let Some(state) = client_signal.next().await {
+        });
-        //     widget_transmitter
-        //         .send_expect(ModuleUpdateEvent::Update(state))
-        //         .await;
-        // }
-        // });
 
         Ok(())
     }
@@ -58,37 +59,116 @@ impl Module<GtkBox> for NetworkManagerModule {
         self,
         context: WidgetContext<Event, ()>,
         _info: &ModuleInfo,
-    ) -> Result<ModuleParts<GtkBox>> {
+    ) -> Result<ModuleParts<gtk::Box>> {
-        let container = GtkBox::new(Orientation::Horizontal, 0);
+        let container = gtk::Box::new(Orientation::Horizontal, 0);
 
-        // Wired icon
+        // TODO: Check if passing the widget context in its entirety here is possible
-        let wired_icon = Image::new();
+        // We cannot use recv_glib_async() here because the lifetimes don't work out
-        wired_icon.add_class("icon");
+        spawn_future_local(handle_update_events(
-        wired_icon.add_class("wired-icon");
+            context.subscribe(),
-        container.add(&wired_icon);
+            container.clone(),
-
+            self.icon_size,
-        // Wifi icon
+            context.ironbar.image_provider(),
-        let wifi_icon = Image::new();
+        ));
-        wifi_icon.add_class("icon");
-        wifi_icon.add_class("wifi-icon");
-        container.add(&wifi_icon);
-
-        // Cellular icon
-        let cellular_icon = Image::new();
-        cellular_icon.add_class("icon");
-        cellular_icon.add_class("cellular-icon");
-        container.add(&cellular_icon);
-
-        // VPN icon
-        let vpn_icon = Image::new();
-        vpn_icon.add_class("icon");
-        vpn_icon.add_class("vpn-icon");
-        container.add(&vpn_icon);
-
-        context
-            .subscribe()
-            .recv_glib_async((), move |(), event| async {});
 
         Ok(ModuleParts::new(container, None))
     }
 }
+
+async fn handle_update_events(
+    mut rx: broadcast::Receiver<Event>,
+    container: gtk::Box,
+    icon_size: i32,
+    image_provider: Provider,
+) {
+    let mut icons = HashMap::<String, Image>::new();
+
+    while let Result::Ok(event) = rx.recv().await {
+        println!("NM UI event: {:?}", event);
+
+        match event {
+            Event::DeviceAdded { interface, r#type } => {
+                if !is_supported_device_type(&r#type) {
+                    continue;
+                }
+                let icon = Image::new();
+                icon.add_class("icon");
+                container.add(&icon);
+                icons.insert(interface, icon);
+            }
+            Event::DeviceStateChanged {
+                interface,
+                r#type,
+                state,
+            } => {
+                if !is_supported_device_type(&r#type) {
+                    continue;
+                }
+                let icon = icons
+                    .get(&interface)
+                    .expect("the icon for the interface to be present");
+                let icon_name = get_icon_for_device_state(&r#type, &state);
+                match icon_name {
+                    Some(icon_name) => {
+                        image_provider
+                            .load_into_image_silent(icon_name, icon_size, false, icon)
+                            .await;
+                        icon.show();
+                    }
+                    None => {
+                        icon.hide();
+                    }
+                }
+            }
+        };
+    }
+}
+
+fn is_supported_device_type(r#type: &DeviceType) -> bool {
+    matches!(
+        r#type,
+        DeviceType::Ethernet | DeviceType::Wifi | DeviceType::Tun | DeviceType::Wireguard
+    )
+}
+
+fn get_icon_for_device_state(r#type: &DeviceType, state: &DeviceState) -> Option<&'static str> {
+    match r#type {
+        DeviceType::Ethernet => match state {
+            DeviceState::Unavailable => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::Disconnected => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::Prepare => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::Config => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::NeedAuth => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::IpConfig => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::IpCheck => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::Secondaries => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::Activated => Some("icon:network-wired-symbolic"),
+            DeviceState::Deactivating => Some("icon:network-wired-disconnected-symbolic"),
+            DeviceState::Failed => Some("icon:network-wired-disconnected-symbolic"),
+            _ => None,
+        },
+        DeviceType::Wifi => match state {
+            DeviceState::Unavailable => Some("icon:network-wireless-hardware-disabled-symbolic"),
+            DeviceState::Disconnected => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::Prepare => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::Config => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::NeedAuth => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::IpConfig => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::IpCheck => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::Secondaries => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::Activated => Some("icon:network-wireless-connected-symbolic"),
+            DeviceState::Deactivating => Some("icon:network-wireless-offline-symbolic"),
+            DeviceState::Failed => Some("icon:network-wireless-offline-symbolic"),
+            _ => None,
+        },
+        DeviceType::Tun => match state {
+            DeviceState::Activated => Some("icon:network-vpn-symbolic"),
+            _ => None,
+        },
+        DeviceType::Wireguard => match state {
+            DeviceState::Activated => Some("icon:network-vpn-symbolic"),
+            _ => None,
+        },
+        _ => panic!("Device type should be a supported one"),
+    }
+}