From 8f65efa8f92528c6f259e34f5aed2ae692efb16e Mon Sep 17 00:00:00 2001
From: tmfang <tmfang@google.com>
Date: Mon, 12 Nov 2018 17:42:31 +0800
Subject: [PATCH] Create a new visual design for action buttons

- Create a new style for two action buttons.
- Change the layout of two_action_buttons.
  Since we use same style for positive/nagative
  buttons now, we only remain button1/button2 for
  this layout.
- Create a setButtonIcon interface in ActionButtonPreference.
  So, user can set icon for each button in this preferece.

Test: visual, robotest
Bug: 116346008
Change-Id: I511272cba5fd961349b85cae6d30004ddabe2c8e
---
 res/layout/two_action_buttons.xml             |  51 +++------
 res/values/styles.xml                         |   5 +
 .../widget/ActionButtonPreference.java        | 100 +++++++++++-------
 .../applications/LayoutPreferenceTest.java    |   8 +-
 ...BluetoothDetailsButtonsControllerTest.java |   4 +-
 .../widget/ActionButtonPreferenceTest.java    |  90 +++++++++-------
 6 files changed, 139 insertions(+), 119 deletions(-)

diff --git a/res/layout/two_action_buttons.xml b/res/layout/two_action_buttons.xml
index 23bb886c901..2b4aa5d33fc 100644
--- a/res/layout/two_action_buttons.xml
+++ b/res/layout/two_action_buttons.xml
@@ -19,47 +19,28 @@
     xmlns:android="http://schemas.android.com/apk/res/android"
     android:layout_width="match_parent"
     android:layout_height="wrap_content"
-    android:gravity="center"
-    android:paddingTop="24dp"
-    android:paddingStart="68dp"
-    android:paddingEnd="24dp"
+    android:paddingStart="8dp"
+    android:paddingEnd="8dp"
     android:orientation="horizontal">
 
-    <FrameLayout
+    <Button
+        android:id="@+id/button1"
+        style="@style/SettingsActionButton"
         android:layout_width="0dp"
+        android:layout_height="wrap_content"
         android:layout_weight="1"
-        android:layout_height="wrap_content">
-        <Button
-            android:id="@+id/button1_positive"
-            style="@style/ActionPrimaryButton"
-            android:layout_width="match_parent"
-            android:layout_height="wrap_content"
-            android:layout_marginBottom="4dp" />
-        <Button
-            android:id="@+id/button1_negative"
-            style="@style/ActionSecondaryButton"
-            android:layout_width="match_parent"
-            android:layout_height="wrap_content"
-            android:layout_marginBottom="4dp" />
-    </FrameLayout>
+        android:paddingTop="20dp"
+        android:paddingBottom="20dp"/>
     <Space
-        android:layout_width="16dp"
+        android:layout_width="8dp"
         android:layout_height="wrap_content" />
-    <FrameLayout
+
+    <Button
+        android:id="@+id/button2"
+        style="@style/SettingsActionButton"
         android:layout_width="0dp"
+        android:layout_height="wrap_content"
         android:layout_weight="1"
-        android:layout_height="wrap_content">
-        <Button
-            android:id="@+id/button2_positive"
-            style="@style/ActionPrimaryButton"
-            android:layout_width="match_parent"
-            android:layout_height="wrap_content"
-            android:layout_marginBottom="4dp" />
-        <Button
-            android:id="@+id/button2_negative"
-            style="@style/ActionSecondaryButton"
-            android:layout_width="match_parent"
-            android:layout_height="wrap_content"
-            android:layout_marginBottom="4dp" />
-    </FrameLayout>
+        android:paddingTop="20dp"
+        android:paddingBottom="20dp"/>
 </LinearLayout>
\ No newline at end of file
diff --git a/res/values/styles.xml b/res/values/styles.xml
index 8fa357bc75f..58be846e38a 100644
--- a/res/values/styles.xml
+++ b/res/values/styles.xml
@@ -426,6 +426,11 @@
 
     <style name="ActionSecondaryButton" parent="android:Widget.DeviceDefault.Button"/>
 
