977056 - [Dialer] Visual discrepancies: Call ended view under conference call participants list

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Noemí Freire (:noemi)]

Remove the visual gap between the UX proposal and the current implementation for the following scenario:
scenario a- Call ended view under conference call participant list
1. Pending to be implemented

Comment 1

[Noemí Freire (:noemi)]

Attachment: a- Call ended view under conference call participants list

Comment 2

[Germán Toro del Valle (:gtorodelvalle)]

Hi Carrie, we would need some rethinking about this case. Right now in master what we are doing when there is an established conference call and the participant list overlay is shown is to hide the overlay and to show a banner in the main call screen noticing that certain participant has left the conference.

The new proposal aims to show a "Call Ended" message and the conference call duration in the overlay when any participant leaves the conference call. I have several doubts about this:
  1) Should we keep the participant list overlay visible after any participant leaves or should we hide it once the "Call Ended" message and call duration is shown to go back to the call screen?
  2) Related to the previous one. What happens when there are only 3 participants in the call, this is the one hosting it and 2 others and one of them leaves the conference. In that case, the conference call is finished and a normal (1 to 1) call is going one. Which flow and information should be follow then?

Thanks!

Comment 3

[Carrie Wang [New account: [e-mail]]]

Hi Germán, 
I wonder where the new proposal came from?
I think if we are going to provide this "call end & call duration" information, then it will be the only change of the whole interaction. I think we can still follow the original behavior form the spec that I uploaded here. Please check p. 17-18. Users can tap the group call module to trigger the participant list when they receive the "leave" banner. The participant list will then display the end call and duration info besides the participant name (like the attachment Noemi uploaded here) and users can tap the back key to close the window and get back to the main call screen. 
About the 2nd issue, I suggest that if there is only two callers at a time (includes the host), we shall get back to the normal call scenario (not applying the group call scenario, no participant list). Thanks!

Comment 4

[Carrie Wang [New account: [e-mail]]]

Attachment: Dialer UX Spec 0.7(1).pdf

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Hi Carrie, thank you very much for your reply but maybe I did not make myself clear, sorry! :-)

Let me try to rephrase it. Imagine there is a established conference call and the user clicks on the header to show the participant list. At this very point (with the participant list shown), some participant leaves the conference and we show the suggested "Call ended" message as specified at attachment 8382142 [details]. Related to this:
  1) Should we keep the participant list overlay visible after any participant leaves or should we hide it once the "Call Ended" message and call duration is shown to go back to the call screen? In the current implementation in master we just hide the overlay and do not show any "Call ended" message.
  2) Related to the previous one. What happens when there are only 3 participants in the call, this is the one hosting it and 2 others and one of them leaves the conference. In that case, the conference call is finished and a normal (1 to 1) call is going one. Which flow of screens and information should be follow then?
The important issue here is that the participant leaves the conference call WHILE the participant list is shown.
Thanks again!

Comment 7

[Carrie Wang [New account: [e-mail]]]

Hi Germán,

1)
If the user is in the participant list (5 callers) and someone just left the group call, it will show "Call ended" and call duration beside the name. After 3 seconds, we shall update the view to show remaining callers (4 callers, the one who left will disappear) and users can then tap back key to get back to the main call screen.

2-1)
If the user is in the participant list (2 callers) and one of them left, then follow the same behavior as 1), but if the user get back to the main call screen, the conference call module on the top (which showed "Group call (3)") will disappear and display the normal 1-1 call screen (which shows the caller's name) so that the user can't get to the participant list anymore.

2-2)
If the user is in the main call screen and one of the two caller has left the group call, the banner will show who left the call and the call module disappear and be replaced with the normal 1-1 call screen which displays the only caller's name.

Does this make sense to you?

I'll try to update the spec (but might take some time cause I'm on my PTO) but if you need more info, please don't hesitate to ping me. Thanks!

Comment 8

[Germán Toro del Valle (:gtorodelvalle)]

Sounds good to me except from one minor issue regarding 2.1) and more concretely the overlay shown when one of the 3 participants leaves the conference. In that case, should we show attachment 8382142 [details] with the text "Conference (1)" in the header and the only other party of the call? Notice that there is no conference call going on once the third participant leaves the conference. Thanks!

There's no hurry regarding replying to this since I don't think I'll start working on it before you come back from your PTO. So enjoy! ;-) Thanks!

Comment 9

[Anthony Ricaud (:rik)]

Wilfred: We don't think we can take this during this sprint and it feels like this is "nice to have" feature and not a requirement of the visual refresh. Can you confirm this feature can ride the next train?

Comment 10

[Wilfred Mathanaraj [:WDM]]

not part of 2.0 VR work - so can be moved to 2.1 or later.

Comment 11

[Stephany Wilkes]

Unblocking 2.0 based on Wilfred's comment #10, noted as being for 2.1, and cleared the ni? for Carrie.

Comment 13

[Germán Toro del Valle (:gtorodelvalle)]

Hi Carrie, although I'll try to contact you via IRC to clarify the doubts which came to my mind :p Would you be so kind to have a look at comment 8?, please :)

On the other hand, there could be a problem or weird situation related to 2.1 in comment 7. You mention to follow the same behaviour as in 1, so we keep the participan list overlay until the user closes it. The edge case would be the following: imagine a conference call among 3 people. The DuT is currently showing the participant list overlay. One of the 2 other parties leaves the conference and according to your proposal we keep the overlay shown but including only the other party with whom the DuT has a 1 to 1 call established. Imagine that that other party also hangs up the call. How do we proceed in that case? Take into consideration that since the call would be disconnected the call screen will disappear upwards as soon as the other party hang ups the call.

To this regard, my proposal would be to close the participant list overlay once a conference call ends. We would show the "Call Ended - 12:34" message on the participant list overlay (if shown) and after 3 seconds we would close the overlay. Below the user will find a typical 1 to 1 call to the still connected party, which is what it is going on in fact. What do you think? (BTW, this behaviour would also solve the issue commented in comment 8 :) )

If you need further details, please do not hesitate to contact me via IRC.

Comment 14

[Carrie Wang [New account: [e-mail]]]

Hi Germán,

Yes, I agree with you here. I think we should directly switch to the general 1-1 call page when there is only user and the other party in this conference call (which means, it's not a "conference call" anymore).
So after the second last party leaves the call, there will be 3 seconds to display "Call ended" + time duration, and then the page will close and directly switch back to typical 1-1 page.

Does this make sense to you? Thanks!

Comment 15

[Germán Toro del Valle (:gtorodelvalle)]

Absolutely ;) Thanks!

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 22250.html

Comment 17

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: [video] Call duration in conference participant list overlay

Hi ladies, would you be so kind to take a look at the video I just recorded including this feature, please? Thanks!

I included Carol for the visual part and Carrie for the interaction one ;)

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

On the other hand, Carrie, I have additional questions about the time which we should show regarding the call duration of conference calls. Let see it with an example:
1. The DuT establishes a call with number_1. This starts a timer which is associated to the call to number_1. Let's call it timer_1.
2. The DuT adds or receives a second call to or from number_2. This starts a second timer associated to this second call. Let's call it timer_2.
3. The DuT creates a conference including the 2 ongoing calls. This starts a third timer associated to the conference call. Let's call it timer_c
4. The user expands the conference call participant list overlay.

My questions are:
i. Which time should we show if number_1 leaves the conference? Should we show timer_1 (this is the time since the call to number_1 was established)? Should we show timer_c (this is the time since the conference call was established)? Right now I am showing timer_1 :)

Take into consideration that once number_1 leaves the conference. The 1 to 1 call to number_2 keeps going on and timer_2 (this is the time since the original call to number_2 was established) is shown in the call screen.

