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

VERIFIED FIXED in 2.1 S8 (7Nov)

Status

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: fang.chen1, Assigned: paco)

Tracking

({feature})

unspecified
2.1 S8 (7Nov)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v2.2 verified)

Details

(Whiteboard: kephera_53278 [planned-sprint c=][in-sprint=v2.1-S7])

User Story

As a user I would like to be able to put a call on hold

AC1: When the call is put on hold, the other party hears the call go on hold.
AC2: When a call is on hold I can resume it.
AC3: After the call is put on hold, I can start new calls via the Address Book, the Dialpad or the Call Log. If the second call is established, I can place the active call on hold and retrieve the held call (User on hold will hear the “call on hold” sound as per AC1).
AC4: If a user has two calls, one active and one on hold, and the call on hold is finished by the other party, I am notified abut it (Bug 995938)
AC5: If a user has two calls, one active and one on hold, and the active one hangs up, the on hold one is kept held and can still be retrieved. 
AC6:the group call functionality should still be working as in previous FirefoxOS versions

Attachments

(15 attachments, 2 obsolete attachments)

241.47 KB, image/png
Details
245.14 KB, image/png
Details
2.98 KB, application/zip
Details
75.38 KB, image/gif
Details
252.51 KB, image/png
Details
480.02 KB, image/png
cawang
: ui-review-
Details
27 bytes, text/plain
Details
460.43 KB, image/png
cawang
: ui-review+
Details
154.14 KB, image/png
Details
326.06 KB, image/png
Details
3.05 MB, application/pdf
Details
4.24 KB, application/zip
Details
52 bytes, text/plain
drs
: review+
gtorodelvalle
: feedback+
Details
975.90 KB, image/png
Carol
: ui-review+
Details
46 bytes, text/x-github-pull-request
drs
: review+
gsvelto
: feedback+
Details | Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: kephera_53278
(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.
(Reporter)

Updated

5 years ago
Blocks: 948791
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.
Summary: [zffos1.3][P3](Local) The DuT lacks the option to place calls on hold. → [Dialer] The Active call screen does not have the option to place calls on hold.
Hi,

UX input needed here, ni to Carrie.
Flags: needinfo?(cawang)
Keywords: feature
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!
Flags: needinfo?(cawang)
Duplicate of this bug: 975301
Adding to backlog to be properly prioritized. Thanks!
blocking-b2g: --- → backlog
Duplicate of this bug: 897441
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?
Flags: needinfo?(b.pmm)
Updated US and ACs according to the Certification requirements (Thanks Beatriz)
User Story: (updated)
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!
Flags: needinfo?(b.pmm) → needinfo?(cawang)
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!
Flags: needinfo?(cawang)
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!
setting ni to Carrie, per comment 14
Flags: needinfo?(cawang)
I like the way you address the status changes of the icon, but I'll ni? Carol to review the icon design. :)
Flags: needinfo?(cawang) → needinfo?(chuang)
Hi Pau,
The solution looks pretty clear! thanks!!!
Flags: needinfo?(chuang)
Assignee: nobody → gtorodelvalle
Target Milestone: --- → 2.1 S1 (1aug)
Whiteboard: kephera_53278 → kephera_53278 [planned-sprint c=3]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: kephera_53278 [planned-sprint c=3] → kephera_53278 [planned-sprint]
Whiteboard: kephera_53278 [planned-sprint] → kephera_53278 [planned-sprint c=3]
(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?
Flags: needinfo?(chuang)
(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! :)
Flags: needinfo?(chuang)
Here I attach the corresponding assets. Specs to follow.
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!
Posted image Hit state animation
It's a very basic animation to give you an idea of how it works.
Can we see how this would look like in the case of two calls?
Visual Specs for Active call - Put on hold.
(Assignee)

Comment 26

5 years ago
Posted image hit-state-callscreen.png (obsolete) —
Attachment #8462507 - Flags: ui-review?(cawang)
(Assignee)

Comment 27

5 years ago
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.
Attachment #8462507 - Attachment is obsolete: true
Attachment #8462507 - Flags: ui-review?(cawang)
Attachment #8462510 - Flags: ui-review?(cawang)
(Assignee)

Comment 28

5 years ago
Carrie, here is the demo video of hit state.
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.
Attachment #8462510 - Flags: ui-review?(cawang) → ui-review-
(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!
(Assignee)

Comment 31

5 years ago
Posted image 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?
Attachment #8463252 - Flags: ui-review?(cawang)
Comment on attachment 8463252 [details]
on-hold-button.png

Yes, it looks nice. Thanks. :)
Attachment #8463252 - Flags: ui-review?(cawang) → ui-review+
(Assignee)

Comment 33

5 years ago
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)
Attachment #8462600 - Flags: ui-review?(cawang)
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!
Flags: needinfo?(pacorampas)
(Assignee)

Updated

5 years ago
Attachment #8462600 - Flags: ui-review?(cawang)
Flags: needinfo?(pacorampas)
(Assignee)

Comment 35

5 years ago
Ok, I am creating a new bug.
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.
Flags: needinfo?(chuang)
Flags: needinfo?(b.pmm)
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.
Flags: needinfo?(b.pmm) → needinfo?(cawang)
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.
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.
Flags: needinfo?(chuang)
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?
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!
Flags: needinfo?(cawang)
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
Flags: needinfo?(pacorampas)
Flags: needinfo?(b.pmm)
Yeap! That's what we'll do. Thanks!
Flags: needinfo?(b.pmm)
(Assignee)

Comment 45

5 years ago
Yes, we know it. Sorry for doesn't remove the flag
Flags: needinfo?(pacorampas)
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!
Flags: needinfo?(cawang)
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
Flags: needinfo?(cawang) → needinfo?(oteo)
In addition, from the spec I attached here, please check p. 29, what shall we do with BT headset? Thanks
Flags: needinfo?(b.pmm)
(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.
Flags: needinfo?(oteo)
(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? :)
Flags: needinfo?(cawang)
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!
Flags: needinfo?(cawang)
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 :)
Flags: needinfo?(cawang)
QA Contact: lolimartinezcr
Whiteboard: kephera_53278 [planned-sprint c=3] → kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S1]
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Depends on: 1048285
Assignee: gtorodelvalle → pacorampas
Whiteboard: kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S1] → kephera_53278
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Posted file Assets. PNG. Merge
Here I attach the merge icon assets. Thanks!
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!
Flags: needinfo?(cawang)

