Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813703004:
chooser: Don't call mutate on new drawable (Closed)
Descriptionchooser: Don't call mutate on new drawable
Stops calling mutate() to avoid creating a new DrawableConstantState and uses
the constant state to decided if the icon needs updating.
Calling mutate() on new Drawables would cause a new DrawableConstantState to
be created for each Drawable. This in turn would cause notifyDataSetChanged to be
called for every update to the list.
BUG=543466
Review-Url: https://codereview.chromium.org/2813703004
Cr-Commit-Position: refs/heads/master@{#465073}
Committed: https://chromium.googlesource.com/chromium/src/+/483ae59e8536d954321f8c29851ca4310f3a33b5
Patch Set 1 #
Total comments: 10
Patch Set 2 : Don't call mutate and compare ConstantState. #
Total comments: 4
Patch Set 3 : Address tedchoc's comments #
Depends on Patchset: Messages
Total messages: 24 (11 generated)
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ortuno@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc: PTAL. I believe the problem didn't lie with the icons themselves but rather with the fact that we were calling notifyDataSetChanged() for every update because the icons were always different. This CL tries to avoid that by always passing the same Drawable and having ItemChooserDialog create a new drawable for selected items so that their state is not shared with the rest of the icons. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:354: // If the row is selected create a new drawable to avoid this seems very bluetooth specific...I would like to find a way where this class thinks the icons are independent and it would be up to the caller to ensure this. If instead of using tints on the drawables, what happens if we did make it a layer drawable where you are just switching between layers? Then the bitmaps are entirely shared and it is purely the state that is different between them (where tinting affects the actual bitmap contents). https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:356: row.mImageView.setImageDrawable(isSelected If we do end up needing to do this, it might be nicer to have a getSelectedDrawableForItem(Drawable) that can be overwritten in bluetooth but in the base class it would just return the passed in arg. https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:584: * @param icon Drawable to show left of the description. A tint list should be set on This doesn't necessarily need to be a tint list. It could also be a layer drawable that supports selected as one of the options. It might worth stating that the drawable should be stateful and handle the selected state to be renderered correctly.
https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:354: // If the row is selected create a new drawable to avoid On 2017/04/11 at 20:58:28, Ted C wrote: > this seems very bluetooth specific...I would like to find a way where this class thinks the icons are independent and it would be up to the caller to ensure this. > I investigated some more. I believe the problem is not actually creating the drawables but rather calling notifyDataSetChanged() for all updates. Because the client calls addOrUpdateItem with a new Drawable each time, ItemChooserRow#hasSameContents() returns false causing the whole list to be redrawn for every update even if the icon itself hasn't changed. If we want ItemChooserDialog to think the icons are independent then we would have to change BluetoothChooserDialog to only update items where the icons have changed. I think this makes the interface worse since now we are basically moving the "AddOrUpdate" logic down to the clients. This patch fixed the problem by no longer passing a new Drawable for every update and thus avoiding a call to notifyDataSetChanged for every update. What about taking a drawable id instead of an actual Drawable? Then ItemChooserDialog would create the Drawable itself. Though that means that we would always make a vector Drawable with three states which might be too restrictive e.g. we wouldn't be able to use a LevelListDrawable for a new feature. > If instead of using tints on the drawables, what happens if we did make it a layer drawable where you are just switching between layers? Then the bitmaps are entirely shared and it is purely the state that is different between them (where tinting affects the actual bitmap contents). If the Layer Drawable is different between updates then we would still have the same problem; notifyDataSetChanged would be called for all updates.
https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:354: // If the row is selected create a new drawable to avoid On 2017/04/12 00:47:06, ortuno wrote: > On 2017/04/11 at 20:58:28, Ted C wrote: > > this seems very bluetooth specific...I would like to find a way where this > class thinks the icons are independent and it would be up to the caller to > ensure this. > > > > I investigated some more. I believe the problem is not actually creating the > drawables but rather calling notifyDataSetChanged() for all updates. What exactly is "all updates"? In general, I'm confused of how this list differs from bookmarks, downloads, history where we have a list view where every item has it's own drawable. Something about this "smells" like we are doing the wrong thing. Calling notifyDataSetChanged "should" be a relative no op as the views should be mapped to the same they were before. setImageDrawable with the same icon that was set before should be a no-op. Granted, this adapter doesn't have hasStableIds == true, which might cause more randomization that I'd expect. In general, the ordering should match the previous positioning, so unless you are actively scrolling notifyDataSetChanged should be very easy. The other thing we could consider is not have the icons built until getView is called (i.e. instead of passing a Drawable, we would pass a DrawableProvider). Then we are at least not building drawables for views we don't show. Then the drawableprovider would be in charge of rebuilding the drawables only when the bluetooth signal state changed. In the bluetooth code, are we calling addOrUpdateItem regardless of whether the icon/text changed? Are we calling it for all items on udpates? If we are calling it just for the item that is changed, then it again feels like we aren't doing the right thing here and using the convertView as is without resetting all the data. You "should" be able to reset just a single image fairly rapidly and not have it muck with the whole world. In general, this class should think that each drawable is individual and it should be up to the client to figure out a way to make that happen. > > Because the client calls addOrUpdateItem with a new Drawable each time, > ItemChooserRow#hasSameContents() returns false causing the whole list to be > redrawn for every update even if the icon itself hasn't changed. > > If we want ItemChooserDialog to think the icons are independent then we would > have to change BluetoothChooserDialog to only update items where the icons have > changed. I think this makes the interface worse since now we are basically > moving the "AddOrUpdate" logic down to the clients. > > This patch fixed the problem by no longer passing a new Drawable for every > update and thus avoiding a call to notifyDataSetChanged for every update. > > What about taking a drawable id instead of an actual Drawable? Then > ItemChooserDialog would create the Drawable itself. Though that means that we > would always make a vector Drawable with three states which might be too > restrictive e.g. we wouldn't be able to use a LevelListDrawable for a new > feature. > > > If instead of using tints on the drawables, what happens if we did make it a > layer drawable where you are just switching between layers? Then the bitmaps > are entirely shared and it is purely the state that is different between them > (where tinting affects the actual bitmap contents). > > If the Layer Drawable is different between updates then we would still have the > same problem; notifyDataSetChanged would be called for all updates.
tedchoc@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor as he's recently been fighting drawables for in product help
https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:219: if (!ApiCompatibilityUtils.objectEquals(icon, oldItem.mIcon)) { I think you only really need two changes to get the benefit you want: 1. Change this to compare icon.getConstantState() to oldItem.mIcon.getConstantState(). 2. Change getConnectedIcon() to not use mutate(). I'm curious if that works. https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:357: ? item.mIcon.getConstantState().newDrawable().mutate() Do you have to call mutate() here? That's more heavyweight than just calling newDrawable(). If you're doing this because you want to properly handle Drawable#setState() (View#drawableStateChanged() based on selected), you should be able to get around the mutate() call. That kind of state seems to get stored on the Drawable itself, not the Drawable's ConstantState. I will say, each Drawable really only ever expects to be bound to one View. If you look at either View#setBackground() or ImageView#setImageDrawable(), it builds a tight connection with the Drawable by registering as the Drawable's callback. If you have more than one View with the same Drawable you can get weird results: If my understanding is correct (and it might not be!) you can have scenarios like: 1. Register DRAWABLE_A with VIEW_1 and then VIEW_2. 2. DRAWABLE_A invalidateSelf() calls only notify VIEW_2 because it stole the callback. 3. Change VIEW_1 to DRAWABLE_B actually breaks even more things: - It clears the callback on DRAWABLE_A because it thought it still owned it. This means VIEW_2 doesn't get told of any updates either. - It clears the visibility on DRAWABLE_A even though VIEW_2 still is trying to show it.
https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:354: // If the row is selected create a new drawable to avoid On 2017/04/12 at 17:03:28, Ted C wrote: > On 2017/04/12 00:47:06, ortuno wrote: > > On 2017/04/11 at 20:58:28, Ted C wrote: > > > this seems very bluetooth specific...I would like to find a way where this > > class thinks the icons are independent and it would be up to the caller to > > ensure this. > > > > > > > I investigated some more. I believe the problem is not actually creating the > > drawables but rather calling notifyDataSetChanged() for all updates. > > What exactly is "all updates"? > > In general, I'm confused of how this list differs from bookmarks, downloads, history where we have a list view where every item has it's own drawable. > > Something about this "smells" like we are doing the wrong thing. > > Calling notifyDataSetChanged "should" be a relative no op as the views should be mapped to the same they were before. setImageDrawable with the same icon that was set before should be a no-op. > > Granted, this adapter doesn't have hasStableIds == true, which might cause more randomization that I'd expect. In general, the ordering should match the previous positioning, so unless you are actively scrolling notifyDataSetChanged should be very easy. > > The other thing we could consider is not have the icons built until getView is called (i.e. instead of passing a Drawable, we would pass a DrawableProvider). Then we are at least not building drawables for views we don't show. Then the drawableprovider would be in charge of rebuilding the drawables only when the bluetooth signal state changed. > > In the bluetooth code, are we calling addOrUpdateItem regardless of whether the icon/text changed? Are we calling it for all items on udpates? If we are calling it just for the item that is changed, then it again feels like we aren't doing the right thing here and using the convertView as is without resetting all the data. You "should" be able to reset just a single image fairly rapidly and not have it muck with the whole world. I don't think this matters much anymore because dtrainor suggestion below fixes the problem but: Some context about how Bluetooth works: Bluetooth devices broadcast an Advertising packet a couple of times a second. When we receive this packet, we simply notify the chooser that some information about this device has changed; it could be the Signal Strength, the name, the appearance or its connection state or it could be nothing. Right now, the Bluetooth code itself doesn't keep track of what changed, if anything, it just tells ItemChooserDialog: "I got an update about a device, here is what I know". Then the chooser grabs that information and decides if we actually have to update the chooser and calls notifyDataSetChanged() if so. ItemChooserDialog makes this call by comparing the new information with the members of ItemChooserRow. The patch that introduced the icon made it so that each new update had a completely new icon. This results in notifyDataSetChanged() being called every time we receive a new Advertising packet from the device. In a busy BT environment like a google office this happens tens of times a second causing touch events to get ignored. We could definitely change Bluetooth code to only notify when a device changes but then we would be duplicating some of the logic from ItemChooserDialog in BluetoothChooserDialog since we would have to keep a set of devices, and their information. Or we could try to expose this data from ItemChooserDialog to help BluetoothChooserDialog make the decision. > > In general, this class should think that each drawable is individual and it should be up to the client to figure out a way to make that happen. > > > > > Because the client calls addOrUpdateItem with a new Drawable each time, > > ItemChooserRow#hasSameContents() returns false causing the whole list to be > > redrawn for every update even if the icon itself hasn't changed. > > > > If we want ItemChooserDialog to think the icons are independent then we would > > have to change BluetoothChooserDialog to only update items where the icons have > > changed. I think this makes the interface worse since now we are basically > > moving the "AddOrUpdate" logic down to the clients. > > > > This patch fixed the problem by no longer passing a new Drawable for every > > update and thus avoiding a call to notifyDataSetChanged for every update. > > > > What about taking a drawable id instead of an actual Drawable? Then > > ItemChooserDialog would create the Drawable itself. Though that means that we > > would always make a vector Drawable with three states which might be too > > restrictive e.g. we wouldn't be able to use a LevelListDrawable for a new > > feature. > > > > > If instead of using tints on the drawables, what happens if we did make it a > > layer drawable where you are just switching between layers? Then the bitmaps > > are entirely shared and it is purely the state that is different between them > > (where tinting affects the actual bitmap contents). > > > > If the Layer Drawable is different between updates then we would still have the > > same problem; notifyDataSetChanged would be called for all updates. https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:357: ? item.mIcon.getConstantState().newDrawable().mutate() On 2017/04/12 at 19:29:03, David Trainor-ping if over 24h wrote: > Do you have to call mutate() here? That's more heavyweight than just calling newDrawable(). > > If you're doing this because you want to properly handle Drawable#setState() (View#drawableStateChanged() based on selected), you should be able to get around the mutate() call. That kind of state seems to get stored on the Drawable itself, not the Drawable's ConstantState. > Ugh I wish things like this were documented better. It would have save us so much time! This actually makes things much easier as we can just compare the constant state before trying to update the list. Thanks! > I will say, each Drawable really only ever expects to be bound to one View. If you look at either View#setBackground() or ImageView#setImageDrawable(), it builds a tight connection with the Drawable by registering as the Drawable's callback. If you have more than one View with the same Drawable you can get weird results: > > If my understanding is correct (and it might not be!) you can have scenarios like: > 1. Register DRAWABLE_A with VIEW_1 and then VIEW_2. > 2. DRAWABLE_A invalidateSelf() calls only notify VIEW_2 because it stole the callback. > 3. Change VIEW_1 to DRAWABLE_B actually breaks even more things: > - It clears the callback on DRAWABLE_A because it thought it still owned it. This means VIEW_2 doesn't get told of any updates either. > - It clears the visibility on DRAWABLE_A even though VIEW_2 still is trying to show it. That makes sense. Thanks for the explanation :) I'll have to be more careful with how I handle icons. https://codereview.chromium.org/2813703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:584: * @param icon Drawable to show left of the description. A tint list should be set on On 2017/04/11 at 20:58:29, Ted C wrote: > This doesn't necessarily need to be a tint list. It could also be a layer drawable that supports selected as one of the options. It might worth stating that the drawable should be stateful and handle the selected state to be renderered correctly. Done.
PTAL again. This follows dtrainor's suggestion to not call mutate() anymore which allows us to compare the icons' constant state to decide if the icon should be updated or not.
lgtm https://codereview.chromium.org/2813703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:97: if ((mIcon != null) && !mIcon.getConstantState().equals(icon.getConstantState())) { don't need the extra ()s around the first condition https://codereview.chromium.org/2813703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:358: // If the row is selected create a new drawable to avoid no longer required?
lgtm % ted's nits
Description was changed from ========== chooser: Create a new icon if the row is selected Before, clients had to provide a new Drawable for each item update so that selecting an item wouldn't cause other row icons to change colors. If the client updated the list very often, we would see performance issues since notifyDataSetChanged() would be called for every update even if the actual icon didn't change. With this CL, clients can pass the same Drawable to ItemChooserDialog#addOrUpdateItem and a new Drawable will be created only when a row is selected. This: 1. Avoids having to call notifyDataSetChanged() for each item update. 2. Avoids having clients to create a new Drawable for every update. BUG=543466 ========== to ========== chooser: Don't call mutate on new drawable Stops calling mutate() to avoid creating a new DrawableConstantState and uses the constant state to decided if the icon needs updating. Calling mutate() on new Drawables would cause a new DrawableConstantState to be created for each Drawable. This in turn would cause notifyDataSetChanged to be called for every update to the list. BUG=543466 ==========
The CQ bit was checked by ortuno@chromium.org
Thanks! https://codereview.chromium.org/2813703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2813703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:97: if ((mIcon != null) && !mIcon.getConstantState().equals(icon.getConstantState())) { On 2017/04/14 at 19:40:52, Ted C wrote: > don't need the extra ()s around the first condition Done. https://codereview.chromium.org/2813703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:358: // If the row is selected create a new drawable to avoid On 2017/04/14 at 19:40:52, Ted C wrote: > no longer required? Done.
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2813703004/#ps40001 (title: "Address tedchoc's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492472404536120,
"parent_rev": "bdce3ee117af8ea68331c572dbaa877b8d4fdcf6", "commit_rev":
"483ae59e8536d954321f8c29851ca4310f3a33b5"}
Message was sent while issue was closed.
Description was changed from ========== chooser: Don't call mutate on new drawable Stops calling mutate() to avoid creating a new DrawableConstantState and uses the constant state to decided if the icon needs updating. Calling mutate() on new Drawables would cause a new DrawableConstantState to be created for each Drawable. This in turn would cause notifyDataSetChanged to be called for every update to the list. BUG=543466 ========== to ========== chooser: Don't call mutate on new drawable Stops calling mutate() to avoid creating a new DrawableConstantState and uses the constant state to decided if the icon needs updating. Calling mutate() on new Drawables would cause a new DrawableConstantState to be created for each Drawable. This in turn would cause notifyDataSetChanged to be called for every update to the list. BUG=543466 Review-Url: https://codereview.chromium.org/2813703004 Cr-Commit-Position: refs/heads/master@{#465073} Committed: https://chromium.googlesource.com/chromium/src/+/483ae59e8536d954321f8c29851c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/483ae59e8536d954321f8c29851c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
