Desktop: Fixed notifications on macOS

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",

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: {

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);