Desktop Notification support
ClosedPublic
Actions

Authored by btrahan on Jun 9 2015, 12:23 AM.

Details

Reviewers

epriestley

Maniphest Tasks

T4139: Add support for desktop notifications

Commits

Restricted Diffusion Commit
rP1bb2978a895a: Desktop Notification support

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

btrahan updated this revision to Diff 31956.Jun 9 2015, 12:23 AM

btrahan added a task: T4139: Add support for desktop notifications.

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.

Herald added subscribers: Korvin, epriestley. · View Herald TranscriptJun 9 2015, 12:23 AM

Screen Shot 2015-06-08 at 5.26.28 PM.png (414×914 px, 135 KB)

btrahan updated this object.Jun 9 2015, 12:28 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.

joshuaspence added a subscriber: joshuaspence.Jun 9 2015, 8:00 AM

• sshannin awarded a token.Jun 9 2015, 1:32 PM

tycho.tatitscheff awarded a token.Jun 9 2015, 2:34 PM

tycho.tatitscheff added a subscriber: tycho.tatitscheff.

rbalik added a subscriber: rbalik.Jun 9 2015, 4:55 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.