Closed Bug 1074666 Opened 5 years ago Closed 5 years ago

As a user, I should be notified when someone joins a room

Categories

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

enhancement
Points:
8

Tracking

(firefox35 verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- verified
firefox36 --- verified
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: mikedeboer)

References

()

Details

(Whiteboard: [rooms])

User Story

* As a user, I should be notified when someone joins a room, so that I know they are present.

UX:
* The toolbar icon should be "Blue" whenever there is at least one room with one participant in
* Play a sound

Implementation:
* When a store is updated, if the number of participants across all rooms is non-zero, change the style of the toolbar icon to be "Blue". It only reverts to "Normal" when the number of participants across all rooms is zero.
* When an increase of participants is detected in any room, play a sound

Attachments

(4 files, 6 obsolete files)

111.34 KB, image/png
Details
17.51 KB, patch
standard8
: review+
Details | Diff | Splinter Review
7.08 KB, patch
standard8
: review+
Details | Diff | Splinter Review
32.29 KB, patch
standard8
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
Blocks: 1074667
Blocks: 1074659
Severity: normal → enhancement
User Story: (updated)
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
Status: ASSIGNED → NEW
Iteration: 35.3 → ---
Whiteboard: [rooms] → [rooms] [blocked on room info]
Assignee: MattN+bmo → nobody
backlog: --- → Fx35+
Priority: -- → P1
No longer depends on: 1074665
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Depends on: 1087041
Iteration: --- → 36.2
Flags: needinfo?(mmucci)
Added to IT 36.2
Flags: needinfo?(mmucci)
Attached patch Part 1: no ws. (obsolete) — Splinter Review
Comment on attachment 8514345 [details] [diff] [review]
Part 1: add support for room updates, fix event dispatching and support room participant processing

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

Ok, this looks good. r=Standard8 with the comment addressed.

