POC: Use a tint list to change colors

POC: Use a tint list to change colors

DO NOT SUMMIT

[ortuno, 2017-03-08 05:55:41]

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:389:
mConnectedIcon.getConstantState().newDrawable().mutate(),
Creating a new Drawable for each item seemed really inefficient.
Because we create a new drawable for each item we can no longer test that the
right icon was set. See BluetoothChooserDialogTest.

Replacing this with mConnectedIcon.mutate() would cause the state to be shared
across all instances of the icon.

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:365:
row.mImageView.setImageDrawable(null);
This seemed really hacky and prone to error. If anyone ever removes this line
only manual testing will show that the behavior has changed.

[Ted C, 2017-03-08 20:54:02]

tedchoc@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Ted C, 2017-03-08 20:54:03]

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:389:
mConnectedIcon.getConstantState().newDrawable().mutate(),
On 2017/03/08 05:55:41, ortuno wrote:
> Creating a new Drawable for each item seemed really inefficient.
> Because we create a new drawable for each item we can no longer test that the
> right icon was set. See BluetoothChooserDialogTest.
> 
> Replacing this with mConnectedIcon.mutate() would cause the state to be shared
> across all instances of the icon.

mutate() still often shares the expensive data structures, but it just ensures
the state isn't shared, so I don't think this is as expensive as you think.

When you say the state is shared across all instances, what do you mean?  Like
if you select one it selects the other?

That is surprising as it Drawable#mutate states that it should create a new
constant state.  I wonder if this is a bug in VectorDrawableCompat that it isn't
doing this.

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
row.mImageView.setImageDrawable(icon.mIcon);
If you were to do row.mImageView.jumpToDrawableState(); would that address it?

You definitely shouldn't have to set it to null for this to get reflected, so
there is something wonky going on.  When you say the drawable state isn't
reflected, are you seeing the tint not being updated to the state you expect?

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:252:
assertTrue(itemAdapter.getItem(1).hasSameContents("id-2", "Name 2",
connectedIcon));
Again, I don't think we need to verify the icon, we should just verify the row
(and potentially the drawable state) is set to selected as it is purely up to
android at that point to do the right thing.

[ortuno, 2017-03-08 23:23:59]

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:389:
mConnectedIcon.getConstantState().newDrawable().mutate(),
On 2017/03/08 at 20:54:03, Ted C wrote:
> On 2017/03/08 05:55:41, ortuno wrote:
> > Creating a new Drawable for each item seemed really inefficient.
> > Because we create a new drawable for each item we can no longer test that
the
> > right icon was set. See BluetoothChooserDialogTest.
> > 
> > Replacing this with mConnectedIcon.mutate() would cause the state to be
shared
> > across all instances of the icon.
> 
> mutate() still often shares the expensive data structures, but it just ensures
the state isn't shared, so I don't think this is as expensive as you think.
> 
> When you say the state is shared across all instances, what do you mean?  Like
if you select one it selects the other?
> 

Correct. In a list with two items with icons selecting the second item results
in both items changing color. Selecting the first item results in no change of
color of either item.

> That is surprising as it Drawable#mutate states that it should create a new
constant state.  I wonder if this is a bug in VectorDrawableCompat that it isn't
doing this.

My understanding based on some experimentation when writing tests for the other
patch was that given two different Drawable objects that share the same state,
mutate() would ensure that they no longer share the state:

Drawable d1 = res.getDrawable(R.drawable.ic_bluetooth_connected, null);
Drawable d2 = res.getDrawable(R.drawable.ic_bluetooth_connected, null);
// d1 and d2 share the same DrawableConstantState. Any changes to d2 would be
// reflected on d1 and vice versa.

d2.mutate();
// d1 and d2 no longer share the same DrawableConstantState. Changes in d2 would
// not be reflected on d1 and vice versa.

But if we call mutate() on the same object, which I believe is what we would be
doing, it wouldn't have any effect:

