946866 - [DSDS][Dialer] On-the-fly SIM change when calling

Status: VERIFIED FIXED

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

Description

[Wilfred Mathanaraj [:WDM]]

As a user, when I long tap the button "Dial" in the Dialer, I want to be able to choose from which SIM the call must be made.

Acceptance Criteria:

AC1: I only want this option to apply for that specific call 
AC2: I want to see the default SIM for making a call highlighted on this screen 
AC3: I want a "remember my choice" option, and when I click on it the
default option in settings should be changed to this new SIM  
AC4: When I select a SIM I want the call to be placed. I should not have to press on dial again to make that call.

Comment 1

[Wilfred Mathanaraj [:WDM]]

AC3 is removed as acceptance criteria

Comment 2

[Ken Chang[:kenkai]]

(In reply to Wilfred Mathanaraj [:WDM] from comment #0)
> As a user, when I long tap the button "Dial" in the Dialer, I want to be
> able to choose from which SIM the call must be made.
> 
> Acceptance Criteria:
> 
> AC1: I only want this option to apply for that specific call 
> AC2: I want to see the default SIM for making a call highlighted on this
> screen 
> AC3: I want a "remember my choice" option, and when I click on it the
> default option in settings should be changed to this new SIM  
> AC4: When I select a SIM I want the call to be placed. I should not have to
> press on dial again to make that call.
Does we need to have a method in dailer to notify user this feature can be triggered when he/she long press "dial"?

Comment 3

[José Vittone]

Attachment: icon-long-tap-indicator.zip

Comment 4

[José Vittone]

Attachment: DSDS.Dailer.png

Visuals and visual specs related to DSDS. NI to Carrie in order to review the correct IxD implementation.

Comment 5

[José Vittone]

Attachment: DSDS.Dailer_SIM-picker.png

Comment 6

[José Vittone]

Attachment: SPEC_DSDS.Dialer.png

Comment 7

[José Vittone]

Attachment: SPEC_DSDS.Dialer.sim_picker.png

Comment 8

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

Hi Jose,

Looks nice. Thanks for the help!

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Anthony,

what's the preferable value for `always ask` ? I would update this value from SimCardManager and want to ask you about the better value for this special case. 

My personal opinion would be a specific `Number` value maybe 999 or -1 to represent special case, what do you think ?

Comment 10

[Anthony Ricaud (:rik)]

-1 has my preference. Thanks. Is there a bug opened to track this btw?

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Anthony Ricaud (:rik) from comment #10)
> -1 has my preference. Thanks. Is there a bug opened to track this btw?

Thanks for your quick reply ! I track the related SimcardManager works on bug 972150. 

Let's use -1 for this case, yeah. :P

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

features should not block release, remove blocking flag

Comment 13

[David Scravaglieri [:scravag]]

Changing target milestone

Comment 14

[Enpei]

Hi Wilfred,

Could you update the AC since AC 3 is no longer valid?

Thanks.

Comment 15

[José Vittone]

Anthony Ricaud pointed out via email:
> My only remark is about the SIM picker screen.
> I think we should include the number we’re about to dial in the header.

I think Carrie may have a word about this. From our side (TEF), we believe it is good to add the phone number on the header because if you stay on the picker for a while you may loose context. NI to Carrie.

Comment 16

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8374235 [details]
SPEC_DSDS.Dialer.sim_picker.png

The |text-align: right| attr is unclear to me. Are you saying here that you want the (default) part to be aligned to the right side of the button (not pictured)? I'm not sure how else to interpret it.

Comment 17

[José Vittone]

Attachment: SPEC_DSDS.Dialer.sim_picker.png

Hello Doug, sorry for the confusion. "text-align: right" should not be there. Updating the SPEC.

Comment 18

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

Yes, I think "numbers" + "dial via" will be clear, but my concern is that can we detect the SIM numbers every time (I think we discussed this problem before)? I need some one to confirm it. Maybe ni? Hsinyi.

(In reply to José Vittone from comment #15)
> Anthony Ricaud pointed out via email:
> > My only remark is about the SIM picker screen.
> > I think we should include the number we’re about to dial in the header.
> 
> I think Carrie may have a word about this. From our side (TEF), we believe
> it is good to add the phone number on the header because if you stay on the
> picker for a while you may loose context. NI to Carrie.

Comment 19

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

(In reply to :Carrie Wang from comment #18)
> Yes, I think "numbers" + "dial via" will be clear, but my concern is that
> can we detect the SIM numbers every time (I think we discussed this problem
> before)? I need some one to confirm it. Maybe ni? Hsinyi.
> 
Carrie, you got it right. Phone numbers are optional. Not every SIM card provides the information.

> (In reply to José Vittone from comment #15)
> > Anthony Ricaud pointed out via email:
> > > My only remark is about the SIM picker screen.
> > > I think we should include the number we’re about to dial in the header.
> > 
> > I think Carrie may have a word about this. From our side (TEF), we believe
> > it is good to add the phone number on the header because if you stay on the
> > picker for a while you may loose context. NI to Carrie.

Comment 20

[Doug Sherk (:drs) (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #19)
> Carrie, you got it right. Phone numbers are optional. Not every SIM card
> provides the information.

Is there a reason not to show them if they're provided?

Comment 21

[Doug Sherk (:drs) (inactive)]

Attachment: [DSDS][Dialer] On-the-fly SIM change when calling.

Could I get your thoughts please? Things I still have to do:
* Refactor some of the UI code for generating buttons so it can be shared with the contacts app for bug 947189, bug 947186 and anywhere else like the SMS app. I think I will do this as part of one of those bugs and make them depend on this.
* Write tests for the new functionality.
* Refactor the "ask me every time" code into the shared section.

Comment 22

[Doug Sherk (:drs) (inactive)]

Attachment: [DSDS][Dialer][Settings] Refactor SimSettingsHelper into shared code.

Additional to-do:
* Add listeners for changes to settings which should update the UI.

Comment 23

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Doug Sherk (:drs) from comment #22)
> Created attachment 8378665 [details] [diff] [review]
> [DSDS][Dialer][Settings] Refactor SimSettingsHelper into shared code.
> 
> Additional to-do:
> * Add listeners for changes to settings which should update the UI.

Doug++ !!

Comment 24

[Doug Sherk (:drs) (inactive)]

Attachment: Refactor SIM card options into shared code.

Comment 25

[Doug Sherk (:drs) (inactive)]

Attachment: Refactor SimSettingsHelper into shared code.

Comment 26

[Doug Sherk (:drs) (inactive)]

Attachment: On-the-fly SIM change when calling.

I would consider these ready for review. I'm not requesting review? because I still need to add unit tests. The changes to the contacts and SMS apps should come shortly after this since I did all the required refactoring already.

Comment 27

[Anthony Ricaud (:rik)]

Sorry to give this feedback so late. The architecture of the patch is to rethink.

keypad.js should know nothing about the events on the call button and nothing about the DOM and events of the SIM picker. That way, it will be easy to port it to Contacts.

keypad.js should only do something like CallButton.init(callButtonElement, callbackAfterSimKnown);
And callbackAfterSimKnown is a callback that would take a SIM card as a parameter.

You can look at https://github.com/mozilla-b2g/gaia/pull/15436/files (that never landed) to see what I mean.

Keypad:
- Calls CallButton.init
- Listens to click events on the call button for the "redial last number" feature.
CallButton:
- Decides what kind of events to listen to based on number of SIMs and user settings
- Asks the SIM picker to display itself when needed
- Sends the SIM card to use to place a call (whether or not we displayed a SIM picker)
SIM Picker:
- DOM creation and event listening for the UI
- Sends the SIM chosen to CallButton (or maybe directly to Keypad, I'm not sure about that one)


In sim_menu_helper.js (I would rename sim_picker.js), we should not do any DOM manipulation before opening the SIM picker.

Let me know if my explanations are not clear.

Comment 28

[Doug Sherk (:drs) (inactive)]

Attachment: On-the-fly SIM change when calling.

OK, I still have to add tests, but I think this deals with all of your review comments. Please review it as if it's actually going through review, other than that.

One note is that I didn't see a need for the CallButton object/class, so I chose to exclude it. I think without it, it's equally easy to abstract for any other use case like a contacts or SMS button.

Comment 29

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

Alright, here it is, fully ready for review, with refactors, old tests fixed, new tests, and linting.

Julien, would you review the commits marked as [Settings] or [SMS] please? Anthony, please take the ones marked as [Dialer], or all of them.

/not sure if this is the process you guys usually use, I'm used to posting a series of patches

Also, note that I haven't done any of the localization work needed. I have no idea how that process works, so I just left some stub localized files lying around.

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Doug Sherk (:drs) from comment #29)
> Created attachment 8382765 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685
> 
> Alright, here it is, fully ready for review, with refactors, old tests
> fixed, new tests, and linting.
> 
> Julien, would you review the commits marked as [Settings] or [SMS] please?
> Anthony, please take the ones marked as [Dialer], or all of them.
> 
> /not sure if this is the process you guys usually use, I'm used to posting a
> series of patches

Usually we do only one commit but really I don't mind having well separated commits.

> 
> Also, note that I haven't done any of the localization work needed. I have
> no idea how that process works, so I just left some stub localized files
> lying around.

You don't need to do anything for l10n work, you just need to update the en-US files and the l10n process will start from there.

The only thing to remember is to change the key whenever you change the value. (not needed for the technical side, I know, but needed for the l10n side).

Haven't looked in details yet but this looks good for the SMS part.

Comment 31

[Doug Sherk (:drs) (inactive)]

Filed bug 977915 as a result of a workaround I had to do for some platform problems.

Comment 32

[Doug Sherk (:drs) (inactive)]

Filed bug 978300.

Comment 33

[Joe Cheng [:jcheng] (please needinfo)]

move milestone to S3 Mar 14 since it's almost done

Comment 34

[Anthony Ricaud (:rik)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

Things I'm not sure about:
- Moving action_menu.js from SMS to shared. This looks overkill. All we need is "display some HTML, attach some event handlers, hide some HTML". That's like 10 lines of code. This option_menu thing is more like 100 lines.
- SIMPicker takes two callbacks and that's weird to me. keypad.js should not know how to place a call anymore I think.

Things I'm sure about:
- The unit tests should never rely on code from anything but the module being tested. You'll need more mocks.
- We should be displaying the phone number to be called in the SIM picker header.
- Current code doesn't change the Call button to display the user's preferred SIM.


Because of that, I think we should really introduce a CallButton class that would modify the DOM, install event handlers according to user preferences and call TelephonyHelper. Also, it's weird to have some methods of TelephonyHelper that take two connections as parameters. I think we should remove all code from TelephonyHelper that deals with connections and let CallButton and SIMPicker collaborate to provide a connection to TelephonyHelper.
In order to do that, we need to modify the contacts application to use CallButton and SIMPicker.

I'm doing all of this in my head without coding so some of my assumptions may be wrong.

Comment 35

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

(In reply to Anthony Ricaud (:rik) from comment #34)
> Comment on attachment 8382765 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

(IRC)
> Rik: and CallButton shouldn't LazyLoad sim_picker dependencies

I wasn't sure how to deal with this. Are you saying that we should just load it directly from the index.html file, or lazy load it from another JS file?

> Things I'm not sure about:
> - Moving action_menu.js from SMS to shared. This looks overkill. All we need
> is "display some HTML, attach some event handlers, hide some HTML". That's
> like 10 lines of code. This option_menu thing is more like 100 lines.

OptionMenu really Just Works (tm). I suspect that we would have more duplicated code without it, and I don't think it's as simple as 10 lines, after accounting for callbacks, localization, etc.

> - Current code doesn't change the Call button to display the user's
> preferred SIM.

I think this falls out of the scope of this bug. Filed bug 980208.

> Because of that, I think we should really introduce a CallButton class that
> would modify the DOM, install event handlers according to user preferences
> and call TelephonyHelper. Also, it's weird to have some methods of
> TelephonyHelper that take two connections as parameters. I think we should
> remove all code from TelephonyHelper that deals with connections and let
> CallButton and SIMPicker collaborate to provide a connection to
> TelephonyHelper.
> In order to do that, we need to modify the contacts application to use
> CallButton and SIMPicker.

This is a really serious refactor which doesn't actually save any code or, IMO, any sanity. To me, it intuitively makes more sense for TelephonyHelper to be handling connections than CallButton or SIMPicker. We don't save any code because, at the end of the day, we still have to pass a card index into the telephony DOM API, so we have to carry around both the index and the connection. MozMobileConnection doesn't have an index on it, so we can't use that. If you still think we should do this, can you explain more of your reasoning?

Updated the PR.

Comment 36

[Anthony Ricaud (:rik)]

I've discussed this IRL with Julien so removing his feedback request.
(In reply to Doug Sherk (:drs) from comment #35)
> (IRC)
> > Rik: and CallButton shouldn't LazyLoad sim_picker dependencies
> 
> I wasn't sure how to deal with this. Are you saying that we should just load
> it directly from the index.html file, or lazy load it from another JS file?
My IRC comment was amended to say CallButton.init(). We should lazy load sim_picker.js (and it's dependencies) when we want to open the SimPicker UI (aka JIT ;) ). Right now, because it's in .init(), it's loaded in the startup path. Also, sim_picker.js should lazy load it's HTML.

> > Things I'm not sure about:
> > - Moving action_menu.js from SMS to shared. This looks overkill. All we need
> > is "display some HTML, attach some event handlers, hide some HTML". That's
> > like 10 lines of code. This option_menu thing is more like 100 lines.
> 
> OptionMenu really Just Works (tm). I suspect that we would have more
> duplicated code without it, and I don't think it's as simple as 10 lines,
> after accounting for callbacks, localization, etc.
No, it doesn't "just work". You had to modify it to do what you want and move it to shared. (and add the relevant unit tests)
You have two callbacks to install. One listening to the parent of the sim buttons. Another one for the cancel button that calls SimPicker.hide().
For the localisation, it's just _('select-sim-menu-button', {n: i+1}) in the loop.
Trust me, it will be less code in sim_picker.js without a dependency. And it will also be more static HTML. Julien also agrees.
I'll write that part so don't worry about it.

> This is a really serious refactor which doesn't actually save any code or,
> IMO, any sanity. To me, it intuitively makes more sense for TelephonyHelper
> to be handling connections than CallButton or SIMPicker. We don't save any
> code because, at the end of the day, we still have to pass a card index into
> the telephony DOM API, so we have to carry around both the index and the
> connection. MozMobileConnection doesn't have an index on it, so we can't use
> that. If you still think we should do this, can you explain more of your
> reasoning?
I was wrong saying "let CallButton and SIMPicker collaborate to provide a connection to TelephonyHelper.". We should let CallButton and SIMPicker collaborate to provide a __serviceId__ to TelephonyHelper. Does that make more sense that way?

Comment 37

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

(In reply to Anthony Ricaud (:rik) from comment #36)
> I was wrong saying "let CallButton and SIMPicker collaborate to provide a
> connection to TelephonyHelper.". We should let CallButton and SIMPicker
> collaborate to provide a __serviceId__ to TelephonyHelper. Does that make
> more sense that way?

I chose to move this code to CallButton since we don't actually load SimPicker unless there is more than 1 card. I'll likely have to refactor this for the contacts and SMS apps, but we'll cross that bridge when we come to it, I guess.

Updated PR.

Comment 38

[Anthony Ricaud (:rik)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

This is looking very good! I've left a bunch of comments to address but they are mostly easy fixes or style conventions. There is still one architecture issue left with CallButton reading KeypadManager.phoneNumber.

I'll still have to test on a few phones for edge cases (emergency dialing for example).

Congrats on staying sane with this huge patch as a first Gaia feature!

Comment 39

[Anthony Ricaud (:rik)]

José: In your spec for the SIM Picker, we're changing the size of the buttons from 1.4rem to 1.7rem. Shouldn't a standard menu across the OS be consistent? Just checking if it was considered.

Comment 40

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

Updated PR.

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

I know we discussed on IRC about reusing OptionMenu but now that I understand better what we need, I really think it's not a good fit for this.

OptionMenu was intended to have several items to do really different actions (for example: delete a message, forward the message, etc). This is _not_ what we want to do here. It's already bigger than what I really want, and you're only adding more stuff in it. Soon it will just be an abstraction for the DOM. (I'm exagerating a bit here :) ).

So I really think we need to go the simpler way of doing a HTML-based menu, using building blocks. The "append" part would be simply a static HTML span element displayed by CSS (I can imagine the line having a "default" class, and therefore we could just do ".sim-picker > .item > span { display: none } .sim-picker > .item.default > span { display: initial }" or something similar.

Let's do this!

Comment 42

[Anthony Ricaud (:rik)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

We're missing a few tests but nothing bad.

Comment 43

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

I don't think we need review from Julien anymore since the only thing we're doing that has to do with the SMS app is refactoring SimSettingsHelper.

Comment 44

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #39)
> José: In your spec for the SIM Picker, we're changing the size of the
> buttons from 1.4rem to 1.7rem. Shouldn't a standard menu across the OS be
> consistent? Just checking if it was considered.

I'd also like to note that I think the standard 1.4rem size looks much nicer.

Comment 45

[Anthony Ricaud (:rik)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

Yeah, we're very very close.
We have a few adjustments to make the call button tests cleaner (I should have noted that earlier), one modification in the sim_picker code to make it safer and a few more tests for the sim_picker.

Comment 46

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

8th or so time's a charm, right?

Comment 47

[Anthony Ricaud (:rik)]

Comment on attachment 8382765 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685

We're good to go here. Please squash all the patches into one before landing.

Video capture of Doug tonight: http://i.imgur.com/TVKJjlA.gif

Comment 48

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/365889e2f4bb856bfc43458a1ee94db3b28b7e63

Comment 49

[Joe Cheng [:jcheng] (please needinfo)]

verifyme

Comment 50

[Enpei]

Hi Doug,

I build latest code but still send key in dialer still the old one, can you check again?
Gaia      ec28d9dcf57ca8da142f9f2fea33bc288b76ed59
Gecko     0bacffad36f65a27929ca85164792750c695c3cb
BuildID   20140313053055
Version   30.0a1

Comment 51

[Doug Sherk (:drs) (inactive)]

(In reply to Enpei from comment #50)
> Hi Doug,
> 
> I build latest code but still send key in dialer still the old one, can you
> check again?
> Gaia      ec28d9dcf57ca8da142f9f2fea33bc288b76ed59
> Gecko     0bacffad36f65a27929ca85164792750c695c3cb
> BuildID   20140313053055
> Version   30.0a1

It definitely works for me, Rik, and Francisco. What device are you on?

Comment 52

[Doug Sherk (:drs) (inactive)]

Oh, sorry, I see. You're actually 1 rev out of date. Please try again with the latest Gaia.

Comment 53

[Enpei]

Verified on Fugu
Gaia      b5d0f9b0cd5dce38b79287a9363226b912929270
Gecko     7312341e00e2785419fc4746e291646299ba3018
BuildID   20140313120331
Version   30.0a1

All acceptance criteria are fulfilled.

Hi Al, I didn't change status to Verified because in-moztrap is still "?". Could you help to confirm that then change the status?

Comment 54

[José Vittone]

(In reply to Anthony Ricaud (:rik) from comment #39)
> José: In your spec for the SIM Picker, we're changing the size of the
> buttons from 1.4rem to 1.7rem. Shouldn't a standard menu across the OS be
> consistent? Just checking if it was considered.

Sorry for the delay Anthony. You are right, the font size should be the same as what's been defined in BB. However, I'm talking with Arnau because it seems that it is not properly applied in BB.

Thanks!

Comment 55

[Al Tsai [:atsai]]

https://moztrap.mozilla.org/manage/case/11676/
https://moztrap.mozilla.org/manage/case/11766/

Comment 56

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

Also okay on this
Gaia      d2651c13d6370d62bf833b31c7e2e5f054510136
Gecko     c8a17e25111585c0eebb829f8208190ea432c71e
BuildID   20140318053055
Version   30.0a1

Comment 57

[Johan Lorenzo [:jlorenzo]]

Clearing the backlog tag as bug 1093585 was created.