From d0fef1478d1ae72c333daa4bca8933af879065c3 Mon Sep 17 00:00:00 2001 From: funman300 Date: Sat, 18 Jul 2026 16:47:06 -0700 Subject: [PATCH] niri-scratchpad: pull shown window to focused output; don't drop tiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes found reviewing the paths the acceptance suite couldn't cover. Show-while-shown-elsewhere used activate_window, which sets active_monitor_idx and so dragged *focus* to whichever output the window was parked on. A scratchpad should come to you: if the window is on another monitor, remove and re-add it floating on the active workspace; if it is already on the active monitor, focus in place as before. add_removed_tile_floating dropped the RemovedTile on both of its early returns (NoOutputs, monitor-not-found), destroying the window while its client was still alive and leaving it wedged with no surface anywhere. Both were unreachable via scratchpad_show's guards, but it is a window-loss footgun for the next caller. It now returns Result<(), RemovedTile> so callers can stash the tile instead. Adds two layout tests (they simulate outputs, so multi-monitor is covered without a second physical display) and corrects the acceptance suite's test-8 skip message, which claimed the code used active_output — it never did; it uses active_workspace. cargo test --lib scratchpad: 10/10. acceptance.py: 8 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 --- niri-patches/0001-scratchpad.patch | 138 +++++++++++++++++++++--- pkg/niri-scratchpad/tests/acceptance.py | 6 +- 2 files changed, 129 insertions(+), 15 deletions(-) diff --git a/niri-patches/0001-scratchpad.patch b/niri-patches/0001-scratchpad.patch index ebf4d9a..b02bd8a 100644 --- a/niri-patches/0001-scratchpad.patch +++ b/niri-patches/0001-scratchpad.patch @@ -249,7 +249,7 @@ index f1ad4932..87b5ea7a 100644 self.niri.layout.toggle_overview(); self.niri.queue_redraw_all(); diff --git a/src/layout/mod.rs b/src/layout/mod.rs -index 5c4dd639..faf05ea7 100644 +index 5c4dd639..8696fcd9 100644 --- a/src/layout/mod.rs +++ b/src/layout/mod.rs @@ -32,6 +32,7 @@ @@ -325,7 +325,7 @@ index 5c4dd639..faf05ea7 100644 if let Some(state) = &self.interactive_move { match state { InteractiveMoveState::Starting { window_id, .. } => { -@@ -1204,6 +1228,108 @@ impl Layout { +@@ -1204,6 +1228,164 @@ impl Layout { None } @@ -373,9 +373,30 @@ index 5c4dd639..faf05ea7 100644 + } + self.scratchpad_shown = None; + } else { -+ // shown + not focused -> focus it in place. `activate_window` walks -+ // every monitor/workspace looking for the window and focuses it -+ // without moving it, which is exactly what we want here. ++ // shown + not focused -> bring it to the monitor you are on, then ++ // focus it. `activate_window` alone would move *focus* to whichever ++ // monitor the window is parked on (it sets active_monitor_idx), ++ // dragging you across outputs; a scratchpad should come to you. ++ // Same monitor: nothing to move, just focus. ++ if self.monitor_idx_of_window(&id) != self.active_monitor_idx() { ++ if let Some(mut removed) = self.remove_window(&id, Transaction::new()) { ++ // remove_window cleared scratchpad_shown; re-set it below ++ // once the tile is placed. ++ removed.tile.stop_move_animations(); ++ ++ let Some(ws_id) = self.active_workspace().map(|ws| ws.id()) else { ++ self.scratchpad.push_front(removed); // no active ws ++ return; ++ }; ++ ++ match self.add_removed_tile_floating(ws_id, removed) { ++ Ok(()) => self.scratchpad_shown = Some(id), ++ // Could not place it; stash rather than drop it. ++ Err(tile) => self.scratchpad.push_front(tile), ++ } ++ return; ++ } ++ } + self.activate_window(&id); + } + return; @@ -400,21 +421,55 @@ index 5c4dd639..faf05ea7 100644 + return; + } + }; -+ self.add_removed_tile_floating(ws_id, removed); -+ self.scratchpad_shown = Some(id); ++ match self.add_removed_tile_floating(ws_id, removed) { ++ Ok(()) => self.scratchpad_shown = Some(id), ++ // Could not place it; stash rather than drop it. ++ Err(tile) => self.scratchpad.push_front(tile), ++ } ++ } ++ ++ /// Index of the monitor holding `window`, or None if it is not in the layout ++ /// (which includes every window stashed in the scratchpad). ++ fn monitor_idx_of_window(&self, window: &W::Id) -> Option { ++ let MonitorSet::Normal { monitors, .. } = &self.monitor_set else { ++ return None; ++ }; ++ monitors.iter().position(|mon| { ++ mon.workspaces ++ .iter() ++ .any(|ws| ws.windows().any(|win| win.id() == window)) ++ }) ++ } ++ ++ /// Index of the currently active monitor, if there are any outputs. ++ fn active_monitor_idx(&self) -> Option { ++ match &self.monitor_set { ++ MonitorSet::Normal { ++ active_monitor_idx, .. ++ } => Some(*active_monitor_idx), ++ _ => None, ++ } + } + + /// Re-add a previously-removed tile to the workspace `ws_id`, forcing it onto + /// the floating layer. Mirrors the re-add pattern in `move_to_workspace`. -+ fn add_removed_tile_floating(&mut self, ws_id: WorkspaceId, removed: RemovedTile) { ++ /// ++ /// Returns the tile back in `Err` when it could not be placed, so the caller ++ /// can stash it. Dropping it here would destroy the window while its client ++ /// is still alive, leaving it permanently wedged with no surface anywhere. ++ fn add_removed_tile_floating( ++ &mut self, ++ ws_id: WorkspaceId, ++ removed: RemovedTile, ++ ) -> Result<(), RemovedTile> { + let MonitorSet::Normal { monitors, .. } = &mut self.monitor_set else { -+ return; ++ return Err(removed); + }; + let Some(mon) = monitors + .iter_mut() + .find(|mon| mon.workspaces.iter().any(|ws| ws.id() == ws_id)) + else { -+ return; ++ return Err(removed); + }; + + mon.add_tile( @@ -429,12 +484,13 @@ index 5c4dd639..faf05ea7 100644 + removed.is_full_width, + true, + ); ++ Ok(()) + } + pub fn descendants_added(&mut self, id: &W::Id) -> bool { for ws in self.workspaces_mut() { if ws.descendants_added(id) { -@@ -2444,6 +2570,24 @@ impl Layout { +@@ -2444,6 +2626,24 @@ impl Layout { } } @@ -460,7 +516,7 @@ index 5c4dd639..faf05ea7 100644 let mut seen_workspace_name = Vec::::new(); diff --git a/src/layout/tests.rs b/src/layout/tests.rs -index 31265dd9..9037ff08 100644 +index 31265dd9..4c22a115 100644 --- a/src/layout/tests.rs +++ b/src/layout/tests.rs @@ -753,6 +753,8 @@ enum Op { @@ -485,7 +541,7 @@ index 31265dd9..9037ff08 100644 } } } -@@ -3929,3 +3937,169 @@ proptest! { +@@ -3929,3 +3937,225 @@ proptest! { check_ops_with_options(options, ops); } } @@ -655,3 +711,59 @@ index 31265dd9..9037ff08 100644 + "shown window should be on the still-focused origin output" + ); +} ++ ++#[test] ++fn scratchpad_show_pulls_already_shown_window_to_focused_output() { ++ // Show on output 1, walk to output 2, then press show again. The window must ++ // come to output 2 — not drag focus back to output 1, which is what plain ++ // `activate_window` would do. ++ let ops = [ ++ Op::AddOutput(1), ++ Op::AddOutput(2), ++ Op::FocusOutput(1), ++ Op::AddWindow { params: TestWindowParams::new(0) }, // lands on output 1 ++ Op::ScratchpadStash(0), ++ Op::ScratchpadShow, // shown on output 1 ++ Op::FocusOutput(2), // shown, but no longer focused ++ Op::ScratchpadShow, ++ ]; ++ let layout = check_ops(ops); ++ ++ let MonitorSet::Normal { monitors, active_monitor_idx, .. } = &layout.monitor_set else { ++ unreachable!() ++ }; ++ ++ assert_eq!(*active_monitor_idx, 1, "focus must stay on output 2"); ++ assert!( ++ monitors[1].windows().any(|w| *w.id() == 0), ++ "window should have been pulled to output 2" ++ ); ++ assert!( ++ !monitors[0].windows().any(|w| *w.id() == 0), ++ "window must not be left behind on output 1" ++ ); ++ assert_eq!(layout.scratchpad_shown, Some(0), "still tracked as shown"); ++ assert!(layout.scratchpad.is_empty(), "must not be re-stashed"); ++} ++ ++#[test] ++fn scratchpad_show_focuses_in_place_on_same_output() { ++ // Same output, shown but not focused: focus it without moving it. ++ let ops = [ ++ Op::AddOutput(1), ++ Op::AddWindow { params: TestWindowParams::new(0) }, ++ Op::ScratchpadStash(0), ++ Op::ScratchpadShow, // shown and focused ++ Op::AddWindow { params: TestWindowParams::new(1) }, // steals focus ++ Op::ScratchpadShow, ++ ]; ++ let layout = check_ops(ops); ++ ++ assert_eq!(layout.scratchpad_shown, Some(0)); ++ assert!(layout.scratchpad.is_empty(), "must not be re-stashed"); ++ assert_eq!( ++ layout.focus().map(|w| *w.id()), ++ Some(0), ++ "shown-but-unfocused window should get focus, not be hidden" ++ ); ++} diff --git a/pkg/niri-scratchpad/tests/acceptance.py b/pkg/niri-scratchpad/tests/acceptance.py index 066475e..565da0b 100644 --- a/pkg/niri-scratchpad/tests/acceptance.py +++ b/pkg/niri-scratchpad/tests/acceptance.py @@ -306,8 +306,10 @@ def main(): os.unlink(cfg.name) # #8 multi-monitor cannot be exercised nested (single winit output). - print(" [SKIP] 8 multi-monitor: needs a physical 2nd output; " - "code uses active_output (correct by construction).") + print(" [SKIP] 8 multi-monitor: needs a physical 2nd output. Covered by the " + "layout unit tests instead (scratchpad_shows_on_the_focused_output, " + "scratchpad_show_pulls_already_shown_window_to_focused_output), which " + "simulate outputs: `cargo test --lib scratchpad`.") passed = sum(1 for _, ok, _ in results if ok) failed = sum(1 for _, ok, _ in results if not ok)