Closed Bug 1081028 Opened 5 years ago Closed 5 years ago

[Dialer] Though CDMA Network doesn't support single call hold, but user can hold single call via BT Headset

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: promise09th, Assigned: gsvelto)

References

Details

(Whiteboard: [LibGLA,17519,425,Carrier,A])

Attachments

(3 files, 3 obsolete files)

1. Title : Though CDMA Network doesn't support single call hold, but user can hold single call via BT Headset
2. Precondition :
 - Insert CDMA SIM
 - Connect BT Headset
3. Tester's Action : 
 1) Connecting call
 2) Hold current call via BT Headset
4. Detailed Symptom : Callscreen UI is changed and Call is not held
5. Expected : Callscreen UI is unchanged and Call is not held
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Mozilla Frame : Cannot test this issue because Mozilla frame doesn't recognize CDMA SIM
8. Gaia revision : 87ee41fcb3f9a14d7a8bb67f1dd7fd95a6bcd0f0

CDMA Network is not supported single call hold feature. But now, user can hold single call via BT Headset in CDMA Network. In this case, gecko and modem layer doesn't hold current call, but callscreen show "Hold" UI.
So, I think if user try to hold in CDMA network, device rejects this request and device show Error popup.
Whiteboard: [LibGLA,None,None,A]
Confirming, in the code handling BT headset commands we invoke holdOrResumeSingleCall() whenever there's only one active call, CDMA or not:

https://github.com/mozilla-b2g/gaia/blob/cfe5165889a5e07779fe73aaa7d9d1b08b2845fe/apps/callscreen/js/calls_handler.js#L339

holdOrResumeSingleCall() doesnt' check if we're in CDMA mode or not, it will only try to hold the call and won't verify if the call was really held or not before updating the screen:

https://github.com/mozilla-b2g/gaia/blob/cfe5165889a5e07779fe73aaa7d9d1b08b2845fe/apps/callscreen/js/calls_handler.js#L339

Long story short, there's two way of solving this:

- The quick fix, we can just check if we're in CDMA mode within holdOrResumeSingleCall() and turn the function in a no-op if we are

- The long-term approriate fix: we should not change the dialer appearance after having executed an action, we should instead wait for the oncallschanged event and use the information there to adjust the UI. As with other bugs this would essentially mean moving from a model where the UI changes are directly driven by the code to one in which they're in response to state changes, i.e. an MVC model.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here's a quick WIP patch that moves changing the callscreen state from the holdOrResumeSingleCall() method to the onCallsChanged() handler. This breaks the unit-tests and I haven't verified if it works correctly under all scenarios, it was just meant as a WIP to verify if it's feasible to go for a more robust solution to this problem.
Attachment #8503116 - Flags: feedback?(drs+bugzilla)
Comment on attachment 8503116 [details] [diff] [review]
[PATCH WIP] Display the call being held only when it's confirmed to be in the held state

This seems reasonable to me. I suggest you take a look at bug 977588 though as this will change the code here. The nice thing about this approach is that this will work no matter where in our code we place a call on hold.
Attachment #8503116 - Flags: feedback?(drs+bugzilla) → feedback+
See Also: → 977588
Whiteboard: [LibGLA,None,None,A] → [LibGLA,17519,425,Carrier,A]
Created attachment 8503116 [details] [diff] [review] [diff] [review]
[PATCH WIP] Display the call being held only when it's confirmed to be in the held state

This patch has some issues

1. When receiving incoming call, callscreen show not "incoming call" UI but "connected call" UI always.
2. Device don't show correct hold call UI in GSM network because device can't receive callschanged event when user hold current call (device can receive statechange event only).

Please check these things


Thank you
Flags: needinfo?(gsvelto)
I missed comment

I think you move your code from "calls_handler.js" to "handled_call.js"
(In reply to promise09th from comment #4)
> 1. When receiving incoming call, callscreen show not "incoming call" UI but
> "connected call" UI always.
> 2. Device don't show correct hold call UI in GSM network because device
> can't receive callschanged event when user hold current call (device can
> receive statechange event only).

