bluetooth: Implement RSSI indicator on android

bluetooth: Implement RSSI indicator on android

This indicator gives an indication to users of how far their Bluetooth
device is.

Screenshot in: https://bugs.chromium.org/p/chromium/issues/detail?id=629689#c12

BUG=629689

[ortuno, 2016-08-24 22:06:06]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 22:06:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 23:43:52]

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

[commit-bot: I haz the power, 2016-08-24 23:43:53]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[ortuno, 2016-08-25 17:33:03]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 17:33:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 19:05:13]

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

[commit-bot: I haz the power, 2016-08-25 19:05:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[ortuno, 2016-08-25 22:01:59]

ortuno@chromium.org changed reviewers:
+ juncai@chromium.org

[ortuno, 2016-08-25 22:02:00]

juncai: PTAL

[ortuno, 2016-08-25 22:02:19]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:03:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:13:40]

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

[commit-bot: I haz the power, 2016-08-26 00:13:41]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-08-26 19:37:48]

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/item_chooser_dialog_row_with_icon.xml
(right):

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/item_chooser_dialog_row_with_icon.xml:6:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Since we already have an item_chooser_dialog_row.xml:
https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/item_choo...
I am wondering if that one can be reused by adding the ImageView to it instead
of creating a item_chooser_dialog_row_with_icon.xml? I noticed that there is
RelativeLayout:
https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/web_notif...

And can set element.setVisibility(View.GONE); to hide a view, and
android:layout_alignWithParentIfMissing to make a view to take the missing
view's space.

If so, when the "connected"/"paired" status need to be added to the row, it will
be easier.

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:52:
R.drawable.ic_signal_cellular_0_bar_black_48dp,
Are the signal strength icon images added in different directory the same size?

I noticed that it needs to use different image size, for example:
//chrome/android/java/res/drawable-mdpi/account_management_incognito.png is:
40 by 40
//chrome/android/java/res/drawable-hdpi/account_management_incognito.png is:
60 by 60
//chrome/android/java/res/drawable-xhdpi/account_management_incognito.png is:
80 by 80
//chrome/android/java/res/drawable-xxhdpi/account_management_incognito.png is:
120 by 120
//chrome/android/java/res/drawable-xxxhdpi/account_management_incognito.png is:
160 by 160

The original signal icons have four different sizes: 18 by 18, 24 by 24, 36 by
36, 48 by 48, here it has five directories, so maybe can use same images for
//chrome/android/java/res/drawable-xxhdpi/ and
//chrome/android/java/res/drawable-xxxhdpi/?

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:53:
R.drawable.ic_signal_cellular_0_bar_white_48dp,
The filename probably doesn't need to include "48dp" if they are different
sizes, it can be something like the ones that are used on desktop:
//ui/resources/default_100_percent/common/signal_0_bar.png

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:54:
"Signal Strengh Level: Lowest"),
typo:
s/Strengh/Strength

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:58:
"Signal Strengh Level: Lower"),
How about naming them something more straightforward, like:
"Signal Strength Level: 0 bar"
"Signal Strength Level: 1 bar"
"Signal Strength Level: 2 bar"
"Signal Strength Level: 3 bar"
"Signal Strength Level: 4 bar"?

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:61:
R.drawable.ic_signal_cellular_2_bar_white_48dp,
As I understand, the black image is for item not selected yet, and white image
is for selected item.

Can you upload some screenshots to show the chooser with selected and
non-selected item?

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:366:
signalStrengthLevel == -1 ? null : SIGNAL_STRENGTH_ICONS[signalStrengthLevel]));
maybe add some assert to make sure |signalStrengthLevel| is 0-4 inclusively.

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:105:
private ItemChooserRowIcon mIcon;
The class ItemChooserRowIcon has a member variable |mIcon|, so maybe name
something different here?

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:217:
mChooserDialog.addOrUpdateDevice("id-1", "Name 1", -1 /* No rssi */);
maybe change the comment to be something more descriptive like: "No signal
strength icon shown"?

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:229:
ItemChooserDialog.ItemChooserRow item1_with_no_icon =
Use lowerCamelCase.

Same for the other variable names.

https://codereview.chromium.org/2271413002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2271413002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:146: // choser
is idle and should no longer be updated.
typo:
s/choser/chooser

[ortuno, 2016-09-12 05:11:28]

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/item_chooser_dialog_row_with_icon.xml
(right):

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/item_chooser_dialog_row_with_icon.xml:6:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
On 2016/08/26 at 19:37:48, juncai wrote:
> Since we already have an item_chooser_dialog_row.xml:
>
https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/item_choo...
> I am wondering if that one can be reused by adding the ImageView to it instead
of creating a item_chooser_dialog_row_with_icon.xml? I noticed that there is
RelativeLayout:
>
https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/web_notif...
> 
> And can set element.setVisibility(View.GONE); to hide a view, and
android:layout_alignWithParentIfMissing to make a view to take the missing
view's space.
> 
> If so, when the "connected"/"paired" status need to be added to the row, it
will be easier.

