932731 - [DSDS][Gaia][Sim Manager] add functionality to set outgoing call, outgoing messages and data

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

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

As title, add select sim functions.

Comment 1

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

In this bug, we can set outgoing call, outgoing messages and data from the select list

Comment 2

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

Attachment: WIP

Comment 3

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

Comment on attachment 825779 [details] [review]
WIP

Arthur, I updated a lot in SimCardManager. 

You can try on nightly to check their functions. (because our phone is not equipped with latest gecko API, you can't do too much on your phone) Please try it and give me some feedbacks to me.

By the way, because it is hard to split functions into different branches (code and html changes fast at development stage), in the end, I mixed all of them into this branch. 

That's all ! Big thanks to Arthur :P

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 825779 [details] [review]
WIP

EJ, thank you for the effort. Overall it looks great! I will add some comments in github later.

Comment 5

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

Comment on attachment 825779 [details] [review]
WIP

Hi Arthur,

I moved some changed into another patch and I think I can land this basic part first. Because Gecko API is not ready, I think the better way now is to hide the entry point to make sure there is no one will see this update.

Thanks :)

Comment 6

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

Currently it works on Fugu for outgoing call / outgoing messages now. 

But for outgoing data, we are still waiting for Gecko's patch to make it work.

Comment 7

[Kai-Chih Hu [:khu]]

Plan to land it in the 1st week of sprint 6.

Comment 8

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

Hi EJ,

I'm kinda confused. Why does this block bug 932730? Would you please check the bug dependency again and reset if if needed?

Comment 9

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #8)
> Hi EJ,
> 
> I'm kinda confused. Why does this block bug 932730? Would you please check
> the bug dependency again and reset if if needed?

yeah Hsin-Yi, there is no dependency on bug 932730, I will remove that. 

Thanks for telling me this !

Comment 10

[Kai-Chih Hu [:khu]]

Anshul said that he is going to do the testing tomorrow.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 825779 [details] [review]
WIP

Mark f? per comment 10. EJ, I will review this patch today, please also wait for Anshul's feedback before landing.

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 825779 [details] [review]
WIP

EJ, good work! Please check the comments I left in gihtub. And please also change the "else" coding style. Thanks!

Comment 13

[Anshul]

