From 7340e2beff57df4b3bb016dff1eb6fb3c2218dd3 Mon Sep 17 00:00:00 2001 From: Ian Douglas Scott Date: Thu, 13 Mar 2025 12:50:02 -0700 Subject: [PATCH] Improve handling of XDG activation tokens in shell Requires https://github.com/Smithay/smithay/pull/1676. This changes two things: * `Workspace::is_empty` no longer checks if there are activation tokens, but a separate `Workspace::can_auto_remove` checks if the workspace is empty and has no activation tokens. - When we add workspace pinning, that can also be checked there. * `Workspace` no longer contains a `pending_tokens` list that is updated on `refresh`. Instead, `can_auto_remove` takes the xdg activation state as an argument. Since `Workspace::refresh` normally is run for focused workspaces, this fixes allowing non-focused workspaces to be removed when an activation token expires. It seems generally good to avoid tracking the activation tokens in two places, and this is probably more efficient than needing to refresh in more places. By splitting this, we still don't remove an empty workspace if it has a pending activation token, but we also don't add an empty workspace for an activation token. This mitigates the confusing behavior with activation tokens that aren't used, but having to wait a few seconds in some cases before a workspace is removed is still a little confusing. (We probably want `cosmic-term` and `cosmic-workspace` to either consume the activation tokens they are passed, or not be passed tokens when started by keybinding?) Fixes https://github.com/pop-os/cosmic-comp/issues/1099. --- src/input/actions.rs | 1 - src/input/mod.rs | 1 - src/shell/element/stack.rs | 3 - src/shell/element/window.rs | 1 - src/shell/grabs/menu/default.rs | 1 - src/shell/mod.rs | 79 ++++++++++++-------------- src/shell/workspace.rs | 30 +++++++--- src/state.rs | 6 +- src/wayland/handlers/xdg_activation.rs | 2 - src/wayland/handlers/xdg_shell/mod.rs | 3 +- src/xwayland.rs | 3 +- 11 files changed, 61 insertions(+), 69 deletions(-) diff --git a/src/input/actions.rs b/src/input/actions.rs index 64c064d4..3b219ed8 100644 --- a/src/input/actions.rs +++ b/src/input/actions.rs @@ -993,7 +993,6 @@ impl State { let output = shell.seats.last_active().active_output(); let workspace = shell.active_space_mut(&output).unwrap(); - workspace.pending_tokens.insert(token.clone()); let handle = workspace.handle; std::mem::drop(shell); data.user_data diff --git a/src/input/mod.rs b/src/input/mod.rs index 6ca44fd4..7956416b 100644 --- a/src/input/mod.rs +++ b/src/input/mod.rs @@ -725,7 +725,6 @@ impl State { false, &state.common.config, &state.common.event_loop_handle, - &state.common.xdg_activation_state, false, ); drop(shell); diff --git a/src/shell/element/stack.rs b/src/shell/element/stack.rs index 6872d36e..725a9d28 100644 --- a/src/shell/element/stack.rs +++ b/src/shell/element/stack.rs @@ -705,7 +705,6 @@ impl CosmicStack { true, &state.common.config, &state.common.event_loop_handle, - &state.common.xdg_activation_state, false, ); if let Some((grab, focus)) = res { @@ -848,7 +847,6 @@ impl Program for CosmicStackInternal { false, &state.common.config, &state.common.event_loop_handle, - &state.common.xdg_activation_state, false, ); if let Some((grab, focus)) = res { @@ -1493,7 +1491,6 @@ impl PointerTarget for CosmicStack { true, &state.common.config, &state.common.event_loop_handle, - &state.common.xdg_activation_state, false, ); if let Some((grab, focus)) = res { diff --git a/src/shell/element/window.rs b/src/shell/element/window.rs index fe1ad3c4..837c4939 100644 --- a/src/shell/element/window.rs +++ b/src/shell/element/window.rs @@ -439,7 +439,6 @@ impl Program for CosmicWindowInternal { false, &state.common.config, &state.common.event_loop_handle, - &state.common.xdg_activation_state, false, ); if let Some((grab, focus)) = res { diff --git a/src/shell/grabs/menu/default.rs b/src/shell/grabs/menu/default.rs index 4d821d25..40df674e 100644 --- a/src/shell/grabs/menu/default.rs +++ b/src/shell/grabs/menu/default.rs @@ -256,7 +256,6 @@ pub fn window_items( false, &state.common.config, &state.common.event_loop_handle, - &state.common.xdg_activation_state, false, ); diff --git a/src/shell/mod.rs b/src/shell/mod.rs index 0cee9910..b01e9e90 100644 --- a/src/shell/mod.rs +++ b/src/shell/mod.rs @@ -110,6 +110,7 @@ const GESTURE_MAX_LENGTH: f64 = 150.0; const GESTURE_POSITION_THRESHOLD: f64 = 0.5; const GESTURE_VELOCITY_THRESHOLD: f64 = 0.02; const MOVE_GRAB_Y_OFFSET: f64 = 16.; +const ACTIVATION_TOKEN_EXPIRE_TIME: Duration = Duration::from_secs(5); #[derive(Debug, Clone)] pub enum Trigger { @@ -532,7 +533,7 @@ impl WorkspaceSet { self.output = new_output.clone(); } - fn refresh(&mut self, xdg_activation_state: &XdgActivationState) { + fn refresh(&mut self) { if let Some((_, start)) = self.previously_active { match start { WorkspaceDelta::Shortcut(st) => { @@ -551,7 +552,7 @@ impl WorkspaceSet { _ => {} } } else { - self.workspaces[self.active].refresh(xdg_activation_state); + self.workspaces[self.active].refresh(); } self.sticky_layer.refresh(); } @@ -575,7 +576,11 @@ impl WorkspaceSet { self.workspaces.push(workspace); } - fn ensure_last_empty(&mut self, state: &mut WorkspaceUpdateGuard) { + fn ensure_last_empty( + &mut self, + state: &mut WorkspaceUpdateGuard, + xdg_activation_state: &XdgActivationState, + ) { // add empty at the end, if necessary if self.workspaces.last().map_or(true, |last| !last.is_empty()) { self.add_empty_workspace(state); @@ -590,7 +595,7 @@ impl WorkspaceSet { .map(|(i, workspace)| { let previous_is_empty = i > 0 && self.workspaces.get(i - 1).map_or(false, |w| w.is_empty()); - let keep = if workspace.is_empty() { + let keep = if workspace.can_auto_remove(xdg_activation_state) { // Keep empty workspace if it's active, or it's the last workspace, // and the previous worspace is not both active and empty. i == self.active @@ -656,7 +661,6 @@ impl Workspaces { &mut self, output: &Output, workspace_state: &mut WorkspaceUpdateGuard<'_, State>, - xdg_activation_state: &XdgActivationState, ) { if self.sets.contains_key(output) { return; @@ -723,7 +727,7 @@ impl Workspaces { } for (i, workspace) in set.workspaces.iter_mut().enumerate() { workspace.set_output(output); - workspace.refresh(xdg_activation_state); + workspace.refresh(); workspace_set_idx(workspace_state, i as u8 + 1, set.idx, &workspace.handle); if i == set.active { workspace_state.add_workspace_state(&workspace.handle, WState::Active); @@ -768,7 +772,7 @@ impl Workspaces { let new_set = self.sets.get_mut(&new_output).unwrap(); let workspace_group = new_set.group; for (i, mut workspace) in set.workspaces.into_iter().enumerate() { - if workspace.is_empty() { + if workspace.can_auto_remove(xdg_activation_state) { workspace_state.remove_workspace(workspace.handle); } else { // update workspace protocol state @@ -776,7 +780,7 @@ impl Workspaces { // update mapping workspace.set_output(&new_output); - workspace.refresh(xdg_activation_state); + workspace.refresh(); new_set.workspaces.push(workspace); // If workspace was active, and the new set's active workspace is empty, make this workspace @@ -835,7 +839,6 @@ impl Workspaces { to: &Output, handle: &WorkspaceHandle, workspace_state: &mut WorkspaceUpdateGuard<'_, State>, - xdg_activation_state: &XdgActivationState, ) { if !self.sets.contains_key(to) { return; @@ -848,7 +851,7 @@ impl Workspaces { let new_set = self.sets.get_mut(to).unwrap(); move_workspace_to_group(&mut workspace, &new_set.group, workspace_state); workspace.set_output(to); - workspace.refresh(xdg_activation_state); + workspace.refresh(); new_set.workspaces.insert(new_set.active + 1, workspace) } } @@ -970,7 +973,10 @@ impl Workspaces { let mut active = self.sets[0].active; let mut keep = vec![true; len]; for i in 0..len { - let has_windows = self.sets.values().any(|s| !s.workspaces[i].is_empty()); + let has_windows = self + .sets + .values() + .any(|s| !s.workspaces[i].can_auto_remove(xdg_activation_state)); if !has_windows && i != active && i != len - 1 { for workspace in self.sets.values().map(|s| &s.workspaces[i]) { @@ -1004,13 +1010,13 @@ impl Workspaces { } WorkspaceMode::OutputBound => { for set in self.sets.values_mut() { - set.ensure_last_empty(workspace_state); + set.ensure_last_empty(workspace_state, xdg_activation_state); } } } for set in self.sets.values_mut() { - set.refresh(xdg_activation_state) + set.refresh() } } @@ -1098,7 +1104,7 @@ impl Workspaces { ) } - pub fn set_theme(&mut self, theme: cosmic::Theme, xdg_activation_state: &XdgActivationState) { + pub fn set_theme(&mut self, theme: cosmic::Theme) { for (_, s) in &mut self.sets { s.theme = theme.clone(); @@ -1117,10 +1123,10 @@ impl Workspaces { } } - self.force_redraw(xdg_activation_state); + self.force_redraw(); } - pub fn force_redraw(&mut self, xdg_activation_state: &XdgActivationState) { + pub fn force_redraw(&mut self) { for (_, s) in &mut self.sets { s.sticky_layer.mapped().for_each(|m| { m.force_redraw(); @@ -1132,7 +1138,7 @@ impl Workspaces { m.force_redraw(); }); - w.refresh(xdg_activation_state); + w.refresh(); w.dirty.store(true, Ordering::Relaxed); w.recalculate(); } @@ -1189,11 +1195,9 @@ pub struct InvalidWorkspaceIndex; impl Common { pub fn add_output(&mut self, output: &Output) { let mut shell = self.shell.write().unwrap(); - shell.workspaces.add_output( - output, - &mut self.workspace_state.update(), - &self.xdg_activation_state, - ); + shell + .workspaces + .add_output(output, &mut self.workspace_state.update()); if let Some(state) = shell.zoom_state.as_ref() { output.user_data().insert_if_missing_threadsafe(|| { @@ -1233,13 +1237,9 @@ impl Common { } let mut shell = self.shell.write().unwrap(); - shell.workspaces.migrate_workspace( - from, - to, - handle, - &mut self.workspace_state.update(), - &self.xdg_activation_state, - ); + shell + .workspaces + .migrate_workspace(from, to, handle, &mut self.workspace_state.update()); std::mem::drop(shell); self.refresh(); // fixes index of moved workspace @@ -1273,9 +1273,8 @@ impl Common { #[profiling::function] pub fn refresh(&mut self) { - self.xdg_activation_state.retain_tokens(|_, data| { - Instant::now().duration_since(data.timestamp) < Duration::from_secs(5) - }); + self.xdg_activation_state + .retain_tokens(|_, data| data.timestamp.elapsed() < ACTIVATION_TOKEN_EXPIRE_TIME); self.shell.write().unwrap().refresh( &self.xdg_activation_state, &mut self.workspace_state.update(), @@ -1658,13 +1657,9 @@ impl Shell { } } - pub fn refresh_active_space( - &mut self, - output: &Output, - xdg_activation_state: &XdgActivationState, - ) { + pub fn refresh_active_space(&mut self, output: &Output) { if let Some(w) = self.workspaces.active_mut(output) { - w.refresh(xdg_activation_state) + w.refresh() } } @@ -2919,7 +2914,6 @@ impl Shell { move_out_of_stack: bool, config: &Config, evlh: &LoopHandle<'static, State>, - xdg_activation_state: &XdgActivationState, client_initiated: bool, ) -> Option<(MoveGrab, Focus)> { let serial = serial.into(); @@ -3094,7 +3088,7 @@ impl Shell { self.workspaces .space_for_handle_mut(&workspace_handle) .unwrap() - .refresh(xdg_activation_state); + .refresh(); } mapped.set_activate(true); @@ -3931,7 +3925,7 @@ impl Shell { *container = toolkit; drop(container); self.refresh(xdg_activation_state, workspace_state); - self.workspaces.force_redraw(xdg_activation_state); + self.workspaces.force_redraw(); } } @@ -3943,8 +3937,7 @@ impl Shell { ) { self.theme = theme.clone(); self.refresh(xdg_activation_state, workspace_state); - self.workspaces - .set_theme(theme.clone(), xdg_activation_state); + self.workspaces.set_theme(theme.clone()); } pub fn theme(&self) -> &cosmic::Theme { diff --git a/src/shell/workspace.rs b/src/shell/workspace.rs index 1d63fbff..bfbd672a 100644 --- a/src/shell/workspace.rs +++ b/src/shell/workspace.rs @@ -1,3 +1,4 @@ +use crate::wayland::handlers::xdg_activation::ActivationContext; use crate::{ backend::render::{ element::{AsGlowRenderer, FromGlesError}, @@ -42,11 +43,11 @@ use smithay::{ wayland::{ compositor::{add_blocker, Blocker, BlockerState}, seat::WaylandFocus, - xdg_activation::{XdgActivationState, XdgActivationToken}, + xdg_activation::XdgActivationState, }, }; use std::{ - collections::{HashMap, HashSet, VecDeque}, + collections::{HashMap, VecDeque}, sync::{ atomic::{AtomicBool, Ordering}, Arc, @@ -85,7 +86,6 @@ pub struct Workspace { pub focus_stack: FocusStacks, pub screencopy: ScreencopySessions, pub output_stack: VecDeque, - pub pending_tokens: HashSet, pub(super) backdrop_id: Id, pub dirty: AtomicBool, } @@ -258,14 +258,13 @@ impl Workspace { queue.push_back(output_name); queue }, - pending_tokens: HashSet::new(), backdrop_id: Id::new(), dirty: AtomicBool::new(false), } } #[profiling::function] - pub fn refresh(&mut self, xdg_activation_state: &XdgActivationState) { + pub fn refresh(&mut self) { // TODO: `Option::take_if` once stabilitized if self.fullscreen.as_ref().is_some_and(|w| !w.alive()) { let _ = self.fullscreen.take(); @@ -273,9 +272,25 @@ impl Workspace { self.floating_layer.refresh(); self.tiling_layer.refresh(); + } - self.pending_tokens - .retain(|token| xdg_activation_state.data_for_token(token).is_some()); + fn has_activation_token(&self, xdg_activation_state: &XdgActivationState) -> bool { + xdg_activation_state.tokens().any(|(_, data)| { + if let ActivationContext::Workspace(handle) = + data.user_data.get::().unwrap() + { + *handle == self.handle + && data.timestamp.elapsed() < super::ACTIVATION_TOKEN_EXPIRE_TIME + } else { + false + } + }) + } + + // Auto-removal of workspaces is allowed if empty, unless blocked by an + // unused and unexpired activation token. + pub fn can_auto_remove(&self, xdg_activation_state: &XdgActivationState) -> bool { + self.is_empty() && !self.has_activation_token(xdg_activation_state) } pub fn refresh_focus_stack(&mut self) { @@ -1071,7 +1086,6 @@ impl Workspace { self.floating_layer.mapped().next().is_none() && self.tiling_layer.mapped().next().is_none() && self.minimized_windows.is_empty() - && self.pending_tokens.is_empty() } pub fn is_fullscreen(&self, mapped: &CosmicMapped) -> bool { diff --git a/src/state.rs b/src/state.rs index 50e016cf..7bccb412 100644 --- a/src/state.rs +++ b/src/state.rs @@ -361,11 +361,7 @@ impl BackendData { }); match final_config.enabled { - OutputState::Enabled => { - shell - .workspaces - .add_output(&output, workspace_state, xdg_activation_state) - } + OutputState::Enabled => shell.workspaces.add_output(&output, workspace_state), _ => { let shell = &mut *shell; shell.workspaces.remove_output( diff --git a/src/wayland/handlers/xdg_activation.rs b/src/wayland/handlers/xdg_activation.rs index 2dc70d2f..10bc14ca 100644 --- a/src/wayland/handlers/xdg_activation.rs +++ b/src/wayland/handlers/xdg_activation.rs @@ -45,7 +45,6 @@ impl XdgActivationHandler for State { let output = seat.active_output(); let mut shell = self.common.shell.write().unwrap(); let workspace = shell.active_space_mut(&output).unwrap(); - workspace.pending_tokens.insert(token.clone()); let handle = workspace.handle; data.user_data .insert_if_missing(move || ActivationContext::Workspace(handle)); @@ -89,7 +88,6 @@ impl XdgActivationHandler for State { let output = seat.active_output(); let mut shell = self.common.shell.write().unwrap(); let workspace = shell.active_space_mut(&output).unwrap(); - workspace.pending_tokens.insert(token.clone()); let handle = workspace.handle; data.user_data .insert_if_missing(move || ActivationContext::Workspace(handle)); diff --git a/src/wayland/handlers/xdg_shell/mod.rs b/src/wayland/handlers/xdg_shell/mod.rs index c8695e22..056e48fd 100644 --- a/src/wayland/handlers/xdg_shell/mod.rs +++ b/src/wayland/handlers/xdg_shell/mod.rs @@ -174,7 +174,6 @@ impl XdgShellHandler for State { false, &self.common.config, &self.common.event_loop_handle, - &self.common.xdg_activation_state, true, ) { std::mem::drop(shell); @@ -421,7 +420,7 @@ impl XdgShellHandler for State { .visible_output_for_surface(surface.wl_surface()) .cloned(); if let Some(output) = output.as_ref() { - shell.refresh_active_space(output, &self.common.xdg_activation_state); + shell.refresh_active_space(output); } // animations might be unblocked now diff --git a/src/xwayland.rs b/src/xwayland.rs index fd146aae..93b0e4b1 100644 --- a/src/xwayland.rs +++ b/src/xwayland.rs @@ -430,7 +430,7 @@ impl XwmHandler for State { shell.outputs().cloned().collect::>() }; for output in outputs.iter() { - shell.refresh_active_space(output, &self.common.xdg_activation_state); + shell.refresh_active_space(output); } for output in outputs.into_iter() { @@ -593,7 +593,6 @@ impl XwmHandler for State { false, &self.common.config, &self.common.event_loop_handle, - &self.common.xdg_activation_state, true, ) { std::mem::drop(shell);