1032629 - [Settings] APN settings UX refresh

Status: RESOLVED FIXED

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

Description

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

APN settings should follow the new UX spec.

Comment 1

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

Attachment: WIP

Comment 2

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

Comment on attachment 8458026 [details]
WIP

Hi Jose and EJ, the patch is almost ready. Before I dive into writing tests, I would like some feedback from you. The patch is a bit complicated but I've tried to separate it to stand alone modules as much as I can.

I would suggest to start from the four modules:
- ApnList
  It wraps user added apn items and which are stored in the async storage.
- ApnSelections
  It records the id of the apn item used for each apn type.
- ApnSettings
  It wraps the value under ril.data.apnSettings.
- ApnSettingsManager
  It operates the above three modules and provides functions to apn settings panels. It is also responsible for restoring default apn lists and recovering apn items based on the value of ril.data.apnSettings (we need this because sometimes the value of ril.data.apnSettings gets changed in other apps).

Let me know if there is any problem, thanks a lot!

Comment 3

[Jenny Lee]

Attachment: [2.1 Settings] APN Settings v1.4.pdf

Hello, please take a look at the latest APN spec. Tks!

Comment 4

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

Comment on attachment 8458026 [details]
WIP

Arthur, I already did the first round of feedback but because I am not so familiar with APN, I may need your help to go through some parts.

Thanks !

Comment 6

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

Comment on attachment 8458026 [details]
WIP

Hi EJ, I've addressed your comments and add tests. Would you mind check the patch again?

Gabriele, Jose, the patch also modified system app and wap push app. For the system app part, it stores the default apn items to the settings db, so we don't need to load the entire apn.json file.

For the wap push app, it clears the apn selection when receiving CP messages.

Thanks for helping!

Comment 7

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

Comment on attachment 8458026 [details]
WIP

Jenny, could you help review the UI? Thanks.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8458026 [details]
WIP

I'm fine with the change to the wappush app but from what I can see this is clearing the APN selection when the user stores the CP message, not when it is received. Is that the intended behavior?

Comment 9

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

Yes, it is the intended behavior. Clearing the APN selection is performed together with applying new apn settings to ril.apn.apnSettings. I was mixed with the timing of receiving and storing CP messages.

Comment 10

[Jenny Lee]

Comment on attachment 8458026 [details]
WIP

Hi Arthur, per discussion, please make change accordingly, thank you!

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8458026 [details]
WIP

OK, LGTM for the WAP Push part.

Comment 12

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

Attachment: APN list UI

Hi Helen, could you help review the UI, especially for the separator and highlighting as they have never been introduced before. Thanks.

Comment 13

[Helen Huang [:HHuang]]

I think it relates control component, so I'm going to needinfo Przemek to ask for the feedback.

Hi Przemek, can you take a look the screenshot on Comment 12? Looks like it added a divider in the list that caused the highlight be divided to 2 parts, what do you think?

Comment 14

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

Note that the highlight only appears when users click on the left part but not on the radio button.

Comment 15

[Przemek Abratowski [:przemek] UX]

I feel the highlight can still cover both the text area and the radio button area, even though the radio button control is not effected. Right now I don't like the way the highlight is cut off. I feel the divider line is enough to hint at the division between the text and the radio button before the press.

Comment 16

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

Comment on attachment 8458026 [details]
WIP

Thanks Arthur :)

Comment 17

[Helen Huang [:HHuang]]

Comment on attachment 8473461 [details]
APN list UI

As offline discussion with Przemek, it's okay to keep going this way.
Thanks for the implement!

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] from comment #6)
> Gabriele, Jose, the patch also modified system app and wap push app. For the
> system app part, it stores the default apn items to the settings db, so we
> don't need to load the entire apn.json file.

Looking really good. Much better than the spaghetti code we had, awesome work Arthur. Keep the review request at me until I finish reviewing this (hopefully tomorrow).

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8458026 [details]
WIP

Left a few comments/concerns in the PR. They are mostly related to the APNs added through client provisioning messages. Please, have a look at them and request review at me once you address them. Feel free to ping me on IRC if you need further explanation.

Really good work here Arthur, thanks!

PS. Rebase the patch as an important patch about OMA CP for DSDS landed a few days ago and clean the tests at [1] please.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/test/unit/carrier_test.js

Comment 20

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

Comment on attachment 8458026 [details]
WIP

Thanks for reviewing! Please check my response in github.

The updated patch takes `_id` into consideration and replaces `mobileCode` with `operatorCode` as suggested. carrier_test.js and related files have also been removed. Would you mind take a look at it again? Thanks.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] from comment #20)
> Comment on attachment 8458026 [details]
> WIP
> 
> Thanks for reviewing! Please check my response in github.

Thanks for addressing the issues you commented earlier. There is still one. Let's discuss tomorrow on IRC as it seems I wasn't myself clear enough ;).

Comment 22

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

I think I got your point. Patch updated!

It seems we did not list all matched apns before, is that correct?

Comment 23

[Kai-Chih Hu [:khu]]

Confirmed with EPM/EM, and this can be landed before FL.

Comment 24

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

Hi Jose, I've addressed the issue by using all apn items from apn.json. Would you mind check it again? (The changes are in the last commit.)

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Arthur Chen [:arthurcc] from comment #24)
> Hi Jose, I've addressed the issue by using all apn items from apn.json.
> Would you mind check it again? (The changes are in the last commit.)

Update: Arthur and myself are working on reviewing this together.

Arthur, ping me on IRC when you go online. Thanks!

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8458026 [details]
WIP

LGTM. r=me

Rebase, squash, wait for a green Try and land at will please.

Impressive work Arthur!

Comment 27

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

Jose, thank you so much for the great help!

master: 93a13e87ff78f96639a68a65a058a1c3002bbc11

Comment 28

[Francesco Lodolo [:flod]]

> add-apn = Add New Apn

This should be APN (by specs and logic), no need to update the string ID.

But the main problem is that you reused the same string (reset-apn) for a label and a title, which is not OK and needs to be fixed before FL.

Comment 29

[Francesco Lodolo [:flod]]

Found another issue. Not sure how else I can ask to test patches with pseudolocales...

https://github.com/mozilla-b2g/gaia/blob/93a13e87ff78f96639a68a65a058a1c3002bbc11/apps/settings/elements/apn_editor.html#L56

The string is called apn-type

Found out another issue unrelated to this patch, worth fixing
https://github.com/mozilla-b2g/gaia/blob/93a13e87ff78f96639a68a65a058a1c3002bbc11/apps/settings/elements/apn_editor.html#L66

The key is called apn-protocol-ipv4v6

Comment 30

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

I've created a follow-up bug for the l10n issues. Please refer to bug 1060274.