darwin-notifications: Migrate darwin-notifications to electron's native Notifications. #1022
Conversation
manavmehta
force-pushed
the
mac-notif
branch
3 times, most recently
from
f4e6c2f to
e07a4a6
Compare
manavmehta
changed the title
manavmehta
force-pushed
the
mac-notif
branch
2 times, most recently
from
aaaef3d to
cd9712b
Compare
manavmehta
force-pushed
the
mac-notif
branch
2 times, most recently
from
adfc7a3 to
de45ae3
Compare
There was a problem hiding this comment.
Chrome now also supports inline notification replies. I think it would be better to use that API to implement this feature in the web app so we don’t have to maintain this platform-specific monkey patch in zulip-desktop.
| */ | ||
| import {Notification, nativeImage} from 'electron'; | ||
|
|
||
| import fetch from 'node-fetch'; |
There was a problem hiding this comment.
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.
| const response = await fetch(imageURL); | ||
| const buffer = await response.buffer(); | ||
| const base64String = buffer.toString('base64'); | ||
| const dataUri = `data:image/png;base64,${base64String}`; |
There was a problem hiding this comment.
My bad. Referring to this piece of code, I missed this part somehow fiddling around. Adding it back.
| ipcRenderer.send('create-notification', notificationOptions, profilePicURL); | ||
| ipcRenderer.on('replied', async (_event: Electron.IpcRendererEvent, response: string) => { | ||
| await this.notificationHandler({response}); | ||
| ipcRenderer.removeAllListeners('replied'); |
There was a problem hiding this comment.
Why removeAllListeners? Did you just want to use ipcRenderer.once instead?
| // 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. |
There was a problem hiding this comment.
I don’t understand this new comment. Notification.close() is part of the standard Notification API. It’s not Zulip-specific.
There was a problem hiding this comment.
The previous comment got me believing that this was specific to node-mac-notifier. So I tried to refine it. I'll remodel it.
| profilePicURL: string | ||
| ) => { | ||
| const profilePic = await getNativeImagefromUrl(profilePicURL); | ||
| notificationOptions.icon = profilePic; |
There was a problem hiding this comment.
Why do we need to go through the NativeImage dance? Can we not just set icon to the URL?
There was a problem hiding this comment.
Trying to do that from the beginning but it throws this error while converting
There was a problem hiding this comment.
@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
|
Heads up @manavmehta, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
@manavmehta would you be up for finishing this? |
|
Any updates on this? |
|
@mateuszmandera Sorry I was really occupied the last two months. I'll try to get back by Tuesday :) |
|
@manavmehta Awesome, thanks! |
|
@mateuszmandera do you have macOS too? |
…ve Notifications. Electron's native Notifications now supports inline replies on macOS and thus we don't need any 3rd party package. This commit removes the usage of node-mac-notifier and brings native Notification in picture. For comprehensibility, ipc-helpers will contain the functions which make use of Main process APIs with already functioning code in Renderer process. This is a potential solution to people not getting notifications on macOS.
manavmehta
force-pushed
the
mac-notif
branch
from
de45ae3 to
c816233
Compare
|
@manavmehta Sadly no 😞 |
node-mac-notifier no longer builds on macOS with Electron 11 (error: no template named 'remove_cv_t' in namespace 'std'). It was previously implicated in crashes on macOS (zulip#1016). And we no longer have any macOS developers that seem to be maintaining this feature (e.g. zulip#1022 is stalled). Signed-off-by: Anders Kaseorg <[email protected]>
node-mac-notifier no longer builds on macOS with Electron 11 (error: no template named 'remove_cv_t' in namespace 'std'). It was previously implicated in crashes on macOS (zulip#1016). And we no longer have any macOS developers that seem to be maintaining this feature (e.g. zulip#1022 is stalled). Signed-off-by: Anders Kaseorg <[email protected]>
|
Considering the discussion going on in CZO stream, I will be closing this PR for now. |
What's this PR do?
Electron's native Notifications now supports inline replies on macOS and thus we don't need any 3rd party package.
This commit removes the usage of node-mac-notifier and brings native Notification in picture.
For comprehensibility, ipc-helpers.ts will contain the functions which make use of Main process APIs with already functioning code in Renderer process.
Any background context you want to provide?
This is a potential solution to people not getting notifications on macOS.
You have tested this PR on:
Follow-ups