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

niri-scratchpad: refresh patch with cast-leak and unmap-path fixes

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
funman300
2026-07-18 10:46:02 -07:00
parent a9ce3a13cf
commit f557ffdfc7
1 changed files with 130 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
niri-patches/0001-scratchpad.patch
+130 -24
View File
@@ -56,21 +56,112 @@ index 0aa3cb4f..a5b712e2 100644
/// Toggle (open/close) the Overview. /// Toggle (open/close) the Overview.
ToggleOverview {}, ToggleOverview {},
/// Open the Overview. /// 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 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 --- a/src/handlers/xdg_shell.rs
+++ b/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()); .find_window_and_output(surface.wl_surface());
let Some((mapped, output)) = win_out else { let Some((mapped, output)) = win_out else {
+ // A window whose client died while stashed in the scratchpad is in no + // 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. + // workspace, so find_window_and_output can't see it; evict it here.
+ if self + //
+ .niri + // Casts must still be stopped: a Cast lives in niri.casting, independent
+ .layout + // of the layout, and the render loop just skips ids it can't find — so a
+ .remove_from_scratchpad(surface.wl_surface()) + // 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; + return;
+ } + }
// I have no idea how this can happen, but I saw it happen once, in a weird interaction // 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.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..a90d9e6e 100644 index 5c4dd639..faf05ea7 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 @@
@@ -137,7 +228,20 @@ index 5c4dd639..a90d9e6e 100644
pub struct RemovedTile<W: LayoutElement> { pub struct RemovedTile<W: LayoutElement> {
tile: Tile<W>, tile: Tile<W>,
/// Width of the column the tile was in. /// Width of the column the tile was in.
@@ -699,6 +706,8 @@ impl<W: LayoutElement> Layout<W> { @@ -499,6 +506,12 @@ pub struct RemovedTile<W: LayoutElement> {
is_floating: bool,
}
+impl<W: LayoutElement> RemovedTile<W> {
+ 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<W: LayoutElement> Layout<W> {
last_active_workspace_id: HashMap::new(), last_active_workspace_id: HashMap::new(),
interactive_move: None, interactive_move: None,
dnd: None, dnd: None,
@@ -146,7 +250,7 @@ index 5c4dd639..a90d9e6e 100644
clock, clock,
update_render_elements_time: Duration::ZERO, update_render_elements_time: Duration::ZERO,
overview_open: false, overview_open: false,
@@ -724,6 +733,8 @@ impl<W: LayoutElement> Layout<W> { @@ -724,6 +739,8 @@ impl<W: LayoutElement> Layout<W> {
last_active_workspace_id: HashMap::new(), last_active_workspace_id: HashMap::new(),
interactive_move: None, interactive_move: None,
dnd: None, dnd: None,
@@ -155,7 +259,7 @@ index 5c4dd639..a90d9e6e 100644
clock, clock,
update_render_elements_time: Duration::ZERO, update_render_elements_time: Duration::ZERO,
overview_open: false, overview_open: false,
@@ -1114,6 +1125,13 @@ impl<W: LayoutElement> Layout<W> { @@ -1114,6 +1131,13 @@ impl<W: LayoutElement> Layout<W> {
window: &W::Id, window: &W::Id,
transaction: Transaction, transaction: Transaction,
) -> Option<RemovedTile<W>> { ) -> Option<RemovedTile<W>> {
@@ -169,7 +273,7 @@ index 5c4dd639..a90d9e6e 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 +1222,106 @@ impl<W: LayoutElement> Layout<W> { @@ -1204,6 +1228,108 @@ impl<W: LayoutElement> Layout<W> {
None None
} }
@@ -184,17 +288,19 @@ index 5c4dd639..a90d9e6e 100644
+ // (Task 4) if this window was the one shown. + // (Task 4) if this window was the one shown.
+ } + }
+ +
+ /// Evict a stashed window from the scratchpad by its surface. Returns true + /// Evict a stashed window from the scratchpad by its surface, returning the
+ /// if one was removed. Needed on the destroy/unmap paths: a window whose + /// tile so the caller can run the rest of the teardown (casts, unmapped
+ /// client dies *while stashed* is in no workspace, so `find_window_and_output` + /// bookkeeping). Needed on both the destroy and unmap paths: a window that
+ /// (and therefore `remove_window`) never sees it — this is the only thing + /// dies or unmaps *while stashed* is in no workspace, so
+ /// that drops it. A shown scratchpad window lives in a workspace and is + /// `find_window_and_output` (and therefore `remove_window`) never sees it —
+ /// handled by `remove_window` instead. + /// this is the only thing that takes it out. A shown scratchpad window lives
+ pub fn remove_from_scratchpad(&mut self, surface: &WlSurface) -> bool { + /// in a workspace and is handled by `remove_window` instead.
+ let before = self.scratchpad.len(); + pub fn remove_from_scratchpad(&mut self, surface: &WlSurface) -> Option<RemovedTile<W>> {
+ self.scratchpad + let idx = self
+ .retain(|rt| !rt.tile.window().is_wl_surface(surface)); + .scratchpad
+ self.scratchpad.len() != before + .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. + /// 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 { 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 +2562,24 @@ impl<W: LayoutElement> Layout<W> { @@ -2444,6 +2570,24 @@ impl<W: LayoutElement> Layout<W> {
} }
} }