Notification custom layouts: paint the button icon for Material.

chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java

Index: chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
index 56f067322507dee04ffe4be02b652bc94b2103c8..e8bca7457097cd0269a4f36b167d2190271b0df5 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
@@ -7,7 +7,13 @@
 import android.app.Notification;
 import android.app.PendingIntent;
 import android.content.Context;
+import android.content.res.Resources;
 import android.graphics.Bitmap;
+import android.graphics.BitmapFactory;
+import android.graphics.Canvas;
+import android.graphics.Paint;
+import android.graphics.PorterDuff;
+import android.graphics.PorterDuffColorFilter;
 import android.os.Build;
 import android.support.v4.app.NotificationCompat;
 import android.support.v4.app.NotificationCompat.Action;
@@ -17,6 +23,7 @@
 import android.view.View;
 import android.widget.RemoteViews;
 
+import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 
@@ -60,7 +67,23 @@
      * flexible amount of padding. If the font size is scaled up the applied padding will be scaled
      * down towards 0.
      */
-    private static final int MAX_SCALABLE_PADDING_DIP = 3;
+    private static final int MAX_SCALABLE_PADDING_DP = 3;
+
+    /**
+     * The amount of padding at the start of the button, either before an icon or before the text.
+     */
+    private static final int BUTTON_PADDING_START_DP = 8;
+
+    /**
+     * The amount of padding between the icon and text of a button. Used only if there is an icon.
+     */
+    private static final int BUTTON_ICON_PADDING_DP = 8;
+
+    /**
+     * The color named "secondary_text_default_material_light" with hex value #8a000000 converted to

[Peter Beverloo, 2015/11/16 11:18:12]

nit: don't name the value (that's up to the style to define)

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

Acknowledged.

+     * an int.
+     */
+    private static final int BUTTON_ICON_COLOR_MATERIAL = -1979711488;

[Peter Beverloo, 2015/11/16 11:18:13]

_ID?

[newt (away), 2015/11/16 17:50:09]

You can just write this as "0x8a000000". Or you could use
secondary_text_default_material_light directly if it's publicly exported or
available via the support library (which seems to be the case), e.g.
Resources.getColor(R.color.secondary_text_default_material_light)

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

Done (the getColor part). Nice.

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

This number is not an id, it's a color value.

 
     private final Context mContext;
 
@@ -101,21 +124,9 @@ public Notification build() {
             view.setViewPadding(R.id.title, 0, scaledPadding, 0, 0);
             view.setViewPadding(R.id.body, 0, scaledPadding, 0, scaledPadding);
         }
+        addActionButtons(bigView);
 
