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.

Sign up for GitHub

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

Jump to bottom

dexc-desktop: Desktop notifications #2599

Merged
merged 6 commits into from
Jan 17, 2024
Merged

dexc-desktop: Desktop notifications #2599

merged 6 commits into from
Jan 17, 2024

Conversation

peterzen
Copy link
Member

@peterzen peterzen commented Nov 9, 2023 •
edited
Loading

This PR changes the way desktop notifications are sent when running in WebView. Instead of the HTML5 Notifications API the sendDesktopNotification call is used.

Tested on Firefox, Chrome, Safari, Edge.
dexc-desktop tested on Linux, Windows 10, Windows 11, macOS.

macOS solution is implemented in #2631

Closes #2576

Edit:

A recent update of Edge removed the ability to request notification permission from JS (Notification.requestPermission). The supported way now, apparently, is to have the user grant the permission in the browser settings.
https://answers.microsoft.com/en-us/microsoftedge/forum/all/webpush-notifications-blocked/a65a0e3e-dd0f-427f-b16e-31a98d60b0c4

This leaves us with 2 choices; a) when running in Edge, disable the "Enable desktop notifications" checkbox and display a note about how to change the settings, b) leave it as is and revise in a later iteration. I'm leaning towards b) as Edge has a 12% market share and this PR is already rather large but if this is important I'll do another PR.

@ukane-philemon
Copy link
Contributor

macOS solution is WIP (ideas welcome on how to plug in webkit.AddScriptMessageHandler)

I'd be glad to assist. You can focus on other operating systems, I'll implement for macOS.

PS: I have something already, but it seems the work in this PR is not fully fleshed out so I'll wait.

@peterzen
Copy link
Member Author

Windows and Linux work, the macOS bit is left to be sorted. Thanks for looking into it!

ukane-philemon
ukane-philemon suggested changes Nov 12, 2023
View reviewed changes
client/webserver/site/src/js/notifications.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/notifications.ts Outdated Show resolved Hide resolved
@peterzen peterzen force-pushed the desktop-notifications branch from 515b3bd to daa7f89 Compare November 14, 2023 18:29
@peterzen
Copy link
Member Author

I'd be glad to assist. You can focus on other operating systems, I'll implement for macOS.

PS: I have something already, but it seems the work in this PR is not fully fleshed out so I'll wait.

@ukane-philemon have you been able to look into this?

@ukane-philemon
Copy link
Contributor

@ukane-philemon have you been able to look into this?

Yes. I'll push to a branch soon.

PS: Does Notifier.requestNtfnPermission work when running as a webview app on Linux in this PR?

@peterzen
Copy link
Member Author

peterzen commented Nov 26, 2023 •
edited
Loading

PS: Does Notifier.requestNtfnPermission work when running as a webview app on Linux in this PR?

The HTML5 Notification API doesn't work, hence the OSDesktopNotifier implementation:

https://github.com/peterzen/dcrdex/blob/daa7f896ac56bc5aedb8de5dc20c25c0e6225272/client/webserver/site/src/js/notifications.ts#L101-L121

@ukane-philemon ukane-philemon mentioned this pull request Nov 27, 2023
Closed
@ukane-philemon
Copy link
Contributor

@ukane-philemon have you been able to look into this?

Yes. I'll push to a branch soon.

PS: Does Notifier.requestNtfnPermission work when running as a webview app on Linux in this PR?

I encountered a blocker while implementing this but already asked for suggestions here: progrium/darwinkit#231

@peterzen
Copy link
Member Author

Yes. I'll push to a branch soon.
PS: Does Notifier.requestNtfnPermission work when running as a webview app on Linux in this PR?

No, it gets silently ignored - the notification API isn't implemented in webview (which kind of makes sense), this is why I use the sendDesktopNotification in the Linux and Windows implementations in this PR. I think the same is true for WebKit on macOS.

I encountered a blocker while implementing this but already asked for suggestions here: progrium/macdriver#231

Thanks for posting this. I noticed some functions like registerNewClass are not included in the test case and wondered if these should be added?

@ukane-philemon
Copy link
Contributor

I noticed some functions like registerNewClass are not included in the test case and wondered if these should be added?

In the macdrive repo?

@peterzen
Copy link
Member Author

I noticed some functions like registerNewClass are not included in the test case and wondered if these should be added?

In the macdrive repo?

No, to the test case in the progrium post. Could you point me at your full code please?

@ukane-philemon
Copy link
Contributor

ukane-philemon commented Dec 11, 2023 •
edited
Loading

No, to the test case in the progrium post. Could you point me at your full code please?

Oh, okay.

Edit: @peterzen please see: #2631.

@ukane-philemon ukane-philemon mentioned this pull request Dec 12, 2023
Closed
@peterzen peterzen force-pushed the desktop-notifications branch from daa7f89 to 8591fe6 Compare December 12, 2023 22:36
@peterzen peterzen marked this pull request as ready for review December 12, 2023 22:44
buck54321
buck54321 reviewed Jan 4, 2024
View reviewed changes
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Tested on Linux and Mac.

On Mac, the notification doesn't have our icon.

client/webserver/site/src/js/notifications.ts
browserNtfnSettings[noteType] = enabled
State.storeLocal(browserNotificationsSettingsKey(), browserNtfnSettings)
export function updateNtfnSetting (noteType: string, enabled: boolean) {
fetchDesktopNtfnSettings()
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary, right?

client/webserver/site/src/js/settings.ts Outdated Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor

On Mac, the notification doesn't have our icon.

yh. We’re currently using beep.

@peterzen
Copy link
Member Author

peterzen commented Jan 4, 2024

On Mac, the notification doesn't have our icon.

beeep doesn't support icons on macOS.

gen2brain/beeep#39

@peterzen peterzen force-pushed the desktop-notifications branch from 766e478 to 45c264e Compare January 11, 2024 20:18
@peterzen peterzen force-pushed the desktop-notifications branch from 45c264e to c509a3f Compare January 11, 2024 20:19
@buck54321
Copy link
Member

On Mac, the notification doesn't have our icon.

beeep doesn't support icons on macOS.

gen2brain/beeep#39

We'll leave this as in need of a solution for now then. I'll open an issue. I'm sure there's a way to avoid beeep and use system calls of some kind.

@buck54321 buck54321 merged commit 122c1e7 into decred:master Jan 17, 2024
5 checks passed
@peterzen peterzen deleted the desktop-notifications branch January 17, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ukane-philemon ukane-philemon ukane-philemon requested changes

@buck54321 buck54321 buck54321 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dexc-desktop: Browser notifications not displayed
3 participants
@peterzen @ukane-philemon @buck54321