Skip to content

fix(overlays): destroy unrealized layer windows on teardown, not close() (#632) - #637

Merged
annikahannig merged 1 commit into
mainfrom
fix/overlays-destroy-not-close-632
Jul 30, 2026
Merged

fix(overlays): destroy unrealized layer windows on teardown, not close() (#632)#637
annikahannig merged 1 commit into
mainfrom
fix/overlays-destroy-not-close-632

Conversation

@vibechoom

Copy link
Copy Markdown
Contributor

Closes #632

The bug

All five per-monitor overlay close_all functions tore down their layer-shell windows with gtk::Window::close():

  • trollshell/src/modal.rs
  • trollshell/src/overlays/sidebar.rs
  • trollshell/src/overlays/frame.rs
  • trollshell/src/overlays/osd.rs
  • trollshell/src/overlays/notifications.rs

GTK 4.22's own GIR docs draw a sharp line between the two calls:

  • gtk_window_close"Requests that the window is closed. This is similar to what happens when a window manager close button is clicked." A request, routed through close-request.
  • gtk_window_destroy"Drops the internal reference GTK holds on toplevel windows."

close() is not the call that releases GTK's internal toplevel reference — destroy() is.

hytte-ui's layer-shell builder deliberately never shows these windows (crates/hytte-ui/src/layer_window.rs:98: "Construct the gtk::Window, wire up layer-shell, but don't show"), 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. If destroy() never fires, every hot-plug/kanshi switch that never opened a given drawer on that monitor leaks another live subscriber re-reconciling an orphaned widget tree.

The fix

Switched all five close_all sites from close() to destroy(). It works on unrealized windows, it's the documented ref-dropping call, and — unlike close() — it can't be vetoed by a future close-request handler on these shell-owned, non-cancellable surfaces.

Left the surrounding structure alone:

trollshell/src/overlays/prompt.rs was not touched, per the issue and task scope:

  • Its abort_subscription has the same if let Some(x) = …borrow_mut().take() shape as the fixed sites, but its body calls handle.abort() on a tokio AbortHandle — no GTK, nothing to destroy.
  • Its own close_prompt() does call w.close(), but that window is only ever in PROMPT_WINDOW's slot after show_prompt has already called window.set_visible(true) + window.present() — i.e. it's realized by the time close() runs, and it's out of the issue's five named files, so it wasn't touched even though the same underlying GTK distinction technically applies to it.

Other connect_destroy dependencies found

Swept the whole tree for connect_destroy:

  • trollshell/src/plugins/region.rs:392 — the one already named in the issue (plugin-panel reconcile teardown on a sidebar/drawer child). Now fires correctly once the parent drawer window is destroy()d instead of close()d.
  • trollshell/src/plugins/region.rs:212 — same shape, one layer down: build_region's render-subscription teardown on the mount-region gtk::Box itself. This container is used both by the sidebar slots (now fixed transitively, since sidebar::close_all now destroy()s the parent window whose children cascade) and by the bar's bar_left/center/right_slot regions, which live inside Bar's own window (crates/hytte-ui/src/bar.rs) — not one of this issue's five files.
  • crates/hytte-reactive/src/bind.rs:55 — the generic bind() apply-loop's WeakRef self-free. Same family: relies on the widget's destroy signal firing when its ancestor window goes away.

Also found, outside connect_destroy but the same underlying defect: crates/hytte-ui/src/bar.rs:195 and :208, and crates/hytte-ui/src/popup.rs:287, also call window.close() for teardown. The Bar window is always shown (mapped) before a rebuild ever tears it down, so it's less likely to hit the "unrealized, so close() no-ops entirely" failure mode this issue centers on — but the same "close() isn't the ref-dropping call" concern applies in principle. Left untouched: none of these are trollshell's five named files, they're a different crate (hytte-ui), and the issue explicitly scopes to the five trollshell/src sites — flagging here rather than expanding scope.

Test feasibility

No hermetic test was added. trollshell/src/modal.rs's own existing test-module doc comment already states the crate has no system-tests/xvfb harness (unlike hytte-bus/hytte-reactive/hytte-services/hytte-ui, which do), and the existing close_all_resets_drawer_open_state test relies on PANELS being empty on a test thread specifically so the drain touches no GTK at all. Asserting that destroy() actually drops GTK's internal toplevel reference needs a live display server plus a booted App (to construct a real layer window), which this crate's test harness doesn't have — a test that doesn't exercise real GTK window realization wouldn't be honest coverage of this change, so I didn't write one. Confirmed via cargo test -p trollshell (233 passed, 0 failed) that nothing else regressed.

Live-verify note

The honest manual check for this fix: on a two-monitor Niri session, hold a glib::WeakRef to (or otherwise track) a drawer/sidebar/frame/OSD/toast window that's never been opened on the secondary output, force several kanshi profile switches (hot-plug/hot-unplug cycles), and confirm:

  1. The tracked window's WeakRef no longer upgrades after the corresponding close_all (or, short of instrumenting a WeakRef, that gtk::Window::toplevels()'s count doesn't grow per cycle for surfaces that were never opened).
  2. Plugin panels mounted in the sidebar/drawer still reconcile correctly (respond to plugin state changes) after several such switches — i.e. no stale, orphaned region.rs:392 subscriber is left double-rendering into a dead tree.

Gates run

  • unset RUSTC_WRAPPER && cargo clippy -p trollshell --all-targets -- -D warnings — clean.
  • cargo test -p trollshell — 233 passed, 0 failed.
  • nix fmt twice — 0 files changed both times.

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) <[email protected]>
@annikahannig
annikahannig merged commit ee96105 into main Jul 30, 2026
1 check passed
vibechoom added a commit that referenced this pull request Jul 31, 2026
…urst falsified (#672)

Closes #660

Bring docs/live-verify.md current across the ten PRs that merged after the
last refresh (#634): #637, #639, #644, #645, #662, #663, #664, #666, #668,
plus #642 (coverage bump only, per the issue's own conclusion — its default
station-id fix needs no live-verify caveat since write_default_config only
touches an absent places.toml).

Corrected the #630 entry: it told a verifier to expect modal::close_all
tearing down with window.close(); #637 changed that call to destroy() after
#630 had already merged, so the entry named a call the code no longer makes
(exactly the issue's finding). Also added a new #645 entry to the Network
panel section for the networkd startup-refresh retry, since that path used to
latch "no link manager has answered yet" forever on a transient first-refresh
failure (curable only by a shell restart) and now self-heals — swept the rest
of the doc for the same class of falsified expectation (inert-until-restart)
and found nothing else affected; the wifi-side #609/#634 entries describe a
different code path and are unaffected.

New entries added:
- #637/#639 — the destroy()-not-close() sweep across the five trollshell
  overlays and the three remaining hytte-ui sites (bar.rs, popup.rs),
  including #637's WeakRef/toplevels()-count kanshi hot-plug gesture.
- #644/#663 — the RefCell-borrow-across-GTK-call sweep (31 sites total
  across overlays/modal.rs and the rest of the UI, incl. an 8-panel-wide
  reactive_list.rs helper and the control-center). Both documented with the
  same "nothing changes" honest pass condition #630 already used.
- #664 — mpris Auto chip fix + bare-string artist widening. Needs two live
  MPRIS players and a shell restart for the new CSS class, both called out.
- #666 — new top-level "Claude bridge" section for the keyless loopback
  shim, nobody has run this service live yet. Carries the PR's flagged
  riskiest assumption (hive-claude's SessionNotFound classification) with
  its journal tell.
- #668 — new top-level "D-Bus name ownership" section. Documents that
  recovery going from ~250ms to up to 5 minutes after a squatter exits is
  the expected cost of the fix, not a regression to file.

Left alone: #642 (no doc change needed beyond the coverage line, matching
the issue's own recommendation after checking places.toml's write-once
behavior); #637's abort_subscription/prompt.rs exclusions and #644/#663's
"deliberately not touched" sites (no live-visible behavior, nothing to
verify).

Gates: markdown-only diff (git diff --stat: 1 file, docs/live-verify.md).
nix fmt run twice (1 file reformatted first pass — a prose reflow — 0
changed second pass). Clippy/tests deliberately skipped: nix/package.nix's
crane filter excludes markdown, so the packaged derivation is unaffected and
a nix build would burn a full workspace recompile for zero signal (the same
crane-filter trap #662 already documented). nix build was not run, per
instructions.

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants