From f557ffdfc70cc5240d5af79d878e60e17cd7790c Mon Sep 17 00:00:00 2001 From: funman300 Date: Sat, 18 Jul 2026 10:46:02 -0700 Subject: [PATCH] niri-scratchpad: refresh patch with cast-leak and unmap-path fixes Review of the destroy-path fix found two Important defects, both now closed: a stashed window whose client died never stopped its screencast (leaking a PipeWire stream/portal session nothing could reclaim), and the unmap path had the identical hole (a stashed window that unmapped was never reinstated into unmapped_windows, wedging it permanently on re-map). Also corrects a factually wrong hook-teardown comment. 117 unit tests and 8/8 live acceptance pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- niri-patches/0001-scratchpad.patch | 154 ++++++++++++++++++++++++----- 1 file changed, 130 insertions(+), 24 deletions(-) diff --git a/niri-patches/0001-scratchpad.patch b/niri-patches/0001-scratchpad.patch index 9fc3ae3..596c6e5 100644 --- a/niri-patches/0001-scratchpad.patch +++ b/niri-patches/0001-scratchpad.patch @@ -56,21 +56,112 @@ index 0aa3cb4f..a5b712e2 100644 /// Toggle (open/close) the Overview. ToggleOverview {}, /// Open the Overview. +diff --git a/src/handlers/compositor.rs b/src/handlers/compositor.rs +index f38b2934..a085ff70 100644 +--- a/src/handlers/compositor.rs ++++ b/src/handlers/compositor.rs +@@ -24,6 +24,7 @@ use crate::layout::{ActivateWindow, AddWindowTarget, LayoutElement as _}; + use crate::niri::{CastTarget, ClientState, LockState, State}; + use crate::utils::transaction::Transaction; + use crate::utils::{is_mapped, send_scale_transform}; ++use crate::window::mapped::MappedId; + use crate::window::{InitialConfigureState, Mapped, ResolvedWindowRules, Unmapped}; + + impl CompositorHandler for State { +@@ -368,6 +369,52 @@ impl CompositorHandler for State { + return; + } + ++ // A window stashed in the scratchpad is in no workspace, so the lookup ++ // above can't see it — but its client still owns the surface and may ++ // unmap it at any time. Handle that here, otherwise the tile would sit ++ // in the stash with a bufferless surface and never be reinstated into ++ // unmapped_windows, so a later re-map could not fire and the window ++ // would be permanently wedged. ++ if !is_mapped(surface) { ++ if let Some(removed) = self.niri.layout.remove_from_scratchpad(surface) { ++ trace!("stashed toplevel got unmapped"); ++ ++ let window = removed.window().window.clone(); ++ // Annotated to pin the inherent Mapped::id() -> MappedId; the ++ // LayoutElement trait also has an id(), returning &Window. ++ let id: MappedId = removed.window().id(); ++ // Drop the Mapped here so its mapped-toplevel pre-commit hook is ++ // torn down before we install the default one. The two are ++ // distinct HookIds (Mapped::drop removes only its own), so this ++ // ordering is tidiness, not correctness. ++ drop(removed); ++ ++ window.on_commit(); ++ ++ self.niri ++ .stop_casts_for_target(CastTarget::Window { id: id.get() }); ++ self.add_default_dmabuf_pre_commit_hook(surface); ++ ++ // Newly-unmapped toplevels must perform the initial ++ // commit-configure sequence afresh. ++ let unmapped = Unmapped::new(window); ++ self.niri.unmapped_windows.insert(surface.clone(), unmapped); ++ ++ // Nothing else to do: a stashed window is in no workspace, so it ++ // can't be the focus and isn't rendered on any output. It is also ++ // absent from window_mru_ui, unless the overlay happened to be ++ // open when it was stashed — that leaves a stale thumbnail, which ++ // the MRU lookups bail on harmlessly. ++ // ++ // A stashed window that commits while still mapped falls through ++ // to the unrecognized-surface trace below. Buffer state is still ++ // handled (on_commit_buffer_handler runs unconditionally), so only ++ // Window-level geometry/popup bookkeeping goes stale, and that ++ // self-heals on the first commit after the window is shown. ++ return; ++ } ++ } ++ + // This is a commit of a non-toplevel root. + } + diff --git a/src/handlers/xdg_shell.rs b/src/handlers/xdg_shell.rs -index 38440c90..a174921e 100644 +index 38440c90..fe16901f 100644 --- a/src/handlers/xdg_shell.rs +++ b/src/handlers/xdg_shell.rs -@@ -834,6 +834,15 @@ impl XdgShellHandler for State { +@@ -47,6 +47,7 @@ use crate::utils::transaction::Transaction; + use crate::utils::{ + get_monotonic_time, output_matches_name, send_scale_transform, update_tiled_state, ResizeEdge, + }; ++use crate::window::mapped::MappedId; + use crate::window::{InitialConfigureState, ResolvedWindowRules, Unmapped, WindowRef}; + + impl XdgShellHandler for State { +@@ -834,6 +835,33 @@ impl XdgShellHandler for State { .find_window_and_output(surface.wl_surface()); let Some((mapped, output)) = win_out else { + // A window whose client died while stashed in the scratchpad is in no + // workspace, so find_window_and_output can't see it; evict it here. -+ if self -+ .niri -+ .layout -+ .remove_from_scratchpad(surface.wl_surface()) -+ { ++ // ++ // Casts must still be stopped: a Cast lives in niri.casting, independent ++ // of the layout, and the render loop just skips ids it can't find — so a ++ // cast of a stashed window would otherwise stay live and frozen forever. ++ // ++ // The rest of the teardown below is genuinely not needed here: ++ // window_mru_ui and layout.focus() are both derived from ++ // layout.workspaces(), so a stashed window is in neither; and there is ++ // nothing on screen to snapshot, animate closed, or redraw. ++ // ++ // We also deliberately skip re-adding the default dmabuf pre-commit hook ++ // that the normal path installs. Mapped::drop removes only its own ++ // mapped-toplevel hook (a distinct HookId), and the surface cannot gain a ++ // new role after toplevel destruction, so nothing will sample it again; ++ // destroyed() removes hooks with `if let Some(..)`, making the absent ++ // entry a no-op rather than an error. ++ if let Some(removed) = self.niri.layout.remove_from_scratchpad(surface.wl_surface()) { ++ // Annotated to pin the inherent Mapped::id() -> MappedId; the ++ // LayoutElement trait also has an id(), returning &Window. ++ let id: MappedId = removed.window().id(); ++ drop(removed); ++ self.niri ++ .stop_casts_for_target(CastTarget::Window { id: id.get() }); + return; + } // I have no idea how this can happen, but I saw it happen once, in a weird interaction @@ -106,7 +197,7 @@ index f1ad4932..91bc8942 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..a90d9e6e 100644 +index 5c4dd639..faf05ea7 100644 --- a/src/layout/mod.rs +++ b/src/layout/mod.rs @@ -32,6 +32,7 @@ @@ -137,7 +228,20 @@ index 5c4dd639..a90d9e6e 100644 pub struct RemovedTile { tile: Tile, /// Width of the column the tile was in. -@@ -699,6 +706,8 @@ impl Layout { +@@ -499,6 +506,12 @@ pub struct RemovedTile { + is_floating: bool, + } + ++impl RemovedTile { ++ pub fn window(&self) -> &W { ++ self.tile.window() ++ } ++} ++ + /// Whether to activate a newly added window. + #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] + pub enum ActivateWindow { +@@ -699,6 +712,8 @@ impl Layout { last_active_workspace_id: HashMap::new(), interactive_move: None, dnd: None, @@ -146,7 +250,7 @@ index 5c4dd639..a90d9e6e 100644 clock, update_render_elements_time: Duration::ZERO, overview_open: false, -@@ -724,6 +733,8 @@ impl Layout { +@@ -724,6 +739,8 @@ impl Layout { last_active_workspace_id: HashMap::new(), interactive_move: None, dnd: None, @@ -155,7 +259,7 @@ index 5c4dd639..a90d9e6e 100644 clock, update_render_elements_time: Duration::ZERO, overview_open: false, -@@ -1114,6 +1125,13 @@ impl Layout { +@@ -1114,6 +1131,13 @@ impl Layout { window: &W::Id, transaction: Transaction, ) -> Option> { @@ -169,7 +273,7 @@ index 5c4dd639..a90d9e6e 100644 if let Some(state) = &self.interactive_move { match state { InteractiveMoveState::Starting { window_id, .. } => { -@@ -1204,6 +1222,106 @@ impl Layout { +@@ -1204,6 +1228,108 @@ impl Layout { None } @@ -184,17 +288,19 @@ index 5c4dd639..a90d9e6e 100644 + // (Task 4) if this window was the one shown. + } + -+ /// Evict a stashed window from the scratchpad by its surface. Returns true -+ /// if one was removed. Needed on the destroy/unmap paths: a window whose -+ /// client dies *while stashed* is in no workspace, so `find_window_and_output` -+ /// (and therefore `remove_window`) never sees it — this is the only thing -+ /// that drops it. A shown scratchpad window lives in a workspace and is -+ /// handled by `remove_window` instead. -+ pub fn remove_from_scratchpad(&mut self, surface: &WlSurface) -> bool { -+ let before = self.scratchpad.len(); -+ self.scratchpad -+ .retain(|rt| !rt.tile.window().is_wl_surface(surface)); -+ self.scratchpad.len() != before ++ /// Evict a stashed window from the scratchpad by its surface, returning the ++ /// tile so the caller can run the rest of the teardown (casts, unmapped ++ /// bookkeeping). Needed on both the destroy and unmap paths: a window that ++ /// dies or unmaps *while stashed* is in no workspace, so ++ /// `find_window_and_output` (and therefore `remove_window`) never sees it — ++ /// this is the only thing that takes it out. A shown scratchpad window lives ++ /// in a workspace and is handled by `remove_window` instead. ++ pub fn remove_from_scratchpad(&mut self, surface: &WlSurface) -> Option> { ++ let idx = self ++ .scratchpad ++ .iter() ++ .position(|rt| rt.tile.window().is_wl_surface(surface))?; ++ self.scratchpad.remove(idx) + } + + /// Show / toggle / cycle the scratchpad. See the state table in the plan. @@ -276,7 +382,7 @@ index 5c4dd639..a90d9e6e 100644 pub fn descendants_added(&mut self, id: &W::Id) -> bool { for ws in self.workspaces_mut() { if ws.descendants_added(id) { -@@ -2444,6 +2562,24 @@ impl Layout { +@@ -2444,6 +2570,24 @@ impl Layout { } }