1208346 - nsIAlertService: Do Not Disturb backend

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Matthew N. [:MattN]]

Implement a method to toggle a Do Not Disturb mode for the alert service.

The proposal is to have this affect all (application and website) alerts.

Perhaps a follow-up is to provide an API to return the OS DND mode so it can be conveyed in preferences too. We already honour the Windows setting for XUL notifications via nsAlertsService::ShouldShowAlert[1].

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsAlertsService.cpp?rev=0ac19d3bf7bf#37

Comment 1

[Matthew N. [:MattN]]

Philipp, is the spec up-to-date with your current thoughts for the initial version? Are we going with a time-based DND or just use the session? I'd like to start on this tomorrow.

Comment 2

[Matthew N. [:MattN]]

We may want to expose whether a system alert service is in use,and likely has its own DND feature e.g. OS X, so we can avoid showing our own DND UI but I'm not sure if we can reliably detect that without first trying to display a notification.

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #1)
> Philipp, is the spec up-to-date with your current thoughts for the initial
> version? Are we going with a time-based DND or just use the session? I'd
> like to start on this tomorrow.

I just updated the mocks in Invision (same link).
We can make do with just making it session based in 44. There should however be two things:
- A way to manually re-enable in about:preferences (to be designed)
- After disabling through the popup, we should show one last (transient) notification that tells the user that he can turn it off again in about:preferences (shown only the first time the user sets DND)

Comment 4

[Bill Maggs (bmaggs)]

The "until I restart" solution is the best one for 44. In the long run the timed (relative or otherwise) approach of "DND for an hour" is the way to go, but not at first.

Comment 5

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

Bug 1208346 - Alert service Do Not Disturb backend. r=wchen

Comment 6

[Matthew N. [:MattN]]

I'll write unit and mochi tests in a separate patch but wanted to start the review and have something others can build on for the UI while I'm on PTO for the next two days.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8671176 [details]
MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

https://reviewboard.mozilla.org/r/21551/#review19557

What are the plans for hooking this up to the native integrations (for example, if DnD is enabled through the browser but we are using system notifications this should tell the system to turn on DnD).

::: toolkit/components/alerts/nsAlertsService.cpp:182
(Diff revision 1)
> +NS_IMETHODIMP nsAlertsService::GetDoNotDisturbTemporarily(bool* aRetVal) {

Can you rename this to:
::GetIsTemporaryDoNotDisturbEnabled(bool* retval) ?

Alternatively, we could remove the word "Temporary" from all of the function names, since I don't know what we are calling the permanent do-no-disturb feature.

::: toolkit/components/alerts/nsIAlertsService.idl:47
(Diff revision 1)
> +   *             TODO

Is this empty TODO still necessary?

::: toolkit/components/alerts/nsIAlertsService.idl:86
(Diff revision 1)
> +   * Enables a temporary Do Not Disturb mode for the service to reduce the amount of disruption alerts cause the user.

s/disruption alerts/disruption that alerts/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

What are your thoughts on just making a SetDoNotDisturbTemporarily setter instead of creating the separate EnableTemporaryDoNotDisturb and DisableTemporaryDoNotDisturb functions? That seems like less code overall and won't impact readability.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8671176 [details]
MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

https://reviewboard.mozilla.org/r/21551/#review19565

::: toolkit/components/alerts/nsAlertsService.cpp:154
(Diff revision 1)
> +NS_IMETHODIMP nsAlertsService::EnableTemporaryDoNotDisturb() {
> +  // Try the system notification service.

Please follow convention in file to place { on new line.

::: toolkit/components/alerts/nsXULAlerts.cpp:168
(Diff revision 1)
> +nsresult nsXULAlerts::EnableTemporaryDoNotDisturb() {

Please follow convention of file to place return type on new line and { on new line.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/21551/#review19567

I don't know how to change a bug from r+ to r- in ReviewBoard, but the patch attached to this bug is causing an exception to be thrown with the following code:

notifier = Cc["@mozilla.org/alerts-service;1"].
               getService(Ci.nsIAlertsService);
               
I've filed bug 1214036 for the ReviewBoard issue.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Please disregard comment #10, a clobber build fixed the issue for me.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8671176 [details]
MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

Are there follow-up bugs for libnotify and OS X notifications?

Comment 13

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/21551/#review19557

I look at this as a Firefox feature not as an OS-integration feature and I don't think setting DND for Firefox means that the user doesn't want notifications from any application.

> Can you rename this to:
> ::GetIsTemporaryDoNotDisturbEnabled(bool* retval) ?
> 
> Alternatively, we could remove the word "Temporary" from all of the function names, since I don't know what we are calling the permanent do-no-disturb feature.

The "temporary" is used to distiniguish from other DND reasons e.g. based on hour of the day or whether a projector is attached. I used "temporary" since the plan is for the toggle to not persist, either reverting at the end of the session or after some time period.

Maybe s/Temporary/Manual/?

This getter is used to know the state of the checkbox and the checkbox is for temporary DND so I think the "temporary"/"manual" part should stay.

> Is this empty TODO still necessary?

I was just going to document the 2 new callbacks added in other bugs as I happened to notice it. This can be done in another bug or I may just fix it.

Comment 14

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> What are your thoughts on just making a SetDoNotDisturbTemporarily setter
> instead of creating the separate EnableTemporaryDoNotDisturb and
> DisableTemporaryDoNotDisturb functions? That seems like less code overall
> and won't impact readability.

I thought about using a setter but it seemed like we will need to pass options like time periods for the next iteration. e.g. Do not disturb me for 4 hours or do not disturb me until I say otherwise (even after Fx restarts) so I was trying to avoid having to change it. If you want I can use a setter and we can refactor later.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Yes, I would prefer that we refactor later when those decisions are made. As it is now we should keep the API simple.

Comment 16

[Matthew N. [:MattN]]

It was clear from :phlsa that we wanted this to be time-based and we were simply punting on that for 44

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #16)
> It was clear from :phlsa that we wanted this to be time-based and we were
> simply punting on that for 44

It wouldn't be the first time that we changed our minds on something before it got implemented. I still don't see a reason to not implement it the simpler and more straightforward way the first time, and then do the refactoring as needed.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8671176 [details]
MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8671176 [details]
MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws

https://reviewboard.mozilla.org/r/21551/#review19953

::: toolkit/components/alerts/nsIAlertsService.idl:11
(Diff revision 2)
>  [scriptable, uuid(9d0284bf-db40-42da-8f0d-c2769dbde7aa)]

Should you rev the uuid now that new topics can be passed to observe()?

Comment 20

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/21551/#review19953

> Should you rev the uuid now that new topics can be passed to observe()?

I don't believe that's necessary since it doesn't affect binary compatibility and there was already more than one topic so existing consumers should have already been branching based on the topic.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e0899a46ca4

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e95f27b7d871

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e0899a46ca4
https://hg.mozilla.org/mozilla-central/rev/e95f27b7d871