Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2098983002:
Distinguish devices with same name in chooser on Android (Closed)
DescriptionDistinguish devices with same name in chooser on Android
For devices with different names, the chooser just shows their names;
for devices with the same name, the chooser shows their name with
its unique device identifier.
So the chooser will look like:
device_different_name
device_same_name (device_1)
device_same_name (device_2)
device_same_name (device_3)
BUG=544121
Committed: https://crrev.com/6a7671ea682848648119b99f825fb9090976e271
Cr-Commit-Position: refs/heads/master@{#403295}
Patch Set 1 : distinguished devices with same name in chooser on Android #Patch Set 2 : fixed test error #Patch Set 3 : added space between device name and id #Patch Set 4 : added test code #
Total comments: 6
Patch Set 5 : address comments #
Total comments: 13
Patch Set 6 : address comments #
Messages
Total messages: 17 (6 generated)
juncai@chromium.org changed reviewers: + yfriedman@chromium.org
Please take a look.
Description was changed from ========== Distinguish devices with same name in chooser on Android For devices with different names, the chooser just shows their names; for devices with the same name, the chooser shows their name with its unique device identifier. So the chooser will look like: device_different_name device_same_name(device_1) device_same_name(device_2) device_same_name(device_3) BUG=544121 ========== to ========== Distinguish devices with same name in chooser on Android For devices with different names, the chooser just shows their names; for devices with the same name, the chooser shows their name with its unique device identifier. So the chooser will look like: device_different_name device_same_name (device_1) device_same_name (device_2) device_same_name (device_3) BUG=544121 ==========
https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:141: private HashMap<String, Integer> mDeviceNameMap = new HashMap<String, Integer>(); doesn't seem like "deviceName" should be part of "ItemChooserDialog". It should be more abstract https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:157: Integer counter = mDeviceNameMap.get(description); Nit: I think it's better to code against primitive types and let the autoboxing happen just when dealing with container objects. For example, the addition below will likely incur additional overhead vs a primitive https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:501: public String getItemDescriptionForTesting(int position) { Seems unnecessary. You can just use |getItemAdapterForTesting| and call getItemDescription on that.
https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:141: private HashMap<String, Integer> mDeviceNameMap = new HashMap<String, Integer>(); On 2016/06/29 21:18:19, Yaron wrote: > doesn't seem like "deviceName" should be part of "ItemChooserDialog". It should > be more abstract Done. https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:157: Integer counter = mDeviceNameMap.get(description); On 2016/06/29 21:18:19, Yaron wrote: > Nit: I think it's better to code against primitive types and let the autoboxing > happen just when dealing with container objects. For example, the addition below > will likely incur additional overhead vs a primitive Done. https://codereview.chromium.org/2098983002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:501: public String getItemDescriptionForTesting(int position) { On 2016/06/29 21:18:19, Yaron wrote: > Seems unnecessary. You can just use |getItemAdapterForTesting| and call > getItemDescription on that. Done.
yfriedman@chromium.org changed reviewers: + tedchoc@chromium.org
+Ted ok, I think this lg but would prefer another owner look as I'm rusty from a few weeks off
https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:141: private HashMap<String, Integer> mItemDescriptionMap = new HashMap<String, Integer>(); Use Map instead of HashMap for the variable type (unless you need callers to know it is a hashmap) w/ the newer java, you can do the following: private HashMap<String, Integer> mItemDescriptionMap = new HashMap<>(); https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:157: if (mItemDescriptionMap.containsKey(description)) { Nitty-est of all nits, but I prefer my count maps to do this (slightly more terse) int count = mItemDescriptionMap.containsKey(description) ? mItemDescriptionMap.get(description) : 0; mItemDescriptionMap.put(description, count + 1); https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:198: * Returns the description of the |position|th item. For items with in java land, wrap at 100 chars and the |<param>| style is very c++-y Use the @param to describe the positions usage instead of trying to put it in the one line. https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:202: public String getItemDescription(int position) { this name is a bit confusing since we have mDescription in the items. Maybe something like "getDisplayText(int position)" to make it clearer it is different from description https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:207: : mActivity.getString(R.string.item_chooser_item_name_with_id, same general indenting comment as the other cl, should be 8 from the start of the previous line https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:208: description, item.mKey); General question...what is mKey? Would we prefer number to be the ith instance of this item? I was thinking of this more like a file with the same name where the second file just gets a _1 added to it. But if mKey is potentially more useful, than that is ok.
https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:141: private HashMap<String, Integer> mItemDescriptionMap = new HashMap<String, Integer>(); On 2016/06/30 17:43:45, Ted C wrote: > Use Map instead of HashMap for the variable type (unless you need callers to > know it is a hashmap) > > w/ the newer java, you can do the following: > > private HashMap<String, Integer> mItemDescriptionMap = new HashMap<>(); Done. https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:157: if (mItemDescriptionMap.containsKey(description)) { On 2016/06/30 17:43:45, Ted C wrote: > Nitty-est of all nits, but I prefer my count maps to do this (slightly more > terse) > > int count = mItemDescriptionMap.containsKey(description) > ? mItemDescriptionMap.get(description) : 0; > mItemDescriptionMap.put(description, count + 1); > Done. https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:198: * Returns the description of the |position|th item. For items with On 2016/06/30 17:43:45, Ted C wrote: > in java land, wrap at 100 chars > > and the |<param>| style is very c++-y > > Use the @param to describe the positions usage instead of trying to put it in > the one line. Done. https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:202: public String getItemDescription(int position) { On 2016/06/30 17:43:45, Ted C wrote: > this name is a bit confusing since we have mDescription in the items. > > Maybe something like "getDisplayText(int position)" to make it clearer it is > different from description Done. https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:207: : mActivity.getString(R.string.item_chooser_item_name_with_id, On 2016/06/30 17:43:45, Ted C wrote: > same general indenting comment as the other cl, should be 8 from the start of > the previous line Done. https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:208: description, item.mKey); On 2016/06/30 17:43:45, Ted C wrote: > General question...what is mKey? > > Would we prefer number to be the ith instance of this item? I was thinking of > this more like a file with the same name where the second file just gets a _1 > added to it. But if mKey is potentially more useful, than that is ok. For WebBluetooth, mKey is the unique device id. And by displaying this device id for devices with same name, it's easier for user to distinguish them. Also I think it is easier and more straightforward to implement since appending an index to the name needs to keep track of the index of each device was assigned.
lgtm https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2098983002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:208: description, item.mKey); On 2016/06/30 20:36:31, juncai wrote: > On 2016/06/30 17:43:45, Ted C wrote: > > General question...what is mKey? > > > > Would we prefer number to be the ith instance of this item? I was thinking of > > this more like a file with the same name where the second file just gets a _1 > > added to it. But if mKey is potentially more useful, than that is ok. > > For WebBluetooth, mKey is the unique device id. And by displaying this device id > for devices with same name, it's easier for user to distinguish them. Also I > think it is easier and more straightforward to implement since appending an > index to the name needs to keep track of the index of each device was assigned. Acknowledged.
The CQ bit was checked by juncai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Distinguish devices with same name in chooser on Android For devices with different names, the chooser just shows their names; for devices with the same name, the chooser shows their name with its unique device identifier. So the chooser will look like: device_different_name device_same_name (device_1) device_same_name (device_2) device_same_name (device_3) BUG=544121 ========== to ========== Distinguish devices with same name in chooser on Android For devices with different names, the chooser just shows their names; for devices with the same name, the chooser shows their name with its unique device identifier. So the chooser will look like: device_different_name device_same_name (device_1) device_same_name (device_2) device_same_name (device_3) BUG=544121 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Distinguish devices with same name in chooser on Android For devices with different names, the chooser just shows their names; for devices with the same name, the chooser shows their name with its unique device identifier. So the chooser will look like: device_different_name device_same_name (device_1) device_same_name (device_2) device_same_name (device_3) BUG=544121 ========== to ========== Distinguish devices with same name in chooser on Android For devices with different names, the chooser just shows their names; for devices with the same name, the chooser shows their name with its unique device identifier. So the chooser will look like: device_different_name device_same_name (device_1) device_same_name (device_2) device_same_name (device_3) BUG=544121 Committed: https://crrev.com/6a7671ea682848648119b99f825fb9090976e271 Cr-Commit-Position: refs/heads/master@{#403295} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6a7671ea682848648119b99f825fb9090976e271 Cr-Commit-Position: refs/heads/master@{#403295} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