Drawable d1 = res.getDrawable(R.drawable.ic_bluetooth_connected, null);
Drawable d2 = d1.mutate();
// Any Drawables that shared state with d1/d2 will no longer share its state
with them but d1/d2
// still have the same DrawableConstantState.

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
row.mImageView.setImageDrawable(icon.mIcon);
On 2017/03/08 at 20:54:03, Ted C wrote:
> If you were to do row.mImageView.jumpToDrawableState(); would that address it?
> 
> You definitely shouldn't have to set it to null for this to get reflected, so
there is something wonky going on.  When you say the drawable state isn't
reflected, are you seeing the tint not being updated to the state you expect?

The behavior is rather weird as if the state is not propagated to the
ImageView's drawable:

1. See item with icon
2. Tap on item to select it -> Nothing happens
3. Wait a couple of minutes or scroll all the way down the list -> Icon's color
changes O.O

I tried with jumpDrawablesToCurrentState() and invalidateDrawable() neither
worked.

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:252:
assertTrue(itemAdapter.getItem(1).hasSameContents("id-2", "Name 2",
connectedIcon));
On 2017/03/08 at 20:54:03, Ted C wrote:
> Again, I don't think we need to verify the icon, we should just verify the row
(and potentially the drawable state) is set to selected as it is purely up to
android at that point to do the right thing.

This doesn't test the selected state. It tests that the logic inside
BluetoothChooserDialog.addOrUpdateDevice[1] is correct i.e.:

if (isGATTConnected) {
  // .. set icon
}

Follow up patches will add more icons and this logic will be more complex so I
think it makes sense to test it.

https://codereview.chromium.org/2704263004/diff/100001/chrome/android/java/sr...

[Ted C, 2017-03-09 17:03:26]

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:389:
mConnectedIcon.getConstantState().newDrawable().mutate(),
On 2017/03/08 23:23:58, ortuno wrote:
> On 2017/03/08 at 20:54:03, Ted C wrote:
> > On 2017/03/08 05:55:41, ortuno wrote:
> > > Creating a new Drawable for each item seemed really inefficient.
> > > Because we create a new drawable for each item we can no longer test that
> the
> > > right icon was set. See BluetoothChooserDialogTest.
> > > 
> > > Replacing this with mConnectedIcon.mutate() would cause the state to be
> shared
> > > across all instances of the icon.
> > 
> > mutate() still often shares the expensive data structures, but it just
ensures
> the state isn't shared, so I don't think this is as expensive as you think.
> > 
> > When you say the state is shared across all instances, what do you mean? 
Like
> if you select one it selects the other?
> > 
> 
> Correct. In a list with two items with icons selecting the second item results
> in both items changing color. Selecting the first item results in no change of
> color of either item.
> 
> > That is surprising as it Drawable#mutate states that it should create a new
> constant state.  I wonder if this is a bug in VectorDrawableCompat that it
isn't
> doing this.
> 
> My understanding based on some experimentation when writing tests for the
other
> patch was that given two different Drawable objects that share the same state,
> mutate() would ensure that they no longer share the state:
> 
> Drawable d1 = res.getDrawable(R.drawable.ic_bluetooth_connected, null);
> Drawable d2 = res.getDrawable(R.drawable.ic_bluetooth_connected, null);
> // d1 and d2 share the same DrawableConstantState. Any changes to d2 would be
> // reflected on d1 and vice versa.
> 
> d2.mutate();
> // d1 and d2 no longer share the same DrawableConstantState. Changes in d2
would
> // not be reflected on d1 and vice versa.
> 
> But if we call mutate() on the same object, which I believe is what we would
be
> doing, it wouldn't have any effect:
> 
> Drawable d1 = res.getDrawable(R.drawable.ic_bluetooth_connected, null);
> Drawable d2 = d1.mutate();
> // Any Drawables that shared state with d1/d2 will no longer share its state
> with them but d1/d2
> // still have the same DrawableConstantState.

