1129443 - Add item in Settings to manage tab queue

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

The user may want to configure certain things about this feature like disabling/enabling it, etc. We should leave this under an item in Settings for the time being.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Add item in Settings to manage tab queue

This patch uses build flags introduced in the patch attached to bug 1132185 (not landed at the time of submitting this patch)

Comment 2

[Martyn Haigh (:mhaigh)]

Comment on attachment 8563466 [details] [diff] [review]
Add item in Settings to manage tab queue

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +661,4 @@
>                  } else if (pref instanceof PanelsPreferenceCategory) {
>                      mPanelsPreferenceCategory = (PanelsPreferenceCategory) pref;
>                  }
> +                if(!(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD) && pref.getSummary().equals(getString(R.string.pref_category_customize_summary))) {

The first condition here is incorrect - I need to remove the negation.

-> if(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD && pref.getSummary()...

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8563466 [details] [diff] [review]
Add item in Settings to manage tab queue

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

Looks good, just one comment.

You should also add this to the testSettingsMenuItems test, and make sure you it only runs on nightly+tab_queue - this should be pretty straightforward since there should be other prefs behind flags.

(I'm not finding the patches where this pref is actually used. Point me to them? Or is it not done yet?)

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +661,4 @@
>                  } else if (pref instanceof PanelsPreferenceCategory) {
>                      mPanelsPreferenceCategory = (PanelsPreferenceCategory) pref;
>                  }
> +                if(!(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD) && pref.getSummary().equals(getString(R.string.pref_category_customize_summary))) {

Can you add an android:key to the category_customize PreferenceScreen xml element, and check for that instead of string matching?

::: mobile/android/base/resources/xml-v11/preferences_customize_tablet.xml
@@ +38,5 @@
>                      android:persistent="true" />
>  
> +    <CheckBoxPreference android:key="android.not_a_preference.tab_queue"
> +                        android:title="@string/pref_tab_queue_title"
> +                        android:summary="@string/pref_tab_queue_summary" />

Is there a android:defaultValue?

Comment 4

[Richard Newman [:rnewman]]

> -> if(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD &&
> pref.getSummary()...

Consider making MOZ_ANDROID_TAB_QUEUE depend on NIGHTLY_BUILD in Bug 1132185, unless you want to have functionality in Nightly while the feature ships elsewhere. This will simplify your conditionals, and minimize the amount of code you need to change when you let this feature ride the trains.

E.g.,

  # Enable the tab queue in Nightly only. This will go away in Bug 1132507.
  MOZ_ANDROID_TAB_QUEUE=$NIGHTLY_BUILD

then

  if (AppConstants.MOZ_ANDROID_TAB_QUEUE && pref.getSummary()) {

Comment 5

[Martyn Haigh (:mhaigh)]

(In reply to Richard Newman [:rnewman] from comment #4)
> > -> if(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD &&
> > pref.getSummary()...
> 
> Consider making MOZ_ANDROID_TAB_QUEUE depend on NIGHTLY_BUILD in Bug
> 1132185, unless you want to have functionality in Nightly while the feature
> ships elsewhere. This will simplify your conditionals, and minimize the
> amount of code you need to change when you let this feature ride the trains.
> 
> E.g.,
> 
>   # Enable the tab queue in Nightly only. This will go away in Bug 1132507.
>   MOZ_ANDROID_TAB_QUEUE=$NIGHTLY_BUILD
> 

We removed this flag in the submitted patch for bug 1132185 (as keeping it in would enable it across the board - we currently just want to be able to enable from mozconfig), but have added bug 1133524 to track the tab queue dependency on the nightly flag.

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Add item in Settings to manage tab queue

Just asking for review again to make sure that I got the test stuff correct - it was pretty simple but having not done it before I want to make sure I've not missed anything.  

Also I've corrected the tab queue pref name as on further inspection using the 'android.not_a_preference' prefix implies that the preference is gecko based, which this is not.

The preferences currently aren't used anywhere as that's a separate bug (yet to file).

Final copy is in flux - tracking that in bug 1133755

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8565448 [details] [diff] [review]
Add item in Settings to manage tab queue

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

So this is NOT a gecko pref, but rather, an Android pref, correct? See comment about Gecko/Android preferences, the naming is a bit confusing...

r+ with changing the preference keys back.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +662,4 @@
>                  } else if (pref instanceof PanelsPreferenceCategory) {
>                      mPanelsPreferenceCategory = (PanelsPreferenceCategory) pref;
>                  }
> +                if((AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD) && (pref.hasKey() && PREFS_CUSTOMIZE_SCREEN.equals(pref.getKey()))) {

I think pref.getKey() is already a variable in this scope, so you don't need to make all these calls to get the key.

::: mobile/android/base/resources/xml-v11/preferences.xml
@@ +18,4 @@
>  
>      <PreferenceScreen android:title="@string/pref_category_customize"
>                        android:summary="@string/pref_category_customize_summary"
> +                      android:key="preferences.customize_screen"

Actually...android:key being prefixed by android.not_a_preference means it is NOT a Gecko pref, so you should switch the naming of these keys back.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#765

...these really should be named android.not_gecko_pref, but I think at the time of writing, all the prefs were Gecko, so things were either a pref or not, and the case where we use Android-only-not-Gecko SharedPreferences wasn't considered. Hence the confusing naming.

::: mobile/android/base/resources/xml-v11/preferences_customize.xml
@@ +30,4 @@
>                      android:entryValues="@array/pref_restore_values"
>                      android:persistent="true" />
>  
> +    <CheckBoxPreference android:key="app.feature.tab_queue"

Ditto android.not_a_preference naming.

::: mobile/android/base/resources/xml-v11/preferences_customize_tablet.xml
@@ +37,4 @@
>                      android:entryValues="@array/pref_restore_values"
>                      android:persistent="true" />
>  
> +    <CheckBoxPreference android:key="app.feature.tab_queue"

Ditto android.not_a_preference naming.

::: mobile/android/base/strings.xml.in
@@ +124,4 @@
>    <string name="pref_category_advanced">&pref_category_advanced;</string>
>    <string name="pref_category_customize">&pref_category_customize;</string>
>    <string name="pref_category_customize_summary">&pref_category_customize_summary;</string>
> +  <string name="pref_category_customize_alt_summary">&pref_category_customize_alt_summary;</string>

Nit: maybe keep the newline separator between the sections?

::: mobile/android/base/tests/testSettingsMenuItems.java
@@ +181,5 @@
>          }
>  
> +        // Tab Queue
> +        if (AppConstants.NIGHTLY_BUILD && AppConstants.MOZ_ANDROID_TAB_QUEUE) {
> +            String[] tabQueue = { StringHelper.TAB_QUEUE_LABEL, "Prevent tabs from opening immediately, but open all queued tabs the next time " + StringHelper.BRAND_NAME + " loads." };

I guess technically we should add the alternative summary text, but we don't actually check for it anyways right now, so that's probably fine - though it could be good to have the check to make sure we're not shipping something in the wrong release. Up to you.

Comment 8

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/ee6252e71b8b

Comment 9

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/b6f1c25cf603

Comment 10

[Martyn Haigh (:mhaigh)]

removed the section of test which was failing

https://hg.mozilla.org/integration/fx-team/rev/9d5ae6ec32cb

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/9d5ae6ec32cb

Comment 12

[Ton [:Tonnes]]

>> +<!ENTITY pref_tab_queue_summary "Prevent tabs from opening immediately, but open all queued tabs the next time &brandShortName; loads.">

Sorry for being finicky and mentioning this after noticing during l10n (again), but please avoid using end stops in summaries or tooltips. This is now an exeption in this file where it probably should not - the only summary needing one led to bug 1147465.