funman300/dotfiles
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

niri-scratchpad: pull shown window to focused output; don't drop tiles

Browse Source 
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<W>> 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 <noreply@anthropic.com>
This commit is contained in:
funman300
2026-07-18 16:47:06 -07:00
parent 640deb397e
commit d0fef1478d
2 changed files with 129 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
niri-patches/0001-scratchpad.patch
+125 -13
View File
@@ -249,7 +249,7 @@ index f1ad4932..87b5ea7a 100644
self.niri.layout.toggle_overview(); self.niri.layout.toggle_overview();
self.niri.queue_redraw_all(); self.niri.queue_redraw_all();
diff --git a/src/layout/mod.rs b/src/layout/mod.rs 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 --- a/src/layout/mod.rs
+++ b/src/layout/mod.rs +++ b/src/layout/mod.rs
@@ -32,6 +32,7 @@ @@ -32,6 +32,7 @@
@@ -325,7 +325,7 @@ index 5c4dd639..faf05ea7 100644
if let Some(state) = &self.interactive_move { if let Some(state) = &self.interactive_move {
match state { match state {
InteractiveMoveState::Starting { window_id, .. } => { InteractiveMoveState::Starting { window_id, .. } => {
@@ -1204,6 +1228,108 @@ impl<W: LayoutElement> Layout<W> { @@ -1204,6 +1228,164 @@ impl<W: LayoutElement> Layout<W> {
None None
} }
@@ -373,9 +373,30 @@ index 5c4dd639..faf05ea7 100644
+ } + }
+ self.scratchpad_shown = None; + self.scratchpad_shown = None;
+ } else { + } else {
+ // shown + not focused -> focus it in place. `activate_window` walks + // shown + not focused -> bring it to the monitor you are on, then
+ // every monitor/workspace looking for the window and focuses it + // focus it. `activate_window` alone would move *focus* to whichever
+ // without moving it, which is exactly what we want here. + // 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); + self.activate_window(&id);
+ } + }
+ return; + return;
@@ -400,21 +421,55 @@ index 5c4dd639..faf05ea7 100644
+ return; + return;
+ } + }
+ }; + };
+ self.add_removed_tile_floating(ws_id, removed); + match self.add_removed_tile_floating(ws_id, removed) {
+ self.scratchpad_shown = Some(id); + 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<usize> {
+ 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<usize> {
+ 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 + /// 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`. + /// the floating layer. Mirrors the re-add pattern in `move_to_workspace`.
+ fn add_removed_tile_floating(&mut self, ws_id: WorkspaceId, removed: RemovedTile<W>) { + ///
+ /// 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<W>,
+ ) -> Result<(), RemovedTile<W>> {
+ let MonitorSet::Normal { monitors, .. } = &mut self.monitor_set else { + let MonitorSet::Normal { monitors, .. } = &mut self.monitor_set else {
+ return; + return Err(removed);
+ }; + };
+ let Some(mon) = monitors + let Some(mon) = monitors
+ .iter_mut() + .iter_mut()
+ .find(|mon| mon.workspaces.iter().any(|ws| ws.id() == ws_id)) + .find(|mon| mon.workspaces.iter().any(|ws| ws.id() == ws_id))
+ else { + else {
+ return; + return Err(removed);
+ }; + };
+ +
+ mon.add_tile( + mon.add_tile(
@@ -429,12 +484,13 @@ index 5c4dd639..faf05ea7 100644
+ removed.is_full_width, + removed.is_full_width,
+ true, + true,
+ ); + );
+ Ok(())
+ } + }
+ +
pub fn descendants_added(&mut self, id: &W::Id) -> bool { pub fn descendants_added(&mut self, id: &W::Id) -> bool {
for ws in self.workspaces_mut() { for ws in self.workspaces_mut() {
if ws.descendants_added(id) { if ws.descendants_added(id) {
@@ -2444,6 +2570,24 @@ impl<W: LayoutElement> Layout<W> { @@ -2444,6 +2626,24 @@ impl<W: LayoutElement> Layout<W> {
} }
} }
@@ -460,7 +516,7 @@ index 5c4dd639..faf05ea7 100644
let mut seen_workspace_name = Vec::<String>::new(); let mut seen_workspace_name = Vec::<String>::new();
diff --git a/src/layout/tests.rs b/src/layout/tests.rs 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 --- a/src/layout/tests.rs
+++ b/src/layout/tests.rs +++ b/src/layout/tests.rs
@@ -753,6 +753,8 @@ enum Op { @@ -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); check_ops_with_options(options, ops);
} }
} }
@@ -655,3 +711,59 @@ index 31265dd9..9037ff08 100644
+ "shown window should be on the still-focused origin output" + "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"
+ );
+}
pkg/niri-scratchpad/tests/acceptance.py
+4 -2
View File
@@ -306,8 +306,10 @@ def main():
os.unlink(cfg.name) os.unlink(cfg.name)
# #8 multi-monitor cannot be exercised nested (single winit output). # #8 multi-monitor cannot be exercised nested (single winit output).
print(" [SKIP] 8 multi-monitor: needs a physical 2nd output; " print(" [SKIP] 8 multi-monitor: needs a physical 2nd output. Covered by the "
"code uses active_output (correct by construction).") "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) passed = sum(1 for _, ok, _ in results if ok)
failed = sum(1 for _, ok, _ in results if not ok) failed = sum(1 for _, ok, _ in results if not ok)