Thanks a lot for your feedback, this is very helpful!
Flags: needinfo?(gsvelto)
This is a second WIP. Here I've moved all the existing CallScreen.render() invocations into the oncallschanged handler (this obviously broke some unit-tests). I haven't tested it super-thoroughly because my Flame sometimes crashes when using a BT headset but it generally seems to work correctly.
Attachment #8503116 - Attachment is obsolete: true
Precondition : I insert GSM SIM

When I apply your patch, callscreen UI is not normally shown. Please check attached image.

Thank you
Created attachment 8504380 [details]
bug_1081028_applied_patch.png

Sorry, I missed information.
When I hold current call via BT headset, this callscreen UI is shown.
Flags: needinfo?(gsvelto)
(In reply to promise09th from comment #9)
> Sorry, I missed information.
> When I hold current call via BT headset, this callscreen UI is shown.

Good, that's one of the things I hadn't tested well because my Flame crashes when I pair it with my BT headset. I'll try again today and see what needs fixing.
Flags: needinfo?(gsvelto)
Since the other patch turned out more complicated than I thought here's the quick, straightforward fix that disables trying to put a single call on hold if we're on a CDMA network. Unit-tests are missing, I'll add them later.
Assignee: nobody → gsvelto
Attachment #8504079 - Attachment is obsolete: true
Status: NEW → ASSIGNED
FYI I've filed bug 1082588 for making the UI state-driven instead of action-driven and moved my WIP patch there.
Created attachment 8504747 [details] [diff] [review] [diff] [review]
[PATCH] Do not try to hold a single call in CDMA mode since it's not supported

This patch should need Bug 970187, so this patch is not working in v2.0.
But, if I apply both Bug 970187 patch and your patch, it is working well.
In CDMA network, I don't hold current call, and I can hold current call in GSM network.

Thank you
(In reply to promise09th from comment #13)
> This patch should need Bug 970187, so this patch is not working in v2.0.
> But, if I apply both Bug 970187 patch and your patch, it is working well.
> In CDMA network, I don't hold current call, and I can hold current call in
> GSM network.

If this works for you I can prepare a patch that works against 2.0 without having to uplift bug 970187 which is rather large and has Gecko dependencies. I'll fix the unit-tests in the meantime and ask for review to land on master.
Here's the (quick) fix with unit-test added.
Attachment #8504747 - Attachment is obsolete: true
Attachment #8506011 - Flags: review?(drs.bugzilla)
Comment on attachment 8506011 [details] [diff] [review]
[PATCH] Do not try to hold a single call in CDMA mode since it's not supported

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

This seems reasonable, but I get the feeling that it's not totally robust. I think this is a consequence of the structure of the CDMA logic sprinkled into CallsHandler, and we can't do anything about it without refactors.

::: apps/callscreen/test/unit/calls_handler_test.js
@@ +1780,5 @@
> +          MockNavigatorMozTelephony.mTriggerCallsChanged();
> +
> +          var holdSpy = this.sinon.spy(call1, 'hold');
> +          triggerCommand('CHLD=2');
> +          sinon.assert.notCalled(holdSpy);

Nit: use this pattern instead:

```js
this.sinon.spy(call1, 'hold');
triggerCommand('CHLD=2');
sinon.assert.notCalled(call1.hold);
```
Attachment #8506011 - Flags: review?(drs.bugzilla) → review+
See Also: → 1082588
Addressed the last nit and merged to gaia/master 35f24c53c7ffdaa2bbac905528e3fc515f0d8920

https://github.com/mozilla-b2g/gaia/commit/35f24c53c7ffdaa2bbac905528e3fc515f0d8920
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Dear Gabriele Svelto

I found one issue when I apply this patch.
So, I make new bugzilla

Bug 1087166
Blocks: 1087166
Blocks: 1096234
You need to log in before you can comment on or make changes to this bug.