-        if (!mActions.isEmpty()) {
-            bigView.setViewVisibility(R.id.button_divider, View.VISIBLE);
-            bigView.setViewVisibility(R.id.buttons, View.VISIBLE);
-            for (Action action : mActions) {
-                RemoteViews button = new RemoteViews(
-                        mContext.getPackageName(), R.layout.web_notification_button);
-                button.setTextViewCompoundDrawablesRelative(R.id.button, action.getIcon(), 0, 0, 0);
-                button.setTextViewText(R.id.button, action.getTitle());
-                button.setOnClickPendingIntent(R.id.button, action.getActionIntent());
-                bigView.addView(R.id.buttons, button);
-            }
-        }
-
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
+        if (useMaterial()) {
             compactView.setViewVisibility(R.id.small_icon_overlay, View.VISIBLE);
             bigView.setViewVisibility(R.id.small_icon_overlay, View.VISIBLE);
         } else {
@@ -207,6 +218,53 @@ public NotificationBuilder setVibrate(long[] pattern) {
         return this;
     }
 
+    /**
+     * If there are actions, shows the button related views, and adds a button for each action.
+     */
+    private void addActionButtons(RemoteViews bigView) {
+        if (mActions.isEmpty()) {
+            return;
+        }
+        bigView.setViewVisibility(R.id.button_divider, View.VISIBLE);
+        bigView.setViewVisibility(R.id.buttons, View.VISIBLE);
+        Resources resources = mContext.getResources();
+        DisplayMetrics metrics = resources.getDisplayMetrics();
+        boolean ltr = ApiCompatibilityUtils.getLayoutDirection(resources.getConfiguration())

[Peter Beverloo, 2015/11/16 11:18:13]

LocalizationUtils.isLayoutRtl()

It's much more common for branches to check on RTL rather than LTR, and this
allows you to re-use some existing infrastructure. Note that it does cache the
value, which may affect your testing.

[newt (away), 2015/11/16 17:50:09]

"isRtl" is the typical variable name in cases like this

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

Done.

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

Done.

+                == View.LAYOUT_DIRECTION_LTR;
+        for (Action action : mActions) {
+            RemoteViews view =
+                    new RemoteViews(mContext.getPackageName(), R.layout.web_notification_button);
+
+            if (action.getIcon() != 0) {

[Peter Beverloo, 2015/11/16 11:18:13]

You mention in the XML file that a Bitmap is the format we get when loading an
image from the network, but that feature actually doesn't exist (yet). It would
be worthwhile to rephrase 243-245 as a TODO. (I think the comment in the layout
adds value.)

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

Done.

+                BitmapFactory.Options options = new BitmapFactory.Options();
+                options.inMutable = true;

[Peter Beverloo, 2015/11/16 11:18:12]

Why do we need this?

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

Only mutable bitmaps can be painted, otherwise an additional mutable copy must
be made.

+                Bitmap icon = BitmapFactory.decodeResource(resources, action.getIcon(), options);
+
+                // Currently the only button icon we use is the settings gear, which is known to
+                // have the correct color for Holo (white) but not for Material. Once we have web
+                // developer provided icons we'll need to decide how to process them, if at all.
+                if (useMaterial()) {
+                    paintBitmap(icon, BUTTON_ICON_COLOR_MATERIAL);

[newt (away), 2015/11/16 17:50:09]

Could you use "setColorFilter" on the ImageButton instead of doing the painting
yourself?

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

But how could we? We only have RemoteViews here, and those do not expose all
possible methods of the views, they must be annotated with RemotableViewMethod.

[newt (away), 2015/11/17 20:04:40]

setColorFilter(int) is @RemotableViewMethod, but it uses SRC_ATOP blending,
which doesn't seem to work for your purposes, correct?

[Michael van Ouwerkerk, 2015/11/18 14:08:40]

The top view needs to be a Button, so that TalkBack can read its text when
tapped. So the view below can be an ImageView, no need to use an ImageButton for
that part.

Ah yes there is setColorFilter(int) for ImageView which can be called with
RemoteViews#setInt(int viewId, String methodName, int value). When using a
different color to achieve the same result with SRC_ATOP, this actually works.
This is cool, simpler, thanks :-) We might have different requirements in future
for web developer provided icons, but for now this will do.

+                }
+                view.setImageViewBitmap(R.id.button_icon, icon);
+
+                // Set the padding of the button so the text does not overlap with the icon. Flip
+                // between left and right manually as RemoteViews does not expose a method that sets
+                // padding in a writing-direction independent way.
+                int buttonPadding =
+                        dpToPx(BUTTON_PADDING_START_DP + BUTTON_ICON_PADDING_DP, metrics)
+                        + icon.getWidth();

[newt (away), 2015/11/16 17:50:08]

Are we sure that the icon won't be unexpectedly wide? This is only a concern if
you're using website-supplied icons.

[Michael van Ouwerkerk, 2015/11/17 12:00:23]

For now, we are sure it is ok, as we don't have web developer provided images
yet. Once we do, we'll need to decide on what processing steps to apply.

+                int buttonPaddingLeft = ltr ? buttonPadding : 0;
+                int buttonPaddingRight = ltr ? 0 : buttonPadding;
+                view.setViewPadding(R.id.button, buttonPaddingLeft, 0, buttonPaddingRight, 0);
+            }
+
+            view.setTextViewText(R.id.button, action.getTitle());
+            view.setOnClickPendingIntent(R.id.button, action.getActionIntent());
+            bigView.addView(R.id.buttons, view);
+        }
+    }
+
     @Nullable
     private static CharSequence limitLength(@Nullable CharSequence input) {
         if (input == null) {
@@ -252,7 +310,33 @@ static int calculateScaledPadding(float fontScale, DisplayMetrics displayMetrics
             fontScale = Math.min(fontScale, FONT_SCALE_LARGE);
             paddingScale = (FONT_SCALE_LARGE - fontScale) / (FONT_SCALE_LARGE - 1.0f);
         }
-        return Math.round(TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP,
-                paddingScale * MAX_SCALABLE_PADDING_DIP, displayMetrics));
+        return dpToPx(paddingScale * MAX_SCALABLE_PADDING_DP, displayMetrics);
+    }
+
+    /**
+     * Converts a dp value to a px value.
+     */
+    private static int dpToPx(float value, DisplayMetrics displayMetrics) {
+        return Math.round(
+                TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, value, displayMetrics));
+    }
+
+    /**
+     * Whether to use the Material look and feel or fall back to Holo.
+     */
+    private static boolean useMaterial() {
+        return Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
+    }
+
+    /**
+     * Paints |bitmap| with |color| using |PorterDuff.Mode.MULTIPLY|.
+     */
+    @VisibleForTesting
+    static Bitmap paintBitmap(Bitmap bitmap, int color) {
+        Paint paint = new Paint();
+        paint.setColorFilter(new PorterDuffColorFilter(color, PorterDuff.Mode.MULTIPLY));
+        Canvas canvas = new Canvas(bitmap);
+        canvas.drawBitmap(bitmap, 0, 0, paint);
+        return bitmap;
     }
 }