+    <style name="SettingsActionButton" parent="android:Widget.DeviceDefault.Button.Borderless.Colored">
+        <item name="android:drawablePadding">4dp</item>
+        <item name="android:drawableTint">@*android:color/btn_colored_borderless_text_material</item>
+    </style>
+
     <style name="LockPatternContainerStyle">
         <item name="android:maxHeight">400dp</item>
         <item name="android:maxWidth">420dp</item>
diff --git a/src/com/android/settings/widget/ActionButtonPreference.java b/src/com/android/settings/widget/ActionButtonPreference.java
index 347a784b2ba..9fe80c0c1d7 100644
--- a/src/com/android/settings/widget/ActionButtonPreference.java
+++ b/src/com/android/settings/widget/ActionButtonPreference.java
@@ -17,11 +17,15 @@
 package com.android.settings.widget;
 
 import android.content.Context;
+import android.content.res.Resources;
+import android.graphics.drawable.Drawable;
 import android.text.TextUtils;
 import android.util.AttributeSet;
+import android.util.Log;
 import android.view.View;
 import android.widget.Button;
 
+import androidx.annotation.DrawableRes;
 import androidx.annotation.StringRes;
 import androidx.preference.Preference;
 import androidx.preference.PreferenceViewHolder;
@@ -30,6 +34,7 @@ import com.android.settings.R;
 
 public class ActionButtonPreference extends Preference {
 
+    private final String TAG = "ActionButtonPreference";
     private final ButtonInfo mButton1Info = new ButtonInfo();
     private final ButtonInfo mButton2Info = new ButtonInfo();
 
@@ -62,12 +67,11 @@ public class ActionButtonPreference extends Preference {
     @Override
     public void onBindViewHolder(PreferenceViewHolder holder) {
         super.onBindViewHolder(holder);
-        holder.setDividerAllowedAbove(false);
-        holder.setDividerAllowedBelow(false);
-        mButton1Info.mPositiveButton = (Button) holder.findViewById(R.id.button1_positive);
-        mButton1Info.mNegativeButton = (Button) holder.findViewById(R.id.button1_negative);
-        mButton2Info.mPositiveButton = (Button) holder.findViewById(R.id.button2_positive);
-        mButton2Info.mNegativeButton = (Button) holder.findViewById(R.id.button2_negative);
+        holder.setDividerAllowedAbove(true);
+        holder.setDividerAllowedBelow(true);
+
+        mButton1Info.mButton = (Button) holder.findViewById(R.id.button1);
+        mButton2Info.mButton = (Button) holder.findViewById(R.id.button2);
 
         mButton1Info.setUpButton();
         mButton2Info.setUpButton();
@@ -82,6 +86,22 @@ public class ActionButtonPreference extends Preference {
         return this;
     }
 
+    public ActionButtonPreference setButton1Icon(@DrawableRes int iconResId) {
+        if (iconResId == 0) {
+            return this;
+        }
+
+        final Drawable icon;
+        try {
+            icon = getContext().getDrawable(iconResId);
+            mButton1Info.mIcon = icon;
+            notifyChanged();
+        } catch (Resources.NotFoundException exception) {
+            Log.e(TAG, "Resource does not exist: " + iconResId);
+        }
+        return this;
+    }
+
     public ActionButtonPreference setButton1Enabled(boolean isEnabled) {
         if (isEnabled != mButton1Info.mIsEnabled) {
             mButton1Info.mIsEnabled = isEnabled;
@@ -99,6 +119,22 @@ public class ActionButtonPreference extends Preference {
         return this;
     }
 
+    public ActionButtonPreference setButton2Icon(@DrawableRes int iconResId) {
+        if (iconResId == 0) {
+            return this;
+        }
+
+        final Drawable icon;
+        try {
+            icon = getContext().getDrawable(iconResId);
+            mButton2Info.mIcon = icon;
+            notifyChanged();
+        } catch (Resources.NotFoundException exception) {
+            Log.e(TAG, "Resource does not exist: " + iconResId);
+        }
+        return this;
+    }
+
     public ActionButtonPreference setButton2Enabled(boolean isEnabled) {
         if (isEnabled != mButton2Info.mIsEnabled) {
             mButton2Info.mIsEnabled = isEnabled;
@@ -123,65 +159,51 @@ public class ActionButtonPreference extends Preference {
         return this;
     }
 
+    @Deprecated
     public ActionButtonPreference setButton1Positive(boolean isPositive) {
-        if (isPositive != mButton1Info.mIsPositive) {
-            mButton1Info.mIsPositive = isPositive;
-            notifyChanged();
-        }
         return this;
     }
 
+    @Deprecated
     public ActionButtonPreference setButton2Positive(boolean isPositive) {
-        if (isPositive != mButton2Info.mIsPositive) {
-            mButton2Info.mIsPositive = isPositive;
-            notifyChanged();
-        }
         return this;
     }
-    public ActionButtonPreference setButton1Visible(boolean isPositive) {
-        if (isPositive != mButton1Info.mIsVisible) {
-            mButton1Info.mIsVisible = isPositive;
+
+    public ActionButtonPreference setButton1Visible(boolean isVisible) {
+        if (isVisible != mButton1Info.mIsVisible) {
+            mButton1Info.mIsVisible = isVisible;
             notifyChanged();
         }
         return this;
     }
 
-    public ActionButtonPreference setButton2Visible(boolean isPositive) {
-        if (isPositive != mButton2Info.mIsVisible) {
-            mButton2Info.mIsVisible = isPositive;
+    public ActionButtonPreference setButton2Visible(boolean isVisible) {
+        if (isVisible != mButton2Info.mIsVisible) {
+            mButton2Info.mIsVisible = isVisible;
             notifyChanged();
         }
         return this;
     }
 
     static class ButtonInfo {
-        private Button mPositiveButton;
-        private Button mNegativeButton;
+        private Button mButton;
         private CharSequence mText;
+        private Drawable mIcon;
         private View.OnClickListener mListener;
-        private boolean mIsPositive = true;
         private boolean mIsEnabled = true;
         private boolean mIsVisible = true;
 
         void setUpButton() {
-            setUpButton(mPositiveButton);
-            setUpButton(mNegativeButton);
-            if (!mIsVisible) {
-                mPositiveButton.setVisibility(View.INVISIBLE);
-                mNegativeButton.setVisibility(View.INVISIBLE);
-            } else if (mIsPositive) {
-                mPositiveButton.setVisibility(View.VISIBLE);
-                mNegativeButton.setVisibility(View.INVISIBLE);
+            mButton.setText(mText);
+            mButton.setOnClickListener(mListener);
+            mButton.setEnabled(mIsEnabled);
+            mButton.setCompoundDrawablesWithIntrinsicBounds(
+                    null /* left */, mIcon /* top */, null /* right */, null /* bottom */);
+            if (mIsVisible) {
+                mButton.setVisibility(View.VISIBLE);
             } else {
-                mPositiveButton.setVisibility(View.INVISIBLE);
-                mNegativeButton.setVisibility(View.VISIBLE);
+                mButton.setVisibility(View.GONE);
             }
         }
-
-        private void setUpButton(Button button) {
-            button.setText(mText);
-            button.setOnClickListener(mListener);
-            button.setEnabled(mIsEnabled);
-        }
     }
 }
\ No newline at end of file
diff --git a/tests/robotests/src/com/android/settings/applications/LayoutPreferenceTest.java b/tests/robotests/src/com/android/settings/applications/LayoutPreferenceTest.java
index da65165c1e9..1d2bb3a0b88 100644
--- a/tests/robotests/src/com/android/settings/applications/LayoutPreferenceTest.java
+++ b/tests/robotests/src/com/android/settings/applications/LayoutPreferenceTest.java
@@ -75,12 +75,12 @@ public class LayoutPreferenceTest {
 
     @Test
     public void disableSomeView_shouldMaintainStateAfterBind() {
-        mPreference.findViewById(R.id.button1_positive).setEnabled(false);
-        mPreference.findViewById(R.id.button2_positive).setEnabled(true);
+        mPreference.findViewById(R.id.button1).setEnabled(false);
+        mPreference.findViewById(R.id.button2).setEnabled(true);
 
         mPreference.onBindViewHolder(mHolder);
 
-        assertThat(mPreference.findViewById(R.id.button1_positive).isEnabled()).isFalse();
-        assertThat(mPreference.findViewById(R.id.button2_positive).isEnabled()).isTrue();
+        assertThat(mPreference.findViewById(R.id.button1).isEnabled()).isFalse();
+        assertThat(mPreference.findViewById(R.id.button2).isEnabled()).isTrue();
     }
 }
diff --git a/tests/robotests/src/com/android/settings/bluetooth/BluetoothDetailsButtonsControllerTest.java b/tests/robotests/src/com/android/settings/bluetooth/BluetoothDetailsButtonsControllerTest.java
index d17fd9640a0..6c555dd2ff2 100644
--- a/tests/robotests/src/com/android/settings/bluetooth/BluetoothDetailsButtonsControllerTest.java
+++ b/tests/robotests/src/com/android/settings/bluetooth/BluetoothDetailsButtonsControllerTest.java
@@ -56,8 +56,8 @@ public class BluetoothDetailsButtonsControllerTest extends BluetoothDetailsContr
         super.setUp();
         final View buttons = View.inflate(
                 RuntimeEnvironment.application, R.layout.two_action_buttons, null /* parent */);
-        mConnectButton = buttons.findViewById(R.id.button2_positive);
-        mForgetButton = buttons.findViewById(R.id.button1_positive);
+        mConnectButton = buttons.findViewById(R.id.button2);
+        mForgetButton = buttons.findViewById(R.id.button1);
         mController =
             new BluetoothDetailsButtonsController(mContext, mFragment, mCachedDevice, mLifecycle);
         mButtonsPref = ActionButtonPreferenceTest.createMock();
diff --git a/tests/robotests/src/com/android/settings/widget/ActionButtonPreferenceTest.java b/tests/robotests/src/com/android/settings/widget/ActionButtonPreferenceTest.java
index a66f6d55ca0..fb952a7c2eb 100644
--- a/tests/robotests/src/com/android/settings/widget/ActionButtonPreferenceTest.java
+++ b/tests/robotests/src/com/android/settings/widget/ActionButtonPreferenceTest.java
@@ -25,6 +25,7 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import android.content.Context;
+import android.graphics.drawable.Drawable;
 import android.view.View;
 import android.widget.Button;
 
@@ -59,41 +60,17 @@ public class ActionButtonPreferenceTest {
         mPref.setButton1Visible(false).setButton2Visible(false);
         mPref.onBindViewHolder(mHolder);
 
-        assertThat(mRootView.findViewById(R.id.button1_positive).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-        assertThat(mRootView.findViewById(R.id.button1_negative).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-
-        assertThat(mRootView.findViewById(R.id.button2_positive).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-        assertThat(mRootView.findViewById(R.id.button2_negative).getVisibility())
-                .isEqualTo(View.INVISIBLE);
+        assertThat(mRootView.findViewById(R.id.button1).getVisibility())
+                .isEqualTo(View.GONE);
+        assertThat(mRootView.findViewById(R.id.button2).getVisibility())
+                .isEqualTo(View.GONE);
 
         mPref.setButton1Visible(true).setButton2Visible(true);
         mPref.onBindViewHolder(mHolder);
 
-        assertThat(mRootView.findViewById(R.id.button1_positive).getVisibility())
+        assertThat(mRootView.findViewById(R.id.button1).getVisibility())
                 .isEqualTo(View.VISIBLE);
-        assertThat(mRootView.findViewById(R.id.button1_negative).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-        assertThat(mRootView.findViewById(R.id.button2_positive).getVisibility())
-                .isEqualTo(View.VISIBLE);
-        assertThat(mRootView.findViewById(R.id.button2_negative).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-    }
-
-    @Test
-    public void setPositiveNegative_shouldHideOppositeButton() {
-        mPref.setButton1Positive(true).setButton2Positive(false);
-        mPref.onBindViewHolder(mHolder);
-
-        assertThat(mRootView.findViewById(R.id.button1_positive).getVisibility())
-                .isEqualTo(View.VISIBLE);
-        assertThat(mRootView.findViewById(R.id.button1_negative).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-        assertThat(mRootView.findViewById(R.id.button2_positive).getVisibility())
-                .isEqualTo(View.INVISIBLE);
-        assertThat(mRootView.findViewById(R.id.button2_negative).getVisibility())
+        assertThat(mRootView.findViewById(R.id.button2).getVisibility())
                 .isEqualTo(View.VISIBLE);
     }
 
@@ -102,36 +79,71 @@ public class ActionButtonPreferenceTest {
         mPref.setButton1Enabled(true).setButton2Enabled(false);
         mPref.onBindViewHolder(mHolder);
 
-        assertThat(mRootView.findViewById(R.id.button1_positive).isEnabled()).isTrue();
-        assertThat(mRootView.findViewById(R.id.button1_negative).isEnabled()).isTrue();
-        assertThat(mRootView.findViewById(R.id.button2_positive).isEnabled()).isFalse();
-        assertThat(mRootView.findViewById(R.id.button2_negative).isEnabled()).isFalse();
+        assertThat(mRootView.findViewById(R.id.button1).isEnabled()).isTrue();
+        assertThat(mRootView.findViewById(R.id.button2).isEnabled()).isFalse();
     }
 
     @Test
-    public void setText() {
+    public void setText_shouldShowSameText() {
         mPref.setButton1Text(R.string.settings_label);
         mPref.onBindViewHolder(mHolder);
 
-        assertThat(((Button) mRootView.findViewById(R.id.button1_positive)).getText())
-                .isEqualTo(mContext.getText(R.string.settings_label));
-        assertThat(((Button) mRootView.findViewById(R.id.button1_negative)).getText())
+        assertThat(((Button) mRootView.findViewById(R.id.button1)).getText())
                 .isEqualTo(mContext.getText(R.string.settings_label));
     }
 
+    @Test
+    public void setButtonIcon_iconMustDisplayAboveText() {
+        mPref.setButton1Text(R.string.settings_label);
+        mPref.setButton1Icon(R.drawable.ic_settings);
+        mPref.onBindViewHolder(mHolder);
+        final Drawable[] drawablesAroundText =
+                ((Button) mRootView.findViewById(R.id.button1))
+                        .getCompoundDrawables();
+
+        assertThat(drawablesAroundText[1 /* top */]).isNotNull();
+    }
+
+    @Test
+    public void setButtonIcon_iconResourceIdIsZero_shouldNotDisplayIcon() {
+        mPref.setButton1Text(R.string.settings_label);
+        mPref.setButton1Icon(0);
+        mPref.onBindViewHolder(mHolder);
+        final Drawable[] drawablesAroundText =
+                ((Button) mRootView.findViewById(R.id.button1))
+                        .getCompoundDrawables();
+
+        assertThat(drawablesAroundText[1 /* top */]).isNull();
+    }
+
+    @Test
+    public void setButtonIcon_iconResourceIdNotExisting_shouldNotDisplayIconAndCrash() {
+        mPref.setButton1Text(R.string.settings_label);
+        mPref.setButton1Icon(999999999 /* not existing id */);
+        // Should not crash here
+        mPref.onBindViewHolder(mHolder);
+        final Drawable[] drawablesAroundText =
+                ((Button) mRootView.findViewById(R.id.button1))
+                        .getCompoundDrawables();
+
+        assertThat(drawablesAroundText[1 /* top */]).isNull();
+    }
+
     public static ActionButtonPreference createMock() {
         final ActionButtonPreference pref = mock(ActionButtonPreference.class);
         when(pref.setButton1Text(anyInt())).thenReturn(pref);
+        when(pref.setButton1Icon(anyInt())).thenReturn(pref);
         when(pref.setButton1Positive(anyBoolean())).thenReturn(pref);
         when(pref.setButton1Enabled(anyBoolean())).thenReturn(pref);
         when(pref.setButton1Visible(anyBoolean())).thenReturn(pref);
         when(pref.setButton1OnClickListener(any(View.OnClickListener.class))).thenReturn(pref);
 
         when(pref.setButton2Text(anyInt())).thenReturn(pref);
+        when(pref.setButton2Icon(anyInt())).thenReturn(pref);
         when(pref.setButton2Positive(anyBoolean())).thenReturn(pref);
         when(pref.setButton2Enabled(anyBoolean())).thenReturn(pref);
         when(pref.setButton2Visible(anyBoolean())).thenReturn(pref);
         when(pref.setButton2OnClickListener(any(View.OnClickListener.class))).thenReturn(pref);
         return pref;
     }
-}
+}
\ No newline at end of file