Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

darwin-notifications: Migrate darwin-notifications to electron's native Notifications. #1022

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/main/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as ProxyUtil from '../renderer/js/utils/proxy-util';
import {sentryInit} from '../renderer/js/utils/sentry-util';

import {appUpdater} from './autoupdater';
import {showDarwinNotification} from './ipc-helpers';
import * as AppMenu from './menu';
import {_getServerSettings, _saveServerIcon, _isOnline} from './request';
import {setAutoLaunch} from './startup';
Expand Down Expand Up @@ -273,6 +274,10 @@ ${error}`
toggleApp();
});

ipcMain.on('create-notification', async (event, notificationOptions: Electron.NotificationConstructorOptions, profilePicURL: string) => {
await showDarwinNotification(event, notificationOptions, profilePicURL);
});

ipcMain.on('toggle-badge-option', () => {
BadgeSettings.updateBadge(badgeCount, mainWindow);
});
Expand Down
38 changes: 38 additions & 0 deletions app/main/ipc-helpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* This file contains the functions which make use of Main process APIs with already functioning code in Renderer process.
*/
import {Notification, nativeImage} from 'electron';

import fetch from 'node-fetch';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node-fetch doesn’t use the Electron network stack, so this won’t work with custom certificates, proxies, etc. We should use electron.net like everything else.


export const showDarwinNotification = async (
event: Electron.IpcMainEvent,
notificationOptions: Electron.NotificationConstructorOptions,
profilePicURL: string
) => {
const profilePic = await getNativeImagefromUrl(profilePicURL);
notificationOptions.icon = profilePic;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to go through the NativeImage dance? Can we not just set icon to the URL?

Copy link
Collaborator Author

@manavmehta manavmehta Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to do that from the beginning but it throws this error while converting
this

Copy link
Member

@akashnimare akashnimare Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andersk the API doesn't support passing the URL directly. That's why we have to go through the nativeImage process.
https://www.electronjs.org/docs/api/notification#new-notificationoptions

cc @priyank-p


const notification = new Notification(notificationOptions);
notification.show();
notification.on('reply', (_event, response) => {
event.sender.send('replied', response);
});
};

/* TODO: Migrate url->dataUri conversion part to renderer process
to make use of browser caching of images.
*/
const getNativeImagefromUrl = async (imageURL: string) => {
try {
const response = await fetch(imageURL);
const buffer = await response.buffer();
const base64String = buffer.toString('base64');
const dataUri = `data:image/png;base64,${base64String}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know it’s a PNG?

Copy link
Collaborator Author

@manavmehta manavmehta Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Referring to this piece of code, I missed this part somehow fiddling around. Adding it back.

const image = nativeImage.createFromDataURL(dataUri);
return image;
} catch (error) {
console.error(error);
return null;
}
};
43 changes: 16 additions & 27 deletions app/renderer/js/notification/darwin-notifications.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import {ipcRenderer} from 'electron';

import MacNotifier from 'node-mac-notifier';

import electron_bridge from '../electron-bridge';
import * as ConfigUtil from '../utils/config-util';

import {
appId, customReply, focusCurrentServer, parseReply
customReply, focusCurrentServer, parseReply
} from './helpers';

type ReplyHandler = (response: string) => void;
Expand All @@ -22,30 +20,21 @@ class DarwinNotification {
tag: number;

constructor(title: string, options: NotificationOptions) {
const silent: boolean = ConfigUtil.getConfigItem('silent') || false;
const {icon} = options;
const profilePic = new URL(icon, location.href).href;

const profilePicURL = new URL(options.icon, location.href).href;
this.tag = Number.parseInt(options.tag, 10);
const notification = new MacNotifier(title, Object.assign(options, {
bundleId: appId,
canReply: true,
silent,
icon: profilePic
}));

notification.addEventListener('click', () => {
// Focus to the server who sent the
// notification if not focused already
if (clickHandler) {
clickHandler();
}

focusCurrentServer();
ipcRenderer.send('focus-app');
const notificationOptions: Electron.NotificationConstructorOptions = {
title,
body: options.body,
silent: ConfigUtil.getConfigItem('silent') || false,
hasReply: true,
timeoutType: 'default'
};

ipcRenderer.send('create-notification', notificationOptions, profilePicURL);
ipcRenderer.on('replied', async (_event: Electron.IpcRendererEvent, response: string) => {
await this.notificationHandler({response});
ipcRenderer.removeAllListeners('replied');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removeAllListeners? Did you just want to use ipcRenderer.once instead?

});

notification.addEventListener('reply', this.notificationHandler);
}

static requestPermission(): void {
Expand Down Expand Up @@ -103,8 +92,8 @@ class DarwinNotification {
customReply(response);
}

// Method specific to notification api
// used by zulip
// Method specific to Zulip's notification module, not the package used to create the same.
// If at all the need arises to migrate to another API (or npm package), DO NOT REMOVE THIS.
Comment on lines +95 to +96
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t understand this new comment. Notification.close() is part of the standard Notification API. It’s not Zulip-specific.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous comment got me believing that this was specific to node-mac-notifier. So I tried to refine it. I'll remodel it.

close(): void {
// Do nothing
}
Expand Down
32 changes: 21 additions & 11 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@
"dependencies": {
"@electron-elements/send-feedback": "^2.0.3",
"@sentry/electron": "^2.0.1",
"@types/node-fetch": "^2.5.7",
"@yaireo/tagify": "^3.20.2",
"adm-zip": "^0.4.16",
"auto-launch": "^5.0.5",
Expand All @@ -161,12 +162,10 @@
"i18n": "^0.13.2",
"iso-639-1": "^2.1.4",
"nan": "^2.14.2",
"node-fetch": "^2.6.0",
"node-json-db": "^1.1.0",
"semver": "^7.3.2"
},
"optionalDependencies": {
"node-mac-notifier": "^1.1.0"
},
"devDependencies": {
"@types/adm-zip": "^0.4.33",
"@types/auto-launch": "^5.0.1",
Expand Down