From 413daa7ba2ea0ebd1424c024fd0566423aaea3f8 Mon Sep 17 00:00:00 2001 From: srdusr <99972264+srdusr@users.noreply.github.com> Date: Fri, 14 Feb 2025 21:36:00 +0200 Subject: Fix workspace switches undoing themselves within milliseconds sync()'s per-tick platform.focus(id) re-assertion (added to keep real Wayland/X11 keyboard focus following core's own bookkeeping) ran unconditionally on every dirty tick, including when nothing about focus had actually changed. focus_window (core) has its own, separate side effect of switching to the focused window's workspace when it differs from the current one - correct when focus genuinely moves to a window on another workspace, but this call was never gated on focus having changed at all: switching workspace via activate_workspace left the still-focused window's own workspace field untouched, so the very next dirty tick's blind re-assertion of that same focus saw a mismatch against the just-changed current_workspace and switched straight back. Confirmed live via temporary core-side logging: two switch_workspace calls a few milliseconds apart, the second one undoing the first every single time, for every workspace switch that didn't also change which window was focused. Gated the re-assertion on the focused id actually changing since the last sync() call. Real focus-follows-real-platform-focus still happens on every genuine change, which is all the original fix needed. --- crates/core/src/manager/workspaces.rs | 9 +------- crates/srdwm/src/main.rs | 42 ++++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 19 deletions(-) (limited to 'crates') diff --git a/crates/core/src/manager/workspaces.rs b/crates/core/src/manager/workspaces.rs index ddc8341..6ffb21b 100644 --- a/crates/core/src/manager/workspaces.rs +++ b/crates/core/src/manager/workspaces.rs @@ -50,14 +50,7 @@ impl WindowManager { /// doesn't need its own separate bookkeeping. pub fn switch_workspace(&mut self, id: WorkspaceId) { let target = if self.auto_back_and_forth && id == self.current_workspace { self.previous_workspace } else { id }; - let exists = self.workspaces.iter().any(|w| w.id == target); - log::warn!( - "SWITCH-WS-DIAG requested_id={id} auto_back_and_forth={} current={} previous={} target={target} exists={exists}", - self.auto_back_and_forth, - self.current_workspace, - self.previous_workspace - ); - if exists && target != self.current_workspace { + if self.workspaces.iter().any(|w| w.id == target) && target != self.current_workspace { self.previous_workspace = self.current_workspace; self.current_workspace = target; } diff --git a/crates/srdwm/src/main.rs b/crates/srdwm/src/main.rs index bb331dc..c6f4f2b 100644 --- a/crates/srdwm/src/main.rs +++ b/crates/srdwm/src/main.rs @@ -1,5 +1,5 @@ use srdwm_config::Engine; -use srdwm_core::{Event, WindowManager}; +use srdwm_core::{Event, WindowId, WindowManager}; use srdwm_platform::{Platform, PlatformKind}; use std::cell::RefCell; use std::path::PathBuf; @@ -355,7 +355,7 @@ fn default_platform_kind() -> PlatformKind { /// is explicitly shown via `Platform::restore` before geometry/decoration /// are pushed, since on X11 `apply_geometry` only reconfigures an existing /// mapping, it doesn't create one. -fn sync(wm: &Rc>, platform: &mut dyn Platform) { +fn sync(wm: &Rc>, platform: &mut dyn Platform, last_synced_focus: &mut Option) { for id in wm.borrow_mut().take_close_requests() { if let Err(e) = platform.close(id) { log::warn!("close({id}) failed: {e}"); @@ -390,14 +390,33 @@ fn sync(wm: &Rc>, platform: &mut dyn Platform) { // `_NET_ACTIVE_WINDOW` never moved - confirmed live: `srd dispatch // focus` on an XWayland window left it at `0x0`. `Platform::focus`'s // own impls now go through the same real-focus-sync path a mouse click - // already uses (see their doc comments), so calling it here every tick - // closes the gap for all of those callers at once. Cheap when nothing - // actually changed - `set_keyboard_focus` early-returns if the target - // surface is already focused. - if let Some(id) = focused { - if let Err(e) = platform.focus(id) { - log::warn!("focus({id}) failed: {e}"); + // already uses (see their doc comments), so calling it here every dirty + // tick closes the gap for all of those callers at once. + // + // Gated on the focused id actually having *changed* since the last + // call, not just "call every time, it's cheap when nothing changed" as + // originally reasoned - that reasoning covered `set_keyboard_focus` + // alone (a real early-return-if-already-focused no-op) but missed that + // `focus_window` (core) has its own side effect of switching to the + // focused window's workspace if it differs from the current one. Live- + // reproduced this session: `srd dispatch activate_workspace` changed + // `current_workspace` correctly, and within the same dirty tick this + // unconditional re-assertion of the still-focused (unchanged, still on + // the *old* workspace) window's focus saw that mismatch and switched + // straight back - every workspace switch with no accompanying focus + // change silently undid itself within milliseconds, confirmed via the + // core diagnostic logging two `switch_workspace` calls a few + // milliseconds apart, the second putting it right back where it + // started. Re-asserting real platform focus still happens on every + // *genuine* focus change, which is everything the comment above this + // one actually needed fixed. + if focused != *last_synced_focus { + if let Some(id) = focused { + if let Err(e) = platform.focus(id) { + log::warn!("focus({id}) failed: {e}"); + } } + *last_synced_focus = focused; } let (visible, hidden) = { let wm = wm.borrow(); @@ -551,7 +570,8 @@ fn main() -> Result<(), Box> { log::debug!("no 'ready' handler registered"); } - sync(&wm, platform.as_mut()); + let mut last_synced_focus: Option = None; + sync(&wm, platform.as_mut(), &mut last_synced_focus); while running.get() { if SHUTDOWN_REQUESTED.load(Ordering::SeqCst) { @@ -621,7 +641,7 @@ fn main() -> Result<(), Box> { } } if dirty { - sync(&wm, platform.as_mut()); + sync(&wm, platform.as_mut(), &mut last_synced_focus); } } -- cgit v1.2.3