fix(launcher): item state changes not handled correctly

This completely rewrites the item open state handling code (again) in a more logical way that should prevent incorrect states, and removes some locking issues.

src/modules/launcher/item.rs

@@ -1,5 +1,6 @@
 use crate::collection::Collection;
 use crate::icon::{find_desktop_file, get_icon};
+use crate::modules::launcher::open_state::OpenState;
 use crate::modules::launcher::popup::Popup;
 use crate::modules::launcher::FocusEvent;
 use crate::sway::SwayNode;
@@ -9,7 +10,7 @@ use gtk::prelude::*;
 use gtk::{Button, IconTheme, Image};
 use std::process::{Command, Stdio};
 use std::rc::Rc;
-use std::sync::{Arc, Mutex, RwLock};
+use std::sync::{Arc, RwLock};
 use tokio::spawn;
 use tokio::sync::mpsc;
 use tracing::error;
@@ -18,7 +19,7 @@ use tracing::error;
 pub struct LauncherItem {
     pub app_id: String,
     pub favorite: bool,
-    pub windows: Rc<Mutex<Collection<i32, LauncherWindow>>>,
+    pub windows: Rc<RwLock<Collection<i32, LauncherWindow>>>,
     pub state: Arc<RwLock<State>>,
     pub button: Button,
 }