Ah good idea. Done.

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:52:
R.drawable.ic_signal_cellular_0_bar_black_48dp,
On 2016/08/26 at 19:37:48, juncai wrote:
> Are the signal strength icon images added in different directory the same
size?
> 

Each directory has a copy of the image in a different size:

mdpi -> 24px x 24px
hdpi -> 36px x 36px
xhdip -> 48px x 48px
...

> I noticed that it needs to use different image size, for example:
> //chrome/android/java/res/drawable-mdpi/account_management_incognito.png is:
> 40 by 40
> //chrome/android/java/res/drawable-hdpi/account_management_incognito.png is:
> 60 by 60
> //chrome/android/java/res/drawable-xhdpi/account_management_incognito.png is:
> 80 by 80
> //chrome/android/java/res/drawable-xxhdpi/account_management_incognito.png is:
> 120 by 120
> //chrome/android/java/res/drawable-xxxhdpi/account_management_incognito.png
is:
> 160 by 160

I think account_management is an exception. The sizes of
ic_check_googblue_24dp.png are the same as or
ic_signal_cellular_0_bar_black_24dp. 

> 
> The original signal icons have four different sizes: 18 by 18, 24 by 24, 36 by
36, 48 by 48, here it has five directories, so maybe can use same images for
//chrome/android/java/res/drawable-xxhdpi/ and
//chrome/android/java/res/drawable-xxxhdpi/?

Hmm not sure what you mean? I think you might be confusing dp
(density-independent pixel) with px (pixel). All images are 24dp but have
different pixel resolution for example the one in drawable-mdpi is 24px by 24px
but the one in xxxhdpi is 96px by 96px.

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:53:
R.drawable.ic_signal_cellular_0_bar_white_48dp,
On 2016/08/26 at 19:37:48, juncai wrote:
> The filename probably doesn't need to include "48dp" if they are different
sizes, it can be something like the ones that are used on desktop:
> //ui/resources/default_100_percent/common/signal_0_bar.png

They all have the same dp so I think the name is appropriate. Also the name
helps the linter make sure the image is the right size. For example a 24dp image
in a xhdpi folder should be 48 x 48 pixels. If we were to remove the 24dp from
the name then we could accidentally add a 24 x 24 pixels image without the
linter complaining.

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:54:
"Signal Strengh Level: Lowest"),
On 2016/08/26 at 19:37:48, juncai wrote:
> typo:
> s/Strengh/Strength

Done.

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:58:
"Signal Strengh Level: Lower"),
On 2016/08/26 at 19:37:48, juncai wrote:
> How about naming them something more straightforward, like:
> "Signal Strength Level: 0 bar"
> "Signal Strength Level: 1 bar"
> "Signal Strength Level: 2 bar"
> "Signal Strength Level: 3 bar"
> "Signal Strength Level: 4 bar"?

Ahh good idea. That also reminded me to not hard code strings and to add tests.

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:61:
R.drawable.ic_signal_cellular_2_bar_white_48dp,
On 2016/08/26 at 19:37:48, juncai wrote:
> As I understand, the black image is for item not selected yet, and white image
is for selected item.
> 
> Can you upload some screenshots to show the chooser with selected and
non-selected item?

Done.

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:366:
signalStrengthLevel == -1 ? null : SIGNAL_STRENGTH_ICONS[signalStrengthLevel]));
On 2016/08/26 at 19:37:48, juncai wrote:
> maybe add some assert to make sure |signalStrengthLevel| is 0-4 inclusively.

Done.

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:105:
private ItemChooserRowIcon mIcon;
On 2016/08/26 at 19:37:48, juncai wrote:
> The class ItemChooserRowIcon has a member variable |mIcon|, so maybe name
something different here?

Done.

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:217:
mChooserDialog.addOrUpdateDevice("id-1", "Name 1", -1 /* No rssi */);
On 2016/08/26 at 19:37:48, juncai wrote:
> maybe change the comment to be something more descriptive like: "No signal
strength icon shown"?

Done.

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:229:
ItemChooserDialog.ItemChooserRow item1_with_no_icon =
On 2016/08/26 at 19:37:48, juncai wrote:
> Use lowerCamelCase.
> 
> Same for the other variable names.

Done.

https://codereview.chromium.org/2271413002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2271413002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:146: // choser
is idle and should no longer be updated.
On 2016/08/26 at 19:37:48, juncai wrote:
> typo:
> s/choser/chooser

Done.

[ortuno, 2016-09-12 05:11:33]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 05:11:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 06:33:46]

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

