Page MenuHomePhabricator

Desktop Notification support
ClosedPublic

Authored by btrahan on Jun 9 2015, 12:23 AM.
Tags
None
Tokens
"Mountain of Wealth" token, awarded by joshuaspence."Pterodactyl" token, awarded by yelirekim."Mountain of Wealth" token, awarded by richardvanvelzen."Baby Tequila" token, awarded by tycho.tatitscheff."Evil Spooky Haunted Tree" token, awarded by sshannin.

Details

Summary

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.

Test Plan

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

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

btrahan updated this revision to Diff 31956.Jun 9 2015, 12:23 AM
btrahan retitled this revision from to Draft - Desktop Notification support.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
btrahan updated this object.Jun 9 2015, 12:28 AM
epriestley edited edge metadata.Jun 9 2015, 1:24 AM

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
8–9

We should probably also require notification.enabled since these aren't very meaningful if real-time isn't hooked up.

rbalik added a subscriber: rbalik.Jun 9 2015, 4:55 PM
btrahan planned changes to this revision.Jun 9 2015, 7:08 PM

Taking it out of your queue for now.

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
50

tag makes the OS / browser aggregate notifications with the same tag. (so one at a time)

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.

btrahan updated this revision to Diff 32340.Jun 19 2015, 11:29 PM
btrahan marked an inline comment as done.
btrahan edited edge metadata.

finished + rebase

btrahan retitled this revision from Draft - Desktop Notification support to Desktop Notification support.Jun 19 2015, 11:31 PM
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
epriestley accepted this revision.Jun 22 2015, 6:01 PM
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php
104–106

Debugging?

This revision is now accepted and ready to land.Jun 22 2015, 6:01 PM
This revision was automatically updated to reflect the committed changes.