Updated

5 years ago
Flags: needinfo?(b.pmm)
I'm putting this back in this sprint since Paco is actively working on it.
Status: NEW → ASSIGNED
Whiteboard: kephera_53278 → kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S1]
Target Milestone: 2.1 S3 (29aug) → 2.1 S2 (15aug)
(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 :)
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!
Flags: needinfo?(cawang)
(Assignee)

Comment 58

5 years ago
Posted file 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
Attachment #8469910 - Flags: feedback?(gtorodelvalle)
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.
Flags: needinfo?(cawang)
Comment on attachment 8469910 [details]
patch in github

Hi Paco, comments included in Github ;)
Attachment #8469910 - Flags: feedback?(gtorodelvalle) → feedback-
Whiteboard: kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S1] → kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S2]
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
(Assignee)

Updated

5 years ago
Attachment #8469910 - Flags: feedback- → feedback?(gtorodelvalle)
Comment on attachment 8469910 [details]
patch in github

Fine with me. In-depth review by a Call Screen peer pending ;) Thanks Paco!
Attachment #8469910 - Flags: feedback?(gtorodelvalle) → feedback+
(Assignee)

Updated

5 years ago
Attachment #8469910 - Flags: review?(anthony)
(Assignee)

Comment 65

5 years ago
Posted image onhold.png (obsolete) —
Attachment #8478958 - Flags: ui-review?(chuang)
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!!! :)
Flags: needinfo?(pacorampas)
(Assignee)

Comment 67

5 years ago
> 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.
Flags: needinfo?(pacorampas)
(Assignee)

Comment 68

5 years ago
Posted image onhold.png
Updated :)
Attachment #8478958 - Attachment is obsolete: true
Attachment #8478958 - Flags: ui-review?(chuang)
Attachment #8479650 - Flags: ui-review?(chuang)
Comment on attachment 8479650 [details]
onhold.png

Thanks!
Attachment #8479650 - Flags: ui-review?(chuang) → ui-review+
Whiteboard: kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S2] → kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S3]
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
Whiteboard: kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S3] → kephera_53278 [planned-sprint][in-sprint=v2.1-S3]
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.
Attachment #8469910 - Flags: review?(anthony) → review?(drs+bugzilla)
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.
Attachment #8469910 - Flags: review?(drs+bugzilla) → review-
Gabriele: Could you take a look at this patch under the CDMA case?
Flags: needinfo?(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.
Flags: needinfo?(gsvelto)
Comment on attachment 8469910 [details]
patch in github

According to Paco's report, this is ready for another round of review.
Attachment #8469910 - Flags: review- → review?(drs+bugzilla)
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.
Attachment #8469910 - Flags: review?(drs+bugzilla) → review-
(Assignee)

Comment 76

5 years ago
> 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.
(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.
Whiteboard: kephera_53278 [planned-sprint][in-sprint=v2.1-S3] → kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S4]
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
(Assignee)

Updated

5 years ago
Attachment #8469910 - Flags: review- → review?(drs+bugzilla)
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.
Attachment #8469910 - Flags: review?(drs+bugzilla) → review-
(Assignee)

Updated

5 years ago
See Also: → 1069836
(Assignee)

Updated

5 years ago
Attachment #8469910 - Flags: review- → review?(drs+bugzilla)
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).
Attachment #8469910 - Flags: review?(drs+bugzilla)
Attachment #8469910 - Flags: review+
Attachment #8469910 - Flags: feedback?(anthony)
Duplicate of this bug: 1065953
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.
Attachment #8469910 - Flags: feedback?(anthony) → feedback-
(Assignee)

