Organize and consolidate TextScalePreference code

TextScalePreference is the only SeekBarPreference used in Settings, so
merge it with SeekBarPreference for more organized and readable logic.

(Note: If a common SeekBarPreference widget is needed in future, the
SeekBarPreference class now available in AndroidX is likely a better
choice, although not suited to this particular usage.)

Fixed: 977206
Change-Id: I4a4751ba96bd5b4f8d007546dd6db00884eeaf5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003215
Commit-Queue: Natalie Chouinard <chouinard@chromium.org>
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733041}

chrome/android/chrome_java_sources.gni

@@ -1371,7 +1371,6 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/settings/LocationSettings.java",
   "java/src/org/chromium/chrome/browser/settings/MainPreferences.java",
   "java/src/org/chromium/chrome/browser/settings/NfcSystemLevelSetting.java",
-  "java/src/org/chromium/chrome/browser/settings/SeekBarPreference.java",
   "java/src/org/chromium/chrome/browser/settings/SettingsActivity.java",
   "java/src/org/chromium/chrome/browser/settings/SettingsLauncher.java",
   "java/src/org/chromium/chrome/browser/settings/about/AboutChromePreferenceOSVersion.java",

chrome/android/java/src/org/chromium/chrome/browser/settings/SeekBarPreference.java

-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// Based on frameworks/base/core/java/android/preference/SeekBarPreference.java,
-// extended to support floating-point min/max/step and a summary label.
-
-package org.chromium.chrome.browser.settings;
-
-import android.content.Context;
-import android.support.v7.preference.Preference;
-import android.support.v7.preference.PreferenceViewHolder;
-import android.text.TextUtils;
-import android.util.AttributeSet;
-import android.widget.SeekBar;
-import android.widget.SeekBar.OnSeekBarChangeListener;
-import android.widget.TextView;
-
-import org.chromium.chrome.R;
-
-/**
- * A preference that allows the user to set a value by sliding a seek bar.
- *
- * When this preference is used, a layout must be provided that contains a SeekBar with ID "seekbar"
- * and a TextView with ID "seekbar_amount".
- */
-public class SeekBarPreference extends Preference implements OnSeekBarChangeListener {
-    private float mMin;
-    private float mMax;
-    private float mStep;
-    private float mValue;
-    private boolean mTrackingTouch;
-    CharSequence mSummary;
-    private TextView mSummaryView;
-
-    /**
-     * Constructor for inflating from XML.
-     */
-    public SeekBarPreference(Context context, AttributeSet attrs) {
-        super(context, attrs);
-        mMin = 0.5f;
-        mMax = 2.0f;
-        mStep = 0.05f;
-        mValue = mMin;
-    }
-
-    /**
-     * Sets the progress value for the seekbar. The value will be adjusted, if needed, to ensure
-     * it's within the valid range.
-     */
-    public void setValue(float value) {
-        setValue(value, true);
-    }
-
-    /**
-     * Returns whether the user is currently dragging the seek bar.
-     */
-    public boolean isTrackingTouch() {
-        return mTrackingTouch;
-    }
-
-    @Override
-    public void onBindViewHolder(PreferenceViewHolder holder) {
-        super.onBindViewHolder(holder);
-        SeekBar seekBar = (SeekBar) holder.findViewById(R.id.seekbar);
-        seekBar.setOnSeekBarChangeListener(this);
-        seekBar.setMax(prefValueToSeekBarProgress(mMax));
-        seekBar.setProgress(prefValueToSeekBarProgress(mValue));
-        seekBar.setEnabled(isEnabled());
-        mSummaryView = (TextView) holder.findViewById(R.id.seekbar_amount);
-        mSummaryView.setText(mSummary);
-    }
-
-    /**
-     * Sets the summary for this Preference with a CharSequence. Unlike the superclass
-     * implementation, this does not call notifyChanged() as that would cancel the
-     * current slider drag.
-     *
-     * @param summary The summary for the preference.
-     */
-    @Override
-    public void setSummary(CharSequence summary) {
-        if (TextUtils.equals(summary, mSummary)) return;
-
-        mSummary = summary;
-        if (mSummaryView != null) mSummaryView.setText(summary);
-    }
-
-    @Override
-    public CharSequence getSummary() {
-        return mSummary;
-    }
-
-    private float seekBarProgressToPrefValue(int seekBarProgress) {
-        // SeekBar only supports integer steps, and always starts from 0.
-        // So must convert floating point pref values to/from integers,
-        // appropriately scaled for the SeekBar.
-        return mMin + seekBarProgress * mStep;
-    }
-
-    private int prefValueToSeekBarProgress(float prefValue) {
-        return Math.round((prefValue - mMin) / mStep);
-    }
-
-    private void setValue(float value, boolean notifyChanged) {
-        value = Math.min(mMax, Math.max(mMin, value));
-        if (value != mValue) {
-            mValue = value;
-            if (notifyChanged) notifyChanged();
-        }
-    }
-
-    /**
-     * Persist the seekBar's progress value if callChangeListener
-     * returns true, otherwise set the seekBar's progress to the stored value
-     */
-    private void syncProgress(SeekBar seekBar) {
-        float value = seekBarProgressToPrefValue(seekBar.getProgress());
-        if (value != mValue) {
-            if (callChangeListener(value)) {
-                setValue(value, false);
-            } else {
-                seekBar.setProgress(prefValueToSeekBarProgress(mValue));
-            }
-        }
-    }
-
-    @Override
-    public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
-        if (fromUser) syncProgress(seekBar);
-    }
-
-    @Override
-    public void onStartTrackingTouch(SeekBar seekBar) {
-        mTrackingTouch = true;
-    }
-
-    @Override
-    public void onStopTrackingTouch(SeekBar seekBar) {
-        mTrackingTouch = false;
-    }
-}

