1198841 - Brief message to invite someone to join when joining a room with someone already there

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Ed Lee :Mardak]

Split from bug 1126733 comment 22. Desktop might be able to detect participants a different way than standalone to avoid showing the invite message.

Comment 1

[Ed Lee :Mardak]

Attachment: wip

This adds a boolean roomHasGuests to SetupRoomInfo and UpdateRoomInfo.

I used guests instead of counting participants because before joining owner's room, if already occupied, participants = 1, guests = 1

After joining: participants = 2, guests = 1

After guest leaves: participants = 1, guests = 0

Alternatively, we could pass over the raw participants array as part of the room data?

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8653971 [details] [diff] [review]
wip

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

I'm torn with working out if there's guests in the activeRoomStore versus the view. It feels like its putting view logic into the activeRoomStore, but it my be more convenient in future if we add more functionality around guests. I think I would marginally prefer the guest logic in the view and store the participants list. 

Having tested it out, the other thing I'm slightly concerned about is what happens when someone leaves the room - there's a delay due to the push notification before we re-show the invite view. What I think we should be able to do here is in remotePeerDisconnected we remove the guest from activeRoomStore's participant list, this would then show the invite as soon as they leave.

Lastly, I think the other thing I'm concerned about is if a peer was in the room, and timed out. The client isn't currently notified about it, so we'd end up not displaying the invite. Maybe in this case, we should have the additional timeout of, say 5 seconds, just so that we get something displayed?

::: browser/components/loop/content/js/roomViews.jsx
@@ +593,5 @@
>          }));
>      },
>  
>      _shouldRenderInvitationOverlay: function() {
> +      return this.state.roomState !== ROOM_STATES.HAS_PARTICIPANTS &&

If you could add jsdoc once this function is updated, that'd be really nice.

Comment 3

[Ed Lee :Mardak]

Attachment: wip v2

(In reply to Mark Banner (:standard8) from comment #2)
> I think I would marginally prefer the guest logic in the view and
> store the participants list. 
Is this patch what you meant?

> What I think we should be able to do here is in remotePeerDisconnected
> we remove the guest from activeRoomStore's participant list
We currently only support owner and guest, so is filtering for just the owner acceptable? 

> Lastly, I think the other thing I'm concerned about is if a peer was in the
> room, and timed out. The client isn't currently notified about it, so we'd
> end up not displaying the invite. Maybe in this case, we should have the
> additional timeout of, say 5 seconds, just so that we get something
> displayed?
Timeout from when?

Also, from my testing, a guest leaving normally dispatches:

1) connectionStatus Session.streamDestroyed
2) dataChannelsAvailable
3) connectionStatus Session.connectionDestroyed
4) remotePeerDisconnected
5) updateRoomList
6) updateRoomInfo

Whereas forcing the guest's browser to quit:

1) connectionStatus Session.streamDestroyed
2) dataChannelsAvailable
3) connectionStatus Session.connectionDestroyed
4) remotePeerDisconnected

And only later does updateRoomList happen. But if someone tries to join the room again, it'll say "There are already two people in this conversation."

With the updating of participants on remotePeerDisconnected, it'll correctly show the invite message right away.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8654345 [details] [diff] [review]
wip v2

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

(In reply to Ed Lee :Mardak from comment #3)
> (In reply to Mark Banner (:standard8) from comment #2)
> > I think I would marginally prefer the guest logic in the view and
> > store the participants list. 
> Is this patch what you meant?

Yes, looks reasonable.

> > What I think we should be able to do here is in remotePeerDisconnected
> > we remove the guest from activeRoomStore's participant list
> We currently only support owner and guest, so is filtering for just the
> owner acceptable? 

Yes, I think that's fine.

> And only later does updateRoomList happen. But if someone tries to join the
> room again, it'll say "There are already two people in this conversation."
> 
> With the updating of participants on remotePeerDisconnected, it'll correctly
> show the invite message right away.

Yeah, that should be enough. I think that should work fine.

Comment 5

[Ed Lee :Mardak]

Attachment: v1

Also fixes some tests that were using findRenderedComponentWithType expecting it to only succeed if it was rendering content, but the component can be found even if its render returns null.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8656452 [details] [diff] [review]
v1

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

This is almost there, but I think we should try and make the code a little bit more readable.

::: browser/components/loop/content/js/roomViews.jsx
@@ +602,5 @@
> +      // Don't show if the room has participants whether from the room state or
> +      // there being non-owner guests in the participants array.
> +      return this.state.roomState !== ROOM_STATES.HAS_PARTICIPANTS &&
> +        (typeof this.state.participants !== "object" ||
> +         this.state.participants.filter(function(participant) {

Sorry, I should have mentioned this before, please can you split the typeof and filter out to their own additional function or to a separate variable?

The return statement will then be easier to read as it'll be something like

return this.state.roomState !== ROOM_STATES.HAS_PARTICIPANTS && !hasGuests;

That'll be easier to understand what's happening a bit quicker when reading the code.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +771,5 @@
>      remotePeerDisconnected: function() {
>        this.setStoreState({
> +        // Update the participants to just the owner.
> +        participants: this.getStoreState("participants") &&
> +          this.getStoreState("participants").filter(function(participant) {

Similarly, it'd be nice to separate this out to a separate function/variable, so we're not doing the work inside the place where we're using the data.

Comment 7

[Ed Lee :Mardak]

Attachment: v2

(In reply to Mark Banner (:standard8) from comment #6)
> The return statement will then be easier to read as it'll be something like
> return this.state.roomState !== ROOM_STATES.HAS_PARTICIPANTS && !hasGuests;
Moved the negated logic to hasGuests variable.

> > +        participants: this.getStoreState("participants") &&
> > +          this.getStoreState("participants").filter(function(participant) {
> it'd be nice to separate this out to a separate function/variable
Moved this a participants variable.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8657126 [details] [diff] [review]
v2

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

Looks good, r=Standard8

Comment 9

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/ba7eec9f555db8ed6387fe1606574dfdc0bf9c8f
Bug 1198841 - Brief message to invite someone to join when joining a room with someone already there [r=Standard8]

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba7eec9f555d