My suggestion would be to show the time since the conference call was established when a participant leaves the call (this is timer_c). What do you think? :)

Comment 19

[Carrie Wang [New account: [e-mail]]]

Hi Germán, 

I'd suggest showing timer_c. Because users will not calculate how long each participant has been connected. They will like to know at which point the participant leaves the conference call. In addition, it will be very confusing to switch the time display between the participants list and the typical call screen. So yes, I would go with your suggestion. Thanks!

Comment 20

[Carol Huang [:Carol]]

Comment on attachment 8463973 [details]
[video] Call duration in conference participant list overlay

Hi Germán, 
The participant list looks fine. Just one little thing, could you remove the last divider between the list & the bottom blocks? I'll attach a screenshot.
Thank you!

Comment 21

[Carol Huang [:Carol]]

Attachment: Screenshot_participant list.jpg

please remove last the divider in the list. Thanks!! :)

Comment 22

[Loli (:lolimartinezcr)]

Tests cases created

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 8463897 [details]
22250.html

This is not easy to review because I'm a bit lost in the required changes. Could you point me at a list of changes this patch is trying to do?

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Anthony Ricaud (:rik) from comment #23)
> Comment on attachment 8463897 [details]
> 22250.html
> 
> This is not easy to review because I'm a bit lost in the required changes.
> Could you point me at a list of changes this patch is trying to do?

Ups, I missed this comment :O Sorry!

Yeah, basically the idea is to show the conference call duration when the participant list overlay is shown and any of the participants leaves the conference. The call duration should be shown during 3 seconds and the participant entry should be removed from the list afterwards. In case the conference is amongst 3 parties and 1 of the other ones leaves the conference, the call duration should be shown and after 3 seconds, the participant list overlay should be removed showing the user the corresponding 1 to 1 call left :-)

You can find this behaviour in this video: https://www.youtube.com/watch?v=iF1K2DovRV8 Please, notice that in the video the call duration shown is not the conference call one, this is fixed in the final patch proposed, but bug 1047255 impedes recording a proper new video :)

If you need further information, please do not hesitate to need-info me ;) I'll need-info you so you get notify about my reply :)

Comment 25

[Anthony Ricaud (:rik)]

Comment on attachment 8463897 [details]
22250.html

Thanks for the explanations. I left some comments on Github last week that I still want to see answered and some new ones this week. In particular, why are we moving from 2s to 3s?

I believe the current approach is too complex and we're missing new tests.

Comment 26

[Germán Toro del Valle (:gtorodelvalle)]

Hi Anthony, I left new comments in the pull request. Anyhow, in my opinion, this bug should be revisited once we solve another more important one we currently have regarding conference calls. Please, establish a conference call amongst 3 parties and just hang up one of the remote ones with the participant list hidden. You'll see weird things going on on the screen: 3 call rows (1 of which is the conference call), 2 of which appear with a red background, then the conference row and the hung up call go away, then the pending 1 to 1 call goes to normal :O).

The reason of this misbehaviour you may not be familiar with is the ordering of the event notifications which has somehow changed (very) recently. In fact, they broke also my patch for this bug for which I luckily recorded a video when it was still working :p . Now, when a conference call is established and one participant leaves the conference, the group call changes (TelephonyCall -> groupchange and ConferenceGroup -> callschanged) are notified before the call disconnect event (TelephonyCall -> statuschange) and that causes all that misbehaviour. We can discuss about it on IRC on next Wednesday when I am back from my PTO ;)

Comment 27

[Germán Toro del Valle (:gtorodelvalle)]

Regarding comment 26 ;)

Comment 28

[Anthony Ricaud (:rik)]

Hsin-Yi: Have you changed the order of events being dispatched as suggested by Germán in comment 26?

Comment 29

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

Hi Anthony,

Not sure what's the order that was working for German before. However, according to comment 26, the event order German has now, i.e. when a conference call is established and one participant leaves the conference, the group call changes (TelephonyCall -> groupchange and ConferenceGroup -> callschanged) are notified before the call disconnect event (TelephonyCall -> statuschange), is exactly the API design since day 1. Please refer to bug 772765 comment 87, steps d.3 ~ d.6.

Please let me know if you need more help!

Comment 30

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Pull request 24825

Since currently it seems to be some issue with the localisation and the timers in the Call Screen app, I suggest you to do the following to be able to test the new functionality:
  1. Substitute |_('callEnded')| in https://github.com/mozilla-b2g/gaia/pull/24825/files#diff-0cc741478a6ca5d2f75dd44f7f7ab18bR343 by |'Call Ended'|;
  2. Substitute |return groupDurationChildNode.textContent;| in https://github.com/mozilla-b2g/gaia/pull/24825/files#diff-8c52bec747d7922bc983df471c349280R122 by |return '12:34';|.

I included some comments to try to explain what's going on but we can decide to delete them if you consider it appropriate, Doug ;)

Comment 31

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

Yes, one of the items in my proposal is splitting the conference call participants UI and logic out from CallScreen. I think that this would be a good time to do so. From the logic side, this looks like a fairly clean break, as most of this logic operates on `this.groupCalls` and `this.groupCallsList` and has little else in the way of dependencies. From the UI side, this is an opportunity to lazy-load more HTML.

Other than that, I don't think it makes sense to switch reviewers mid-flight like this. So I'm redirecting the actual review back to Anthony.

Comment 32

[Germán Toro del Valle (:gtorodelvalle)]

Hi Doug, yeah I probably should have asked you for feedback only ;)

Regarding your proposal and trying to be aligned with it, would you be so kind to provide additional information regarding this splitting, please? Do you mean moving the conference call participant UI and logic out from CallScreen to ConferenceGroupHandler? To a new ConferenceGroupScreen/ConferenceParticipantScreen/ConferenceParticipantOverlay object? I guess the second option but just to confirm it ;)

Thanks!

Comment 33

[Doug Sherk (:drs) (inactive)]

(In reply to Germán Toro del Valle from comment #32)
> Hi Doug, yeah I probably should have asked you for feedback only ;)
> 
> Regarding your proposal and trying to be aligned with it, would you be so
> kind to provide additional information regarding this splitting, please? Do
> you mean moving the conference call participant UI and logic out from
> CallScreen to ConferenceGroupHandler? To a new
> ConferenceGroupScreen/ConferenceParticipantScreen/
> ConferenceParticipantOverlay object? I guess the second option but just to
> confirm it ;)
> 
> Thanks!

In general, in my proposal, I was aiming for having UI and controller classes, though this wouldn't be true MVC. We already have ConferenceGroupHandler, which is basically the controller/logic class for conference groups. But we still have a bunch of UI code on CallScreen.

I don't want to use the "Controller" and "View" names because this might imply a tighter interpretation of MVC than we're going for. So I think "Handler", which is already in widespread use, and "UI", would be good suffixes for these classes. In this case, I'd suggest creating a new ConferenceGroupUI class (the UI code for ConferenceGroupHandler), which would bundle the UI logic that we'll be taking out of CallScreen.

We don't have to get the names right immediately, and as always this is completely open to discussion. It's expensive to refactor, but not expensive to rename things. So let's just make sure that we get the former right.

Comment 34

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

OK, I have a first version of the (handler-UI) refactoring. Since I am currently observing issues with the localisation and the timers, I suggest you to tweak the following lines to test the patch:
  1. https://github.com/mozilla-b2g/gaia/pull/24825/files#diff-8c52bec747d7922bc983df471c349280R123 -> return '12:34'; // return groupDurationChildNode.textContent;
  2. https://github.com/mozilla-b2g/gaia/pull/24825/files#diff-0cc741478a6ca5d2f75dd44f7f7ab18bR346 -> 'Call Ended'; // _('callEnded');

