Show group names in notifications, and only show the latest

_locales/en/messages.json

@@ -1190,36 +1190,34 @@
     "description": "Label for disabling notifications"
   },
   "nameAndMessage": {
-    "message": "Both sender name and message",
+    "message": "Name, content, and actions",
     "description": "Label for setting notifications to display name and message text"
   },
   "noNameOrMessage": {
-    "message": "Neither name nor message",
+    "message": "No name or content",
     "description": "Label for setting notifications to display no name and no message text"
   },
   "nameOnly": {
-    "message": "Only sender name",
+    "message": "Name only",
     "description": "Label for setting notifications to display sender name only"
   },
   "newMessage": {
     "message": "New Message",
     "description": "Displayed in notifications for only 1 message"
   },
-  "newMessages": {
+  "notificationSenderInGroup": {
-    "message": "New Messages",
+    "message": "$sender$ in $group$",
-    "description": "Displayed in notifications for multiple messages"
+    "description": "Displayed in notifications for messages in a group",
-  },
+    "placeholders": {
-  "notificationMostRecentFrom": {
+      "sender": {
-    "message": "Most recent from:",
+        "content": "$1",
-    "description": "Displayed in notifications when setting is 'name only' and more than one message is waiting"
+        "example": "John"
-  },
+      },
-  "notificationFrom": {
+      "group": {
-    "message": "From:",
+        "content": "$1",
-    "description": "Displayed in notifications when setting is 'name only' and one message is waiting"
+        "example": "NYC Rock Climbers"
-  },
+      }
-  "notificationMostRecent": {
+    }
-    "message": "Most recent:",
-    "description": "Displayed in notifications when setting is 'name and message' and more than one message is waiting"
   },
   "notificationReaction": {
     "message": "$sender$ reacted $emoji$ to your message",
@@ -1234,19 +1232,6 @@
       }
     }
   },
-  "notificationReactionMostRecent": {
-    "message": "Most recent: $sender$ reacted $emoji$ to your message",
-    "placeholders": {
-      "sender": {
-        "content": "$1",
-        "example": "John"
-      },
-      "emoji": {
-        "content": "$2",
-        "example": "👍"
-      }
-    }
-  },
   "notificationReactionMessage": {
     "message": "$sender$ reacted $emoji$ to: $message$",
     "placeholders": {
@@ -1264,23 +1249,6 @@
       }
     }
   },
