[Settings] APN settings UX refresh

RESOLVED FIXED in 2.1 S3 (29aug)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: arthurcc, Assigned: arthurcc)

Tracking

unspecified
2.1 S3 (29aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.1)

Details

(Whiteboard: [p=8])

Attachments

(3 attachments)

APN settings should follow the new UX spec.
Assignee: nobody → arthur.chen
Status: NEW → ASSIGNED
Whiteboard: [p=8]
Target Milestone: --- → 2.0 S6 (18july)
Posted file WIP
feature-b2g: --- → 2.1
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
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!
Attachment #8458026 - Flags: feedback?(josea.olivera)
Attachment #8458026 - Flags: feedback?(ejchen)
Hello, please take a look at the latest APN spec. Tks!
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 !
Attachment #8458026 - Flags: feedback?(ejchen) → feedback+
Duplicate of this bug: 1026507
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
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!
Attachment #8458026 - Flags: review?(josea.olivera)
Attachment #8458026 - Flags: review?(gsvelto)
Attachment #8458026 - Flags: review?(ejchen)
Attachment #8458026 - Flags: feedback?(josea.olivera)
Comment on attachment 8458026 [details]
WIP

Jenny, could you help review the UI? Thanks.
Attachment #8458026 - Flags: ui-review?(jelee)
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?
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 on attachment 8458026 [details]
WIP

Hi Arthur, per discussion, please make change accordingly, thank you!
Attachment #8458026 - Flags: ui-review?(jelee) → ui-review+
Comment on attachment 8458026 [details]
WIP

OK, LGTM for the WAP Push part.
Attachment #8458026 - Flags: review?(gsvelto) → review+
Posted image 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.
Attachment #8473461 - Flags: ui-review?(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?
Flags: needinfo?(pabratowski)
Note that the highlight only appears when users click on the left part but not on the radio button.
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.
Flags: needinfo?(pabratowski)
Comment on attachment 8458026 [details]
WIP

Thanks Arthur :)
Attachment #8458026 - Flags: review?(ejchen) → review+
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
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!
Attachment #8473461 - Flags: ui-review?(hhuang) → ui-review+
(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 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
Attachment #8458026 - Flags: review?(josea.olivera)
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.
Attachment #8458026 - Flags: review?(josea.olivera)
(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 ;).
I think I got your point. Patch updated!

It seems we did not list all matched apns before, is that correct?
Confirmed with EPM/EM, and this can be landed before FL.
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.)
Flags: needinfo?(josea.olivera)
(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!
Flags: needinfo?(josea.olivera)
Comment on attachment 8458026 [details]
WIP

LGTM. r=me

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

Impressive work Arthur!
Attachment #8458026 - Flags: review?(josea.olivera) → review+
Jose, thank you so much for the great help!

master: 93a13e87ff78f96639a68a65a058a1c3002bbc11
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
> 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.
Flags: needinfo?(josea.olivera)
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
I've created a follow-up bug for the l10n issues. Please refer to bug 1060274.
Flags: needinfo?(josea.olivera)
You need to log in before you can comment on or make changes to this bug.