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

Visual notifications shown when "Do not disturb" is turned on #1299

Closed
alya opened this issue Apr 11, 2023 · 4 comments · Fixed by #1302
Closed

Visual notifications shown when "Do not disturb" is turned on #1299

alya opened this issue Apr 11, 2023 · 4 comments · Fixed by #1302

Comments

@alya
Copy link
Collaborator

alya commented Apr 11, 2023

As reported on CZO, visual desktop notifications are still shown when the "Do not disturb" option is turned on. We should fix this.

@_Anders Kaseorg|699 said:

This seems to have been broken since v5.0.0 (specifically v5.0.0~6).

@cyrilchukwuebuka
Copy link

@alya I would love to be assigned this issue
I'm currently examining the bug for a fix

@alya
Copy link
Collaborator Author

alya commented Apr 13, 2023

Sure! Please post a comment describing your proposed approach when you're ready. You can have the issue assigned to you after you have a rough plan for how to address it. Thanks!

@enesonus
Copy link

@alya I think that the problem is we do not sync inapp DND mode state with the machine's DND mode state. As far as I understood, we only look at in-app dnd mode to send visual or voice notifications. Electron documentation for notifications suggests using macos-notification-state package to check if the app is allowed to send a notification for macOS. I could not find this package in zulip-desktop repo so concluded that we do not have a mechanism for checking macOS machines' DND mode.

For linux I could not find a promising fix but this thread seems useful and also It might be a good idea to look at VSCode's notification implementation for linux based machines.

If you could assign me to this issue I would be happy to work on it!

@cyrilchukwuebuka
Copy link

cyrilchukwuebuka commented Apr 18, 2023

Referencing what @enesonus said, the application does not have a native check and integration with OS for notification state as documented on Electron such as in Windows, using the windows-notification-state and other OS and their packages, though it's secondary to the current bug as basically from the knowledge I could garner Zulip desktop makes use of browser Window Notification.

This current bug could be related to #4174 issue with PR(#4223) on the Electron repository because Zulip desktop makes use of webview to display web contents such as a Zulip chat web page and as such is possible, the notification could as well come from the webview.
In this technique, the expedient approach is to implement ses.setPermissionRequestHandler(handler) for the notification and media(sound) which would restrict audio and visual notification display from a webview webcontent when DND is enabled/disabled or in specific settings options such as "mute all sounds from Zulip" and "show desktop notifications"

Alternatively, disabling notifications both the visual and sound notifications could also be achieved dynamically using a similar technique on the webview instance showNotificationSettings method which calls ipcRender.on("show-notification-settings"),

Webview show notification

preload ipcRenderer

disable noification

So once the notification settings come up, we can disable desktop notifications for visual and/or audio. The only drawback to this method is that it only applies to the current displayed webview. Likewise for the enabling option.

@andersk, I would be glad if you could review the feasibility of these solutions as outlined above in order for @alya to assign the issue to me

andersk added a commit to andersk/zulip-desktop that referenced this issue Apr 18, 2023
Fixes zulip#1299.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
andersk added a commit that referenced this issue Apr 18, 2023
Fixes #1299.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants