From 849ef418a66689f394be4b6a57413bee8e39ce9d Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Sat, 28 Nov 2020 12:02:26 +0000 Subject: [PATCH] Desktop: Fixed notifications on macOS --- packages/app-desktop/package-lock.json | 45 ++++--- packages/app-desktop/package.json | 6 +- packages/lib/models/Setting.ts | 6 + .../lib/services/AlarmServiceDriverNode.ts | 113 +++++++++++++++--- 4 files changed, 132 insertions(+), 38 deletions(-) diff --git a/packages/app-desktop/package-lock.json b/packages/app-desktop/package-lock.json index dd7d6e61aa4..67520780a87 100644 --- a/packages/app-desktop/package-lock.json +++ b/packages/app-desktop/package-lock.json @@ -8828,9 +8828,7 @@ "is-docker": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/is-docker/-/is-docker-2.1.1.tgz", - "integrity": "sha512-ZOoqiXfEwtGknTiuDEy8pN2CfE3TxMHprvNer1mXiqwkOT77Rw3YVrUQ52EqAOU3QAWDQ+bQdx7HJzrv7LS2Hw==", - "dev": true, - "optional": true + "integrity": "sha512-ZOoqiXfEwtGknTiuDEy8pN2CfE3TxMHprvNer1mXiqwkOT77Rw3YVrUQ52EqAOU3QAWDQ+bQdx7HJzrv7LS2Hw==" }, "is-dotfile": { "version": "1.0.3", @@ -9021,9 +9019,12 @@ "dev": true }, "is-wsl": { - "version": "2.1.1", - "resolved": "https://registry.npmjs.org/is-wsl/-/is-wsl-2.1.1.tgz", - "integrity": "sha512-umZHcSrwlDHo2TGMXv0DZ8dIUGunZ2Iv68YZnrmCiBPkZ4aaOhtv7pXJKeki9k3qJ3RJr0cDyitcl5wEH3AYog==" + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/is-wsl/-/is-wsl-2.2.0.tgz", + "integrity": "sha512-fKzAra0rGJUUBwGBgNkHZuToZcn+TtXHpeCgmkMJMMYx1sQDYaCSyjJBSCa2nH1DGm7s3n1oBnohoVTBaN7Lww==", + "requires": { + "is-docker": "^2.0.0" + } }, "is-yarn-global": { "version": "0.3.0", @@ -11707,21 +11708,30 @@ "dev": true }, "node-notifier": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/node-notifier/-/node-notifier-6.0.0.tgz", - "integrity": "sha512-SVfQ/wMw+DesunOm5cKqr6yDcvUTDl/yc97ybGHMrteNEY6oekXpNpS3lZwgLlwz0FLgHoiW28ZpmBHUDg37cw==", + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/node-notifier/-/node-notifier-8.0.0.tgz", + "integrity": "sha512-46z7DUmcjoYdaWyXouuFNNfUo6eFa94t23c53c+lG/9Cvauk4a98rAUp9672X5dxGdQmLpPzTxzu8f/OeEPaFA==", "requires": { "growly": "^1.3.0", - "is-wsl": "^2.1.1", - "semver": "^6.3.0", + "is-wsl": "^2.2.0", + "semver": "^7.3.2", "shellwords": "^0.1.1", - "which": "^1.3.1" + "uuid": "^8.3.0", + "which": "^2.0.2" }, "dependencies": { "semver": { - "version": "6.3.0", - "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.0.tgz", - "integrity": "sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw==" + "version": "7.3.2", + "resolved": "https://registry.npmjs.org/semver/-/semver-7.3.2.tgz", + "integrity": "sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ==" + }, + "which": { + "version": "2.0.2", + "resolved": "https://registry.npmjs.org/which/-/which-2.0.2.tgz", + "integrity": "sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA==", + "requires": { + "isexe": "^2.0.0" + } } } }, @@ -14984,9 +14994,7 @@ "uuid": { "version": "8.3.1", "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.1.tgz", - "integrity": "sha512-FOmRr+FmWEIG8uhZv6C2bTgEVXsHk08kE7mPlrBbEe+c3r9pjceVPgupIfNIhc4yx55H69OXANrUaSuu9eInKg==", - "dev": true, - "optional": true + "integrity": "sha512-FOmRr+FmWEIG8uhZv6C2bTgEVXsHk08kE7mPlrBbEe+c3r9pjceVPgupIfNIhc4yx55H69OXANrUaSuu9eInKg==" }, "v8-to-istanbul": { "version": "7.0.0", @@ -15202,6 +15210,7 @@ "version": "1.3.1", "resolved": "https://registry.npmjs.org/which/-/which-1.3.1.tgz", "integrity": "sha512-HxJdYWq1MTIQbJ3nw0cqssHoTNU267KlrDuGZ1WYlxDStUtKUhOaJmh112/TZmHxxUfuJqPXSOm7tDyas0OSIQ==", + "dev": true, "requires": { "isexe": "^2.0.0" } diff --git a/packages/app-desktop/package.json b/packages/app-desktop/package.json index ed0ef9a3c56..fb08fe394b1 100644 --- a/packages/app-desktop/package.json +++ b/packages/app-desktop/package.json @@ -1,6 +1,6 @@ { "name": "@joplin/app-desktop", - "version": "1.4.16", + "version": "1.4.17", "description": "Joplin for Desktop", "main": "main.js", "private": true, @@ -32,7 +32,7 @@ "build/images/*" ], "afterAllArtifactBuild": "./generateSha512.js", - "asar": false, + "asar": true, "asarUnpack": "./node_modules/node-notifier/vendor/**", "win": { "rfc3161TimeStampServer": "http://timestamp.comodoca.com/rfc3161", @@ -137,7 +137,7 @@ "md5": "^2.2.1", "moment": "^2.22.2", "node-fetch": "^1.7.3", - "node-notifier": "^6.0.0", + "node-notifier": "^8.0.0", "pretty-bytes": "^5.3.0", "re-resizable": "^6.5.4", "react": "16.13.1", diff --git a/packages/lib/models/Setting.ts b/packages/lib/models/Setting.ts index 6ea9f96d654..c2bedddea9d 100644 --- a/packages/lib/models/Setting.ts +++ b/packages/lib/models/Setting.ts @@ -446,6 +446,12 @@ class Setting extends BaseModel { options: () => themeOptions(), }, + notificationPermission: { + value: '', + type: SettingItemType.String, + public: false, + }, + showNoteCounts: { value: true, type: SettingItemType.Bool, public: false, advanced: true, appTypes: ['desktop'], label: () => _('Show note counts') }, layoutButtonSequence: { diff --git a/packages/lib/services/AlarmServiceDriverNode.ts b/packages/lib/services/AlarmServiceDriverNode.ts index 597c15c1924..0781a1570d6 100644 --- a/packages/lib/services/AlarmServiceDriverNode.ts +++ b/packages/lib/services/AlarmServiceDriverNode.ts @@ -1,6 +1,7 @@ import eventManager from '../eventManager'; import { Notification } from '../models/Alarm'; import shim from '../shim'; +import Setting from '../models/Setting'; const notifier = require('node-notifier'); const bridge = require('electron').remote.require('./bridge').default; @@ -43,6 +44,90 @@ export default class AlarmServiceDriverNode { delete this.notifications_[id]; } + private displayDefaultNotification(notification: Notification) { + const o: any = { + appID: this.appName_, + title: notification.title, + icon: `${bridge().electronApp().buildDir()}/icons/512x512.png`, + }; + if ('body' in notification) o.message = notification.body; + + // Message is required on Windows 7 however we don't want to repeat the title so + // make it an empty string. + // https://github.com/laurent22/joplin/issues/2144 + if (!o.message) o.message = '-'; + + this.logger().info('AlarmServiceDriverNode::scheduleNotification: Triggering notification (default):', o); + + notifier.notify(o, (error: any, response: any) => { + this.logger().info('AlarmServiceDriverNode::scheduleNotification: node-notifier response:', error, response); + }); + } + + private displayMacNotification(notification: Notification) { + // On macOS, node-notifier is broken: + // + // https://github.com/mikaelbr/node-notifier/issues/352 + // + // However we can use the native browser notification as described + // there: + // + // https://www.electronjs.org/docs/tutorial/notifications + // + // In fact it's likely that we could use this on other platforms too + try { + const options: any = { + body: notification.body ? notification.body : '-', + onerror: (error: any) => { + this.logger().error('AlarmServiceDriverNode::displayMacNotification', error); + }, + }; + + this.logger().info('AlarmServiceDriverNode::displayMacNotification: Triggering notification (macOS):', notification.title, options); + + new Notification(notification.title, options); + } catch (error) { + this.logger().error('AlarmServiceDriverNode::displayMacNotification', error); + } + } + + private async checkPermission() { + if (shim.isMac() && shim.isElectron()) { + this.logger().info(`AlarmServiceDriverNode::checkPermission: Permission in settings is "${Setting.value('notificationPermission')}"`); + + if (Setting.value('notificationPermission') !== '') return Setting.value('notificationPermission'); + + // In theory `Notification.requestPermission()` should be used to + // ask for permission but in practice this API is unreliable. In + // particular, it returns "granted" immediately even when + // notifications definitely aren't allowed (and creating a new + // notification would fail). + // + // Because of that, our approach is to trigger a notification, which + // should prompt macOS to ask for permission. Once this is done we + // manually save the result in the settings. Of course it means that + // if permission is changed afterwards, for example from the + // notification center, we won't know it and notifications will + // fail. + // + // All this means that for now this checkPermission function always + // returns "granted" and the setting has only two values: "granted" + // or "" (which means we need to do the check permission trick). + // + // The lack of "denied" value is acceptable in our context because + // if a user doesn't want notifications, they can simply not set + // alarms. + + new Notification('Checking permissions...', { + body: 'Permission has been granted', + }); + + Setting.setValue('notificationPermission', 'granted'); + } + + return 'granted'; + } + async scheduleNotification(notification: Notification) { const now = Date.now(); const interval = notification.date.getTime() - now; @@ -52,6 +137,12 @@ export default class AlarmServiceDriverNode { throw new Error(`Trying to create a notification from an invalid object: ${JSON.stringify(notification)}`); } + const permission = await this.checkPermission(); + if (permission !== 'granted') { + this.logger().info(`AlarmServiceDriverNode::scheduleNotification: Notification ${notification.id}: Cancelled because permission was not granted.`); + return; + } + this.logger().info(`AlarmServiceDriverNode::scheduleNotification: Notification ${notification.id} with interval: ${interval}ms`); if (this.notifications_[notification.id]) shim.clearTimeout(this.notifications_[notification.id].timeoutId); @@ -76,23 +167,11 @@ export default class AlarmServiceDriverNode { }, maxInterval); } else { timeoutId = shim.setTimeout(() => { - const o: any = { - appID: this.appName_, - title: notification.title, - icon: `${bridge().electronApp().buildDir()}/icons/512x512.png`, - }; - if ('body' in notification) o.message = notification.body; - - // Message is required on Windows 7 however we don't want to repeat the title so - // make it an empty string. - // https://github.com/laurent22/joplin/issues/2144 - if (!o.message) o.message = '-'; - - this.logger().info('AlarmServiceDriverNode::scheduleNotification: Triggering notification:', o); - - notifier.notify(o, (error: any, response: any) => { - this.logger().info('AlarmServiceDriverNode::scheduleNotification: node-notifier response:', error, response); - }); + if (shim.isMac() && shim.isElectron()) { + this.displayMacNotification(notification); + } else { + this.displayDefaultNotification(notification); + } this.clearNotification(notification.id);