Implement the new Photo picker, part two.

chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java

+// Copyright 2016 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.
+
+package org.chromium.chrome.browser.photo_picker;
+
+import android.content.Context;
+import android.content.res.Resources;
+import android.graphics.Bitmap;
+import android.graphics.BitmapFactory;
+import android.graphics.Color;
+import android.graphics.PorterDuff;
+import android.graphics.drawable.BitmapDrawable;
+import android.graphics.drawable.Drawable;
+import android.support.annotation.Nullable;
+import android.support.v4.content.ContextCompat;
+import android.util.AttributeSet;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.ImageView;
+import android.widget.TextView;
+
+import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.widget.selection.SelectableItemView;
+import org.chromium.chrome.browser.widget.selection.SelectionDelegate;
+
+import java.util.List;
+
+/**
+ * A container class for a view showing a photo in the photo picker.
+ */
+public class PickerBitmapView extends SelectableItemView<PickerBitmap> {
+    // Our context.
+    private Context mContext;
+
+    // Our parent category.
+    private PickerCategoryView mCategoryView;
+
+    // Our selection delegate.
+    private SelectionDelegate<PickerBitmap> mSelectionDelegate;
+
+    // The request details (meta-data) for the bitmap shown.
+    private PickerBitmap mRequest;
+
+    // The image view containing the bitmap.
+    private ImageView mIconView;
+
+    // The view behind the image, representing the selection border.
+    private View mBorderView;
+
+    // The camera/gallery special tile (with icon as drawable).
+    private TextView mSpecialTile;
+
+    // Whether the image has been loaded already.
+    private boolean mImageLoaded;
+
+    /**
+     * Constructor for inflating from XML.
+     */
+    public PickerBitmapView(Context context, AttributeSet attrs) {
+        super(context, attrs);
+        mContext = context;
+    }
+
+    @Override
+    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
+        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
+
+        if (mCategoryView == null) return;
+
+        int width = mCategoryView.getImageSize();
+        int height = mCategoryView.getImageSize();
+        setMeasuredDimension(width, height);
+    }
+
+    @Override
+    protected void onFinishInflate() {
+        super.onFinishInflate();
+        mIconView = (ImageView) findViewById(R.id.bitmap_view);
+        mBorderView = findViewById(R.id.border);
+        mSpecialTile = (TextView) findViewById(R.id.special_tile);
+    }
+
+    @Override
+    public void onClick() {
+        // TODO(finnur): Remove the null checks here and below.
+        if (mRequest != null && mRequest.type() == PickerBitmap.TileTypes.GALLERY) {
+            mCategoryView.showGallery();
+            return;
+        } else if (mRequest != null && mRequest.type() == PickerBitmap.TileTypes.CAMERA) {
+            mCategoryView.showCamera();
+            return;
+        }
+
+        mSelectionDelegate.toggleSelectionForItem(mRequest);

[Theresa, 2017/03/31 18:05:56]

I think this could be simplified to calling super.onLongClick(this), which calls
toggleSelectionForItem and setChecked().

A comment indicating that a regular click on an item in the photo picker selects
it would be helpful.

[Finnur, 2017/04/03 17:30:29]

Yup. Seems to work fine that way.

+        setChecked(!super.isChecked());
+    }
+
+    @Override
+    public void setChecked(boolean checked) {
+        if (mRequest != null && mRequest.type() != PickerBitmap.TileTypes.PICTURE) {
+            return;
+        }
+
+        super.setChecked(checked);
+        updateSelectionState();
+    }
+
+    @Override
+    public void onSelectionStateChange(List<PickerBitmap> selectedItems) {
+        boolean selected = selectedItems.contains(mRequest);
+
+        if (mRequest != null && mRequest.type() != PickerBitmap.TileTypes.PICTURE) {
+            if (selected) mSelectionDelegate.toggleSelectionForItem(mRequest);
+            updateSelectionState();

[Theresa, 2017/03/31 18:05:56]

Instead of unselecting the camera/gallery here, could you override
toggleSelectionForItem() to return false if the item type isn't PICTURE and call
super if the type is PICTURE?

[Finnur, 2017/04/03 17:30:29]

Done (much better).

+            return;
+        }
+
+        boolean checked = super.isChecked();
+
+        if (!mCategoryView.isMultiSelect() && !selected && checked) {
+            super.toggle();

[Theresa, 2017/03/31 18:05:56]

I'm confused about how the item could be !selected but checked? Is it because
this isn't calling super before this check?

I think that this is trying to unset the checked state if this multi-selection
isn't allowed. Can this be done earlier then the onSelectionStateChange() call? 

[Finnur, 2017/04/03 17:30:29]

I don't remember why this is needed anymore and since this CL no longer supports
multi-selection, I think it is best to remove this and revisit at a later date.

+        }
+
+        updateSelectionState();
+    }
+
+    /**
+     * Pre-initializes the PickerBitmapView.
+     * @param categoryView The category view showing the images.
+     */
+    public void preInitialize(PickerCategoryView categoryView) {
+        mCategoryView = categoryView;
+        mSelectionDelegate = mCategoryView.getSelectionDelegate();
+        super.setSelectionDelegate(mSelectionDelegate);
+    }
+
+    /**
+     * Completes the initialization of the PickerBitmapView. Must be called before the image can
+     * respond to click events.
+     * @param request The request represented by this PickerBitmapView.
+     * @param thumbnail The Bitmap to use for the thumbnail (or null).
+     * @param placeholder Whether the image given is a placeholder or the actual image.
+     */
+    public void initialize(PickerBitmap request, @Nullable Bitmap thumbnail, boolean placeholder) {
+        resetTile();
+
+        mRequest = request;
+        setItem(request);
+        setThumbnailBitmap(thumbnail);
+        mImageLoaded = !placeholder;
+
+        updateSelectionState();
+
+        setOnClickListener(this);

[Theresa, 2017/03/31 18:05:56]

The superclass already calls setOnClickListener() after inflation is finished.

[Finnur, 2017/04/03 17:30:29]

Removed.

+    }
+
+    /**
+     * Initialization for the special tiles (camera/gallery icon).
+     */
+    public void initializeSpecialTile() {
+        int size = mCategoryView.getImageSize();

[Theresa, 2017/03/31 18:05:56]

You shouldn't need to programatically construct a bitmap for the camera and
gallery icons. Rahter than scaling the camera and collections 24dp icons up to
file_picker_special_icon_size (which is 48dp?), you should just include the 48 x
48dp icons (or SVG equivalents) and use something like this:

Drawable image = ApiCompatibilityUtils.getDrawable(resources, iconBitmapId);
ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(mSpecialTile,
null, drawable, null, null);

[Finnur, 2017/04/03 17:30:29]

I don't have much experience with how the drawables work. Is this accomplished
by moving the current assets from drawable-xxxhdpi to drawable-xxhdpi and xxhdpi
to xhdpi (continuing all the way down to drawable-mdpi) or is it enough to
simply delete the mdpi asset? Or are you referring to something completely
different? :)

