From 86a87e26b8a2742dbacad714087b1f9cd72466c2 Mon Sep 17 00:00:00 2001 From: Felix Hammerl Date: Tue, 16 Sep 2014 19:01:49 +0200 Subject: [PATCH] [WO-565] Improve notifications * Introduce 2 sec timeout for sent notifications * Notify only for new messages in the inbox * Close pending notes when a msg is marked unread in the inbox --- .jshintrc | 1 + src/js/controller/mail-list.js | 64 +++++++++++++++---------- src/js/controller/navigation.js | 4 +- src/js/dao/email-dao.js | 6 ++- src/js/util/notification.js | 56 +++++++++++++++++----- test/integration/email-dao-test.js | 6 +-- test/unit/email-dao-test.js | 21 ++++++++ test/unit/mail-list-ctrl-test.js | 77 ++++++++++++------------------ 8 files changed, 143 insertions(+), 92 deletions(-) diff --git a/.jshintrc b/.jshintrc index 15b5482..c6b8a83 100644 --- a/.jshintrc +++ b/.jshintrc @@ -19,6 +19,7 @@ "predef": [ "console", + "Notification", "importScripts", "process", "Event", diff --git a/src/js/controller/mail-list.js b/src/js/controller/mail-list.js index 1e862cf..193eb65 100644 --- a/src/js/controller/mail-list.js +++ b/src/js/controller/mail-list.js @@ -8,7 +8,8 @@ define(function(require) { emailDao, outboxBo, keychainDao, searchTimeout, firstSelect; var INIT_DISPLAY_LEN = 20, - SCROLL_DISPLAY_LEN = 10; + SCROLL_DISPLAY_LEN = 10, + FOLDER_TYPE_INBOX = 'Inbox'; var MailListCtrl = function($scope, $routeParams) { // @@ -19,6 +20,11 @@ define(function(require) { outboxBo = appController._outboxBo; keychainDao = appController._keychain; + /** + * Gathers unread notifications to be cancelled later + */ + $scope.pendingNotifications = []; + // // scope functions // @@ -80,6 +86,13 @@ define(function(require) { return; } + // let's close pending notifications for unread messages in the inbox + if (currentFolder().type === FOLDER_TYPE_INBOX) { + while ($scope.pendingNotifications.length) { + notification.close($scope.pendingNotifications.shift()); + } + } + $scope.toggleUnread(email); } }; @@ -364,7 +377,7 @@ define(function(require) { // (emailDao || {}).onIncomingMessage = function(msgs) { - var popupId, popupTitle, popupMessage, unreadMsgs; + var note, title, message, unreadMsgs; unreadMsgs = msgs.filter(function(msg) { return msg.unread; @@ -374,34 +387,35 @@ define(function(require) { return; } - popupId = '' + unreadMsgs[0].uid; - if (unreadMsgs.length > 1) { - popupTitle = unreadMsgs.length + ' new messages'; - popupMessage = _.pluck(unreadMsgs, 'subject').join('\n'); + if (unreadMsgs.length === 1) { + title = unreadMsgs[0].from[0].name || unreadMsgs[0].from[0].address; + message = unreadMsgs[0].subject; } else { - popupTitle = unreadMsgs[0].from[0].name || unreadMsgs[0].from[0].address; - popupMessage = unreadMsgs[0].subject; + title = unreadMsgs.length + ' new messages'; + message = _.pluck(unreadMsgs, 'subject').join('\n'); } - notification.create({ - id: popupId, - title: popupTitle, - message: popupMessage + note = notification.create({ + title: title, + message: message, + onClick: function() { + // force toggle into read mode when notification is clicked + firstSelect = false; + + // remove from pending notificatiosn + var index = $scope.pendingNotifications.indexOf(note); + if (index !== -1) { + $scope.pendingNotifications.splice(index, 1); + } + + // mark message as read + $scope.select(_.findWhere(currentFolder().messages, { + uid: unreadMsgs[0].uid + })); + } }); + $scope.pendingNotifications.push(note); }; - - notification.setOnClickedListener(function(uidString) { - var uid = parseInt(uidString, 10); - - if (isNaN(uid)) { - return; - } - - firstSelect = false; - $scope.select(_.findWhere(currentFolder().messages, { - uid: uid - })); - }); }; // diff --git a/src/js/controller/navigation.js b/src/js/controller/navigation.js index 54aae00..11f2041 100644 --- a/src/js/controller/navigation.js +++ b/src/js/controller/navigation.js @@ -106,9 +106,9 @@ define(function(require) { function sentNotification(email) { notification.create({ - id: 'o' + email.id, title: 'Message sent', - message: email.subject + message: email.subject, + timeout: 2000 }, function() {}); } diff --git a/src/js/dao/email-dao.js b/src/js/dao/email-dao.js index 230e111..b6ceb2f 100644 --- a/src/js/dao/email-dao.js +++ b/src/js/dao/email-dao.js @@ -408,7 +408,11 @@ define(function(require) { [].unshift.apply(folder.messages, messages); // add the new messages to the folder updateUnreadCount(folder); // update the unread count - self.onIncomingMessage(messages); // notify about new messages + + // notify about new messages only for the inbox + if (folder.type === FOLDER_TYPE_INBOX) { + self.onIncomingMessage(messages); + } done(); }); } diff --git a/src/js/util/notification.js b/src/js/util/notification.js index eec10aa..187be3e 100644 --- a/src/js/util/notification.js +++ b/src/js/util/notification.js @@ -5,22 +5,52 @@ define(function(require) { var self = {}; - self.create = function(options, callback) { - callback = callback || function() {}; - if (window.chrome && chrome.notifications) { - chrome.notifications.create(options.id, { - type: 'basic', - title: options.title, - message: options.message, - iconUrl: chrome.runtime.getURL(cfg.iconPath) - }, callback); + if (window.Notification) { + self.hasPermission = Notification.permission === "granted"; + } + + /** + * Creates a notification. Requests permission if not already granted + * + * @param {String} options.title The notification title + * @param {String} options.message The notification message + * @param {Number} options.timeout (optional) Timeout when the notification is closed in milliseconds + * @param {Function} options.onClick (optional) callback when the notification is clicked + * @returns {Notification} A notification instance + */ + self.create = function(options) { + options.onClick = options.onClick || function() {}; + + if (!window.Notification) { + return; } + + if (!self.hasPermission) { + // don't wait until callback returns + Notification.requestPermission(function(permission) { + if (permission === "granted") { + self.hasPermission = true; + } + }); + } + + var notification = new Notification(options.title, { + body: options.message, + icon: cfg.iconPath + }); + notification.onclick = options.onClick; + + if (options.timeout > 0) { + setTimeout(function() { + notification.close(); + }, options.timeout); + } + + return notification; }; - self.setOnClickedListener = function(listener) { - if (window.chrome && chrome.notifications) { - chrome.notifications.onClicked.addListener(listener); - } + self.close = function(notification) { + notification.close(); }; return self; diff --git a/test/integration/email-dao-test.js b/test/integration/email-dao-test.js index 7bac5c8..ad2b952 100644 --- a/test/integration/email-dao-test.js +++ b/test/integration/email-dao-test.js @@ -367,11 +367,7 @@ define(function(require) { }, function(err, folder) { expect(err).to.not.exist; expect(folder.exists).to.equal(1); - emailDao.onIncomingMessage = function(messages) { - expect(messages.length).to.equal(1); - expect(messages[0].id).to.equal('b'); - done(); - }; + done(); }); }); }); diff --git a/test/unit/email-dao-test.js b/test/unit/email-dao-test.js index ef063dc..a5ac9c6 100644 --- a/test/unit/email-dao-test.js +++ b/test/unit/email-dao-test.js @@ -444,6 +444,27 @@ define(function(require) { }); }); + it('should not notify for other folders', function(done) { + opts.folder = sentFolder; + + imapListStub.withArgs(opts).yieldsAsync(null, [message]); + + localStoreStub.withArgs({ + folder: sentFolder, + emails: [message] + }).yieldsAsync(); + + dao.fetchMessages(opts, function(err) { + expect(err).to.not.exist; + expect(sentFolder.messages).to.contain(message); + expect(notified).to.be.false; + expect(localStoreStub.calledOnce).to.be.true; + expect(imapListStub.calledOnce).to.be.true; + + done(); + }); + }); + it('should verify verification mails', function(done) { message.subject = verificationSubject; diff --git a/test/unit/mail-list-ctrl-test.js b/test/unit/mail-list-ctrl-test.js index f83ad78..7dab559 100644 --- a/test/unit/mail-list-ctrl-test.js +++ b/test/unit/mail-list-ctrl-test.js @@ -15,7 +15,7 @@ define(function(require) { describe('Mail List controller unit test', function() { var scope, ctrl, origEmailDao, emailDaoMock, keychainMock, deviceStorageMock, - emailAddress, notificationClickedHandler, emails, + emailAddress, emails, hasChrome, hasSocket, hasRuntime, hasIdentity; beforeEach(function() { @@ -37,10 +37,6 @@ define(function(require) { window.chrome.identity = {}; } - sinon.stub(notification, 'setOnClickedListener', function(func) { - notificationClickedHandler = func; - }); - emails = [{ unread: true }, { @@ -86,8 +82,6 @@ define(function(require) { }); afterEach(function() { - notification.setOnClickedListener.restore(); - if (!hasSocket) { delete window.chrome.socket; } @@ -260,6 +254,10 @@ define(function(require) { scope._stopWatchTask(); }); + afterEach(function() { + notification.create.restore(); + }); + it('should succeed for single mail', function(done) { var mail = { uid: 123, @@ -271,14 +269,21 @@ define(function(require) { }; sinon.stub(notification, 'create', function(opts) { - expect(opts.id).to.equal('' + mail.uid); expect(opts.title).to.equal(mail.from[0].address); expect(opts.message).to.equal(mail.subject); - notification.create.restore(); + opts.onClick(); + expect(scope.state.mailList.selected).to.equal(mail); done(); }); + scope.state.nav = { + currentFolder: { + type: 'asd', + messages: [mail] + } + }; + emailDaoMock.onIncomingMessage([mail]); }); @@ -307,50 +312,23 @@ define(function(require) { }]; sinon.stub(notification, 'create', function(opts) { - expect(opts.id).to.equal('' + mails[0].uid); expect(opts.title).to.equal('2 new messages'); expect(opts.message).to.equal(mails[0].subject + '\n' + mails[1].subject); - notification.create.restore(); + opts.onClick(); + expect(scope.state.mailList.selected).to.equal(mails[0]); done(); }); + scope.state.nav = { + currentFolder: { + type: 'asd', + messages: mails + } + }; + emailDaoMock.onIncomingMessage(mails); }); - - it('should focus mail when clicked', function() { - var mail = { - uid: 123, - from: [{ - address: 'asd' - }], - subject: 'asdasd', - unread: true - }; - - scope.state.nav = { - currentFolder: { - type: 'asd', - messages: [mail] - } - }; - - notificationClickedHandler('123'); - expect(scope.state.mailList.selected).to.equal(mail); - }); - - it('should not change focus mail when popup id is NaN', function() { - scope.state.nav = { - currentFolder: { - type: 'asd', - messages: [] - } - }; - var focus = scope.state.mailList.selected = {}; - - notificationClickedHandler(''); - expect(scope.state.mailList.selected).to.equal(focus); - }); }); describe('getBody', function() { @@ -368,6 +346,9 @@ define(function(require) { describe('select', function() { it('should decrypt, focus mark an unread mail as read', function() { + scope.pendingNotifications = ['asd']; + sinon.stub(notification, 'close'); + var mail = { from: [{ address: 'asd' @@ -377,7 +358,7 @@ define(function(require) { scope.state = { nav: { currentFolder: { - type: 'asd' + type: 'Inbox' } }, mailList: {}, @@ -393,6 +374,10 @@ define(function(require) { expect(emailDaoMock.decryptBody.calledOnce).to.be.true; expect(keychainMock.refreshKeyForUserId.calledOnce).to.be.true; expect(scope.state.mailList.selected).to.equal(mail); + expect(notification.close.calledWith('asd')).to.be.true; + expect(notification.close.calledOnce).to.be.true; + + notification.close.restore(); }); it('should decrypt and focus a read mail', function() {