Loli (:lolimartinezcr) Reporter
	Description • 10 years ago

Hamachi
2.0
Gecko-fcf1d2a
Gaia-2bb6663

Reproducible 100%

STRs:
1. Tap dialer application
2. Write phone number
3. Tap call button
4. While user can see "Connecting" user can see "add other call" button enable

Actual result
user can see "add other call" button enabled. I think it isn't correct because while is "connecting" isn't posible call other phone (I need info UX)

Expected result:
User can see "Add other call" button disabled.

	Loli (:lolimartinezcr) Reporter
	Comment 1 • 10 years ago

Also Keypad and microphone buttons should be disabled while call is "connecting"

	Maria Angeles Oteo (:oteo)
	Comment 2 • 10 years ago

Just commenting that this bug is not a regression and these buttons have always been enabled when the call is "connecting".

Tested on hamachi v1.4 version:
B-90
Gecko-dcfc7e2
Gaia-43da854
Platform version:30.0
BuildID:20140612024834

	Victoria Gerchinhoren [:vicky]
	Comment 3 • 10 years ago

Yes, they have been implemented like that, but Loli is right, we should fix this.

Thanks!

	Maria Angeles Oteo (:oteo)
	Comment 4 • 10 years ago

Paco, can you work on it?
Thanks a lot!

	Anthony Ricaud (:rik)
	Comment 9 • 10 years ago

I'm not sure this is a desirable UX. Let me share two scenarios:

- I'm calling a service center. I know that as soon as it answers, I'll have to type numbers to navigate. So I open the keyboard while it is connecting.
- I'm setting up a conference call. I dial the first person. While I wait for the first person to answer, I start looking for the second one.

Carrie: What do you think?

