972019 - Desktop client needs the ability to terminate a call

Status: VERIFIED FIXED

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

Comment 1

[Mark Banner (:standard8)]

Romain, please update the user story, and if we can get a mock-up from Boriss that would be good.

Comment 2

[Mark Banner (:standard8)]

Also, what's the UX for end of call? Do we just close the window?

Comment 3

[Romain Testard [:RT]]

Title changed, we really only need to be able to terminate a call.
When the call terminates, both ends are prompted for user feedback (974873).
I am not sure about the UX, Boriss can update us here?

Comment 4

[Maire Reavy [:mreavy]]

Mark -- I'm assigning this to you as part of bug 972024.  Thanks.

Comment 5

[Mark Banner (:standard8)]

Just to clarify this is for desktop & client. If it makes sense to split it up, then we should divide into two bugs.

Note that PR 6 on loop-client has just made some models & views shared, and the establish-call branch I hope to land on gecko-dev on Monday.

We should continue with the theme of sharing models & views as much as possible.

Comment 6

[Romain Gauthier [:tOkeshu]]

Attachment: Terminate a call (gecko-dev)

Comment 7

[Romain Gauthier [:tOkeshu]]

Attachment: Terminate a call (loop-client)

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8401329 [details] [review]
Terminate a call (loop-client)

As commented in the PR, I think this needs a few more tweaks before we can take it.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8401327 [details] [review]
Terminate a call (gecko-dev)

As commented on the PR, we're not hiding things when we should be, and I think we need to keep the ended logic of the router.

Comment 10

[Romain Gauthier [:tOkeshu]]

As discussed with Niko, I reassign this bug to him.

Comment 11

[Mark Banner (:standard8)]

I just expanded the user story to include ensuring the correct reaction at both ends - namely releasing video & audio devices & notifying the user.

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

There's currently a bug in the SDK preventing to retrieve the connection information from a `clientDisconnect` event, so we can't identify from which part the call has been actually terminated.

The docs [1] describes a `connection` property being attached to the event object, but it's undefined.

[1]: http://tokbox.com/opentok/libraries/client/js/reference/ConnectionEvent.html

Toggling the bug as confidential because partner is mentioned.

Comment 13

[Adam Roach [:abr]]

(In reply to Nicolas Perriault (:NiKo`) from comment #12)
> There's currently a bug in the SDK preventing to retrieve the connection
> information from a `clientDisconnect` event, so we can't identify from which
> part the call has been actually terminated.
> 
> The docs [1] describes a `connection` property being attached to the event
> object, but it's undefined.

Response from TB engineering:

Here is how it is supposed to work (and I believe it does):

For a session with two clients, A and B:

When "Client A" disconnects, Client A receives a sessionDisconnected event indicating that "Client A" has disconnected from the session. "Client B" receives a ConnectionDestroyed event with the connection object of "Client A". 

At this point "Client B" is still connected to the session. 

Does that make sense and is that what you are seeing? It was not clear from Nicolas's email which client's perspective he was considering. If this is not what you are seeing, perhaps we have a bug and it would be helpful if you could let us know what events are seen by each client in the session. If it is what you are seeing, perhaps we can improve our documentation.

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/mozilla/loop-client/pull/8

Call termination & notifications patch. Note that there's room for factorizing a lot between ConversationRouter and WebappRouter, but the patch is large enough already. We should file an issue for this refactoring which should be addressed in a next PR.

Comment 15

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/adamroach/gecko-dev/pull/12

Desktop side version of the patch. Same remark as with previous patch regarding routers logic factorization.

Comment 16

[Mark Banner (:standard8)]

I think the current patches can land once reviewed, but before we can close this bug, we need to address the following:

* We need some sort of "Your call has ended" confirmation after the user clicks stop (at either end) - having a blank screen doesn't look good
* The stop button should be removed from the display on the desktop end
* After the call is ended on the link-clicker end, the "Start this call" button, should either be "restart" and enabled, or not visible
* It would be nice if we could sort out the video layout so that the local video isn't flickering around at the start/end of call, but I think this would be best in another bug

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8403923 [details] [review]
https://github.com/mozilla/loop-client/pull/8

Looks good once my latest comment is addressed.

r=Standard8. Please squash commits and add the r= before landing.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8403925 [details] [review]
https://github.com/adamroach/gecko-dev/pull/12

Looks good, once the comments on the PR are addressed.

r=Standard8. Please squash commits and add the r= before landing.

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

https://github.com/mozilla/loop-client/pull/8 patch landed: https://github.com/mozilla/loop-client/commit/49fc40c67af491640d7eaa2dd748b78e274144da

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

https://github.com/adamroach/gecko-dev/pull/12 patch landed: https://github.com/adamroach/gecko-dev/commit/2bf6d1318eeb22bc0f697ee67bb67a5dbe7f3598

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/mozilla/loop-client/pull/18

This patch eases sharing part of the conversation routing logic, avoiding duplicating code between Desktop and Webapp. This is the webapp (loop-client) part of the patch.

Comment 22

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: https://github.com/adamroach/gecko-dev/pull/14

Gecko part of the shared conversation router patch.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8404971 [details] [review]
https://github.com/mozilla/loop-client/pull/18

r=dmose; comments in the PR

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8404972 [details] [review]
https://github.com/adamroach/gecko-dev/pull/14

r=dmose; comments in the PR.  Thanks for the this de-duplification of code!

Comment 25

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Landed https://github.com/mozilla/loop-client/pull/18 with r=dmose: https://github.com/mozilla/loop-client/commit/0141aee2225c9d2b2c04052932ff05189afebee8

Comment 26

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Landed https://github.com/adamroach/gecko-dev/pull/14 with r=dmose: https://github.com/adamroach/gecko-dev/commit/4fe7a83d6571867b5f87b5c05aed5a918cf83947

Comment 27

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Created new bugs for remaining issues which no longer blocks this:

- bug 997178: Loop client should allow end user to restart a terminated conversation
- bug 997181: Fix flickering video layout when a call starts/ends

Closing this one.

Comment 28

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/1e682ccc611b
https://hg.mozilla.org/projects/elm/rev/31a076cedfa8
https://hg.mozilla.org/projects/elm/rev/f6773284d67f
https://hg.mozilla.org/projects/elm/rev/097354ffa089

Comment 29

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/1e682ccc611b
https://hg.mozilla.org/mozilla-central/rev/31a076cedfa8
https://hg.mozilla.org/mozilla-central/rev/f6773284d67f
https://hg.mozilla.org/mozilla-central/rev/097354ffa089

Comment 30

[Stefan Plewako [:stef]]

Please use meaningful string ids in the future.

Comment 31

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> Please use meaningful string ids in the future.

Please define "meaningful string ids", thanks.

Comment 32

[Stefan Plewako [:stef]]

(In reply to Nicolas Perriault (:NiKo`) from comment #31)
> Please define "meaningful string ids", thanks.

https://bugzilla.mozilla.org/show_bug.cgi?id=972020#c19 and 20

Comment 33

[u279076]

Verified fixed via previous smoketesting.