977588 - [Dialer] The Active call screen does not have the option to place calls on hold.

Status: VERIFIED FIXED

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

Description

[FangChen]

User Agent: Mozilla/5.0 (Windows NT 6.1; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; TCO_20140227092630; rv:11.0) like Gecko

Steps to reproduce:

    A.- Overview Description (technical background, concise explanation of the bug):
    The DuT lacks the option to place calls on hold.
    
    ________________________________________________________________________________
    B.- Steps to Reproduce (initial conditions, required resources, step by step instructions to reproduce):
    1- From the DuT, perform a voice call to a valid number.
2- Try to place the call on hold


Actual results:

There is no way to place the call on hold through menu options or using the code "2 send" to make a second call.



Expected results:

The DuT must offer some way to place the voice call on hold.

Comment 1

[FangChen]

Built ID :20140218180032
OS version
1.3.0.0-prerelease
QC RIL commit:
a406d321  Add null check when handling set preferred network type
QC RIL TAG version: AU_LINUX_GECKO_B2G_JB_3.2.01.03.00.112.223

gaia commit:
744fb691 Merge pull request #15526 from steveck-chung/bug-961231

gecko commit:
6840e8c2 Bumping gaia.json for 1 gaia-1_3 revision(s) a=gaia-bump

Comment 2

[isabelrios]

Hi, FYi, 

Through menu it is possible to place the call on hold initiating another call by taping on '+' and then dailing or selecting a contact

Through MMI placing a call on hold is not implemented yet and out of scope for 1.3, please see bug 890912.

Thanks

Comment 3

[Jason Smith [:jsmith]]

