947140 - [DSDS][Message] Long tap to send message

Status: VERIFIED FIXED

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

Description

[Wilfred Mathanaraj [:WDM]]

As a user when I long tap the button "Send" in the Messaging App I want to be prompted in order to choose from which SIM the message must be sent

Acceptance Criteria

AC1: the default SIM is highlighted on the prompt
AC2: I want to have a "remember my choice" option
AC3: If I select a different SIM and and select "remember my choice" I want the default option in settings to change.

Comment 1

[Julien Wajsberg [:julienw]]

Wilfred, are AC2 and AC3 still accurate?

Comment 2

[José Vittone]

Attachment: DSDS.sms_0000_DSDS-1-line.png

Visuals and visual specs related to DSDS.

Comment 3

[José Vittone]

Attachment: DSDS.sms_0003_DSDS-multiple-lines.png

Comment 4

[José Vittone]

Attachment: DSDS.SMS-MMS.sim_picker.png

Comment 5

[José Vittone]

Attachment: DSDS.SMS-MMS.Inbox.png

Comment 6

[José Vittone]

Attachment: DSDS.SMS-MMS.message_time.png

Comment 7

[José Vittone]

Attachment: icon-long-tap-indicator.zip

Comment 8

[José Vittone]

Attachment: SPEC_DSDS.SMS.compose.png

Comment 9

[José Vittone]

Attachment: SPEC_DSDS.SMS.inbox.png

Comment 10

[José Vittone]

Attachment: SPEC_DSDS.SMS.sim_picker.png

Comment 11

[José Vittone]

Attachment: SPEC_DSDS.SMS.thread.png

Comment 12

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

features should not block release, remove blocking flag

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: FFOS_MessagesAppCommsApp_DSDS_V1.4_20140128_V3.0.pdf

Ref for AC2: page 7, page 8

(AC1 is bug 947139)

Comment 14

[Doug Sherk (:drs) (inactive)]

I'm taking this because it'll be awkward for me not to since I'm working on refactors to support this.

Comment 15

[Doug Sherk (:drs) (inactive)]

every. time.

Comment 16

[Julien Wajsberg [:julienw]]

Doug, please see my patch in bug 947180 (and you can provide comments on the PR if you want too), as I started some basic refactoring and some new APIs too.

Comment 17

