Closed Bug 1094878 Opened 5 years ago Closed 5 years ago

Call is not on hold anymore after hanging up another call even though the icon says it is

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2+, b2g-v2.2 verified)

VERIFIED FIXED
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified

People

(Reporter: jlorenzo, Assigned: gtorodelvalle)

References

Details

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
drs
: review+
Details | Review
STR
1. Make a phone call to another device
2. Put this call on hold
3. Receive another phone call and pick it up
4. Hang up the second call
5. Say "hello"

Expected result
Like when you mute the microphone, you should be in the same state as when you picked up the second call; that is to say, the call is still on hold.

Actual result
Even if the icon says the call is on hold, you're able to hear "hello" on the other device.
Paco, Germán, could one of you take a look at this?
Flags: needinfo?(pacorampas)
Flags: needinfo?(gtorodelvalle)
[Blocking Requested - why for this release]: Major issue on a new feature with the Dialer.
triage: major issue from new feature
blocking-b2g: 2.2? → 2.2+
Assignee: nobody → pacorampas
Flags: needinfo?(pacorampas)
Attached file patch in github (obsolete) —
This patch clean the active-state of on hold button when you add, remove or merge calls. 

Could you check if is a good approach?
Attachment #8518884 - Flags: feedback?(gtorodelvalle)
Hi guys! After thinking about it, I am need-infoing Carrie about this issue just to have her thoughts about the expected behaviour mentioned in comment 0 or description :) Should the DuT behave as Johan mentions or should it just retake automatically the held call? I am asking this because there are many scenarios where the suggested behaviour and final decision may affect. To name a few:
  1. Imagine that after 3, the user decides to switch between calls putting one on hold and retaking the other by typing on the call headers. After doing that and hanging the ongoing call, should we keep the other one held or should it be retaken?
  2. Imagine that, after putting the first call on hold, instead of an incoming second call reaching the phone, the user decides to establish a second outgoing call using the "place new call" button (as a side note just mentioning that this would automatically put the currently ongoing call on hold if it was not already held). This second call is attended and hung up (by any side). Should the first (currently held) call be kept as on hold or should it be automatically retaken?
  3. In fact and referring to the STR mentioned by Johan in comment 0 but also to the case I mention in 2, do you think it should make any difference if the call is hung up by the DuT or the other side regarding the desired behaviour when keeping the other call as held or retaking it.

Just pointing this out so we provide the user with an uniform and user-friendly approach in all the related scenarios. I am sure Carrie will provide us with the best solution ;)

Thanks!
Flags: needinfo?(gtorodelvalle) → needinfo?(cawang)
See Also: → 1082588
Hi, 


I'd suggest that if users manually tap the "hold" button in a call (as what you described in comment 5 case 2), we keep the latest status of the call for them, which means, the first call will still be on hold when users finish the second call. However, for other cases, I'd suggest following our current design. I don't feel it's the same as "mute" case. Because if there are two calls at a time, you can directly tap on it to activate a call and hold the other one, you don't really need to tap "hold" button first and then tap the other one and toggle-off the "hold" of it there. So I think users can understand once the call is finished, the connection will be switched to the other one directly. I think this can make the behavior consistant and easy to understand. Thanks!
Flags: needinfo?(cawang)
I know the way that users answer the second call is also tapping the "hold" button and it may look like the case 2 from comment 5, but I still don't feel it's the same. I think we shall rethink the design of answering the second call (ignore the new call/ decline current/ hold current) rather than adding an additional step when users drop the call and wanna switch back to the previous one. Thanks!
See Also: → 1095601
See Also: 1082588
See Also: → 1095579
Hi Carrie, thanks for your comments.

We currently have a first working implementation of the following (I hope I make myself more or less clear :) ):
  1. If the user puts the ongoing call on hold (no matter if 1-to-1 or conference call) clicking on the "put on hold" button:
    1.1. If a new incoming call reaches the phone or the user makes a second outgoing call:
       1.1.1. If this second call is established and hung up, the first (held) call is kept as held.
       1.1.2. If this second call is established and the user switches between calls, when the second incoming call is hung up (in fact any of them), the first (the other one) is resumed automatically.
       1.1.3. If this second call is established and the user merges both calls, the held status of the first ongoing call is removed, forgotten or no longer taken into consideration, just as in 1.1.2.

There's another scenario I would like to check with you:
  1. A first (incoming or outgoing) call is established.
  2. A second (incoming or outgoing) call is established.
  3. Both calls are merged into a conference call. According to 1.1.3 before, the held status of the first call is no longer taken into consideration.
  4. The conference call is put on hold.
  5. Any of the remote participants leaves the conference call, so a 1-to-1 call to the connected party goes on.
Right now this 1-to-1 call is kept as held since the call conference it was taking part in was held.
This scenario could be extended if the conference call was established amongst more than 3 parties in which case if any of the remote participants leaves the conference call, it is kept as held.
Another extension of this scenario is when after 4. a new call reaches or is made from the phone. If some of the calls participating in the conference call leaves the conference, the conference call itself or the pending 1-to-1 call (if all the rest of the participants leave the conference) is kept as held if these additional calls are hung up and of course the user does not switch between them.