(I'm clearing the review while we wait for UX input)

	Anthony Ricaud (:rik)
	Comment 10 • 10 years ago

For each solution, I think it's better to disable the buttons that do the actions. So disabling call buttons and keypad buttons. Not the buttons that open the keyboard or display the dialer.

	Maria Angeles Oteo (:oteo)
	Comment 11 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #9)
> I'm not sure this is a desirable UX. Let me share two scenarios:
> 
> - I'm calling a service center. I know that as soon as it answers, I'll have
> to type numbers to navigate. So I open the keyboard while it is connecting.
> - I'm setting up a conference call. I dial the first person. While I wait
> for the first person to answer, I start looking for the second one.

You are right, in fact if I am in a noisy environment and I have to join to a conference call I would like also to mute my microphone before the call is successfully established.

I'm checking in my i-phone and the only option that is disabled is the option to make a multiparty call. The option to open the address book is available and if I go to a contact and try to call him (while the call has not been established) I am returned to the call (no action is taken).
If I do the same after the call has been connected, the first call is put on hold and the second one (with the contact I have selected) is started.

The only weird thing that I have seen on FxOS is that if I click on "Add calls" so Contact is opened, go to details and click on a phone number, while connecting, nothing happens (which seems to be ok) however if after connecting the call I click again on the phone number no action is done (which is not ok)

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 12 • 10 years ago

My suggestion would be only disable the "Add call" button here. Because users can't really connect to a second call at that moment, the button is useless in this scenario and might cause confusion. 

p.s. I can't reproduce Maria's problem here. I can add a call successfully when there is one call connected.

	Maria Angeles Oteo (:oteo)
	Comment 13 • 10 years ago

I continue seeing that issue in 2.0 latest version following this STR:
1. A starts a call with contact B
2. During the connection, A clicks on "Add Call" button in the call screen
3. Contacts is opened and A select contact C in the Agenda
4. When A press on contact C telephone number in details, nothing happens, what's is ok
5. B answers and the call is established between A and B
6. After finishing the call, when you try to call to other contact of the Agenda from contact details, the call button does not work.

Anyway, if we implement Carrie's proposal and disable the "Add call", this bug would disappear ;)

	Paco Rampas [:paco]
	Comment 15 • 10 years ago

Comment on attachment 8448569 [details]
patch in github

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

	Paco Rampas [:paco]
	Comment 17 • 10 years ago

Now, it is only disable the add call button, like Carrie said.

	Anthony Ricaud (:rik)
	Comment 18 • 10 years ago

Carrie: If we can disable all the buttons that actually place a call (in the call log, in contacts, etc) when we cannot place a call, would that be a better UX than disabling this callscreen button?

We can take this patch to improve the current situation but we should probably open another bug to do even better in the future.

	Anthony Ricaud (:rik)
	Comment 19 • 10 years ago

Comment on attachment 8448571 [details] [review]
patch in github

I think the logic in this patch is wrong. We should disable the "place new call" button when there is at least one call that is in the "connecting" state. And re-enable it when there is no calls in the connecting state.

Hsin-Yi: Can you confirm that those are the only cases when we cannot place a new call?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 20 • 10 years ago

(In reply to Carrie Wang [:carrie] from comment #12)
> My suggestion would be only disable the "Add call" button here. Because
> users can't really connect to a second call at that moment, the button is
> useless in this scenario and might cause confusion. 
> 
> p.s. I can't reproduce Maria's problem here. I can add a call successfully
> when there is one call connected.

Echo on Carrie's suggestion.

We can't disable keypad even the call is connecting, not connected yet. It's due to the 3gpp spec definition. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=852314#c13

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 21 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #19)
> Comment on attachment 8448571 [details] [review]
> patch in github
> 
> I think the logic in this patch is wrong. 

Agree the logic is wrong.

> We should disable the "place new
> call" button when there is at least one call that is in the "connecting"
> state. And re-enable it when there is no calls in the connecting state.
> 
> Hsin-Yi: Can you confirm that those are the only cases when we cannot place
> a new call?

Rik, you are right. And to be more precise, if there's at least one call with "dialing" or "alerting" state. That's when user sees 'Connecting' on call screen.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 22 • 10 years ago

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> (In reply to Anthony Ricaud (:rik) from comment #19)
> > Comment on attachment 8448571 [details] [review]
> > patch in github
> > 
> > I think the logic in this patch is wrong. 
> 
> Agree the logic is wrong.
> 
> > We should disable the "place new
> > call" button when there is at least one call that is in the "connecting"
> > state. And re-enable it when there is no calls in the connecting state.
> > 
> > Hsin-Yi: Can you confirm that those are the only cases when we cannot place
> > a new call?
> 
> Rik, you are right. And to be more precise, if there's at least one call
> with "dialing" or "alerting" state. That's when user sees 'Connecting' on
> call screen.

And here is the list of thorough call states:
http://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyCall.cpp?from=TelephonyCall.cpp#68-99

	Paco Rampas [:paco]
	Comment 23 • 10 years ago

Comment on attachment 8448571 [details] [review]
patch in github

I have updated the patch. I think currently the logic is like your comment 19. Also, I have changed the issues that you said to me on github. 

There are two commits on patch because I have just started the changes of tests.

	Anthony Ricaud (:rik)
	Comment 24 • 10 years ago

Comment on attachment 8448571 [details] [review]
patch in github

Sorry, it is not :( Comments in Github.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 25 • 10 years ago

Hi Anthony! I prepared a new PR since the previous one was causing some weird behaviours and this way it is faster :)

As you will see I have to update calls_handler.js and handled_call.js to get the desired behavior since the oncallschange does not notify (at least it wasn't) when a call evolves from 'dialing' to 'connected' for example.

Any comments are more than welcome ;)

P.D.: The testing of this patch was kind of weird due to bug 1039553, but I would say I am pretty confident about it :)

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 26 • 10 years ago

Hi again! I noticed something which may end up being confusing to the user. The thing is that currently the "Place new call" button is kept enabled when there are 2 ongoing calls (1 held and 1 ongoing, I mean) which does not have much sense since it is not possible to establish a third one. What do you think about this, Carrie?

In case you want to change the current behaviour we could deal with it in this very bug or open a new one for it ;) Thanks!

	Anthony Ricaud (:rik)
	Comment 27 • 10 years ago

Germàn: Before reviewing, I'd like to check if Gecko should send us that oncallschanged.

Hsin-Yi: I thought that Telephony.oncallschanged was fired every time after all the  TelephonyCall.statechange are fired. We don't receive a oncallschanged when a call is going from 'dialing' to 'connected'. Is that expected?

	Anthony Ricaud (:rik)
	Comment 28 • 10 years ago

Germán, Carrie: Let's open a new bug for that.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 29 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #27)
> Germàn: Before reviewing, I'd like to check if Gecko should send us that
> oncallschanged.
> 
> Hsin-Yi: I thought that Telephony.oncallschanged was fired every time after
> all the  TelephonyCall.statechange are fired.

No, it's not the design.
Telephony.oncallschanged is fired when 1) Telephony.calls array is loaded and sync. with backend 2) when the number of Telephony.calls array changes, i.e. a new call is added or an existing call is removed

> We don't receive a
> oncallschanged when a call is going from 'dialing' to 'connected'. Is that
> expected?

It's intentional as there is call state change but no the number of calls change.

	Anthony Ricaud (:rik)
	Comment 30 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