Ah yes, you are 100% right.  I was incorrectly thinking we were already creating
new objects for each, but that was wrong.

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
row.mImageView.setImageDrawable(icon.mIcon);
On 2017/03/08 23:23:58, ortuno wrote:
> On 2017/03/08 at 20:54:03, Ted C wrote:
> > If you were to do row.mImageView.jumpToDrawableState(); would that address
it?
> > 
> > You definitely shouldn't have to set it to null for this to get reflected,
so
> there is something wonky going on.  When you say the drawable state isn't
> reflected, are you seeing the tint not being updated to the state you expect?
> 
> The behavior is rather weird as if the state is not propagated to the
> ImageView's drawable:
> 
> 1. See item with icon
> 2. Tap on item to select it -> Nothing happens
> 3. Wait a couple of minutes or scroll all the way down the list -> Icon's
color
> changes O.O
> 
> I tried with jumpDrawablesToCurrentState() and invalidateDrawable() neither
> worked.

Tapping on an item results in notifyDataSetChanged?  I wouldn't necessarily have
expected this function to be called at all on tap (scrolling would because
eventually the list items would need to be rebuilt).

I'd probably just create an quick extension to ImageView that logs what drawable
states are being set to verify.  Something seems weird here in terms of state
being applied and I'd be inclined to think our tint list isn't correct or the
list view isn't applying the state we expect.

[ortuno, 2017-03-10 00:38:49]

So to sum up I had three concerns with this approach:

1. Some hacky calls to make it work correctly.
2. Making new Drawables seemed expensive and somewhat unnecessary.
3. No way to test that BluetoothChooserDialog was passing the right icon to
ItemChooserDialog.

We figured out how to solve 1. and you mentioned that creating new Drawables
isn't that expensive so no need to worry about 2.

That leaves us with 3. What do you think of the following approach:

Introduce a new BluetoothIconProvider member with (initially) one function:

public class BluetoothIconProvider {
  Drawable mConnectedIcon;
  public BluetoothIconProvider(Drawable connectedIcon) {
    mConnectedIcon = connectedIcon;
  }

  Drawable getConnectedIcon() {
    return mConnectedIcon.getConstantState().newDrawable().mutate();
  }
}

And then in tests we would use:

public class BluetoothIconProviderTest extends BluetoothIconProvider {
  Drawable mConnectedIcon;
  public BluetoothIconProviderTest(Drawable connectedIcon) {
    mConnectedIcon = connectedIcon;
  }
  
  @Override
  Drawable getConnectedIcon() {
    return mConnectedIcon;
  }
}

And BluetoothChooserDialog:

void addOrUpdateDevice(...) {
  Drawable icon;
  if (isGATTConnected) {
    icon = mIconProvider.getConnectedIcon();
  }
  mItemChooserDialog.addOrUpdateItem(deviceId, deviceName, icon, iconDesc);
}

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
row.mImageView.setImageDrawable(icon.mIcon);
On 2017/03/09 at 17:03:26, Ted C wrote:
> On 2017/03/08 23:23:58, ortuno wrote:
> > On 2017/03/08 at 20:54:03, Ted C wrote:
> > > If you were to do row.mImageView.jumpToDrawableState(); would that address
it?
> > > 
> > > You definitely shouldn't have to set it to null for this to get reflected,
so
> > there is something wonky going on.  When you say the drawable state isn't
> > reflected, are you seeing the tint not being updated to the state you
expect?
> > 
> > The behavior is rather weird as if the state is not propagated to the
> > ImageView's drawable:
> > 
> > 1. See item with icon
> > 2. Tap on item to select it -> Nothing happens
> > 3. Wait a couple of minutes or scroll all the way down the list -> Icon's
color
> > changes O.O
> > 
> > I tried with jumpDrawablesToCurrentState() and invalidateDrawable() neither
> > worked.
> 
> Tapping on an item results in notifyDataSetChanged?

This was it! We were calling notifyDataSetChanged() in onItemClick()[1] because
older code used to manually set the color. I guess the state wasn't being set
into the new drawable. Removing that line fixed the issue. I think it's
interesting that the TextView which also uses a tint list didn't suffer from
this.

[1]
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

