Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2823853004:
DO NO SUMMIT: bluetooth: Use a LevelListDrawable instead of an array of drawables
|
Created:
3 years, 8 months ago by ortuno Modified:
3 years, 8 months ago Reviewers:
David Trainor- moved to gerrit CC:
chromium-reviews, ortuno+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDO NO SUMMIT: bluetooth: Use a LevelListDrawable instead of an array of drawables
Patch Set 1 #
Total comments: 5
Patch Set 2 : Save icons level #
Total comments: 1
Depends on Patchset: Messages
Total messages: 8 (1 generated)
https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:402: icon = mSignalStrengthLevelIcon.getConstantState().newDrawable(); Calling newDrawable on a LevelListDrawable creates a Drawable with a different constant state which results in refreshing the list for every new update; same problem http://crrev.com/2813703004 solved. https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:360: int drawableLevel = item.mIcon.getLevel(); This seems pretty leaky.
dtrainor@chromium.org changed reviewers: + dtrainor@chromium.org
https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:402: icon = mSignalStrengthLevelIcon.getConstantState().newDrawable(); On 2017/04/18 06:57:07, ortuno wrote: > Calling newDrawable on a LevelListDrawable creates a Drawable with a different > constant state which results in refreshing the list for every new update; same > problem http://crrev.com/2813703004 solved. Ah yeah it still caches the internal wrapped drawable states, but copies the container state :(. Unfortunate! https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:360: int drawableLevel = item.mIcon.getLevel(); On 2017/04/18 06:57:07, ortuno wrote: > This seems pretty leaky. Should mIconLevel or "mSignalLevel" or something be a top level item concept? That way if it changes you just know to refresh and can always push here instead of pulling the level first?
In chat you said: "maybe it'd be worth just passing the drawable id over". Do you mean changing ItemChooserDialog#addOrUpdateItem to take an int i.e. the drawable id? I proposed that in the past but I'm not sure why would that be better than the current approach of passing the drawables directly? https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:360: int drawableLevel = item.mIcon.getLevel(); On 2017/04/19 at 21:32:07, David Trainor-ping if over 24h wrote: > On 2017/04/18 06:57:07, ortuno wrote: > > This seems pretty leaky. > > Should mIconLevel or "mSignalLevel" or something be a top level item concept? That way if it changes you just know to refresh and can always push here instead of pulling the level first? Oh right, we can save it as part of ItemChooserRow and then set it here. Done.
On 2017/04/19 23:48:02, ortuno wrote: > In chat you said: "maybe it'd be worth just passing the drawable id over". Do > you mean changing ItemChooserDialog#addOrUpdateItem to take an int i.e. the > drawable id? I proposed that in the past but I'm not sure why would that be > better than the current approach of passing the drawables directly? > > https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java > (right): > > https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:360: > int drawableLevel = item.mIcon.getLevel(); > On 2017/04/19 at 21:32:07, David Trainor-ping if over 24h wrote: > > On 2017/04/18 06:57:07, ortuno wrote: > > > This seems pretty leaky. > > > > Should mIconLevel or "mSignalLevel" or something be a top level item concept? > That way if it changes you just know to refresh and can always push here instead > of pulling the level first? > > Oh right, we can save it as part of ItemChooserRow and then set it here. Done. Sorry for the delay I was OOO last Thurs/Fri! This lgtm. I had a question about this (which you may have answered in that patch a week ago, so sorry if this is redundant!). Would it make more sense to send a semantic representation of what we need to push and load the drawable when we build the actual visuals for the row? E.g. could we just have a boolean mIsGATTConnected and also an int mSignalStrengthLevel in your ItemChooserRow? That way we're comparing exactly what we mean instead of the UI elements. If you call Resources.getDrawable() it actually tries to use a cached version of the ConstantState, which means it's pretty much as optimal as this is :). Just thinking that might make this a bit cleaner for you. You won't need level lists, you won't need to pass the icon across, etc..
https://codereview.chromium.org/2823853004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2823853004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:402: icon = mSignalStrengthLevelIcon.getConstantState().newDrawable(); This line makes the dialog pretty unusable. Since the constant state is different for each returned icon we end up redrawing the list for every update which causes a lot of ignored touch actions.
On 2017/04/25 at 03:13:33, dtrainor wrote: > On 2017/04/19 23:48:02, ortuno wrote: > > In chat you said: "maybe it'd be worth just passing the drawable id over". Do > > you mean changing ItemChooserDialog#addOrUpdateItem to take an int i.e. the > > drawable id? I proposed that in the past but I'm not sure why would that be > > better than the current approach of passing the drawables directly? > > > > https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... > > File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java > > (right): > > > > https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:360: > > int drawableLevel = item.mIcon.getLevel(); > > On 2017/04/19 at 21:32:07, David Trainor-ping if over 24h wrote: > > > On 2017/04/18 06:57:07, ortuno wrote: > > > > This seems pretty leaky. > > > > > > Should mIconLevel or "mSignalLevel" or something be a top level item concept? > > That way if it changes you just know to refresh and can always push here instead > > of pulling the level first? > > > > Oh right, we can save it as part of ItemChooserRow and then set it here. Done. > > Sorry for the delay I was OOO last Thurs/Fri! This lgtm. > > I had a question about this (which you may have answered in that patch a week ago, so sorry if this is redundant!). Would it make more sense to send a semantic representation of what we need to push and load the drawable when we build the actual visuals for the row? E.g. could we just have a boolean mIsGATTConnected and also an int mSignalStrengthLevel in your ItemChooserRow? That way we're comparing exactly what we mean instead of the UI elements. If you call Resources.getDrawable() it actually tries to use a cached version of the ConstantState, which means it's pretty much as optimal as this is :). > > Just thinking that might make this a bit cleaner for you. You won't need level lists, you won't need to pass the icon across, etc.. True but I think those concepts are too Bluetooth specific to be included in ItemChooserDialog. BluetoothChooserDialog is supposed to be the class that holds all bluetooth-specific concetps. ItemChooserDialog should be more general.
On 2017/04/26 01:07:51, ortuno wrote: > On 2017/04/25 at 03:13:33, dtrainor wrote: > > On 2017/04/19 23:48:02, ortuno wrote: > > > In chat you said: "maybe it'd be worth just passing the drawable id over". > Do > > > you mean changing ItemChooserDialog#addOrUpdateItem to take an int i.e. the > > > drawable id? I proposed that in the past but I'm not sure why would that be > > > better than the current approach of passing the drawables directly? > > > > > > > https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... > > > File > chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java > > > (right): > > > > > > > https://codereview.chromium.org/2823853004/diff/1/chrome/android/java/src/org... > > > > chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:360: > > > int drawableLevel = item.mIcon.getLevel(); > > > On 2017/04/19 at 21:32:07, David Trainor-ping if over 24h wrote: > > > > On 2017/04/18 06:57:07, ortuno wrote: > > > > > This seems pretty leaky. > > > > > > > > Should mIconLevel or "mSignalLevel" or something be a top level item > concept? > > > That way if it changes you just know to refresh and can always push here > instead > > > of pulling the level first? > > > > > > Oh right, we can save it as part of ItemChooserRow and then set it here. > Done. > > > > Sorry for the delay I was OOO last Thurs/Fri! This lgtm. > > > > I had a question about this (which you may have answered in that patch a week > ago, so sorry if this is redundant!). Would it make more sense to send a > semantic representation of what we need to push and load the drawable when we > build the actual visuals for the row? E.g. could we just have a boolean > mIsGATTConnected and also an int mSignalStrengthLevel in your ItemChooserRow? > That way we're comparing exactly what we mean instead of the UI elements. If > you call Resources.getDrawable() it actually tries to use a cached version of > the ConstantState, which means it's pretty much as optimal as this is :). > > > > Just thinking that might make this a bit cleaner for you. You won't need > level lists, you won't need to pass the icon across, etc.. > > True but I think those concepts are too Bluetooth specific to be included in > ItemChooserDialog. BluetoothChooserDialog is supposed to be the class that holds > all bluetooth-specific concetps. ItemChooserDialog should be more general. Ah sorry I thought this was all bluetooth specific code. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
