AC3 is removed as acceptance criteria

(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"?

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

Hi Jose, Looks nice. Thanks for the help!

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 ?

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

(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

features should not block release, remove blocking flag

Changing target milestone

Hi Wilfred, Could you update the AC since AC 3 is no longer valid? Thanks.

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

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

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.

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

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

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.

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

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

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.

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.

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.

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.

(In reply to Doug Sherk (:drs) from comment #29) > Created attachment 8382765 [details] > 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.

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

Filed bug 978300.

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

Comment on attachment 8382765 [details] 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 on attachment 8382765 [details] 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] > 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.

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 on attachment 8382765 [details] 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 on attachment 8382765 [details] 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!

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 on attachment 8382765 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685 Updated PR.

Comment on attachment 8382765 [details] 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 on attachment 8382765 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685 We're missing a few tests but nothing bad.

Comment on attachment 8382765 [details] 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.

(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 on attachment 8382765 [details] 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 on attachment 8382765 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16685 8th or so time's a charm, right?

Comment on attachment 8382765 [details] 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

verifyme

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

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

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

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?

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

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

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

Clearing the backlog tag as bug 1093585 was created.