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

Status: RESOLVED FIXED

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

Description

[promise09th]

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.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

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.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Display the call being held only when it's confirmed to be in the held state

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.

Comment 3

[Doug Sherk (:drs) (inactive)]

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.

Comment 4

[promise09th]

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

Comment 5

[promise09th]

I missed comment

I think you move your code from "calls_handler.js" to "handled_call.js"

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

(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!

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Change the callscreen appearance only in response to a change in the underlying state

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.

Comment 8

[promise09th]

Attachment: bug_1081028_applied_patch.png

Precondition : I insert GSM SIM

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

Thank you

Comment 9

[promise09th]

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.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

(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.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Do not try to hold a single call in CDMA mode since it's not supported

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.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

FYI I've filed bug 1082588 for making the UI state-driven instead of action-driven and moved my WIP patch there.

Comment 13

[promise09th]

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

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

(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.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Do not try to hold a single call in CDMA mode since it's not supported

Here's the (quick) fix with unit-test added.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Do not try to hold a single call in CDMA mode since it's not supported

Here's the pull-request.

Comment 17

[Doug Sherk (:drs) (inactive)]

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);
```

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

Addressed the last nit and merged to gaia/master 35f24c53c7ffdaa2bbac905528e3fc515f0d8920

https://github.com/mozilla-b2g/gaia/commit/35f24c53c7ffdaa2bbac905528e3fc515f0d8920

Comment 19

[promise09th]

Dear Gabriele Svelto

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

Bug 1087166