[commit-bot: I haz the power, 2016-09-12 06:33:47]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[juncai, 2016-09-12 20:05:49]

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

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:52:
R.drawable.ic_signal_cellular_0_bar_black_48dp,
On 2016/09/12 05:11:27, ortuno wrote:
> On 2016/08/26 at 19:37:48, juncai wrote:
> > Are the signal strength icon images added in different directory the same
> size?
> > 
> 
> Each directory has a copy of the image in a different size:
> 
> mdpi -> 24px x 24px
> hdpi -> 36px x 36px
> xhdip -> 48px x 48px
> ...
> 
> > I noticed that it needs to use different image size, for example:
> > //chrome/android/java/res/drawable-mdpi/account_management_incognito.png is:
> > 40 by 40
> > //chrome/android/java/res/drawable-hdpi/account_management_incognito.png is:
> > 60 by 60
> > //chrome/android/java/res/drawable-xhdpi/account_management_incognito.png
is:
> > 80 by 80
> > //chrome/android/java/res/drawable-xxhdpi/account_management_incognito.png
is:
> > 120 by 120
> > //chrome/android/java/res/drawable-xxxhdpi/account_management_incognito.png
> is:
> > 160 by 160
> 
> I think account_management is an exception. The sizes of
> ic_check_googblue_24dp.png are the same as or
> ic_signal_cellular_0_bar_black_24dp. 
> 
> > 
> > The original signal icons have four different sizes: 18 by 18, 24 by 24, 36
by
> 36, 48 by 48, here it has five directories, so maybe can use same images for
> //chrome/android/java/res/drawable-xxhdpi/ and
> //chrome/android/java/res/drawable-xxxhdpi/?
> 
> Hmm not sure what you mean? I think you might be confusing dp
> (density-independent pixel) with px (pixel). All images are 24dp but have
> different pixel resolution for example the one in drawable-mdpi is 24px by
24px
> but the one in xxxhdpi is 96px by 96px.

ah, I see. Thanks!

https://codereview.chromium.org/2271413002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:53:
R.drawable.ic_signal_cellular_0_bar_white_48dp,
On 2016/09/12 05:11:27, ortuno wrote:
> On 2016/08/26 at 19:37:48, juncai wrote:
> > The filename probably doesn't need to include "48dp" if they are different
> sizes, it can be something like the ones that are used on desktop:
> > //ui/resources/default_100_percent/common/signal_0_bar.png
> 
> They all have the same dp so I think the name is appropriate. Also the name
> helps the linter make sure the image is the right size. For example a 24dp
image
> in a xhdpi folder should be 48 x 48 pixels. If we were to remove the 24dp from
> the name then we could accidentally add a 24 x 24 pixels image without the
> linter complaining.

Acknowledged.

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right):

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/item_chooser_dialog_row.xml:20:
android:contentDescription="@null"
wondering why the character '»' is here?

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

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:354:
assert 0 <= signalStrengthLevel && signalStrengthLevel <= 4;
I think this assert needs to be moved inside the following if statement.
Otherwise when |signalStrengthLevel| is actually -1, the assert fails.

I saw the test code tests when |signalStrengthLevel| is -1, just wondering why
the test code didn't fail at the assert?

Also instead of using the hard code number 4, maybe calculate the size of
|SIGNAL_STRENGTH_ICONS|.

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:129:
final int last = first + items.getChildCount() - 1;
Maybe use: getLastVisiblePosition() here for |last|?

[ortuno, 2016-09-13 03:00:45]

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right):

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/item_chooser_dialog_row.xml:20:
android:contentDescription="@null"
On 2016/09/12 at 20:05:49, juncai wrote:
> wondering why the character '»' is here?

Argh, my editor accidentally added a tab here. Done.

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

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:354:
assert 0 <= signalStrengthLevel && signalStrengthLevel <= 4;
On 2016/09/12 at 20:05:49, juncai wrote:
> I think this assert needs to be moved inside the following if statement.
Otherwise when |signalStrengthLevel| is actually -1, the assert fails.
> 

Made the first condition -1.

> I saw the test code tests when |signalStrengthLevel| is -1, just wondering why
the test code didn't fail at the assert?
> 

It's probably because asserts are not enabled on build bots.

> Also instead of using the hard code number 4, maybe calculate the size of
|SIGNAL_STRENGTH_ICONS|.

Done.

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2271413002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:129:
final int last = first + items.getChildCount() - 1;
On 2016/09/12 at 20:05:49, juncai wrote:
> Maybe use: getLastVisiblePosition() here for |last|?

Done.

[ortuno, 2016-09-13 03:00:52]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 03:01:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 04:15:27]

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

[commit-bot: I haz the power, 2016-09-13 04:15:28]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-09-13 17:12:44]

