Closed
Bug 1094878
Opened 10 years ago
Closed 10 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)
Tracking
(blocking-b2g:2.2+, b2g-v2.2 verified)
People
(Reporter: jlorenzo, Assigned: gtorodelvalle)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Comment 1•10 years ago
|
||
Paco, Germán, could one of you take a look at this?
Flags: needinfo?(pacorampas)
Flags: needinfo?(gtorodelvalle)
Reporter | ||
Comment 2•10 years ago
|
||
[Blocking Requested - why for this release]: Major issue on a new feature with the Dialer.
Updated•10 years ago
|
Assignee: nobody → pacorampas
Flags: needinfo?(pacorampas)
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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!
Assignee | ||
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: pacorampas → gtorodelvalle
Assignee | ||
Comment 10•10 years ago
|
||
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)
Reporter | ||
Comment 11•10 years ago
|
||
I'm okay with checking that patch before it reaches master. I'll keep you tuned.
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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-
Assignee | ||
Comment 16•10 years ago
|
||
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)
Comment 19•10 years ago
|
||
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-
Assignee | ||
Comment 20•10 years ago
|
||
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 21•10 years ago
|
||
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-
Assignee | ||
Comment 22•10 years ago
|
||
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 23•10 years ago
|
||
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+
Assignee | ||
Comment 24•10 years ago
|
||
Comments and demos included ;) I'll ping you via IRC for the final check ;) Thanks!
Flags: needinfo?(gtorodelvalle)
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
Merged in master: https://github.com/mozilla-b2g/gaia/commit/ad8e103094a85504c26080eecbcc68a9ce33b9ca
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(gtorodelvalle)
Resolution: --- → FIXED
Reporter | ||
Updated•10 years ago
|
QA Contact: jlorenzo
Comment 28•10 years ago
|
||
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)
Comment 29•10 years ago
|
||
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)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Description
•