872329 - Apply existing settings to proposed Settings UI rework hierarchy

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Chenxia Liu [:liuche]]

Fit the existing settings into the proposed hierarchy in https://etherpad.mozilla.org/settings-reorg

Comment 1

[Chenxia Liu [:liuche]]

Ian, here's a straight lift of settings into the new settings layout. Let me know what you think - some of the preference screens are a little sparse, because we don't have some of the features. Also reusing strings at the moment (see etherpad for some notes), so let me know if you have specific strings in mind already.

Works for tablet, v11+ phones, and pre-v11 phones (I need to fix the transitions on old phones so it's not so jerky).

Comment 2

[Chenxia Liu [:liuche]]

Derp, forgot to include link: http://people.mozilla.com/~liuche/bug-872329/fennec.apk

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Part 1: New strings.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Part 2: resource xmls and hooking into GeckoPreferences

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Part 3: Remove transistions for GeckoPreferences

Ugly transitions in pre-v11 phones.

Comment 6

[Chenxia Liu [:liuche]]

Talked to ibarlow in person about some changes to the settings reorganization: two proposed versions are on the updated etherpad, saved as revision 1 (https://etherpad.mozilla.org/settings-reorg)

I'm making two apks to reflect to the different settings hierarchies.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Early feedback: I don't think "Show product announcements [ ]" belongs in "Privacy". The rest of "Privacy" is focused on what happens when using web content. "Show product announcements [ ]" seems more like Telemetry/FHR/CrashReporter. Maybe.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Early feedback: I don't think "Show product announcements [ ]" belongs in
> "Privacy". The rest of "Privacy" is focused on what happens when using web
> content. "Show product announcements [ ]" seems more like
> Telemetry/FHR/CrashReporter. Maybe.

Do not block on this. Keep moving ahead.

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Early feedback: I don't think "Show product announcements [ ]" belongs in
> "Privacy". The rest of "Privacy" is focused on what happens when using web
> content. "Show product announcements [ ]" seems more like
> Telemetry/FHR/CrashReporter. Maybe.

Yeah, looking at this again I think I agree with that.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Privacy - no top category (ICS)

Screenshot of privacy screen with no top privacy category ("General").

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Original categories layout (A) (froyo)

This screenshot is kind of weird-looking because categories do not automatically have a bottom cut-off, so it kind of looks like "Customize" spans more than we intended in the mock.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Faked categories layout (A) (froyo)

This screenshot displays the "faked" category in Froyo, basically inserting a category w/o a title.

The v3-layout-A.apk uses this (though there is extra space in ICS, that you'll see when you try it out).

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Rearranged categories layout (A) (ICS)

This is another alternative layout for A w/o the faked categories, where we rearrange the nested screens above the category.

Comment 14

[Chenxia Liu [:liuche]]

Ian, the test apks (v3-layout*) I promised you are here: http://people.mozilla.com/~liuche/bug-872329/

A and B are from our mocks, and C is another mock I threw together with a "Vendor" screen, that collects data collection and Mozilla info (see etherpad for more details).

The other screenshots are alternatives that I went through, with screenshots so you have an idea of what they look like.

One note: launching preferences from the data notification works, but I need to make some tweaks to get the scrolling right. Just scroll down manually to the telemetry stuff and pretend that it was done automatically :)

Let me know what you think.

Comment 15

[Ian Barlow (:ibarlow)]

Thanks for these options, Chenxia!

I quite like what you did with option C. Using a "Mozilla" section to roll up the about, FAQs, Feedback and data sharing stuff makes sense to me. 

My only feedback so far is that exposing the "customize" items on the first page still feels a little awkward. Like we discussed last week, it's nice to have options on that top level, but given that these are things that you would likely only turn on or off once it might not make sense to promote them like this. Maybe for now the top level should just be titles that you have to drill down into:

Sync
---
Display
---
Customize
---
Privacy
---
Mozilla

With these last changes, I would be happy to land this into Nightly to get some wider feedback. 

From here, we can use follow-ups to work on polishing strings / adding descriptions as you pointed out in the etherpad.

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Part 1: new strings + grouping unused ones

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 2: v4 layout xmls

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Screenshot: settings categories top level

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 1: new strings + grouping unused ones v1

(added commit message)

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Part 2: layout xmls v1

Comment 21

[Chenxia Liu [:liuche]]

Going to file a follow-up for auto-scrolling to data reporting so that this can get landed and final strings can be decided.

Comment 22

[Chenxia Liu [:liuche]]

Attachment: Screenshot: "Mozilla" category

Ian, just a heads up with the data reporting category. I thought it would be good to separate the "data collection" from the "info" stuff.

Comment 23

[Ian Barlow (:ibarlow)]

YEP(In reply to Chenxia Liu [:liuche] from comment #22)
> Created attachment 756661 [details]
> Screenshot: "Mozilla" category
> 
> Ian, just a heads up with the data reporting category. I thought it would be
> good to separate the "data collection" from the "info" stuff.

You read my mind :)

Comment 24

[Chenxia Liu [:liuche]]

Updated apk for anyone who wants to see the flow: http://people.mozilla.com/~liuche/bug-872329/v4.apk

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 756652 [details] [diff] [review]
Part 1: new strings + grouping unused ones v1

Review of attachment 756652 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +66,4 @@
>  
>  <!ENTITY settings "Settings">
>  <!ENTITY settings_title "Settings">
> +<!-- NEW CATEGORY STRINGS -->

Probably don't want the word "new" in here, as it will quickly become old. I don't think we need this header really.

::: mobile/android/base/strings.xml.in
@@ +78,5 @@
>  
>    <string name="settings">&settings;</string>
>    <string name="settings_title">&settings_title;</string>
> +
> +  <!-- NEW CATEGORY STRINGS -->

Same

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 756654 [details] [diff] [review]
Part 2: layout xmls v1

Review of attachment 756654 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I'm sure this breaks some tests, so you should run it through try and fix 'em before you land.

::: mobile/android/base/resources/xml-v11/preferences_customize_tablet.xml
@@ +11,5 @@
> +                  android:title="@string/pref_category_customize"
> +                  android:enabled="false">
> +
> +    <org.mozilla.gecko.SyncPreference android:title="@string/pref_sync"
> +                                      android:persistent="false" />

Add a note about this being different between phone tablet. Can we do that throughout to help simplify anyone trying to figure it out?

Comment 27

[Chenxia Liu [:liuche]]

Attachment: Part 1: new strings + grouping unused ones v2

Comment 28

[Chenxia Liu [:liuche]]

Attachment: Part 2: layout xmls v2

Added clarification comments in xml resource files.

Tests patch will follow.

Comment 29

[Chenxia Liu [:liuche]]

Attachment: Part 2: layout xmls v3

Used incorrect default resource file for tablet.

Comment 30

[Chenxia Liu [:liuche]]

Attachment: Part 3: tests!

Re-worked and re-enabled the settings tests; also fixed other broken tests. Running everything on try atm because robocop: https://tbpl.mozilla.org/?tree=Try&rev=9789c2314bbe

Comment 31

[Chenxia Liu [:liuche]]

Attachment: Part 2: layout xmls v3

Use correct resource for tablet Customize fragment on settings launch.

Comment 32

[Chenxia Liu [:liuche]]

Attachment: Part 3: tests v2

Re-enable settings tests, fix a keyboard dismiss that fail on devices with hardware keyboard, clean up some other tests, etc.

WIP because still need to handle bug 882191.

Comment 33

[Chenxia Liu [:liuche]]

Attachment: Part 2: layout xmls v4

Bitrot caused by landing bug 873072

Comment 34

[Chenxia Liu [:liuche]]

Comment on attachment 761559 [details] [diff] [review]
Part 2: layout xmls v3

Mis-click in obsoleting patches.

Comment 35

[Chenxia Liu [:liuche]]

Attachment: Part 1: new strings + grouping unused ones v3

Un-bitrotted. This keeps the old strings so we won't have to risk re-localizing them in case we want to go back to any of them. Bug 877791 tracks removing old strings when we've finalized them.

Comment 36

[Chenxia Liu [:liuche]]

Attachment: Part 2: layout xmls v5

Un-bitrot.

Comment 37

[Chenxia Liu [:liuche]]

Attachment: Part 3: tests v3

Quick look-over from you, Geoff, since some of this is from your patch in bug 882191. I removed the Privacy/Privacy & Settings duality because that was a bug on my part - they should all use the same strings.

Comment 38

[Chenxia Liu [:liuche]]

Try build with test fix from bug 882191 applied on top: https://tbpl.mozilla.org/?tree=Try&rev=9d119ffdb171

Comment 39

[Geoff Brown [:gbrown]]

Comment on attachment 767006 [details] [diff] [review]
Part 3: tests v3

Review of attachment 767006 [details] [diff] [review]:
-----------------------------------------------------------------

This is great -- mostly just nits below. The try run looks a little rough still, especially testSettingsMenuItems -- let's get that running reliably for the next review.

::: mobile/android/base/tests/BaseTest.java.in
@@ +401,5 @@
>  
>      /** 
>       * Select <item> from Menu > "Settings" > <section>
> +     *
> +     * Caller must already be in Settings, by calling selectMenuItem("Settings").

I really prefer this the way it was: Have selectSettingsItem call selectMenuItem("Settings") rather than make all the callers do it.

@@ +406,4 @@
>       */
>      public void selectSettingsItem(String section, String item) {
>          String itemName = "^" + item + "$";
> +        waitForText(section);

Not waitForEnabledText()? You don't need that any more?

::: mobile/android/base/tests/testSettingsMenuItems.java.in
@@ +2,5 @@
>  package @ANDROID_PACKAGE_NAME@.tests;
>  
>  import @ANDROID_PACKAGE_NAME@.*;
>  
> +import android.test.ActivityInstrumentationTestCase2;

Are you sure you need this?

@@ +92,2 @@
>  
> +        navigation = new Navigation(new Device());

nit - use mDevice (from BaseTest) instead of a new instance

@@ +99,5 @@
>          verifyUrl("about:home");
>          mActions.sendSpecialKey(Actions.SpecialKey.BACK);
>          waitForText("Enter Search"); // Waiting for page title to appear to be sure that is fully loaded before opening the menu
>  
> +        Device mDevice = new Device();

use mDevice from BaseTest

@@ +113,5 @@
> +     *
> +     * Sync location is a top level menu item on phones, but is under "Customize" on tablets.
> +     *
> +     */
> +    public void checkForSync(Device device) {

Just use mDevice member -- no need to pass as a parameter.

@@ +137,5 @@
> +            for (String[] item : sectionItems) {
> +                int itemLen = item.length;
> +
> +                // Each item must at least have a title.
> +                ActivityInstrumentationTestCase2.assertTrue(item.length > 0);

Why not use a method from mAsserter here?

@@ +174,5 @@
> +                  mSolo.clickOnText("^Cancel$");
> +                }
> +            }
> +            // Navigate back a screen if on a phone.
> +            Device device = new Device();

mDevice again

Comment 40

[Chenxia Liu [:liuche]]

Thanks, I'll make those changes. While looking at the try builds, I discovered that I had added a regression (bug 886916) that was breaking the settings tests. I'll land that and upload a new patch with review comments addressed.

Comment 41

[Chenxia Liu [:liuche]]

Attachment: Part 3: tests v4

Responded to comments, fixed some other tests that dealt with nested settings.

When testing locally, I'm seeing a failure in testImportFromAndroid on a 2.3 phone, but not my 4.0 phone or 4.0 tablet, where the last history item is not found. I suspect it's a race condition related to [1], but let me know what you think.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testImportFromAndroid.java.in#54

Comment 42

[Chenxia Liu [:liuche]]

Attachment: Part 3: tests v5

Whoops, qref

Comment 43

[Geoff Brown [:gbrown]]

(In reply to Chenxia Liu [:liuche] from comment #41)
> Created attachment 767551 [details] [diff] [review]
> Part 3: tests v4
> 
> Responded to comments, fixed some other tests that dealt with nested
> settings.
> 
> When testing locally, I'm seeing a failure in testImportFromAndroid on a 2.3
> phone, but not my 4.0 phone or 4.0 tablet, where the last history item is
> not found. I suspect it's a race condition related to [1], but let me know
> what you think.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> testImportFromAndroid.java.in#54

I think you are probably right. Does it help if you change the timeout to something like 10000 instead of MAX_WAIT_MS?

Comment 44

[Geoff Brown [:gbrown]]

Comment on attachment 767553 [details] [diff] [review]
Part 3: tests v5

Review of attachment 767553 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for all the updates. 

r+ conditional on a good try run, of course.

::: mobile/android/base/tests/BaseTest.java.in
@@ +457,5 @@
>              mSolo.clickOnText(itemName);
>          } else {
> +            // Record that we've timed out waiting for "Settings" on the top menu
> +            // screen so we can skip the timeout step next time.
> +            menuSettingsTimedOut = true;

Good idea!

Comment 45

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44419f25ea67
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a80bf8b30bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b8ab2edf59

Comment 46

[Chenxia Liu [:liuche]]

This enables testSettingsMenuItems, so dropping a ref to bug 843947 here because these patches should fix the intermittent oranges.

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44419f25ea67
https://hg.mozilla.org/mozilla-central/rev/0a80bf8b30bf
https://hg.mozilla.org/mozilla-central/rev/61b8ab2edf59