[Theresa, 2017/04/04 15:48:32]

Something completely different :).

Currently you're constructing an empty "Bitmap tile". I don't think that should
be needed at all (I could be wrong).

Then below you're calling:
Bitmap icon = BitmapFactory.decodeResource(resources, iconBitmapId);
BitmapDrawable drawable = new BitmapDrawable resources,
Bitmap.createScaledBitmap(icon, (int) pixels, (int) pixels, true));
ApiCompatibilityUtils.setCompoundDrawable....

You should be able to avoid programatically creating bitmaps and BitmapDrawables
and do something like this:
Drawable image = ApiCompatibilityUtils.getDrawable(resources, iconBitmapId);
ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(mSpecialTile,
null, drawable, null, null);


If the PNG you're using is smaller than mCategoryView.getImageSize(), then
you'll probably want to get a bigger PNG.

[Finnur, 2017/04/04 18:05:36]

I've introduced the vector format to this, so the comments are partly addressing
code that's changed. 

Although I think there's probably room for improvements, I've ran out of time
for today and didn't want to hold the review up for this. I'll try to improve
this further tomorrow. Particularly wondering if I can use the vector directly
in xml... hmmm...

By the way, the png will always be smaller than getImageSize (it's a centered
icon on a gray background).

[Theresa, 2017/04/04 18:40:45]

Vector drawables cannot be used directly in XML - it crashes on older versions
of Android.

[Finnur, 2017/04/05 15:14:39]

Understood. I've done it the way you suggested. Thanks!

