1047561 - Create settings UI for enabling the new tablet UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

So that users can opt-in to use the new tablet UI.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Add UI to enable/disable the new tablet UI (r=mcomella)

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8468611 [details] [diff] [review]
Add UI to enable/disable the new tablet UI (r=mcomella)

Strings are hardcoded because they are not meant to be official l10n material.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8468611 [details] [diff] [review]
Add UI to enable/disable the new tablet UI (r=mcomella)

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

r+ w/ nits.

::: mobile/android/base/resources/xml/preferences_display.xml
@@ +24,5 @@
>      <CheckBoxPreference android:key="browser.chrome.dynamictoolbar"
>                          android:title="@string/pref_scroll_title_bar2"
>                          android:summary="@string/pref_scroll_title_bar_summary" />
>  
> +    <!-- Title string is hardcoded here because it's not meant to be translated -->

nit: Add why is it not meant to be translated.

@@ +27,5 @@
>  
> +    <!-- Title string is hardcoded here because it's not meant to be translated -->
> +    <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui"
> +                        android:title="Enable new tablet UI"
> +                        android:defaultValue="true" />

This is duplicated here and in the patch in bug 1046200. I imagine it's because we don't seem to be calling `PreferenceManager.setDefaultValues` anywhere (see [1]).

I'd make a note in the other patch that this value should be updated too when merging to m-c.

[1]: http://stackoverflow.com/a/3911077

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8468611 [details] [diff] [review]
Add UI to enable/disable the new tablet UI (r=mcomella)

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

::: mobile/android/base/resources/xml/preferences_display.xml
@@ +27,5 @@
>  
> +    <!-- Title string is hardcoded here because it's not meant to be translated -->
> +    <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui"
> +                        android:title="Enable new tablet UI"
> +                        android:defaultValue="true" />

Actually, does this defaultValue even do anything without calling PM.setDefaultValues?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #4)
> Comment on attachment 8468611 [details] [diff] [review]
> Add UI to enable/disable the new tablet UI (r=mcomella)
> 
> Review of attachment 8468611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/xml/preferences_display.xml
> @@ +27,5 @@
> >  
> > +    <!-- Title string is hardcoded here because it's not meant to be translated -->
> > +    <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui"
> > +                        android:title="Enable new tablet UI"
> > +                        android:defaultValue="true" />
> 
> Actually, does this defaultValue even do anything without calling
> PM.setDefaultValues?

Apparently, it does.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #3)
> Comment on attachment 8468611 [details] [diff] [review]
> Add UI to enable/disable the new tablet UI (r=mcomella)
> 
> Review of attachment 8468611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ w/ nits.
> 
> ::: mobile/android/base/resources/xml/preferences_display.xml
> @@ +24,5 @@
> >      <CheckBoxPreference android:key="browser.chrome.dynamictoolbar"
> >                          android:title="@string/pref_scroll_title_bar2"
> >                          android:summary="@string/pref_scroll_title_bar_summary" />
> >  
> > +    <!-- Title string is hardcoded here because it's not meant to be translated -->
> 
> nit: Add why is it not meant to be translated.

Done.

> @@ +27,5 @@
> >  
> > +    <!-- Title string is hardcoded here because it's not meant to be translated -->
> > +    <CheckBoxPreference android:key="android.not_a_preference.new_tablet_ui"
> > +                        android:title="Enable new tablet UI"
> > +                        android:defaultValue="true" />
> 
> This is duplicated here and in the patch in bug 1046200. I imagine it's
> because we don't seem to be calling `PreferenceManager.setDefaultValues`
> anywhere (see [1]).
> 
> I'd make a note in the other patch that this value should be updated too
> when merging to m-c.

Yep, done. I'm also keeping a list of things we'll need to do before merging to m-c in the tablet-refresh etherpad.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Add UI to enable/disable the new tablet UI (r=mcomella)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8472394 [details] [diff] [review]
Add UI to enable/disable the new tablet UI (r=mcomella)

Decided to make this a bit more future proof and only show the UI toggle on non-release builds. Updated the robotium tests accordingly.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8472394 [details] [diff] [review]
Add UI to enable/disable the new tablet UI (r=mcomella)

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

Where do you make the change to ensure this only appears on non-release builds? I just see the updated tests.

::: mobile/android/base/tests/testSettingsMenuItems.java
@@ +158,5 @@
>              // Text reflow - only built if *not* release build
>              String[] textReflowUi = { StringHelper.TEXT_REFLOW_LABEL };
>              settingsMap.get(PATH_DISPLAY).add(textReflowUi);
>  
> +            // New tablet UI can only be enabled in non-release build

nit: The comment is technically redundant because it's inside the `if (!AppConstants.RELEASE_BUILD)` block, but I'm fine with leaving it in.

Don't forget to mark this in the etherpad for removal.

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Add UI to enable/disable the new tablet UI (r=mcomella)

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8472901 [details] [diff] [review]
Add UI to enable/disable the new tablet UI (r=mcomella)

(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8472394 [details] [diff] [review]
> Add UI to enable/disable the new tablet UI (r=mcomella)
> 
> Review of attachment 8472394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Where do you make the change to ensure this only appears on non-release
> builds? I just see the updated tests.

Oops, forgot to squash this part in.

> ::: mobile/android/base/tests/testSettingsMenuItems.java
> @@ +158,5 @@
> >              // Text reflow - only built if *not* release build
> >              String[] textReflowUi = { StringHelper.TEXT_REFLOW_LABEL };
> >              settingsMap.get(PATH_DISPLAY).add(textReflowUi);
> >  
> > +            // New tablet UI can only be enabled in non-release build
> 
> nit: The comment is technically redundant because it's inside the `if
> (!AppConstants.RELEASE_BUILD)` block, but I'm fine with leaving it in.
> 
> Don't forget to mark this in the etherpad for removal.

Good point, done.

Comment 12

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/projects/larch/rev/ced8f132378c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ced8f132378c

Comment 14

[Aaron Train [:aaronmt]]

Preference forgot to check for isTablet, filed bug above.