From d43426254049e86b871b70b21417128a901e6bd6 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 10 May 2018 16:42:28 -0400 Subject: [PATCH 1/4] Remove last notification before creating new one --- js/notifications.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/js/notifications.js b/js/notifications.js index 759af602c..a6cd20a9d 100644 --- a/js/notifications.js +++ b/js/notifications.js @@ -128,6 +128,9 @@ drawAttention(); + if (this.lastNotification) { + this.lastNotification.close(); + } const notification = new Notification(title, { body: message, icon: iconUrl, From 24002149f695a08c0787d725e569cee214b99750 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 10 May 2018 17:03:06 -0400 Subject: [PATCH 2/4] Log read sync reception and remove notification --- js/read_syncs.js | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/js/read_syncs.js b/js/read_syncs.js index 5f123505a..3a248fd2e 100644 --- a/js/read_syncs.js +++ b/js/read_syncs.js @@ -24,21 +24,32 @@ message.get('source') === receipt.get('sender') ); }); - if (message) { - Whisper.Notifications.remove(message); - return message.markRead(receipt.get('read_at')).then( - function() { - this.notifyConversation(message); - this.remove(receipt); - }.bind(this) - ); - } else { - console.log( - 'No message for read sync', - receipt.get('sender'), - receipt.get('timestamp') - ); - } + const notificationForMessage = message + ? Whisper.Notifications.findWhere({ messageId: message.id }) + : null; + const removedNotification = Whisper.Notifications.remove( + notificationForMessage + ); + const receiptSender = receipt.get('sender'); + const receiptTimestamp = receipt.get('timestamp'); + const wasMessageFound = Boolean(message); + const wasNotificationFound = Boolean(notificationForMessage); + const wasNotificationRemoved = Boolean(removedNotification); + console.log('Receive read sync:', { + receiptSender, + receiptTimestamp, + wasMessageFound, + wasNotificationFound, + wasNotificationRemoved, + }); + return message + ? message.markRead(receipt.get('read_at')).then( + function() { + this.notifyConversation(message); + this.remove(receipt); + }.bind(this) + ) + : Promise.resolve(); }.bind(this) ); }, From 18a76ffb49f325239e7caed94adbd08f600a20fe Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Thu, 10 May 2018 17:07:42 -0700 Subject: [PATCH 3/4] Debounce notifications so we don't orphan them Creating/destroying notifications too quickly in testing on macOS would result in them sticking around forever, requiring manual user dismissal. We want to dismiss them for the user when we close or our window is activated. So now we debounce() calls to our notifications code. --- js/notifications.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/js/notifications.js b/js/notifications.js index a6cd20a9d..380e911de 100644 --- a/js/notifications.js +++ b/js/notifications.js @@ -8,6 +8,7 @@ /* global Signal: false */ /* global storage: false */ /* global Whisper: false */ +/* global _: false */ // eslint-disable-next-line func-names (function() { @@ -29,6 +30,13 @@ this.on('remove', this.onRemove); this.lastNotification = null; + + // Testing indicated that trying to create/destroy notifications too quickly + // resulted in notifications that stuck around forever, requiring the user + // to manually close them. This introduces a minimum amount of time between calls, + // and batches up the quick successive update() calls we get from an incoming + // read sync, which might have a number of messages referenced inside of it. + this.update = _.debounce(this.update, 1000); }, onClick(conversationId) { const conversation = ConversationController.get(conversationId); From b1a54c416fd723659566f8b52ed7c941d0cb8f00 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Thu, 10 May 2018 17:27:22 -0700 Subject: [PATCH 4/4] Notifications: All calls are debounced except for shutdown clear --- js/background.js | 2 +- js/notifications.js | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/js/background.js b/js/background.js index ff7761f74..b8a047b09 100644 --- a/js/background.js +++ b/js/background.js @@ -240,7 +240,7 @@ }); window.addEventListener('focus', () => Whisper.Notifications.clear()); - window.addEventListener('unload', () => Whisper.Notifications.clear()); + window.addEventListener('unload', () => Whisper.Notifications.fastClear()); Whisper.events.on('showConversation', function(conversation) { if (appView) { diff --git a/js/notifications.js b/js/notifications.js index 380e911de..b367c57b3 100644 --- a/js/notifications.js +++ b/js/notifications.js @@ -36,6 +36,7 @@ // to manually close them. This introduces a minimum amount of time between calls, // and batches up the quick successive update() calls we get from an incoming // read sync, which might have a number of messages referenced inside of it. + this.fastUpdate = this.update; this.update = _.debounce(this.update, 1000); }, onClick(conversationId) { @@ -43,6 +44,11 @@ this.trigger('click', conversation); }, update() { + if (this.lastNotification) { + this.lastNotification.close(); + this.lastNotification = null; + } + const { isEnabled } = this; const isAppFocused = isFocused(); const isAudioNotificationEnabled = @@ -70,7 +76,7 @@ if (status.type !== 'ok') { if (status.shouldClearNotifications) { - this.clear(); + this.reset([]); } return; @@ -136,9 +142,6 @@ drawAttention(); - if (this.lastNotification) { - this.lastNotification.close(); - } const notification = new Notification(title, { body: message, icon: iconUrl, @@ -159,18 +162,19 @@ }, onRemove() { console.log('Remove notification'); - if (this.length === 0) { - this.clear(); - } else { - this.update(); - } + this.update(); }, clear() { console.log('Remove all notifications'); - if (this.lastNotification) { - this.lastNotification.close(); - } this.reset([]); + this.update(); + }, + // We don't usually call this, but when the process is shutting down, we should at + // least try to remove the notification immediately instead of waiting for the + // normal debounce. + fastClear() { + this.reset([]); + this.fastUpdate(); }, enable() { const needUpdate = !this.isEnabled;