(In reply to Arthur Chen [:arthurcc] from comment #11)
> Comment on attachment 825779 [details] [review]
> WIP
> 
> Mark f? per comment 10. EJ, I will review this patch today, please also wait
> for Anshul's feedback before landing.
Arthur, the change seems to be working fine. The only issue I see is the icon for the SIM Manager, which shows up as airplane mode icon instead.

Comment 14

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

(In reply to Anshul from comment #13)
> (In reply to Arthur Chen [:arthurcc] from comment #11)
> > Comment on attachment 825779 [details] [review]
> > WIP
> > 
> > Mark f? per comment 10. EJ, I will review this patch today, please also wait
> > for Anshul's feedback before landing.
> Arthur, the change seems to be working fine. The only issue I see is the
> icon for the SIM Manager, which shows up as airplane mode icon instead.

Hi Anshul,

yeah you are right, when I am writing code, there is only UX spec and no any icons from UI. I just got notified that our UI released UI spec and needed images yesterday. I will try to use the new design after all functions work and got a positive feedback. 

Thanks for pointing out that issue.

Comment 15

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

ni? cawang for visuals. please redirect as needed. thanks

Comment 16

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

(In reply to Joe Cheng [:jcheng] from comment #15)
> ni? cawang for visuals. please redirect as needed. thanks

ni? Fang since she's the visual designer for DSDS. Thanks!

Comment 17

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

icons already provided. clear ni?

Comment 18

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

Comment on attachment 825779 [details] [review]
WIP

Hi Arthur, 

please help me review the code again. 

Thanks for your great help.

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 825779 [details] [review]
WIP

Thank you for the patch, EJ! There are a few comments I would like you address before merging. Please check github.

Comment 20

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

Comment on attachment 825779 [details] [review]
WIP

thanks, Arthur. 

I will go get my fugu to make sure these changes not break anything on device.

thanks for your great help again.

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 825779 [details] [review]
WIP

Good job! r=me. Please squash the commits and make sure travis is green before merging. Thank you for the great help!

Comment 22

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

thanks Arthur for your great help. 

landed on gaia/master : 9e0df13811b577026b70ccfdcd9b4a309746e8ba

Comment 23

[Julien Wajsberg [:julienw]]

This removed the "configure" activity handler.

reverted in master: 663a826061e1b47fdfb006733c21fa069b855759

Also, please put the bug number in the commit log ;)

Comment 24

[Julien Wajsberg [:julienw]]

Hey Zac,

is it possible to have an integration test covering the configure activity? There are several ways:
* in the notification panel, tap the "wifi" icon
* in the SMS app, tap "new message", tap "settings" icon, tap "settings" item.

Thanks!

Comment 25

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

(In reply to Julien Wajsberg [:julienw] from comment #23)
> This removed the "configure" activity handler.
> 
> reverted in master: 663a826061e1b47fdfb006733c21fa069b855759
> 
> Also, please put the bug number in the commit log ;)

Thanks for pointing out the missing lines, Julien. 

I just re-landed that back to gaia master on 1d82b5f63db21d049de10ff9de3215d916a0c2a7.

Comment 27

[Francesco Lodolo [:flod]]

Is there a reason for this difference (call singular, messages plural)

sim-manager-outgoing-call=Outgoing call
sIm-manager-outgoing-call-desc=Calls will be sent from
sim-manager-outgoing-messages=Outgoing messages
sim-manager-outgoing-messages-desc=SMS will be sent from

Not sure also if we're case sensitive on l10n IDs (second string: in the code there's sim-, in the .properties file sIm-)

Comment 28

[Zac C (:zac)]

Hey JulienW, sorry i missed that.
The first one is already in the works.

The second one I will file a task for!

Comment 29

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

(In reply to Francesco Lodolo [:flod] from comment #27)
> Is there a reason for this difference (call singular, messages plural)
> 
> sim-manager-outgoing-call=Outgoing call
> sIm-manager-outgoing-call-desc=Calls will be sent from
> sim-manager-outgoing-messages=Outgoing messages
> sim-manager-outgoing-messages-desc=SMS will be sent from
> 
> Not sure also if we're case sensitive on l10n IDs (second string: in the
> code there's sim-, in the .properties file sIm-)

There is no specific reason for singular and plural. 

And i didn't notice that I put sIm in this locale file, thanks for pointing out this, because right now i am working on another bug and is related to simcard manager. I will give it fix at the same time. 

Thanks Francesco.

Comment 30

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

(In reply to Zac C (:zac) from comment #28)
> Hey JulienW, sorry i missed that.
> The first one is already in the works.
> 
> The second one I will file a task for!

Thanks Zac.

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I kind of suspect this PR to have created the regression on Settings launch because of https://github.com/mozilla-b2g/gaia/commit/1d82b5f63db21d049de10ff9de3215d916a0c2a7#diff-f36e65bf103d8ff227c8349e0411b107R258 line 258. 

When accessing mozMobileConnection at startup it calls some expensive backend stuff. If this PR is the suspect you may consider changing the way you display the stuff, by showing the UI disabled by default for example, and delaying a little bit the check to turn it on or off once the app is already started (like many other things in settings).

needinfo? bkelly so he knows where to look for the regression (at least this PR is my suspect, maybe I'm wrong).

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment 31 is correct.  This commit caused a settings app launch regression.  I did the following with and without the commit:

  sudo pip install b2gperf
  adb forward tcp:2828 tcp:2828
  b2gperf --delay=10 Settings

The numbers I got:

  With commit 1d82b5f:  median:848, mean:854, std: 14, max:896, min:840
  Reverted commit:      median:726, mean:731, std: 21, max:816, min:709

I'm sorry, but I think we need to back this out on master.  Please reland after implementing a lazy load strategy as suggested in comment 31.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Backed out on master:  a45dd9fe5ba3f5e5bd04fb6c32df4fcb006d9982

Note, I also had to back out bug 930222 as it blocked the back out of this bug.

Comment 34

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

Attachment: add lazyload

This is a patch with lazyload so that we can open settings app quicker.

Comment 35

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

(In reply to Ben Kelly [:bkelly] from comment #33)
> Backed out on master:  a45dd9fe5ba3f5e5bd04fb6c32df4fcb006d9982
> 
> Note, I also had to back out bug 930222 as it blocked the back out of this
> bug.

Thanks Ben & Vivien for pointing out this, but can you file a bug next time if you think there is any performance issue ? 

We are already working on this performance issue on another bug.

It is not healthy when waking up in the morning and found the patch (with another unrelated one) were backed out accidentally at night without notifying.  -_-

Thanks for you guys' help again.

Comment 36

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

Tested on Unagi.

// before patch
2013-12-04 11:21:08 | B2GPerfRunner | INFO | Results for Settings, cold_load_time: median:796, mean:805, std: 30, max:943, min:779, all:852,798,943,800,791,790,799,788,805,847,801,792,788,812,794,791,820,796,789,796,820,792,802,835,790,789,791,779,785,803

// after patch 
2013-12-04 10:44:46 | B2GPerfRunner | INFO | Results for Settings, cold_load_time: median:669, mean:680, std: 35, max:833, min:651, all:744,656,705,833,686,674,658,662,725,671,665,688,657,664,667,670,674,665,651,711,672,673,656,663,659,661,660,668,694,676

Comment 37

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8342111 [details] [review]
add lazyload

r=me. Thanks EJ!

Comment 38

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

Thanks all , merged on master/gaia : ff0c4028b7ec88cea1394d7267771b0d1f554373

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Thanks guys!  The new patch improved load time even further than it was before.

(In reply to EJ Chen [:eragonj] from comment #35)
> Thanks Ben & Vivien for pointing out this, but can you file a bug next time
> if you think there is any performance issue ? 
> 
> We are already working on this performance issue on another bug.
> 
> It is not healthy when waking up in the morning and found the patch (with
> another unrelated one) were backed out accidentally at night without
> notifying.  -_-

In regards to the back out, I believe its project policy to treat large performance regressions the same as breaking tests.  Sheriffs back them out in order to keep the project running smoothly.  I know this sucks, but its worse for everyone overall if we leave master in a bad state.  In the future we plan to have better infrastructure to help people catch regressions before landing.

Also, I did try to reach out to you over IRC, but I think our time zones are quite different unfortunately.

Dietrich, does this sound correct to you?

Comment 40

[Dietrich Ayala (:dietrich)]

Absolutely. It is always better to pro-actively backout. If there's perf problem the patch should not have landed in the first place.

EJ: project-wide we need to think of backouts as awesome dedication to making sure we're always fast :D

Comment 41

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

Yeah, I will try to monitor performances change for later development to avoid this kind of situation happen. 

And for sure, timezone sucks haha.

Thanks for you guys' help again.