>  I wouldn't necessarily have expected this function to be called at all on tap
(scrolling would because eventually the list items would need to be rebuilt).
> 
> I'd probably just create an quick extension to ImageView that logs what
drawable states are being set to verify.  Something seems weird here in terms of
state being applied and I'd be inclined to think our tint list isn't correct or
the list view isn't applying the state we expect.

[Ted C, 2017-03-10 05:10:55]

On 2017/03/10 00:38:49, ortuno wrote:
> So to sum up I had three concerns with this approach:
> 
> 1. Some hacky calls to make it work correctly.
> 2. Making new Drawables seemed expensive and somewhat unnecessary.
> 3. No way to test that BluetoothChooserDialog was passing the right icon to
> ItemChooserDialog.
> 
> We figured out how to solve 1. and you mentioned that creating new Drawables
> isn't that expensive so no need to worry about 2.
> 
> That leaves us with 3. What do you think of the following approach:
> 
> Introduce a new BluetoothIconProvider member with (initially) one function:
> 
> public class BluetoothIconProvider {
>   Drawable mConnectedIcon;
>   public BluetoothIconProvider(Drawable connectedIcon) {
>     mConnectedIcon = connectedIcon;
>   }
> 
>   Drawable getConnectedIcon() {
>     return mConnectedIcon.getConstantState().newDrawable().mutate();
>   }
> }
> 
> And then in tests we would use:
> 
> public class BluetoothIconProviderTest extends BluetoothIconProvider {
>   Drawable mConnectedIcon;
>   public BluetoothIconProviderTest(Drawable connectedIcon) {
>     mConnectedIcon = connectedIcon;
>   }
>   
>   @Override
>   Drawable getConnectedIcon() {
>     return mConnectedIcon;
>   }
> }
> 
> And BluetoothChooserDialog:
> 
> void addOrUpdateDevice(...) {
>   Drawable icon;
>   if (isGATTConnected) {
>     icon = mIconProvider.getConnectedIcon();
>   }
>   mItemChooserDialog.addOrUpdateItem(deviceId, deviceName, icon, iconDesc);
> }

I'd still argue that you are testing too many things in this case.  Why not have
a static function
static @DrawableRes int getDrawableResourceId(BluetoothStateyThings) {
}

Then you could isolate the logic that picks the resource ID.

The test it sounds like you want to add is actually testing the icon setting
behavior on
the generic item chooser dialog itself, and I'd rather see tests specifically
around that to
ensure icons are updated when you call things, etc...  Then the bluetooth code
just
tests the code path that chooses the icon.

Now if the goal is to verify that an native update triggers a refresh with a new
icon
specified, then I wonder if you could just extend
ItemChooserDialog.ItemChooserRowIcon
to include the resource id.  Then in tests get the row icon from the underlying
dialog
cast it to your inner class and verify the id.  You could also look at
overriding BluetoothChooserDialog
in the tests like:

TestBluetoothChooserDialog extends BluetoothChooserDialog {
    ItemChooserDialog.ItemChooserRowIcon createConnectedRowIcon() {
        // do code you can mock here where you return a constant drawable.
    }
}

Then you could test the flow where you expect an updated icon and you can verify
the
underlying icon.  I'd be tempted to just overwriting in tests as we do try to
not introduce too
much indirection in code that exists to test one small thing, but I don't feel
too strongly here.