LGTM. Thanks!

[ortuno, 2016-09-13 23:09:50]

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

[ortuno, 2016-09-13 23:09:51]

tedchoc: PTAL

[Ted C, 2016-09-15 22:47:41]

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

[Ted C, 2016-09-15 22:47:41]

Sorry for the delay, but looking at the number of assets we'd need to add for
this, I wonder if we should either be drawing this with simple android canvas
draw calls or if we could use the svg versions.

Adding Ian (ianwen) to get the latest update on the status of svgs in Chrome. 
Can we use them?  If so, do you have any pointers about how we could use them
here?

[Ian Wen, 2016-09-16 21:30:06]

On 2016/09/15 22:47:41, Ted C wrote:
> Sorry for the delay, but looking at the number of assets we'd need to add for
> this, I wonder if we should either be drawing this with simple android canvas
> draw calls or if we could use the svg versions.
> 
> Adding Ian (ianwen) to get the latest update on the status of svgs in Chrome. 
> Can we use them?  If so, do you have any pointers about how we could use them
> here?

Currently, as far as I know, vector drawables can only be used by app:src_compat
tag in xml layouts. One example is
https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/download_....

One suggestion that I can think of is to use a layer drawable, and put it in the
drawable folder. In this layer drawable, reference a ic_signal_cellular_*_white
as layers. Then in java you could load it and set level to it.

Feel free to ping me if you encounter any problem with the above approach.

[Ian Wen, 2016-09-16 23:37:23]

On 2016/09/16 21:30:06, Ian Wen wrote:
> On 2016/09/15 22:47:41, Ted C wrote:
> > Sorry for the delay, but looking at the number of assets we'd need to add
for
> > this, I wonder if we should either be drawing this with simple android
canvas
> > draw calls or if we could use the svg versions.
> > 
> > Adding Ian (ianwen) to get the latest update on the status of svgs in
Chrome. 
> > Can we use them?  If so, do you have any pointers about how we could use
them
> > here?
> 
> Currently, as far as I know, vector drawables can only be used by
app:src_compat
> tag in xml layouts. One example is
>
https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/download_....
> 
> One suggestion that I can think of is to use a layer drawable, and put it in
the
> drawable folder. In this layer drawable, reference a
ic_signal_cellular_*_white
> as layers. Then in java you could load it and set level to it.
> 
> Feel free to ping me if you encounter any problem with the above approach.

Sorry. I meant "level-list-drawable", not "layer drawable".

https://developer.android.com/reference/android/graphics/drawable/LevelListDr...

[ortuno, 2016-09-21 06:36:24]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 06:36:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 08:06:12]

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

[commit-bot: I haz the power, 2016-09-21 08:06:14]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-09-26 06:12:52]

juncai: PTAL again. I changed the Dialog so that user can specify if an icon
should be displayed, added some tests for it and now we use vector icons.

[ortuno, 2016-09-26 06:13:01]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 06:13:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 07:09:33]

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

[commit-bot: I haz the power, 2016-09-26 07:09:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ian Wen, 2016-09-26 18:12:49]

Thanks for using vector drawable!