I think a cleaner approach is to have aCall.addEventListener('statechange', CallsHandler.updatePlaceNewCall.bind(CallsHandler))

And updatePlaceNewCall does something like (pseudoish-code):
var isDialing = telephony.calls.some( call => call.state == 'dialing' or 'alerting')
if (isDialing) CallScreen.disablePlaceNewCall()
else CallScreen.enablePlaceNewCall()


Also, I don't understand the changes in ConferenceGroupHandler, could you explain them?

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 31 • 10 years ago

Hi, 

So, our rule is that if there is a call connecting, we will disable the add call button. In this case, if there are two calls, one is on hold and the other is connecting, then yes, we shall disable the add call button here. Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 32 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

Yeah, I would say that with this approach the code is better grouped. The only drawback is that we check if there is an alerting or dialing call and update the "place new call" enabled/disabled button status (this does not mean it may change) on every call status change, but I guess you already counted on that ;)

	Anthony Ricaud (:rik)
	Comment 33 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

We're missing the tests that disable the place new call button (when we receive another call or place another call).

I've left two small comments on Github too.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 34 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

Comments included in the patch ;) Thanks Anthony!

	Anthony Ricaud (:rik)
	Comment 35 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

On top of my Github comments, we still need the tests in the suites:
telephony.oncallschanged handling > receiving a first incoming call
telephony.oncallschanged handling > receiving an extra incoming call

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 36 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

I included those additional tests and the comments you left in Github ;-) BTW, I left some comments regarding the addition of listeners in mock objects as well as using spies instead of updating properties in them.

If you consider it appropriate, please do not hesitate to contact me via IRC once you have a look at the patch again since it would let us speed the landing up :) Thanks!

Last but not least, I included a new commit with the latest changes as you always ask me to but I always forget. We can always squash all the commits once you set the r+ ;)

	Anthony Ricaud (:rik)
	Comment 37 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

We still need one more test in handled_call.js that checks that we install the proper statechange listener.

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 38 • 10 years ago

Comment on attachment 8457945 [details]
21867.html

Hi Anthony,
I updated an already existent test: https://github.com/mozilla-b2g/gaia/pull/21867/files#diff-08f2a55579da54c0d78bfff6b34abaa3L144
and included the suggested one: https://github.com/mozilla-b2g/gaia/pull/21867/files#diff-08f2a55579da54c0d78bfff6b34abaa3R148
Thanks!

	Germán Toro del Valle (:gtorodelvalle) Assignee
	Comment 39 • 10 years ago

Landed in master: https://github.com/mozilla-b2g/gaia/commit/87a968f356a82e1123582a8dcd41e1df69f3c163

	Loli (:lolimartinezcr) Reporter
	Comment 40 • 10 years ago

Tested and working
2.1
Flame
Gecko-d5a4ba9
Gaia-80a41e0

2.0
Flame
Gecko-7f737a2
Gaia-6155d46

Hamachi
1.4
Gecko-63a5966
Gaia-31cd873

	Loli (:lolimartinezcr) Reporter
	Comment 41 • 10 years ago

(In reply to Loli from comment #40)
> Tested and working
> 2.1
> Flame
> Gecko-d5a4ba9
> Gaia-80a41e0
> 
> 2.0     ----> Add call button is ENABLED
> Flame
> Gecko-7f737a2
> Gaia-6155d46
> 
> Hamachi   ----> Add call button is ENABLED
> 1.4
> Gecko-63a5966
> Gaia-31cd873

In 2.0 and 1.4 should be fixed?

	Anthony Ricaud (:rik)
	Comment 42 • 10 years ago

I'll leave the needinfo for Maria in case she disagrees. I believe this shouldn't be fixed in 1.4 and 2.0 because it's a user experience refinement.

	Maria Angeles Oteo (:oteo)
	Comment 43 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #42)
> I'll leave the needinfo for Maria in case she disagrees. I believe this
> shouldn't be fixed in 1.4 and 2.0 because it's a user experience refinement.
Completely agree with you, it's an enhacement, not necessary for older versions.
Thanks for asking!

	Loli (:lolimartinezcr) Reporter
	Comment 44 • 10 years ago

(In reply to Maria Angeles Oteo (:oteo) PTO 4th-25th Aug from comment #43)
> (In reply to Anthony Ricaud (:rik) from comment #42)
> > I'll leave the needinfo for Maria in case she disagrees. I believe this
> > shouldn't be fixed in 1.4 and 2.0 because it's a user experience refinement.
> Completely agree with you, it's an enhacement, not necessary for older
> versions.
> Thanks for asking!

In this case I change status to VERIFIED