Merge branch 'fix/audit-findings' into dev

# Conflicts:
#	assets/icons-needed.txt
#	src/main.rs
#	src/notifications/mod.rs
#	src/notifications/popup.rs

src/notifications/mod.rs

@@ -1,17 +1,29 @@
 pub mod popup;
 
 use std::sync::atomic::{AtomicU32, Ordering};
+use std::time::Duration;
 use tokio::sync::mpsc;
 use zbus::zvariant::OwnedValue;
 
+/// How long a shown notification should stay up before auto-dismissing.
+/// Distinct from `Option<Duration>` mainly for readability at call sites —
+/// `Never` covers both the spec's `expire_timeout == 0` ("never expire")
+/// and a critical-urgency notification with no explicit timeout, which
+/// conventionally shouldn't auto-dismiss either.
+#[derive(Debug, Clone, Copy)]
+pub enum Expire {
+    Never,
+    After(Duration),
+}
+
 pub enum NotifEvent {
     Show {
         id: u32,
         app_name: String,
         summary: String,
         body: String,
-        timeout_ms: u32,
         urgency: Urgency,
+        expire: Expire,
     },
     Close(u32),
 }
@@ -24,6 +36,15 @@ pub enum Urgency {
 }
 
 impl Urgency {
+    /// Spec: hints["urgency"] is a byte, 0=low, 1=normal, 2=critical (default normal).
+    fn from_hint(hint: Option<&OwnedValue>) -> Self {
+        match hint.and_then(|v| u8::try_from(v.clone()).ok()) {
+            Some(0) => Urgency::Low,
+            Some(2) => Urgency::Critical,
+            _ => Urgency::Normal,
+        }
+    }
+
     /// CSS class suffix for `.notification-card.urgency-<kind>`.
     pub fn css_class(self) -> Option<&'static str> {
         match self {
@@ -34,6 +55,27 @@ impl Urgency {
     }
 }
 
+/// Maps a `Notify` call's `expire_timeout` (plus whether the `urgency` hint
+/// was critical) to our internal `Expire`, per the freedesktop notification
+/// spec: `0` always means never expire; a negative value means "server
+/// picks a default" (5s here, except critical notifications, which
+/// conventionally persist); any non-negative value is taken literally.
+/// Pulled out of `NotifServer::notify` so this mapping is unit-testable
+/// without a live D-Bus connection.
+fn compute_expire(expire_timeout: i32, urgency_critical: bool) -> Expire {
+    match expire_timeout {
+        0 => Expire::Never,
+        t if t < 0 => {
+            if urgency_critical {
+                Expire::Never
+            } else {
+                Expire::After(Duration::from_millis(5000))
+            }
+        }
+        t => Expire::After(Duration::from_millis(t as u64)),
+    }
+}
+
 struct NotifServer {
     tx: mpsc::Sender<NotifEvent>,
     next_id: AtomicU32,
@@ -59,17 +101,15 @@ impl NotifServer {
         } else {
             self.next_id.fetch_add(1, Ordering::Relaxed)
         };
-        let timeout_ms = if expire_timeout <= 0 {
-            5000
-        } else {
-            expire_timeout as u32
-        };
-        // Spec: hints["urgency"] is a byte, 0=low, 1=normal, 2=critical (default normal).
-        let urgency = match hints.get("urgency").and_then(|v| u8::try_from(v.clone()).ok()) {
-            Some(0) => Urgency::Low,
-            Some(2) => Urgency::Critical,
-            _ => Urgency::Normal,
-        };
+        // Per spec: 0 means "never expire" — this used to be lumped in
+        // with "-1: let the server pick a default" and coerced to a fixed
+        // 5s, so a sender explicitly asking for a persistent notification
+        // (e.g. a progress/error dialog) got auto-dismissed anyway.
+        // Critical-urgency notifications conventionally persist too, even
+        // when the sender left expire_timeout at the server-default (-1).
+        let urgency = Urgency::from_hint(hints.get("urgency"));
+        let expire = compute_expire(expire_timeout, urgency == Urgency::Critical);
+
         let _ = self
             .tx
             .send(NotifEvent::Show {
@@ -77,8 +117,8 @@ impl NotifServer {
                 app_name: app_name.to_string(),
                 summary: summary.to_string(),
                 body: body.to_string(),
-                timeout_ms,
                 urgency,
+                expire,
             })
             .await;
         id
@@ -104,6 +144,7 @@ impl NotifServer {
 
 pub fn spawn() {
     let (tx, rx) = mpsc::channel(32);
+    let (conn_tx, conn_rx) = tokio::sync::oneshot::channel();
 
     relm4::spawn(async move {
         let server = NotifServer {
@@ -111,7 +152,7 @@ pub fn spawn() {
             next_id: AtomicU32::new(1),
         };
         // Builder failures here would only occur with invalid static strings — safe to unwrap.
-        let _conn = zbus::connection::Builder::session()
+        let conn = zbus::connection::Builder::session()
             .unwrap()
             .name("org.freedesktop.Notifications")
             .unwrap()
@@ -120,8 +161,55 @@ pub fn spawn() {
             .build()
             .await
             .expect("failed to claim org.freedesktop.Notifications on D-Bus session bus");
+        // Hand the connection to popup::run so it can emit `NotificationClosed`
+        // (spec-mandated whenever a notification actually goes away) — the
+        // dismiss decisions all happen over there, not in this interface impl.
+        let _ = conn_tx.send(conn);
         std::future::pending::<()>().await
     });
 
-    relm4::spawn_local(popup::run(rx));
+    relm4::spawn_local(async move {
+        if let Ok(conn) = conn_rx.await {
+            popup::run(rx, conn).await;
+        }
+    });
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn zero_timeout_never_expires_regardless_of_urgency() {
+        assert!(matches!(compute_expire(0, false), Expire::Never));
+        assert!(matches!(compute_expire(0, true), Expire::Never));
+    }
+
+    #[test]
+    fn negative_timeout_defaults_to_five_seconds_for_normal_urgency() {
+        match compute_expire(-1, false) {
+            Expire::After(d) => assert_eq!(d, Duration::from_millis(5000)),
+            Expire::Never => panic!("expected a 5s default, got Never"),
+        }
+    }
+
+    #[test]
+    fn negative_timeout_persists_for_critical_urgency() {
+        assert!(matches!(compute_expire(-1, true), Expire::Never));
+    }
+
+    #[test]
+    fn positive_timeout_is_taken_literally() {
+        match compute_expire(1500, false) {
+            Expire::After(d) => assert_eq!(d, Duration::from_millis(1500)),
+            Expire::Never => panic!("expected 1500ms, got Never"),
+        }
+        // Even for critical urgency, an explicit positive timeout is honored
+        // rather than overridden to Never — "critical persists" is only the
+        // *default* when the sender didn't specify one.
+        match compute_expire(1500, true) {
+            Expire::After(d) => assert_eq!(d, Duration::from_millis(1500)),
+            Expire::Never => panic!("expected 1500ms, got Never"),
+        }
+    }
 }