As you'll see I based this first proposal on the already existent code and the way we currently deal with call nodes and events in the app. On the other hand, I have only moved to the new conference call UI object the code related to dealing with the participant list.

Thanks!

Comment 35

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

This seems fairly reasonable structurally, but I think the following would be better:
1) The conference group HTML in index.html should be lazy-loaded from within ConferenceGroupUI.
2) Only ConferenceGroupHandler and maybe a little bit of CallScreen should be calling directly to ConferenceGroupUI. Any other actors should call into it through ConferenceGroupHandler.

Comment 36

[Anthony Ricaud (:rik)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

I'll leave the review to Doug since he already has done a lot of work on this.

Comment 37

[Germán Toro del Valle (:gtorodelvalle)]

Hi Doug, just mentioning that I updated the PR including your latest comments for you to review ;) Thanks!

Comment 38

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

I left some comments on the PR, but this needs some comments from a high level as well. I stopped once I got to the CSS, and didn't review the tests either as they will likely need significant renaming at the very least.

We're muddling our terminology here a bit and this is making it more difficult to see a clear flow of logic. Let's write this down and iron it out.

In particular, I don't like how the handler and UI functions are named identically. The naming of the functions on `ConferenceGroupHandler` should imply action, whereas the ones on `ConferenceGroupUI` should imply presentation only.

Both classes should get descriptions at the top of their respective files describing what they do and what functionality they expose. Further down, we should break up the private and public members into clearly labeled sections.

ConferenceGroupHandler
---
* A call being handled by this class should be referred to as a "participant." This is because it implies action and more broadly encompasses the functionality of being part of the call.
* Exposes the following:
  . addParticipant()
  . removeParticipant()
  . signalConferenceEnded() (this should have a comment explaining why we need this instead of just clearing everything immediately)

ConferenceGroupUI
---
* This should refer to participants as "call nodes."
* The menu that pops up when you hit on the group details should be called the "group details overlay."
* Any time we're referring to the information surrounding a group, like we see in the overlay, we'll call it "group details."
* Exposes the following:
  . addCallNode()
  . removeCallNode()
  . showGroupDetailsOverlay()
  . hideGroupDetailsOverlay()
  . markConferenceAsEnded() (this should have a comment explaining why we need this instead of just clearing everything immediately)
  . setGroupDetailsHeader()

Comment 39

[Germán Toro del Valle (:gtorodelvalle)]

Hi Hsin-Yi, I have a question for you related to comment comment 29. Imagine an ongoing conference call established by the DuT with 2 other parties. Imagine one of these 2 other parties hangs up the phone and consequently a 1 to 1 call keeps going between the DuT and the other party. At this point the groupchanged event is notified in the DuT to both call instances: the ongoing one and the one supposedly disconnected. The point is that when the groupchanged event is notified I am getting "connected" as the state of these 2 calls so I have no way of knowing which one is the ongoing/unmerged one and which one is the disconnected (or disconnecting) one. Am I missing anything? Any clue is more than appreciated ;)

Comment 40

[Germán Toro del Valle (:gtorodelvalle)]

Please, see comment 39 ;) Thanks!

Comment 41

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #39)
> Hi Hsin-Yi, I have a question for you related to comment comment 29. Imagine
> an ongoing conference call established by the DuT with 2 other parties.
> Imagine one of these 2 other parties hangs up the phone and consequently a 1
> to 1 call keeps going between the DuT and the other party. At this point the
> groupchanged event is notified in the DuT to both call instances: the
> ongoing one and the one supposedly disconnected. The point is that when the
> groupchanged event is notified I am getting "connected" as the state of
> these 2 calls 

I would say there must be something wrong. There shouldn't be two single connected calls at the same time. I remember we encountered this situation before and it turned out that we need fix on modem... In the meanwhile, I also got the feeling that bug 1059649 might have a chance to resolve/cover this issue as we don't rely on a specific flag coming from modem. Could you please try Aknow's patches? If the patches can't solve this problem, please attach ril log and NI me again.

> so I have no way of knowing which one is the ongoing/unmerged
> one and which one is the disconnected (or disconnecting) one. Am I missing
> anything? Any clue is more than appreciated ;)

Comment 42

[Germán Toro del Valle (:gtorodelvalle)]

As an aside note just mentioning that the weird behaviour I mentioned in comment 26 seems to be related to the same issue consisting of having the 2 calls (participating in a conference call) as "connected" when 1 of them hung up and the groupchanged is notified. More concretely, the condition in this if https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/handled_call.js#L19 is always true and the hung up call is shown on the call screen with a red background :)
I forced the HandledCall._wasUnmerged() to return false for the hung up call and it works as expected ;)

@Hsin-Yi, I'm still compiling the build including Aknow's patches. I'll get back to you with the results as soon as I have it ;)

Comment 43

[Germán Toro del Valle (:gtorodelvalle)]

Hi Hsin-Yi, what I am observing after applying Aknow's patches is that the groupchange event is no longer notified to the hung up call in the scenario mentioned in comment 39. Will this be the final desired behaviour? :) Thanks!

Comment 44

[Germán Toro del Valle (:gtorodelvalle)]

Oh boy! I am observing the following after applying Aknow's patches. Let's go step by step:
1. DuT receives a phone call from Device1. Let's name this call as 'Call1'.
2. DuT makes a call to Device2. Let's name this call as 'Call2'.
3. DuT creates a conference call amongst the 3 parties.
At this precise point I am observing:
  3.1. Call1 is notified the groupchange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance.
  3.2. Call2 is notified the groupchange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance.
  3.3. Call1 is notified the statechange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance. Not really sure is this should be the desired behaviour but could be fine.
  3.4. Call2 is notified the statechange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance. Not really sure is this should be the desired behaviour but could be fine.
4. Device2 hangs up the call (Call2).
At this point I am observing:
  4.1. Call1 is notified the groupchange event. The call.state is 'connected' and the call.group is null. Sounds good to me since this is the unmerged call or the 1 to 1 call still going on once the conference call has ended.
  4.2. Call2 is notified the statechange event. The call.state is blank (not 'disconnected' or 'disconnecting' as I would expect) :O and the call.group is the TelephonyCallGroup instance (this call was participating in). I really doubt this is the desired behaviour :(

I'll check this in a build without applying Aknow's patches and I'll comment the results.

Comment 45

[Germán Toro del Valle (:gtorodelvalle)]

In case of not applying Aknow's patches (this is, using today's build: Gecko-2bfbd70.Gaia-6cbfaf9) what I am observing is:
1. DuT receives a phone call from Device1. Let's name this call as 'Call1'.
2. DuT makes a call to Device2. Let's name this call as 'Call2'.
3. DuT creates a conference call amongst the 3 parties.
At this precise point I am observing:
  3.1. Call1 is notified the groupchange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance. SAME AS BEFORE.
  3.2. Call2 is notified the groupchange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance. SAME AS BEFORE.
  3.3. Call1 is notified the statechange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance. Not really sure is this should be the desired behaviour but could be fine. SAME AS BEFORE.
  3.4. Call2 is notified the statechange event. The call.state is 'connected' and the call.group is the TelephonyCallGroup instance. Not really sure is this should be the desired behaviour but could be fine. SAME AS BEFORE.
