diff --git a/shell/plugins/notifications/NotificationLogic.js b/shell/plugins/notifications/NotificationLogic.js index b5a6f3ea..b152dc06 100644 --- a/shell/plugins/notifications/NotificationLogic.js +++ b/shell/plugins/notifications/NotificationLogic.js @@ -223,6 +223,23 @@ function popupRowChanged(row, updated) { return false } +// The same message from the same sender under a new id. A web app open in +// several tabs (HEY, Gmail, Calendar) fires one notification per tab for a +// single reminder, all within the same moment — what the user means by that +// is one toast, not a stack of identical ones. The image and click target +// count too: every screen recording toast shares its text but previews and +// opens a different file. +var DUPLICATE_ROLES = ["app", "summary", "body", "image", "execArgv"] + +function isDuplicatePopup(row, snapshot) { + if (!row || !snapshot || row.originalId === snapshot.originalId) return false + for (var i = 0; i < DUPLICATE_ROLES.length; i++) { + var role = DUPLICATE_ROLES[i] + if ((row[role] || "") !== (snapshot[role] || "")) return false + } + return true +} + // A client updating a notification through replaces_id keeps the identity of // the popup it took over: the file name is the timestamp and id the popup was // first persisted under, and the restore, replace and archive paths all key @@ -462,6 +479,7 @@ if (typeof module !== "undefined") { snapshotOf: snapshotOf, popupRoles: popupRoles, popupRowChanged: popupRowChanged, + isDuplicatePopup: isDuplicatePopup, replacementSnapshot: replacementSnapshot, historyEntry: historyEntry, parseSettings: parseSettings, diff --git a/shell/plugins/notifications/Service.qml b/shell/plugins/notifications/Service.qml index 25f5feff..cba13cbe 100644 --- a/shell/plugins/notifications/Service.qml +++ b/shell/plugins/notifications/Service.qml @@ -189,6 +189,7 @@ Item { // Repeater is mid-incubation while we mutate its model. Qt.callLater(function() { removePopupsByOriginalId(snapshot.originalId, NotificationLogic.popupFileName(snapshot)) + removeDuplicatePopups(service.currentContent(notification, snapshot)) popupModel.insert(0, snapshot) // An update that arrived while the insert was deferred found no row to // write to, and a property that already changed will not change again. @@ -310,6 +311,40 @@ Item { } } + // What the notification says now: a replaces_id update may have landed + // while its insert was deferred, and the snapshot still holds the original. + function currentContent(notification, snapshot) { + try { + return NotificationLogic.replacementSnapshot(notification, snapshot.originalId, snapshot.timestamp) + } catch (e) { + // Torn down by the server meanwhile — the snapshot is all there is. + return snapshot + } + } + + // A notification repeating a toast already on screen takes its place, the + // same way a replaces_id update would: the newest copy stays, its timer + // starts fresh, and history keeps a single entry. The superseded copy is + // dismissed at the server so its sender stops holding it open. + // Only toasts with a live notification behind them qualify: a restored or + // replayed row shares its images with an entry already in history, and + // deleting its file here would leave that entry pointing at nothing. + function removeDuplicatePopups(snapshot) { + for (var i = popupModel.count - 1; i >= 0; i--) { + var row = popupModel.get(i) + if (!NotificationLogic.isDuplicatePopup(row, snapshot) || isRestoredRow(row)) continue + var ref = liveRefs[row.originalId] + if (!ref) continue + deletePopupFileFor(row) + popupModel.remove(i) + try { + if (ref.tracked) ref.dismiss() + } catch (e) { + // Object already torn down by the server — nothing to dismiss. + } + } + } + function dismissPopup(index) { removePopup(index, "dismiss") } diff --git a/test/shell.d/notifications-test.sh b/test/shell.d/notifications-test.sh index f06e0481..48414ed4 100644 --- a/test/shell.d/notifications-test.sh +++ b/test/shell.d/notifications-test.sh @@ -360,6 +360,30 @@ assert( 'notifications ignore identity fields when deciding whether a refresh has work' ) +const heyReminder = { originalId: 20, app: 'Chromium', summary: 'Interview', body: 'Today, 11:00 AM' } +assert( + notifications.isDuplicatePopup(heyReminder, Object.assign({}, heyReminder, { originalId: 21 })), + 'notifications treat the same message from the same sender under a new id as a duplicate' +) +assert( + !notifications.isDuplicatePopup(heyReminder, heyReminder), + 'notifications leave a same-id update to the replaces_id path' +) +assert( + !notifications.isDuplicatePopup(heyReminder, Object.assign({}, heyReminder, { originalId: 21, body: 'Today, 2:00 PM' })), + 'notifications keep toasts whose body differs' +) +assert( + !notifications.isDuplicatePopup(heyReminder, Object.assign({}, heyReminder, { originalId: 21, app: 'Slack' })), + 'notifications keep identical text from a different sender' +) +assert( + !notifications.isDuplicatePopup( + { originalId: 30, app: 'omarchy-action', summary: 'Screen recording saved', body: '', image: '/tmp/a.png', execArgv: '["mpv","--","/tmp/a.mp4"]' }, + { originalId: 31, app: 'omarchy-action', summary: 'Screen recording saved', body: '', image: '/tmp/b.png', execArgv: '["mpv","--","/tmp/b.mp4"]' }), + 'notifications keep same-text toasts that preview and open different files' +) + const settings = notifications.parseSettings(JSON.stringify({ version: 3, dnd: true })) assertEqual(settings.dnd, true, 'notifications parse the persisted DND state') assertEqual(settings.legacy, false, 'notifications do not flag a current settings file as legacy') @@ -647,6 +671,14 @@ assert( /watchForUpdates\(notification, snapshot\)/.test(serviceQml), 'notifications service watches a shown notification for in-place updates' ) +assert( + /removePopupsByOriginalId\(snapshot\.originalId, [^\n]*\)\n\s*removeDuplicatePopups\(service\.currentContent\(notification, snapshot\)\)\n\s*popupModel\.insert\(0, snapshot\)/.test(serviceQml), + 'notifications service replaces an on-screen duplicate before showing the new copy' +) +assert( + /isDuplicatePopup\(row, snapshot\) \|\| isRestoredRow\(row\)\) continue\n\s*var ref = liveRefs\[row\.originalId\]\n\s*if \(!ref\) continue/.test(serviceQml), + 'notifications service only collapses duplicates of toasts still backed by a live notification' +) assert( /if \(signal && typeof signal\.connect === "function"\) signal\.connect\(refresh\)/.test(serviceQml), 'notifications service refreshes the popup from every property the card draws'