(In reply to Isabel Rios [:isabel_rios] from comment #2)
> Hi, FYi, 
> 
> Through menu it is possible to place the call on hold initiating another
> call by taping on '+' and then dailing or selecting a contact
> 
> Through MMI placing a call on hold is not implemented yet and out of scope
> for 1.3, please see bug 890912.
> 
> Thanks

So is this bug works for me then? This is implemented as far as I can tell.

Comment 4

[Beatriz Rodríguez [:brg]]

This bug is to ask for a dedicated option to place a call on hold in the active call screen instead of having to select the option to add a new call.

Comment 5

[Noemí Freire (:noemi)]

Hi,

UX input needed here, ni to Carrie.

Comment 6

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

I think currently we already provide "mute" and "add call" button for the similar scenario.
The "on hold" option is more like the landline experience. From the UX perspective, the priority of the feature is not that high. Thanks!

Comment 8

[Noemí Freire (:noemi)]

Adding to backlog to be properly prioritized. Thanks!

Comment 10

[Maria Angeles Oteo (:oteo)]

As this bug is certification waiver, Telefonica will start working on it, providing the IxD in order to be confirmed and aligned with Mozilla UX team (Carrie). Pau can you start working on it?

Comment 11

[Maria Angeles Oteo (:oteo)]

Updated US and ACs according to the Certification requirements (Thanks Beatriz)

Comment 12

[Pau Masiá [:Pau]]

Attachment: Put active call on hold flow

Here I attach the corresponding flow for the "put on hold an active call" scenario.

Carrie, I believe I need your approval for that. 

I also have a doubt about how the icons in the icon pad should look like when the user puts an active call on hold because, from my point of view, since the user is who puts a running call to on hold he/she might like to get access to the other options of the icon pad. And currently, when call is on hold all the icons of the icon pad are disabled.

I hope you can resolve my doubt.

Thanks a lot!

Comment 13

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

Hi Pau, 


Firstly, I'd suggest adopting a new icon for hold button. The one on the mockup is not very clear, especially when the call is being on hold, users would not know which button they shall tap to reactive it.
I'm thinking about maybe having two icon design for the status changes. If the user taps the hold button, the icon will change to an "active" one so that user will know they shall tap the button again to active the call.

Secondly, I think only the speaker and mute button should be disabled. Users may need other functions during the call hold. What do you think? Thanks!

Comment 14

[Pau Masiá [:Pau]]

Ok, Carrie. 

I've thought about what you said of the icon issue and at first I tried to place the same icon that appears as an indicator next to the counter but I realized it didn't work well within the icon pad because there where too many phone shapes in a very reduced area so, after that, I came up with a simple solution which, in my opinion could solve the problem. (Look at the new attachment)

What do you think?

On the other hand, I completely agree with you regarding which buttons should appear disabled.

Thanks!

Comment 15

[Pau Masiá [:Pau]]

Attachment: Put active call on hold flow. Version 2

Comment 16

[Maria Angeles Oteo (:oteo)]

setting ni to Carrie, per comment 14

Comment 17

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

I like the way you address the status changes of the icon, but I'll ni? Carol to review the icon design. :)

Comment 18

[Carol Huang [:Carol]]

Hi Pau,
The solution looks pretty clear! thanks!!!

Comment 19

[Pau Masiá [:Pau]]

(In reply to Carol Huang [:Carol] from comment #18)
> Hi Pau,
> The solution looks pretty clear! thanks!!!

Thanks Carol! So, may I upload the specs and assets for this?

Comment 20

[Carol Huang [:Carol]]

(In reply to Pau Masiá from comment #19)
> (In reply to Carol Huang [:Carol] from comment #18)
> > Hi Pau,
> > The solution looks pretty clear! thanks!!!
> 
> Thanks Carol! So, may I upload the specs and assets for this?

Hey Pau,
Yes, please upload the specs and assets! :)

Comment 21

[Pau Masiá [:Pau]]

Attachment: Assets. PNG. On hold

Here I attach the corresponding assets. Specs to follow.

Comment 22

[Pau Masiá [:Pau]]

We have been thinking about a new way of how hit states in the Dialer app should be shown.
As part of a new concept, we propose that the new hit states have a little animation in order to be more fresh and dynamic.

In the following hours I will upload a mockup of it.

Thanks!

Comment 23

[Pau Masiá [:Pau]]

Attachment: Hit state animation

It's a very basic animation to give you an idea of how it works.

Comment 24

[Anthony Ricaud (:rik)]

Can we see how this would look like in the case of two calls?

Comment 25

[Pau Masiá [:Pau]]

Attachment: Visual Spec: Active call. Put on hold

Visual Specs for Active call - Put on hold.

Comment 26

[Paco Rampas [:paco]]

Attachment: hit-state-callscreen.png

Comment 27

[Paco Rampas [:paco]]

Attachment: hit-state-callscreen.png

Hello Carrie, I have worked with pau for add a new hit state. What do you think about the result? I'm uploading a video too, I will attach it.

Comment 28

[Paco Rampas [:paco]]

Attachment: video demo hit state

Carrie, here is the demo video of hit state.

Comment 29

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

Comment on attachment 8462510 [details]
hit-state-callscreen.png

Hi Paco, 

it looks different from what Pau proposed in comment 14. Have you guys changed the design? Thanks.

Comment 30

[Pau Masiá [:Pau]]

(In reply to Carrie Wang [:carrie] from comment #29)
> Comment on attachment 8462510 [details]
> hit-state-callscreen.png
> 
> Hi Paco, 
> 
> it looks different from what Pau proposed in comment 14. Have you guys
> changed the design? Thanks.

Hi Carrie,

You're right, but the thing is that the change of icon that I proposed is not implemented yet. We only wanted you to notice the new hit states proposal and tell us what do you think about them.

Thanks again!

Comment 31

[Paco Rampas [:paco]]

Attachment: on-hold-button.png

You are right, the previous screen shots and video were confusing.

- I attach here the on hold button like Pau have designed.

- And the video and "hit-state-callscreen.png" is only to show the hit state animation. What do you think about the hit state?

Comment 32

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

Comment on attachment 8463252 [details]
on-hold-button.png

Yes, it looks nice. Thanks. :)

Comment 33

[Paco Rampas [:paco]]

Comment on attachment 8462600 [details]
video demo hit state

Could you give us feedback about the hit state animation? (The image of hold button doesn't change, it is only to show the animation of hit state)

Comment 34

[Maria Angeles Oteo (:oteo)]

Hi guys, I think that the hit state animation should be handled in other bug as it affects to all the buttons in the Call screen.

As Carrie has already agreed with the UX proposal about the "Call on hold" button I think we are ready for the implementation and German can start working on it, based in the current animations.

Paco, can you create another bug with the proposal for the new hit state animation so Carol can evaluate it?
Thanks to all of you!

Comment 35

[Paco Rampas [:paco]]

Ok, I am creating a new bug.

Comment 36

[Anthony Ricaud (:rik)]

Thanks for splitting this in two bugs, it makes more sense.

Before implementing this, I think we still need to answer the question from comment 24. I think this will affect the general design. Needinfo-ing Pau and Carol to get this answered.

Comment 37

[Pau Masiá [:Pau]]

I'm not sure I can really help in here because I think there are some interaction issues that might be solved before I can apply the visual design to it. Carry could you help on this? If I can help in any other way please let me know. I'm also available via email and skype.

Thanks.

Comment 38

[Pau Masiá [:Pau]]

I believe I know how a scenario of a second active call should look like but, I've some doubts about what should happen when, in this scenario, the user wants to pause the active call also. How should it look like?

I'll attach a mockup of the first one but I think I need help for the second one.

Comment 39

[Carol Huang [:Carol]]

Attachment: Call_2ndcallactive.png

There will be some situation when there's more than one Call.(see attachment)
We should figure out the interaction issues first in order to solve the visual problem.

Comment 40

[Pau Masiá [:Pau]]

Attachment: UX Spec. Conference call

Here's my proposal to solve the issue Anthony said. As I don't really know if the option of putting on hold both two calls makes sense, I've proposed another version where the "merge calls" button replaces the "on hold" one in order to do not confuse the user and it is replaced again by the "on hold" one when the calls are merged.

Any feedback please?

Comment 41

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

Hi Pau, I think the merge call scenario works fine with your proposal here. 
It's really difficult to point out what else you've missed in the spec. So I've uploaded an original Dialer spec here. You can take a look and check if you've considered all the pages that is related to this hold button. Thanks!

Comment 42

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

Attachment: Dialer UX Spec 0.7(1).pdf

Comment 43

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

Hi, 

I think the hit state looks fine, but we can't just change here in Dialer pad. This should be implemented on the whole system and it should be review by the visual team. You can discuss it in bug 1044996. Thanks

Comment 44

[Pau Masiá [:Pau]]

Yeap! That's what we'll do. Thanks!

Comment 45

[Paco Rampas [:paco]]

Yes, we know it. Sorry for doesn't remove the flag

Comment 46

[Pau Masiá [:Pau]]

Hi Carrie, I've checked your specs attached and, a priori, I haven't found any extra scenario not covered yet, so I believe it's ok for me. Do you agree?

Thanks!

Comment 47

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

Pau, with your proposal in comment 40, users should always take a call. They cannot put two calls on hold at the same time. I don't know if this matches your requirement from User Stories and ACs?
ni? Maria to confirm the requirement. Thanks

Comment 48

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

In addition, from the spec I attached here, please check p. 29, what shall we do with BT headset? Thanks

Comment 49

[Maria Angeles Oteo (:oteo)]

(In reply to Carrie Wang [:carrie] from comment #47)
> Pau, with your proposal in comment 40, users should always take a call. They
> cannot put two calls on hold at the same time. I don't know if this matches
> your requirement from User Stories and ACs?
> ni? Maria to confirm the requirement. Thanks

That's not possible, as Hsin-Yi explains in another bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1016277#c41), there can only be one call on hold at the same time due to GSM restrictions.

Comment 50

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

(In reply to Carrie Wang [:carrie] from comment #48)
> In addition, from the spec I attached here, please check p. 29, what shall
> we do with BT headset? Thanks

Hi Carrie, in my opinion it does not matter if the call is put on hold using a BT device or directly from the phone, or does it? :)

So my proposal would be to implement what Pau suggests in attachment 8463890 [details] to deal with single and multiple call scenarios. In fact, for the single call scenario (no merge button applies) we would implement attachment 8456850 [details], no matter if a call is put on hold using a BT device or the corresponding button of the call screen.

Are you fine with proposal? :)

Comment 51

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

Hi  Germán, 
For the sake of consistency, I think we should change the BT call page. We can apply the same tool bar as in attachment 8456850 [details] and remove the "Resume" button at the bottom and expend the cancel button as the normal call. This would be my proposal. Thanks!

Comment 52

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

OK, summing it up and covering all the scenarios:

1. Single call no connected BT device -> attachment 8456850 [details] expanding the "Cancel" button to cover all the available space at the bottom.
2. Single call connected BT device -> attachment 8456850 [details] expanding the "Cancel" button to cover all the available space at the bottom. This means we will substitute the current screen we show to the user when the call is set on hold via a BT device with the one proposed as the third screen in that attachment. BTW, to be precise in this scenario the speaker icon would be substituted with the headset one to notify the user that a BT headset is connected.
3. Call waiting scenario (this is establishing 2 calls, keeping one of them on hold) -> attachment 8463890 [details] In this scenario, the "Merge" button would be disabled during the establishment of the second call and enabled once the call is established.

Sorry Carrie to insist, but could you confirm this? Or include modifications if desired, of course :)

Comment 53

[Pau Masiá [:Pau]]

Attachment: Assets. PNG. Merge

Here I attach the merge icon assets. Thanks!

Comment 54

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

Hi  Germán, 

Almost correct...except that we don't need to expand the "cancel" button in attachment 8456850 [details] or 8463890 [details]. I mentioned the extension of the cancel button is based on the old BT call page. The attachment that Pau provided here is the latest design of the cancel call button. So no need to change it. Thanks!

Comment 55

[Doug Sherk (:drs) (inactive)]

I'm putting this back in this sprint since Paco is actively working on it.

Comment 56

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

(In reply to Carrie Wang [:carrie] from comment #54)
> Hi  Germán, 
> 
> Almost correct...except that we don't need to expand the "cancel" button in
> attachment 8456850 [details] or 8463890 [details]. I mentioned the extension
> of the cancel button is based on the old BT call page. The attachment that
> Pau provided here is the latest design of the cancel call button. So no need
> to change it. Thanks!

Great! The size of the "cancel" button is currently being dealt with in bug 1043133 with Carol so we are good :)

Comment 57

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

Hi Carrie! I do have a (hopefully :p) final doubt: I saw in the third screen of attachment 8458636 [details] that the keypad button is kept enabled even when the call is put on hold. This is what Paco has currently implemented but I think it does not have much sense but maybe I am missing something :) What do you think about it? Thanks!

Comment 58

[Paco Rampas [:paco]]

Attachment: patch in github

Hi German, I have attached here all my progress in this bug. I think is in the right way but I would like have your feedback. Thanks :D

Comment 59

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

Hi Germán,

From comment 13, we've reached a conclusion that we only disable mute and speaker buttons when the call is being on-hold. I always think keypad is something very useful in a lot of scenarios. For example, if you are talking with a friend (so you put the call on hold) and need to note down some numbers (phone num), then the keypad will be very useful in this emergency case.

Comment 60

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

Comment on attachment 8469910 [details]
patch in github

Hi Paco, comments included in Github ;)

Comment 61

[Paco Rampas [:paco]]

Comment on attachment 8469910 [details]
patch in github

>https://github.com/mozilla-b2g/gaia/pull/22348

Comment 62

[Paco Rampas [:paco]]

Comment on attachment 8469910 [details]
patch in github

>https://github.com/mozilla-b2g/gaia/pull/22348

Comment 63

[Paco Rampas [:paco]]

Comment on attachment 8469910 [details]
patch in github

>https://github.com/mozilla-b2g/gaia/pull/22348

Comment 64

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

Comment on attachment 8469910 [details]
patch in github

Fine with me. In-depth review by a Call Screen peer pending ;) Thanks Paco!