> 
>
https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
> File
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
> (right):
> 
>
https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
> row.mImageView.setImageDrawable(icon.mIcon);
> On 2017/03/09 at 17:03:26, Ted C wrote:
> > On 2017/03/08 23:23:58, ortuno wrote:
> > > On 2017/03/08 at 20:54:03, Ted C wrote:
> > > > If you were to do row.mImageView.jumpToDrawableState(); would that
address
> it?
> > > > 
> > > > You definitely shouldn't have to set it to null for this to get
reflected,
> so
> > > there is something wonky going on.  When you say the drawable state isn't
> > > reflected, are you seeing the tint not being updated to the state you
> expect?
> > > 
> > > The behavior is rather weird as if the state is not propagated to the
> > > ImageView's drawable:
> > > 
> > > 1. See item with icon
> > > 2. Tap on item to select it -> Nothing happens
> > > 3. Wait a couple of minutes or scroll all the way down the list -> Icon's
> color
> > > changes O.O
> > > 
> > > I tried with jumpDrawablesToCurrentState() and invalidateDrawable()
neither
> > > worked.
> > 
> > Tapping on an item results in notifyDataSetChanged?
> 
> This was it! We were calling notifyDataSetChanged() in onItemClick()[1]
because
> older code used to manually set the color. I guess the state wasn't being set
> into the new drawable. Removing that line fixed the issue. I think it's
> interesting that the TextView which also uses a tint list didn't suffer from
> this.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> >  I wouldn't necessarily have expected this function to be called at all on
tap
> (scrolling would because eventually the list items would need to be rebuilt).
> > 
> > I'd probably just create an quick extension to ImageView that logs what
> drawable states are being set to verify.  Something seems weird here in terms
of
> state being applied and I'd be inclined to think our tint list isn't correct
or
> the list view isn't applying the state we expect.

[Ted C, 2017-03-10 05:11:03]

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
row.mImageView.setImageDrawable(icon.mIcon);
On 2017/03/10 00:38:49, ortuno wrote:
> On 2017/03/09 at 17:03:26, Ted C wrote:
> > On 2017/03/08 23:23:58, ortuno wrote:
> > > On 2017/03/08 at 20:54:03, Ted C wrote:
> > > > If you were to do row.mImageView.jumpToDrawableState(); would that
address
> it?
> > > > 
> > > > You definitely shouldn't have to set it to null for this to get
reflected,
> so
> > > there is something wonky going on.  When you say the drawable state isn't
> > > reflected, are you seeing the tint not being updated to the state you
> expect?
> > > 
> > > The behavior is rather weird as if the state is not propagated to the
> > > ImageView's drawable:
> > > 
> > > 1. See item with icon
> > > 2. Tap on item to select it -> Nothing happens
> > > 3. Wait a couple of minutes or scroll all the way down the list -> Icon's
> color
> > > changes O.O
> > > 
> > > I tried with jumpDrawablesToCurrentState() and invalidateDrawable()
neither
> > > worked.
> > 
> > Tapping on an item results in notifyDataSetChanged?
> 
> This was it! We were calling notifyDataSetChanged() in onItemClick()[1]
because
> older code used to manually set the color. I guess the state wasn't being set
> into the new drawable. Removing that line fixed the issue. I think it's
> interesting that the TextView which also uses a tint list didn't suffer from
> this.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> >  I wouldn't necessarily have expected this function to be called at all on
tap
> (scrolling would because eventually the list items would need to be rebuilt).
> > 
> > I'd probably just create an quick extension to ImageView that logs what
> drawable states are being set to verify.  Something seems weird here in terms
of
> state being applied and I'd be inclined to think our tint list isn't correct
or
> the list view isn't applying the state we expect.

Are you saying that removing notifyDataSetChanged solved this issue?   That is
actually the opposite of what I expected to be honest.

If removing notifyDataSetChanged solves the problem, it might come back if you
were to select something and scroll the list to where the item was off screen
and then back on (where the views would be regenerated), so the items might be
wrong at that point.

Maybe we need to be calling:
https://developer.android.com/reference/android/view/View.html#setSelected(bo...

On the image view in this case if it is part of the selected row.

You "should" be able to call notifyDataSetChanged many times (it would be very
inefficient if you did), but it would be regenerate all the views in their
current state.

[ortuno, 2017-03-13 06:13:23]