4. Device2 hangs up the call (Call2).
At this point I am observing:
  4.1. Call1 is notified the groupchange event. The call.state is 'connected' and the call.group is null. Sounds good to me since this is the unmerged call or the 1 to 1 call still going on once the conference call has ended. SAME AS BEFORE.
  4.2. Call2 is notified the groupchange event. The call.state is 'connected' and the call.group is null. IN AKNOW'S PATCH THIS EVENT IS NOT NOTIFIED.
  4.3. Call2 is notified the statechange event. The call.state is 'disconnected' and the call.group is null. IN AKNOW'S CASE, THE GROUP WAS STORING THE TelephonyCallGroup THIS CALL WAS PARTICIPATING IN.

It seems to me that this would be a great time to have a meeting between back-end and front-end telephony teams to look over the event notification flow and data as well as to try to update the wiki documentation (https://developer.mozilla.org/en-US/docs/Web/API/Telephony) which is pretty outdated :( Probably a great change to document the errors which may be thrown in all the possible scenarios too (see bug 846403) :) What do you think?

Comment 46

[Doug Sherk (:drs) (inactive)]

Attachment: Logcat of hanging up one call in a conference call

Germán, thanks for your very thorough explanation. It was very helpful for my debugging. I took a look and it seems that there's a few 100ms delay before calls are marked as 'disconnected'. I was also able to repro the issue that you described where there are two calls with the 'connected' state, but not in the same `conferenceGroup`.

Here's what I found:

Starting state: DuT is in a conference call with 2 other devices, #1 and #2. Device #1 just hung up.

[-0s or immediately before]:
* mozTelephony.conferenceGroup.calls = [{(#1) state: 'connected'}, {(#2) state: 'connected'}]

[+0s or immediately after]:
* mozTelephony.conferenceGroup.calls = [{(#2) state: 'connected'}]
* mozTelephony.calls = [{(#1) state: 'connected'}]

[~0.1s]:
* mozTelephony.calls = [{(#1) state: 'connected'}, {(#2) state: 'connected'}]

[~0.5s]:
* mozTelephony.calls = [{(#2) state: 'connected'}]
* dangling pointer (if you hold one): [{(#1) state: 'disconnected'}]

Hsin-Yi, this looks like bugged behavior to me. I would think ideally that at +0s, call #1 should have state='disconnected', as Germán has said. I've attached a logcat with RIL debugging enabled. I added '$$$$$$$$$$$$$$$$$$$$$$$$' around the spot that I hung up the second device.

Germán, what would you like out of a meeting? It's not clear to me what we would accomplish that we can't by discussing in this bug. It seems to me like this is obviously buggy behavior with a fairly clear desired outcome.

I see a way to work around this, by abusing the differences between the -0s and +0s states. But I think it would be much better to fix this on the platform side.

Comment 47

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #43)
> Hi Hsin-Yi, what I am observing after applying Aknow's patches is that the
> groupchange event is no longer notified to the hung up call in the scenario
> mentioned in comment 39. Will this be the final desired behaviour? :) Thanks!

Hi Germán,
No, we didn't tend to change the behaviour in Bug 1059649.

Hi Aknow,
I reviewed the patch of Bug 1059649 again, and I still didn't see how we changed the event order. Could you help confirm and reply Germán and Doug's comment 44, 45 and 46? Thank you.

Comment 48

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #47)
> (In reply to Germán Toro del Valle from comment #43)
> > Hi Hsin-Yi, what I am observing after applying Aknow's patches is that the
> > groupchange event is no longer notified to the hung up call in the scenario
> > mentioned in comment 39. Will this be the final desired behaviour? :) Thanks!
> 
> Hi Germán,
> No, we didn't tend to change the behaviour in Bug 1059649.
> 
> Hi Aknow,
> I reviewed the patch of Bug 1059649 again, and I still didn't see how we
> changed the event order. Could you help confirm and reply Germán and Doug's
> comment 44, 45 and 46? Thank you.

Doug,

Is the comment 46 based on the current design (w/o my patch)?
Could you check whether the following event sequence is reasonable or not?

Germán,

I log the sequence of the events with my patches of Bug 1059649.
However, the result is different than yours.

Steps:
1. conference call1, call2
2. remote hangup call2

Observe:

[ call2.ongroupchange ]
telephony: 0
conference: 1
 - call1, connected, group:+

[ conference.oncallschanged(call2) ]
telephony: 0
conference: 1
 - call1, connected, group:+

[ call2.onstatechange ]
telephony: 0
conference: 1
 - call1, connected, group:+

[ call2.disconnected ]
telephony: 0
conference: 1
 - call1, connected, group:+

[ call1.ongroupchange ]
telephony: 0
conference: 0

[ conference.oncallschanged(call1) ]
telephony: 0
conference: 0

[ telephony.oncallschanged(call1) ]
telephony: 1
 - call1, connected, group:-
conference: 0

Comment 49

[Szu-Yu Chen [:aknow] (inactive)]

NI for comment 48

Comment 50

[Germán Toro del Valle (:gtorodelvalle)]

Hi Doug, yeah we could use this bug to try to clarify and to document the desired (and observed) telephony event flows, call status and provided data throughout the distinct calling scenarios. My idea of a meeting was to try to join everyone around a (virtual) table just in case someone could miss this bug and could be left aside, and also to get it done ASAP. But as I said we can try to use this bug for it ;)

In my opinion, it would be highly valuable to have some kind of sequence diagram specifying this event and data flow. I really do not know if this knowledge is spread out amongst the members of the backend Telephony team or if someone could provided a quick sketch about it. I would also be highly recommendable to revisit the documentation at https://developer.mozilla.org/en-US/docs/Web/API/Telephony :)

Comment 51

[Germán Toro del Valle (:gtorodelvalle)]

Hi Szu-Yu, I am sorry but I am not sure I am understanding (able to interpret) the results you are observing :( Anyhow, maybe we are not using the same base build or any other variable (even telephony network) which escapes to our control :(

Although I guess Doug we'll just mention it in a sec., we discussed about it via IRC and we decided to start clarifying the event notification flow for the scenario which affects this bug (this is, the hanging up of calls participating in a conference call) and from there try to clearly state the same for the other calling scenarios. I'll lead this effort although I'll need to contact you for some confirmations :)

Comment 52

[Szu-Yu Chen [:aknow] (inactive)]

Let me explain my observation. It's based on the testing on emulator.

Steps:
1. Have two calls (call1, call2) and conference them
2. Remote party hangup call2

Right after 2, I observed following event sequence: (I'll use 'conference' to name 'TelephonyCallGroup')

(1) call2 is notified the groupchange event (Event param => call2, state: disconnected)

Then I list the current calls.
Only one call remained in conference.calls, which is call1 with 'connected' state

telephony.calls: 0 (empty)
conference.calls: 1 (has 1 call)
 - call1, connected, in group: Yes


(2) conference is notified the callschanged event (Event param => call2, state: disconnected)

telephony.calls: 0
conference.calls: 1
 - call1, connected, in group: Yes


(3) call2 is notified the statechange event (Event param => call2, state: disconnected)

telephony.calls: 0
conference.calls: 1
 - call1, connected, in group: Yes


(4) call2 is notified the disconnected event  (Event param => call2, state: disconnected)

telephony.calls: 0
conference.calls: 1
 - call1, connected, in group: Yes


(5) call1 is notified the groupchange event (Event param => call1, state: connected)

telephony.calls: 0
conference.calls: 0


(6) conference is notified the callschanged event (Event param => call1, state: connected)

telephony.calls: 0
conference.calls: 0


(7) telephony is notified the callschanged event (Event param => call1, state: connected)

telephony: 1
 - call1, connected, in group: No
conference: 0

Comment 53

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Szu-Yu Chen [:aknow] from comment #52)
> Let me explain my observation. It's based on the testing on emulator.
> 
> Steps:
> 1. Have two calls (call1, call2) and conference them
> 2. Remote party hangup call2
> 
> Right after 2, I observed following event sequence: (I'll use 'conference'
> to name 'TelephonyCallGroup')
> 
> (1) call2 is notified the groupchange event (Event param => call2, state:
> disconnected)
> 
> Then I list the current calls.
> Only one call remained in conference.calls, which is call1 with 'connected'
> state
> 
> telephony.calls: 0 (empty)
> conference.calls: 1 (has 1 call)
>  - call1, connected, in group: Yes
> 
> 
> (2) conference is notified the callschanged event (Event param => call2,
> state: disconnected)
> 
> telephony.calls: 0
> conference.calls: 1
>  - call1, connected, in group: Yes
> 
> 
> (3) call2 is notified the statechange event (Event param => call2, state:
> disconnected)
> 
> telephony.calls: 0
> conference.calls: 1
>  - call1, connected, in group: Yes
> 
> 
> (4) call2 is notified the disconnected event  (Event param => call2, state:
> disconnected)
> 
> telephony.calls: 0
> conference.calls: 1
>  - call1, connected, in group: Yes
> 
> 
> (5) call1 is notified the groupchange event (Event param => call1, state:
> connected)
> 
> telephony.calls: 0
> conference.calls: 0
> 
> 
> (6) conference is notified the callschanged event (Event param => call1,
> state: connected)
> 
> telephony.calls: 0
> conference.calls: 0
> 
> 
> (7) telephony is notified the callschanged event (Event param => call1,
> state: connected)
> 
> telephony: 1
>  - call1, connected, in group: No
> conference: 0

Wow! I guess your observed behaviour is after applying your patches for bug 1059649, right? Anyhow, I am not sure what you are observing is what I would expect as the correct event and data flow :) Being cases where telephony.calls and conference.calls have no entries just an example :s But maybe it is just me ;)

Comment 54

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - diagram.svg

On the other hand, I logged the observed behaviour (event and data flow) using Gecko-63faaa5.Gaia-9e8e3bb (master) in a Flame with KK without applying Szu-Yu's patches for bug 1059649 and created the attached diagram.

In fact, I found a tool which may be of great help here: http://bramp.github.io/js-sequence-diagrams/

I'll attach the source code of my diagram in a sec., so you can paste it in the left hand side area of the tool to get it depicted and also in case you want to edit it.

I'll also attach the patch I used for my testing and the logs I got.

Anyhow, what I think we really need here is a spec sequence diagram of the expected or desired event and data flow we all agree on. It would be also great if everyone observed the same behaviour :p

Comment 55

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - diagram - source code for http://bramp.github.io/js-sequence-diagrams/

Comment 56

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - logs

Comment 57

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - logs.patch

Comment 58

[Germán Toro del Valle (:gtorodelvalle)]

BTW, my "EXPECTED" comments in attachment 8508559 [details] are just "as far as my understanding" or "as I would expect". Please, do not take it as anything else ;) Thanks!

Comment 59

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #53)
> (In reply to Szu-Yu Chen [:aknow] from comment #52)
> > Let me explain my observation. It's based on the testing on emulator.
> > 
> > Steps:
> > 1. Have two calls (call1, call2) and conference them
> > 2. Remote party hangup call2
> > 
> > Right after 2, I observed following event sequence: (I'll use 'conference'
> > to name 'TelephonyCallGroup')
> > 
> > (1) call2 is notified the groupchange event (Event param => call2, state:
> > disconnected)
> > 
> > Then I list the current calls.
> > Only one call remained in conference.calls, which is call1 with 'connected'
> > state
> > 
> > telephony.calls: 0 (empty)
> > conference.calls: 1 (has 1 call)
> >  - call1, connected, in group: Yes
> > 
> > 
> > (2) conference is notified the callschanged event (Event param => call2,
> > state: disconnected)
> > 
> > telephony.calls: 0
> > conference.calls: 1
> >  - call1, connected, in group: Yes
> > 
> > 
> > (3) call2 is notified the statechange event (Event param => call2, state:
> > disconnected)
> > 
> > telephony.calls: 0
> > conference.calls: 1
> >  - call1, connected, in group: Yes
> > 
> > 
> > (4) call2 is notified the disconnected event  (Event param => call2, state:
> > disconnected)
> > 
> > telephony.calls: 0
> > conference.calls: 1
> >  - call1, connected, in group: Yes
> > 
> > 
> > (5) call1 is notified the groupchange event (Event param => call1, state:
> > connected)
> > 
> > telephony.calls: 0
> > conference.calls: 0
> > 
> > 
> > (6) conference is notified the callschanged event (Event param => call1,
> > state: connected)
> > 
> > telephony.calls: 0
> > conference.calls: 0
> > 
> > 
> > (7) telephony is notified the callschanged event (Event param => call1,
> > state: connected)
> > 
> > telephony: 1
> >  - call1, connected, in group: No
> > conference: 0
> 
> Wow! I guess your observed behaviour is after applying your patches for bug
> 1059649, right? Anyhow, I am not sure what you are observing is what I would
> expect as the correct event and data flow :) Being cases where
> telephony.calls and conference.calls have no entries just an example :s But
> maybe it is just me ;)

Aknow's observation is actually the design, please see bug 772765 comment 87. The sequence is also the same as what I observed on a real device. 

For me, the sequence looks fine. But the tricky point is about the parameters -- step (5) and (6), i.e. |telephony.calls: 0| and |conference.calls: 0|. I think it really confuses API user that there's definitely one call but there's no entry for the call.  We would need to correct it IMO.

Comment 60

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - diagram - Gecko-0dbdde5.Gaia-3c90141.svg

Just in case it helps, I just observed a new behaviour using today's build (Gecko-0dbdde5.Gaia-3c90141) in my Flame with KK.

I'll upload the logs and the sequence diagram source code in a sec.

Comment 61

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - js-sequence - Gecko-0dbdde5.Gaia-3c90141.txt

Source code of the sequence diagram susceptible of being pasted in http://bramp.github.io/js-sequence-diagrams/ ;)

Comment 62

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference - logs - Gecko-0dbdde5.Gaia-3c90141.txt

Comment 63

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #59)
> (In reply to Germán Toro del Valle from comment #53)
> > (In reply to Szu-Yu Chen [:aknow] from comment #52)
> > > Let me explain my observation. It's based on the testing on emulator.
> > > 
> > > Steps:
> > > 1. Have two calls (call1, call2) and conference them
> > > 2. Remote party hangup call2
> > > 
> > > Right after 2, I observed following event sequence: (I'll use 'conference'
> > > to name 'TelephonyCallGroup')
> > > 
> > > (1) call2 is notified the groupchange event (Event param => call2, state:
> > > disconnected)
> > > 
> > > Then I list the current calls.
> > > Only one call remained in conference.calls, which is call1 with 'connected'
> > > state
> > > 
> > > telephony.calls: 0 (empty)
> > > conference.calls: 1 (has 1 call)
> > >  - call1, connected, in group: Yes
> > > 
> > > 
> > > (2) conference is notified the callschanged event (Event param => call2,
> > > state: disconnected)
> > > 
> > > telephony.calls: 0
> > > conference.calls: 1
> > >  - call1, connected, in group: Yes
> > > 
> > > 
> > > (3) call2 is notified the statechange event (Event param => call2, state:
> > > disconnected)
> > > 
> > > telephony.calls: 0
> > > conference.calls: 1
> > >  - call1, connected, in group: Yes
> > > 
> > > 
> > > (4) call2 is notified the disconnected event  (Event param => call2, state:
> > > disconnected)
> > > 
> > > telephony.calls: 0
> > > conference.calls: 1
> > >  - call1, connected, in group: Yes
> > > 
> > > 
> > > (5) call1 is notified the groupchange event (Event param => call1, state:
> > > connected)
> > > 
> > > telephony.calls: 0
> > > conference.calls: 0
> > > 
> > > 
> > > (6) conference is notified the callschanged event (Event param => call1,
> > > state: connected)
> > > 
> > > telephony.calls: 0
> > > conference.calls: 0
> > > 
> > > 
> > > (7) telephony is notified the callschanged event (Event param => call1,
> > > state: connected)
> > > 
> > > telephony: 1
> > >  - call1, connected, in group: No
> > > conference: 0
> > 
> > Wow! I guess your observed behaviour is after applying your patches for bug
> > 1059649, right? Anyhow, I am not sure what you are observing is what I would
> > expect as the correct event and data flow :) Being cases where
> > telephony.calls and conference.calls have no entries just an example :s But
> > maybe it is just me ;)
> 
> Aknow's observation is actually the design, please see bug 772765 comment
> 87. The sequence is also the same as what I observed on a real device. 
> 
> For me, the sequence looks fine. But the tricky point is about the
> parameters -- step (5) and (6), i.e. |telephony.calls: 0| and
> |conference.calls: 0|. I think it really confuses API user that there's
> definitely one call but there's no entry for the call.  We would need to
> correct it IMO.

Thanks Hsin-YI ;) I'll generate the sequence diagram for this behaviour and I'll attach it to the bug.

Since bug 1059649 already landed, I guess your and Aknow's observed behaviour is after applying the patches for bug 1059649 :) Those patches may not be in the build I am using (it was generated last night).

On the other hand, is there a bug to solve the 'telephony.calls: 0 conference.calls: 0' issue? I could create it if not ;) Thanks!

Comment 64

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #63)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #59)
> > (In reply to Germán Toro del Valle from comment #53)
> > > (In reply to Szu-Yu Chen [:aknow] from comment #52)
> > > > Let me explain my observation. It's based on the testing on emulator.
> > > > 
> > > > Steps:
> > > > 1. Have two calls (call1, call2) and conference them
> > > > 2. Remote party hangup call2
> > > > 
> > > > Right after 2, I observed following event sequence: (I'll use 'conference'
> > > > to name 'TelephonyCallGroup')
> > > > 
> > > > (1) call2 is notified the groupchange event (Event param => call2, state:
> > > > disconnected)
> > > > 
> > > > Then I list the current calls.
> > > > Only one call remained in conference.calls, which is call1 with 'connected'
> > > > state
> > > > 
> > > > telephony.calls: 0 (empty)
> > > > conference.calls: 1 (has 1 call)
> > > >  - call1, connected, in group: Yes
> > > > 
> > > > 
> > > > (2) conference is notified the callschanged event (Event param => call2,
> > > > state: disconnected)
> > > > 
> > > > telephony.calls: 0
> > > > conference.calls: 1
> > > >  - call1, connected, in group: Yes
> > > > 
> > > > 
> > > > (3) call2 is notified the statechange event (Event param => call2, state:
> > > > disconnected)
> > > > 
> > > > telephony.calls: 0
> > > > conference.calls: 1
> > > >  - call1, connected, in group: Yes
> > > > 
> > > > 
> > > > (4) call2 is notified the disconnected event  (Event param => call2, state:
> > > > disconnected)
> > > > 
> > > > telephony.calls: 0
> > > > conference.calls: 1
> > > >  - call1, connected, in group: Yes
> > > > 
> > > > 
> > > > (5) call1 is notified the groupchange event (Event param => call1, state:
> > > > connected)
> > > > 
> > > > telephony.calls: 0
> > > > conference.calls: 0
> > > > 
> > > > 
> > > > (6) conference is notified the callschanged event (Event param => call1,
> > > > state: connected)
> > > > 
> > > > telephony.calls: 0
> > > > conference.calls: 0
> > > > 
> > > > 
> > > > (7) telephony is notified the callschanged event (Event param => call1,
> > > > state: connected)
> > > > 
> > > > telephony: 1
> > > >  - call1, connected, in group: No
> > > > conference: 0
> > > 
> > > Wow! I guess your observed behaviour is after applying your patches for bug
> > > 1059649, right? Anyhow, I am not sure what you are observing is what I would
> > > expect as the correct event and data flow :) Being cases where
> > > telephony.calls and conference.calls have no entries just an example :s But
> > > maybe it is just me ;)
> > 
> > Aknow's observation is actually the design, please see bug 772765 comment
> > 87. The sequence is also the same as what I observed on a real device. 
> > 
> > For me, the sequence looks fine. But the tricky point is about the
> > parameters -- step (5) and (6), i.e. |telephony.calls: 0| and
> > |conference.calls: 0|. I think it really confuses API user that there's
> > definitely one call but there's no entry for the call.  We would need to
> > correct it IMO.
> 
> Thanks Hsin-YI ;) I'll generate the sequence diagram for this behaviour and
> I'll attach it to the bug.
> 

Thanks for all the sequence diagram!!! Does the designated sequence work for Dialer? If not, could you provide a suggestion? We could see if and how we could achieve that on gecko.

> Since bug 1059649 already landed, I guess your and Aknow's observed
> behaviour is after applying the patches for bug 1059649 :) 

Per comment 48, Aknow's observation is based on his latest patch, you are right. My observation is totally the same no matter with or without applying the patches for bug 1059649.

> Those patches may
> not be in the build I am using (it was generated last night).
> 

I think so.

> On the other hand, is there a bug to solve the 'telephony.calls: 0
> conference.calls: 0' issue? I could create it if not ;) Thanks!

Not yet ~ Please go ahead!

Comment 65

[Germán Toro del Valle (:gtorodelvalle)]

New bug to deal with the 'telephony.calls: 0 conference.calls: 0' cases: bug 1087305 ;)

Since in my opinion the flow mentioned by Aknow in comment 52 will be enough to solve this very bug as well as bug 1083402, I won't create dependencies between the new created bug and these other 2 bugs.

Comment 66

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - diagram - spec.svg

OK, this is the sequence diagram for the flow mentioned by Aknow in https://bugzilla.mozilla.org/show_bug.cgi?id=977056#c52

Anyhow, I have a couple of comments here:
 1. I am not really sure there's a distinction between the steps (3) and (4), aren't they the same? I merged them into one in the sequence diagram.
 2. I included '???' for the information which was not specified or I was not sure about. It would be great if you could have a look at the diagram, Hsin-Yi, as well as to the source code I'll attach in a sec., to include the right information instead of this '???' :) If you prefer, just comment the data which should be shown there and I'll update the diagram.

Thanks!

Comment 67

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - js-sequence - spec

Comment 68

[Germán Toro del Valle (:gtorodelvalle)]

I decided to create the dependency since bug 1059649 is needed to properly solve this bug ;)

Comment 69

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #66)
> Created attachment 8509450 [details]
> Participant leaving conference (3 parties) - diagram - spec.svg
> 
> OK, this is the sequence diagram for the flow mentioned by Aknow in
> https://bugzilla.mozilla.org/show_bug.cgi?id=977056#c52
> 
> Anyhow, I have a couple of comments here:
>  1. I am not really sure there's a distinction between the steps (3) and
> (4), aren't they the same? I merged them into one in the sequence diagram.

No, no difference as you understand. Doug and I have been thinking about phasing duplicated events out.
I'd encourage you to use 'onstatechange'.

>  2. I included '???' for the information which was not specified or I was
> not sure about. It would be great if you could have a look at the diagram,
> Hsin-Yi, as well as to the source code I'll attach in a sec., to include the
> right information instead of this '???' :) If you prefer, just comment the
> data which should be shown there and I'll update the diagram.
> 
> Thanks!

Comments inline:

Note over RIL: Device_2 hangs up
RIL->TC2: ongroupchange(ev)
Note right of TC2: ev.call: TC2 \nev.call.state: 'disconnected' \nev.call.group: ??? \nmozTelephony.calls: [] \nmozTelephony.conferenceGroup.calls: [TC1] \nTC1.state: 'connected' \nTC1.group: TCG'
[Hsinyi] ev.call.group: null

RIL-TCG: oncallschanged(ev)
Note right of TCG: ev.call: TC2 \nev.call.state: 'disconnected' \nev.call.group: ??? \nmozTelephony.calls: [] \nmozTelephony.conferenceGroup.calls: [TC1] \nTC1.state: 'connected' \nTC1.group: TCG
[Hsinyi] ev.call.group: null


RIL->TC2: onstatechange(ev)
Note right of TC2: ev.call: TC2 \nev.call.state: 'disconnected' \nev.call.group: ??? \nmozTelephony.calls: [] \nmozTelephony.conferenceGroup.calls: [TC1] \nTC1.state: 'connected' \nTC1.group: TCG
[Hsinyi] ev.call.group: null

RIL->TC1: ongroupchange(ev)
Note right of TC1: ev.call: TC1 \nev.call.state: 'connected' \nev.call.group: ??? \nmozTelephony.calls: ??? \nmozTelephony.conferenceGroup.calls: ???
[Hsinyi] ev.call.group: null
         mozTelephony.calls: [] // we expect [TC1] in bug 1087305, right?
         mozTelephony.conferenceGroup.calls: []

RIL->TCG: oncallschanged(ev)
Note right of TCG: ev.call: TC1 \nev.call.state: 'connected' \nev.call.group: ??? \nmozTelephony.calls: ??? \nmozTelephony.conferenceGroup.calls: ???
[Hsinyi] ev.call.group: null
         mozTelephony.calls: [] // we expect [TC1] in bug 1087305, right?
         mozTelephony.conferenceGroup.calls: []

RIL->mozTelephony: oncallschanged(ev)
Note right of mozTelephony: TC1 \nev.call.state: 'connected' \nev.call.group: null \nmozTelephony.calls: [TC1] \nmozTelephony.conferenceGroup.calls: []
[Hsinyi] ev.call.group: null
         mozTelephony.calls: [TC1]
         mozTelephony.conferenceGroup.calls: []


(In reply to Germán Toro del Valle from comment #68)
> I decided to create the dependency since bug 1059649 is needed to properly
> solve this bug ;)

Awesome!

Comment 70

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - diagram - spec.svg

Current event notification and data sequence diagram according to comment 69.

Comment 71

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - js-sequence - spec.txt

Current event notification and data sequence diagram source code (to be used in http://bramp.github.io/js-sequence-diagrams/) according to comment 69.

Comment 72

[Germán Toro del Valle (:gtorodelvalle)]

I hope this does not add confusion but helps us come to a consensus and the best possible implementation :)

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1087305#c2 and https://bugzilla.mozilla.org/show_bug.cgi?id=1087305#c3 I would definitely suggest having an uniform and stable snapshot of the data during the notification of all the events. From a mozTelephony.calls and mozTelephony.conferenceGroup.calls perspective this would basically be having:
  - mozTelephony.calls: [TC1]
  - mozTelephony.conferenceGroup.calls: []
since once the participant left the conference, this is over.

I created a new sequence diagram with "// SUGGESTED: " entries I'll attach in a sec. It would be great to have your thoughts about it, Hsin-Yi :)

Comment 73

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - diagram - SUGGESTED UDPATES.svg

"Minor" suggested changes to have an uniform and stable data snapshot during the notification of all the events ;)

Comment 74

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - js-sequence - SUGGESTED UPDATES.txt

"Minor" suggested changes to have an uniform and stable data snapshot during the notification of all the events ;)

Comment 75

[Germán Toro del Valle (:gtorodelvalle)]

Need-infoing Hsin-Yi regarding comment 72, comment 73 and comment 74.

Comment 76

[Germán Toro del Valle (:gtorodelvalle)]

Oh boy! This is awkward :(

Hsin-Yi, using the same build (which Johan sent it over to me) we are currently observing distinct event notification and data flows :(

In fact, Johan is getting a behaviour pretty similar to you: https://pastebin.mozilla.org/6863258 (hanging the call to +33XXXXXXXXX from the corresponding device and letting the one to 888 keep going once the conference call is over) BTW, not considering the last event notification from line 47 on which we had not identified in our spec (attachment 8510217 [details]).

On the other hand (and as I said using the same build), I am getting: https://pastebin.mozilla.org/6863701 (hanging the call to 606XXXXXX from the corresponding device and letting the one to 123 keep going once the conference call is over).

Any clue, Hsin-Yi, why we may be getting this distinct results? :) Thanks!

Comment 77

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #76)
> Oh boy! This is awkward :(
> 
> Hsin-Yi, using the same build (which Johan sent it over to me) we are
> currently observing distinct event notification and data flows :(
> 
> In fact, Johan is getting a behaviour pretty similar to you:
> https://pastebin.mozilla.org/6863258 (hanging the call to +33XXXXXXXXX from
> the corresponding device and letting the one to 888 keep going once the
> conference call is over) BTW, not considering the last event notification
> from line 47 on which we had not identified in our spec (attachment 8510217 [details]
> [details]).
> 
> On the other hand (and as I said using the same build), I am getting:
> https://pastebin.mozilla.org/6863701 (hanging the call to 606XXXXXX from the
> corresponding device and letting the one to 123 keep going once the
> conference call is over).
> 
> Any clue, Hsin-Yi, why we may be getting this distinct results? :) Thanks!

Hi Germán,

I guess it's because of multi-threading. So events for Call1 (hang-up-call) are interrupted by those for Call2. Can you please check the sequence of the events for Call1 only? And can you please provide the log with your debug messages and ril debug messages? To enable ril messages, please refer to https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Changing_preferences. Please grab the logs by "adb logcat -b radio -b main -v time" thank you.

b.t.w. once you successfully enable ril debugger, you should be able to see a lot of messages of prefix "RIL worker"

Comment 78

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #77)
> (In reply to Germán Toro del Valle from comment #76)
> > Oh boy! This is awkward :(
> > 
> > Hsin-Yi, using the same build (which Johan sent it over to me) we are
> > currently observing distinct event notification and data flows :(
> > 
> > In fact, Johan is getting a behaviour pretty similar to you:
> > https://pastebin.mozilla.org/6863258 (hanging the call to +33XXXXXXXXX from
> > the corresponding device and letting the one to 888 keep going once the
> > conference call is over) BTW, not considering the last event notification
> > from line 47 on which we had not identified in our spec (attachment 8510217 [details]
> > [details]).
> > 
> > On the other hand (and as I said using the same build), I am getting:
> > https://pastebin.mozilla.org/6863701 (hanging the call to 606XXXXXX from the
> > corresponding device and letting the one to 123 keep going once the
> > conference call is over).
> > 
> > Any clue, Hsin-Yi, why we may be getting this distinct results? :) Thanks!
> 
> Hi Germán,
> 
> I guess it's because of multi-threading. So events for Call1 (hang-up-call)
> are interrupted by those for Call2. Can you please check the sequence of the
> events for Call1 only? And can you please provide the log with your debug
> messages and ril debug messages? To enable ril messages, please refer to
> https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Changing_preferences. Please
> grab the logs by "adb logcat -b radio -b main -v time" thank you.
> 
> b.t.w. once you successfully enable ril debugger, you should be able to see
> a lot of messages of prefix "RIL worker"

Hi Hsin-Yi, I am not sure I get the first part of your comment :p but these are the logs I get when only logging the events notified to Call 2 (this is the hung-up-call): https://pastebin.mozilla.org/6881631

In the case of the logging of Call 1 (the call which keeps ongoing) I get: https://pastebin.mozilla.org/6881632

But they are the same I get when all the logging is enabled (but filtered by call): https://pastebin.mozilla.org/6863258

Comment 79

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: radio logcat observed by gtorodelvalle

These are the logs I am getting. I left enabled my logs from Gaia not to distort the test ;)

Any clue why I am getting this event notification and data flow? :)

Comment 80

[Germán Toro del Valle (:gtorodelvalle)]

BTW, the call to 606XXXXXX is Call 2 (this is, the hung-up-call) and the call 63030XXXXXXXXX is Call 1 (this is, the call which keeps on going once Call 2 is hung up) ;)

Comment 81

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #79)
> Created attachment 8511020 [details]
> radio logcat observed by gtorodelvalle
> 
> These are the logs I am getting. I left enabled my logs from Gaia not to
> distort the test ;)
> 
> Any clue why I am getting this event notification and data flow? :)

I saw an error that is very likely the reason that you didn't receive call2.ondisconnected. I'll provide a WIP in Bug 1089534, please give it a try then. I want to thank you very much again for all the debugging :)

Comment 82

[Germán Toro del Valle (:gtorodelvalle)]

I added myself to the CC list of bug 1089534 ;) I'll try the patch as soon as it is available.