@@ -27,38 +28,7 @@ pub struct LauncherItem {
 pub struct LauncherWindow {
     pub con_id: i32,
     pub name: Option<String>,
-}
+    pub open_state: OpenState,
-
-#[derive(Debug, Clone, Eq, PartialEq)]
-pub enum OpenState {
-    Closed,
-    Open,
-    Focused,
-    Urgent,
-}
-
-impl OpenState {
-    pub const fn from_node(node: &SwayNode) -> Self {
-        if node.focused {
-            Self::Urgent
-        } else if node.urgent {
-            Self::Focused
-        } else {
-            Self::Open
-        }
-    }
-
-    pub fn highest_of(a: &Self, b: &Self) -> Self {
-        if a == &Self::Urgent || b == &Self::Urgent {
-            Self::Urgent
-        } else if a == &Self::Focused || b == &Self::Focused {
-            Self::Focused
-        } else if a == &Self::Open || b == &Self::Open {
-            Self::Open
-        } else {
-            Self::Closed
-        }
-    }
 }
 
 #[derive(Debug, Clone)]
@@ -89,7 +59,7 @@ impl LauncherItem {
         let item = Self {
             app_id,
             favorite,
-            windows: Rc::new(Mutex::new(Collection::new())),
+            windows: Rc::new(RwLock::new(Collection::new())),
             state: Arc::new(RwLock::new(state)),
             button,
         };
@@ -107,6 +77,7 @@ impl LauncherItem {
             LauncherWindow {
                 con_id: node.id,
                 name: node.name.clone(),
+                open_state: OpenState::from_node(node),
             },
         ));
 
@@ -118,7 +89,7 @@ impl LauncherItem {
         let item = Self {
             app_id: node.get_id().to_string(),
             favorite: false,
-            windows: Rc::new(Mutex::new(windows)),
+            windows: Rc::new(RwLock::new(windows)),
             state: Arc::new(RwLock::new(state)),
             button,
         };
@@ -130,7 +101,10 @@ impl LauncherItem {
     fn configure_button(&self, config: &ButtonConfig) {
         let button = &self.button;
 
-        let windows = self.windows.lock().expect("Failed to get lock on windows");
+        let windows = self
+            .windows
+            .read()
+            .expect("Failed to get read lock on windows");
 
         let name = if windows.len() == 1 {
             windows
@@ -163,7 +137,7 @@ impl LauncherItem {
 
         button.connect_clicked(move |_| {
             let state = state.read().expect("Failed to get read lock on state");
-            if state.open_state != OpenState::Closed {
+            if state.open_state.is_open() {
                 focus_tx.try_send(()).expect("Failed to send focus event");
             } else {
                 // attempt to find desktop file and launch
@@ -215,7 +189,7 @@ impl LauncherItem {
         let tx_hover = config.tx.clone();
 
         button.connect_enter_notify_event(move |button, _| {
-            let windows = windows.lock().expect("Failed to get lock on windows");
+            let windows = windows.read().expect("Failed to get read lock on windows");
             if windows.len() > 1 {
                 popup.set_windows(windows.as_slice(), &tx_hover);
                 popup.show(button);
@@ -266,22 +240,53 @@ impl LauncherItem {
             style.remove_class("favorite");
         }
 
-        if state.open_state == OpenState::Open {
+        if state.open_state.is_open() {
             style.add_class("open");
         } else {
             style.remove_class("open");
         }
 
-        if state.open_state == OpenState::Focused {
+        if state.open_state.is_focused() {
             style.add_class("focused");
         } else {
             style.remove_class("focused");
         }
 
-        if state.open_state == OpenState::Urgent {
+        if state.open_state.is_urgent() {
             style.add_class("urgent");
         } else {
             style.remove_class("urgent");
         }
     }
+
+    /// Sets the open state for a specific window on the item
+    /// and updates the item state based on all its windows.
+    pub fn set_window_open_state(&self, window_id: i32, new_state: OpenState, state: &mut State) {
+        let mut windows = self
+            .windows
+            .write()
+            .expect("Failed to get write lock on windows");
+
+        let window = windows.iter_mut().find(|w| w.con_id == window_id);
+        if let Some(window) = window {
+            window.open_state = new_state;
+
+            state.open_state =
+                OpenState::merge_states(windows.iter().map(|w| &w.open_state).collect());
+        }
+    }
+
+    /// Sets the open state on the item and all its windows.
+    /// This overrides the existing open states.
+    pub fn set_open_state(&self, new_state: OpenState, state: &mut State) {
+        state.open_state = new_state;
+        let mut windows = self
+            .windows
+            .write()
+            .expect("Failed to get write lock on windows");
+
+        windows
+            .iter_mut()
+            .for_each(|window| window.open_state = new_state);
+    }
 }

src/modules/launcher/mod.rs

@@ -1,8 +1,10 @@
 mod item;
+mod open_state;
 mod popup;
 
 use crate::collection::Collection;
-use crate::modules::launcher::item::{ButtonConfig, LauncherItem, LauncherWindow, OpenState};
+use crate::modules::launcher::item::{ButtonConfig, LauncherItem, LauncherWindow};
+use crate::modules::launcher::open_state::OpenState;
 use crate::modules::launcher::popup::Popup;
 use crate::modules::{Module, ModuleInfo};
 use crate::sway::{get_client, SwayNode};
@@ -14,6 +16,7 @@ use std::rc::Rc;
 use tokio::spawn;
 use tokio::sync::mpsc;
 use tokio::task::spawn_blocking;
+use tracing::debug;
 
 #[derive(Debug, Deserialize, Clone)]
 pub struct LauncherModule {
@@ -67,31 +70,37 @@ impl Launcher {
     /// Adds a new window to the launcher.
     /// This gets added to an existing group
     /// if an instance of the program is already open.
-    fn add_window(&mut self, window: SwayNode) {
+    fn add_window(&mut self, node: SwayNode) {
-        let id = window.get_id().to_string();
+        let id = node.get_id().to_string();
+
+        debug!("Adding window with ID {}", id);
 
         if let Some(item) = self.items.get_mut(&id) {
             let mut state = item
                 .state
                 .write()
                 .expect("Failed to get write lock on state");
-            let new_open_state = OpenState::from_node(&window);
+            let new_open_state = OpenState::from_node(&node);
-            state.open_state = OpenState::highest_of(&state.open_state, &new_open_state);
+            state.open_state = OpenState::merge_states(vec![&state.open_state, &new_open_state]);
-            state.is_xwayland = window.is_xwayland();
+            state.is_xwayland = node.is_xwayland();
 
             item.update_button_classes(&state);
 
-            let mut windows = item.windows.lock().expect("Failed to get lock on windows");
+            let mut windows = item
+                .windows
+                .write()
+                .expect("Failed to get write lock on windows");
 
             windows.insert(
-                window.id,
+                node.id,
                 LauncherWindow {
-                    con_id: window.id,
+                    con_id: node.id,
-                    name: window.name,
+                    name: node.name,
+                    open_state: new_open_state,
                 },
             );
         } else {
-            let item = LauncherItem::from_node(&window, &self.button_config);
+            let item = LauncherItem::from_node(&node, &self.button_config);
 
             self.container.add(&item.button);
             self.items.insert(id, item);
@@ -104,11 +113,15 @@ impl Launcher {
     fn remove_window(&mut self, window: &SwayNode) {
         let id = window.get_id().to_string();
 
+        debug!("Removing window with ID {}", id);
+
         let item = self.items.get_mut(&id);
 
         let remove = if let Some(item) = item {
             let windows = Rc::clone(&item.windows);
-            let mut windows = windows.lock().expect("Failed to get lock on windows");
+            let mut windows = windows
+                .write()
+                .expect("Failed to get write lock on windows");
 
             windows.remove(&window.id);
 
@@ -135,24 +148,33 @@ impl Launcher {
         }
     }
 
-    fn set_window_focused(&mut self, window: &SwayNode) {
+    /// Unfocuses the currently focused window
-        let id = window.get_id().to_string();
+    /// and focuses the newly focused one.
+    fn set_window_focused(&mut self, node: &SwayNode) {
+        let id = node.get_id().to_string();
 
-        let currently_focused = self.items.iter_mut().find(|item| {
+        debug!("Setting window with ID {} focused", id);
+
+        let prev_focused = self.items.iter_mut().find(|item| {
             item.state
                 .read()
                 .expect("Failed to get read lock on state")
                 .open_state
-                == OpenState::Focused
+                .is_focused()
         });
 
-        if let Some(currently_focused) = currently_focused {
+        if let Some(prev_focused) = prev_focused {
-            let mut state = currently_focused
+            let mut state = prev_focused
                 .state
                 .write()
                 .expect("Failed to get write lock on state");
-            state.open_state = OpenState::Open;
+
-            currently_focused.update_button_classes(&state);
+            // if a window from the same item took focus,
+            // we don't need to unfocus the item.
+            if prev_focused.app_id != id {
+                prev_focused.set_open_state(OpenState::open(), &mut state);
+                prev_focused.update_button_classes(&state);
+            }
         }
 
         let item = self.items.get_mut(&id);
@@ -161,17 +183,23 @@ impl Launcher {
                 .state
                 .write()
                 .expect("Failed to get write lock on state");
-            state.open_state = OpenState::Focused;
+            item.set_window_open_state(node.id, OpenState::focused(), &mut state);
             item.update_button_classes(&state);
         }
     }
 
+    /// Updates the window title for the given node.
     fn set_window_title(&mut self, window: SwayNode) {
         let id = window.get_id().to_string();
         let item = self.items.get_mut(&id);
 
+        debug!("Updating title for window with ID {}", id);
+
         if let (Some(item), Some(name)) = (item, window.name) {
-            let mut windows = item.windows.lock().expect("Failed to get lock on windows");
+            let mut windows = item
+                .windows
+                .write()
+                .expect("Failed to get write lock on windows");
             if windows.len() == 1 {
                 item.set_title(&name, &self.button_config);
             } else if let Some(window) = windows.get_mut(&window.id) {
@@ -184,17 +212,23 @@ impl Launcher {
         }
     }
 
-    fn set_window_urgent(&mut self, window: &SwayNode) {
+    /// Updates the window urgency based on the given node.
-        let id = window.get_id().to_string();
+    fn set_window_urgent(&mut self, node: &SwayNode) {
+        let id = node.get_id().to_string();
         let item = self.items.get_mut(&id);
 
+        debug!(
+            "Setting urgency to {} for window with ID {}",
+            node.urgent, id
+        );
+
         if let Some(item) = item {
             let mut state = item
                 .state
                 .write()
                 .expect("Failed to get write lock on state");
-            state.open_state =
+
-                OpenState::highest_of(&state.open_state, &OpenState::from_node(window));
+            item.set_window_open_state(node.id, OpenState::urgent(node.urgent), &mut state);
             item.update_button_classes(&state);
         }
     }

src/modules/launcher/open_state.rs

@@ -0,0 +1,74 @@
+use crate::sway::SwayNode;
+
+/// Open state for a launcher item, or item window.
+#[derive(Debug, Clone, Eq, PartialEq, Copy)]
+pub enum OpenState {
+    Closed,
+    Open { focused: bool, urgent: bool },
+}
+
+impl OpenState {
+    /// Creates from `SwayNode`
+    pub const fn from_node(node: &SwayNode) -> Self {
+        Self::Open {
+            focused: node.focused,
+            urgent: node.urgent,
+        }
+    }
+
+    /// Creates open without focused/urgent
+    pub const fn open() -> Self {
+        Self::Open {
+            focused: false,
+            urgent: false,
+        }
+    }
+
+    /// Creates open with focused
+    pub const fn focused() -> Self {
+        Self::Open {
+            focused: true,
+            urgent: false,
+        }
+    }
+
+    /// Creates open with urgent
+    pub const fn urgent(urgent: bool) -> Self {
+        Self::Open {
+            focused: false,
+            urgent,
+        }
+    }
+
+    /// Checks if open
+    pub fn is_open(self) -> bool {
+        self != Self::Closed
+    }
+
+    /// Checks if open with focus
+    pub const fn is_focused(self) -> bool {
+        matches!(self, Self::Open { focused: true, .. })
+    }
+
+    /// check if open with urgent
+    pub const fn is_urgent(self) -> bool {
+        matches!(self, Self::Open { urgent: true, .. })
+    }
+
+    /// Merges states together to produce a single state.
+    /// This is effectively an OR operation,
+    /// so sets state to open and flags to true if any state is open
+    /// or any instance of the flag is true.
+    pub fn merge_states(states: Vec<&Self>) -> Self {
+        states.iter().fold(Self::Closed, |merged, current| {
+            if merged.is_open() || current.is_open() {
+                Self::Open {
+                    focused: merged.is_focused() || current.is_focused(),
+                    urgent: merged.is_urgent() || current.is_urgent(),
+                }
+            } else {
+                Self::Closed
+            }
+        })
+    }
+}