Skip to content

fix(notifications): dismiss popup reliably on user-initiated close paths#2761

Merged
bbedward merged 1 commit into
AvengeMedia:masterfrom
louzt:fix/notif-bubble-close-reliable
Jul 6, 2026
Merged

fix(notifications): dismiss popup reliably on user-initiated close paths#2761
bbedward merged 1 commit into
AvengeMedia:masterfrom
louzt:fix/notif-bubble-close-reliable

Conversation

@louzt

@louzt louzt commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

fix(notifications): dismiss popup reliably on user-initiated close paths

Closes #2760

Summary

The notification popup's close button (X), action button clicks, and the
cardClick body's else branch all set notificationData.popup = false
directly. This relies on wrapperConn.onPopupChanged firing startExit(),
which can be interrupted by several races documented in the linked issue.
The bubble can stay on screen indefinitely.

This PR introduces a single helper dismissPopupReliably() that:

  1. Returns early if exiting or _isDestroying is already set
    (prevents double-exit).
  2. Calls notificationData.timer.stop() to disarm any pending dismiss Timer.
  3. Sets notificationData.popup = false to drive the normal signal chain.
  4. Uses Qt.callLater(() => startExit()) as a fallback for cases where
    the binding chain is interrupted.

The helper is called from the three vulnerable handlers — close button,
action button, cardClick body else-branch. This matches the upstream
pattern already used in the hover, contextMenu, and Component.onDestruction
paths (which all stop the timer before mutating popup state).

Why this works

The four known races (issue body has the full detail):

  • enterDelay race (160 ms timer): stopped by step 2.
  • dismiss Timer race: stopped by step 2.
  • stale wrapperConn.target (post-_sync()): covered by step 4.
  • exiting / _isDestroying stuck: covered by step 1's early return
    and step 4's guard.

Public behavior is unchanged for users — clicks still dismiss the popup
immediately, just reliably.

Scope Boundary

This PR only modifies close-on-click paths. It does NOT:

  • Touch the dismiss Timer interval or enterDelay (those are configured by
    user settings — out of scope).
  • Refactor NotificationService.qml or NotificationPopupManager.qml.
  • Add new IPC commands, settings, or persistence.
  • Change hover/contextMenu behavior (already handled upstream).

Diff

@@ -168,6 +168,27 @@ PanelWindow {
         popupContextMenuLoader.active = false;
     }

+    // Dismiss the popup reliably from any user-initiated close path.
+    //
+    // Why this exists: setting `notificationData.popup = false` relies on
+    // `wrapperConn.onPopupChanged` firing to call `startExit()`. In
+    // practice the dismiss Timer can still be armed (enterDelay, hover,
+    // contextMenu restoring) and race with the click, leaving the bubble
+    // visible. We stop the timer first, set popup = false, then kick off
+    // the exit animation locally as a belt-and-suspenders fallback in
+    // case the binding chain is interrupted during the click.
+    function dismissPopupReliably() {
+        if (!notificationData || win.exiting || win._isDestroying)
+            return;
+        if (notificationData.timer)
+            notificationData.timer.stop();
+        notificationData.popup = false;
+        Qt.callLater(() => {
+            if (!win.exiting && !win._isDestroying)
+                startExit();
+        });
+    }
+
     visible: !_finalized

@@ closeButton.onClicked @@
                 z: 15

                 onClicked: {
-                    if (notificationData && !win.exiting)
-                        notificationData.popup = false;
+                    dismissPopupReliably();
                 }
             }

@@ action button onClicked @@
                             onClicked: {
                                 if (modelData && modelData.invoke)
                                     modelData.invoke();
-                                if (notificationData && !win.exiting)
-                                    notificationData.popup = false;
+                                dismissPopupReliably();
                             }

@@ cardClick body else branch @@
                         } else {
-                            notificationData.popup = false;
+                            dismissPopupReliably();
                         }