[Doug Sherk (:drs) (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Doug, please see my patch in bug 947180 (and you can provide comments on the
> PR if you want too), as I started some basic refactoring and some new APIs
> too.

We talked about this a bit on IRC. Julien pointed me to the SMS app's OptionMenu, which sort of does what we want here. I was initially supportive of refactoring it and using it for the SIM selection menus across apps, but on further thought, I'm not anymore. OptionMenu works entirely dynamically which forces us to add view code to our application/JS logic, and it's slower since a good chunk of the SIM menus can be created statically. I would still like to refactor it and augment it, but I think that falls out of the current scope of this bug and bug 946866.

Comment 18

[Julien Wajsberg [:julienw]]

Yep, I would agree on a OptionMenu refactoring but we can do this with the same API so in a separate patch.

Comment 19

[Doug Sherk (:drs) (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Yep, I would agree on a OptionMenu refactoring but we can do this with the
> same API so in a separate patch.

Sorry, I don't understand what you're saying here. My plan is to refactor the code I wrote for the dialer, use that for the SMS app too, then we can refactor and combine OptionMenu with my API later. Does that sound OK to you?

Comment 20

[Julien Wajsberg [:julienw]]

I was saying that in SMS we could just use the existing OptionMenu. Later on, we can refactor OptionMenu, keeping the same API, and so this could belong to another bug/patch.

Your plan works for me too, but I wonder if this makes much sense to share such a simple screen. Note that SMS already have methods to access the the SIM information that we use in other places too.

Comment 21

[Julien Wajsberg [:julienw]]

Carrie, Ayman, one question here. Remember that when we press "Download" for a MMS that needs to switch the SIM, we have a confirmation dialog, because switching the MMS default SIM will also switch the default SIM for Data, since it's the same configuration.

Do we need to have the same confirmation dialog when we choose the non-default SIM when we longpress to send a MMS?

Comment 22

[Doug Sherk (:drs) (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> I was saying that in SMS we could just use the existing OptionMenu. Later
> on, we can refactor OptionMenu, keeping the same API, and so this could
> belong to another bug/patch.
> 
> Your plan works for me too, but I wonder if this makes much sense to share
> such a simple screen. Note that SMS already have methods to access the the
> SIM information that we use in other places too.

Yeah, I see what you're saying. Check out https://github.com/DouglasSherk/gaia/blob/f3cbaa04815dcbe9865788913b0f08344810424b/shared/js/sim_menu_helper.js to see how much logic there is. I think even if we use OptionMenu, there will be a significant amount of duplication of code unless we factor out the menu generation code as well.

Comment 23

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

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Carrie, Ayman, one question here. Remember that when we press "Download" for
> a MMS that needs to switch the SIM, we have a confirmation dialog, because
> switching the MMS default SIM will also switch the default SIM for Data,
> since it's the same configuration.
> 
> Do we need to have the same confirmation dialog when we choose the
> non-default SIM when we longpress to send a MMS?

For this question, I think it's related to bug 947139.
https://bugzilla.mozilla.org/show_bug.cgi?id=947139
If we reach a conclusion there, then we can answer it here. Thanks!

Comment 24

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

from a separate email, this is targeted to be done by end of Sprint 3

Comment 25

[ayman maat :maat]

removing ni to me? Carrie is covering this in comment 23

Comment 26

[Doug Sherk (:drs) (inactive)]

Francisco, can you take this? I'll be doing refactors in bug 947189 to support this, so the work here will mostly be just getting it working with SMS.

Comment 27

[Julien Wajsberg [:julienw]]

Will need to integrate with the work from bug 947139 as well.

I think I can have a first look on this bug today or tomorrow, but I definitely won't have time to finish it :(

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

I'll take a look if Julien cannot finish it.

Comment 29

[Doug Sherk (:drs) (inactive)]

I'm taking this proper.

Comment 30

[Julien Wajsberg [:julienw]]

I'm also having a first look so that we can discuss about it later today.

Comment 31

[Doug Sherk (:drs) (inactive)]

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

Comment 32

[Anthony Ricaud (:rik)]

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

I left some comments pretty fast, I may have stupid things so take them with some caution.

Also, be careful with the API changes since we landed a few patches that use SimPicker/CallButton. So check if you have modified all call sites.

Comment 33

[Julien Wajsberg [:julienw]]

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

r=me for the SMS + refactoring part, with the additional commits we discussed on IRC.

I'd like francisco to have a look to the contacts part though, so adding the review flag for him.

Thanks for this hard work!

Comment 34

[Paul Yang [:pyang] (away)]

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2535&filter-productversion=164
Leave sim card issue as expected; will prepare test cases for this later.

Comment 35

[Doug Sherk (:drs) (inactive)]

Francisco: Once we get your review, I think we'll be good to go. I updated the PR so it should be passing all tests other than the one that was failing due to problems in bug 947189. I still have to rebase, though.

Comment 36

[Francisco Jordano [:arcturus] [:francisco]]

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

Excellent job!

Thanks for adding the suggestions, also tried on the phone and works for me.

Again, Doug you nailed it, amazing job!

Comment 37

[Doug Sherk (:drs) (inactive)]

\o/ Thanks, Francisco!

We just need to land 947189 first now.

Comment 38

[Anthony Ricaud (:rik)]

Bug 947189 has landed but we need to address everything in bug 947189 comment 34.

Also, voicemail and calling from call log are using SIM Picker directly so we need to wrap those calls in LazyL10n.get.

Comment 39

[Anthony Ricaud (:rik)]

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

This looks good but we need to new tests.

Also, I'd like to test if we can import the HTML of the SIM Picker instead of "copying" it.

Comment 40

[Anthony Ricaud (:rik)]

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

\o/ Finished just in time: http://ljdchost.com/8cimjld.gif

Comment 42

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/b34d6784c1a3e8995f3f9a361ca7e93969d38600

Comment 43

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

Works fine with this
Gaia      d2651c13d6370d62bf833b31c7e2e5f054510136
Gecko     c8a17e25111585c0eebb829f8208190ea432c71e
BuildID   20140318053055
Version   30.0a1