multi: Implement OSDesktopNotifier for MacOS and some refactoring #2631
Conversation
|
|
||
| Doc.bind(enabledCheckbox, 'click', async (e: Event) => { |
There was a problem hiding this comment.
This bind() should not be necessary to change either, the frontend logic remains the same in webview or webkit.
|
Thanks @peterzen. Some of the changes here should proly come in another PR, I had them fixed since it was notification-related and wanted to put this up real quick so you can see the changes as requested. Moving to draft for now. |
Awesome, thanks a lot for figuring out the |
ukane-philemon
force-pushed
the
macos-notes
branch
from
e04d503 to
32e9eda
Compare
ukane-philemon
force-pushed
the
macos-notes
branch
from
32e9eda to
3bf646a
Compare
| @@ -203,8 +203,8 @@ export default class Application { | |||
| this.attachCommon(this.header) | |||
| this.attach({}) | |||
| this.updateMenuItemsDisplay() | |||
| // initialize browser notifications | |||
| ntfn.fetchBrowserNtfnSettings() | |||
| // initialize desktop notifications | |||
There was a problem hiding this comment.
This code still manages the browser ntfn's - as well as the desktop toasts. Renaming these functions to suggest they handle desktop notifications only isn't warranted.
| @@ -175,25 +175,23 @@ export default class SettingsPage extends BasePage { | |||
| this.renderDesktopNtfnSettings() | |||
| } | |||
|
|
|||
| async updateNtfnSetting (e: Event) { | |||
| updateNtfnSetting (e: Event) { | |||
There was a problem hiding this comment.
No need for the async here - this is fixed in the parent PR.
Okay. I think you can just cherry-pick what you need for your PR since this builds on top of it. It’ll be easier for reviewers to test everything in one go then I can just close this, okay? |
Good idea, it's indeed more straightforward to test all 3 platforms together. Your changes are all in one commit though so I can't cherry-pick the Darwin related modifications - any chance you could maybe separate the core changes from the refactor stuff? |
Signed-off-by: Philemon Ukane <[email protected]>
ukane-philemon
force-pushed
the
macos-notes
branch
from
3bf646a to
6ee877f
Compare
| static async sendDesktopNotification (title: string, body?: string): Promise<void> { | ||
| if (!desktopNtfnSettings.browserNtfnEnabled) return | ||
| if (window.webkit) await window.webkit.messageHandlers.dexcHandler.postMessage(['sendOSNotification', title, body]) // See: client/cmd/dexc-desktop/app_darwin.go#L673-#L697. | ||
| else await (window as any).sendOSNotification(title, body) // this calls a function exported via webview.Bind() |
There was a problem hiding this comment.
Instead of
window as anyadd the method to the global Window definition.
sendOSNotification: (title: string, body?: string) => voidThere was a problem hiding this comment.
This PR was superseded by #2599, these changes were rolled into it.
|
|
||
| static async sendDesktopNotification (title: string, body?: string): Promise<void> { | ||
| if (!desktopNtfnSettings.browserNtfnEnabled) return | ||
| if (window.webkit) await window.webkit.messageHandlers.dexcHandler.postMessage(['sendOSNotification', title, body]) // See: client/cmd/dexc-desktop/app_darwin.go#L673-#L697. |
There was a problem hiding this comment.
window.webkit is defined on Linux too, apparently, so this breaks Linux desktop notifications.
There was a problem hiding this comment.
It works correctly because we test for webview first.
https://github.com/peterzen/dcrdex/blob/c509a3ffdb7a4ae626f395ef0f7ead3f9239f851/client/webserver/site/src/js/notifications.ts#L117-L120
But it did seem a bit brittle indeed, refactored the check to not rely on window.webkit in #2599. c509a3f
|
@buck54321, I think @peterzen has merged these changes to his PR so we can close this now. |
Depends on #2599