-  "notificationReactionMessageMostRecent": {
-    "message": "Most recent: $sender$ reacted $emoji$ to: $message$",
-    "placeholders": {
-      "sender": {
-        "content": "$1",
-        "example": "John"
-      },
-      "emoji": {
-        "content": "$2",
-        "example": "👍"
-      },
-      "message": {
-        "content": "$3",
-        "example": "Sounds good."
-      }
-    }
-  },
   "sendFailed": {
     "message": "Send failed",
     "description": "Shown on outgoing message if it fails to send"

js/models/conversations.js

@@ -2274,11 +2274,7 @@
       _.defaults(options, { sendReadReceipts: true });
 
       const conversationId = this.id;
-      Whisper.Notifications.remove(
+      Whisper.Notifications.removeBy({ conversationId });
-        Whisper.Notifications.where({
-          conversationId,
-        })
-      );
 
       let unreadMessages = await this.getUnread();
       const oldUnread = unreadMessages.filter(
@@ -2888,32 +2884,6 @@
 
       return null;
     },
-    getAvatar() {
-      const title = this.get('name');
-      const color = this.getColor();
-      const avatar = this.get('avatar') || this.get('profileAvatar');
-
-      if (avatar && avatar.path) {
-        return { url: getAbsoluteAttachmentPath(avatar.path), color };
-      } else if (this.isPrivate()) {
-        return {
-          color,
-          content: this.getInitials(title) || '#',
-        };
-      }
-      return { url: 'images/group_default.png', color };
-    },
-
-    getNotificationIcon() {
-      return new Promise(resolve => {
-        const avatar = this.getAvatar();
-        if (avatar.url) {
-          resolve(avatar.url);
-        } else {
-          resolve(new Whisper.IdenticonSVGView(avatar).getDataUrl());
-        }
-      });
-    },
 
     async notify(message, reaction) {
       if (this.get('muteExpiresAt') && Date.now() < this.get('muteExpiresAt')) {
@@ -2929,22 +2899,40 @@
       const sender = reaction
         ? ConversationController.get(reaction.get('fromId'))
         : message.getContact();
+      const senderName = sender ? sender.getTitle() : i18n('unknownContact');
+      const senderTitle = this.isPrivate()
+        ? senderName
+        : i18n('notificationSenderInGroup', {
+            sender: senderName,
+            group: this.getTitle(),
+          });
 
-      const iconUrl = await sender.getNotificationIcon();
+      let notificationIconUrl;
+      const avatar = this.get('avatar') || this.get('profileAvatar');
+      if (avatar && avatar.path) {
+        notificationIconUrl = getAbsoluteAttachmentPath(avatar.path);
+      } else if (this.isPrivate()) {
+        notificationIconUrl = await new Whisper.IdenticonSVGView({
+          color: this.getColor(),
+          content: this.getInitials(this.get('name')) || '#',
+        }).getDataUrl();
+      } else {
+        // Not technically needed, but helps us be explicit: we don't show an icon for a
+        //   group that doesn't have an icon.
+        notificationIconUrl = undefined;
+      }
 
       const messageJSON = message.toJSON();
-      const messageSentAt = messageJSON.sent_at;
       const messageId = message.id;
       const isExpiringMessage = Message.hasExpiration(messageJSON);
 
       Whisper.Notifications.add({
+        senderTitle,
         conversationId,
-        iconUrl,
+        notificationIconUrl,
         isExpiringMessage,
         message: message.getNotificationText(),
         messageId,
-        messageSentAt,
-        title: sender.getTitle(),
         reaction: reaction ? reaction.toJSON() : null,
       });
     },

js/models/messages.js

@@ -1450,11 +1450,7 @@
         this.set({ expirationStartTimestamp });
       }
 
-      Whisper.Notifications.remove(
+      Whisper.Notifications.removeBy({ messageId: this.id });
-        Whisper.Notifications.where({
-          messageId: this.id,
-        })
-      );
 
       if (!skipSave) {
         await window.Signal.Data.saveMessage(this.attributes, {
@@ -2924,10 +2920,7 @@
       });
 
       // Remove any notifications for this message
-      const notificationForMessage = Whisper.Notifications.findWhere({
+      Whisper.Notifications.removeBy({ messageId: this.get('id') });
-        messageId: this.get('id'),
-      });
-      Whisper.Notifications.remove(notificationForMessage);
 
       // Erase the contents of this message
       await this.eraseContents(

js/notifications.js

@@ -14,29 +14,61 @@
   window.Whisper = window.Whisper || {};
   const { Settings } = Signal.Types;
 
+  // The keys and values don't match here. This is because the values correspond to old
+  //   setting names. In the future, we may wish to migrate these to match.
   const SettingNames = {
-    COUNT: 'count',
+    NO_NAME_OR_MESSAGE: 'count',
-    NAME: 'name',
+    NAME_ONLY: 'name',
-    MESSAGE: 'message',
+    NAME_AND_MESSAGE: 'message',
   };
 
-  Whisper.Notifications = new (Backbone.Collection.extend({
+  // Electron, at least on Windows and macOS, only shows one notification at a time (see
-    initialize() {
+  //   issues [#15364][0] and [#21646][1], among others). Because of that, we have a
-      this.isEnabled = false;
+  //   single slot for notifications, and once a notification is dismissed, all of
-      this.on('add', this.update);
+  //   Signal's notifications are dismissed.
-      this.on('remove', this.onRemove);
+  // [0]: https://github.com/electron/electron/issues/15364
+  // [1]: https://github.com/electron/electron/issues/21646
+  Whisper.Notifications = {
+    ...Backbone.Events,
 
-      this.lastNotification = null;
+    isEnabled: false,
 
-      // Testing indicated that trying to create/destroy notifications too quickly
+    // This is either a standard `Notification` or null.
-      //   resulted in notifications that stuck around forever, requiring the user
+    lastNotification: null,
-      //   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
+    // This is either null or an object of this shape:
-      //   read sync, which might have a number of messages referenced inside of it.
+    //
-      this.fastUpdate = this.update;
+    //     {
-      this.update = _.debounce(this.update, 1000);
+    //       conversationId: string;
+    //       messageId: string;
+    //       senderTitle: string;
+    //       message: string;
+    //       notificationIconUrl: string | void;
+    //       isExpiringMessage: boolean;
+    //       reaction: {
+    //         emoji: string;
+    //       };
+    //     }
+    notificationData: null,
+
+    add(notificationData) {
+      this.notificationData = notificationData;
+      this.update();
     },
-    update() {
+
+    removeBy({ conversationId, messageId }) {
+      const shouldClear =
+        Boolean(this.notificationData) &&
+        ((conversationId &&
+          this.notificationData.conversationId === conversationId) ||
+          (messageId && this.notificationData.messageId === messageId));
+      if (shouldClear) {
+        this.clear();
+        this.update();
+      }
+    },
+
+    fastUpdate() {
       if (this.lastNotification) {
         this.lastNotification.close();
         this.lastNotification = null;
@@ -47,7 +79,6 @@
       const isAudioNotificationEnabled =
         storage.get('audio-notification') || false;
       const isAudioNotificationSupported = Settings.isAudioNotificationSupported();
-      const numNotifications = this.length;
       const userSetting = this.getUserSetting();
 
       const status = Signal.Notifications.getStatus({
@@ -55,101 +86,68 @@
         isAudioNotificationEnabled,
         isAudioNotificationSupported,
         isEnabled,
-        numNotifications,
+        hasNotifications: Boolean(this.notificationData),
         userSetting,
       });
 
       if (status.type !== 'ok') {
         if (status.shouldClearNotifications) {
-          this.reset([]);
+          this.notificationData = null;
         }
 
         return;
       }
 
-      let title;
+      let notificationTitle;
-      let message;
+      let notificationMessage;
-      let iconUrl;
+      let notificationIconUrl;
 
-      // NOTE: i18n has more complex rules for pluralization than just
+      const {
-      // distinguishing between zero (0) and other (non-zero),
+        conversationId,
-      // e.g. Russian:
+        messageId,
-      // http://docs.translatehouse.org/projects/localization-guide/en/latest/l10n/pluralforms.html
+        senderTitle,
-      const newMessageCountLabel = `${numNotifications} ${
+        message,
-        numNotifications === 1 ? i18n('newMessage') : i18n('newMessages')
+        isExpiringMessage,
-      }`;
+        reaction,
+      } = this.notificationData;
 
-      const last = this.last().toJSON();
+      if (
-      switch (userSetting) {
+        userSetting === SettingNames.NAME_ONLY ||
-        case SettingNames.COUNT:
+        userSetting === SettingNames.NAME_AND_MESSAGE
-          title = 'Signal';
+      ) {
-          message = newMessageCountLabel;
+        notificationTitle = senderTitle;
-          break;
+        ({ notificationIconUrl } = this.notificationData);
-        case SettingNames.NAME: {
+
-          const lastMessageTitle = last.title;
+        const shouldHideExpiringMessageBody =
-          title = newMessageCountLabel;
+          isExpiringMessage && Signal.OS.isMacOS();
-          // eslint-disable-next-line prefer-destructuring
+        if (shouldHideExpiringMessageBody) {
-          iconUrl = last.iconUrl;
+          notificationMessage = i18n('newMessage');
-          if (numNotifications === 1) {
+        } else if (userSetting === SettingNames.NAME_ONLY) {
-            if (last.reaction) {
+          if (reaction) {
-              message = i18n('notificationReaction', {
+            notificationMessage = i18n('notificationReaction', {
-                sender: lastMessageTitle,
+              sender: senderTitle,
-                emoji: last.reaction.emoji,
+              emoji: reaction.emoji,
-              });
-            } else {
-              message = `${i18n('notificationFrom')} ${lastMessageTitle}`;
-            }
-          } else if (last.reaction) {
-            message = i18n('notificationReactionMostRecent', {
-              sender: lastMessageTitle,
-              emoji: last.reaction.emoji,
             });
           } else {
-            message = `${i18n(
+            notificationMessage = i18n('newMessage');
-              'notificationMostRecentFrom'
-            )} ${lastMessageTitle}`;
           }
-          break;
+        } else if (reaction) {
+          notificationMessage = i18n('notificationReactionMessage', {
+            sender: senderTitle,
+            emoji: reaction.emoji,
+            message,
+          });
+        } else {
+          notificationMessage = message;
         }
-        case SettingNames.MESSAGE:
+      } else {
-          if (numNotifications === 1) {
+        if (userSetting !== SettingNames.NO_NAME_OR_MESSAGE) {
-            // eslint-disable-next-line prefer-destructuring
-            title = last.title;
-            if (last.reaction) {
-              message = i18n('notificationReactionMessage', {
-                sender: last.title,
-                emoji: last.reaction.emoji,
-                message: last.message,
-              });
-            } else {
-              // eslint-disable-next-line prefer-destructuring
-              message = last.message;
-            }
-          } else if (last.reaction) {
-            title = newMessageCountLabel;
-            message = i18n('notificationReactionMessageMostRecent', {
-              sender: last.title,
-              emoji: last.reaction.emoji,
-              message: last.message,
-            });
-          } else {
-            title = newMessageCountLabel;
-            message = `${i18n('notificationMostRecent')} ${last.message}`;
-          }
-          // eslint-disable-next-line prefer-destructuring
-          iconUrl = last.iconUrl;
-          break;
-        default:
           window.log.error(
             `Error: Unknown user notification setting: '${userSetting}'`
           );
-          break;
+        }
-      }
+        notificationTitle = 'Signal';
-
+        notificationMessage = i18n('newMessage');
-      const shouldHideExpiringMessageBody =
-        last.isExpiringMessage && Signal.OS.isMacOS();
-      if (shouldHideExpiringMessageBody) {
-        message = i18n('newMessage');
       }
 
       const shouldDrawAttention = storage.get(
@@ -162,38 +160,31 @@
 
       this.lastNotification = window.Signal.Services.notify({
         platform: window.platform,
-        title,
+        title: notificationTitle,
-        icon: iconUrl,
+        icon: notificationIconUrl,
-        message,
+        message: notificationMessage,
         silent: !status.shouldPlayNotificationSound,
         onNotificationClick: () => {
-          this.trigger('click', last.conversationId, last.messageId);
+          this.trigger('click', conversationId, messageId);
         },
       });
+    },
 
-      // We continue to build up more and more messages for our notifications
-      // until the user comes back to our app or closes the app. Then we’ll
-      // clear everything out. The good news is that we'll have a maximum of
-      // 1 notification in the Notification area (something like
-      // ‘10 new messages’) assuming that `Notification::close` does its job.
-    },
     getUserSetting() {
-      return storage.get('notification-setting') || SettingNames.MESSAGE;
+      return (
-    },
+        storage.get('notification-setting') || SettingNames.NAME_AND_MESSAGE
-    onRemove() {
+      );
-      window.log.info('Remove notification');
-      this.update();
     },
     clear() {
-      window.log.info('Remove all notifications');
+      window.log.info('Removing notification');
-      this.reset([]);
+      this.notificationData = null;
       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.notificationData = null;
       this.fastUpdate();
     },
     enable() {
@@ -206,5 +197,15 @@
     disable() {
       this.isEnabled = false;
     },
-  }))();
+  };
+
+  // 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.
+  Whisper.Notifications.update = _.debounce(
+    Whisper.Notifications.fastUpdate.bind(Whisper.Notifications),
+    1000
+  );
 })();

js/read_syncs.js

@@ -46,12 +46,9 @@
 
           return item.isIncoming() && senderId === receipt.get('senderId');
         });
-        const notificationForMessage = found
+        if (found) {
-          ? Whisper.Notifications.findWhere({ messageId: found.id })
+          Whisper.Notifications.removeBy({ messageId: found.id });
-          : null;
+        } else {
-        Whisper.Notifications.remove(notificationForMessage);
-
-        if (!found) {
           window.log.info(
             'No message for read sync',
             receipt.get('senderId'),

test/models/conversations_test.js

@@ -1,149 +0,0 @@
-/* global storage, textsecure, Whisper */
-
-'use strict';
-
-describe('ConversationCollection', () => {
-  textsecure.messaging = new textsecure.MessageSender('');
-
-  before(clearDatabase);
-  after(clearDatabase);
-
-  it('should be ordered newest to oldest', () => {
-    const conversations = new Whisper.ConversationCollection();
-    // Timestamps
-    const today = new Date();
-    const tomorrow = new Date();
-    tomorrow.setDate(today.getDate() + 1);
-
-    // Add convos
-    conversations.add({ timestamp: today });
-    conversations.add({ timestamp: tomorrow });
-
-    const { models } = conversations;
-    const firstTimestamp = models[0].get('timestamp').getTime();
-    const secondTimestamp = models[1].get('timestamp').getTime();
-
-    // Compare timestamps
-    assert(firstTimestamp > secondTimestamp);
-  });
-});
-
-describe('Conversation', () => {
-  const attributes = { type: 'private', id: '+18085555555' };
-  before(async () => {
-    const convo = new Whisper.ConversationCollection().add(attributes);
-    await window.Signal.Data.saveConversation(convo.attributes, {
-      Conversation: Whisper.Conversation,
-    });
-
-    const message = convo.messageCollection.add({
-      body: 'hello world',
-      conversationId: convo.id,
-      type: 'outgoing',
-      sent_at: Date.now(),
-      received_at: Date.now(),
-    });
-    await window.Signal.Data.saveMessage(message.attributes, {
-      Message: Whisper.Message,
-    });
-  });
-  after(clearDatabase);
-
-  it('sorts its contacts in an intl-friendly way', () => {
-    const convo = new Whisper.Conversation({ id: '+18085555555' });
-    convo.contactCollection.add(
-      new Whisper.Conversation({
-        name: 'C',
-      })
-    );
-    convo.contactCollection.add(
-      new Whisper.Conversation({
-        name: 'B',
-      })
-    );
-    convo.contactCollection.add(
-      new Whisper.Conversation({
-        name: 'Á',
-      })
-    );
-
-    assert.strictEqual(convo.contactCollection.at('0').get('name'), 'Á');
-    assert.strictEqual(convo.contactCollection.at('1').get('name'), 'B');
-    assert.strictEqual(convo.contactCollection.at('2').get('name'), 'C');
-  });
-
-  it('adds conversation to message collection upon leaving group', async () => {
-    const convo = new Whisper.ConversationCollection().add({
-      type: 'group',
-      id: 'a random string',
-    });
-    await convo.leaveGroup();
-    assert.notEqual(convo.messageCollection.length, 0);
-  });
-
-  it('has a title', () => {
-    const convos = new Whisper.ConversationCollection();
-    let convo = convos.add(attributes);
-    assert.equal(convo.getTitle(), '+1 808-555-5555');
-
-    convo = convos.add({ type: '' });
-    assert.equal(convo.getTitle(), 'Unknown group');
-
-    convo = convos.add({ name: 'name' });
-    assert.equal(convo.getTitle(), 'name');
-  });
-
-  it('returns the number', () => {
-    const convos = new Whisper.ConversationCollection();
-    let convo = convos.add(attributes);
-    assert.equal(convo.getNumber(), '+1 808-555-5555');
-
-    convo = convos.add({ type: '' });
-    assert.equal(convo.getNumber(), '');
-  });
-
-  it('has an avatar', () => {
-    const convo = new Whisper.ConversationCollection().add(attributes);
-    const avatar = convo.getAvatar();
-    assert.property(avatar, 'content');
-    assert.property(avatar, 'color');
-  });
-
-  describe('phone number parsing', () => {
-    after(() => {
-      storage.remove('regionCode');
-    });
-    function checkAttributes(number) {
-      const convo = new Whisper.ConversationCollection().add({
-        type: 'private',
-      });
-      convo.set('id', number);
-      convo.validate(convo.attributes);
-      assert.strictEqual(convo.get('id'), '+14155555555', number);
-    }
-    it('processes the phone number when validating', () => {
-      ['+14155555555'].forEach(checkAttributes);
-    });
-    it('defaults to the local regionCode', () => {
-      storage.put('regionCode', 'US');
-      ['14155555555', '4155555555'].forEach(checkAttributes);
-    });
-    it('works with common phone number formats', () => {
-      storage.put('regionCode', 'US');
-      [
-        '415 555 5555',
-        '415-555-5555',
-        '(415) 555 5555',
-        '(415) 555-5555',
-        '1 415 555 5555',
-        '1 415-555-5555',
-        '1 (415) 555 5555',
-        '1 (415) 555-5555',
-        '+1 415 555 5555',
-        '+1 415-555-5555',
-        '+1 (415) 555 5555',
-        '+1 (415) 555-5555',
-      ].forEach(checkAttributes);
-    });
-  });
-});

ts/notifications/getStatus.ts

@@ -3,7 +3,7 @@ interface Environment {
   isAudioNotificationEnabled: boolean;
   isAudioNotificationSupported: boolean;
   isEnabled: boolean;
-  numNotifications: number;
+  hasNotifications: boolean;
   userSetting: UserSetting;
 }
 
@@ -28,7 +28,7 @@ export const getStatus = ({
   isAudioNotificationEnabled,
   isAudioNotificationSupported,
   isEnabled,
-  numNotifications,
+  hasNotifications,
   userSetting,
 }: Environment): Status => {
   const type = ((): Type => {
@@ -36,7 +36,6 @@ export const getStatus = ({
       return 'disabled';
     }
 
-    const hasNotifications = numNotifications > 0;
     if (!hasNotifications) {
       return 'noNotifications';
     }