Comment 65

[Paco Rampas [:paco]]

Attachment: onhold.png

Comment 66

[Carol Huang [:Carol]]

Hi Paco,
one quick question.
I wonder in you screenshot, screen 4 and 5 should switch to match your description on the bottom?
Let me know if it's just misplaced then I can do the UI review.
Thanks!!! :)

Comment 67

[Paco Rampas [:paco]]

> I wonder in you screenshot, screen 4 and 5 should switch to match your
> description on the bottom?
> Let me know if it's just misplaced then I can do the UI review.
Yes, you are right. I am going to update the shots.

Comment 68

[Paco Rampas [:paco]]

Attachment: onhold.png

Updated :)

Comment 69

[Carol Huang [:Carol]]

Comment on attachment 8479650 [details]
onhold.png

Thanks!

Comment 70

[Anthony Ricaud (:rik)]

Comment on attachment 8469910 [details]
patch in github

Redirecting to Doug.

In the review, please pay attention to the separation of concerns (call_screen for DOM operations, calls_handler for logic, handled_call for call specific behaviour).
Also, the state of the buttons (active or not) should be managed by device APIs feedback rather than DOM events.

Comment 71

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8469910 [details]
patch in github

This is actually pretty good from a high level. The SoC is handled well and button states are based on feedback from the telephony API. There are mistakes though, and I left many comments on the PR. I will likely have more on the next review round.

