From 91a591c6ca93938a85229183110082df3605d4f7 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Fri, 4 Sep 2020 18:07:24 -0500 Subject: [PATCH] Show group names in notifications, and only show the latest --- _locales/en/messages.json | 64 +++------ js/models/conversations.js | 60 ++++---- js/models/messages.js | 11 +- js/notifications.js | 227 +++++++++++++++--------------- js/read_syncs.js | 9 +- test/models/conversations_test.js | 149 -------------------- ts/notifications/getStatus.ts | 5 +- 7 files changed, 161 insertions(+), 364 deletions(-) delete mode 100644 test/models/conversations_test.js diff --git a/_locales/en/messages.json b/_locales/en/messages.json index 037fe68563..181fe1fba5 100644 --- a/_locales/en/messages.json +++ b/_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": { - "message": "New Messages", - "description": "Displayed in notifications for multiple messages" - }, - "notificationMostRecentFrom": { - "message": "Most recent from:", - "description": "Displayed in notifications when setting is 'name only' and more than one message is waiting" - }, - "notificationFrom": { - "message": "From:", - "description": "Displayed in notifications when setting is 'name only' and one message is waiting" - }, - "notificationMostRecent": { - "message": "Most recent:", - "description": "Displayed in notifications when setting is 'name and message' and more than one message is waiting" + "notificationSenderInGroup": { + "message": "$sender$ in $group$", + "description": "Displayed in notifications for messages in a group", + "placeholders": { + "sender": { + "content": "$1", + "example": "John" + }, + "group": { + "content": "$1", + "example": "NYC Rock Climbers" + } + } }, "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" diff --git a/js/models/conversations.js b/js/models/conversations.js index dae198687b..b47daf55e2 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -2274,11 +2274,7 @@ _.defaults(options, { sendReadReceipts: true }); const conversationId = this.id; - Whisper.Notifications.remove( - Whisper.Notifications.where({ - conversationId, - }) - ); + Whisper.Notifications.removeBy({ 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, }); }, diff --git a/js/models/messages.js b/js/models/messages.js index a1a5a06ae6..e18bb525f8 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -1450,11 +1450,7 @@ this.set({ expirationStartTimestamp }); } - Whisper.Notifications.remove( - Whisper.Notifications.where({ - messageId: this.id, - }) - ); + Whisper.Notifications.removeBy({ 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({ - messageId: this.get('id'), - }); - Whisper.Notifications.remove(notificationForMessage); + Whisper.Notifications.removeBy({ messageId: this.get('id') }); // Erase the contents of this message await this.eraseContents( diff --git a/js/notifications.js b/js/notifications.js index 36c8a2080a..0cba0bce95 100644 --- a/js/notifications.js +++ b/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', - NAME: 'name', - MESSAGE: 'message', + NO_NAME_OR_MESSAGE: 'count', + NAME_ONLY: 'name', + NAME_AND_MESSAGE: 'message', }; - Whisper.Notifications = new (Backbone.Collection.extend({ - initialize() { - this.isEnabled = false; - this.on('add', this.update); - this.on('remove', this.onRemove); + // Electron, at least on Windows and macOS, only shows one notification at a time (see + // issues [#15364][0] and [#21646][1], among others). Because of that, we have a + // single slot for notifications, and once a notification is dismissed, all of + // Signal's notifications are dismissed. + // [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 - // 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.fastUpdate = this.update; - this.update = _.debounce(this.update, 1000); + // This is either a standard `Notification` or null. + lastNotification: null, + + // This is either null or an object of this shape: + // + // { + // 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 message; - let iconUrl; + let notificationTitle; + let notificationMessage; + let notificationIconUrl; - // NOTE: i18n has more complex rules for pluralization than just - // distinguishing between zero (0) and other (non-zero), - // e.g. Russian: - // http://docs.translatehouse.org/projects/localization-guide/en/latest/l10n/pluralforms.html - const newMessageCountLabel = `${numNotifications} ${ - numNotifications === 1 ? i18n('newMessage') : i18n('newMessages') - }`; + const { + conversationId, + messageId, + senderTitle, + message, + isExpiringMessage, + reaction, + } = this.notificationData; - const last = this.last().toJSON(); - switch (userSetting) { - case SettingNames.COUNT: - title = 'Signal'; - message = newMessageCountLabel; - break; - case SettingNames.NAME: { - const lastMessageTitle = last.title; - title = newMessageCountLabel; - // eslint-disable-next-line prefer-destructuring - iconUrl = last.iconUrl; - if (numNotifications === 1) { - if (last.reaction) { - message = i18n('notificationReaction', { - sender: lastMessageTitle, - emoji: last.reaction.emoji, - }); - } else { - message = `${i18n('notificationFrom')} ${lastMessageTitle}`; - } - } else if (last.reaction) { - message = i18n('notificationReactionMostRecent', { - sender: lastMessageTitle, - emoji: last.reaction.emoji, + if ( + userSetting === SettingNames.NAME_ONLY || + userSetting === SettingNames.NAME_AND_MESSAGE + ) { + notificationTitle = senderTitle; + ({ notificationIconUrl } = this.notificationData); + + const shouldHideExpiringMessageBody = + isExpiringMessage && Signal.OS.isMacOS(); + if (shouldHideExpiringMessageBody) { + notificationMessage = i18n('newMessage'); + } else if (userSetting === SettingNames.NAME_ONLY) { + if (reaction) { + notificationMessage = i18n('notificationReaction', { + sender: senderTitle, + emoji: reaction.emoji, }); } else { - message = `${i18n( - 'notificationMostRecentFrom' - )} ${lastMessageTitle}`; + notificationMessage = i18n('newMessage'); } - break; + } else if (reaction) { + notificationMessage = i18n('notificationReactionMessage', { + sender: senderTitle, + emoji: reaction.emoji, + message, + }); + } else { + notificationMessage = message; } - case SettingNames.MESSAGE: - if (numNotifications === 1) { - // 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: + } else { + if (userSetting !== SettingNames.NO_NAME_OR_MESSAGE) { window.log.error( `Error: Unknown user notification setting: '${userSetting}'` ); - break; - } - - const shouldHideExpiringMessageBody = - last.isExpiringMessage && Signal.OS.isMacOS(); - if (shouldHideExpiringMessageBody) { - message = i18n('newMessage'); + } + notificationTitle = 'Signal'; + notificationMessage = i18n('newMessage'); } const shouldDrawAttention = storage.get( @@ -162,38 +160,31 @@ this.lastNotification = window.Signal.Services.notify({ platform: window.platform, - title, - icon: iconUrl, - message, + title: notificationTitle, + icon: notificationIconUrl, + 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; - }, - onRemove() { - window.log.info('Remove notification'); - this.update(); + return ( + storage.get('notification-setting') || SettingNames.NAME_AND_MESSAGE + ); }, clear() { - window.log.info('Remove all notifications'); - this.reset([]); + window.log.info('Removing notification'); + 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 + ); })(); diff --git a/js/read_syncs.js b/js/read_syncs.js index bdb9e89ef8..39cad32bea 100644 --- a/js/read_syncs.js +++ b/js/read_syncs.js @@ -46,12 +46,9 @@ return item.isIncoming() && senderId === receipt.get('senderId'); }); - const notificationForMessage = found - ? Whisper.Notifications.findWhere({ messageId: found.id }) - : null; - Whisper.Notifications.remove(notificationForMessage); - - if (!found) { + if (found) { + Whisper.Notifications.removeBy({ messageId: found.id }); + } else { window.log.info( 'No message for read sync', receipt.get('senderId'), diff --git a/test/models/conversations_test.js b/test/models/conversations_test.js deleted file mode 100644 index 4f12b100fd..0000000000 --- a/test/models/conversations_test.js +++ /dev/null @@ -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); - }); - }); -}); diff --git a/ts/notifications/getStatus.ts b/ts/notifications/getStatus.ts index 7c693435c4..75faaefad8 100644 --- a/ts/notifications/getStatus.ts +++ b/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'; }