Skip to content

fix(hytte-ui): destroy() the three remaining close()-teardown sites (#638) - #639

Merged
annikahannig merged 1 commit into
mainfrom
fix/hytte-ui-destroy-not-close-638
Jul 30, 2026
Merged

fix(hytte-ui): destroy() the three remaining close()-teardown sites (#638)#639
annikahannig merged 1 commit into
mainfrom
fix/hytte-ui-destroy-not-close-638

Conversation

@vibechoom

Copy link
Copy Markdown
Contributor

Closes #638

The close()/destroy() distinction

Per GTK 4.22's own GIR docs:

  • gtk_window_close"Requests that the window is closed… similar to what happens when a window manager close button is clicked." A request, routed through close-request, which the default handler only turns into a real teardown for a realized window.
  • gtk_window_destroy"Drops the internal reference GTK holds on toplevel windows." The documented ref-dropping call, unconditionally.

hytte-ui's layer-shell builder deliberately never shows windows at construction (crates/hytte-ui/src/layer_window.rs:98: "wire up layer-shell, but don't show"), so the unrealized case is reachable here the same way #632 found it reachable in trollshell.

The three sites

  • crates/hytte-ui/src/bar.rsBarHandle::close() and impl Drop for BarHandle, both self.window.close()self.window.destroy().
  • crates/hytte-ui/src/popup.rsclose_catchers's win.close()win.destroy(), tearing down the per-monitor dismiss-catcher windows.

Also corrected popup.rs's doc comments (on close_catchers, and the test-module preamble) — they already asserted the destroy-specific "removes it from GTK's window list" semantics while the code only called close(). Now destroy() is what's called and what's documented, so the comments are true rather than aspirational.

close-request grep result — the "check before changing" site

grep -rn "close_request\|connect_close_request" crates/ trollshell/ finds exactly one hit: crates/trollshell-control-center/src/main.rs:113, on the control-center's own adw::ApplicationWindow (its handler just cancels a poll timer on close). That window has no relationship to BarHandle or popup.rs's catcher windows — it's a separate binary that never links the shell.

I also grepped for direct callers of BarHandle::close() (as opposed to the self.window.close() call inside its own impl): none exist in trollshell/ or crates/. BarHandle is dropped (via Vec<BarHandle> clearing in main.rs on monitor rebuild) far more than it's explicitly .close()'d.

Conclusion: nothing relies on a close-request handler running for either the Bar window or a popup dismiss-catcher. Changed all three sites unconditionally, per the issue's expected outcome.

Test feasibility

hytte-ui does have a system-tests feature bucket (gated GTK unit tests under xvfb-run, crates/hytte-ui/Cargo.toml:16), and popup.rs already has an existing test (close_catchers_drains_and_destroys_idempotently) using the weak-ref-upgrade-after-close()ing pattern — it still passes unchanged, since its windows are present()-realized (both close() and destroy() tear down a realized window; this test doesn't discriminate the two, it just keeps regression-covering the teardown mechanism itself).

The genuinely new, previously-uncovered case is BarHandle's Drop on an unrealized window (the issue's "more interesting" site) — so I added bar::tests::drop_destroys_an_unrealized_bar_window, mirroring popup.rs's technique: construct a bare gtk::Window (never .present()'d, so unrealized), wrap it in BarHandle via the private struct literal (the test module is a child of bar), drop the handle, iterate the main context, and assert the weak ref no longer upgrades.

I did not take this on faith — I verified it discriminates before finalizing: I temporarily reverted the Drop impl to close() and reran the test under xvfb-run; it failed with exactly the panic message asserting the leak. Restoring destroy() makes it pass again. So this is a real regression test, not a tautology that would pass either way.

Local run (via nix shell nixpkgs#xvfb-run layered on the devShell, since neither is in the devShell or workspace buildInputs — matching flake.nix's own system-tests check):

xvfb-run -a cargo test -p hytte-ui --features system-tests
# 105 passed; 0 failed (includes bar::tests::drop_destroys_an_unrealized_bar_window
# and popup::tests::close_catchers_drains_and_destroys_idempotently)

Live-verify note

A lack of change is the pass condition here: bars and popup dismiss-catchers should still disappear correctly in the same situations they always did.

  • Multi-monitor hot-plug/kanshi profile switches: bars on removed monitors should still disappear (no visible bar surviving a monitor teardown), and rebuilt bars on remaining/re-added monitors should behave normally.
  • Popup dismiss-catchers: opening a bar-chip popup, then dismissing it (click-outside, Escape, autohide, or scroll) should still make the full-screen catcher windows disappear on every output — no leftover invisible click-eater.
  • Nothing here should be observably different from before the change, since every site was already realized in the popup case and destroy() is a superset of what a realized-window close() achieves; the BarHandle::Drop path is the one place behavior could differ (an unrealized bar handle dropped mid-setup now actually tears down instead of silently no-op'ing), which has no normal-operation user-visible symptom to watch for beyond "nothing leaks."

Gates run

  • unset RUSTC_WRAPPER && cargo clippy -p hytte-ui --all-targets --features system-tests -- -D warnings — clean.
  • cargo test -p hytte-ui — 46 passed, 0 failed (hermetic bucket).
  • xvfb-run cargo test -p hytte-ui --features system-tests — 105 passed, 0 failed.
  • nix fmt twice — 0 files changed both times.

…638)

`gtk::Window::close()` is a *request* — GTK 4.22's GIR docs describe it as
"similar to what happens when a window manager close button is clicked,"
routed through `close-request`, which the default handler only turns into a
teardown for a *realized* window. `gtk_window_destroy` is the documented
ref-dropping call: "Drops the internal reference GTK holds on toplevel
windows."

`hytte-ui`'s layer-shell builder deliberately never shows its windows at
construction (`layer_window.rs:98`: "wire up layer-shell, but don't show"),
so the unrealized case is reachable in this crate the same way #632 found it
reachable in `trollshell`. Three sites still called `close()`:

- `bar.rs`'s `BarHandle::close()` and its `Drop` impl.
- `popup.rs`'s `close_catchers`, which tears down the per-monitor dismiss-
  catcher windows.

Switched all three to `destroy()`. Also corrected `popup.rs`'s doc comments
(on `close_catchers` and the test-module preamble), which already claimed
the destroy-specific "removes it from GTK's window list" semantics while the
code only called `close()`.

Added a `system-tests`-gated regression test for the `BarHandle` Drop path
(the one the issue flags as "more interesting": a handle dropped before its
bar was ever shown holds an unrealized window). Empirically confirmed it
discriminates: reverting the Drop impl to `close()` makes the new test fail
under `xvfb-run`, and `destroy()` makes it pass — this is not a tautology.

Closes #638
@annikahannig
annikahannig merged commit 743e106 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