diff --git a/trollshell/src/modal.rs b/trollshell/src/modal.rs index 77e6754..49e1ef1 100644 --- a/trollshell/src/modal.rs +++ b/trollshell/src/modal.rs @@ -1,5 +1,6 @@ use std::cell::RefCell; use std::collections::HashMap; +use std::rc::Rc; use hytte::adw; use hytte::futures_signals::signal::{Mutable, Signal}; @@ -433,7 +434,15 @@ impl GateId { } thread_local! { - static PANELS: RefCell> = RefCell::new(HashMap::new()); + /// `Rc`, not a bare `ModalPanel` (#643): every entry point below + /// needs a whole `&ModalPanel` to drive a run of GTK calls + /// (`set_stack_page` / `show_panel` / `set_reveal_child` / `on_page_show`), + /// and the only way to do that without holding this cell borrowed across all + /// of it is to clone a handle out first. `close_all` and `install` are the + /// `borrow_mut()` counterparties that make a held shared borrow a hazard; + /// a `BorrowMutError` unwinding through a glib callback aborts the process. + /// Mirrors `overlays::osd`'s `OSDS`. + static PANELS: RefCell>> = RefCell::new(HashMap::new()); /// Per-connector drawer-open state, decoupled from `ModalPanel` lifetime /// so subscribers (OSD, bar CSS) can wire up before `install` runs and /// survive bar rebuilds on hot-plug. @@ -501,6 +510,21 @@ fn drawer_open_state(key: &str) -> Mutable { }) } +/// Every mounted drawer, as owned handles with the [`PANELS`] borrow already +/// released. The read half of the #643 sweep for this module: every caller that +/// drives GTK over the panel set takes this snapshot instead of iterating +/// `panels.borrow().values()` across those calls, so no GTK emission can ever +/// find `PANELS` borrowed. Cloning an `Rc` per drawer is a refcount bump. +fn live_panels() -> Vec> { + PANELS.with(|panels| panels.borrow().values().map(Rc::clone).collect()) +} + +/// One mounted drawer by connector key, as an owned handle with the [`PANELS`] +/// borrow already released. Single-key counterpart to [`live_panels`]. +fn live_panel(key: &str) -> Option> { + PANELS.with(|panels| panels.borrow().get(key).map(Rc::clone)) +} + /// Build the drawer for one monitor and mount it as a layer-shell window. /// /// `bar` is the bar this drawer hangs off; its `edge` and `offset` (the @@ -581,7 +605,7 @@ pub fn install(monitor: &Monitor, bar: &BarHandle, edge: Edge, offset: i32) { drop(PANELS.with(|panels| { panels.borrow_mut().insert( key.clone(), - ModalPanel { + Rc::new(ModalPanel { window, revealer, stack, @@ -591,7 +615,7 @@ pub fn install(monitor: &Monitor, bar: &BarHandle, edge: Edge, offset: i32) { geometry, pending_center: RefCell::new(None), open_state: drawer_open_state(&key), - }, + }), ) })); } @@ -959,20 +983,23 @@ fn wire_retract_finish(revealer: >k::Revealer, key: String) { if r.is_child_revealed() { return; } - let closed_plugin_panel = PANELS.with(|panels| { - let panels = panels.borrow(); - // Post-#627, `close_all` drains every entry out of `PANELS` before - // closing any window, so a synchronous `child-revealed` emission - // during one of those closes now always lands here (this key is - // already gone). Benign: `close_all`'s own tail performs - // everything the `Some` arm below would have — - // `set_visible(false)` on a window that's being dropped anyway, - // `*current = None`, `open_state.set(false)`, and - // (unconditionally) `set_active_panel(None)` — so returning - // `false` here just skips redundant work, not a missed teardown. - let Some(panel) = panels.get(&key) else { - return false; - }; + // Post-#627, `close_all` drains every entry out of `PANELS` before + // closing any window, so a synchronous `child-revealed` emission + // during one of those closes now always lands on the `None` branch + // (this key is already gone). Benign: `close_all`'s own tail performs + // everything the `Some` arm below would have — `set_visible(false)` + // on a window that's being dropped anyway, `*current = None`, + // `open_state.set(false)`, and (unconditionally) + // `set_active_panel(None)` — so doing nothing here just skips + // redundant work, not a missed teardown. + // + // The lookup is a `.cloned()` handle rather than a `Ref` held over the + // body (#643): `set_visible(false)` is a GTK call, and `open_state + // .set(false)` wakes every drawer-open subscriber — `futures-signals` + // calls `Waker::wake()` synchronously from `notify`. Both are paths a + // re-entrant `PANELS` borrow could arrive on. + let panel = PANELS.with(|panels| panels.borrow().get(&key).map(Rc::clone)); + let closed_plugin_panel = panel.is_some_and(|panel| { panel.window.set_visible(false); let was_plugin = matches!(&*panel.current.borrow(), Some(Active::Plugin(_))); *panel.current.borrow_mut() = None; @@ -1117,15 +1144,19 @@ fn reset_drawer_open_states() { /// Future work could open `target` on the primary monitor in that case; /// for v1 we treat the deep-link as "in this drawer, jump there". pub fn switch_active(target: Page) { - PANELS.with(|panels| { - for panel in panels.borrow().values() { - if panel.current.borrow().is_some() { - set_stack_page(panel, target); - *panel.current.borrow_mut() = Some(Active::Builtin(target)); - on_page_show(panel, target); - } + // Snapshot the handles first, then act with no `PANELS` borrow live (#643): + // `set_stack_page` and `on_page_show` are runs of GTK calls, and holding the + // shared borrow across them means any synchronous emission that re-enters + // `PANELS` — `close_all`'s and `install`'s `borrow_mut()` are the + // counterparties — panics, fatally, from inside a glib callback. Cloning an + // `Rc` per open drawer is a refcount bump. + for panel in live_panels() { + if panel.current.borrow().is_some() { + set_stack_page(&panel, target); + *panel.current.borrow_mut() = Some(Active::Builtin(target)); + on_page_show(&panel, target); } - }); + } recompute_gates(); } @@ -1133,22 +1164,24 @@ pub fn switch_active(target: Page) { /// callbacks (e.g. the power-menu action rows) that don't carry a monitor /// handle but want the drawer to close after their action fires. pub fn dismiss_all() { - PANELS.with(|panels| { - for panel in panels.borrow().values() { - panel.revealer.set_reveal_child(false); - } - }); + // Snapshot before revealing (#643): `set_reveal_child(false)` starts the + // retract, and `wire_retract_finish`'s `child-revealed` handler — which + // reads `PANELS` itself — is what finishes it. + for panel in live_panels() { + panel.revealer.set_reveal_child(false); + } } /// Begin the retract animation. The notify-child-revealed handler finishes /// the teardown (hiding the single surface, catcher and card together) when it /// ends. fn retract_by_key(key: &str) { - PANELS.with(|panels| { - if let Some(panel) = panels.borrow().get(key) { - panel.revealer.set_reveal_child(false); - } - }); + // Bind-then-act (#643): an `if let Some(panel) = panels.borrow().get(key)` + // keeps the scrutinee's `Ref` alive for the whole then-block, i.e. across + // `set_reveal_child` — whose completion handler re-reads `PANELS`. + if let Some(panel) = live_panel(key) { + panel.revealer.set_reveal_child(false); + } } pub fn open(monitor: &Monitor, page: Page) { @@ -1161,14 +1194,14 @@ pub fn open(monitor: &Monitor, page: Page) { /// anchors flush with the bar's trailing main-axis edge (`pending_center` = /// `None`, `main_margin` = 0). No-op if no drawer is mounted for `key`. fn open_by_key(key: &str, page: Page) { - PANELS.with(|panels| { - let panels = panels.borrow(); - let Some(panel) = panels.get(key) else { - return; - }; + // Resolve to an owned handle first (#643): `show_panel` presents the + // surface and reveals the card — a long run of GTK calls, plus an + // `open_state.set(true)` that wakes subscribers synchronously — none of + // which may run under a live `PANELS` borrow. + if let Some(panel) = live_panel(key) { *panel.pending_center.borrow_mut() = None; - show_panel(panel, page, 0); - }); + show_panel(&panel, page, 0); + } recompute_gates(); } @@ -1199,17 +1232,15 @@ pub fn open_on_focused(preferred: Option<&str>, page: Page) { /// early as possible (risk #1 in the PR: measure-before-show). No-op if no drawer /// is mounted for `key`. fn open_plugin_by_key(key: &str, plugin_id: &str) { - PANELS.with(|panels| { - let panels = panels.borrow(); - let Some(panel) = panels.get(key) else { - return; - }; + // Owned handle, no live `PANELS` borrow (#643) — as in [`open_by_key`], + // plus `set_active_panel`, which publishes into the plugin host. + if let Some(panel) = live_panel(key) { *panel.pending_center.borrow_mut() = None; // Publish the selection BEFORE presenting so the child reconciles first // (`show_panel_active` also publishes it via `on_active_show`; idempotent). crate::plugins::set_active_panel(Some(plugin_id)); - show_panel_active(panel, Active::Plugin(plugin_id.to_owned()), 0); - }); + show_panel_active(&panel, Active::Plugin(plugin_id.to_owned()), 0); + } recompute_gates(); } @@ -1262,11 +1293,14 @@ fn toggle_inner( retract_on_same: bool, ) { let key = monitor_key(monitor); - PANELS.with(|panels| { - let panels = panels.borrow(); - let Some(panel) = panels.get(&key) else { - return; - }; + // Owned handle before any of the three arms run (#643): every one of them + // drives GTK — `set_reveal_child`, `set_stack_page`, `on_page_show`, + // `trigger_center`'s measure, `show_panel` — and the `Ref` a + // `panels.borrow().get(&key)` produces would be live across all of it. This + // is the busiest entry point in the module (every bar chip lands here), so + // it is also the one most worth not making conditional on whether GTK + // happens to emit synchronously today. + if let Some(panel) = live_panel(&key) { let current = panel.current.borrow().clone(); match current { // Same built-in page already open → retract (`toggle`) or re-run @@ -1277,30 +1311,30 @@ fn toggle_inner( if retract_on_same { panel.revealer.set_reveal_child(false); } else { - on_page_show(panel, page); + on_page_show(&panel, page); } } Some(_) => { - set_stack_page(panel, page); + set_stack_page(&panel, page); *panel.current.borrow_mut() = Some(Active::Builtin(page)); - on_page_show(panel, page); + on_page_show(&panel, page); } None => { // Build + set the visible child first so `measure` reflects the // target page's natural size, not whatever was last shown, and // so the `main_margin_for_center` measure below sees the real // page. `show_panel` re-sets it (idempotent). - set_stack_page(panel, page); - let chip_center = trigger_center(panel, trigger.upcast_ref()); + set_stack_page(&panel, page); + let chip_center = trigger_center(&panel, trigger.upcast_ref()); *panel.pending_center.borrow_mut() = chip_center; // Best-effort margin now (may use a pre-map measure that // underestimates); the window-map handler recomputes from the // real card allocation once the surface is mapped. - let margin = chip_center.map_or(0, |c| main_margin_for_center(panel, c)); - show_panel(panel, page, margin); + let margin = chip_center.map_or(0, |c| main_margin_for_center(&panel, c)); + show_panel(&panel, page, margin); } } - }); + } // Swap/open may have changed which page is visible; the same-page retract // branch is recomputed later by `wire_retract_finish`. Idempotent. recompute_gates(); @@ -1382,18 +1416,18 @@ fn wire_recenter_on_map(window: >k::Window, key: String) { window.connect_map(move |_| { let key = key.clone(); glib::idle_add_local_once(move || { - PANELS.with(|panels| { - let panels = panels.borrow(); - let Some(panel) = panels.get(&key) else { - return; - }; - apply_stats_scroll(panel); - let Some(center) = *panel.pending_center.borrow() else { - return; - }; - let margin = main_margin_for_center(panel, center); - set_widget_margin(&panel.positioner, panel.geometry.main_layer_edge(), margin); - }); + // Owned handle, no live `PANELS` borrow (#643): `apply_stats_scroll` + // activates a `GAction`, and `main_margin_for_center` measures the + // card — both GTK, both under a shared borrow before this. + let Some(panel) = live_panel(&key) else { + return; + }; + apply_stats_scroll(&panel); + let Some(center) = *panel.pending_center.borrow() else { + return; + }; + let margin = main_margin_for_center(&panel, center); + set_widget_margin(&panel.positioner, panel.geometry.main_layer_edge(), margin); }); }); } diff --git a/trollshell/src/overlays/consent.rs b/trollshell/src/overlays/consent.rs index 50a6bb9..b50335a 100644 --- a/trollshell/src/overlays/consent.rs +++ b/trollshell/src/overlays/consent.rs @@ -67,9 +67,14 @@ pub fn install(monitor: &Monitor) { tracing::debug!("consent::install: monitor has no connector name; skipping"); return; }; - MONITORS.with(|m| { - m.borrow_mut().insert(connector, monitor.clone()); - }); + // Tail-expression `insert` + an outer `drop`, for uniformity with the other + // three `install` sites in this sweep (#643). **The weakest one:** the + // displaced value is a `Monitor`, whose drop is a `GdkMonitor` refcount + // decrement — that emits nothing, so unlike `sidebar`/`frame`/`osd` there is + // no plausible re-entrant path even in principle. Converted anyway so the + // shape is consistent and nobody has to re-derive which of the four were + // "the real ones"; the cost is one `drop(…)`. + drop(MONITORS.with(|m| m.borrow_mut().insert(connector, monitor.clone()))); } /// Close any live prompt and forget the mounted monitors before a hot-plug diff --git a/trollshell/src/overlays/frame.rs b/trollshell/src/overlays/frame.rs index bcd0791..29afd33 100644 --- a/trollshell/src/overlays/frame.rs +++ b/trollshell/src/overlays/frame.rs @@ -167,15 +167,23 @@ pub fn install(monitor: &Monitor, bar: &BarHandle) { // Register the surface + its raw subscription so `close_all` can tear // both down on the next monitor hot-plug (re-keys cleanly by connector). - FRAMES.with(|map| { + // Tail-expression `insert` + an outer `drop`, not a bare `insert(…);` + // statement (#643): the displaced `FrameView` would otherwise be a + // temporary of the same statement as the `borrow_mut()` `RefMut`, and + // statement temporaries drop in reverse creation order — so it would run + // its drop glue (a `JoinHandle` plus a `gtk::Window` unref) with `FRAMES` + // still borrowed. A refcount decrement, not a widget teardown: disposing + // the window needs `destroy()`, which is `close_all`'s job. Same shape and + // same weak reachability as the annotated `modal::install` site. + drop(FRAMES.with(|map| { map.borrow_mut().insert( connector, FrameView { window, sidebar_sub, }, - ); - }); + ) + })); } /// Close every frame overlay and abort its sidebar tick-loop subscription, diff --git a/trollshell/src/overlays/notifications.rs b/trollshell/src/overlays/notifications.rs index 91253ca..34f7837 100644 --- a/trollshell/src/overlays/notifications.rs +++ b/trollshell/src/overlays/notifications.rs @@ -106,7 +106,12 @@ struct CardEntry { thread_local! { /// Mounted toast surfaces keyed by `Monitor.connector()`. Each /// entry owns its layer-shell window and the per-window state. - static TOAST_WINDOWS: RefCell> = + /// + /// `Rc`, not a bare `ToastView` (#643): [`route_emission`] has to + /// hand a view to [`apply_emission`], which runs a long stretch of GTK work, + /// and the only way to do that without holding this cell borrowed across all + /// of it is to clone a handle out. Mirrors `overlays::osd`'s `OSDS`. + static TOAST_WINDOWS: RefCell>> = RefCell::new(HashMap::new()); /// Set after the first `install()` call so module-level @@ -131,7 +136,17 @@ pub fn install(monitor: &Monitor) { }; let view = build_toast_view(monitor); - TOAST_WINDOWS.with(|map| map.borrow_mut().insert(connector, view)); + // **The semicolon rule** (#643). This `insert` returns the displaced + // `Rc`, and it is safe *only* because it is the closure's tail + // expression: the returned `Option` is moved out to the caller before the + // `borrow_mut()` `RefMut` — a temporary of that same expression — drops, so + // the displaced view runs its drop glue in the *outer* statement, with + // `TOAST_WINDOWS` no longer borrowed. Putting a semicolon after + // `insert(...)` inside the closure inverts that: the `Option` becomes a + // statement temporary dropped *before* the `RefMut`, i.e. GObject unrefs + // under a live borrow. Do not add one. (See the four `install` sites in + // `sidebar`/`frame`/`osd`/`consent`, which had exactly that shape.) + TOAST_WINDOWS.with(|map| map.borrow_mut().insert(connector, Rc::new(view))); if !SUBS_INSTALLED.with(Cell::get) { SUBS_INSTALLED.with(|c| c.set(true)); @@ -199,19 +214,23 @@ fn install_subscriptions() { /// trollshell hasn't mounted on). fn route_emission(notifs: &[Notification], dnd_on: bool, muted: &HashSet) { let target_name = focused_output::current(); - TOAST_WINDOWS.with(|map| { + // Resolve to an owned handle and let the borrow end at this `let` (#643). + // `apply_emission` is nothing but GTK work — `remove`/`append`/`set_visible` + // on the card stack — and `install`/`close_all` are the `borrow_mut()` + // counterparties on this cell. A shared borrow held across all that would + // panic on any re-entry, and a `BorrowMutError` unwinding through a glib + // callback aborts the process rather than failing the update. + let view = TOAST_WINDOWS.with(|map| { let map = map.borrow(); - if map.is_empty() { - return; - } - let view = target_name + target_name .as_ref() .and_then(|n| map.get(n)) - .or_else(|| map.values().next()); - if let Some(view) = view { - apply_emission(view, notifs, dnd_on, muted); - } + .or_else(|| map.values().next()) + .map(Rc::clone) }); + if let Some(view) = view { + apply_emission(&view, notifs, dnd_on, muted); + } } /// Toast-management logic running against a single per-monitor view. @@ -224,8 +243,31 @@ fn apply_emission( dnd_on: bool, muted: &HashSet, ) { - let mut map = view.card_map.borrow_mut(); - let mut suppressed = view.suppressed_during_dnd.borrow_mut(); + // Take both cells for the duration of the rebuild rather than holding a + // `RefMut` across it (#643). The named bindings this replaces stayed live + // all the way down past `vbox.remove`, `replace_card`/`mount_card`, the + // overflow `remove`/`append`, and the closing `set_visible` — spelling (2) + // of the sweep, a `let`-bound borrow instead of a chained temporary, but + // exactly the same hazard. + // + // This is the one site in the cluster where the tree *documents* the + // emission as synchronous rather than leaving it unverified: the comment on + // the card loop below, and `replace_card`'s own comment on + // `vbox.remove(&old.widget)`, both state that unmapping a card fires its + // `connect_unmap` → `resume_expiry`, and #593's keep-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`. + // + // Trade-off, stated plainly: while this runs, the cells hold empty + // `Default`s, so a re-entrant reader would see no cards rather than the live + // set. That is the same trade #643's other take-and-restore conversions make + // (`panels::stats`'s per-core bars, `panels::connections`' row vecs), and it + // is strictly better than the alternative, which is not a wrong answer but a + // `BorrowMutError` unwinding through a glib callback — a process abort. + // Nothing re-enters via the driver itself: `route_emission` is pumped from a + // `spawn_local`ed `for_each`, which cannot re-poll itself synchronously. + let mut map = view.card_map.take(); + let mut suppressed = view.suppressed_during_dnd.take(); let visible = filter_visible(notifs, dnd_on, muted, &mut suppressed); gc_suppressed(notifs, &mut suppressed); @@ -286,25 +328,35 @@ fn apply_emission( // unconditionally: it holds no hover state (no timer to strand), and the // remove-then-append is what keeps it pinned below every card the loop // above may have appended this pass. - { - let mut slot = view.overflow_card.borrow_mut(); - if tail_noncritical_count == 0 { - if let Some(card) = slot.take() { - view.vbox.remove(&card); - } - } else { - if let Some(card) = slot.take() { - view.vbox.remove(&card); - } - let card = build_overflow_card(&view.monitor, tail_noncritical_count); - view.vbox.append(&card); - *slot = Some(card); - } + // + // Take-then-act here too (#643): the `let mut slot = ….borrow_mut()` this + // replaces was held across the `remove()` and the `append()` below. Both + // arms started with the same removal, so it is hoisted out. + if let Some(card) = view.overflow_card.take() { + view.vbox.remove(&card); } + let overflow = if tail_noncritical_count == 0 { + None + } else { + let card = build_overflow_card(&view.monitor, tail_noncritical_count); + view.vbox.append(&card); + Some(card) + }; + + // Decide visibility from the locals, *before* handing anything back: the + // original spelled this `!map.is_empty() || view.overflow_card.borrow() + // .is_some()`, an argument-position `Ref` that is a temporary of the whole + // statement and so stayed alive across `set_visible` — spelling (4). + let any_mounted = !map.is_empty() || overflow.is_some(); + + // Store back. Each cell holds the empty `Default` its `take()` left behind + // (`HashMap`/`HashSet`/`Option::None` allocate nothing until first use), so + // these assignments drop nothing of consequence inside the borrow. + *view.card_map.borrow_mut() = map; + *view.suppressed_during_dnd.borrow_mut() = suppressed; + *view.overflow_card.borrow_mut() = overflow; - // Show/hide window based on whether any cards are mounted. - view.window - .set_visible(!map.is_empty() || view.overflow_card.borrow().is_some()); + view.window.set_visible(any_mounted); } struct Partition<'a> { diff --git a/trollshell/src/overlays/osd.rs b/trollshell/src/overlays/osd.rs index 2c23562..fd8fb06 100644 --- a/trollshell/src/overlays/osd.rs +++ b/trollshell/src/overlays/osd.rs @@ -168,9 +168,18 @@ pub fn install(monitor: &Monitor) { ); view.drawer_sub.replace(Some(drawer_sub)); - OSDS.with(|map| { - map.borrow_mut().insert(connector.clone(), view); - }); + // Tail-expression `insert` + an outer `drop`, not a bare `insert(…);` + // statement (#643): the displaced `Rc` would otherwise be a + // temporary of the same statement as the `borrow_mut()` `RefMut`, and + // statement temporaries drop in reverse creation order — so it would run + // its drop glue with `OSDS` still borrowed. At install time `OSDS` holds + // the only strong `Rc` (`route_show`'s clone is short-lived and on the same + // thread), so that drop would reach the whole `OsdView`: the `SourceId` + // slots, the `drawer_sub` `JoinHandle`, and a `gtk::Window` unref. Still a + // refcount decrement rather than a dispose (that needs `destroy()`, which + // is `close_all`'s job), and reachable only on a double `install` for one + // connector — same weak-but-free standard as the rest of the group. + drop(OSDS.with(|map| map.borrow_mut().insert(connector.clone(), view))); if !SUBS_INSTALLED.with(Cell::get) { SUBS_INSTALLED.with(|c| c.set(true)); @@ -413,22 +422,27 @@ fn install_subscriptions() { /// map (e.g. niri startup, monitor disconnect). fn route_show(state: &State) { let target_name: Option = focused_output::current(); - OSDS.with(|map| { + // Clone the `Rc` out and let the borrow end at this `let` (#643). `show()` + // is a run of GTK calls (`set_visible`, `present`, label/icon setters, timer + // arming), and `install`/`close_all` are the `borrow_mut()` counterparties + // on this cell — a shared borrow held across all that panics on any + // re-entry, and a `BorrowMutError` through a glib callback aborts the + // process. `OSDS` already stores `Rc`, so this is just a refcount + // bump. + let view = OSDS.with(|map| { let map = map.borrow(); - if map.is_empty() { - return; - } - let view = target_name + target_name .as_ref() .and_then(|name| map.get(name)) - .or_else(|| map.values().next()); - if let Some(view) = view { - if view.drawer_open.get() { - return; // drawer is showing the same control; OSD is redundant - } - show(view, state); - } + .or_else(|| map.values().next()) + .map(Rc::clone) }); + if let Some(view) = view { + if view.drawer_open.get() { + return; // drawer is showing the same control; OSD is redundant + } + show(&view, state); + } } /// Raise a transient "leave-by" nudge — the public entry point behind the diff --git a/trollshell/src/overlays/sidebar.rs b/trollshell/src/overlays/sidebar.rs index e6099e0..6a0a44e 100644 --- a/trollshell/src/overlays/sidebar.rs +++ b/trollshell/src/overlays/sidebar.rs @@ -179,12 +179,21 @@ pub fn is_settled(monitor: &Monitor) -> bool { /// Internal: keyed lookup used by both the public API and tests. fn is_settled_for_key(key: &str) -> bool { - PANELS.with(|panels| { + // Copy the two handles out and read them with no `PANELS` borrow live + // (#643). `is_child_revealed()` is only a property *getter*, so it cannot + // emit — but the sweep's definition is "any borrow across a GTK call" and + // deliberately does not carve out getters, and this runs from `frame.rs`'s + // per-frame tick callback while `install`/`close_all` hold the `borrow_mut()` + // counterparties. Cheaper to settle it than to keep the exemption as + // folklore. (`current_visible_width_for_key` just above reads + // `open_state.get()` — a `Mutable`, not GTK — so it needs nothing.) + let handles = PANELS.with(|panels| { panels .borrow() .get(key) - .is_none_or(|p| p.revealer.is_child_revealed() == p.open_state.get()) - }) + .map(|p| (p.revealer.clone(), p.open_state.get())) + }); + handles.is_none_or(|(revealer, open)| revealer.is_child_revealed() == open) } /// Build the sidebar surface for one monitor, mount it as a layer-shell @@ -238,7 +247,24 @@ pub fn install(monitor: &Monitor) { })) }; - PANELS.with(|panels| { + // `drop(…with(|…| …insert(…)))`, not a bare `insert(…);` statement (#643, + // mirroring the annotated `modal::install` site). `insert` returns the + // displaced `SidebarPanel`; as a bare statement that value is a temporary + // of the *same* statement as the `borrow_mut()` `RefMut`, and statement + // temporaries drop in reverse creation order — so it would run its drop + // glue with `PANELS` still borrowed. Tail-expression + outer `drop` moves + // that past the borrow. + // + // Stating the mechanism precisely, because the cluster has muddled it + // before: this is a *refcount decrement*, not a widget teardown. GTK holds + // its own reference to a mapped toplevel, so dropping the Rust `gtk::Window` + // handle does not dispose the window (that needs `destroy()`, which + // `close_all` calls). What actually runs here is `JoinHandle`/`Mutable` + // drop glue plus two GObject unrefs. Reachable only if `install` ran twice + // for one key without an intervening `close_all`, which `main.rs` currently + // prevents — weak, like the rest of the `install` group, and converted for + // the same reason: not holding the borrow costs nothing. + drop(PANELS.with(|panels| { panels.borrow_mut().insert( key, SidebarPanel { @@ -249,8 +275,8 @@ pub fn install(monitor: &Monitor) { visibility_subscription, zone_tick, }, - ); - }); + ) + })); } /// Layer-shell window anchored Left + Top + Bottom — full screen height, diff --git a/trollshell/src/widgets/tasks.rs b/trollshell/src/widgets/tasks.rs index 1b75ea7..de23ed0 100644 --- a/trollshell/src/widgets/tasks.rs +++ b/trollshell/src/widgets/tasks.rs @@ -390,8 +390,15 @@ fn build_create_popover( if summary.is_empty() { return; } - let lists = lists_for_create.borrow(); + // Read the dropdown *before* borrowing (#643). The `drop(lists)` below + // used to come after `list_picker_for_create.selected()`, so the `Ref` + // was live across that call; the sweep's four-spelling definition says + // "any borrow across a GTK call" and does not carve out getters, so + // rather than lean on `selected()` being a plain property read, the two + // statements are simply swapped. `wire_lists_bind` holds the + // `borrow_mut()` counterparty on this cell. let idx = list_picker_for_create.selected() as usize; + let lists = lists_for_create.borrow(); let Some(list) = lists.get(idx) else { return; }; @@ -685,7 +692,17 @@ impl DuePicker { } fn refresh_summary(&self) { - match *self.mode.borrow() { + // Copy both values out first (#643). `match *self.mode.borrow() { … }` + // borrows the place expression for the *whole* match, so the `Ref` was + // live across every arm's `set_visible`/`set_text` — spelling (3) — with + // a second, nested `self.selected.borrow()` inside the `Pick` arm. Three + // `borrow_mut()` counterparties sit on `mode` (`set_mode`, `set_value`, + // `reset`) and three more on `selected`, and one of them — + // `set_mode` — is reached from a `connect_toggled` handler on the chips. + // Both cells hold `Copy` payloads, so this is a plain register copy. + let mode = *self.mode.borrow(); + let selected = *self.selected.borrow(); + match mode { DueMode::None => { self.summary_label.set_visible(false); } @@ -698,7 +715,7 @@ impl DuePicker { self.summary_label.set_text("Due tomorrow"); } DueMode::Pick => { - let label = self.selected.borrow().map_or_else( + let label = selected.map_or_else( || "Pick a date".to_string(), |d| format!("Due {}", short_date(d)), ); diff --git a/trollshell/src/widgets/tray.rs b/trollshell/src/widgets/tray.rs index 2dbc8cd..d41ae7d 100644 --- a/trollshell/src/widgets/tray.rs +++ b/trollshell/src/widgets/tray.rs @@ -60,7 +60,13 @@ fn update_tray( items: &[TrayItem], monitor: &Monitor, ) { - let mut map = button_map.borrow_mut(); + // Take the map for the whole diff rather than holding a `RefMut` across it + // (#643, spelling (2)): the binding this replaces stayed live past + // `container.remove()`, `apply_item_button_visuals`, `create_item_button`, + // `container.append()` and `insert_after()`. A synchronous emission + // re-entering this cell would panic, and a `BorrowMutError` unwinding + // through a glib callback aborts the process. Stored back at the end. + let mut map = button_map.take(); let prev_keys: Vec = map.keys().cloned().collect(); let new_keys: Vec = items.iter().map(|i| i.key.clone()).collect(); @@ -103,6 +109,10 @@ fn update_tray( prev = Some(btn.clone()); } } + + // The cell holds the empty `ButtonMap` `take()` left behind, so this + // write-back drops nothing inside the borrow (#643). + *button_map.borrow_mut() = map; } /// Create a tray button for `item_cell` and wire up click handlers. @@ -118,10 +128,12 @@ fn create_item_button(item_cell: &ItemCell, monitor: &Monitor) -> gtk::Button { let btn = gtk::Button::new(); btn.add_css_class("ts-tray-item"); - { - let item = item_cell.borrow(); - apply_item_button_visuals(&btn, &item); - } + // Clone the snapshot out rather than passing a live `Ref` (#643, spelling + // (3)): `apply_item_button_visuals` is a run of GTK calls (icon build, + // `set_child`, `set_tooltip_text`), and `update_tray`'s reuse arm holds the + // `borrow_mut()` counterparty on this same cell. + let item = item_cell.borrow().clone(); + apply_item_button_visuals(&btn, &item); // Primary click: Activate or show-menu depending on ItemIsMenu. { diff --git a/trollshell/src/widgets/window_list.rs b/trollshell/src/widgets/window_list.rs index 736a81f..e92b0ce 100644 --- a/trollshell/src/widgets/window_list.rs +++ b/trollshell/src/widgets/window_list.rs @@ -46,7 +46,12 @@ fn update_windows( button_map: &Rc>>, windows: &[Window], ) { - let mut map = button_map.borrow_mut(); + // Take the map for the whole diff rather than holding a `RefMut` across it + // (#643, spelling (2)): the binding this replaces stayed live past + // `container.remove()`, `apply_window_visuals`, `container.append()` and + // `insert_after()`. Same shape as `widgets/workspaces.rs`'s and + // `widgets/tray.rs`'s diff loops. Stored back at the end. + let mut map = button_map.take(); let prev_keys: Vec = map.keys().copied().collect(); let new_keys: Vec = windows.iter().map(|w| w.id).collect(); @@ -86,6 +91,10 @@ fn update_windows( prev = Some(btn.clone()); } } + + // The cell holds the empty `HashMap` `take()` left behind, so this + // write-back drops nothing inside the borrow (#643). + *button_map.borrow_mut() = map; } /// Window's button label: title, falling back to app id, falling back to a diff --git a/trollshell/src/widgets/workspaces.rs b/trollshell/src/widgets/workspaces.rs index 315f4fa..c775f87 100644 --- a/trollshell/src/widgets/workspaces.rs +++ b/trollshell/src/widgets/workspaces.rs @@ -96,7 +96,13 @@ fn update_workspaces( button_map: &Rc>>, workspaces: &[niri::Workspace], ) { - let mut map = button_map.borrow_mut(); + // Take the map for the whole diff rather than holding a `RefMut` across it + // (#643, spelling (2)): the binding this replaces stayed live past + // `container.remove()`, `apply_workspace_visuals`, `container.append()` and + // `insert_after()`, so any synchronous emission re-entering this cell would + // panic — and a `BorrowMutError` unwinding through a glib callback aborts + // the process rather than failing the update. Stored back at the end. + let mut map = button_map.take(); let prev_keys: Vec = map.keys().copied().collect(); let new_keys: Vec = workspaces.iter().map(|ws| ws.id).collect(); @@ -136,6 +142,11 @@ fn update_workspaces( prev = Some(btn.clone()); } } + + // The cell holds the empty `HashMap` `take()` left behind — which allocates + // nothing until first insert — so this write-back drops nothing inside the + // borrow (#643). + *button_map.borrow_mut() = map; } /// Re-apply the label and the focused/active classes to an existing pill.