chrome/android/java/src/org/chromium/chrome/browser/settings/accessibility/AccessibilitySettings.java

@@ -22,8 +22,6 @@ import org.chromium.chrome.browser.settings.ChromeBaseCheckBoxPreference;
 import org.chromium.chrome.browser.settings.SettingsUtils;
 import org.chromium.chrome.browser.util.AccessibilityUtil;
 
-import java.text.NumberFormat;
-
 /**
  * Fragment to keep track of all the accessibility related preferences.
  */
@@ -34,16 +32,14 @@ public class AccessibilitySettings
     static final String PREF_READER_FOR_ACCESSIBILITY = "reader_for_accessibility";
     static final String PREF_CAPTIONS = "captions";
 
-    private NumberFormat mFormat;
-    private FontSizePrefs mFontSizePrefs;
-
     private TextScalePreference mTextScalePref;
     private ChromeBaseCheckBoxPreference mForceEnableZoomPref;
 
+    private FontSizePrefs mFontSizePrefs = FontSizePrefs.getInstance();
     private FontSizePrefsObserver mFontSizePrefsObserver = new FontSizePrefsObserver() {
         @Override
         public void onFontScaleFactorChanged(float fontScaleFactor, float userFontScaleFactor) {
-            updateTextScaleSummary(userFontScaleFactor);
+            mTextScalePref.updateFontScaleFactors(fontScaleFactor, userFontScaleFactor, true);
         }
 
         @Override
@@ -53,19 +49,26 @@ public class AccessibilitySettings
     };
 
     @Override
-    public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
+    public void onActivityCreated(Bundle savedInstanceState) {
+        super.onActivityCreated(savedInstanceState);
+
         getActivity().setTitle(R.string.prefs_accessibility);
-        SettingsUtils.addPreferencesFromResource(this, R.xml.accessibility_preferences);
+        setDivider(null);
+    }
 
