fix(overlays): finish the RefCell-across-GTK sweep — notifications cluster, install inserts, shared borrows (#643) - #673
Merged
Conversation
…uster, install inserts, shared borrows (#643) The overlay half of #643, sequenced behind #644/#663 which owned these files. 22 sites, all four spellings of the corrected definition (*any* borrow held across a GTK call, not just `borrow_mut()`). notifications.rs `apply_emission` is the headline: two named `RefMut`s plus the overflow slot held live across `vbox.remove`, `replace_card`/ `mount_card`, the overflow `remove`/`append` and the closing `set_visible`, whose argument was itself an `overflow_card.borrow()` temporary. It is the one site in the cluster whose emission the tree *documents* as synchronous (`connect_unmap` → `resume_expiry`, the mechanism #593's design rests on); it survives today only because that handler touches the expiry bookkeeping rather than `card_map`. Converted with take-and-restore, the `panels/stats.rs` idiom from #663. `TOAST_WINDOWS` and `PANELS` (modal) now store `Rc<_>` so `route_emission` and modal's nine entry points can clone a handle out instead of holding a shared borrow across `show_panel` / `set_stack_page` / `set_reveal_child` / `on_page_show` — mirroring `overlays::osd`'s existing `Rc<OsdView>`. `modal::live_panel`/`live_panels` are the snapshot helpers. The four `install` insert-drop sites (sidebar/frame/osd/consent) get the tail-expression + `drop(...)` shape #644 annotated at `modal::install`. Their comments state the mechanism precisely per the #663 review's correction: a refcount decrement, not a widget teardown. consent's is the weakest of the four — a `GdkMonitor` unref emits nothing — and says so. Also `tasks.rs::refresh_summary` (a `match *self.mode.borrow()` holding across every arm's `set_visible`/`set_text`, which #663 missed inside a range its body certified clean), `tasks.rs::do_create`'s getter-ordering folklore, and four sites the closing re-grep turned up in `widgets/{workspaces,tray,window_list}.rs`. No new tests: `trollshell` is a binary crate with no `system-tests`/xvfb bucket, and every site needs a live GTK widget tree to reach. The `Waker`-based harness doesn't apply — the two sites here with a `Mutable` wake inside the borrow still need a populated `PANELS`, i.e. GTK windows. Closes #643. Refs #663, #644, #631, #630, #627, #593. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The overlay half of the
RefCell-borrow-across-a-GTK-call sweep, sequencedbehind #644 and #663 (which owned these files) and covering the four groups
#643's scope-rewrite comment left open, plus what the closing re-grep turned up.
22 sites, all four spellings. The definition applied is the corrected one
from #643: any borrow held across a GTK call, not just
borrow_mut().1. The
notifications.rsnamed-binding cluster — highest priorityThis is the one site in the whole cluster where the tree documents the
emission as synchronous rather than leaving it unverified. The card-loop comment
and
replace_card's comment onvbox.remove(&old.widget)both state thatunmapping a card fires its
connect_unmap→resume_expiry, and #593's entirekeep-the-card-mounted design is built on that being immediate. It doesn't panic
today only because that handler reaches for the expiry bookkeeping rather than
for
card_map.trollshell/src/overlays/notifications.rs:269(apply_emission)let mut map = view.card_map.borrow_mut();live acrossvbox.remove,replace_card/mount_card, the overflowremove/append, andset_visibletrollshell/src/overlays/notifications.rs:270(apply_emission)let mut suppressed = view.suppressed_during_dnd.borrow_mut();same spantrollshell/src/overlays/notifications.rs:335(apply_emission)let mut slot = view.overflow_card.borrow_mut();live acrossremove()+append()trollshell/src/overlays/notifications.rs:350(apply_emission)set_visible(!map.is_empty() || view.overflow_card.borrow().is_some())trollshell/src/overlays/notifications.rs:223(route_emission)borrow()TOAST_WINDOWS.borrow()held across the whole ofapply_emissionFix: take-and-restore for the three cells (the
panels/stats.rsbuild_live_per_core_rowidiom from #663), compute the visibility bool from thelocals before handing anything back, and
Rc<ToastView>inTOAST_WINDOWSsoroute_emissioncan clone a handle out — mirroring howoverlays::osdalreadystores
Rc<OsdView>.Trade-off stated in the code, not hidden: while
apply_emissionruns, thecells hold empty
Defaults, so a re-entrant reader would see no cards ratherthan the live set. Same trade #663 made for
stats.rs's per-core bars andconnections.rs's row vecs, and it beats the alternative, which is not a wronganswer but a
BorrowMutErrorthrough a glib callback — a process abort. Nothingre-enters via the driver itself:
route_emissionis pumped from aspawn_localedfor_each, which cannot re-poll itself synchronously.2. The four
installinsert-drop sitesThe shape #644 annotated at
modal.rs, surviving unannotated in four moreplaces. All are the semicolon-rule hazard:
insertreturns the displaced value,and as a bare statement that value is a temporary of the same statement as
the
borrow_mut()RefMut— statement temporaries drop in reverse creationorder, so it runs its drop glue while the cell is still borrowed. Converted to
tail-expression
insert+ an outerdrop(...).trollshell/src/overlays/sidebar.rs:267— displacedSidebarPaneltrollshell/src/overlays/frame.rs:178— displacedFrameViewtrollshell/src/overlays/osd.rs:182— displacedRc<OsdView>trollshell/src/overlays/consent.rs:77— displacedMonitorTwo honesty notes.
Reachability. All four need
installto run twice for one key without anintervening
close_all, whichmain.rsprevents today. Weakest group in thesweep; converted because not holding the borrow costs nothing.
Mechanism. The #663 review's correction is applied here: this is a refcount
decrement, not a widget teardown. GTK holds its own reference to a mapped
toplevel, so dropping the Rust
gtk::Windowhandle does not dispose the window— that needs
destroy(), whichclose_allcalls. What actually runs inside theborrow is
JoinHandle/Mutable/SourceIddrop glue plus GObject unrefs. Thein-code comments say that rather than repeating the teardown framing.
The consent site is the weakest of the four and I'd rather say so than let it
read as evidence: the displaced value is a
Monitor, whose drop is aGdkMonitorunref, which emits nothing. There is no plausible re-entrant patheven in principle. Converted purely so the shape is uniform and nobody has to
re-derive which of the four were "the real ones".
3. Spelling-(3) shared
borrow()sitesmodal.rsneeded a type change to fix at all: every entry point wants a whole&ModalPanelto drive a run of GTK calls, soPANELSnow storesRc<ModalPanel>and two helpers —live_panel(key)(modal.rs:524) /live_panels()(modal.rs:518) — hand back owned handles with the borrowalready released.
close_allandinstallare theborrow_mut()counterparties.
trollshell/src/modal.rs:445— thePANELSvalue typetrollshell/src/modal.rs:1001(wire_retract_finish) —Refacrosswindow.set_visible(false)andopen_state.set(false), which wakes every drawer-open subscriber (futures-signalscallsWaker::wake()synchronously fromnotify)trollshell/src/modal.rs:1153(switch_active) — acrossset_stack_page/on_page_showtrollshell/src/modal.rs:1170(dismiss_all) — acrossset_reveal_childtrollshell/src/modal.rs:1182(retract_by_key) —if letscrutineeRefacrossset_reveal_childtrollshell/src/modal.rs:1201(open_by_key) — acrossshow_paneltrollshell/src/modal.rs:1237(open_plugin_by_key) — acrossshow_panel_active+set_active_paneltrollshell/src/modal.rs:1303(toggle_inner) — across all three arms; the busiest entry point in the module (every bar chip lands here)trollshell/src/modal.rs:1422(wire_recenter_on_map) — acrossapply_stats_scroll+main_margin_for_centertrollshell/src/overlays/osd.rs:432(route_show) —OSDS.borrow()acrossshow();OSDSwas alreadyRc<OsdView>, so this is a refcount bumptrollshell/src/overlays/sidebar.rs:190(is_settled_for_key) —PANELS.borrow()acrossrevealer.is_child_revealed()That last one is a property getter, which cannot emit. I converted it anyway
rather than leaving the getter exemption as folklore — #663's review flagged
exactly that gap ("if getters are categorically safe, that belongs in the
definition explicitly"). Three lines; cheaper to settle than to argue.
4.
widgets/tasks.rstrollshell/src/widgets/tasks.rs:695(refresh_summary) — spelling (3).match *self.mode.borrow() { … }borrows the place expression for the whole match, so theRefwas live across every arm'sset_visible/set_text, with a second nestedself.selected.borrow()in thePickarm. Threeborrow_mut()counterparties onmode(set_mode,set_value,reset) and three onselected;set_modeis reached from aconnect_toggledhandler. Both payloads areCopy, so the fix is two register copies.trollshell/src/widgets/tasks.rs:393(do_create) — the audit-trail correction from fix(ui): release RefCell borrows across GTK calls outside overlays/ (#643) #663's review.drop(lists)came afterDropDown::selected(), so the exclusion was only ever correct on getter-versus-setter grounds, which the definition never states. Rather than codify that exemption, the two statements are swapped.Also found by the closing re-grep (not in #643's list)
Same mechanical idiom; leaving them would have meant declaring these files clean
while three known named-
RefMut-across-remove/appendsites survived.trollshell/src/widgets/workspaces.rs:100(update_workspaces) — (2),RefMutacrosscontainer.remove/apply_workspace_visuals/append/insert_aftertrollshell/src/widgets/tray.rs:64(update_tray) — (2), same span pluscreate_item_buttontrollshell/src/widgets/window_list.rs:50(update_windows) — (2), same spantrollshell/src/widgets/tray.rs:131(create_item_button) — (3),item_cell.borrow()acrossapply_item_button_visuals;update_tray's reuse arm holds theborrow_mut()counterpartyRuled out, with reasons (so the next pass isn't misled)
Checked and deliberately not converted:
notifications.rs:149(install) — the semicolon rule's worked counter-example.TOAST_WINDOWS.with(|map| map.borrow_mut().insert(k, v))is safe because theinsertis the closure's tail expression: the returnedOptionmoves out past theRefMut. Annotated in place with a "do not add a semicolon here" note rather than needlessly wrapped.modal.rs:1215/:1253(open_on_focused,open_plugin_on_focused),sidebar.rs:142(toggle_on_focused) — key lookups only (contains_key,keys().next().cloned()), no GTK under the borrow.modal.rs:468(recompute_gates),modal.rs:506(drawer_open_state),sidebar.rs:651,osd.rs:162— no GTK call under the borrow. (osd.rs:162'sview.drawer_openis aCell<bool>, not aMutable, so it does not even wake.)sidebar.rs:166(current_visible_width_for_key) — readsopen_state.get(), aMutable, not GTK.prompt.rs:65,osd.rs:210,sidebar.rs:448/:637— abort a task or remove aSourceId; the exclusion fix(overlays): the same RefCell-held-across-a-GTK-close pattern exists in 7 more places (#627/#630 blast radius) #631/fix: the RefCell-across-a-GTK-call pattern also lives outside overlays/ — reactive_list.rs, calendar/tasks/connections rows, control-center, hytte-ui popup (#631 blast radius) #643 already made.modal.rs:1425, andlet-elsescrutinees generally — alet-elsetemporary drops at the end of its ownletstatement, not across a following call.widgets/tasks.rs:682, andif-condition borrows generally — condition temporaries drop before the block runs.widgets/screencast.rs:51—stop_allis niri IPC, no GTK.widgets/calendar.rs:531—group_events_by_dayis pure, and theRefdrops at the end of thatlet, before anypaint_cell.widgets/tray.rs:144/:178,widgets/workspaces.rs:63,widgets/tasks.rs:401— alreadydrop(...)the borrow before the first GTK call (:401after this PR's reorder).widgets/mpris.rs:216/:220,widgets/tasks.rs:79/:162/:173/:184,calendar.rs:309/:355/:403,consent.rs:255,prompt.rs:230,sidebar.rs:462— plain assignments; the displaced value is either non-GTK data or a slot the same function justtake()d toNone.Testing — what I could and could not verify
No new tests, and the reason is structural rather than a judgement call:
trollshellis a binary crate with nosystem-tests/xvfb bucket at all. Itsown
modal.rstest module already says so ("the trollshell binary has nosystem-tests/xvfb harness"). Every site in this PR needs a live GTK widgettree — a
ModalPanel, aToastView, aDuePicker— to reach, so thehytte-ui#[gtk::test]pattern(
close_catchers_tolerates_a_reentrant_teardown_from_destroy) has nowhere torun here. Standing one up would mean adding a
system-testsfeature to thebinary crate and wiring it into
flake.nix'schecks; that is a separatechange and probably its own issue.
The
Waker-based hermetic harness does not apply either, and I checked persite rather than assuming. It needs the reentrancy trigger to be a
Mutablewake reachable without GTK. Two sites here do have a
Mutablewake inside theformerly-live borrow —
wire_retract_finish'sopen_state.set(false)andshow_panel_active'sopen_state.set(true)— which is more than #663 found,but reaching either requires a populated
PANELS, i.e. a realModalPanel,i.e. GTK windows. So the trigger is only reachable through GTK and the harness
can't be built hermetically. #644's
reset_drawer_open_states_does_not_reenter_the_borrowremains the one site inthe cluster where it did apply.
What is verified: the existing hermetic suites still pass over the changed
code, including
sidebar.rs'sis_settled_for_key("nonexistent")/current_visible_width_for_key("nonexistent")guards andmodal.rs'sclose_all/reset_drawer_open_statestests, which now run againstHashMap<String, Rc<ModalPanel>>.Gates run locally:
cargo clippy --workspace --all-targets --features system-tests -- -D warnings(withRUSTC_WRAPPERunset),cargo test --workspace,xvfb-run cargo test --features system-tests -p hytte-ui,nix fmt.Live-verify wanted for the two behavioural conversions, since CI can't reach
them: the drawer (open / swap page / retract / deep-link, and a monitor
hot-plug) and the toast stack (several toasts at once so the "+N more" overflow
card appears and then goes away, plus a
replaces_idupdate while hovering atoast — that last one is the #593 hover-hold path
apply_emissionrewritesaround).
Not repeating a claim that was corrected
crates/trollshell-control-center/src/main.rsis not cited here as evidencefor anything. #643's correction comment established that its
connect_active_notifyroutes everything throughspawn_on_runtime→ tokio → aoneshotawaited insideglib::spawn_future_local, i.e. fully deferred; it isexactly as speculative as every other site, and it is already fixed by #663.
Likewise the
hytte-uiSIGABRT result is cited for what it proves and no more:destroyemits synchronously from dispose and aBorrowMutErrorthrough a glibcallback really does abort the process. It is a synthetic probe of the
mechanism — nothing in the tree installs a
connect_destroyon a catcher —not evidence that any particular site was live.
Closes #643. Refs #663, #644, #631, #630, #627, #593.