Closed Bug 1208346 Opened 4 years ago Closed 4 years ago

nsIAlertService: Do Not Disturb backend

Categories

(Toolkit :: Notifications and Alerts, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla44
Iteration:
44.1 - Oct 5
Tracking Status
firefox44 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

User Story

As a user I want to be able to temporarily stop notifications from interrupting me.

Attachments

(1 file)

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
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.
Flags: needinfo?(philipp)
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.
(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)
Flags: needinfo?(philipp)
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.
Blocks: 1209601
Blocks: 1209602
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 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/
Attachment #8671176 - Flags: review+
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 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.
Attachment #8671176 - Flags: review+
Please disregard comment #10, a clobber build fixed the issue for 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?
Attachment #8671176 - Flags: review+
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.
(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.
Yes, I would prefer that we refactor later when those decisions are made. As it is now we should keep the API simple.
It was clear from :phlsa that we wanted this to be time-based and we were simply punting on that for 44
(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 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
Attachment #8671176 - Attachment description: MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=wchen → MozReview Request: Bug 1208346 - Alert service Do Not Disturb backend. r=jaws
Attachment #8671176 - Flags: review+ → review?(jaws)
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()?
Attachment #8671176 - Flags: review?(jaws) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/3e0899a46ca4
https://hg.mozilla.org/mozilla-central/rev/e95f27b7d871
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.