From 57bde3d30443d073927808b415053b9c363df0c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:28:47 +0200 Subject: [PATCH] fix(overlays): destroy unrealized layer windows on teardown, not close() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five per-monitor overlay `close_all` functions called `gtk::Window::close()` to tear down layer-shell surfaces. Per GTK 4.22's own GIR docs, `gtk_window_close` is a *request* — "similar to what happens when a window manager close button is clicked" — routed through `close-request`; it is not the call that drops GTK's internal toplevel reference. `gtk_window_destroy` is: "Drops the internal reference GTK holds on toplevel windows." `hytte-ui`'s layer-shell builder deliberately never shows these windows (`crates/hytte-ui/src/layer_window.rs:98`), and `modal.rs`'s `EAGER_PAGES` is empty, so nothing forces a realize. A drawer/overlay the user never opened on a given monitor is therefore unrealized when a hot-plug's `close_all` runs, and `close()` on an unrealized window doesn't tear it down — the window and its whole child widget tree survive. Worse, `trollshell/src/plugins/region.rs:392` aborts the plugin-panel reconcile subscription only on `connect_destroy`, so every hot-plug that never opened a given drawer leaks another live subscriber re-reconciling an orphaned widget tree. Switch all five `close_all` sites to `destroy()`: it works on unrealized windows, it's the documented ref-dropping call, and — unlike `close()` — it cannot be vetoed by a future `close-request` handler. Sites: modal.rs, overlays/{sidebar,frame,osd,notifications}.rs. overlays/prompt.rs is deliberately untouched (its window is always shown before close() runs, and its `abort_subscription` has no GTK in it at all). Closes #632 Co-Authored-By: Claude Opus 5 (1M context) --- trollshell/src/modal.rs | 19 +++++++++++++------ trollshell/src/overlays/frame.rs | 15 +++++++++++---- trollshell/src/overlays/notifications.rs | 8 +++++++- trollshell/src/overlays/osd.rs | 10 ++++++++-- trollshell/src/overlays/sidebar.rs | 7 ++++++- 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/trollshell/src/modal.rs b/trollshell/src/modal.rs index df4caa4..28bb94f 100644 --- a/trollshell/src/modal.rs +++ b/trollshell/src/modal.rs @@ -984,21 +984,28 @@ fn wire_retract_finish(revealer: >k::Revealer, key: String) { /// Close and remove all drawers (called before rebuilding bars on hot-plug). /// /// Drains `PANELS` into a local `Vec` and drops the borrow *before* calling -/// `gtk::Window::close` on each entry, rather than closing from inside the -/// `borrow_mut()` (#627). `wire_retract_finish`'s `child-revealed` handler -/// takes its own `borrow()` of the same `RefCell`; if `close()` ever -/// triggered that signal synchronously, running it under an active +/// `gtk::Window::destroy` on each entry, rather than destroying from inside +/// the `borrow_mut()` (#627). `wire_retract_finish`'s `child-revealed` +/// handler takes its own `borrow()` of the same `RefCell`; if `destroy()` +/// ever triggered that signal synchronously, running it under an active /// `borrow_mut()` would panic, and a panic unwinding through a glib callback -/// aborts the process. Whether `gtk_window_close`/`gtk_revealer_unmap` can +/// aborts the process. Whether `gtk_window_destroy`/`gtk_revealer_unmap` can /// emit `notify::child-revealed` synchronously is not verified either way — /// draining first removes the dependency on that question entirely, which is /// the point: not holding the borrow is free, so there's no need to resolve /// it first. +/// +/// Uses `destroy()`, not `close()` (#632): a drawer the user never opened on +/// this monitor is still unrealized when a hot-plug tears it down, and +/// `gtk_window_close` is a *request* routed through `close-request` that does +/// not drop GTK's internal toplevel reference — `gtk_window_destroy` is the +/// call documented to do that, and unlike `close()` it can't be vetoed by a +/// future `close-request` handler. pub fn close_all() { let panels: Vec<_> = PANELS.with(|panels| panels.borrow_mut().drain().map(|(_, v)| v).collect()); for panel in panels { - panel.window.close(); + panel.window.destroy(); } reset_drawer_open_states(); // A monitor teardown that held the open plugin panel must clear the diff --git a/trollshell/src/overlays/frame.rs b/trollshell/src/overlays/frame.rs index eed09ea..6441ef7 100644 --- a/trollshell/src/overlays/frame.rs +++ b/trollshell/src/overlays/frame.rs @@ -183,15 +183,22 @@ pub fn install(monitor: &Monitor, bar: &BarHandle) { /// a vanished output's frame window + raw subscription don't linger (the /// subscription has no `WeakRef` safety net, so it must be aborted explicitly — /// mirrors `sidebar::close_all`). +/// +/// Tears down with `destroy()`, not `close()` (#632): a frame overlay that +/// never showed a border on this monitor is still unrealized, and `close()` +/// neither destroys an unrealized window nor drops GTK's internal toplevel +/// reference — only `destroy()` does, and it can't be vetoed by a +/// `close-request` handler. pub fn close_all() { FRAMES.with(|map| { for (_, view) in map.borrow_mut().drain() { // Abort the raw tick-loop first so it can't queue another draw - // into the surface we're about to close, then close the window. - // The `bind_visible` apply-loop rides on the #224/#243 WeakRef - // fix: it frees itself on its next emission once the window drops. + // into the surface we're about to destroy, then destroy the + // window. The `bind_visible` apply-loop rides on the #224/#243 + // WeakRef fix: it frees itself on its next emission once the + // window drops. view.sidebar_sub.abort(); - view.window.close(); + view.window.destroy(); } }); } diff --git a/trollshell/src/overlays/notifications.rs b/trollshell/src/overlays/notifications.rs index 5f174d6..3db0c22 100644 --- a/trollshell/src/overlays/notifications.rs +++ b/trollshell/src/overlays/notifications.rs @@ -148,10 +148,16 @@ pub fn install(monitor: &Monitor) { /// are left running: they route by connector on each emission, so a fresh /// `install` re-keys the map and they self-heal. `SUBS_INSTALLED` therefore /// stays set — subscriptions wire exactly once for the process lifetime. +/// +/// Tears down with `destroy()`, not `close()` (#632): a toast surface that +/// never showed a notification on this monitor is still unrealized, and +/// `close()` neither destroys an unrealized window nor drops GTK's internal +/// toplevel reference — only `destroy()` does, and it can't be vetoed by a +/// `close-request` handler. pub fn close_all() { TOAST_WINDOWS.with(|map| { for (_, view) in map.borrow_mut().drain() { - view.window.close(); + view.window.destroy(); } }); } diff --git a/trollshell/src/overlays/osd.rs b/trollshell/src/overlays/osd.rs index 13a8898..51c94ce 100644 --- a/trollshell/src/overlays/osd.rs +++ b/trollshell/src/overlays/osd.rs @@ -189,6 +189,12 @@ pub fn install(monitor: &Monitor) { /// focused-output) keep running: they route by connector each emission, so a /// fresh `install` re-keys the map and they self-heal. `SUBS_INSTALLED` stays /// set so they wire exactly once for the process lifetime. +/// +/// Tears down with `destroy()`, not `close()` (#632): an OSD that never +/// popped on this monitor is still unrealized, and `close()` neither +/// destroys an unrealized window nor drops GTK's internal toplevel +/// reference — only `destroy()` does, and it can't be vetoed by a +/// `close-request` handler. pub fn close_all() { OSDS.with(|map| { for (_, view) in map.borrow_mut().drain() { @@ -196,14 +202,14 @@ pub fn close_all() { sub.abort(); } // Cancel pending timers so they can't fire (holding a live `Rc` - // clone of the view) into a closed window after teardown. + // clone of the view) into a destroyed window after teardown. if let Some(id) = view.timeout.take() { id.remove(); } if let Some(id) = view.fade_out_timeout.take() { id.remove(); } - view.window.close(); + view.window.destroy(); } }); } diff --git a/trollshell/src/overlays/sidebar.rs b/trollshell/src/overlays/sidebar.rs index c423afd..50aaea7 100644 --- a/trollshell/src/overlays/sidebar.rs +++ b/trollshell/src/overlays/sidebar.rs @@ -591,6 +591,11 @@ fn wire_escape(window: >k::Window, monitor: Monitor) { /// Close every sidebar surface and drop the per-monitor entries. Called /// before rebuilding bars on hot-plug, so stale layer-shell windows don't /// linger after a monitor disappears. +/// +/// Tears down with `destroy()`, not `close()` (#632): a sidebar never opened +/// on this monitor is still unrealized, and `close()` neither destroys an +/// unrealized window nor drops GTK's internal toplevel reference — only +/// `destroy()` does, and it can't be vetoed by a `close-request` handler. pub fn close_all() { PANELS.with(|panels| { for (key, panel) in panels.borrow_mut().drain() { @@ -616,7 +621,7 @@ pub fn close_all() { id.remove(); } panel.open_state.set(false); - panel.window.close(); + panel.window.destroy(); } }); // SIDEBAR_OPEN is keyed per-monitor and deliberately survives a rebuild