1398283 - General clean-up of Sync Preference screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, enhancement)

Comment 1

[:Grisha Kruglov]

"Synced: enabled" seems redundant, or broken, or both. The only way to "disable" sync, other than disconnecting, is to toggle it off under System account preferences, but that doesn't change this header. Perhaps it should? Either way, it's used incorrectly as a header.

"singed in as" header is clearly redundant.

Comment 2

[:Grisha Kruglov]

These labels show up as titles of two PreferenceCategories. Removing the title strings leaves large blank spaces where the titles were supposed to be. Perhaps that could be solved with custom styles?

We use these categories as a way to group items, and then to be able to append/remove error states from the defined groups. I think the easier approach is to redefine how we display our error states. I'm thinking that moving error states to the top to a new PreferenceCategory which we toggle on/off, something akin to https://storage.googleapis.com/material-design/publish/material_v_12/assets/0Bzhp5Z4wHba3dEZTUF9idzBHMWc/patterns-errors-userinput19.png will allow us to remove the groupings entirely, and remove these headers.

Comment 3

[:Grisha Kruglov]

This is turning into a "cleanup sync pref screen" bug. Changes being made so far:
- re-organize preference grouping, removing two general PreferenceCategories we had prior
- move error states to a separate category, shown at the top as a banner
- move custom auth/sync server url configurations into a new "Additional Settings" category
- remove More...
- remove Manage account
- make tapping on account name/email/avatar lead to "manage account", as opposed to "change avatar" as before

Comment 4

[:Grisha Kruglov]

Attachment: Screenshot_1505518013.png

Regular view after cleanup.

Comment 5

[:Grisha Kruglov]

Attachment: Screenshot_1505518020.png

Additional Settings section

Comment 6

[:Grisha Kruglov]

Attachment: Screenshot_1505518745.png

Error banner

Comment 7

[:Grisha Kruglov]

I'm not happy about the margins in Comment 6, but we can address that in Bug 1398288.

Comment 8

[:Grisha Kruglov]

Attachment: Bug 1398283 - Clean up Sync Preferences screen

Review commit: https://reviewboard.mozilla.org/r/180508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180508/

Comment 9

[:Grisha Kruglov]

https://bugzilla.mozilla.org/show_bug.cgi?id=1227954#c3 provides some context for current redundancy on Sync Preferences screen (two ways to open about:accounts). Seems that I've settled on something very similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1227954#c4.

Comment 11

[:Grisha Kruglov]

More TODO:
- add "Choose what to sync" header above list of data types
- use switches instead of checkboxes

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to :Grisha Kruglov from comment #11)
> More TODO:
> - add "Choose what to sync" header above list of data types
> - use switches instead of checkboxes

At the time I built this, switches were not the Android standard (I think they were HC+?), and AppCompat wasn't really available to this code.  In fact, this whole separate Activity was to work around significant changes in the Preferences APIs Android exposed.  Neither of those constraints (AppCompat, old Preferences API) holds true anymore.

So I'll review the code from a technical standpoint, but I'd really rather see all of this get moved into Fennec's settings screens, removing resources from the Services code base and unifying an ugly divide in the settings UI that has outlived its usefulness.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8908883 [details]
Bug 1398283 - Clean up Sync Preferences screen

https://reviewboard.mozilla.org/r/180508/#review186216

I didn't look too closely at this, but the approach is fine by me, and the details seems fine too.

Can you land the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1375571#c4 as a Pre: to this?

Thanks!

Comment 14

[:Grisha Kruglov]

Attachment: Screenshot_1505776845.png

I'm not sure that the "Choose what to sync" header actually adds any value, but I don't mind it either. Ryan, yay/nay?

Comment 15

[:Grisha Kruglov]

Attachment: Bug 1398283 - Pre: Remove unused fxaccount_remote_error_* strings.

This is follow-up to Bug 1220892, which just forgot to remove these strings.

Review commit: https://reviewboard.mozilla.org/r/181010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181010/

Comment 16

[:Grisha Kruglov]

Comment on attachment 8908883 [details]
Bug 1398283 - Clean up Sync Preferences screen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180508/diff/1-2/

Comment 17

[:Grisha Kruglov]

Comment on attachment 8909542 [details]
Bug 1398283 - Pre: Remove unused fxaccount_remote_error_* strings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181010/diff/1-2/

Comment 18

[:Grisha Kruglov]

Comment on attachment 8908883 [details]
Bug 1398283 - Clean up Sync Preferences screen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180508/diff/2-3/

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8909542 [details]
Bug 1398283 - Pre: Remove unused fxaccount_remote_error_* strings.

https://reviewboard.mozilla.org/r/181010/#review186588

Stamp.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3964204faf6
Pre: Remove unused fxaccount_remote_error_* strings. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/0408636c7ea9
Clean up Sync Preferences screen r=nalexander

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing Android lint:

https://hg.mozilla.org/integration/autoland/rev/3f1a3f52cca8a049ab9c15ccb3509e0ea750e664
https://hg.mozilla.org/integration/autoland/rev/ca289939685261cb52551eb87dad5fa7d0b2c067

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0408636c7ea903cc3fa50da56cf5bfcd432421c5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132031356&repo=autoland

Comment 22

[:Grisha Kruglov]

Comment on attachment 8908883 [details]
Bug 1398283 - Clean up Sync Preferences screen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180508/diff/3-4/

Comment 23

[:Grisha Kruglov]

All right, let's try this again.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d7feb1d2adc
Pre: Remove unused fxaccount_remote_error_* strings. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/9c622588634e
Clean up Sync Preferences screen r=nalexander

Comment 25

[Ryan Feeley [:rfeeley]]

Re: Choose what to sync header… It needs a horizontal line above it (and none between the datatypes)… I'll check the Material design examples and see if that's right.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4d7feb1d2adc
https://hg.mozilla.org/mozilla-central/rev/9c622588634e

Comment 27

[Sorina Florean [:sflorean]]

Verified as fixed on latest Nightly build with Huawei Honor (Android 5.1.1), Honor 8 (Android 7.0) and Asus ZenPad 8(Android 6.0.1).