Comment 72

[Anthony Ricaud (:rik)]

Gabriele: Could you take a look at this patch under the CDMA case?

Comment 73

[Gabriele Svelto [:gsvelto]]

(In reply to Anthony Ricaud (:rik) from comment #72)
> Gabriele: Could you take a look at this patch under the CDMA case?

I've glanced over the changes and there's only one thing that comes to mind. AFAIK in CDMA you can put a single call on hold only if it's the only call present. If we're in call waiting mode then putting a call on hold automatically makes the second call active. So in updateOnHoldStatus() you might want to disable the holdButton when we're in CDMA call waiting mode (use the cdmaCallWaiting() function to check for it).

As a follow up we might consider removing the switch calls button we use now (which appears inside the telephone number) and use the holdButton instead but this would require some new UX.

Merge shouldn't be affected because you can't have more than one call connected in CDMA mode.

Comment 74

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8469910 [details]
patch in github

According to Paco's report, this is ready for another round of review.

Comment 75

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8469910 [details]
patch in github

There are some tests missing, and a few of my comments from the last review round haven't been addressed.

Comment 76

[Paco Rampas [:paco]]

> There are some tests missing, and a few of my comments from the last review
> round haven't been addressed.

Yes, I said you in email. Sorry for that, I misunderstood you.

Comment 77

[Doug Sherk (:drs) (inactive)]

(In reply to Paco Rampas [:paco] from comment #76) 
> Yes, I said you in email. Sorry for that, I misunderstood you.

Ah, sorry, I missed your email. I looked at your report on the standup Etherpad which looked like it was saying that this was ready.

Comment 78

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8469910 [details]
patch in github

This is getting really close, but unfortunately we still have a bit of work to do and some things to clarify. See my comments on the PR.

Comment 79

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8469910 [details]
patch in github

Looks good to me. I left a couple of nits on the PR. Flagging Anthony for comment 70. It looks fine to me, but I'd like to be sure (in particular, the device APIs).

Comment 81

[Anthony Ricaud (:rik)]

Comment on attachment 8469910 [details]
patch in github

Have you tried to hold/resume a single call with a bluetooth headset? I believe this is broken.

Also, there is a merge call that will be broken I believe.

Comment 82

[Paco Rampas [:paco]]

> Have you tried to hold/resume a single call with a bluetooth headset? I
> believe this is broken.

Okk, we are looking for a bt headset for trying it.

> Also, there is a merge call that will be broken I believe.
Could you be more specific? Because, we haven't found a problem with that.

Comment 83

[Anthony Ricaud (:rik)]

There is this error when you merge calls for the first time:
W/GeckoConsole( 2143): [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: " {file: "app://callscreen.gaiamobile.org/js/calls_handler.js" line: 772}]

Comment 84

[Paco Rampas [:paco]]

Attachment: patch in github reopened

I have reopened the PR because the autoloader has closed it.
I have updated the patch with the comments asked in the other old PR. We have seen that there is an issue with HS. 

The issue occurs when you try to hold the active call and answer a new call ongoing. The result is that the active call ends and the ongoing call isn't answered. The result should be: hold the active call and answer the new call. 

The problem is: 
CallsHandler.handleHSCommand(message)

This issue is happening in master too, then I think is better open another bug for handling it.

Comment 85

[Beatriz Rodríguez [:brg]]

(In reply to Paco Rampas [:paco] from comment #84)
> Created attachment 8498828 [details] [review]
> patch in github reopened
> 
> I have reopened the PR because the autoloader has closed it.
> I have updated the patch with the comments asked in the other old PR. We
> have seen that there is an issue with HS. 
> 
> The issue occurs when you try to hold the active call and answer a new call
> ongoing. The result is that the active call ends and the ongoing call isn't
> answered. The result should be: hold the active call and answer the new
> call. 
> 
> The problem is: 
> CallsHandler.handleHSCommand(message)
> 
> This issue is happening in master too, then I think is better open another
> bug for handling it.
Done at bug 1079794. Thanks Paco!

Comment 86

[Anthony Ricaud (:rik)]

Comment on attachment 8498828 [details] [review]
patch in github reopened

Sorry for this very late review. For the next iterations, please address the comments in separate comments, that will help to speed up reviews.

This looks good but I haven't checked if the tests are good and comprehensive. I won't have time to review this so redirecting to Doug.

Comment 87

[Paco Rampas [:paco]]

I am reviewing and I add the proposals that Rik said. Now, we have two commits on the patch like Anthony has proposed.

Comment 88

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498828 [details] [review]
patch in github reopened

There are some tests missing as well as some relatively minor, nit-picky problems that I left comments for on the PR.

Comment 89

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498828 [details] [review]
patch in github reopened

Paco, I think we're almost done here. There are just a few more nits to cover. I don't think that the problems that I thought existed are actually problems at all.

Gabriele, I looked into your concerns here but the flow and presentation of state looks reasonable to me. Could you explain the problems you saw?

Note also that this patch shows us that there are some pretty serious issues with SoC in the code that it touches. I'm not going to request any changes here, but I will write about them in the design guidelines. In particular, I think we could make good use of a `CallsUI` class to go alongside the `CallsHandler` class.

Comment 90

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8498828 [details] [review]
patch in github reopened

As I mentioned in comment 73 in CDMA it's impossible to put a single call on hold and when in call waiting mode we have a different UI that makes a "switch calls" button appear on the same line as the call and allows the user to switch between the two. With this patch the hold button would be visible (and enabled) during CDMA calls which is wrong as it wouldn't do anything if only one call was active  and would switch between the calls in call-waiting mode. My suggestion is to avoid showing the button entirely in CDMA mode in updateMergeAndHoldStatus(). Use isFirstCallOnCdmaNetwork() to figure out if the current call is on a CDMA network or not.

Also I suggest adding unit-tests to ensure that we don't show the hold button in CDMA mode.

Comment 91

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

Thanks for your input Gabriele ;)

Carrie, please have a look at comment 90. It seems the on hold button has no sense in CDMA networks and we were wondering if we should keep it disabled or just hide it in these cases.

For GSM networks what Paco has currently implemented is based on attachment 8463890 [details] so I would probably suggest continuously disabling the put-on-holdb button in the case of CDMA calls since hiding it would probably require additional designs from the UX team (showing 4 buttons when there's just 1 call going on, switching to 5 buttons (including the merge one) when there are 2 ongoing calls, etc.). What do you think? Thanks!

Comment 92

[Paco Rampas [:paco]]

Comment on attachment 8498828 [details] [review]
patch in github reopened

Hello Gabriele,

Thanks for your point of view. We have already made changes for CDMA. Now, the hold buttons is disabled with that kind of networks. Could you check it? We can't doing a real test because we haven't acces to any CDMA network, could you test for us? (If you can record a demo video would be fantastic). 

Direct link to the solution:
https://github.com/pacorampas/gaia/commit/4391b59051f88e514dcc0901cac86a6cf82cb290#diff-32d58191c3d41008c8296b8c3b865dafR810

Thanks

Comment 93

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

Hi, 

I think we can disable it, but when there are two calls at the same time, the "on-hold" button will become the "merge-calls" button rather than the "switch-call" button mentioned in comment 90. So I wonder if we need this "merge-calls" button in CDMA? If not, then graying out the button will be fine to me. Thanks!

Comment 94

[Gabriele Svelto [:gsvelto]]

(In reply to Carrie Wang [:carrie] from comment #93)
> So I wonder if we need this "merge-calls" button in CDMA?

It's impossible to merge calls in CDMA mode AFAIK so we probably don't need it either. For how the switch calls button works in CDMA call waiting mode see page 7 in attachment 8430598 [details]. Also see the mocks in attachment 8434026 [details] and attachment 8434028 [details].

Comment 95

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

Oh! :O So according to comment 94 and to the mocks Gabriele mentions there, I guess the right path to go is to just hide the put-on-hold as well as the merge buttons in case of CDMA networks as Gabriele mentioned in comment 90. We didn't know about this impossibility to merge calls in CDMA networks, that's the reason we went for the disabling option :O

We'll keep you posted as soon as a new version of the patch is available ;)

Comment 96

[Paco Rampas [:paco]]

> It's impossible to merge calls in CDMA mode AFAIK 
We didn't know this. Now, the merge and hold buttons isn't show if there is a call connected with CDMA network. Then, the problem is solved. Could you check if it is working in a real environment? (We haven't access to CDMA network).

Thanks.

Comment 97

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

Ups, I forgot about this :(

Hsin-Yi, would be be so kind to tell us about some QA (or not QA) contact in Taipei to try the latest version of the patch provided by Paco in attachment 8498828 [details] [review] using a CDMA network? It seems to be the only viable way to do it. It would be great if that person could record a video of the screen so we could see the results when switching calls :) Would it be possible? Thank you very much!

Comment 98

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8498828 [details] [review]
patch in github reopened

The changes you did to address CDMA networks look spot on; and also great that you added unit-tests to cover those because as you've noticed testing CDMA is somewhat problematic for us.

Comment 99

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Germán Toro del Valle from comment #97)
> Ups, I forgot about this :(
> 
> Hsin-Yi, would be be so kind to tell us about some QA (or not QA) contact in
> Taipei to try the latest version of the patch provided by Paco in attachment
> 8498828 [details] using a CDMA network? It seems to be the only viable way
> to do it. It would be great if that person could record a video of the
> screen so we could see the results when switching calls :) Would it be
> possible? Thank you very much!

Eric could help. Thanks Eric!

Comment 100

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

Hi Eric (I hope Hsin-Yi was referring to you in comment 99 :) ), would you be so kind to try Paco's patch (attachment 8498828 [details] [review]) using a CDMA network? :) It would be great if you could record a video for us ;) The test to run is basically to establish a call from or to the DuT, establish a second call from or to the DuT, and switch between them. Thank you very much!

Comment 101

[Eric Chang [:ericcc] [:echang]]

Sure, I am working on this now, will update soon

Comment 102

[Eric Chang [:ericcc] [:echang]]

The patch works fine but there are new commits to calls_handler.js and calls_handler_test.js, need to rebase.
http://youtu.be/rGQLhbztBa8

Comment 103

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

Awesome, Eric!!! Really appreciated ;)

Paco, would you be so kind to rebase and request for a new review from Doug, please? Thanks!

Comment 104

[Paco Rampas [:paco]]

The patch is already rebased

Comment 105

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498828 [details] [review]
patch in github reopened

Looks good, thanks for all your work on this and sticking with it!

Comment 106

[Doug Sherk (:drs) (inactive)]

Please put a demo for this on the wiki page:
https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S8#Demos

Also, note that I left a couple of nit comments on the PR. Please address them before landing.

Comment 107

[Paco Rampas [:paco]]

merged: 22d918d469725546ab5a28c5c9b94f78877d1b87

Comment 108

[Paco Rampas [:paco]]

Hi Doug,

You can already watch the demo on https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S8#Demos

:D

Comment 111

[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

Testing done: https://www.mindmup.com/#m:g10B7Y2LpZbqKlueVNjX29jaTM3am8

Issues found:
* On hold button says call is held after the participant on hold hangs up (see bug 1095601)
* On hold button says call is held after merging the call in a conference (see bug 1095579)
* Call is not on hold anymore after hanging up another call even though the icon says it is (bug 1094878)
* Call is not on hold anymore after swapping (see bug 1095571). Need UX input on that one.
* Calls are automatically swapped when there is only one participant left in the conference (bug 1095599). Need UX input on that one.


I need to add the test cases in Moztrap.

Comment 112

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

(In reply to Gabriele Svelto [:gsvelto] from comment #94)
> (In reply to Carrie Wang [:carrie] from comment #93)
> > So I wonder if we need this "merge-calls" button in CDMA?
> 
> It's impossible to merge calls in CDMA mode AFAIK so we probably don't need
> it either. For how the switch calls button works in CDMA call waiting mode
> see page 7 in attachment 8430598 [details]. Also see the mocks in attachment
> 8434026 [details] and attachment 8434028 [details].

Hi Hsinyi and Gabriele,

Ben found an issue that we cannot merge two calls in CDMA because there is no 'merge' button on the call screen.  It looks like it's the decision made in this bug to remove the button.  However, AFAIK, merging call is required for CDMA 3-way calling feature and we probably need to add it back.  Ben will file a bug later for the issue and we could discuss there.

Comment 113

[Michael Schwartz [:m4]]

Hi I'm seeing the same regression reported by https://bugzilla.mozilla.org/show_bug.cgi?id=977588#c112  Is there a follow-up bug?

STR:
# Initiate a MO CDMA call
# Add a new call

Expected: Merge button should be available
Observed: No merge or swap button is available

Comment 114

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #112)
> (In reply to Gabriele Svelto [:gsvelto] from comment #94)
> > (In reply to Carrie Wang [:carrie] from comment #93)
> > > So I wonder if we need this "merge-calls" button in CDMA?
> > 
> > It's impossible to merge calls in CDMA mode AFAIK so we probably don't need
> > it either. For how the switch calls button works in CDMA call waiting mode
> > see page 7 in attachment 8430598 [details]. Also see the mocks in attachment
> > 8434026 [details] and attachment 8434028 [details].
> 
> Hi Hsinyi and Gabriele,
> 
> Ben found an issue that we cannot merge two calls in CDMA because there is
> no 'merge' button on the call screen.  It looks like it's the decision made
> in this bug to remove the button.  However, AFAIK, merging call is required
> for CDMA 3-way calling feature and we probably need to add it back.  Ben
> will file a bug later for the issue and we could discuss there.

Oops... sorry for missing this.

Just to confirm that it's possible to merge CDMA 3way call, not CDMA waiting call. We should add a merge button for the 3way call scenario.

Comment 115

[Gabriele Svelto [:gsvelto]]

OK, I'll open a new bug.

Comment 116

[Gabriele Svelto [:gsvelto]]

Apparently we already had bug 1147123 for enabling the merge button in CDMA 3-way call mode.

Comment 117

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #114)
Why did we advocate for the addition of these checks when TelephonyCall already has fields indicating to the UI whether merge/swap buttons should be provided?

Comment 118

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Michael Schwartz [:m4] from comment #117)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #114)
> Why did we advocate for the addition of these checks when TelephonyCall
> already has fields indicating to the UI whether merge/swap buttons should be
> provided?

I was trying to explaining the real scenario. You are right, for the gaia implementation, TelephonyCall.mergeable/.switchable could be used.