Seems like we resolved all my concerns! Thanks! I'll update the other patch.

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
row.mImageView.setImageDrawable(icon.mIcon);
On 2017/03/10 at 05:11:03, Ted C wrote:
> On 2017/03/10 00:38:49, ortuno wrote:
> > On 2017/03/09 at 17:03:26, Ted C wrote:
> > > On 2017/03/08 23:23:58, ortuno wrote:
> > > > On 2017/03/08 at 20:54:03, Ted C wrote:
> > > > > If you were to do row.mImageView.jumpToDrawableState(); would that
address
> > it?
> > > > > 
> > > > > You definitely shouldn't have to set it to null for this to get
reflected,
> > so
> > > > there is something wonky going on.  When you say the drawable state
isn't
> > > > reflected, are you seeing the tint not being updated to the state you
> > expect?
> > > > 
> > > > The behavior is rather weird as if the state is not propagated to the
> > > > ImageView's drawable:
> > > > 
> > > > 1. See item with icon
> > > > 2. Tap on item to select it -> Nothing happens
> > > > 3. Wait a couple of minutes or scroll all the way down the list ->
Icon's
> > color
> > > > changes O.O
> > > > 
> > > > I tried with jumpDrawablesToCurrentState() and invalidateDrawable()
neither
> > > > worked.
> > > 
> > > Tapping on an item results in notifyDataSetChanged?
> > 
> > This was it! We were calling notifyDataSetChanged() in onItemClick()[1]
because
> > older code used to manually set the color. I guess the state wasn't being
set
> > into the new drawable. Removing that line fixed the issue. I think it's
> > interesting that the TextView which also uses a tint list didn't suffer from
> > this.
> > 
> > [1]
> >
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > 
> > >  I wouldn't necessarily have expected this function to be called at all on
tap
> > (scrolling would because eventually the list items would need to be
rebuilt).
> > > 
> > > I'd probably just create an quick extension to ImageView that logs what
> > drawable states are being set to verify.  Something seems weird here in
terms of
> > state being applied and I'd be inclined to think our tint list isn't correct
or
> > the list view isn't applying the state we expect.
> 
> Are you saying that removing notifyDataSetChanged solved this issue?   That is
actually the opposite of what I expected to be honest.
> 
> If removing notifyDataSetChanged solves the problem, it might come back if you
were to select something and scroll the list to where the item was off screen
and then back on (where the views would be regenerated), so the items might be
wrong at that point.
> 

Right, the icon turned grey for a bit when I scrolled back really fast.

> Maybe we need to be calling:
>
https://developer.android.com/reference/android/view/View.html#setSelected(bo...
> 
> On the image view in this case if it is part of the selected row.
> 
> You "should" be able to call notifyDataSetChanged many times (it would be very
inefficient if you did), but it would be regenerate all the views in their
current state.

tldr; Calling setSelected fixed the problem.

So I added a couple log statements for ImageView#isSelected and
Drawable#getState().

Calling notifyDataSetChanged in onItemClicked():
 - The Drawable state doesn't change. Stays as state_enabled. Future calls to
notifyDataSetChange from us don't change the state but after a couple of seconds
there is another call to notifyDataSetChanged (unsure where it comes from) and
then the drawable state changes to state_active
 - ImageView#isSelected stays as false

Not calling notifyDataSetChanged in onItemClicked():
  - The Drawable state changes immediately to state_active
  - ImageView#isSelected stays as false

Calling notifyDataSetChanged in onItemClicked and calling ImageView#setSelected.
  - The drawable state immediately changes to state_selected
  - ImageView#isSelected becomes true

Seems live we've been incorrectly using state_activated instead of
state_selected.

I'll do the following:

1. Send out patch to use state_selected for the text color and call
TextView#setSelected. Should I add a test to make sure the view is marked as
selected?
2. Fix this patch to use state_selected for the icon color and call
ImageView#setSelected.

Does that sound good to you?

[Ted C, 2017-03-13 22:56:23]

