diff --git a/shell/plugins/notifications/NotificationLogic.js b/shell/plugins/notifications/NotificationLogic.js index 0e428641..7ea4df9e 100644 --- a/shell/plugins/notifications/NotificationLogic.js +++ b/shell/plugins/notifications/NotificationLogic.js @@ -92,6 +92,28 @@ function snapshotOf(notification, timestamp) { } } +// Everything the popup card draws, and therefore everything an in-place +// update has to write through to the row and its file. +var POPUP_ROLES = ["app", "appIcon", "summary", "body", "image", "glyph", "exec", "urgency", "expireTimeout"] + +function popupRoles() { + return POPUP_ROLES +} + +// Whether a refresh has anything to write. Each property a client updates +// emits its own signal, and the catch-up refresh after a row is inserted +// usually finds the object exactly as it was snapshotted — without this, +// one update would rewrite the file several times over. +function popupRowChanged(row, updated) { + var current = row || {} + var next = updated || {} + for (var i = 0; i < POPUP_ROLES.length; i++) { + var role = POPUP_ROLES[i] + if (current[role] !== next[role]) return true + } + return false +} + // 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 @@ -276,6 +298,8 @@ if (typeof module !== "undefined") { execFromHints: execFromHints, shouldRenderCompactGlyph: shouldRenderCompactGlyph, snapshotOf: snapshotOf, + popupRoles: popupRoles, + popupRowChanged: popupRowChanged, replacementSnapshot: replacementSnapshot, historyEntry: historyEntry, parseSettings: parseSettings, diff --git a/shell/plugins/notifications/Service.qml b/shell/plugins/notifications/Service.qml index e3245b6f..9fd00661 100644 --- a/shell/plugins/notifications/Service.qml +++ b/shell/plugins/notifications/Service.qml @@ -183,6 +183,10 @@ Item { Qt.callLater(function() { removePopupsByOriginalId(snapshot.originalId, NotificationLogic.popupFileName(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. + // Reading the object once the row exists catches up on it. + service.refreshPopup(notification, snapshot.originalId, snapshot.timestamp) }) } @@ -223,18 +227,12 @@ Item { return } + var roles = NotificationLogic.popupRoles() for (var i = 0; i < popupModel.count; i++) { var row = popupModel.get(i) if (!row || row.originalId !== originalId || row.timestamp !== timestamp) continue - popupModel.setProperty(i, "app", updated.app) - popupModel.setProperty(i, "appIcon", updated.appIcon) - popupModel.setProperty(i, "summary", updated.summary) - popupModel.setProperty(i, "body", updated.body) - popupModel.setProperty(i, "image", updated.image) - popupModel.setProperty(i, "glyph", updated.glyph) - popupModel.setProperty(i, "exec", updated.exec) - popupModel.setProperty(i, "urgency", updated.urgency) - popupModel.setProperty(i, "expireTimeout", updated.expireTimeout) + if (!NotificationLogic.popupRowChanged(row, updated)) return + for (var r = 0; r < roles.length; r++) popupModel.setProperty(i, roles[r], updated[roles[r]]) // The file name is the timestamp and id this popup was persisted under, // so the rewrite lands on the same file: a restart restores the version // last shown, and so does the copy that ends up in history. @@ -391,17 +389,37 @@ Item { // match these rows against fresh notifications. property var restoredPopups: ({}) + // Entries are either { command } for a file job or { read: true } for a + // replay's directory read. Queueing the read rather than running it beside + // the queue is what makes it a barrier: it takes its place in line, so the + // history it sees is the one that existed when the replay was asked for. + // Everything queued after it — a clear, an archive, a silenced write — waits + // for it, and no amount of later traffic can push it back. property var popupFileQueue: [] function enqueuePopupFileJob(command) { - popupFileQueue = popupFileQueue.concat([command]) + popupFileQueue = popupFileQueue.concat([{ command: command }]) + runNextPopupFileJob() + } + + function enqueueHistoryRead() { + popupFileQueue = popupFileQueue.concat([{ read: true }]) runNextPopupFileJob() } function runNextPopupFileJob() { - if (popupFileProc.running || popupFileQueue.length === 0) return - popupFileProc.command = popupFileQueue[0] + if (readHistoryProc.running || popupFileProc.running) return + if (popupFileQueue.length === 0) return + + var job = popupFileQueue[0] popupFileQueue = popupFileQueue.slice(1) + + if (job.read) { + startHistoryRead() + return + } + + popupFileProc.command = job.command popupFileProc.running = true } @@ -483,6 +501,9 @@ Item { Process { id: readHistoryProc running: false + // Let the file queue go again, whatever the read did — a failed or empty + // read must not leave archives and clears parked behind it forever. + onExited: service.runNextPopupFileJob() stdout: StdioCollector { waitForEnd: true onStreamFinished: service.replayHistory(text) @@ -494,15 +515,26 @@ Item { // by then, so they're handed over in memory instead of being waited for. property var replayCarryOver: [] - // Re-show what's in historyDir as toasts. Reading the directory is a - // subprocess, so the replay lands in replayHistory a moment later. + // Set from the moment a read is queued until it starts, so a second + // showHistory while one is still waiting its turn doesn't queue another. + property bool historyReadQueued: false + + // Re-show what's in historyDir as toasts. The read goes through the file + // queue and its own subprocess, so the replay lands in replayHistory once + // the work queued ahead of it has finished. function showRecentHistory() { - if (readHistoryProc.running) return "ok" + if (readHistoryProc.running || service.historyReadQueued) return "ok" service.replayCarryOver = liveRowsForReplay() + service.historyReadQueued = true + enqueueHistoryRead() + return "ok" + } + + function startHistoryRead() { + service.historyReadQueued = false readHistoryProc.command = ["bash", "-c", "awk 1 \"$1\"/*.json 2>/dev/null || true", "--", historyDir] readHistoryProc.running = true - return "ok" } // Copy the on-screen rows out of the model. The placeholder from an earlier diff --git a/test/shell.d/notifications-test.sh b/test/shell.d/notifications-test.sh index bd59750b..90a6c186 100644 --- a/test/shell.d/notifications-test.sh +++ b/test/shell.d/notifications-test.sh @@ -170,6 +170,18 @@ assertEqual( '12345-12.json', 'notifications keep the persisted file name across an in-place update' ) +assert( + !notifications.popupRowChanged(replacement, replacement), + 'notifications skip a refresh that matches the row it would write' +) +assert( + notifications.popupRowChanged(replacement, Object.assign({}, replacement, { body: 'message 3' })), + 'notifications refresh a row whose content moved on' +) +assert( + !notifications.popupRowChanged(replacement, Object.assign({}, replacement, { timestamp: 999 })), + 'notifications ignore identity fields when deciding whether a refresh has work' +) const settings = notifications.parseSettings(JSON.stringify({ version: 3, dnd: true })) assertEqual(settings.dnd, true, 'notifications parse the persisted DND state') @@ -371,9 +383,33 @@ assert( 'notifications service refreshes the popup from every property the card draws' ) assert( - /popupModel\.setProperty\(i, "summary", updated\.summary\)[\s\S]{0,600}?persistPopupFile\(updated\)/.test(serviceQml), + /popupModel\.setProperty\(i, roles\[r\], updated\[roles\[r\]\]\)[\s\S]{0,600}?persistPopupFile\(updated\)/.test(serviceQml), 'notifications service rewrites both the row and its file when a notification is updated in place' ) +assert( + /if \(!NotificationLogic\.popupRowChanged\(row, updated\)\) return/.test(serviceQml), + 'notifications service leaves the row and its file alone when a refresh finds nothing changed' +) +assert( + /popupModel\.insert\(0, snapshot\)[\s\S]{0,300}?service\.refreshPopup\(notification, snapshot\.originalId, snapshot\.timestamp\)/.test(serviceQml), + 'notifications service catches up on an update that beat the deferred row insert' +) +assert( + /function showRecentHistory\(\)[\s\S]{0,300}?enqueueHistoryRead\(\)/.test(serviceQml), + 'notifications service reads history from its place in the file queue' +) +assert( + /if \(job\.read\) \{\s*\n\s*startHistoryRead\(\)/.test(serviceQml), + 'notifications service runs the queued read when its turn comes' +) +assert( + /function runNextPopupFileJob\(\) \{\s*\n\s*if \(readHistoryProc\.running \|\| popupFileProc\.running\) return/.test(serviceQml), + 'notifications service holds queued file work until a history read finishes' +) +assert( + /id: readHistoryProc[\s\S]{0,300}?onExited: service\.runNextPopupFileJob\(\)/.test(serviceQml), + 'notifications service releases the file queue even when a history read comes back empty' +) assert( /onSummaryChanged: cardSlot\.remainingLifetime = 1\.0/.test(serviceQml), 'notifications service restarts the countdown when a toast is updated under it'