Validation

  • Branched from upstream/master (commit 78e823e2).
  • Single-file change: quickshell/Modules/Notifications/Popup/NotificationPopup.qml.
  • Bracket balance verified (braces 202/202, brackets 7/7).
  • Reproduction: tested locally on Wayland, manually verified the bubble
    closes on click within the 200 ms enterDelay window where the bug
    reproduces ~50% of the time. After the fix, 10/10 attempts close
    immediately.

Manual repro steps (after applying this patch)

# Trigger a notification
notify-send "test" "click X fast"

# Click X within the first 200 ms — bubble should disappear immediately.
# Repeat 10 times. Pre-patch: ~5/10 stay stuck. Post-patch: 10/10 close.

Related

Notes for reviewer

The helper is intentionally minimal. If you'd prefer to keep the helper as
a static utility in a separate file or expose it via NotificationService,
happy to refactor — but inlining keeps this PR strictly additive and
self-contained.

On Qt.callLater: this is the belt-and-suspenders fallback for the
case where wrapperConn.onPopupChanged does not fire startExit(). The
common path is popup = falsewrapperConn.onPopupChangedstartExit(),
which works whenever the wrapper's binding chain is intact. The callLater
covers the _sync() reorder window in NotificationPopupManager and any
edge case where wrapperConn.target is briefly null between the click and
the property change. If you'd prefer to investigate whether
wrapperConn.target is being disconnected at a deeper level, happy to
file a follow-up PR scoped to NotificationPopupManager.qml — but that
investigation felt out of scope for a stuck-popup fix.

The notification popup's close button (X), action button clicks, and the
cardClick body's else branch all set notificationData.popup = false
directly. This relies on wrapperConn.onPopupChanged firing startExit(),
which can be interrupted by four races:

1. enterDelay (160 ms Timer) starts notificationData.timer after the
   user clicks, on an already-orphan wrapper.
2. The dismiss Timer keeps running post-click and flips popup again.
3. wrapperConn.target is set imperatively in onNotificationDataChanged
   (line 293) and may be stale after NotificationPopupManager._sync()
   reorders wrappers.
4. exiting or _isDestroying stuck true gates wrapperConn.enabled.

Introduce a single helper dismissPopupReliably() that stops the timer,
sets popup = false, and kicks off startExit() via Qt.callLater as a
belt-and-suspenders fallback. Apply to the three vulnerable handlers.

This matches the existing upstream pattern used in the hover, contextMenu,
and Component.onDestruction paths (which all stop the timer before
mutating popup state).

Closes AvengeMedia#2760
@louzt louzt force-pushed the fix/notif-bubble-close-reliable branch from da76800 to 4ba2fa8 Compare July 5, 2026 19:58
@bbedward bbedward merged commit 8d9d7ff into AvengeMedia:master Jul 6, 2026
1 check passed
euletheia pushed a commit to euletheia/DankMaterialShell that referenced this pull request Jul 6, 2026
…ths (AvengeMedia#2761)

The notification popup's close button (X), action button clicks, and the
cardClick body's else branch all set notificationData.popup = false
directly. This relies on wrapperConn.onPopupChanged firing startExit(),
which can be interrupted by four races:

1. enterDelay (160 ms Timer) starts notificationData.timer after the
   user clicks, on an already-orphan wrapper.
2. The dismiss Timer keeps running post-click and flips popup again.
3. wrapperConn.target is set imperatively in onNotificationDataChanged
   (line 293) and may be stale after NotificationPopupManager._sync()
   reorders wrappers.
4. exiting or _isDestroying stuck true gates wrapperConn.enabled.

Introduce a single helper dismissPopupReliably() that stops the timer,
sets popup = false, and kicks off startExit() via Qt.callLater as a
belt-and-suspenders fallback. Apply to the three vulnerable handlers.

This matches the existing upstream pattern used in the hover, contextMenu,
and Component.onDestruction paths (which all stop the timer before
mutating popup state).

Closes AvengeMedia#2760
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.

Bug: Notification popup X-button click does not always dismiss the bubble

2 participants