APN settings should follow the new UX spec.
Assignee: nobody → arthur.chen
Status: NEW → ASSIGNED
Target Milestone: --- → 2.0 S6 (18july)
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!
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!
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+
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?
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.
Comment on attachment 8458026 [details] WIP Thanks Arthur :)
Attachment #8458026 - Flags: review?(ejchen) → review+
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  please.  https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/test/unit/carrier_test.js
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.
(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.)
(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 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.
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.
You need to log in before you can comment on or make changes to this bug.