Distinguish devices with same name in chooser on Android

chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser;
 
 import android.app.Activity;
 import android.app.Dialog;
 import android.content.Context;
 import android.content.DialogInterface;
@@ skipping 15 matching lines @@
 import android.widget.ProgressBar;
 import android.widget.TextView;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.util.MathUtils;
 import org.chromium.ui.base.DeviceFormFactor;
 import org.chromium.ui.widget.TextViewWithClickableSpans;
 
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Set;
 
 /**
  * A general-purpose dialog for presenting a list of things to pick from.
  */
 public class ItemChooserDialog {
     /**
      * An interface to implement to get a callback when something has been
      * selected.
@@ skipping 83 matching lines @@
         // The color of the non-highlighted text.
         private final int mDefaultTextColor;
 
         // The zero-based index of the item currently selected in the dialog,
         // or -1 (INVALID_POSITION) if nothing is selected.
         private int mSelectedItem = ListView.INVALID_POSITION;
 
         // A set of keys that are marked as disabled in the dialog.
         private Set<String> mDisabledEntries = new HashSet<String>();
 
+        // Device names are counted in a map.
+        private HashMap<String, Integer> mDeviceNameMap = new HashMap<String, Integer>();

[Yaron, 2016/06/29 21:18:19]

doesn't seem like "deviceName" should be part of "ItemChooserDialog". It should
be more abstract

[juncai, 2016/06/29 22:48:38]

Done.

+
         public ItemAdapter(Context context, int resource) {
             super(context, resource);
 
             mInflater = LayoutInflater.from(context);
 
             mBackgroundHighlightColor = ApiCompatibilityUtils.getColor(getContext().getResources(),
                     R.color.light_active_color);
             mDefaultTextColor = ApiCompatibilityUtils.getColor(getContext().getResources(),
                     R.color.default_text_color);
         }
 
         @Override
+        public void add(ItemChooserRow item) {
+            String description = item.mDescription;
+            Integer counter = mDeviceNameMap.get(description);

[Yaron, 2016/06/29 21:18:19]

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

[juncai, 2016/06/29 22:48:38]

Done.

+            mDeviceNameMap.put(description, counter == null ? 1 : counter + 1);
+            super.add(item);
+        }
+
+        @Override
+        public void remove(ItemChooserRow item) {
+            String description = item.mDescription;
+            Integer counter = mDeviceNameMap.get(description);
+            if (counter != null) {
+                if (counter.intValue() == 1) {
+                    mDeviceNameMap.remove(description);
+                } else {
+                    mDeviceNameMap.put(description, counter - 1);
+                }
+            }
+            super.remove(item);
+        }
+
+        @Override
         public void clear() {
             mSelectedItem = ListView.INVALID_POSITION;
             mConfirmButton.setEnabled(false);
             super.clear();
         }
 
         /**
          * Returns the key of the currently selected item or blank if nothing is
          * selected.
          */
         public String getSelectedItemKey() {
             ItemChooserRow row = getItem(mSelectedItem);
             if (row == null) return "";
             return row.mKey;
         }
 
         /**
+         * Returns the description of the |position|th item. For items with
+         * the same description, their unique keys are appended to distinguish
+         * them.
+         */
+        public String getItemDescription(int position) {
+            ItemChooserRow item = getItem(position);
+            String description = item.mDescription;
+            Integer counter = mDeviceNameMap.get(description);
+            return counter.intValue() == 1
+                    ? description
+                    : mActivity.getString(
+                              R.string.item_chooser_item_name_with_id, description, item.mKey);
+        }
+
+        /**
          * Sets whether the itam is enabled. Disabled items are grayed out.
          * @param id The id of the item to affect.
          * @param enabled Whether the item should be enabled or not.
          */
         public void setEnabled(String id, boolean enabled) {
             if (enabled) {
                 mDisabledEntries.remove(id);
             } else {
                 mDisabledEntries.add(id);
             }
@@ skipping 33 matching lines @@
             } else {
                 view.setBackground(null);
                 if (!isEnabled(position)) {
                     view.setTextColor(ApiCompatibilityUtils.getColor(getContext().getResources(),
                             R.color.primary_text_disabled_material_light));
                 } else {
                     view.setTextColor(mDefaultTextColor);
                 }
             }
 
-            ItemChooserRow item = getItem(position);
-            view.setText(item.mDescription);
+            view.setText(getItemDescription(position));
             return view;
         }
 
         @Override
         public void onItemClick(AdapterView<?> adapter, View view, int position, long id) {
             mSelectedItem = position;
             mConfirmButton.setEnabled(true);
             mItemAdapter.notifyDataSetChanged();
         }
     }
@@ skipping 213 matching lines @@
         return mDialog;
     }
 
     /**
      * Returns the ItemAdapter associated with this class. For use with tests only.
      */
     @VisibleForTesting
     public ItemAdapter getItemAdapterForTesting() {
         return mItemAdapter;
     }
+
+    /**
+     * Returns the description of |position|th item. For use with tests only.
+     */
+    @VisibleForTesting
+    public String getItemDescriptionForTesting(int position) {

[Yaron, 2016/06/29 21:18:19]

Seems unnecessary. You can just use |getItemAdapterForTesting| and call
getItemDescription on that.

[juncai, 2016/06/29 22:48:38]

Done.

+        return mItemAdapter.getItemDescription(position);
+    }
 }