Fixes T4139. Adds a "Desktop Notifications" panel to settings. For now, we start with "Send Desktop Notifications Too" functionality. We can try to be fancy later and only send desktop notifications if the web app doesn't have focus, etc.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T4139: Add support for desktop notifications
- Commits
- Restricted Diffusion Commit
rP1bb2978a895a: Desktop Notification support
Made some comments as a test user on a task and got purdy desktop notifications using Chrome. Then did it again with Firefox.
Played around with permissions form with Chrome and got helpful information about what was up. Played around with Firefox and got similar results, except canceling the dialogue didn't invoke my handler code somehow. Oh Firefox!
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- T4139
- Lint
Lint Warnings Severity Location Code Message Warning webroot/rsrc/js/application/aphlict/behavior-desktop-notifications-control.js:26 W117 JSHintW117 Warning webroot/rsrc/js/core/Notification.js:46 W117 JSHintW117 - Unit
Tests Passed - Build Status
Buildable 6638 Build 6660: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
Worked great for me locally.
could be "app and desktop", "desktop only", "app only", "neither / none" ?
It looks like the site only gets to send one desktop notification at a time? This sort of makes me lean toward "app + desktop" vs "app only" being correct, although I'm not really sure. I'd maybe be inclined to start there and see how far we get, at least.
If anything, I'd maybe try to make this mode smarter by tracking window.focus / window.blur (I think? Or similar -- pretty sure this is possible) and not delivering the desktop notification if a window was in the foreground. So this would be a "if you're in another app, it alerts you that something happened in Phabricator" sort of thing. But that might be a lot of work or fragile or not really align with how they're most useful.
probably need to add an explicit "request permissions button" for various failure modes or even always...
Yeah, it would be nice to show the state (can we? window.Notification.permission?) and have an explicit "authorize" button, I think -- especially since Firefox seems to have weird settings like "this session only".
I had a couple of things misconfigured and wasn't sure if it was a browser thing or a settings thing. Having a nice green "authorized √" would have helped.
| src/applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php | ||
|---|---|---|
| 7–8 | We should probably also require notification.enabled since these aren't very meaningful if real-time isn't hooked up. | |
Comment Actions
It looks like the site only gets to send one desktop notification at a time?
I actually built this that way but it can send more if you want / I can try to mirror how it works in application.
| webroot/rsrc/js/core/Notification.js | ||
|---|---|---|
| 49 | tag makes the OS / browser aggregate notifications with the same tag. (so one at a time) | |
Comment Actions
Oh, interesting. I'm not sure which behavior is better. I think I'd have to use this for a bit to get a better sense of how it fits into my workflow and which behaviors would work best for me.
| src/applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php | ||
|---|---|---|
| 104–106 | Debugging? | |