On the other hand, thanks to you for the great support and quick replies ;)

Comment 83

[Germán Toro del Valle (:gtorodelvalle)]

Niiice! Applying the patch provided by Hsin-Yi in bug 1089534, I get the same results as Johan (see comment 76), this is https://pastebin.mozilla.org/6943662, which are great news :)

Anyhow, I left a comment in bug 1089534 regarding an additional event notification we are getting which is not mentioned in our specs: attachment 8510217 [details], just to try to clarify that issue ;)

Comment 84

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Germán Toro del Valle from comment #83)
> Niiice! Applying the patch provided by Hsin-Yi in bug 1089534, I get the
> same results as Johan (see comment 76), this is
> https://pastebin.mozilla.org/6943662, which are great news :)
> 

Thanks for verifying \o/

> Anyhow, I left a comment in bug 1089534 regarding an additional event
> notification we are getting which is not mentioned in our specs: attachment
> 8510217 [details], just to try to clarify that issue ;)

The last additional event TelephonyCallGroup.onstatechange is expected because in this case, telephonyCallGroup state is changing from 'connected' to 'unknown' (no calls in the group). Everything looks perfect from the API point of view :)

Comment 85

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

Hi Doug, once the event notification and data flow is clearer :) (thanks to Hsin-Yi about that), I updated the patch. Please, take into consideration in case you want to try it that this version of the patch depends on the patch proposed by Hsin-Yi to bug 1089534 ;-)

