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)
Hello (Loop)
Client
Tracking
(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)
111.34 KB,
image/png
|
Details | |
14.22 KB,
patch
|
MattN
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•10 years ago
|
||
Authoritative UX at http://people.mozilla.org/~dhenein/loop/rooms/loop-rooms-spec-4.jpg
Reporter | ||
Updated•10 years ago
|
User Story: (updated)
Reporter | ||
Updated•10 years ago
|
Severity: normal → enhancement
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
(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)
Updated•10 years ago
|
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Updated•10 years ago
|
User Story: (updated)
Updated•10 years ago
|
User Story: (updated)
Updated•10 years ago
|
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Iteration: 35.3 → ---
Whiteboard: [rooms][strings] → [rooms] [strings] [blocked on room info]
Updated•10 years ago
|
backlog: --- → Fx35+
Updated•10 years ago
|
Priority: -- → P1
Updated•10 years ago
|
Whiteboard: [rooms] [strings] [blocked on room info] → [rooms] [strings]
Updated•10 years ago
|
Assignee: nobody → pkerr
Comment 4•10 years ago
|
||
WIP
Reporter | ||
Comment 5•10 years ago
|
||
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).
Comment 6•10 years ago
|
||
WIP update with feedback
Updated•10 years ago
|
Attachment #8525031 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
fix broken test
Updated•10 years ago
|
Attachment #8525534 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
Updated•10 years ago
|
Attachment #8525643 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8525646 -
Flags: review?(mdeboer)
Assignee | ||
Comment 9•10 years ago
|
||
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-
Comment 10•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Flags: needinfo?(mmucci)
Assignee | ||
Updated•10 years ago
|
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
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8525646 -
Attachment is obsolete: true
Attachment #8526061 -
Flags: review?(nperriault)
Assignee | ||
Updated•10 years ago
|
Attachment #8526061 -
Flags: review?(nperriault) → review?(MattN+bmo)
Comment 14•10 years ago
|
||
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+
Updated•9 years ago
|
Iteration: 36.3 → 37.1
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8526061 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8528355 -
Flags: review?(MattN+bmo)
Comment 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
(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.
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8528355 -
Attachment is obsolete: true
Attachment #8529044 -
Flags: review?(MattN+bmo)
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92b25a1ce87f
Assignee | ||
Comment 21•9 years ago
|
||
Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/86069512957c
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/86069512957c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 23•9 years ago
|
||
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?
Comment 24•9 years ago
|
||
Note that if I have the browser on the second monitor, the notification still shows up on the first monitor.
Comment 25•9 years ago
|
||
(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?
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
Reporter | ||
Comment 26•9 years ago
|
||
(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 27•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8529044 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•9 years ago
|
||
Tested extensively on Linux
Comment 30•9 years ago
|
||
Verified fixed FF 35b3, 36.0a2 (2014-12-16) Win 7
You need to log in
before you can comment on or make changes to this bug.
Description
•