Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704263004:
bluetooth: Add connected icon to Bluetooth Chooser on Android (Closed)
Descriptionbluetooth: Add connected icon to Bluetooth Chooser on Android
Adds the ability to show rows with icons to ItemChooserDialog.
Changes BluetoothChooserDialog to pass in an icon for connected devices.
BUG=543466
Review-Url: https://codereview.chromium.org/2704263004
Cr-Commit-Position: refs/heads/master@{#463554}
Committed: https://chromium.googlesource.com/chromium/src/+/a9620b50d02828a39914e3764dea3ef4cf2bdc3b
Patch Set 1 #Patch Set 2 : Remove constant state test #Patch Set 3 : Change name #
Total comments: 16
Patch Set 4 : Address juncai's comments #
Total comments: 2
Patch Set 5 : Address juncai's comments #
Total comments: 29
Patch Set 6 : Address tedchoc's comments #Patch Set 7 : Addess moar comments #Patch Set 8 : clean up #Patch Set 9 : Clean up #
Total comments: 12
Patch Set 10 : Address tedchoc's comments #Dependent Patchsets: Messages
Total messages: 51 (32 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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: + juncai@chromium.org
juncai: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/item_chooser_dialog_row.xml:11: <!-- contentDescription is added at runtime. --> Add some latest screenshots of the chooser (with icons, without icons, etc.) on the issue page? https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:386: row.mImageView.setVisibility(View.VISIBLE); nit: change these three lines to be the same order as inside the else statement. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:580: addOrUpdateItem(key, description, null); nit: add comment for null. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1267: <message name="IDS_BLUETOOTH_DEVICE_CONNECTED" desc="Text to be shown next to Bluetooth devices that are connected."> I think for connected devices, we show the bluetooth icon in front of the device name, and no "Connected" text is shown. And for paired devices, we show "Paired" text below the device name on a new line. Looks like this string is used for the connected icon's description text, maybe change its name and desc to reflect that. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:229: mChooserDialog.addOrUpdateDevice("id-1", "Name 1", false /* is_gatt_connected */); s/is_gatt_connected/isGATTConnected https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:230: mChooserDialog.addOrUpdateDevice("id-2", "Name 2", true /* is_gatt_connected */); ditto. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:249: assertTrue(itemAdapter.getItem(0).hasSameContents("id-1", "Name 1", null)); nit: add comment for null. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:185: { Question: why put these lines of code into a {}? It seems that other test code doesn't do that in this file.
Thanks! https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/item_chooser_dialog_row.xml:11: <!-- contentDescription is added at runtime. --> On 2017/02/22 at 22:22:57, juncai wrote: > Add some latest screenshots of the chooser (with icons, without icons, etc.) on the issue page? Done. https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c71 https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:386: row.mImageView.setVisibility(View.VISIBLE); On 2017/02/22 at 22:22:57, juncai wrote: > nit: change these three lines to be the same order as inside the else statement. The ones below are reversed because I think it makes more sense to first hide the view and then set the image and description to null. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:580: addOrUpdateItem(key, description, null); On 2017/02/22 at 22:22:57, juncai wrote: > nit: add comment for null. Done. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1267: <message name="IDS_BLUETOOTH_DEVICE_CONNECTED" desc="Text to be shown next to Bluetooth devices that are connected."> On 2017/02/22 at 22:22:57, juncai wrote: > I think for connected devices, we show the bluetooth icon in front of the device name, and no "Connected" text is shown. > > And for paired devices, we show "Paired" text below the device name on a new line. > > Looks like this string is used for the connected icon's description text, maybe change its name and desc to reflect that. Done. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:229: mChooserDialog.addOrUpdateDevice("id-1", "Name 1", false /* is_gatt_connected */); On 2017/02/22 at 22:22:57, juncai wrote: > s/is_gatt_connected/isGATTConnected Done. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:230: mChooserDialog.addOrUpdateDevice("id-2", "Name 2", true /* is_gatt_connected */); On 2017/02/22 at 22:22:57, juncai wrote: > ditto. Done. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:249: assertTrue(itemAdapter.getItem(0).hasSameContents("id-1", "Name 1", null)); On 2017/02/22 at 22:22:57, juncai wrote: > nit: add comment for null. Done. https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java (right): https://codereview.chromium.org/2704263004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:185: { On 2017/02/22 at 22:22:57, juncai wrote: > Question: why put these lines of code into a {}? It seems that other test code doesn't do that in this file. Android sometimes reuses views so we need to retrieve each View right before we check its values. Using the braces encloses calls to retrieve the ImageView in another scope so I can reuse the name in the next section. See the next test for an example: { // Add item 1 with no icon. mChooserDialog.addOrUpdateItem("key1", "desc1", mTestIcon1); ImageView icon1 = getIconImageView(items, 0); } { // Add item 2 with no icon. mChooserDialog.addOrUpdateItem("key2", "desc2", mTestIcon2); ImageView icon1 = getIconImageView(items, 0); ImageView icon2 = getIconImageView(items, 1); } I think this makes for more readable tests and ensures the old view is not being used for the next step in the test.
LGTM with a nit. https://codereview.chromium.org/2704263004/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2704263004/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1267: <message name="IDS_BLUETOOTH_DEVICE_CONNECTED" desc="Content Description for the Connected Icon next to connected Bluetooth Devices."> nit: the first letters of "Description", "Connected", "Icon", "Devices" need to be lowercase.
Thanks! https://codereview.chromium.org/2704263004/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2704263004/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1267: <message name="IDS_BLUETOOTH_DEVICE_CONNECTED" desc="Content Description for the Connected Icon next to connected Bluetooth Devices."> On 2017/03/01 at 00:29:56, juncai wrote: > nit: the first letters of "Description", "Connected", "Icon", "Devices" need to be lowercase. Done.
ortuno@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc: PTAL
https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/item_chooser_dialog_row.xml:5: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" Any idea why this is a RelativeLayout? I think a LinearLayout with horizontal is easier to read than a RelativeLayout. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:75: Drawable mConnectedIcon; any reason these aren't private? https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:135: DrawableCompat.setTint( tints work on vectors now? https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:70: private final Drawable mSelected; ideally, you shouldn't need a selected and non-selected icon. Android provides a way to construct a drawable where it uses different underlying drawables based on the view state. https://developer.android.com/reference/android/graphics/drawable/StateListDr... https://developer.android.com/guide/topics/resources/drawable-resource.html#S... https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:85: public static boolean equals( any reason we don't just override Object#equals? https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:89: if (icon1.mIcon != icon2.mIcon) return false; we should probably use !icon1.mIcon.equals(icon2.mIcon), same for the next line https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:131: public boolean hasSameContents(String key, String description) { can we just remove this? https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:252: notifyDataSetChanged(); instead of keeping a set of item keys with icons (mItemsWithIcons), could we just override notifyDataSetChanged and then iterate over the list items and look for anyone with a non-null icon. Having to keep all of these data structures in sync seems overkill to me. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:305: private boolean isSelected(int position) { since this is used just once, I would just inline the boolean check in the one place (or a separate variable above for readability) https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:585: * was not in the chooser. Otherwise updates the items description or icon. There are some odd mechanics of using any icons. Until we customize the behavior, I think it is worth stating here that the presence of any icon results in all rows being inset with the icon dimensions.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/item_chooser_dialog_row.xml:5: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2017/03/03 at 00:56:19, Ted C wrote: > Any idea why this is a RelativeLayout? I think a LinearLayout with horizontal is easier to read than a RelativeLayout. There are a couple of row layouts the ItemChooser will have to support: 1. Single line layout (This is the one we currently have). 2. Single line layout with icon. 3. Two line layout. 4. Two line layout with icon. ianwen suggested: <RelativeLayout android:layout_width="match_parent" android:layout_height="wrap_content" android:background="@android:color/darker_gray"> <ImageView android:id="@+id/icon" android:layout_width="wrap_content" android:layout_height="24dp" android:layout_alignParentStart="true" android:layout_centerInParent="true" android:layout_margin="16dp" android:src="@mipmap/ic_launcher"/> <TextView android:id="@+id/title" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_toEndOf="@id/icon" android:layout_margin="5dp" android:text="Title"/> <TextView android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_below="@id/title" android:layout_toEndOf="@id/icon" android:layout_margin="5dp" android:text="Details"/> </RelativeLayout> So we changed to RelativeLayout. Happy to change if you think LinearLayout is a better layout. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:75: Drawable mConnectedIcon; On 2017/03/03 at 00:56:19, Ted C wrote: > any reason these aren't private? They were being used in tests but not anymore. Changed to private. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:135: DrawableCompat.setTint( On 2017/03/03 at 00:56:19, Ted C wrote: > tints work on vectors now? They don't? I was assuming they did since they were part of DrawableCompat which we use for Vectors. I tested on a M device and the colors were correct. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:70: private final Drawable mSelected; On 2017/03/03 at 00:56:19, Ted C wrote: > ideally, you shouldn't need a selected and non-selected icon. Android provides a way to construct a drawable where it uses different underlying drawables based on the view state. > > https://developer.android.com/reference/android/graphics/drawable/StateListDr... > > https://developer.android.com/guide/topics/resources/drawable-resource.html#S... Yeah I have another patch with that approach but: (1) it required creating a new Drawable for each item with an icon otherwise all the icons would turn white if one gets selected and (2) I couldn't find a way to test that the right icon was being showed. In the end this approach solved both problems: we only ever create two Drawables mConnectedIcon and its selected variant and since we always use the same icon we could test that it was being passed from BluetoothChooserDialog to ItemChooserDialog. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:85: public static boolean equals( On 2017/03/03 at 00:56:19, Ted C wrote: > any reason we don't just override Object#equals? Do you mean the non static Object.equals(Object)? There are a couple of instances in which icon1 could be null e.g. hasSameContents. Rather than having users of the method check if icon1 is null I thought it would be better to make a static method that would account for any of the two icons being null. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:89: if (icon1.mIcon != icon2.mIcon) return false; On 2017/03/03 at 00:56:19, Ted C wrote: > we should probably use !icon1.mIcon.equals(icon2.mIcon), same for the next line Done. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:131: public boolean hasSameContents(String key, String description) { On 2017/03/03 at 00:56:19, Ted C wrote: > can we just remove this? Done. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:252: notifyDataSetChanged(); On 2017/03/03 at 00:56:19, Ted C wrote: > instead of keeping a set of item keys with icons (mItemsWithIcons), could we just override notifyDataSetChanged and then iterate over the list items and look for anyone with a non-null icon. Having to keep all of these data structures in sync seems overkill to me. Agree. I replaced the Set with a boolean that gets set in notifyDatasetChanged. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:305: private boolean isSelected(int position) { On 2017/03/03 at 00:56:19, Ted C wrote: > since this is used just once, I would just inline the boolean check in the one place (or a separate variable above for readability) Done. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:585: * was not in the chooser. Otherwise updates the items description or icon. On 2017/03/03 at 00:56:19, Ted C wrote: > There are some odd mechanics of using any icons. Until we customize the behavior, I think it is worth stating here that the presence of any icon results in all rows being inset with the icon dimensions. Done.
https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/item_chooser_dialog_row.xml:5: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2017/03/03 22:08:22, ortuno wrote: > On 2017/03/03 at 00:56:19, Ted C wrote: > > Any idea why this is a RelativeLayout? I think a LinearLayout with horizontal > is easier to read than a RelativeLayout. > > There are a couple of row layouts the ItemChooser will have to support: > > 1. Single line layout (This is the one we currently have). > 2. Single line layout with icon. > 3. Two line layout. > 4. Two line layout with icon. > > > ianwen suggested: > > <RelativeLayout > android:layout_width="match_parent" > android:layout_height="wrap_content" > android:background="@android:color/darker_gray"> > > <ImageView > android:id="@+id/icon" > android:layout_width="wrap_content" > android:layout_height="24dp" > android:layout_alignParentStart="true" > android:layout_centerInParent="true" > android:layout_margin="16dp" > android:src="@mipmap/ic_launcher"/> > > <TextView > android:id="@+id/title" > android:layout_width="wrap_content" > android:layout_height="wrap_content" > android:layout_toEndOf="@id/icon" > android:layout_margin="5dp" > android:text="Title"/> > > <TextView > android:layout_width="wrap_content" > android:layout_height="wrap_content" > android:layout_below="@id/title" > android:layout_toEndOf="@id/icon" > android:layout_margin="5dp" > android:text="Details"/> > </RelativeLayout> > > So we changed to RelativeLayout. Happy to change if you think LinearLayout is a > better layout. When are we going to support the other types? In general, I don't like designing for a future that might never come to pass. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:75: Drawable mConnectedIcon; On 2017/03/03 22:08:22, ortuno wrote: > On 2017/03/03 at 00:56:19, Ted C wrote: > > any reason these aren't private? > > They were being used in tests but not anymore. Changed to private. If they need to be exposed for testing, they should be marked @VisibleForTesting to make it more obvious the intent (vs being exposed for other classes in the package to modify) https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:135: DrawableCompat.setTint( On 2017/03/03 22:08:22, ortuno wrote: > On 2017/03/03 at 00:56:19, Ted C wrote: > > tints work on vectors now? > > They don't? I was assuming they did since they were part of DrawableCompat which > we use for Vectors. I tested on a M device and the colors were correct. If you have a K device it would be good to verify there, vectors were supported natively later than that and maybe compat doesn't work (or it didn't in the past). It would be awesome if it works though! https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:70: private final Drawable mSelected; On 2017/03/03 22:08:22, ortuno wrote: > On 2017/03/03 at 00:56:19, Ted C wrote: > > ideally, you shouldn't need a selected and non-selected icon. Android > provides a way to construct a drawable where it uses different underlying > drawables based on the view state. > > > > > https://developer.android.com/reference/android/graphics/drawable/StateListDr... > > > > > https://developer.android.com/guide/topics/resources/drawable-resource.html#S... > > Yeah I have another patch with that approach but: (1) it required creating a new > Drawable for each item with an icon otherwise all the icons would turn white if > one gets selected Did you call .mutate() on them? > and (2) I couldn't find a way to test that the right icon was > being showed. I'd argue that isn't something you'd need to do. You would essentially be testing that Android's widget is setting the correct view state based on some framework code. If anything, you could test that the StateListDrawable you create returns the drawable you expect based on the states you pass it, but then you'd be verifying that you constructed it to play nicely with Android. > > In the end this approach solved both problems: we only ever create two Drawables > mConnectedIcon and its selected variant and since we always use the same icon we > could test that it was being passed from BluetoothChooserDialog to > ItemChooserDialog. While this may work, it is handling something in an non-platform consistent way. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:85: public static boolean equals( On 2017/03/03 22:08:23, ortuno wrote: > On 2017/03/03 at 00:56:19, Ted C wrote: > > any reason we don't just override Object#equals? > > Do you mean the non static Object.equals(Object)? There are a couple of > instances in which icon1 could be null e.g. hasSameContents. Rather than having > users of the method check if icon1 is null I thought it would be better to make > a static method that would account for any of the two icons being null. Hmm...what do you think about adding something like this to ApiCompatibilityUtils (to do what Objects.equals does in API 19+) public static boolean objectEquals(Object a, Object b) { return (a == null) ? (b == null) : a.equals(b); } Then we can use these objects in java collections as well as do this sort of behavior.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/item_chooser_dialog_row.xml (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/item_chooser_dialog_row.xml:5: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2017/03/04 at 00:08:46, Ted C wrote: > On 2017/03/03 22:08:22, ortuno wrote: > > On 2017/03/03 at 00:56:19, Ted C wrote: > > > Any idea why this is a RelativeLayout? I think a LinearLayout with horizontal > > is easier to read than a RelativeLayout. > > > > There are a couple of row layouts the ItemChooser will have to support: > > > > 1. Single line layout (This is the one we currently have). > > 2. Single line layout with icon. > > 3. Two line layout. > > 4. Two line layout with icon. > > > > > > ianwen suggested: > > > > <RelativeLayout > > android:layout_width="match_parent" > > android:layout_height="wrap_content" > > android:background="@android:color/darker_gray"> > > > > <ImageView > > android:id="@+id/icon" > > android:layout_width="wrap_content" > > android:layout_height="24dp" > > android:layout_alignParentStart="true" > > android:layout_centerInParent="true" > > android:layout_margin="16dp" > > android:src="@mipmap/ic_launcher"/> > > > > <TextView > > android:id="@+id/title" > > android:layout_width="wrap_content" > > android:layout_height="wrap_content" > > android:layout_toEndOf="@id/icon" > > android:layout_margin="5dp" > > android:text="Title"/> > > > > <TextView > > android:layout_width="wrap_content" > > android:layout_height="wrap_content" > > android:layout_below="@id/title" > > android:layout_toEndOf="@id/icon" > > android:layout_margin="5dp" > > android:text="Details"/> > > </RelativeLayout> > > > > So we changed to RelativeLayout. Happy to change if you think LinearLayout is a > > better layout. > > When are we going to support the other types? In general, I don't like designing for a future that might never come to pass. I'm currently working on adding all those layouts :) First this patch to add the icon, then a follow up to add the second line of text. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:135: DrawableCompat.setTint( On 2017/03/04 at 00:08:46, Ted C wrote: > On 2017/03/03 22:08:22, ortuno wrote: > > On 2017/03/03 at 00:56:19, Ted C wrote: > > > tints work on vectors now? > > > > They don't? I was assuming they did since they were part of DrawableCompat which > > we use for Vectors. I tested on a M device and the colors were correct. > > If you have a K device it would be good to verify there, vectors were supported natively later than that and maybe compat doesn't work (or it didn't in the past). It would be awesome if it works though! tldr; It works! Huzzah! Web Bluetooth only works with L+ so I changed the code in DownloadManagerUi.java[1] to test this. I tried the following two approaches: // Using DrawableCompat. Drawable downloads = VectorDrawableCompat.create(mActivity.getResources(), R.drawable.downloads_big, mActivity.getTheme()); downloads.mutate(); DrawableCompat.setTint( downloads, ApiCompatibilityUtils.getColor(mActivity.getResources(), android.R.color.white)); mSelectableListLayout.initializeEmptyView( downloads, R.string.download_manager_ui_empty, R.string.download_manager_no_results); // Calling setTint on VectorDrawableCompat directly. (I had to suppress a lint warning though). VectorDrawableCompat downloads = VectorDrawableCompat.create(mActivity.getResources(), R.drawable.downloads_big, mActivity.getTheme()); downloads.mutate(); downloads.setTint(ApiCompatibilityUtils.getColor(mActivity.getResources(), android.R.color.black)); mSelectableListLayout.initializeEmptyView( downloads, R.string.download_manager_ui_empty, R.string.download_manager_no_results); Both of them worked! [1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:70: private final Drawable mSelected; On 2017/03/04 at 00:08:46, Ted C wrote: > On 2017/03/03 22:08:22, ortuno wrote: > > On 2017/03/03 at 00:56:19, Ted C wrote: > > > ideally, you shouldn't need a selected and non-selected icon. Android > > provides a way to construct a drawable where it uses different underlying > > drawables based on the view state. > > > > > > > > https://developer.android.com/reference/android/graphics/drawable/StateListDr... > > > > > > > > https://developer.android.com/guide/topics/resources/drawable-resource.html#S... > > > > Yeah I have another patch with that approach but: (1) it required creating a new > > Drawable for each item with an icon otherwise all the icons would turn white if > > one gets selected > > Did you call .mutate() on them? > Ok so I redid the patch that used the tint list (I think that's better than StateList). I wrote the patch on top of this patch to make it easier to see the differences: http://crrev.com/2738503007 > > and (2) I couldn't find a way to test that the right icon was > > being showed. > > I'd argue that isn't something you'd need to do. You would essentially be testing that Android's widget is setting the correct view state based on some framework code. > > If anything, you could test that the StateListDrawable you create returns the drawable you expect based on the states you pass it, but then you'd be verifying that you constructed it to play nicely with Android. > Ah I should have been more specific. I want a test to make sure that BluetoothChooserDialgo.addOrUpdateDevice() passes the right icon to ItemChooserDialog.addOrUpdateItem(). See BluetoothChooserDialogTest > > > > In the end this approach solved both problems: we only ever create two Drawables > > mConnectedIcon and its selected variant and since we always use the same icon we > > could test that it was being passed from BluetoothChooserDialog to > > ItemChooserDialog. > > While this may work, it is handling something in an non-platform consistent way. Happy to go the platform consistent way but I would like to solve the testing problem. As mentioned before I redid this patch to use a more platform consistent way http://crrev.com/2738503007 I annotated it with the reasons I chose to go with this approach instead. https://codereview.chromium.org/2704263004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:85: public static boolean equals( On 2017/03/04 at 00:08:46, Ted C wrote: > On 2017/03/03 22:08:23, ortuno wrote: > > On 2017/03/03 at 00:56:19, Ted C wrote: > > > any reason we don't just override Object#equals? > > > > Do you mean the non static Object.equals(Object)? There are a couple of > > instances in which icon1 could be null e.g. hasSameContents. Rather than having > > users of the method check if icon1 is null I thought it would be better to make > > a static method that would account for any of the two icons being null. > > Hmm...what do you think about adding something like this to ApiCompatibilityUtils (to do what Objects.equals does in API 19+) > > public static boolean objectEquals(Object a, Object b) { > return (a == null) ? (b == null) : a.equals(b); > } > > Then we can use these objects in java collections as well as do this sort of behavior. Good idea! http://crrev.com/2733363003
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...
tedchoc: PTAL this uses the ColorStateList pattern we discussed.
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...
lgtm w/ nits https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/re... File chrome/android/java/res/color/item_chooser_row_icon_color.xml (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/re... chrome/android/java/res/color/item_chooser_row_icon_color.xml:7: <item android:color="@android:color/white" android:state_selected="true"/> I realized that item chooser text is 2 spaces, but all the other colors are 4, so we should standardize towards that. Sorry for missing the other file https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:376: return mConnectedIcon.getConstantState().newDrawable().mutate(); one thing to consider if whether you can keep a pool of icons if it becomes an issue and share them where their connection state is the same. https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:173: private boolean mHasIcon = false; = false is the default, so we should drop that https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:195: public void addOrUpdate(String key, String description, @Nullable Drawable icon, a bit unfair to you, but can you add javadoc to the public methods that you're touching as they should have javadoc https://codereview.chromium.org/2704263004/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:149: final ListView items = (ListView) dialog.findViewById(R.id.items); the finals here are ok, but not terribly consistent with how we write tests in java https://codereview.chromium.org/2704263004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:150: final int actual_position = position - 1; s/actual_position/actualPosition
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Thanks! https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/re... File chrome/android/java/res/color/item_chooser_row_icon_color.xml (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/re... chrome/android/java/res/color/item_chooser_row_icon_color.xml:7: <item android:color="@android:color/white" android:state_selected="true"/> On 2017/04/11 at 00:09:33, Ted C wrote: > I realized that item chooser text is 2 spaces, but all the other colors are 4, so we should standardize towards that. > > Sorry for missing the other file Done. CL for the other file http://crrev.com/2807393002 https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:376: return mConnectedIcon.getConstantState().newDrawable().mutate(); On 2017/04/11 at 00:09:33, Ted C wrote: > one thing to consider if whether you can keep a pool of icons if it becomes an issue and share them where their connection state is the same. The problem about sharing them is that their state also gets shared so when we select one all the others also change color. What about always passing the same icon to addOrUpdateItem and having getView() create a new drawable only if the row is "selected". That way we would avoid sharing the state with "selected" icons. Though I do worry that this would be too "magical". https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:173: private boolean mHasIcon = false; On 2017/04/11 at 00:09:33, Ted C wrote: > = false is the default, so we should drop that Done. https://codereview.chromium.org/2704263004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:195: public void addOrUpdate(String key, String description, @Nullable Drawable icon, On 2017/04/11 at 00:09:33, Ted C wrote: > a bit unfair to you, but can you add javadoc to the public methods that you're touching as they should have javadoc Done. I feel some sense of ownership of this code so I'm always happy to improve it :) https://codereview.chromium.org/2704263004/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java (right): https://codereview.chromium.org/2704263004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:149: final ListView items = (ListView) dialog.findViewById(R.id.items); On 2017/04/11 at 00:09:33, Ted C wrote: > the finals here are ok, but not terribly consistent with how we write tests in java The first two were final because they were used in a callable. Removed the final since that is no longer the case. https://codereview.chromium.org/2704263004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:150: final int actual_position = position - 1; On 2017/04/11 at 00:09:33, Ted C wrote: > s/actual_position/actualPosition Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juncai@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2704263004/#ps180001 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ortuno@chromium.org changed reviewers: + jbudorick@chromium.org
jbudorick: PTAL at build/android/lint
On 2017/04/11 04:56:42, ortuno wrote: > jbudorick: PTAL at build/android/lint lgtm
The CQ bit was checked by ortuno@chromium.org
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": 180001, "attempt_start_ts": 1491892389853520,
"parent_rev": "b466a02f6480a6f2823b823fc69f4f693377863e", "commit_rev":
"a9620b50d02828a39914e3764dea3ef4cf2bdc3b"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add connected icon to Bluetooth Chooser on Android Adds the ability to show rows with icons to ItemChooserDialog. Changes BluetoothChooserDialog to pass in an icon for connected devices. BUG=543466 ========== to ========== bluetooth: Add connected icon to Bluetooth Chooser on Android Adds the ability to show rows with icons to ItemChooserDialog. Changes BluetoothChooserDialog to pass in an icon for connected devices. BUG=543466 Review-Url: https://codereview.chromium.org/2704263004 Cr-Commit-Position: refs/heads/master@{#463554} Committed: https://chromium.googlesource.com/chromium/src/+/a9620b50d02828a39914e3764dea... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a9620b50d02828a39914e3764dea... |