Comments are more than welcome ;) Thanks!

Comment 86

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - diagram - spec.svg

I updated the specs including the last statechange event notification we talk about in comment 84.

According to my logs, it would be only pending the landing of bug 1089534 and the fixing of 1087305 to have this spec fully implemented ;)

Comment 87

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - js-sequence - spec.txt

The source code of the latest version of the sequence diagram susceptible of being pasted in http://bramp.github.io/js-sequence-diagrams/

Comment 88

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

Thanks for the sequence diagrams! There are some tests that we need to add, and some other fairly minor changes to the actual code. See the PR for these.

Please also expand my obsoleted comments as I replied to several of them.

Comment 89

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - js-sequence - logs.txt

Updated logs including information about the value of the mozTelephony.conferenceGroup.state property when the events are notified.

Comment 90

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - diagram - spec.svg

Updated specs including information about the value of the mozTelephony.conferenceGroup.state property when the events are notified. Probably something to be revisited once this bug is over. The current behaviour (after applying the patch for bug 1089534 is enough to solve this bug :) I'll file a new bug about it then ;)

Comment 91

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Participant leaving conference (3 parties) - js-sequence - spec.txt

Updated source code to be used in http://bramp.github.io/js-sequence-diagrams/ including information about the value of the mozTelephony.conferenceGroup.state property when the events are notified.

