Update the AppBannerView appearance again

Update the AppBannerView appearance again

* Add in "pressed" states for the banner and button.
* Update the resources with new ones.
* Change dimensions to account for email conversations with UX folks.
* Redo the measurement algorithm to make it easier to add even more
  widgets.

BUG=341556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253419

[gone, 2014-02-25 02:04:42]

Another round of adding UX requests.

[newt (away), 2014-02-25 19:22:27]

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/drawable/app_banner_button_install.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/drawable/app_banner_button_install.xml:9: <item
android:drawable="@drawable/app_banner_button_install_pressed"
I believe you may be able to inline app_banner_button_install_default.xml and
app_banner_button_install_pressed.xml into this file. Like so:

    <selector xmlns:android="http://schemas.android.com/apk/res/android">
        <item android:state_pressed="true">
            <shape><solid android:color="#b2cb39" /></shape>
        </item>
        <item android:state_pressed="true">
            <shape><solid android:color="#d1dd94" /></shape>
        </item>
    </selector>

Not tested, but I know that sort of nesting works with <layer-list> elements.
Another option is:

    <selector xmlns:android="http://schemas.android.com/apk/res/android">
        <item android:state_pressed="true"
android:drawable="@color/my_favorite_color" />
        <item android:state_pressed="true"
android:drawable="@color/your_favorite_color" />
    </selector>