::: browser/components/loop/LoopRooms.jsm
@@ +74,5 @@
> +const checkForParticipantsUpdate = function(room, updatedRoom) {
> +  // If there are no participants in the room to begin with, ignore this update.
> +  if (!("participants" in room) || !Array.isArray(room.participants) ||
> +      !Array.isArray(updatedRoom.participants)) {
> +    return;

As discussed on irc, this appears to be unnecessary validation - or validation in the wrong place. So it should just be able to be removed.
Attachment #8514345 - Flags: review?(standard8) → review+
Pushed this first part to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/9a8ba4ecc4c0
Keywords: leave-open
Depends on: 1093301
Depends on: 1088650
Attachment #8514849 - Attachment is obsolete: true
Attachment #8518983 - Flags: review?(standard8)
Attachment #8518985 - Flags: review?(standard8)
Comment on attachment 8518983 [details] [diff] [review]
Part 2: change the toolbar icon when participants join and leave

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

This looks fine, but could you write some tests before we land this?
Attachment #8518983 - Flags: review?(standard8)
Comment on attachment 8518985 [details] [diff] [review]
Part 3: play a sound when a participant joined a room

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

I think the general plan looks good, but there's a few questions that I'm unsure of. Also, I think we should add tests for this before we land it.

::: browser/components/loop/LoopRooms.jsm
@@ +174,5 @@
>            checkForParticipantsUpdate(orig, room);
>          }
> +        // Remove the `currSize` for posterity.
> +        if ("currSize" in room) {
> +          delete room.currSize;

Did this creep into the wrong patch?

::: browser/components/loop/MozLoopService.jsm
@@ +999,5 @@
> +      }
> +
> +      let window = gWM.getMostRecentWindow("navigator:browser");
> +      if (window) {
> +        window.LoopUI.playSound("room-joined");

According to bug 1088650, there's for situations where sounds are played with different sounds:

* Rooms - Someone joined a room you own notification (when you are not in the room) 
* Rooms - Room creator joins a room he created
* Rooms - Someone joins a room you are already in 
* Rooms - Other person leave a room when the creator is still in the room

At the moment, I can only get this to work when the first person enters the room, regardless of if they are the own or a visitor.

What's the plan for tackling these cases?

Should we do only the first in gecko and the rest in the conversation window?

::: browser/components/loop/content/shared/js/mixins.js
@@ +152,5 @@
>       *
>       * @param {String} name The filename to play (excluding the extension).
>       */
>      play: function(name, options) {
> +      if (this._isLoopDesktop() && rootObject.navigator.mozLoop.doNotDisturb) {

I'm not sure we need this here at this time. The mixin is only currently used from the conversation window (which doesn't open in dnd mode), and I'm not sure it'd be used from the panel.

The interesting question it does raise, is what happens, if you're in a room/conversation and you switch to dnd mode - should sounds be stopped? I'm not quite sure about this, but I think I'm verging on "no".

If we do keep this, then this should get a unit test created for it somewhere - probably in mixins_test.js.
Attachment #8518985 - Flags: review?(standard8) → review-
Target Milestone: --- → mozilla36
Iteration: 36.2 → 36.3
Comment on attachment 8514345 [details] [diff] [review]
Part 1: add support for room updates, fix event dispatching and support room participant processing

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8514345 - Flags: approval-mozilla-aurora?
Whiteboard: [rooms] [blocked on room info] → [rooms]
Attachment #8514345 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mike -- How much of the desktop sounds work will this bug cover?  Will it cover all the work described in bug 1088650 (once it's complete) or just some?  Are you planning more than 3 patches for this bug?
Flags: needinfo?(mdeboer)
For this bug I only needed 1 sound. I suspect the other sounds will be implemented by other bugs once they're needed.
We can also just use bug 1088650 tot land the sound files in the tree, so we can move it out of the way.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #15)
> For this bug I only needed 1 sound. I suspect the other sounds will be
> implemented by other bugs once they're needed.
> We can also just use bug 1088650 to land the sound files in the tree, so we
> can move it out of the way.

That makes sense.  Do you want to take bug 1088650, or should I find someone else?
Flags: needinfo?(mdeboer)
(In reply to Mark Banner (:standard8) from comment #11)
> Comment on attachment 8518985 [details] [diff] [review]
> Part 3: play a sound when a participant joined a room
> 
> Review of attachment 8518985 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/components/loop/LoopRooms.jsm
> @@ +174,5 @@
> >            checkForParticipantsUpdate(orig, room);
> >          }
> > +        // Remove the `currSize` for posterity.
> > +        if ("currSize" in room) {
> > +          delete room.currSize;
> 
> Did this creep into the wrong patch?
> 
Removal of this attribute was suggested during the discussion among Adam, Mike and me where the change from get/rooms returning a list of rooms each with a very abbreviated set of attributes to returning all the information for each room as if get/rooms/<token> was sent. The currSize attribute was the only one that appeared only on the list of rooms and as a consequence was not going to be returned in the new version of the rooms API. Removing it now kept anyone from using this value to count room participants in the short while that the old API was running. Later, when getAll() is re-written for the new API (since it won't need to cycle through the rooms in the list to get the rest of the attributes) this can be dropped. (This did exist in a version of 1074664 at one point.)

It is only necessary to remove this attribute in the getAll() function, and not the get() function, if we want to go with this measure.
WIP: Fixed broken test in conversation_test.js and added AudioMixin unit tests to mixins_test.js. Made update to playSound logic suggested by Mike.
Attachment #8518985 - Attachment is obsolete: true
Assignee: mdeboer → pkerr
Thanks for taking this, Paul.  

Note standard8's review comment on the "part 2" patch (Comment 10).  We need tests for part 2.  At this point I'd be ok with landing part 2 to get it as much bake time as possible with a "part 4" patch adding the tests for part 2.
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #19)
> Thanks for taking this, Paul.  
> 
> Note standard8's review comment on the "part 2" patch (Comment 10).  We need
> tests for part 2.  At this point I'd be ok with landing part 2 to get it as
> much bake time as possible with a "part 4" patch adding the tests for part 2.

Yes, I saw the comment. I have an outline of a test for the icon state. It could become a part 4, of course.

What I am not sure about is the answer to Standard8's question on the bug 1088650 and the different sounds. That is, does part 3 sufficiently address this bug or is more work needed. Have Mike's follow-on comments cleared things up?
Flags: needinfo?(mdeboer)
ideally sounds should only be played when a remote member joins, bug 1095533 would help a lot there
Depends on: 1095533
You can reed my answer wrt sounds in comment 15.
Added test of icon changed on room partipants > 0
Attachment #8518983 - Attachment is obsolete: true
Comment on attachment 8522622 [details] [diff] [review]
play a sound when a participant joined a room

Added test of icon change.
Attachment #8522622 - Flags: review?(standard8)
Comment on attachment 8523158 [details] [diff] [review]
change the toolbar icon when participants join and leave

Added tests and some code updates. I have not extended the ability to play more than the one sound.
Attachment #8523158 - Flags: review?(standard8)
Comment on attachment 8523158 [details] [diff] [review]
change the toolbar icon when participants join and leave

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

Note: its useful to keep the Part numbers on a patch, especially when there's inter-dependencies between the patches.

Otherwise, looks good. r=Standard8 with the one nit fixed.

::: browser/components/loop/LoopRooms.jsm
@@ +121,5 @@
>    },
>  
>    /**
> +   * @var {Number} participantsCount The total amount of participants currently
> +   *                                 inside a room.

nit: s/inside a room/inside all rooms/
Attachment #8523158 - Flags: review?(standard8) → review+
Update patch with my review nit. I also added a missing ; that I hadn't noticed previously.
Attachment #8523158 - Attachment is obsolete: true
Attachment #8523821 - Flags: review+
Comment on attachment 8522622 [details] [diff] [review]
play a sound when a participant joined a room

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

I think this should really get a mochitest to ensure the sound gets played on join. I'm happy for that to come in a follow-up bug though, given we need to get this landed.

Note: there's one more part which means detecting participants joining doesn't quite work at the moment, this is due to bug 1100318. I'll attach an updated patch that addresses it, and the review comments.

::: browser/components/loop/MozLoopService.jsm
@@ +1011,5 @@
>      LoopRooms.on("add", onRoomsChange);
>      LoopRooms.on("update", onRoomsChange);
> +    LoopRooms.on("joined", (e, roomToken, participant) => {
> +      if (MozLoopServiceInternal.doNotDisturb) {
> +        return;

We should also be adding a check where that the participant isn't the owner (the owner gets a different sound). I'd rather do the owner part in content code, as that seems more appropriate to go with the rest of the sounds (driven by direct user action, rather than a push notification, potentially sometime after the initial action).

::: browser/components/loop/test/shared/mixins_test.js
@@ +176,5 @@
> +        getAudioBlob: sinon.spy(function(name, callback) {
> +          callback(null, new Blob([new ArrayBuffer(10)], {type: 'audio/ogg'}));
> +        })
> +      };
> +      

nit: unnecessary whitespace on blank line
Attachment #8522622 - Flags: review?(standard8) → review+
Updated for review comments and to fix the other issue I mentioned.
Attachment #8522622 - Attachment is obsolete: true
Attachment #8523838 - Flags: review+
Blocks: 1100348
I split out the missing test to bug 1100348.
Comment on attachment 8523821 [details] [diff] [review]
Part 2 Change the toolbar icon when participants join and leave.

Approval Request Comment

requirement for Loop Rooms support for Aurora/35
On m-c; testing with rooms server changes now live in production

[String/UUID change made/needed]: none
Attachment #8523821 - Flags: approval-mozilla-aurora?
Comment on attachment 8523838 [details] [diff] [review]
Part 3 Play a sound when a participant joined a room.

Approval Request Comment

requirement for Loop Rooms support for Aurora/35
On m-c; testing with rooms server changes now live in production

[String/UUID change made/needed]: none
Attachment #8523838 - Flags: approval-mozilla-aurora?
Attachment #8523821 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8523838 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached patch =: retry registration on 401 (obsolete) — Splinter Review
WIP. Need to review tests
(In reply to Paul Kerr [:pkerr] from comment #36)
> Created attachment 8534981 [details] [diff] [review]
> =: retry registration on 401
> 
> WIP. Need to review tests

Wrong bug?
Yes, wrong bug. I should always just cut and paste the number.
Comment on attachment 8534981 [details] [diff] [review]
=: retry registration on 401

Attached to wrong bug.
Attachment #8534981 - Attachment is obsolete: true
(In reply to Paul Kerr [:pkerr] from comment #38)
> Yes, wrong bug. I should always just cut and paste the number.

I'd advise something less destructive, like 'copy' ;)
Verified fixed FF 35b4, 36.0a2 (2014-12-17) Ubuntu 13.04
Status: RESOLVED → VERIFIED
Actually, if I'm not in the room and someone joins in, the system notification shows up and the icon gets blue, but there is no sound. Thoughts?
Flags: needinfo?(pkerr)
Mike, see Paul's comment above.
Assignee: pkerr → mdeboer
Flags: needinfo?(pkerr) → needinfo?(mdeboer)
See Also: → 1113723
(In reply to Paul Silaghi, QA [:pauly] from comment #42)
> Actually, if I'm not in the room and someone joins in, the system
> notification shows up and the icon gets blue, but there is no sound.
> Thoughts?

Filed as bug 1113723. Thanks!
Flags: needinfo?(mdeboer)
You need to log in before you can comment on or make changes to this bug.