Comment 82

5 years ago
> 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.
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}]
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
(Assignee)

Comment 84

5 years ago
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.
Attachment #8498828 - Flags: review?(anthony)
Depends on: 1079794
(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!
See Also: → 1081028
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.
Attachment #8498828 - Flags: review?(anthony) → review?(drs+bugzilla)
Whiteboard: kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S4] → kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S6]
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (Oct24)
Whiteboard: kephera_53278 [planned-sprint c=3][in-sprint=v2.1-S6] → kephera_53278 [planned-sprint c=1][in-sprint=v2.1-S6]
(Assignee)

Comment 87

5 years ago
I am reviewing and I add the proposals that Rik said. Now, we have two commits on the patch like Anthony has proposed.
Flags: needinfo?(drs.bugzilla)
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.
Attachment #8498828 - Flags: review?(drs+bugzilla) → review-
Flags: needinfo?(drs+bugzilla)
No longer depends on: 1079794
(Assignee)

Updated

5 years ago
Attachment #8498828 - Flags: review- → review?(drs.bugzilla)
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.
Attachment #8498828 - Flags: review?(drs.bugzilla)
Attachment #8498828 - Flags: review-
Attachment #8498828 - Flags: feedback?(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.
Attachment #8498828 - Flags: feedback?(gsvelto) → feedback-
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!
Flags: needinfo?(cawang)
(Assignee)

Comment 92

5 years ago
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
Attachment #8498828 - Flags: feedback- → feedback?(gsvelto)
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!
Flags: needinfo?(cawang)
(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].
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 ;)
(Assignee)

Comment 96

5 years ago
> 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.
Whiteboard: kephera_53278 [planned-sprint c=1][in-sprint=v2.1-S6] → kephera_53278 [planned-sprint c=][in-sprint=v2.1-S7]
Target Milestone: 2.1 S7 (24Oct) → 2.1 S9 (21Nov)
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!
Flags: needinfo?(htsai)
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.
Attachment #8498828 - Flags: feedback?(gsvelto) → feedback+
(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!
Flags: needinfo?(htsai)
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!
Flags: needinfo?(echang)
Sure, I am working on this now, will update soon
Flags: needinfo?(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
Awesome, Eric!!! Really appreciated ;)

Paco, would you be so kind to rebase and request for a new review from Doug, please? Thanks!
Flags: needinfo?(pacorampas)
(Assignee)

Comment 104

5 years ago
The patch is already rebased
Flags: needinfo?(pacorampas)
Attachment #8498828 - Flags: review- → review?(drs.bugzilla)
Comment on attachment 8498828 [details] [review]
patch in github reopened

Looks good, thanks for all your work on this and sticking with it!
Attachment #8498828 - Flags: review?(drs.bugzilla) → review+
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.
Flags: needinfo?(pacorampas)
Target Milestone: 2.1 S9 (21Nov) → 2.1 S8 (7Nov)
(Assignee)

Comment 107

5 years ago
merged: 22d918d469725546ab5a28c5c9b94f78877d1b87
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(pacorampas)
Resolution: --- → FIXED
(Assignee)

Comment 108

5 years ago
Hi Doug,

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

:D
Duplicate of this bug: 883036
Duplicate of this bug: 903951
Depends on: 1094878
Depends on: 1095571
Depends on: 1095579
Depends on: 1095601
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.
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(jlorenzo)
(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.
QA Whiteboard: [COM=Gaia::Dialer]
See Also: → 1127559
blocking-b2g: backlog → ---
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
(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.
OK, I'll open a new bug.
Apparently we already had bug 1147123 for enabling the merge button in CDMA 3-way call mode.
(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?
Flags: needinfo?(htsai)
(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.
Flags: needinfo?(htsai)
Depends on: 1168515
Depends on: 1159958
Depends on: 1160325
Depends on: 1160337
Depends on: 1162732
Depends on: 1170296
Depends on: 1169409
Flags: in-moztrap?(jlorenzo)
You need to log in before you can comment on or make changes to this bug.