Only show notifications when done with sync #1507

scottnonnenberg · 2017-09-26T01:19:45Z

Instead of adding OS notifications immediately when we find out about a new unread message, we wait until we get the 'empty' event from MessageReceiver. This tells us that our queue is empty.

This then prevents the parade of notifications if a machine wakes up from sleep. Basically covers situations that the loading screen doesn't already.

liliakai · 2017-09-27T15:57:13Z

Seems like we can clean this up a bit by adding some global state toggle, i.e., Whisper.Notifications.<enable|disable>(), which is disabled by background.js during initial load/reconnect, and enabled by an onEmpty event. Then we can continue calling update on any add, and have update bail early if notifications are disabled, in much the same way that it does when the notification setting is turned off. This would let us drop the options from handleDataMessage...

Also there's a UX change here: we will only display a notification for the last* in a chunk of messages... On OSX that means we won't be able to browse all the unreads in the notification manager, nor see the nice os-level popup coalescing that occurs on the lock screen, i.e., "Signal > 10 new messages". This is not critical but kind of a bummer imho. :\

*Also sometimes it's not the last but the second to last, due to race between the empty event and the processing of an incoming message into a notification... See screenshot for example of receiving 4 messages upon connection, but only creating 1 notification, which is for the second-to-last message.

liliakai · 2017-09-27T16:07:29Z

@scottnonnenberg I'd be happy to pick up this branch where you left off if you like, just say the word. I have a couple ideas ideas to address the issues mentioned above...

scottnonnenberg · 2017-09-28T19:02:55Z

js/notifications.js

@@ -12,38 +12,27 @@
 MESSAGE : 'message'
 };

+ var enabled = false;


Why not make this a member of Whisper.Notifications? Set this.enabled = false in initialize() below?

Hmm guess I was just being paranoid about allowing the state to be modified directly rather than through the explicit enable/disable functions. 🤷‍♀️

scottnonnenberg · 2017-09-28T19:03:52Z

js/notifications.js

 }
- if (window.isFocused()) {


Without this check, we'll show notifications when they come in after the 'empty' event, even if the window is focused. Or am I missing something?

Hmm.. looks like there's a focus check in conversation.notify that accounts for that case, but it probably makes sense to move that check here. Will revise.

scottnonnenberg · 2017-09-28T20:18:31Z

js/notifications.js

@@ -35,6 +35,11 @@
 if (this.length === 0) {
 return;
 }
+ if (window.isFocused()) {


I like that we're only doing this check in one place.

However, it does mean that after the 'empty' event, when the window is focused, every incoming unread message is added to the notifications collection then immediately removed. GIven that conversation.notify() does a ConversationController.getOrCreateAndWait(), perhaps it makes sense to add the isFocused() call back to prevent this extra unnecessary work?

With that extra check, before the empty event, notifications won't be added to the collection if the window happens to be in focus when we process that message (possibly while the loading screen is still up). For a large backlog of messages that might mean an inaccurate count when we finally get the empty event and display the notification.

getOrCreateAndWait should be pretty lightweight given the initial contact fetch on startup, no?

Good point. The "in-focus but not in-focus when we're finally ready to show the alert" is a key scenario I hadn't considered.

I think these changes are ready, just need a quick rebase and we can merge! :0)

Ah, but there is one more wrinkle - in that case, where we're in focus now but we want to save the message for a later 'empty' event - that windows.isFocused() check will clear all current notifications. So we should probably remove the clear() call there.

Well bail at the if !enabled check in that case.

btw, I added the clear() there so that we don't amass a backlog of stale notifications for messages that arrive while the window is in focus. Otherwise, the next message to arrive while out of focus would include the backlog.

scottnonnenberg · 2017-09-29T01:18:10Z

A squash will eliminate that merge conflict, since the conflict is with since-eliminated bits.

This prevents the parade of notifications if a machine wakes up from sleep. Basically covers situations that the loading screen doesn't already. When disabled, notifications will be cached until they are subsequently re-enabled, at which time all the pending notifications will be summarized. From the background page, notifications are disabled during connection attempts until an empty event. This means we can always safely call conversation.notify to queue a notification for the next batch, dropping some options from message and conversation model methods. We've also moved the calls to check window focus and draw attention to the window, which were previously included in the conversation model, but are now performed by the Notification system, because the time that the notification is displayed might be some time after the message is added by the conversation, so decisions about focus and attention should be made in that moment and not before. // FREEBIE

liliakai · 2017-09-29T12:14:08Z

Rebased and squashed.

liliakai · 2017-09-29T12:15:08Z

🤦‍♀️ oops wrong button

Make long-lived socket connections more reliable (#1511) Show offline state faster on loss of network access (#1512) Notifications: - Only show notifications when a large backlog download is complete (#1507) - Ensure final message before 'empty' is ready for notification (#1522) Ensure we always replace $name$ variable in strings (#1520) Update strings for fa, no, pt_BR, pt_PT, ro, zh_CN, zh_TW (#1517) Update electron to v1.6.14 to get security fix (#1519) Eliminate warning on Windows installation with code-signing (#1513) Dev: - Move logging to disk via bunyan - should make message processing faster! (#1506) - Only retry messages on startup, not every reconnect (#1510) - Log call messages instead of throwing error (#1504) - Redact group ids in logging (4c48d12) - Remove manifest.json from project (3a3f249) FREEBIE

scottnonnenberg mentioned this pull request Sep 27, 2017

Only retry messages on startup, not every sockect reconnect #1510

Merged

liliakai force-pushed the notification-on-empty branch from 77115ba to 298f041 Compare September 28, 2017 17:27

scottnonnenberg commented Sep 28, 2017

View reviewed changes

liliakai force-pushed the notification-on-empty branch from 45e154f to 6d86f19 Compare September 29, 2017 12:09

liliakai closed this Sep 29, 2017

liliakai reopened this Sep 29, 2017

scottnonnenberg merged commit 10a3829 into master Sep 29, 2017

scottnonnenberg deleted the notification-on-empty branch September 29, 2017 16:15

scottnonnenberg mentioned this pull request Sep 29, 2017

Eliminate loading screen, load new messages in the background #1442

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only show notifications when done with sync #1507

Only show notifications when done with sync #1507

scottnonnenberg commented Sep 26, 2017

liliakai commented Sep 27, 2017

liliakai commented Sep 27, 2017

scottnonnenberg Sep 28, 2017

liliakai Sep 28, 2017

scottnonnenberg Sep 28, 2017

liliakai Sep 28, 2017

scottnonnenberg Sep 28, 2017 •

edited

Loading

liliakai Sep 28, 2017

scottnonnenberg Sep 28, 2017

scottnonnenberg Sep 28, 2017

liliakai Sep 29, 2017

liliakai Sep 29, 2017

scottnonnenberg commented Sep 29, 2017

liliakai commented Sep 29, 2017

liliakai commented Sep 29, 2017

Only show notifications when done with sync #1507

Only show notifications when done with sync #1507

Conversation

scottnonnenberg commented Sep 26, 2017

liliakai commented Sep 27, 2017

liliakai commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottnonnenberg Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottnonnenberg commented Sep 29, 2017

liliakai commented Sep 29, 2017

liliakai commented Sep 29, 2017

scottnonnenberg Sep 28, 2017 •

edited

Loading