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

Status: VERIFIED FIXED

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

Comment 1

[Mark Banner (:standard8)]

Attachment: Expected UX

Authoritative UX at http://people.mozilla.org/~dhenein/loop/rooms/loop-rooms-spec-4.jpg

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 36.2

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: add support for room updates, fix event dispatching and support room participant processing

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: no ws.

Comment 5

[Mark Banner (:standard8)]

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.

Comment 6

[Mike de Boer [:mikedeboer]]

Pushed this first part to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/9a8ba4ecc4c0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a8ba4ecc4c0

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Part 2: change the toolbar icon when participants join and leave

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Part 3: play a sound when a participant joined a room

Comment 10

[Mark Banner (: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?

Comment 11

[Mark Banner (: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.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e0cd0e35ec15

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 14

[Maire Reavy [:mreavy]]

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?

Comment 15

[Mike de Boer [:mikedeboer]]

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.

Comment 16

[Maire Reavy [:mreavy]]

(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?

Comment 17

[Paul Kerr [:pkerr]]

(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.

Comment 18

[Paul Kerr [:pkerr]]

Attachment: play a sound when a participant joined a room

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.

Comment 19

[Maire Reavy [:mreavy]]

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.

Comment 20

[Paul Kerr [:pkerr]]

(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?

Comment 21

[Mike de Boer [:mikedeboer]]

ideally sounds should only be played when a remote member joins, bug 1095533 would help a lot there

Comment 22

[Mike de Boer [:mikedeboer]]

You can reed my answer wrt sounds in comment 15.

Comment 23

[Paul Kerr [:pkerr]]

Attachment: change the toolbar icon when participants join and leave

Added test of icon changed on room partipants > 0

Comment 24

[Paul Kerr [:pkerr]]

Comment on attachment 8522622 [details] [diff] [review]
play a sound when a participant joined a room

Added test of icon change.

Comment 25

[Paul Kerr [:pkerr]]

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.

Comment 26

[Mark Banner (: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/

Comment 27

[Mark Banner (:standard8)]

Attachment: Part 2 Change the toolbar icon when participants join and leave.

Update patch with my review nit. I also added a missing ; that I hadn't noticed previously.

Comment 28

[Mark Banner (:standard8)]

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

Comment 29

[Mark Banner (:standard8)]

Attachment: Part 3 Play a sound when a participant joined a room.

Updated for review comments and to fix the other issue I mentioned.

Comment 30

[Mark Banner (:standard8)]

I took the liberty of getting these landed:

https://hg.mozilla.org/integration/fx-team/rev/1258fcdd5f05
https://hg.mozilla.org/integration/fx-team/rev/5fa62659358d

Comment 31

[Mark Banner (:standard8)]

I split out the missing test to bug 1100348.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1258fcdd5f05
https://hg.mozilla.org/mozilla-central/rev/5fa62659358d

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/979e6f0add7c
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca2730f13b5c

Comment 36

[Paul Kerr [:pkerr]]

Attachment: =: retry registration on 401

WIP. Need to review tests

Comment 37

[Mike de Boer [:mikedeboer]]

(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?

Comment 38

[Paul Kerr [:pkerr]]

Yes, wrong bug. I should always just cut and paste the number.

Comment 39

[Paul Kerr [:pkerr]]

Comment on attachment 8534981 [details] [diff] [review]
=: retry registration on 401

Attached to wrong bug.

Comment 40

[Mike de Boer [:mikedeboer]]

(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' ;)

Comment 41

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35b4, 36.0a2 (2014-12-17) Ubuntu 13.04

Comment 42

[Paul Silaghi, QA [:pauly]]

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?

Comment 43

[Paul Kerr [:pkerr]]

Mike, see Paul's comment above.

Comment 44

[Mike de Boer [:mikedeboer]]

(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!