-        mFormat = NumberFormat.getPercentInstance();
-        mFontSizePrefs = FontSizePrefs.getInstance();
+    @Override
+    public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
+        SettingsUtils.addPreferencesFromResource(this, R.xml.accessibility_preferences);
 
         mTextScalePref = (TextScalePreference) findPreference(PREF_TEXT_SCALE);
         mTextScalePref.setOnPreferenceChangeListener(this);
+        mTextScalePref.updateFontScaleFactors(mFontSizePrefs.getFontScaleFactor(),
+                mFontSizePrefs.getUserFontScaleFactor(), false);
 
         mForceEnableZoomPref =
                 (ChromeBaseCheckBoxPreference) findPreference(PREF_FORCE_ENABLE_ZOOM);
         mForceEnableZoomPref.setOnPreferenceChangeListener(this);
+        mForceEnableZoomPref.setChecked(mFontSizePrefs.getForceEnableZoom());
 
         ChromeBaseCheckBoxPreference readerForAccessibilityPref =
                 (ChromeBaseCheckBoxPreference) findPreference(PREF_READER_FOR_ACCESSIBILITY);
@@ -101,42 +104,18 @@ public class AccessibilitySettings
         }
     }
 
-    @Override
-    public void onActivityCreated(Bundle savedInstanceState) {
-        super.onActivityCreated(savedInstanceState);
-        setDivider(null);
-    }
-
     @Override
     public void onStart() {
         super.onStart();
-        updateValues();
-        // TODO(crbug.com/977206): Move this call to make TextScalePreference self-contained
-        mTextScalePref.startObservingFontPrefs();
         mFontSizePrefs.addObserver(mFontSizePrefsObserver);
     }
 
     @Override
     public void onStop() {
-        // TODO(crbug.com/977206): Move this call to make TextScalePreference self-contained
-        mTextScalePref.stopObservingFontPrefs();
         mFontSizePrefs.removeObserver(mFontSizePrefsObserver);
         super.onStop();
     }
 
