Skip to content

fix(notifications): keep unchanged toast cards mounted, carry the hover hold across a rebuild - #596

Merged
vibechoom merged 1 commit into
mainfrom
fix/notification-hover-pause-rebuild-593
Jul 29, 2026
Merged

fix(notifications): keep unchanged toast cards mounted, carry the hover hold across a rebuild#596
vibechoom merged 1 commit into
mainfrom
fix/notification-hover-pause-rebuild-593

Conversation

@vibechoom

Copy link
Copy Markdown
Contributor

apply_emission rebuilt every live toast card on every emission of the
combined toast signal. Destroying a card fires its connect_unmap, which
attach_hover_pause (#569) uses to release the service-side hover hold — right
in isolation, since GTK doesn't guarantee a leave on unmap. The replacement
came up with a fresh holds_enter = false, and GTK4 only retargets pointer
focus while processing a crossing or motion event, so a stationary pointer
produced no enter and the pause was never re-established.

Net effect: park the pointer on a toast to read it, let any other notification
arrive or expire, and the countdown silently resumed — the toast dismissed
mid-read. Exactly what #567 was filed to prevent.

Option 1, plus a hold transfer for the residual case

Option 1 (don't rebuild unchanged cards) is what shipped here — the
replaces_id semantics check out for it:

  • Notify builds a whole new Notification and does *existing = notification
    on a replaces_id re-post, so "unchanged" is a content question, not a
    mutation-tracking one.
  • Nothing rendered on a card changes without the content changing.
    created_at is re-stamped by every re-post, and timeout can change, but
    neither reaches the widget tree — so whole-struct equality would have been
    the wrong predicate even if Notification derived PartialEq (it doesn't;
    NotificationImage::Raw carries a pixel buffer).

card_content_eq therefore compares exactly the fields build_card reads, and
its doc comment says to keep the two in sync. The CardEntry that replaces the
bare gtk::Widget in card_map keeps the notification snapshot to compare
against.

That kills the reported failure (an unrelated notification arriving or
expiring) outright. It leaves one narrower case: a genuine replaces_id update
to the toast under the pointer still has to swap the widget, and would still
drop the hold. So replace_card also carries the hold (the issue's option
2, now scoped to the cards that actually need rebuilding): when the outgoing
card holds, a second hold is taken before it is unmapped, so its teardown
only drops the service-side count 1 → 2 → 1. It never reaches zero, nothing
re-arms mid-hover, the recorded remainder is never recomputed or floored, and
the fresh card inherits the hold via its seeded holds_hover, whose eventual
leave/unmap balances it exactly once.

The invariant the connect_unmap teardown protects is intact: it still runs on
every card, and it is still what balances the count when a card goes away while
genuinely hovered. Nothing double-decrements — and TimerState::resume already
clamps a leave with no matching enter to a no-op.

Two smaller consequences of not rebuilding, both deliberate:

  • replace_card keeps the card's place (prev_sibling +
    insert_child_after) instead of re-appending. With everything rebuilt each
    pass, stack order was arbitrary anyway (HashMap iteration); now that it's
    stable, an updated toast jumping to the bottom would be newly visible.
  • The overflow card is left rebuilding unconditionally. It holds no hover
    state, and the remove-then-append is what keeps it pinned below any card the
    main loop appended this pass.

Residual, documented on replace_card

If a replacement card is small enough that the pointer no longer lies over it,
no leave ever comes and the inherited hold lasts until the card unmaps — the
toast then waits for a dismiss instead of expiring. Bounded (the teardown still
balances the count) and strictly the safer of the two failures. Querying the
pointer (the issue's option 3) wouldn't fix this either: the fresh card has no
allocation until the next layout pass.

Tests

Hermetic, both buckets:

  • trollshell::overlays::notificationscard_content_eq and its two
    helpers: identical notifications keep the card; one case per rendered field
    forcing a rebuild; created_at/timeout churn not forcing one; action
    label/order/length; image variant + payload equality (same dimensions,
    different pixels must rebuild).
  • hytte-services::notifications
    hold_transfer_across_a_card_rebuild_never_unpauses pins the 1 → 2 → 1
    transfer sequence on the pure TimerState: stays paused, remainder
    untouched, and only the successor's leave re-arms.

Not covered, and not coverable in CI: the GTK half — that a stationary
pointer produces no enter on a replacement widget — needs a real compositor
and a real pointer. That's what the live-verify below is for.

Live-verify

Needs a running Niri session; I can't run one.

  1. The decisive test. notify-send "A" "park the pointer here" -t 5000,
    put the pointer on the toast and do not move it. After ~2s, from another
    terminal: notify-send "B" "unrelated". A must still be there — it must
    not dismiss while the pointer sits still. Move the pointer off A; it should
    then expire within roughly the time it had left (floored at 1s). Before this
    change, A vanished ~4s after B arrived.
  2. Repeat-fire. With the pointer parked on A, fire three more notifications
    a second apart. A stays.
  3. replaces_id under the pointer. id=$(notify-send -p "P" "0%" -t 5000),
    park the pointer, then notify-send -r "$id" "P" "50%" -t 5000. The card
    updates in place (same position in the stack, not re-appended to the bottom)
    and still does not expire under the stationary pointer.
  4. No stranded pause. Hover a toast, then dismiss it with its ✕ (or let a
    5th non-critical toast push it out of the visible head). Nothing should be
    left paused: subsequent toasts must expire on their own normally.
  5. Ordering / overflow. With >4 non-critical toasts live, the "+N more"
    card stays at the bottom of the stack as toasts come and go.

Closes #593

…er hold

`apply_emission` rebuilt every live toast card on every emission of the
combined toast signal. Destroying a card fires its `connect_unmap`, which
`attach_hover_pause` (#569) uses to release the service-side hover hold —
correct in isolation, since GTK doesn't guarantee a `leave` on unmap. The
replacement came up with a fresh `holds_enter = false`, and GTK4 only
retargets pointer focus while processing a crossing or motion event, so a
stationary pointer produced no `enter` and the pause was never
re-established. Park the pointer on a toast to read it, let any other
notification arrive or expire, and the countdown silently resumed — the
exact behaviour #567 was filed to prevent.

Option 1 from the issue: don't rebuild unchanged cards. The `replaces_id`
semantics support it — `Notify` builds a whole new `Notification` and
assigns over the existing entry, so "unchanged" is a content question. But
whole-struct equality would have been the wrong predicate: `created_at` is
re-stamped by every re-post and `timeout` can change, and neither reaches
the widget tree (`Notification` isn't `PartialEq` anyway —
`NotificationImage::Raw` carries a pixel buffer). `card_content_eq`
therefore compares exactly the fields `build_card` reads; `card_map` holds
a `CardEntry` (widget + the notification snapshot + the hover-hold flag)
instead of a bare widget.

That removes the reported failure — an unrelated notification arriving or
expiring — outright. The narrower case a genuine `replaces_id` update
under the pointer would still hit is covered by `replace_card`, which
carries the hold across the swap: when the outgoing card holds, a second
hold is taken *before* it unmaps, so the teardown only drops the count
1 → 2 → 1. It never reaches zero, nothing re-arms mid-hover, the recorded
remainder is never recomputed or floored, and the fresh card inherits the
hold — its eventual `leave`/`unmap` balances it exactly once. The
`connect_unmap` teardown still runs on every card, so a card that goes
away while genuinely hovered still can't strand the timer paused.

`replace_card` also keeps the card's place in the stack (`prev_sibling` +
`insert_child_after`) rather than re-appending: with everything rebuilt
each pass the order was arbitrary (`HashMap` iteration), and now that it's
stable an updated toast jumping to the bottom would be newly visible. The
overflow card is deliberately left rebuilding unconditionally — it holds
no hover state, and its remove-then-append is what keeps it pinned below
every card the main loop may have appended.

Tests: `card_content_eq` and its two structural helpers get hermetic
coverage in the overlay (identical keeps the card; one case per rendered
field forcing a rebuild; `created_at`/`timeout` churn not forcing one;
action label/order/length; image variant + payload), and the transfer
sequence is pinned on the pure `TimerState` in hytte-services. The GTK
half — that a stationary pointer produces no `enter` on a replacement
widget — needs a real compositor and is live-verify only.

Closes #593

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vibechoom

Copy link
Copy Markdown
Contributor Author

Review (maintainer pass) — no blocking findings

I asked for option 1 if the evidence supported it, and told you not to force it. You read replaces_id first, found option 1 holds, and found that the naive version of it would have been wrong. That's the right sequence.

The correction to my preference

Whole-struct equality would have rebuilt on every re-post, because Notify re-stamps created_at (and timeout can change) while neither reaches the widget tree. card_content_eq comparing exactly the fields build_card reads — documented as needing to stay in sync with it — is the correct predicate, and avoids adding PartialEq derives to service data shapes just to serve a widget concern.

Doing both options, correctly scoped

Option 1 alone would have fixed the reported symptom (an unrelated notification arriving) and left a genuine replaces_id update under a stationary pointer exhibiting the identical bug. Adding the hold transfer for cards that must genuinely be rebuilt closes that too.

The ordering is what makes it work, and it's right: pause_expiry is taken before vbox.remove() fires the outgoing card's unmap. I verified the service-side semantics that relies on — TimerState::pause only aborts on the first hold, and resume only re-arms when the count reaches 0 (crates/hytte-services/src/notifications.rs:594-622). So 1 → 2 → 1 leaves the timer aborted with remaining untouched: no recomputation, no floor reapplied, no double-decrement. And the connect_unmap teardown still runs on every card, so the leak-prevention invariant the original comment protects survives intact.

Adding a service-side test for the transfer rather than only overlay tests was the right instinct — the invariant lives on that side.

The documented residual — correctly reasoned

If a replacement card is small enough that the pointer no longer lies over it, no leave arrives and the inherited hold persists until unmap, so the toast waits for a dismiss instead of expiring. Three things make this the right call rather than a gap:

  1. It's bounded — teardown still balances the count.
  2. It's the safer of the two failure modes. A toast that lingers is a nuisance; one that vanishes mid-read is the bug we're fixing.
  3. Option 3 genuinely can't fix it — a fresh card has no allocation until the next layout pass, so querying the pointer would just read a stale rectangle. Saying so is more useful than silently picking option 3 and shipping a subtler version of the same bug.

Also noted

Preserving stack position via prev_sibling + insert_child_after is a real improvement — ordering was previously HashMap-arbitrary on rebuild. And fixing your own type_complexity clippy failure with a type Mutation alias rather than an #[allow] is the right reflex.

Live-verify remains the decisive test and can't run in CI: park the pointer on toast A, fire B from another app, and A must not dismiss while the pointer sits still.

@vibechoom

Copy link
Copy Markdown
Contributor Author

CI green ✅

flake-check SUCCESS (27m49s — the fastest run yet). Mergeable, reviewed above, no blocking findings.

@annikahannig this is the one that restores what #567 was for. The decisive check, which CI structurally cannot do: park the pointer on a toast to read it, have another app fire a second notification, and the first must not dismiss while your pointer sits still. Before this, any other notification arriving or expiring silently resumed the countdown on every live toast.

@vibechoom
vibechoom merged commit b057b12 into main Jul 29, 2026
1 check passed
@vibechoom
vibechoom deleted the fix/notification-hover-pause-rebuild-593 branch July 29, 2026 06:51
vibechoom added a commit that referenced this pull request Jul 29, 2026
…onth stamp (#603)

* docs: fold #497-#596 into the live-verify checklist, rename off the month stamp

docs/live-verify-2026-07.md covered #458-#496 and was two months stale (#602).
Extended every existing section with the live-verify items from the merged PRs
since #496, added new sections for subsystems that didn't exist in the 2026-07
wave (Stats drawer, Weather & location, Wallpaper & appearance, Notifications,
Wi-Fi, Night light, Widgets, Control-center, Plugin host & protocol), and added
a "Pending merge" section for #598 (still open) so it isn't mixed with shipped
work.

Renamed to docs/live-verify.md (month-agnostic) rather than starting a
docs/live-verify-2026-08.md — this is a living checklist that gets refreshed
periodically, not a dated snapshot of one merge wave. Verified no other file
in the repo referenced the old filename before renaming.

Method: `gh pr list --state merged --limit 200` filtered to #497+, then
`gh pr view <N> --json body` on each of the 54 merged PRs in range, grepped for
"Live-verify"/"Needs live-verify" sections. 38 of the 54 carried one (one of
those, #507, is the meta PR that created this doc originally — not a feature
item). Cross-checked against the seven items Annika listed in #602: all seven
have PR-body support except the "WPA3-only network — check pmf" clause on
#586/#579, which isn't mentioned in either PR body and is flagged as such
in-line rather than silently invented.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: treefmt continuation indent on the #514 entry

The branch shipped with a 4-space continuation indent inside a list item;
treefmt normalises it to 2. CI caught it (flake-check red at 1m36s) even
though the authoring agent reported nix fmt clean — its final run was not
actually idempotent.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: keep the #514 command on one line so treefmt converges

The entry wrapped an inline code span across a line break:

    run `hytte-infobroker auth
        --agent claude` with no grant

prettier is NOT idempotent on that construct — it shifted the continuation
line two spaces left on every pass (4 -> 2 -> 0 across three CI runs), so
re-running nix fmt could never converge and each "fix" just produced the
next red. Putting the whole command inside one unbroken code span removes
the ambiguity; nix fmt is now a fixpoint (three consecutive passes, 0
changed).

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
vibechoom added a commit that referenced this pull request Jul 30, 2026
Five PRs merged after #603's #497-#596 sweep, making the doc stale again in
three ways:

- #598 (nightlight Resolving state) merged — promote its entry out of
  "Pending merge" into the Night light subsection where it belongs, dropping
  the "still OPEN" framing, and remove the now-empty "Pending merge" heading.
- Add entries for #604 (drawer centers against the bar's live extent, not the
  monitor, so it stays correct with the sidebar open, plus the #442 kanshi
  mode-switch case) and #609 (a failed wifi backend probe now logs loudly
  instead of silently reading as "no wireless hardware"), sourced from their
  PR bodies.
- #590/#595/#596 were already folded in by #603 — nothing to add there.

#606 (the LICENSE file) carries no live-verify list; noted explicitly in the
closing section so it doesn't look overlooked.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
vibechoom added a commit that referenced this pull request Jul 30, 2026
#619) (#625)

* fix(notifications): keep the hover hold across a sticky/finite re-post (#619)

"This toast is hovered" was recorded in two independent places that
diverged during any sticky phase: the overlay card's Rc<Cell<bool>> and
TimerState::hover_count. A sticky notification had *no* timer entry at
all, so the overlay's pause_expiry landed on nothing; the next
replaces_id re-post that resolved finite then minted a fresh entry with
hover_count: 0 and armed a 5s sleep under a stationary pointer, and the
#593 card swap's own unmap/resume drove that count to zero and re-armed
it. The toast vanished under a pointer that was supposed to be holding it
— the third defect in this mechanism after #567 -> #593 -> #596.

Fixed service-side so the two records cannot diverge, rather than adding
more compensation in the overlay:

- TimerState::timeout becomes Option<Duration> — the same shape (and
  source) as Notification::timeout. "Sticky" is now a value *inside* the
  state, not the absence of the state.
- arm_expiry + disarm_expiry collapse into one set_expiry(id,
  Option<Duration>). It creates the entry on first post and never removes
  one; dismiss (via clear_timer) remains the only remover, where teardown
  is genuinely correct. A re-post therefore re-arms the deadline while
  leaving hover_count intact, and arms nothing at all while a hold is
  held — the fresh full timeout is recorded as `remaining` for the last
  leave to arm from.
- TimerState::resume only arms when timeout is finite, so a sticky
  notification that gets hovered and un-hovered does not sprout an expiry
  it never had.

Three doc comments asserted invariants the code did not hold and are
corrected: arm_expiry's "before the toast can render and be hovered, so
pause_expiry always finds the entry"; replace_card's "it never reaches
zero"; and attach_hover_pause's "a sticky notification makes the
pause/resume calls harmless no-ops".

Tests: the existing hold_transfer_across_a_card_rebuild_never_unpauses
only pinned the hover_count == 1 case. Adds the sticky->finite
parked-pointer sequence from the report (the hover_count == 0 +
carried-hold path), the symmetric finite->sticky-while-hovered case, a
sticky-post hold that arms nothing on either edge, and a sticky re-post
standing down an armed unhovered countdown. All hermetic.

Closes #619
Refs #567, #569, #593, #596, #599

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(notifications): test the entry lifetime, and arm before publishing

Review follow-ups on the #619 fix.

Test the layer the bug actually lived in. The four tests added in the
first pass all ran against one long-lived TimerState, so they
presupposed the property #619 was about — that the entry survives a
sticky phase. The map surgery is now extracted from set_expiry into
apply_expiry (the same "factored out for unit-testing" treatment
clear_timer already had), with the spawn passed in so a test can drive
the real TimerMap and observe what would have been armed without a
runtime. Verified by falsification: re-adding
`if timeout.is_none() { clear_timer(...); return; }` to apply_expiry
leaves all six state-transition tests green and fails exactly the two
new entry-lifetime tests.

Arm before publishing. notify published to `active` — waking the GTK
toast consumer on another thread — and only then called set_expiry. A
card appended to an already-mapped toast surface can be hovered as soon
as it is allocated, with no compositor round-trip to hide behind, so an
`enter` could land while the posting thread was still between the two
and pause_expiry would no-op: #619 again by a narrower race, and the
doc added in the first pass claimed the opposite. set_expiry now runs
before the list mutation at both post sites, and inside the same
`active` write guard — which also stops a 1ms expire_timeout from
running dismiss before the notification is in the list at all, stranding
it on screen with no timer. Both mutexes are taken in the
`active` -> `timers` order dismiss uses, so the pair cannot invert.

Make the unreachable state unconstructible. TimerState::new(timeout)
duplicated start's job and left Some(t)/hover_count 0/deadline None
constructible — "finite but not counting", which no transition can
produce. Removed in favour of Default, so a state only carries a timeout
once start has given it one, and the doc table's sticky row now
describes a fresh entry exactly.

Stop claiming a hole is closed. replace_card's comment said the only way
the carried hold doesn't return to one is a close, "harmless". It misses
close *and re-post* under the same id, which recreates the entry with
hover_count 0 under a still-holding card and reproduces the #619
symptom. Filed as #626 (it needs an entry generation counter); the
comment now says so instead of asserting the invariant holds.

Refs #619, #626

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

notifications: hover-pause is lost whenever any other notification arrives — the card rebuild drops the hold

1 participant