On 2017/03/13 06:13:23, ortuno wrote:
> Seems like we resolved all my concerns! Thanks! I'll update the other patch.
> 
>
https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
> File
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
> (right):
> 
>
https://codereview.chromium.org/2738503007/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:366:
> row.mImageView.setImageDrawable(icon.mIcon);
> On 2017/03/10 at 05:11:03, Ted C wrote:
> > On 2017/03/10 00:38:49, ortuno wrote:
> > > On 2017/03/09 at 17:03:26, Ted C wrote:
> > > > On 2017/03/08 23:23:58, ortuno wrote:
> > > > > On 2017/03/08 at 20:54:03, Ted C wrote:
> > > > > > If you were to do row.mImageView.jumpToDrawableState(); would that
> address
> > > it?
> > > > > > 
> > > > > > You definitely shouldn't have to set it to null for this to get
> reflected,
> > > so
> > > > > there is something wonky going on.  When you say the drawable state
> isn't
> > > > > reflected, are you seeing the tint not being updated to the state you
> > > expect?
> > > > > 
> > > > > The behavior is rather weird as if the state is not propagated to the
> > > > > ImageView's drawable:
> > > > > 
> > > > > 1. See item with icon
> > > > > 2. Tap on item to select it -> Nothing happens
> > > > > 3. Wait a couple of minutes or scroll all the way down the list ->
> Icon's
> > > color
> > > > > changes O.O
> > > > > 
> > > > > I tried with jumpDrawablesToCurrentState() and invalidateDrawable()
> neither
> > > > > worked.
> > > > 
> > > > Tapping on an item results in notifyDataSetChanged?
> > > 
> > > This was it! We were calling notifyDataSetChanged() in onItemClick()[1]
> because
> > > older code used to manually set the color. I guess the state wasn't being
> set
> > > into the new drawable. Removing that line fixed the issue. I think it's
> > > interesting that the TextView which also uses a tint list didn't suffer
from
> > > this.
> > > 
> > > [1]
> > >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > > 
> > > >  I wouldn't necessarily have expected this function to be called at all
on
> tap
> > > (scrolling would because eventually the list items would need to be
> rebuilt).
> > > > 
> > > > I'd probably just create an quick extension to ImageView that logs what
> > > drawable states are being set to verify.  Something seems weird here in
> terms of
> > > state being applied and I'd be inclined to think our tint list isn't
correct
> or
> > > the list view isn't applying the state we expect.
> > 
> > Are you saying that removing notifyDataSetChanged solved this issue?   That
is
> actually the opposite of what I expected to be honest.
> > 
> > If removing notifyDataSetChanged solves the problem, it might come back if
you
> were to select something and scroll the list to where the item was off screen
> and then back on (where the views would be regenerated), so the items might be
> wrong at that point.
> > 
> 
> Right, the icon turned grey for a bit when I scrolled back really fast.
> 
> > Maybe we need to be calling:
> >
>
https://developer.android.com/reference/android/view/View.html#setSelected(bo...
> > 
> > On the image view in this case if it is part of the selected row.
> > 
> > You "should" be able to call notifyDataSetChanged many times (it would be
very
> inefficient if you did), but it would be regenerate all the views in their
> current state.
> 
> tldr; Calling setSelected fixed the problem.
> 
> So I added a couple log statements for ImageView#isSelected and
> Drawable#getState().
> 
> Calling notifyDataSetChanged in onItemClicked():
>  - The Drawable state doesn't change. Stays as state_enabled. Future calls to
> notifyDataSetChange from us don't change the state but after a couple of
seconds
> there is another call to notifyDataSetChanged (unsure where it comes from) and
> then the drawable state changes to state_active
>  - ImageView#isSelected stays as false
> 
> Not calling notifyDataSetChanged in onItemClicked():
>   - The Drawable state changes immediately to state_active
>   - ImageView#isSelected stays as false
> 
> Calling notifyDataSetChanged in onItemClicked and calling
ImageView#setSelected.
>   - The drawable state immediately changes to state_selected
>   - ImageView#isSelected becomes true

You'll likely want to call setSelected in notifyDataSetChanged as well
to ensure the views are recycled correctly.

> 
> Seems live we've been incorrectly using state_activated instead of
> state_selected.
> 
> I'll do the following:
> 
> 1. Send out patch to use state_selected for the text color and call
> TextView#setSelected. Should I add a test to make sure the view is marked as
> selected?
> 2. Fix this patch to use state_selected for the icon color and call
> ImageView#setSelected.
> 
> Does that sound good to you?

Sounds good to me!  We "might" want to keep state_activated and add
state_selected if the tint is shared among other things (or at least
confirm it doesn't break them).

Thanks for all the investigation work you did as well!