+        Bitmap tile = Bitmap.createBitmap(size, size, Bitmap.Config.ARGB_8888);
+        tile.eraseColor(Color.argb(0, 0, 0, 0));
+
+        int iconBitmapId, labelStringId;
+        if (mRequest != null && mRequest.type() == PickerBitmap.TileTypes.CAMERA) {
+            iconBitmapId = R.drawable.ic_photo_camera;
+            labelStringId = R.string.file_picker_camera;
+        } else {
+            iconBitmapId = R.drawable.ic_collections_black_24dp;
+            labelStringId = R.string.file_picker_browse;
+        }
+
+        Resources resources = mContext.getResources();
+        mSpecialTile.setText(labelStringId);
+        Bitmap icon = BitmapFactory.decodeResource(resources, iconBitmapId);
+        float pixels = resources.getDimensionPixelOffset(R.dimen.file_picker_special_icon_size);
+        BitmapDrawable drawable = new BitmapDrawable(
+                resources, Bitmap.createScaledBitmap(icon, (int) pixels, (int) pixels, true));
+        ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(
+                mSpecialTile, null, drawable, null, null);
+
+        initialize(mRequest, tile, false);
+
+        mSpecialTile.setVisibility(View.VISIBLE);
+    }
+
+    /**
+     * Sets a thumbnail bitmap for the current view.
+     * @param thumbnail The Bitmap to use for the icon ImageView.
+     * @return True if no image was loaded before (e.g. not even a low-res image).
+     */
+    public boolean setThumbnailBitmap(Bitmap thumbnail) {
+        mIconView.setImageBitmap(thumbnail);
+
+        boolean noImageWasLoaded = !mImageLoaded;
+        mImageLoaded = true;
+        updateSelectionState();
+
+        return noImageWasLoaded;
+    }
+
+    /**
+     * Resets the view to its starting state, which is necessary when the view is about to be
+     * re-used.
+     */
+    private void resetTile() {
+        mSpecialTile.setVisibility(View.GONE);
+    }
+
+    /**
+     * Adds padding to the parent of the |view|.
+     * @param view The child view of the view to receive the padding.
+     * @param padding The amount of padding to use (in pixels).
+     */
+    private static void addPaddingToParent(View view, int padding) {

[Theresa, 2017/03/31 18:05:56]

Is anything calling this anymore?

[Finnur, 2017/04/03 17:30:29]

Nope! Removed.

+        ViewGroup layout = (ViewGroup) view.getParent();
+        layout.setPadding(padding, padding, padding, padding);
+        layout.requestLayout();
+    }
+
+    /**
+     * Updates the selection controls for this view.
+     */
+    private void updateSelectionState() {
+        boolean special = mRequest != null && mRequest.type() != PickerBitmap.TileTypes.PICTURE;
+        boolean checked = super.isChecked();
+        boolean anySelection =
+                mSelectionDelegate != null && mSelectionDelegate.isSelectionEnabled();
+        boolean multiSelect = mCategoryView.isMultiSelect();
+        int bgColorId, fgColorId;
+        if (!special) {
+            bgColorId = R.color.file_picker_tile_bg_color;
+            fgColorId = R.color.file_picker_special_tile_color;
+        } else if (!anySelection || !multiSelect) {
+            bgColorId = R.color.file_picker_special_tile_bg_color;
+            fgColorId = R.color.file_picker_special_tile_color;
+        } else {
+            bgColorId = R.color.file_picker_special_tile_disabled_bg_color;

[Theresa, 2017/03/31 18:05:56]

If multi-select is allowed, won't this set the set the bg color for special
tiles to disabled regardless of whether selection is enabled?

[Finnur, 2017/04/03 17:30:29]

Yes, I don't know why that condition is there.

+            fgColorId = R.color.file_picker_special_tile_disabled_color;
+        }
+
+        mBorderView.setBackgroundColor(ContextCompat.getColor(mContext, bgColorId));

[Theresa, 2017/03/31 18:05:56]

Typically we use ApiCompatibilityUtils#getColor()

[Finnur, 2017/04/03 17:30:29]

Done.

+        mSpecialTile.setTextColor(ContextCompat.getColor(mContext, fgColorId));
+        Drawable[] drawables = mSpecialTile.getCompoundDrawables();
+        // The textview only has a top compound drawable (2nd element).
+        if (drawables[1] != null) {
+            int color = ContextCompat.getColor(mContext, fgColorId);
+            drawables[1].setColorFilter(color, PorterDuff.Mode.SRC_IN);
+        }
+    }
+}