I am really sorry but I did not know how to summarise all this and sincerely recording a video does not seem to help much here because there are too many scenarios :(
Flags: needinfo?(cawang)
Hi Germán, 

Actually the way you describe it is really clear. Bravo! :)
I agree with you on all of them. I was thinking about keeping the case 1.1.2 held after the second call is finished, but rethink it, let's keep the swap as a "shuffle" stage - you will not keep carrying your "hold" status anymore, and it will make the rule easier to understand. Besides, I also agree on the second scenario. I think the logic makes sense and it's ready to go. Thanks! 
Ping me anytime if you find any question.
Flags: needinfo?(cawang)
Assignee: pacorampas → gtorodelvalle
Attached file Pull request 26055
Hi guys! This is the patch implementing what we discussed with Carrie in the previous comments.

Johan, since this patch in kind of sensitive regarding what it updates, I thought it would be great if you could check that it does what it should do even before landing it in master. What do you think? We could follow the normal workflow if you consider it appropriate, of course ;) Thanks!
Attachment #8518884 - Attachment is obsolete: true
Attachment #8518884 - Flags: feedback?(gtorodelvalle)
Attachment #8521318 - Flags: review?(drs.bugzilla)
Attachment #8521318 - Flags: feedback?(jlorenzo)
I'm okay with checking that patch before it reaches master. I'll keep you tuned.
Comment on attachment 8521318 [details] [review]
Pull request 26055

I don't think that this is a good approach. This gets us deeper into the hole of handling UI changes as exceptions requiring additional logic, instead of just monitoring backend state. It would be better if we were more careful with requesting backend state changes, and to hook changes there to update the UI when they happen.

Here's what I imagine instead:
* HandledCall has a member on it indicating whether or not a call being placed on hold was set by the user.
* Any logic that places calls on hold or resumes them checks this variable before taking action and requesting toggling of the state.
* The UI is updated only and entirely within an `onstatechange` callback (see bug 1082588 comment 1).

I'm clearing Johan's feedback flag since this will require many changes.
Attachment #8521318 - Flags: review?(drs.bugzilla)
Attachment #8521318 - Flags: review-
Attachment #8521318 - Flags: feedback?(jlorenzo)
Hi Doug, there's an scenario where your approach to keep the "user put the call on hold" status at a HandledCall level is not "feasible", without further changes, I mean. It is the case when a conference call is going on, which can also be put on hold. There's no HandledCall for the conference call. Would you be fine with having this status at a ConferenceGroupHandler level?

Anyhow, I am not completely sure I am in favor of this approach compared to the previous one based on having the "user put the call on hold" status at a CallsHandler level. BTW, in the proposed patch all the code updating the UI is triggered by event handlers (statechange and callschanged). The main reasons are:
  1. All the decisions to be made based on this status are made at the CallsHandler level (this is, resuming or not the pending call once another one has been hung up and responding to clicking on the "put on hold" button to hold or to resume the call). The only current case where this status is checked is when a second call is hung up and this scenario is currently managed by CallsHandler regarding the decision to keep the pending call held or to resume it.
  2. The need to keep somewhere the "user put the call on hold" status for the conference call, if any.
  3. Only 1 call can be put on hold (by the user) at any time (no matter if a normal one or a conference call).

On the other hand, it is true that this "user put the call on hold" status is something which should be stored at a call or conference call level, at least at first glance.

So both approaches have their advantages and limitations. We can chat about them via IRC or via additional comments ;)
Flags: needinfo?(drs.bugzilla)
Comment on attachment 8521318 [details] [review]
Pull request 26055

After discussing it with Doug via IRC we realised that the confusion was due to a unfortunate function name. Sorry (and also glad :p) about that! ;)

BTW, during my testing I noticed and filed bug 1098455 which I decided to deal with as an independent bug since it is not really related to the issue reported in this very bug.
Attachment #8521318 - Flags: review- → review?(drs.bugzilla)
Comment on attachment 8521318 [details] [review]
Pull request 26055

This is a lot closer to what we should be doing than I thought in comment 12. Sorry for not looking deeper at it. On first glance, it looked wrong because I saw many calls to `CallScreen.setOnHoldActiveStatus()`. But what I said still stands, and we only need one call to it, possibly two (for conference calls).

I found some bugs with this patch when testing it. When a call is being placed, the 'on hold' button is in the active, but greyed out, state. It also toggles when calls are being disconnected, even if there's are no user-held calls before the disconnect.

I have proposed a plan in the PR to fix this. It will need some additional work, I suspect, to get it to work correctly with conference calls.
Flags: needinfo?(drs.bugzilla)
Attachment #8521318 - Flags: review?(drs.bugzilla) → review-
Comment on attachment 8521318 [details] [review]
Pull request 26055