Or, for complete, you could use a ColorStateList
(http://developer.android.com/guide/topics/resources/color-list-resource.html)

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/layout/app_banner_view.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/layout/app_banner_view.xml:71:
android:layout_width="fill_parent"
match_parent (fill_parent was deprecated in the early 2000's)

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/values/colors.xml:19: <color
name="app_banner_open_button_fg">#777777</color>
you can also write #fff, #777, and #3999

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
(right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:44:
* the margins are calculated by deducting the background Drawable's padding from
the margins
could you just do this subtraction now and change the values in the XML files?
Is there any reason this needs to happen at run-time?

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:398:
// Enforce a maximum width on the banner, which is defined as the smallest
between:
"between" -> "of"

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:408:
// Bind the height to provide a reasonable aspect ratio.
"Bind" -> "Constrain" or "Limit"?

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:436:
int biggestStackHeight =
what if this is bigger than maxHeight?

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:556:
if (childWidth > 0) availableWidth = Math.min(availableWidth, childWidth);
technically ">" should be ">="

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:583:
return ApiCompatibilityUtils.getMarginStart(p) +
ApiCompatibilityUtils.getMarginEnd(p);
Since this case is symmetric, I'd just use p.leftMargin + p.rightMargin

[gone, 2014-02-25 20:26:11]

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/drawable/app_banner_button_install.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/drawable/app_banner_button_install.xml:9: <item
android:drawable="@drawable/app_banner_button_install_pressed"
On 2014/02/25 19:22:28, newt wrote:
> I believe you may be able to inline app_banner_button_install_default.xml and
> app_banner_button_install_pressed.xml into this file. Like so:
> 
>     <selector xmlns:android="http://schemas.android.com/apk/res/android">
>         <item android:state_pressed="true">
>             <shape><solid android:color="#b2cb39" /></shape>
>         </item>
>         <item android:state_pressed="true">
>             <shape><solid android:color="#d1dd94" /></shape>
>         </item>
>     </selector>
> 
> Not tested, but I know that sort of nesting works with <layer-list> elements.
> Another option is:
> 
>     <selector xmlns:android="http://schemas.android.com/apk/res/android">
>         <item android:state_pressed="true"
> android:drawable="@color/my_favorite_color" />
>         <item android:state_pressed="true"
> android:drawable="@color/your_favorite_color" />
>     </selector>
> 
> Or, for complete, you could use a ColorStateList
> (http://developer.android.com/guide/topics/resources/color-list-resource.html)

Went with the former since the mock seems to keep changing.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/layout/app_banner_view.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/layout/app_banner_view.xml:71:
android:layout_width="fill_parent"
On 2014/02/25 19:22:28, newt wrote:
> match_parent (fill_parent was deprecated in the early 2000's)

I'm old school.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/values/colors.xml:19: <color
name="app_banner_open_button_fg">#777777</color>
On 2014/02/25 19:22:28, newt wrote:
> you can also write #fff, #777, and #3999

I kind of like the explicitness of having all the characters written out, since
all the other ones do it, too.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
(right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:44:
* the margins are calculated by deducting the background Drawable's padding from
the margins
On 2014/02/25 19:22:28, newt wrote:
> could you just do this subtraction now and change the values in the XML files?
> Is there any reason this needs to happen at run-time?

Maybe?  Depends on whether we can guarantee that new assets won't change how
large their drop shadows are.  It's also complicated a bit by the fact that the
Nexus 7 uses different margins, depending on the screen orientation.  If you
don't feel too strongly I'd rather keep it the way it is.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:398:
// Enforce a maximum width on the banner, which is defined as the smallest
between:
On 2014/02/25 19:22:28, newt wrote:
> "between" -> "of"

English!

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:408:
// Bind the height to provide a reasonable aspect ratio.
On 2014/02/25 19:22:28, newt wrote:
> "Bind" -> "Constrain" or "Limit"?

Switched to constrain.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:436:
int biggestStackHeight =
On 2014/02/25 19:22:28, newt wrote:
> what if this is bigger than maxHeight?

Not a problem if you look at "maxHeight" as a "maxHeightSuggestion"!  But yeah,
worst case scenario is that the banner ends up being taller than maxHeight, but
where each of the individual controls is prevented from being freakishly huge. 
The problem is that each of the controls is defined to have a specific size --
the only things that can ruin this are the app title and the install button text
since the strings can be really tall if the user sets it up that way.

I've tried to redo this code a bit so that it constrains each individual control
rather than the whole banner.  Let me know if it looks odd.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:556:
if (childWidth > 0) availableWidth = Math.min(availableWidth, childWidth);
On 2014/02/25 19:22:28, newt wrote:
> technically ">" should be ">="

Done.

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:583:
return ApiCompatibilityUtils.getMarginStart(p) +
ApiCompatibilityUtils.getMarginEnd(p);
On 2014/02/25 19:22:28, newt wrote:
> Since this case is symmetric, I'd just use p.leftMargin + p.rightMargin

Done.

[newt (away), 2014-02-25 20:46:04]

LGTM

Let's install some appz!

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/60001/chrome/android/ja...
chrome/android/java/res/values/colors.xml:19: <color
name="app_banner_open_button_fg">#777777</color>
On 2014/02/25 20:26:11, dfalcantara wrote:
> On 2014/02/25 19:22:28, newt wrote:
> > you can also write #fff, #777, and #3999
> 
> I kind of like the explicitness of having all the characters written out,
since
> all the other ones do it, too.

Sounds good to me.

[gone, 2014-02-25 20:47:23]

Ted for OWNERS

[Ted C, 2014-02-25 21:17:36]

lgtm w/ potential cleanup...maybe

https://chromiumcodereview.appspot.com/177863008/diff/80001/chrome/android/ja...
File chrome/android/java/res/layout/app_banner_view.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/80001/chrome/android/ja...
chrome/android/java/res/layout/app_banner_view.xml:67: <View
Instead of this view, can you use
android:foreground="@color/app_banner_card_highlight" on the AppBannerView?

[gone, 2014-02-25 22:30:13]

https://chromiumcodereview.appspot.com/177863008/diff/80001/chrome/android/ja...
File chrome/android/java/res/layout/app_banner_view.xml (right):

https://chromiumcodereview.appspot.com/177863008/diff/80001/chrome/android/ja...
chrome/android/java/res/layout/app_banner_view.xml:67: <View
On 2014/02/25 21:17:36, Ted C wrote:
> Instead of this view, can you use
> android:foreground="@color/app_banner_card_highlight" on the AppBannerView?

Has the unfortunate side effect of highlighting even the drop shadow.

[gone, 2014-02-25 22:32:23]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-02-25 22:51:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/177863008/80001

[commit-bot: I haz the power, 2014-02-25 22:51:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 22:51:29]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
  Hunk #5 FAILED at 136.
  1 out of 13 hunks FAILED -- saving rejects to file
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java.rej

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
b/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
index
32c2070944b3f71bdfb4e714c0c5a7de76db0fe6..3a302520a668e9696fb1e467158bd5eaa828f82b
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerView.java
@@ -8,10 +8,10 @@ import android.animation.ObjectAnimator;
 import android.content.Context;
 import android.content.res.Configuration;
 import android.content.res.Resources;
-import android.graphics.Point;
 import android.graphics.Rect;
 import android.util.AttributeSet;
 import android.view.LayoutInflater;
+import android.view.MotionEvent;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.Button;
@@ -25,9 +25,29 @@ import org.chromium.ui.base.LocalizationUtils;
 
 /**
  * Lays out a banner for showing info about an app on the Play Store.
- * Rather than utilizing the Android RatingBar, which would require some nasty
styling, a custom
- * View is used to paint a Drawable showing all the stars.  Showing different
ratings is done by
- * adjusting the clipping rectangle width.
+ * The banner mimics the appearance of a Google Now card using a background
Drawable with a shadow.
+ *
+ * PADDING CALCULATIONS
+ * The banner has three different types of padding that need to be accounted
for:
+ * 1) The background Drawable of the banner looks like card with a drop shadow.
 The Drawable
+ *    defines a padding around the card that solely encompasses the space
occupied by the drop
+ *    shadow.
+ * 2) The card itself needs to have padding so that the widgets don't abut the
borders of the card.
+ *    This is defined as mPaddingCard, and is equally applied to all four
sides.
+ * 3) Controls other than the icon are further constrained by mPaddingControls,
which applies only
+ *    to the bottom and end margins.
+ * See {@link #AppBannerView.onMeasure(int, int)} for details.
+ *
+ * MARGIN CALCULATIONS
+ * Margin calculations for the banner are complicated by the background
Drawable's drop shadows,
+ * since the drop shadows are meant to be counted as being part of the margin. 
To deal with this,
+ * the margins are calculated by deducting the background Drawable's padding
from the margins
+ * defined by the XML files.
+ *
+ * EVEN MORE LAYOUT QUIRKS
+ * The layout of the banner, which includes its widget sizes, may change when
the screen is rotated
+ * to account for less screen real estate.  This means that all of the View's
widgets and cached
+ * dimensions must be rebuilt from scratch.
  */
 public class AppBannerView extends SwipableOverlayView implements
View.OnClickListener {
     /**
@@ -53,9 +73,15 @@ public class AppBannerView extends SwipableOverlayView
implements View.OnClickLi
         public void onBannerClicked(AppBannerView banner);
     }
 
+    // Number of milliseconds to wait before showing the card highlight.
+    private static final long MS_HIGHLIGHT_APPEARANCE = 150;
+
     // XML layout for the BannerView.
     private static final int BANNER_LAYOUT = R.layout.app_banner_view;
 
+    // Maximum distance the finger can travel before dismissing the highlight.
+    private static final float HIGHLIGHT_DISTANCE = 20;
+
     // True if the layout is in left-to-right layout mode (regular mode).
     private final boolean mIsLayoutLTR;
 
@@ -67,23 +93,24 @@ public class AppBannerView extends SwipableOverlayView
implements View.OnClickLi
     private TextView mTitleView;
     private Button mButtonView;
     private RatingView mRatingView;
-    private ImageView mLogoView;
+    private View mLogoView;
+    private View mBannerHighlightView;
 
     // Information about the package.
     private AppData mAppData;
 
-    // Variables used during layout calculations and saved to avoid
reallocations.
-    private final Point mSpaceMain;
-    private final Point mSpaceForLogo;
-    private final Point mSpaceForRating;
-    private final Point mSpaceForTitle;
-
     // Dimension values.
     private int mDefinedMaxWidth;
-    private int mPaddingContent;
-    private int mMarginSide;
+    private int mPaddingCard;
+    private int mPaddingControls;
+    private int mMarginLeft;
+    private int mMarginRight;
     private int mMarginBottom;
 
+    // Highlight variables.
+    private boolean mIsBannerPressed;
+    private float mInitialXForHighlight;
+
     // Initial padding values.
     private final Rect mBackgroundDrawablePadding;
 
@@ -109,10 +136,6 @@ public class AppBannerView extends SwipableOverlayView
implements View.OnClickLi
     public AppBannerView(Context context, AttributeSet attrs) {
         super(context, attrs);
         mIsLayoutLTR = !LocalizationUtils.isSystemLayoutDirectionRtl();
-        mSpaceMain = new Point();
-        mSpaceForLogo = new Point();
-        mSpaceForRating = new Point();
-        mSpaceForTitle = new Point();
 
         // Store the background Drawable's padding.  The background used for
banners is a 9-patch,
         // which means that it already defines padding.  We need to take it
into account when adding
@@ -137,41 +160,38 @@ public class AppBannerView extends SwipableOverlayView
implements View.OnClickLi
 
     private void initializeControls() {
         // Cache the banner dimensions, adjusting margins for drop shadows
defined in the background
-        // Drawable.  The Drawable is defined to have the same margin on both
the left and right.
+        // Drawable.
         Resources res = getResources();
         mDefinedMaxWidth =
res.getDimensionPixelSize(R.dimen.app_banner_max_width);
-        mPaddingContent =
res.getDimensionPixelSize(R.dimen.app_banner_padding_content);
-        mMarginSide =
res.getDimensionPixelSize(R.dimen.app_banner_margin_sides)
+        mPaddingCard = res.getDimensionPixelSize(R.dimen.app_banner_padding);
+        mPaddingControls =
res.getDimensionPixelSize(R.dimen.app_banner_padding_controls);
+        mMarginLeft =
res.getDimensionPixelSize(R.dimen.app_banner_margin_sides)
                 - mBackgroundDrawablePadding.left;
+        mMarginRight =
res.getDimensionPixelSize(R.dimen.app_banner_margin_sides)
+                - mBackgroundDrawablePadding.right;
         mMarginBottom =
res.getDimensionPixelSize(R.dimen.app_banner_margin_bottom)
                 - mBackgroundDrawablePadding.bottom;
         if (getLayoutParams() != null) {
             MarginLayoutParams params = (MarginLayoutParams) getLayoutParams();
-            params.leftMargin = mMarginSide;
-            params.rightMargin = mMarginSide;
+            params.leftMargin = mMarginLeft;
+            params.rightMargin = mMarginRight;
             params.bottomMargin = mMarginBottom;
         }
 
-        // Add onto the padding defined by the Drawable's drop shadow.
-        int padding = res.getDimensionPixelSize(R.dimen.app_banner_padding);
-        int paddingStart = mBackgroundDrawablePadding.left + padding;
-        int paddingTop = mBackgroundDrawablePadding.top + padding;
-        int paddingEnd = mBackgroundDrawablePadding.right + padding;
-        int paddingBottom = mBackgroundDrawablePadding.bottom + padding;
-        ApiCompatibilityUtils.setPaddingRelative(
-                this, paddingStart, paddingTop, paddingEnd, paddingBottom);
-
         // Pull out all of the controls we are expecting.
         mIconView = (ImageView) findViewById(R.id.app_icon);
         mTitleView = (TextView) findViewById(R.id.app_title);
         mButtonView = (Button) findViewById(R.id.app_install_button);
         mRatingView = (RatingView) findViewById(R.id.app_rating);
-        mLogoView = (ImageView) findViewById(R.id.store_logo);
+        mLogoView = findViewById(R.id.store_logo);
+        mBannerHighlightView = findViewById(R.id.banner_highlight);
+
         assert mIconView != null;
         assert mTitleView != null;
         assert mButtonView != null;
         assert mLogoView != null;
         assert mRatingView != null;
+        assert mBannerHighlightView != null;
 
         // Set up the button to fire an event.
         mButtonView.setOnClickListener(this);
@@ -199,7 +219,7 @@ public class AppBannerView extends SwipableOverlayView
implements View.OnClickLi
     protected ViewGroup.MarginLayoutParams createLayoutParams() {
         // Define the margin around the entire banner that accounts for the
drop shadow.
         ViewGroup.MarginLayoutParams params = super.createLayoutParams();
-        params.setMargins(mMarginSide, 0, mMarginSide, mMarginBottom);
+        params.setMargins(mMarginLeft, 0, mMarginRight, mMarginBottom);
         return params;
     }
 
@@ -228,24 +248,25 @@ public class AppBannerView extends SwipableOverlayView
implements View.OnClickLi
     void updateButtonState() {
         if (mButtonView == null) return;
 
-        int bgColor;
+        Resources res = getResources();
         int fgColor;
         String text;
         if (mAppData.installState() == AppData.INSTALL_STATE_INSTALLED) {
-            bgColor =
getResources().getColor(R.color.app_banner_open_button_bg);
-            fgColor =
getResources().getColor(R.color.app_banner_open_button_fg);
-            text = getResources().getString(R.string.app_banner_open);
+            ApiCompatibilityUtils.s…
(message too large)

[gone, 2014-02-25 22:59:55]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2014-02-25 23:51:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/177863008/100001

[gone, 2014-02-25 23:54:33]

On 2014/02/25 23:51:18, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/dfalcantara%40chromium.org/177863008/1...

Bleh, it's not happy about the resource changes.  Might need to fork them out.

[Paweł Hajdan Jr., 2014-02-26 05:44:24]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 06:00:40]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:16:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/177863008/100001

[commit-bot: I haz the power, 2014-02-26 12:03:16]

Change committed as 253419