Create settings UI for enabling the new tablet UI

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-larch])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
So that users can opt-in to use the new tablet UI.
(Assignee)

Comment 2

5 years ago
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.
Attachment #8468611 - Flags: review?(michael.l.comella)
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
Attachment #8468611 - Flags: review?(michael.l.comella) → review+
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?
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #8468611 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
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.
Attachment #8472394 - Flags: review?(michael.l.comella)
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.
(Assignee)

Updated

5 years ago
Attachment #8472394 - Attachment is obsolete: true
Attachment #8472394 - Flags: review?(michael.l.comella)
(Assignee)

Comment 11

5 years ago
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.
Attachment #8472901 - Flags: review?(michael.l.comella)
Attachment #8472901 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/ced8f132378c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Depends on: 1068005
Preference forgot to check for isTablet, filed bug above.
You need to log in before you can comment on or make changes to this bug.