Comment 92

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

That was a nice and in-depth review ;) Thanks!

I think I went over all your comments but do not hesitate to raise any other issue ;)

Comment 93

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

I left some additional comments on the PR after some obsoleted comments on the last review cycle. Please expand them all and verify that they've all been addressed.

Comment 94

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

I think I covered all your comments :)

Comment 95

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

We're almost there. My last comments are pretty nit-picky. I think we'll be able to land this on the next iteration.

Comment 96

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

Comments included ;) Thank you very much :)

Comment 97

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8500441 [details] [review]
Pull request 24825

There are two visual problems with this that I can see after testing it:
1) There's a line at the bottom of the conference call participants list. See attachment 8466929 [details].
2) When a participant exits a conference call, their name and/or phone number are not greyed out.

Neither of these are showstopping so I would be fine with doing them as followups. But we must file bugs for them and prioritize them.

I left one additional nit on the PR.

Having said all that, thank you, Germán for your really hard work on this, especially when I started throwing redesigns at you. This will serve as reference code for our Design Guidelines, and your work here has really helped me refine the proposals. We went through a lot of review cycles, even after this had already taken a long time, and you stuck with it.

Please put up a demo for this when you get a chance:
https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S8#Demos

Comment 98

[Germán Toro del Valle (:gtorodelvalle)]