Damn it! That was a hell of a proposal ;) I had the feeling that I should have gathered all the UI updating code in CallsHandler.updateMergeAndOnHoldStatus() from the beginning :( Sorry about that!

BTW, you'll notice a minor change to cover the case when the conference call is the one put on hold by the user ;)

Lessons learned: Do not take code from third parties and add code on top without rethinking the whole solution to the problem ;)

Thanks!
Attachment #8521318 - Flags: review- → review?(drs.bugzilla)
Duplicate of this bug: 1095601
Duplicate of this bug: 1095579
Comment on attachment 8521318 [details] [review]
Pull request 26055

This patch is generally pretty good, but I found a pretty serious problem with it. You can get a call into the user-held state without having ever set it this way. Here are the STR:

1. Dial contact 1 from DuT.
2. Put contact 1 on hold.
3. Have contact 2 dial DuT.
4. Answer it on the DuT.
5. Have contact 1 hang up.
6. Call contact 3 from the DuT.
7. Have contact 2 hang up.

Expected: DuT will not be on hold with contact 3.
Actual: DuT is on hold with contact 3.

I think that this is happening because we're not associating `callHeldByUser` with a call. To fix this, we will probably have to create an association here.

Other than that, it's great to see such good unit test coverage! There were a couple of missing tests that I commented on, but you added many new ones that weren't affected by the code changes in this patch, so thanks for that.
Attachment #8521318 - Flags: review?(drs.bugzilla) → review-
Comment on attachment 8521318 [details] [review]
Pull request 26055

Hi Doug, great catch! ;) Thanks!

Finally it seems that the best approach is a combination of the previously discussed ones ;) Basically it consists on keeping the code managing the 'call held by user' status in CallsHandler but linking this status to the precise held call so situations as the ones you mention in your previous comment are properly managed ;)

Since it can only be one held call at a time I opted to keep the status in a variable pointing to the held call as you will see in the code ;)
Attachment #8521318 - Flags: review- → review?(drs.bugzilla)
Comment on attachment 8521318 [details] [review]
Pull request 26055

Getting there! Please see my comments on the PR.
Attachment #8521318 - Flags: review?(drs.bugzilla) → review-
Comment on attachment 8521318 [details] [review]
Pull request 26055

Comments in the PR included ;) Thank you very much!
Attachment #8521318 - Flags: review- → review?(drs.bugzilla)
Comment on attachment 8521318 [details] [review]
Pull request 26055

Looking good, thanks for this patch! I left a few comments on the PR. Please ping me on IRC before landing this so I can have one final look. I think it'll be fine though. Needinfo for that, and for doing a demo.
Flags: needinfo?(gtorodelvalle)
Attachment #8521318 - Flags: review?(drs.bugzilla) → review+
Comments and demos included ;) I'll ping you via IRC for the final check ;) Thanks!
Flags: needinfo?(gtorodelvalle)
Comment on attachment 8521318 [details] [review]
Pull request 26055

I think I missed your ping. Looks good, please land and post a demo:
https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S9#Demos
Flags: needinfo?(gtorodelvalle)
Duplicate of this bug: 944302
Merged in master: https://github.com/mozilla-b2g/gaia/commit/ad8e103094a85504c26080eecbcc68a9ce33b9ca
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(gtorodelvalle)
Resolution: --- → FIXED
QA Contact: jlorenzo
Hey  Germán!

I'm trying to verify this fix, but I'm a bit confused about what you decided was the right way to handle this. Going back to the original repo steps in comment 0:


Once you hang up that second call, is the intended behavior to automatically reconnect to the first call, or for the first call to remain on hold until the user manually switches back.

When I just tested this, it automatically switches back to the first call. Thanks for your help!
Flags: needinfo?(gtorodelvalle)
Verified the issue is fixed on Flame 2.2
Based on the expected result in comment "0" and comment "19"

as per comment "o" the phone stays in "hold" mode and voice is muted
as per comment "19" the phone is not in "hold" mode and voice couldn't be heard

"Flame 2.2

Device: Flame 2.2 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141201040205
Gaia: 39214fb22c203e8849aaa1c27b773eeb73212921
Gecko: 08be3008650f
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (Unknown)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
"
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(In reply to Robert Mead [:RobertM] from comment #28)
> Hey  Germán!
> 
> I'm trying to verify this fix, but I'm a bit confused about what you decided
> was the right way to handle this. Going back to the original repo steps in
> comment 0:
> 
> 
> Once you hang up that second call, is the intended behavior to automatically
> reconnect to the first call, or for the first call to remain on hold until
> the user manually switches back.
> 
> When I just tested this, it automatically switches back to the first call.
> Thanks for your help!

Hi Robert, putting it simple if the user puts a call on hold, that status should be remembered and retaken automatically if there's any other subsequent incoming or outgoing call which is hung up afterwards, but only if the user does not toggle between both calls or merges them. In any of these 2 last cases, the on-hold status of the first call should be forgotten and no longer taken into consideration.

I hope this helps ;) If not, please do not hesitate to need-info me again :) Thanks!
Flags: needinfo?(gtorodelvalle)
You need to log in before you can comment on or make changes to this bug.