-    private void updateValues() {
-        float userFontScaleFactor = mFontSizePrefs.getUserFontScaleFactor();
-        mTextScalePref.setValue(userFontScaleFactor);
-        updateTextScaleSummary(userFontScaleFactor);
-
-        mForceEnableZoomPref.setChecked(mFontSizePrefs.getForceEnableZoom());
-    }
-
-    // TODO(crbug.com/977206): Move this to within TextScalePreference
-    private void updateTextScaleSummary(float userFontScaleFactor) {
-        mTextScalePref.setSummary(mFormat.format(userFontScaleFactor));
-    }
-
     @Override
     public boolean onPreferenceChange(Preference preference, Object newValue) {
         if (PREF_TEXT_SCALE.equals(preference.getKey())) {

chrome/android/java/src/org/chromium/chrome/browser/settings/accessibility/TextScalePreference.java

@@ -5,33 +5,41 @@
 package org.chromium.chrome.browser.settings.accessibility;
 
 import android.content.Context;
+import android.support.v7.preference.Preference;
 import android.support.v7.preference.PreferenceViewHolder;
 import android.util.AttributeSet;
 import android.util.TypedValue;
+import android.widget.SeekBar;
 import android.widget.TextView;
 
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.accessibility.FontSizePrefs;
-import org.chromium.chrome.browser.accessibility.FontSizePrefs.FontSizePrefsObserver;
-import org.chromium.chrome.browser.settings.SeekBarPreference;
+
+import java.text.NumberFormat;
 
 /**
  * Preference that allows the user to change the scaling factor that's applied to web page text.
  * This also shows a preview of how large a typical web page's text will appear.
  */
-public class TextScalePreference extends SeekBarPreference {
-    private TextView mPreview;
-    private final FontSizePrefs mFontSizePrefs;
+public class TextScalePreference extends Preference implements SeekBar.OnSeekBarChangeListener {
+    private static final float MIN = 0.5f;
+    private static final float MAX = 2.0f;
+    private static final float STEP = 0.05f;
 
-    private final FontSizePrefsObserver mFontSizePrefsObserver = new FontSizePrefsObserver() {
-        @Override
-        public void onFontScaleFactorChanged(float fontScaleFactor, float userFontScaleFactor) {
-            updatePreview();
-        }
+    /**
+     * Online body text tends to be around 13-16px. We ask the user to adjust the text scale until
+     * 12px text is legible, that way all body text will be legible (and since font boosting
+     * approximately preserves relative font size differences, other text will be bigger/smaller as
+     * appropriate).
+     */
+    private static final float SMALLEST_STANDARD_FONT_SIZE_PX = 12.0f;
+
+    private float mUserFontScaleFactor = MIN;
+    private float mFontScaleFactor;
+
+    private TextView mAmount;
+    private TextView mPreview;
 
-        @Override
-        public void onForceEnableZoomChanged(boolean enabled) {}
-    };
+    private NumberFormat mFormat = NumberFormat.getPercentInstance();
 
     /**
      * Constructor for inflating from XML.
@@ -39,8 +47,6 @@ public class TextScalePreference extends SeekBarPreference {
     public TextScalePreference(Context context, AttributeSet attrs) {
         super(context, attrs);
 
-        mFontSizePrefs = FontSizePrefs.getInstance();
-
         setLayoutResource(R.layout.custom_preference);
         setWidgetLayoutResource(R.layout.preference_text_scale);
     }
@@ -48,37 +54,67 @@ public class TextScalePreference extends SeekBarPreference {
     @Override
     public void onBindViewHolder(PreferenceViewHolder holder) {
         super.onBindViewHolder(holder);
-        if (mPreview == null) {
-            mPreview = (TextView) holder.findViewById(R.id.preview);
-            updatePreview();
-        }
+
+        SeekBar seekBar = (SeekBar) holder.findViewById(R.id.seekbar);
+        seekBar.setOnSeekBarChangeListener(this);
+        seekBar.setMax(userFontScaleFactorToProgress(MAX));
+        seekBar.setProgress(userFontScaleFactorToProgress(mUserFontScaleFactor));
+
+        mAmount = (TextView) holder.findViewById(R.id.seekbar_amount);
+        mPreview = (TextView) holder.findViewById(R.id.preview);
+
+        updateViews();
+    }
+
+    void updateFontScaleFactors(
+            float fontScaleFactor, float userFontScaleFactor, boolean updateViews) {
+        mFontScaleFactor = fontScaleFactor;
+        mUserFontScaleFactor = userFontScaleFactor;
+
+        if (updateViews) updateViews();
+    }
+
+    private void updateViews() {
+        mAmount.setText(mFormat.format(mUserFontScaleFactor));
+        mPreview.setTextSize(
+                TypedValue.COMPLEX_UNIT_DIP, SMALLEST_STANDARD_FONT_SIZE_PX * mFontScaleFactor);
     }
 
     /**
-     * Listens for changes to the text scale and updates the preview text as needed. This must be
-     * matched with a call to stopObservingFontPrefs().
+     * {@link SeekBar} only supports integer steps, and starts from 0, so the following methods are
+     * necessary to scale floating point pref values to/from integer progress amounts.
+     *
+     * @param progress {@link SeekBar} progress amount.
+     * @return User font scale factor preference value.
      */
-    public void startObservingFontPrefs() {
-        mFontSizePrefs.addObserver(mFontSizePrefsObserver);
-        updatePreview();
+    private float progressToUserFontScaleFactor(int progress) {
+        return MIN + progress * STEP;
+    }
+
+    private int userFontScaleFactorToProgress(float userFontScaleFactor) {
+        return Math.round((userFontScaleFactor - MIN) / STEP);
     }
 
     /**
-     * Stops listening for changes to the text scale.
+     * Notifies {@link Preference.OnPreferenceChangeListener}s of updated value.
      */
-    public void stopObservingFontPrefs() {
-        mFontSizePrefs.removeObserver(mFontSizePrefsObserver);
+    @Override
+    public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
+        if (!fromUser) return;
+
+        float userFontScaleFactor = progressToUserFontScaleFactor(progress);
+        if (userFontScaleFactor == mUserFontScaleFactor) return;
+
+        callChangeListener(userFontScaleFactor);
     }
 
-    private void updatePreview() {
-        if (mPreview != null) {
-            // Online body text tends to be around 13-16px. We ask the user to adjust the text scale
-            // until 12px text is legible, that way all body text will be legible (and since font
-            // boosting approximately preserves relative font size differences, other text will be
-            // bigger/smaller as appropriate).
-            final float smallestStandardWebPageFontSize = 12.0f; // CSS px
-            mPreview.setTextSize(TypedValue.COMPLEX_UNIT_DIP,
-                    smallestStandardWebPageFontSize * mFontSizePrefs.getFontScaleFactor());
-        }
+    @Override
+    public void onStartTrackingTouch(SeekBar seekBar) {}
+
+    @Override
+    public void onStopTrackingTouch(SeekBar seekBar) {}
+
+    CharSequence getAmountForTesting() {
+        return mAmount.getText();
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/settings/accessibility/AccessibilitySettingsTest.java

@@ -24,7 +24,6 @@ import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.accessibility.FontSizePrefs;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.settings.ChromeBaseCheckBoxPreference;
-import org.chromium.chrome.browser.settings.SeekBarPreference;
 import org.chromium.chrome.browser.settings.SettingsActivityTest;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.util.browser.Features;
@@ -55,8 +54,9 @@ public class AccessibilitySettingsTest {
                         .startSettingsActivity(InstrumentationRegistry.getInstrumentation(),
                                 accessibilitySettingsClassname)
                         .getMainFragment();
-        SeekBarPreference textScalePref = (SeekBarPreference) accessibilitySettings.findPreference(
-                AccessibilitySettings.PREF_TEXT_SCALE);
+        TextScalePreference textScalePref =
+                (TextScalePreference) accessibilitySettings.findPreference(
+                        AccessibilitySettings.PREF_TEXT_SCALE);
         ChromeBaseCheckBoxPreference forceEnableZoomPref =
                 (ChromeBaseCheckBoxPreference) accessibilitySettings.findPreference(
                         AccessibilitySettings.PREF_FORCE_ENABLE_ZOOM);
@@ -71,7 +71,7 @@ public class AccessibilitySettingsTest {
         UiUtils.settleDownUI(InstrumentationRegistry.getInstrumentation());
         // Since above the threshold, this will check the force enable zoom button.
         Assert.assertEquals(
-                percentFormat.format(fontBiggerThanThreshold), textScalePref.getSummary());
+                percentFormat.format(fontBiggerThanThreshold), textScalePref.getAmountForTesting());
         Assert.assertTrue(forceEnableZoomPref.isChecked());
         assertFontSizePrefs(true, fontBiggerThanThreshold);
 
@@ -80,8 +80,8 @@ public class AccessibilitySettingsTest {
         UiUtils.settleDownUI(InstrumentationRegistry.getInstrumentation());
         // Since below the threshold and userSetForceEnableZoom is false, this will uncheck
         // the force enable zoom button.
-        Assert.assertEquals(
-                percentFormat.format(fontSmallerThanThreshold), textScalePref.getSummary());
+        Assert.assertEquals(percentFormat.format(fontSmallerThanThreshold),
+                textScalePref.getAmountForTesting());
         Assert.assertFalse(forceEnableZoomPref.isChecked());
         assertFontSizePrefs(false, fontSmallerThanThreshold);
 
@@ -132,7 +132,7 @@ public class AccessibilitySettingsTest {
     }
 
     private static void userSetTextScale(final AccessibilitySettings accessibilitySettings,
-            final SeekBarPreference textScalePref, final float textScale) {
+            final TextScalePreference textScalePref, final float textScale) {
         PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT,
                 () -> accessibilitySettings.onPreferenceChange(textScalePref, textScale));
     }