Closed Bug 1074667 Opened 10 years ago Closed 9 years ago

As a user, I should have a system notification when someone joins a room

Categories

(Hello (Loop) :: Client, enhancement, P1)

enhancement
Points:
3

Tracking

(firefox35 verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
37.1
Tracking Status
firefox35 --- verified
firefox36 --- verified
backlog Fx35+

People

(Reporter: standard8, Assigned: mikedeboer)

References

Details

(Whiteboard: [rooms] [strings])

User Story

* As a user, I should have a system notification when someone joins a room, so that I know about the activity if the room is closed.

UX:
* A system notification should be displayed
* Clicking the notification, opens the room and focuses Firefox

Strings:
* Title: <Conversation name>
* Text: Someone has joined the conversation! (replaced original UX text "Someone has joined you!")

Implementation:
* When an increase of participants is detected, display a notification - for the particular room.
** (probably using nsIAlertsService)
* Clicking the notification, opens the panel to the rooms list and focuses Firefox

Attachments

(2 files, 6 obsolete files)

      No description provided.
User Story: (updated)
Blocks: 1074659
Severity: normal → enhancement
About "* Clicking the notification, opens the room and focuses Firefox"
I think we should open the panel rather than the room directly as someone clicks the notification as he does not know which room it is at this stage and may not want to join/enable his camera directly.

Darrin do you agree?
Flags: needinfo?(dhenein)
(In reply to Romain Testard [:RT] from comment #2)
> About "* Clicking the notification, opens the room and focuses Firefox"
> I think we should open the panel rather than the room directly as someone
> clicks the notification as he does not know which room it is at this stage
> and may not want to join/enable his camera directly.
> 
> Darrin do you agree?

Yes -- should focus Firefox and open the panel to the rooms list.
Flags: needinfo?(dhenein)
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
User Story: (updated)
User Story: (updated)
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Iteration: 35.3 → ---
Whiteboard: [rooms][strings] → [rooms] [strings] [blocked on room info]
backlog: --- → Fx35+
Priority: -- → P1
Whiteboard: [rooms] [strings] [blocked on room info] → [rooms] [strings]
Assignee: nobody → pkerr
Comment on attachment 8525031 [details] [diff] [review]
WIP Generate system alert when someone joins a room

Review of attachment 8525031 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopService.jsm
@@ +1046,5 @@
> +        try {
> +          alertsService.showAlertNotification(
> +            "", //FIXME(PRK) what is the url to the hello icon?
> +            room.roomName.substr(0, 128), //FIXME(PRK) make a constant
> +            participant.displayName + " has joined you!", //FIXME(PRK) - add string to l10n

This should just use the string "rooms_room_joined_label".

The first round of rooms implementations isn't going to have display names, so although the server side is there, it isn't going to make sense (they'll all be guest).
WIP update with feedback
Attachment #8525031 - Attachment is obsolete: true
fix broken test
Attachment #8525534 - Attachment is obsolete: true
Attachment #8525643 - Attachment is obsolete: true
Attachment #8525646 - Flags: review?(mdeboer)
Comment on attachment 8525646 [details] [diff] [review]
Generate system alert when someone joins a room

Review of attachment 8525646 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopService.jsm
@@ +86,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "gWM",
>                                     "@mozilla.org/appshell/window-mediator;1",
>                                     "nsIWindowMediator");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "alertsService",

Please rename this to `gAlertsService`

@@ +697,5 @@
> +   *
> +   * @param {key} The element id to get strings for.
> +   * @return {Object} Localized attribute/value pairs for the element.
> +   */
> +  getStrings: function(key) {

Since you decided to touch this, let's do it properly! ;-)

So, r- for now and I think it's best to discuss how during a 1:1.

@@ +1037,2 @@
>      LoopRooms.on("joined", (e, roomToken, participant) => {
> +      log.debug("MozLoopServer: room joined", e);

nit: MozLoopService

@@ +1048,5 @@
>        }
> +
> +      LoopRooms.get(roomToken, (error, room) => {
> +        if (error) {
> +          log.debug("LoopRooms.get error", e);

log.error

@@ +1055,5 @@
> +
> +        let alertListener = {
> +          observe: function(subject, topic, data) {
> +            if (topic == "alertclickcallback" && window) {
> +              window.LoopUI.toolbarButton.node.click();

can you `delete window` here to free the reference? We don't want to rely on the observer service lifecycle.

@@ +1062,5 @@
> +        }
> +
> +        try {
> +          alertsService.showAlertNotification(
> +            "",

Can you add a comment above this line that explains why you leave this empty?

@@ +1190,2 @@
>  
> +    return JSON.stringify(strings);

Wow, this is ugly.
Attachment #8525646 - Flags: review?(mdeboer) → review-
I just talked to Mike, and I think it makes sense for Paul to work on the re-registration and push handling bugs (which we are trying to finish ASAP: bug 1028869 and bug 1076650) and for Mike to finish off this bug.  Mike said he was fine with that plan.  

Paul -- This should give you time to focus on bug 1028869 and bug 1076650 (which I'd really like to get in ASAP, especially with all the server/connectivity issues we've seen lately).  Thanks!
Assignee: pkerr → mdeboer
Depends on: 1102193
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Flags: needinfo?(mmucci)
Summary: As a user, I should have a system notification when someone joins a room, in case Firefox is minimised → As a user, I should have a system notification when someone joins a room
Added to IT 36.3
Flags: needinfo?(mmucci)
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> Comment on attachment 8525646 [details] [diff] [review]
> Generate system alert when someone joins a room
> 
> Review of attachment 8525646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +86,5 @@
> >  XPCOMUtils.defineLazyServiceGetter(this, "gWM",
> >                                     "@mozilla.org/appshell/window-mediator;1",
> >                                     "nsIWindowMediator");
> >  
> > +XPCOMUtils.defineLazyServiceGetter(this, "alertsService",
> 
> Please rename this to `gAlertsService`
> 
> @@ +697,5 @@
> > +   *
> > +   * @param {key} The element id to get strings for.
> > +   * @return {Object} Localized attribute/value pairs for the element.
> > +   */
> > +  getStrings: function(key) {
> 
> Since you decided to touch this, let's do it properly! ;-)
> 
> So, r- for now and I think it's best to discuss how during a 1:1.
> 
...
> 
> @@ +1190,2 @@
> >  
> > +    return JSON.stringify(strings);
> 
> Wow, this is ugly.

Mike, can you give me some background as to why the original code returns a json encoded structure and not a string? I simply broke up the function so that MozLoopService code could grab strings without the need to un-stringify locally. I did seem odd (ugly) to me.
Attachment #8525646 - Attachment is obsolete: true
Attachment #8526061 - Flags: review?(nperriault)
Attachment #8526061 - Flags: review?(nperriault) → review?(MattN+bmo)
Comment on attachment 8526061 [details] [diff] [review]
Patch v1: Generate system alert when someone joins a room

Review of attachment 8526061 [details] [diff] [review]:
-----------------------------------------------------------------

Only f+ since it doesn't select the proper panel tab upon click.

::: browser/components/loop/MozLoopService.jsm
@@ +1043,5 @@
>          window.LoopUI.playSound("room-joined");
> +        let alertListener = {
> +          observe: function(subject, topic, data) {
> +            if (topic == "alertclickcallback" && window) {
> +              window.LoopUI.toolbarButton.node.click();

Is this guaranteed to select the proper tab? I suspect we want to open to the Rooms tab.

@@ +1050,5 @@
> +          }
> +        };
> +
> +        try {
> +          gAlertsService.showAlertNotification(

I would rather we use the Notification WebAPI as it's easier to understand (compared to 9 arguments to a function) and it makes it easier for others to modify this code. You could the window defined above to get the Notification constructor and it seems like we already whitelist chrome privileged callers since `new Notification("foo")` works in the Browser Console. This doesn't need to block if you're in a rush but please file a follow-up.

@@ +1051,5 @@
> +        };
> +
> +        try {
> +          gAlertsService.showAlertNotification(
> +            "",

Please file a follow-up to use a Hello icon here.

@@ +1054,5 @@
> +          gAlertsService.showAlertNotification(
> +            "",
> +            room.roomName.substr(0, MAX_ALERT_TITLE_LENGTH),
> +            MozLoopServiceInternal.localizedStrings.get("rooms_room_joined_label"),
> +            true, "", alertListener, "");

The last argument you've specified is optional. If there was a reason you needed to specify "" instead of omitting it then please document it.

@@ +1056,5 @@
> +            room.roomName.substr(0, MAX_ALERT_TITLE_LENGTH),
> +            MozLoopServiceInternal.localizedStrings.get("rooms_room_joined_label"),
> +            true, "", alertListener, "");
> +        } catch (ex) {
> +          log.debug("Room joined: alertsService error", ex);

I think log.error or at least log.warn is appropriate here. Default level output is Error.
Attachment #8526061 - Flags: review?(MattN+bmo) → feedback+
Iteration: 36.3 → 37.1
Attachment #8526061 - Attachment is obsolete: true
Attachment #8528355 - Flags: review?(MattN+bmo)
Comment on attachment 8528355 [details] [diff] [review]
Patch 1.1: Generate system alert when someone joins a room

Review of attachment 8528355 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-loop.js
@@ +17,5 @@
> +   * Play a sound in this window IF there's no sound playing yet.
> +   *
> +   * @param {String} name Name of the sound, like 'ringtone' or 'room-joined'
> +   */
> +  function playSound(name) {

Can you revert the unrelated playSound changes such as moving the method to its previous location so blame doesn't change?

@@ +42,5 @@
>       *
>       * @param {event} event The event opening the panel, used to anchor
>       *                      the panel to the button which triggers it.
>       */
> +    openCallPanel: function(event, tabId) {

Add `tabId` to the JSDoc.

I think there should be tests for this part of the patch.

@@ +47,3 @@
>        let callback = iframe => {
> +        // Helper function to show a specific tab view in the panel.
> +        let showTab = () => {

It's easier to read that this is a function if it has the function keyword: 
function showTab() {

@@ +51,5 @@
> +            return;
> +          }
> +
> +          Services.tm.mainThread.dispatch(function() {
> +            let win = iframe.contentWindow;

Why do we need the dispatch?

@@ +64,5 @@
> +        };
> +
> +        if ("contentWindow" in iframe) {
> +          showTab();
> +        }

Can you explain what this is about? It seems like we will call showTab twice and listen for DOMContentLoaded unnecessarily in some cases.

@@ +136,5 @@
>        this.toolbarButton.node.setAttribute("state", state);
>      },
>  
> +    notify: function(options) {
> +      if (MozLoopService.doNotDisturb) {

"notify" is a bit too ambiguous for the method name. Something like "showNotification" won't be confused with observer notifications.

@@ +141,5 @@
>          return;
>        }
>  
> +      if ("sound" in options) {
> +        playSound(options.sound);

Have you tested this? Doesn't it conflict with the system notification sound from OS X Notification Center? It seems like we should use mozbehavior.soundFile instead of supported Gecko versions: https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Notification.webidl?rev=9712d6370c3f&mark=65,72,75#58

@@ +147,3 @@
>  
> +      if ("message" in options) {
> +        if (!("title" in options)) {

I can see us wanting to have notifications with only a title (like the Notification API allows) in the future but this is fine for now.

OOC, why are you using `in` tests instead of checking truthiness in a few places above and below? Are you fine with sound, title, message, and "icon" being "" for example?

@@ +170,5 @@
> +          } else {
> +            // Open the Loop panel as a default action.
> +            this.openCallPanel(null, options.selectTab || null);
> +          }
> +        }, false);

Nit: false is the default

::: browser/components/loop/MozLoopService.jsm
@@ +1042,5 @@
>        let window = gWM.getMostRecentWindow("navigator:browser");
>        if (window) {
> +        window.LoopUI.notify({
> +          sound: "room-joined",
> +          title: room.roomName.substr(0, MAX_ALERT_TITLE_LENGTH),

I think we should shift the responsibility for capping of the length to notification method so each caller doesn't need to do this. I'm not even sure why we need to do this anyways as I think the notification UI (alert service) should be dealing with this for us.
Attachment #8528355 - Flags: review?(MattN+bmo) → feedback+
Blocks: 1104921
(In reply to Matthew N. [:MattN] from comment #16)
> Have you tested this? Doesn't it conflict with the system notification sound
> from OS X Notification Center? It seems like we should use
> mozbehavior.soundFile instead of supported Gecko versions:
> https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Notification.
> webidl?rev=9712d6370c3f&mark=65,72,75#58

Thanks for that pointer. I checked platform support and found it lacking. This was,  as usual, a feature specifically developed for FirefoxOS. I filed bug 1105222 to make this work for desktop as well.

However, I don't think this will be resolved soon, so what do we do in the mean time? Play two sounds? Skip the second sound and only have the system's default notification sound play? I think the latter is our best option, however unfortunate that may be.
Attachment #8528355 - Attachment is obsolete: true
Attachment #8529044 - Flags: review?(MattN+bmo)
Comment on attachment 8529044 [details] [diff] [review]
Patch 1.2: Generate system alert when someone joins a room

Review of attachment 8529044 [details] [diff] [review]:
-----------------------------------------------------------------

I'll make the changes below and file a follow-up for tests.

::: browser/base/content/browser-loop.js
@@ +27,2 @@
>       */
> +    openCallPanel: function(event, tabId) {

Indicate that tabId is optional via "[tabId]" in the jsdoc and via a default value `tabId = null`

@@ +46,5 @@
> +        // If the panel has been opened and initialized before, we can skip waiting
> +        // for the content to load - because it's already there.
> +        if ("contentWindow" in iframe) {
> +          showTab();
> +        } else {

You could just return after showTab(); instead.
Attachment #8529044 - Flags: review?(MattN+bmo) → review+
Blocks: 1105788
https://hg.mozilla.org/mozilla-central/rev/86069512957c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8529044 [details] [diff] [review]
Patch 1.2: Generate system alert when someone joins a room

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: minimal notification if you're not already in a room when someone joins (icon turns blue; mild single beep).

[Describe test coverage new/current, TBPL]: on central; includes test updates - followup filed for additional unit tests.

[Risks and why]: moderate to low risk - more complex interaction with the system than most loop patches.  code isn't inherently dangerous, but possibilities of oranges may be higher.  Major improvement for users in ability to generate a semi-permanent room (when the person/people who are going to use it may join at random/non-immediate times), and semi-permanent nameable rooms are a major point to the rooms UI.  Risk limited to use of Loop/rooms

[String/UUID change made/needed]: none
Attachment #8529044 - Flags: approval-mozilla-aurora?
Note that if I have the browser on the second monitor, the notification still shows up on the first monitor.
(In reply to Paul Silaghi, QA [:pauly] from comment #24)
> Note that if I have the browser on the second monitor, the notification
> still shows up on the first monitor.

That might be worth a follow-up bug. Randell, what do you think?
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #25)
> (In reply to Paul Silaghi, QA [:pauly] from comment #24)
> > Note that if I have the browser on the second monitor, the notification
> > still shows up on the first monitor.
> 
> That might be worth a follow-up bug. Randell, what do you think?

This probably depends on the system notification manager - e.g. I'm pretty sure Mac notifies on the window where your doc is, regardless of the application.
Comment on attachment 8529044 [details] [diff] [review]
Patch 1.2: Generate system alert when someone joins a room

Approval Request Comment
Transfer request to beta
Attachment #8529044 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8529044 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Tested extensively on Linux
Verified fixed FF 35b3, 36.0a2 (2014-12-16) Win 7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.