Thank you very much for your comments and really valuable reviews, Doug ;)

I included the last nit.

I also created bug 1094746 and bug 1094750 and kindly asked Paco to take care of them so they are properly solved.

I'll wait until the tests pass to merge the PR.

Last, but not least, I'll include a demo in the demo section in a sec.

Comment 99

[Doug Sherk (:drs) (inactive)]

I ran an additional try run here before landing:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=0e2db122b863

https://github.com/mozilla-b2g/gaia/commit/ce15dc9b92ed9c5b7563220eeab9550c77569b52

Comment 100

[Johan Lorenzo [:jlorenzo]]

Verified on:
Gaia-Rev        7004ccfd16e2ad20739bd04b72fa1672ee58686f
Gecko-Rev       afea13fdb3de3abd9ece29d3da5b700abe450988
Build-ID        20141107145850
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.jlorenzo.20141002.104456
FW-Date         Thu Oct  2 10:45:09 CEST 2014
Bootloader      L1TC00011880


Tested the following:
* On call screen, hang up all with participants.
* On call screen, see a participant terminating the call.
* On participant list, hang up all with participants.
* On participant list, see a participant terminating the call.
* On participant list, hang up with one participant while all are terminating the call.
* On contact list, receive a phone call.
* Receive a phone call while in conference

Only two issues noted:
* "Conference (2)" is truncated when you have a conference + another call. See bug 1095522.
* Receive a phone call while in participant list => Bug 1092290 is still occurring

I'll add the missing test cases in Moztrap.

Comment 101

[Zibi Braniecki [:zbraniecki][:gandalf]]

This patch has poor l10n API usage that results in usage of obsolete methods and introduces not retranslatable code.

Please, look at https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices

In particular:
 - do not use .get unless absolutely necessary
 - use data-l10n-id attributes instead of manually assigning textContent
 - design local APIs around l10nId/l10nArgs, not passing result text as arguments

German, do you prefer me to file a follow up bug or reopen this one? Also, do you prefer me to comment in PR with pointers?

Comment 102

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ugh, sorry, wrong bug. Apologies for spamming. :(