Various improvements

crates/eww/Cargo.toml

@@ -36,6 +36,7 @@ gdkx11 = { version = "0.17", optional = true }
 x11rb = { version = "0.11.1", features = ["randr"], optional = true }
 
 zbus = { version = "3.7.0", default-features = false, features = ["tokio"] }
+ordered-stream = "0.2.0"
 
 anyhow.workspace  = true
 bincode.workspace = true

crates/eww/src/widgets/systray.rs

@@ -1,5 +1,5 @@
 use gtk::{cairo::Surface, gdk::ffi::gdk_cairo_surface_create_from_pixbuf, prelude::*};
-use notifier_host::{self, export::ordered_stream::OrderedStreamExt};
+use notifier_host;
 
 // DBus state shared between systray instances, to avoid creating too many connections etc.
 struct DBusSession {
@@ -12,7 +12,6 @@ async fn dbus_session() -> zbus::Result<&'static DBusSession> {
     static DBUS_STATE: tokio::sync::OnceCell<DBusSession> = tokio::sync::OnceCell::const_new();
     DBUS_STATE
         .get_or_try_init(|| async {
-            // TODO error handling?
             let con = zbus::Connection::session().await?;
             notifier_host::Watcher::new().attach_to(&con).await?;
 

crates/notifier_host/src/host.rs

@@ -26,7 +26,9 @@ pub trait Host {
 ///
 /// You still need to call [`run_host`] to have the instance of [`Host`] be notified of new and
 /// removed items.
-pub async fn register_as_host(con: &zbus::Connection) -> zbus::Result<(zbus::names::WellKnownName<'static>, proxy::StatusNotifierWatcherProxy<'static>)> {
+pub async fn register_as_host(
+    con: &zbus::Connection,
+) -> zbus::Result<(zbus::names::WellKnownName<'static>, proxy::StatusNotifierWatcherProxy<'static>)> {
     let snw = proxy::StatusNotifierWatcherProxy::new(con).await?;
 
     // get a well-known name
@@ -62,16 +64,15 @@ pub async fn register_as_host(con: &zbus::Connection) -> zbus::Result<(zbus::nam
 /// This async function runs forever, and only returns if it gets an error! As such, it is
 /// recommended to call this via something like `tokio::spawn` that runs this in the
 /// background.
-pub async fn run_host(host: &mut dyn Host, snw: &proxy::StatusNotifierWatcherProxy<'static>) -> zbus::Error
-{
+pub async fn run_host(host: &mut dyn Host, snw: &proxy::StatusNotifierWatcherProxy<'static>) -> zbus::Error {
     // Replacement for ? operator since we're not returning a Result.
     macro_rules! try_ {
         ($e:expr) => {
             match $e {
                 Ok(x) => x,
                 Err(e) => return e,
             }
-        }
+        };
     }
 
     enum ItemEvent {

crates/notifier_host/src/icon.rs

@@ -3,7 +3,7 @@ use crate::*;
 use gtk::{self, prelude::*};
 
 #[derive(thiserror::Error, Debug)]
-pub enum IconError {
+enum IconError {
     #[error("while fetching icon name: {0}")]
     DBusIconName(#[source] zbus::Error),
     #[error("while fetching icon theme path: {0}")]

crates/notifier_host/src/item.rs

@@ -2,11 +2,9 @@ use crate::*;
 
 use gtk::{self, prelude::*};
 
-/// Recognised values of org.freedesktop.StatusNotifierItem.Status
+/// Recognised values of [`org.freedesktop.StatusNotifierItem.Status`].
 ///
-/// See
-/// <https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/#org.freedesktop.statusnotifieritem.status>
-/// for details.
+/// [`org.freedesktop.StatusNotifierItem.Status`]: https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/#org.freedesktop.statusnotifieritem.status
 #[derive(Debug, Clone, Copy)]
 pub enum Status {
     /// The item doesn't convey important information to the user, it can be considered an "idle"
@@ -37,35 +35,37 @@ impl std::str::FromStr for Status {
     }
 }
 
-/// Split a sevice name e.g. `:1.50:/org/ayatana/NotificationItem/nm_applet` into the address and
-/// path.
+/// A StatusNotifierItem (SNI).
 ///
-/// Original logic from <https://github.com/oknozor/stray/blob/main/stray/src/notifier_watcher/notifier_address.rs>
-fn split_service_name(service: &str) -> zbus::Result<(String, String)> {
-    if let Some((addr, path)) = service.split_once('/') {
-        Ok((addr.to_owned(), format!("/{}", path)))
-    } else if service.contains(':') {
-        // TODO why?
-        let addr = service.split(':').nth(1);
-        // Some StatusNotifierItems will not return an object path, in that case we fallback
-        // to the default path.
-        if let Some(addr) = addr {
-            Ok((addr.to_owned(), "/StatusNotifierItem".to_owned()))
-        } else {
-            Err(zbus::Error::Address(service.to_owned()))
-        }
-    } else {
-        Err(zbus::Error::Address(service.to_owned()))
-    }
-}
-
+/// At the moment, this does not wrap much of the SNI's properties and methods. As such, you should
+/// directly access the `sni` member as needed for functionalty that is not provided.
 pub struct Item {
+    /// The StatusNotifierItem that is wrapped by this instance.
     pub sni: proxy::StatusNotifierItemProxy<'static>,
 }
 
 impl Item {
-    pub async fn from_address(con: &zbus::Connection, addr: &str) -> zbus::Result<Self> {
-        let (addr, path) = split_service_name(addr)?;
+    /// Create an instance from the service's address.
+    ///
+    /// The format of `addr` is `{bus}{object_path}` (e.g.
+    /// `:1.50/org/ayatana/NotificationItem/nm_applet`), which is the format that is used for
+    /// StatusNotifierWatcher's [RegisteredStatusNotifierItems property][rsni]).
+    ///
+    /// [rsni]: https://freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierWatcher/#registeredstatusnotifieritems
+    pub async fn from_address(con: &zbus::Connection, service: &str) -> zbus::Result<Self> {
+        let (addr, path) = {
+            // Based on <https://github.com/oknozor/stray/blob/main/stray/src/notifier_watcher/notifier_address.rs>
+            //
+            // TODO is the service name format actually documented anywhere?
+            if let Some((addr, path)) = service.split_once('/') {
+                (addr.to_owned(), format!("/{}", path))
+            } else if service.starts_with(':') {
+                (service[0..6].to_owned(), names::ITEM_OBJECT.to_owned())
+            } else {
+                return Err(zbus::Error::Address(service.to_owned()));
+            }
+        };
+
         let sni = proxy::StatusNotifierItemProxy::builder(con).destination(addr)?.path(path)?.build().await?;
 
         Ok(Item { sni })
@@ -80,13 +80,17 @@ impl Item {
         }
     }
 
+    /// Get the menu of this item.
     pub async fn menu(&self) -> zbus::Result<gtk::Menu> {
-        // TODO better handling if menu() method doesn't exist
+        // TODO document what this returns if there is no menu.
         let menu = dbusmenu_gtk3::Menu::new(self.sni.destination(), &self.sni.menu().await?);
         Ok(menu.upcast())
     }
 
+    /// Get the current icon.
     pub async fn icon(&self, size: i32, scale: i32) -> Option<gtk::gdk_pixbuf::Pixbuf> {
+        // TODO explain what size and scale mean here
+
         // see icon.rs
         load_icon_from_sni(&self.sni, size, scale).await
     }

crates/notifier_host/src/lib.rs

@@ -25,7 +25,7 @@
 //!
 //! The actual tray implements the [`Host`] trait to be notified of when items (called
 //! `StatusNotifierItem` in the spec and represented by [`Item`]) appear and disappear, then calls
-//! [`run_host_forever`] to run the DBus side of the protocol.
+//! [`run_host`] to run the DBus side of the protocol.
 //!
 //! If there are multiple trays running on the system, there can be multiple `StatusNotifierHost`s,
 //! but only one `StatusNotifierWatcher` (usually from whatever tray was started first).
@@ -44,6 +44,9 @@ pub use item::*;
 mod watcher;
 pub use watcher::*;
 
-pub mod export {
-    pub use zbus::export::ordered_stream;
+pub(crate) mod names {
+    pub const WATCHER_BUS: &str = "org.kde.StatusNotifierWatcher";
+    pub const WATCHER_OBJECT: &str = "/StatusNotifierWatcher";
+
+    pub const ITEM_OBJECT: &str = "/StatusNotifierItem";
 }

crates/notifier_host/src/watcher.rs

@@ -1,12 +1,8 @@
-use crate::*;
-
+use crate::names;
 use zbus::{dbus_interface, export::ordered_stream::OrderedStreamExt, Interface};
 
-pub const WATCHER_BUS_NAME: &str = "org.kde.StatusNotifierWatcher";
-pub const WATCHER_OBJECT_NAME: &str = "/StatusNotifierWatcher";
-
 /// An instance of [`org.kde.StatusNotifierWatcher`]. It only tracks what tray items and trays
-/// exist, and doesn't have any logic for displaying items (for that, see [`Host`]).
+/// exist, and doesn't have any logic for displaying items (for that, see [`Host`][`crate::Host`]).
 ///
 /// While this is usually run alongside the tray, it can also be used standalone.
 ///
@@ -15,8 +11,8 @@ pub const WATCHER_OBJECT_NAME: &str = "/StatusNotifierWatcher";
 pub struct Watcher {
     tasks: tokio::task::JoinSet<()>,
 
-    // NOTE Intentionally using std::sync::Mutex instead of tokio's async mutex, since we don't
-    // need to hold the mutex across an await.
+    // Intentionally using std::sync::Mutex instead of tokio's async mutex, since we don't need to
+    // hold the mutex across an await.
     //
     // See <https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use>
     hosts: std::sync::Arc<std::sync::Mutex<std::collections::HashSet<String>>>,
@@ -60,7 +56,7 @@ impl Watcher {
             let ctxt = ctxt.to_owned();
             let con = con.to_owned();
             async move {
-                if let Err(e) = wait_for_service_exit(con.clone(), service.as_ref().into()).await {
+                if let Err(e) = wait_for_service_exit(&con, service.as_ref().into()).await {
                     log::error!("failed to wait for service exit: {}", e);
                 }
                 log::info!("lost host: {}", service);
@@ -133,7 +129,7 @@ impl Watcher {
             let ctxt = ctxt.to_owned();
             let con = con.to_owned();
             async move {
-                if let Err(e) = wait_for_service_exit(con.clone(), service.as_ref()).await {
+                if let Err(e) = wait_for_service_exit(&con, service.as_ref()).await {
                     log::error!("failed to wait for service exit: {}", e);
                 }
                 println!("gone item: {}", &item);
@@ -185,18 +181,18 @@ impl Watcher {
         Default::default()
     }
 
-    /// Attach and run the Watcher on a connection.
+    /// Attach and run the Watcher (in the background) on a connection.
     pub async fn attach_to(self, con: &zbus::Connection) -> zbus::Result<()> {
-        if !con.object_server().at(WATCHER_OBJECT_NAME, self).await? {
+        if !con.object_server().at(names::WATCHER_OBJECT, self).await? {
             return Err(zbus::Error::Failure(format!(
                 "Object already exists at {} on this connection -- is StatusNotifierWatcher already running?",
-                WATCHER_OBJECT_NAME
+                names::WATCHER_OBJECT
             )));
         }
 
         // not AllowReplacement, not ReplaceExisting, not DoNotQueue
         let flags: [zbus::fdo::RequestNameFlags; 0] = [];
-        match con.request_name_with_flags(WATCHER_BUS_NAME, flags.into_iter().collect()).await {
+        match con.request_name_with_flags(names::WATCHER_BUS, flags.into_iter().collect()).await {
             Ok(zbus::fdo::RequestNameReply::PrimaryOwner) => Ok(()),
             Ok(_) | Err(zbus::Error::NameTaken) => Ok(()), // defer to existing
             Err(e) => Err(e),
@@ -215,7 +211,7 @@ impl Watcher {
         .await
     }
 
-    /// Equivalen to `registered_status_notifier_items_invalidate`, but without requiring `self`.
+    /// Equivalent to `registered_status_notifier_items_invalidate`, but without requiring `self`.
     async fn registered_status_notifier_items_refresh(ctxt: &zbus::SignalContext<'_>) -> zbus::Result<()> {
         zbus::fdo::Properties::properties_changed(
             ctxt,
@@ -231,20 +227,24 @@ impl Watcher {
 /// name](https://dbus2.github.io/zbus/concepts.html#bus-name--service-name) and the [object
 /// path](https://dbus2.github.io/zbus/concepts.html#objects-and-object-paths) within the
 /// connection.
+///
+/// The freedesktop.org specification has the format of this be just the bus name, however some
+/// status items pass non-conforming values. One common one is just the object path.
 async fn parse_service<'a>(
     service: &'a str,
     hdr: zbus::MessageHeader<'_>,
     con: &zbus::Connection,
 ) -> zbus::fdo::Result<(zbus::names::UniqueName<'static>, &'a str)> {
     if service.starts_with('/') {
-        // they sent us just the object path :(
+        // they sent us just the object path
         if let Some(sender) = hdr.sender()? {
             Ok((sender.to_owned(), service))
         } else {
             log::warn!("unknown sender");
             Err(zbus::fdo::Error::InvalidArgs("Unknown bus address".into()))
         }
     } else {
+        // TODO explain what we're doing here
         let busname: zbus::names::BusName = match service.try_into() {
             Ok(x) => x,
             Err(e) => {
@@ -254,11 +254,11 @@ async fn parse_service<'a>(
         };
 
         if let zbus::names::BusName::Unique(unique) = busname {
-            Ok((unique.to_owned(), "/StatusNotifierItem"))
+            Ok((unique.to_owned(), names::ITEM_OBJECT))
         } else {
             let dbus = zbus::fdo::DBusProxy::new(con).await?;
             match dbus.get_name_owner(busname).await {
-                Ok(owner) => Ok((owner.into_inner(), "/StatusNotifierItem")),
+                Ok(owner) => Ok((owner.into_inner(), names::ITEM_OBJECT)),
                 Err(e) => {
                     log::warn!("failed to get owner of {:?}: {}", service, e);
                     Err(e)
@@ -268,14 +268,13 @@ async fn parse_service<'a>(
     }
 }
 
-/// Wait for a DBus service to exit
-async fn wait_for_service_exit(connection: zbus::Connection, service: zbus::names::BusName<'_>) -> zbus::fdo::Result<()> {
-    // TODO do we also want to catch when the object disappears?
-
-    let dbus = zbus::fdo::DBusProxy::new(&connection).await?;
+/// Wait for a DBus service to disappear
+async fn wait_for_service_exit(con: &zbus::Connection, service: zbus::names::BusName<'_>) -> zbus::fdo::Result<()> {
+    let dbus = zbus::fdo::DBusProxy::new(con).await?;
     let mut owner_changes = dbus.receive_name_owner_changed_with_args(&[(0, &service)]).await?;
 
     if !dbus.name_has_owner(service.as_ref()).await? {
+        // service has already disappeared
         return Ok(());
     }