https://codereview.chromium.org/2271413002/diff/160001/build/android/lint/sup...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/2271413002/diff/160001/build/android/lint/sup...
build/android/lint/suppressions.xml:174: <ignore
regexp="chrome/android/java/res/drawable/ic_signal_cellular_0_bar_grey600_24dp.xml"
/>
Are you sure the suppression is needed? It seems downloads_big.xml is not in the
list while it is also a vector drawable.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_signal_cellular_0_bar_grey600_24dp.xml
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_signal_cellular_0_bar_grey600_24dp.xml:6:
android:width="24dp"
I think you could remove "600_24dp" part from the xml names. 24dp is only the
drawable's intrinsic size and if we want we could enlarge it to any size.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:117:
R.drawable.ic_signal_cellular_0_bar_grey600_24dp, null /* theme */),
Maybe remove /* */? This comment style is not commonly seen in Chrome's Java
code.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:68:
public static class ItemChooserRowIcon {
I would vote for using level list drawable instead of a custom class. Benefits
of using level list drawable are:

1. More consistent with the framework. If you see the android doc for
LevelListDrawable, it's highlighted example is to display wifi signals, which is
almost identical to the problem here.

2. You no longer need to override equals() and hashcode(). Simply get level and
compare integers.

Like what you did above, you still need to new LevelListDrawable, call mutate on
it, and add level to it manually.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:387:
icon.setContentDescription(rowIcon.mIconDescription);
If you choose to use level drawable, you might want to use getQuantityString
here with drawable.getLevel().

[juncai, 2016-09-26 18:49:01]

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:68:
public static class ItemChooserRowIcon {
On 2016/09/26 18:12:48, Ian Wen wrote:
> I would vote for using level list drawable instead of a custom class. Benefits
> of using level list drawable are:
> 
> 1. More consistent with the framework. If you see the android doc for
> LevelListDrawable, it's highlighted example is to display wifi signals, which
is
> almost identical to the problem here.
> 
> 2. You no longer need to override equals() and hashcode(). Simply get level
and
> compare integers.
> 
> Like what you did above, you still need to new LevelListDrawable, call mutate
on
> it, and add level to it manually.

Question: if we use LevelListDrawable here, can other icon be shown at the same
place as the signal strength level icon when it is not shown? Because when a
Bluetooth device is connected, we would like to display a Bluetooth connected
icon instead of a signal strength level icon, a screenshot from desktop is at:
https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c63

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:395:

Maybe add some test code to test different color icon is shown when row
selection is changed:
1. select a row and then unselect that row.
2. select a row and then select another row.

[Ian Wen, 2016-09-26 21:39:33]

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:68:
public static class ItemChooserRowIcon {
On 2016/09/26 18:49:01, juncai wrote:
> On 2016/09/26 18:12:48, Ian Wen wrote:
> > I would vote for using level list drawable instead of a custom class.
Benefits
> > of using level list drawable are:
> > 
> > 1. More consistent with the framework. If you see the android doc for
> > LevelListDrawable, it's highlighted example is to display wifi signals,
which
> is
> > almost identical to the problem here.
> > 
> > 2. You no longer need to override equals() and hashcode(). Simply get level
> and
> > compare integers.
> > 
> > Like what you did above, you still need to new LevelListDrawable, call
mutate
> on
> > it, and add level to it manually.
> 
> Question: if we use LevelListDrawable here, can other icon be shown at the
same
> place as the signal strength level icon when it is not shown? Because when a
> Bluetooth device is connected, we would like to display a Bluetooth connected
> icon instead of a signal strength level icon, a screenshot from desktop is at:
> https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c63

The dialog can store just a drawable, and it can still perform getLevel and
setLevel on a normal drawable. These operations will be no-op if the icon is
"connected".

[ortuno, 2016-09-26 23:34:48]

I'm currently addressing the rest of the comments but I just wanted to reply
before the eod in MTV. Thanks!

https://codereview.chromium.org/2271413002/diff/160001/build/android/lint/sup...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/2271413002/diff/160001/build/android/lint/sup...
build/android/lint/suppressions.xml:174: <ignore
regexp="chrome/android/java/res/drawable/ic_signal_cellular_0_bar_grey600_24dp.xml"
/>
On 2016/09/26 at 18:12:48, Ian Wen wrote:
> Are you sure the suppression is needed? It seems downloads_big.xml is not in
the list while it is also a vector drawable.

Maybe I'm missing something but downloads_big.xml is right above these new
items.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:68:
public static class ItemChooserRowIcon {
On 2016/09/26 at 21:39:33, Ian Wen wrote:
> On 2016/09/26 18:49:01, juncai wrote:
> > On 2016/09/26 18:12:48, Ian Wen wrote:
> > > I would vote for using level list drawable instead of a custom class.
Benefits
> > > of using level list drawable are:
> > > 
> > > 1. More consistent with the framework. If you see the android doc for
> > > LevelListDrawable, it's highlighted example is to display wifi signals,
which
> > is
> > > almost identical to the problem here.
> > > 
> > > 2. You no longer need to override equals() and hashcode(). Simply get
level
> > and
> > > compare integers.
> > > 
> > > Like what you did above, you still need to new LevelListDrawable, call
mutate
> > on
> > > it, and add level to it manually.
> > 
> > Question: if we use LevelListDrawable here, can other icon be shown at the
same
> > place as the signal strength level icon when it is not shown? Because when a
> > Bluetooth device is connected, we would like to display a Bluetooth
connected
> > icon instead of a signal strength level icon, a screenshot from desktop is
at:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c63
> 
> The dialog can store just a drawable, and it can still perform getLevel and
setLevel on a normal drawable. These operations will be no-op if the icon is
"connected".

Some context:

ItemChooserDialog is a class that is used by any client that wants to show a
list with some items. There are two clients UsbChooserDialog and
BluetoothChooserDialog. This CL adds the ability for ItemChooserDialog to show
rows with icons. To do this, clients have to initialize ItemChooserDialog with
isUsingIcon set to true and pass in an icon in addOrUpdateItem().

In the case of Bluetooth that icon is sometimes a signal strength level and
sometimes some other icon. 

I had a first version of this patch using LeveListDrawable. The reason I didn't
end up submitting it was that I thought this was more optional. With this
approach we create the icons once in BluetoothChooserDialog and then reuse them
when calling addOrUpdateItem. Maybe I'm missing something but with the
LevelListDrawable approach I believe we would have to create a new
LevelListDrawable for each new update.

> 1. More consistent with the framework. If you see the android doc for
> LevelListDrawable, it's highlighted example is to display wifi signals, which
is
> almost identical to the problem here.
> 
> 2. You no longer need to override equals() and hashcode(). Simply get level
and
> compare integers.

True, but I don't think it quite matches our use case. In the example, there is
an icon on which you can call setLevel when the level changes. In our case the
class that owns that Icon, ItemChooserDialog, doesn't know that the icon
represents a level, it could be a usb icon, a bluetooth icon or a signal level.
Which I think is fine because the Signal Strength level is a Bluetooth-only
concept.

[Ian Wen, 2016-09-27 04:22:42]

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
chrome/android/java/res/layout/item_chooser_dialog_row.xml:19:
android:id="@+id/icon"
1. Instead of wrapping the image view with linear layout, you could simply add
16dp of margin around the imageview.
2. What is even better is to not use relative layout nor linear layout. Instead,
use compound drawable for text view. See this CL about how compound drawable
replaces a linear layout and makes the code much simpler.
https://codereview.chromium.org/2343383002.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:120:
resources.getQuantityString(R.plurals.signal_strength_level_n_bars, 0, 0)),
Thanks for making our product more accessible by adding the content
descriptions!

For this case though, the label should be representative enough of the item, and
the content description should not be needed. In general we do not include
content description  for compound drawables in textview, because the textview
speaks its own word. https://blog.stylingandroid.com/compound-drawable/

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:68:
public static class ItemChooserRowIcon {
On 2016/09/26 23:34:48, ortuno wrote:
> On 2016/09/26 at 21:39:33, Ian Wen wrote:
> > On 2016/09/26 18:49:01, juncai wrote:
> > > On 2016/09/26 18:12:48, Ian Wen wrote:
> > > > I would vote for using level list drawable instead of a custom class.
> Benefits
> > > > of using level list drawable are:
> > > > 
> > > > 1. More consistent with the framework. If you see the android doc for
> > > > LevelListDrawable, it's highlighted example is to display wifi signals,
> which
> > > is
> > > > almost identical to the problem here.
> > > > 
> > > > 2. You no longer need to override equals() and hashcode(). Simply get
> level
> > > and
> > > > compare integers.
> > > > 
> > > > Like what you did above, you still need to new LevelListDrawable, call
> mutate
> > > on
> > > > it, and add level to it manually.
> > > 
> > > Question: if we use LevelListDrawable here, can other icon be shown at the
> same
> > > place as the signal strength level icon when it is not shown? Because when
a
> > > Bluetooth device is connected, we would like to display a Bluetooth
> connected
> > > icon instead of a signal strength level icon, a screenshot from desktop is
> at:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c63
> > 
> > The dialog can store just a drawable, and it can still perform getLevel and
> setLevel on a normal drawable. These operations will be no-op if the icon is
> "connected".
> 
> Some context:
> 
> ItemChooserDialog is a class that is used by any client that wants to show a
> list with some items. There are two clients UsbChooserDialog and
> BluetoothChooserDialog. This CL adds the ability for ItemChooserDialog to show
> rows with icons. To do this, clients have to initialize ItemChooserDialog with
> isUsingIcon set to true and pass in an icon in addOrUpdateItem().
> 
> In the case of Bluetooth that icon is sometimes a signal strength level and
> sometimes some other icon. 
> 
> I had a first version of this patch using LeveListDrawable. The reason I
didn't
> end up submitting it was that I thought this was more optional. With this
> approach we create the icons once in BluetoothChooserDialog and then reuse
them
> when calling addOrUpdateItem. Maybe I'm missing something but with the
> LevelListDrawable approach I believe we would have to create a new
> LevelListDrawable for each new update.
> 
> > 1. More consistent with the framework. If you see the android doc for
> > LevelListDrawable, it's highlighted example is to display wifi signals,
which
> is
> > almost identical to the problem here.
> > 
> > 2. You no longer need to override equals() and hashcode(). Simply get level
> and
> > compare integers.
> 
> True, but I don't think it quite matches our use case. In the example, there
is
> an icon on which you can call setLevel when the level changes. In our case the
> class that owns that Icon, ItemChooserDialog, doesn't know that the icon
> represents a level, it could be a usb icon, a bluetooth icon or a signal
level.
> Which I think is fine because the Signal Strength level is a Bluetooth-only
> concept.
> 

Thanks for the explanation!

If you are worried about creating new drawables, it is fine as it should be
fairly cheap. All the actual bitmaps are not copied. See here for detail:
http://android-developers.blogspot.com/2009/05/drawable-mutations.html.


I created a simple CL to demonstrate the approach that I am proposing.
https://chromiumcodereview.appspot.com/2370203002/

[ortuno, 2016-10-04 07:15:26]

Patchset #10 (id:180001) has been deleted

[ortuno, 2016-10-04 07:16:18]

juncai, tedchoc: WIP. Do not review.

ianwen: After a week of struggling with Android UI and it's undocumented
behavior I started to think LevelListDrawable and StateListDrawable are not the
way to go here:

1. Before there was a one direction relationship between clients of the
ItemChooserDialog and ItemChooserDialog. Clients said "This is what I know about
this device." and the ItemChooserDialog took that information and updated the
relevant portions.

If we want to avoid creating a new LevelListDrawables for each update we would
have to start exposing old drawables to clients which would create a two way
relationship (and means wiring up information from the adapter all the way up to
the clients), which makes the interface more complex. (See addOrUpdateDevice in
BluetoothChooserDialog) Furthermore because clients can now manually update the
icon, clients would have to tell ItemChooserDialog that they have updated the
icon, ItemChooserDialog can no longer figure that out by itself.

2. ItemChooserDialog needs to know about levels. Because of the way ImageView
and levels work, we need to set the ImageView's level before we set the
drawable[1]. (See getView() in ItemChooserDialog)

I appreciate the desire to use the platforms features but I think in this case
they are just increasing the complexity of the interfaces and reducing
readability. If these reasons are not enough for you I'll clean up this patch
and send out for review.

[1]
https://github.com/android/platform_frameworks_base/blob/master/core/java/and...

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
File chrome/android/java/res/drawable/ic_signal_cellular_0_bar_grey600_24dp.xml
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
chrome/android/java/res/drawable/ic_signal_cellular_0_bar_grey600_24dp.xml:6:
android:width="24dp"
On 2016/09/26 at 18:12:48, Ian Wen wrote:
> I think you could remove "600_24dp" part from the xml names. 24dp is only the
drawable's intrinsic size and if we want we could enlarge it to any size.

Done. q: is 24dp the right size?

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/re...
chrome/android/java/res/layout/item_chooser_dialog_row.xml:19:
android:id="@+id/icon"
On 2016/09/27 at 04:22:41, Ian Wen wrote:
> 1. Instead of wrapping the image view with linear layout, you could simply add
16dp of margin around the imageview.
> 2. What is even better is to not use relative layout nor linear layout.
Instead, use compound drawable for text view. See this CL about how compound
drawable replaces a linear layout and makes the code much simpler.
https://codereview.chromium.org/2343383002.

I removed the linear layout. Now I just vertically center the icon. Also added
some margin to the start and end of the icon.

As discussed offline I went with the relative layout instead of the compound
drawable since we will eventually need to have two lines of text.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:117:
R.drawable.ic_signal_cellular_0_bar_grey600_24dp, null /* theme */),
On 2016/09/26 at 18:12:48, Ian Wen wrote:
> Maybe remove /* */? This comment style is not commonly seen in Chrome's Java
code.

If it's OK with you I would rather keep this. There are a couple of examples
throughout the code base and I believe it does improve readability.

https://cs.chromium.org/chromium/src/chrome/test/android/javatests/src/org/ch...

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

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

Though I do think the argument of consistency is stronger so up to you.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:120:
resources.getQuantityString(R.plurals.signal_strength_level_n_bars, 0, 0)),
On 2016/09/27 at 04:22:41, Ian Wen wrote:
> Thanks for making our product more accessible by adding the content
descriptions!
> 
> For this case though, the label should be representative enough of the item,
and the content description should not be needed. In general we do not include
content description  for compound drawables in textview, because the textview
speaks its own word. https://blog.stylingandroid.com/compound-drawable/

As mentioned offline (I think?) the image adds more information about the item,
information that is not available elsewhere so we keep the descriptions.

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:68:
public static class ItemChooserRowIcon {
On 2016/09/27 at 04:22:41, Ian Wen wrote:
> On 2016/09/26 23:34:48, ortuno wrote:
> > On 2016/09/26 at 21:39:33, Ian Wen wrote:
> > > On 2016/09/26 18:49:01, juncai wrote:
> > > > On 2016/09/26 18:12:48, Ian Wen wrote:
> > > > > I would vote for using level list drawable instead of a custom class.
> > Benefits
> > > > > of using level list drawable are:
> > > > > 
> > > > > 1. More consistent with the framework. If you see the android doc for
> > > > > LevelListDrawable, it's highlighted example is to display wifi
signals,
> > which
> > > > is
> > > > > almost identical to the problem here.
> > > > > 
> > > > > 2. You no longer need to override equals() and hashcode(). Simply get
> > level
> > > > and
> > > > > compare integers.
> > > > > 
> > > > > Like what you did above, you still need to new LevelListDrawable, call
> > mutate
> > > > on
> > > > > it, and add level to it manually.
> > > > 
> > > > Question: if we use LevelListDrawable here, can other icon be shown at
the
> > same
> > > > place as the signal strength level icon when it is not shown? Because
when a
> > > > Bluetooth device is connected, we would like to display a Bluetooth
> > connected
> > > > icon instead of a signal strength level icon, a screenshot from desktop
is
> > at:
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c63
> > > 
> > > The dialog can store just a drawable, and it can still perform getLevel
and
> > setLevel on a normal drawable. These operations will be no-op if the icon is
> > "connected".
> > 
> > Some context:
> > 
> > ItemChooserDialog is a class that is used by any client that wants to show a
> > list with some items. There are two clients UsbChooserDialog and
> > BluetoothChooserDialog. This CL adds the ability for ItemChooserDialog to
show
> > rows with icons. To do this, clients have to initialize ItemChooserDialog
with
> > isUsingIcon set to true and pass in an icon in addOrUpdateItem().
> > 
> > In the case of Bluetooth that icon is sometimes a signal strength level and
> > sometimes some other icon. 
> > 
> > I had a first version of this patch using LeveListDrawable. The reason I
didn't
> > end up submitting it was that I thought this was more optional. With this
> > approach we create the icons once in BluetoothChooserDialog and then reuse
them
> > when calling addOrUpdateItem. Maybe I'm missing something but with the
> > LevelListDrawable approach I believe we would have to create a new
> > LevelListDrawable for each new update.
> > 
> > > 1. More consistent with the framework. If you see the android doc for
> > > LevelListDrawable, it's highlighted example is to display wifi signals,
which
> > is
> > > almost identical to the problem here.
> > > 
> > > 2. You no longer need to override equals() and hashcode(). Simply get
level
> > and
> > > compare integers.
> > 
> > True, but I don't think it quite matches our use case. In the example, there
is
> > an icon on which you can call setLevel when the level changes. In our case
the
> > class that owns that Icon, ItemChooserDialog, doesn't know that the icon
> > represents a level, it could be a usb icon, a bluetooth icon or a signal
level.
> > Which I think is fine because the Signal Strength level is a Bluetooth-only
> > concept.
> > 
> 
> Thanks for the explanation!
> 
> If you are worried about creating new drawables, it is fine as it should be
fairly cheap. All the actual bitmaps are not copied. See here for detail:
http://android-developers.blogspot.com/2009/05/drawable-mutations.html.
> 
> 
> I created a simple CL to demonstrate the approach that I am proposing.
https://chromiumcodereview.appspot.com/2370203002/

Thanks for the article and CL :) I really appreciate it. I tried the approach
you proposed in the CL and bumped into some issues. I left comments in the CL.

[Ian Wen, 2016-10-14 01:39:51]

Hello!

I wrote a small demo to show my ideas that could make code simpler. Check this
out: https://github.com/jollycopper/ListViewColorState

My main point is that since update is very frequent, we shouldn't create new
temporary object that adds GC burden to jvm. Instead we should refactor
ItemChooserDialog a bit to enable in-place update. Also this way we could get
rid of the complex equality check.

Let me know what you think and I am willing to help in every way. :)

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:122:
resources, R.drawable.ic_signal_cellular_0_bar_grey, null /* theme */));
Why not simply use mActivity.getTheme() here? Or if you don't care about theme I
would remove the comment as well.

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:417:
new ItemChooserDialog.ItemChooserRow(deviceId, deviceName, icon,
iconDescription),
IIRC this method will be called more than 10 times per second, right?

If so I would favor getting rid of the "new ItemChooserDialog...." statement
because that's too much GC burden to jvm. I see you already have an
ItemChooserRow object, how about we only modify it inplace, and not create new
objects?

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:226:
public void addOrUpdate(ItemChooserRow item, boolean didUpdateOldDrawable) {
I see many equal checks here and above. Which leads me to thinking whether it's
even necessary to check equality. If we convert the dialog so that it gives out
the item row and let the client to update it, then the update logic will be
trivial, and we no longer need to care if anything has changed or not, because
the client will have a better idea.

Also this way we do not need to create temporary objects anymore, especially
when addOrUpdate is called very frequently.

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:356:
if (convertView instanceof RelativeLayout) {
Can the view be of a different type? If not I would simply check if convertView
is null. If so inflate it. Otherwise directly use it.

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:362:
if (mUsingIcon) {
Instead of using this boolean, shall we grab the corresponding itemrow and check
if there is icon in it?

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:373:
if (selected) {
The color manipulation could be simplified by color state list. Please refer to
the demo I sent you. :)

https://codereview.chromium.org/2271413002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:572:
addOrUpdateItem(item, false /* didUpdateOldDrawable */);
I would remove this comment.