bluetooth-chooser: Refactor and clean up bluetooth chooser

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;
 import android.graphics.Color;
 import android.graphics.drawable.ColorDrawable;
 import android.text.SpannableString;
 import android.text.method.LinkMovementMethod;
 import android.view.Gravity;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 import android.view.ViewGroup.LayoutParams;
 import android.view.Window;
 import android.widget.AdapterView;
 import android.widget.ArrayAdapter;
 import android.widget.Button;
 import android.widget.LinearLayout;
 import android.widget.ListView;
 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.Map;
 import java.util.Set;
@@ skipping 15 matching lines @@
          *
          * @param id The id of the item selected. Blank if the dialog was closed
          * without selecting anything.
          */
         void onItemSelected(String id);
     }
 
     /**
      * A class representing one data row in the picker.
      */
     public static class ItemChooserRow {

[Ted C, 2016/10/20 21:50:18]

should/can this class be private now?

[ortuno, 2016/10/21 02:14:07]

Done.

         private final String mKey;
         private String mDescription;
 
         public ItemChooserRow(String key, String description) {
             mKey = key;
             mDescription = description;
         }
 
-        @Override
-        public boolean equals(Object obj) {
-            if (!(obj instanceof ItemChooserRow)) return false;
-            if (this == obj) return true;
-            ItemChooserRow item = (ItemChooserRow) obj;
-            return mKey.equals(item.mKey) && mDescription.equals(item.mDescription);
-        }
-
-        @Override
-        public int hashCode() {
-            return mKey.hashCode() + mDescription.hashCode();
+        public boolean hasSameContents(String key, String description) {

[Ted C, 2016/10/20 21:50:18]

non-private methods should have javadoc

[ortuno, 2016/10/21 02:14:07]

Tests use hasSameContents and a follow up patch will introduce a function that
returns an instance of this class:
https://github.com/jollycopper/ListViewColorState/blob/master/app/src/main/ja...

+            if (!mKey.equals(key)) return false;

[Ted C, 2016/10/20 21:50:18]

use TextUtils.equals

[ortuno, 2016/10/21 02:14:07]

Done.

+            if (!mDescription.equals(description)) return false;
+            return true;
         }
     }
 
     /**
      * The labels to show in the dialog.
      */
     public static class ItemChooserLabels {
         // The title at the top of the dialog.
         public final CharSequence title;
         // The message to show while there are no results.
@@ skipping 19 matching lines @@
             this.searching = searching;
             this.noneFound = noneFound;
             this.statusActive = statusActive;
             this.statusIdleNoneFound = statusIdleNoneFound;
             this.statusIdleSomeFound = statusIdleSomeFound;
             this.positiveButton = positiveButton;
         }
     }
 
     /**
+     * Item holder for performance boost.
+     */
+    private static class ViewHolder {
+        private TextView mTextView;
+
+        public ViewHolder(View view) {
+            mTextView = (TextView) view.findViewById(R.id.description);
+        }
+    }
+
+    /**
      * The various states the dialog can represent.
      */
     private enum State { STARTING, PROGRESS_UPDATE_AVAILABLE, DISCOVERY_IDLE }
 
     /**
      * An adapter for keeping track of which items to show in the dialog.
      */
     public class ItemAdapter extends ArrayAdapter<ItemChooserRow>
             implements AdapterView.OnItemClickListener {
         private final LayoutInflater mInflater;
 
-        // The background color of the highlighted item.
-        private final int mBackgroundHighlightColor;
-
-        // 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>();
 
         // Item descriptions are counted in a map.
         private Map<String, Integer> mItemDescriptionMap = new HashMap<>();
 
         // Map of keys to items so that we can access the items in O(1).
         private Map<String, ItemChooserRow> mKeyToItemMap = new HashMap<>();
 
         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 boolean isEmpty() {
             boolean isEmpty = super.isEmpty();
             if (isEmpty) {
                 assert mKeyToItemMap.isEmpty();
                 assert mDisabledEntries.isEmpty();
                 assert mItemDescriptionMap.isEmpty();
             } else {
                 assert !mKeyToItemMap.isEmpty();
                 assert !mItemDescriptionMap.isEmpty();
             }
             return isEmpty;
         }
 
-        public void addOrUpdate(ItemChooserRow item) {
-            ItemChooserRow oldItem = mKeyToItemMap.get(item.mKey);
+        public void addOrUpdate(String key, String description) {
+            ItemChooserRow oldItem = mKeyToItemMap.get(key);
             if (oldItem != null) {
-                if (oldItem.equals(item)) {
+                if (oldItem.hasSameContents(key, description)) {
                     // No need to update anything.
                     return;
                 }
-                if (!oldItem.mDescription.equals(item.mDescription)) {
+
+                if (!oldItem.mDescription.equals(description)) {

[Ted C, 2016/10/20 21:50:18]

I would use TextUtils.equals here instead (incase we ever want to support a null
mDescription

[ortuno, 2016/10/21 02:14:07]

Ah good idea. I didn't know about TextUtils. Done.

                     removeFromDescriptionsMap(oldItem.mDescription);
-                    oldItem.mDescription = item.mDescription;
+                    oldItem.mDescription = description;
                     addToDescriptionsMap(oldItem.mDescription);
                 }
+
                 notifyDataSetChanged();
                 return;
             }
-            ItemChooserRow result = mKeyToItemMap.put(item.mKey, item);
-            assert result == null;
 
-            addToDescriptionsMap(item.mDescription);
-            add(item);
+            assert !mKeyToItemMap.containsKey(key);
+            ItemChooserRow newItem = new ItemChooserRow(key, description);
+            mKeyToItemMap.put(key, newItem);
+
+            addToDescriptionsMap(newItem.mDescription);
+            add(newItem);
         }
 
-        @Override
-        public void remove(ItemChooserRow item) {
-            ItemChooserRow oldItem = mKeyToItemMap.remove(item.mKey);
+        public void remove(String key) {

[Ted C, 2016/10/20 21:50:18]

I would try to avoid overloads of names in the public api for android classes. 
I could see this being confusing.

Maybe this should be removeEntryWithKey to make it clear that it is not the
same.

[ortuno, 2016/10/21 02:14:07]

Done.

+            ItemChooserRow oldItem = mKeyToItemMap.remove(key);
             if (oldItem == null) return;
             int oldItemPosition = getPosition(oldItem);
             // If the removed item is the item that is currently selected, deselect it
             // and disable the confirm button. Otherwise if the removed item is before
             // the currently selected item, the currently selected item's index needs
             // to be adjusted by one.
             if (oldItemPosition == mSelectedItem) {
                 mSelectedItem = ListView.INVALID_POSITION;
                 mConfirmButton.setEnabled(false);
             } else if (oldItemPosition < mSelectedItem) {
@@ skipping 71 matching lines @@
             return 1;
         }
 
         @Override
         public long getItemId(int position) {
             return position;
         }
 
         @Override
         public View getView(int position, View convertView, ViewGroup parent) {
-            TextView view;
-            if (convertView instanceof TextView) {
-                view = (TextView) convertView;
+            ViewHolder row;
+            if (convertView == null) {
+                convertView = mInflater.inflate(R.layout.item_chooser_dialog_row, parent, false);
+                row = new ViewHolder(convertView);
+                convertView.setTag(row);
             } else {
-                view = (TextView) mInflater.inflate(
-                        R.layout.item_chooser_dialog_row, parent, false);
+                row = (ViewHolder) convertView.getTag();
             }
 
-            // Set highlighting for currently selected item.
-            if (position == mSelectedItem) {
-                view.setBackgroundColor(mBackgroundHighlightColor);
-                view.setTextColor(Color.WHITE);
-            } else {
-                view.setBackground(null);
-                if (!isEnabled(position)) {
-                    view.setTextColor(ApiCompatibilityUtils.getColor(getContext().getResources(),
-                            R.color.primary_text_disabled_material_light));
-                } else {
-                    view.setTextColor(mDefaultTextColor);
-                }
-            }
+            row.mTextView.setEnabled(isEnabled(position));
+            row.mTextView.setText(getDisplayText(position));
 
-            view.setText(getDisplayText(position));
-            return view;
+            return convertView;
         }
 
         @Override
         public void onItemClick(AdapterView<?> adapter, View view, int position, long id) {
             mSelectedItem = position;
             mConfirmButton.setEnabled(true);
-            mItemAdapter.notifyDataSetChanged();
+            notifyDataSetChanged();
         }
 
         private void addToDescriptionsMap(String description) {
             int count = mItemDescriptionMap.containsKey(description)
                     ? mItemDescriptionMap.get(description)
                     : 0;
             mItemDescriptionMap.put(description, count + 1);
         }
 
         private void removeFromDescriptionsMap(String description) {
@@ skipping 78 matching lines @@
             public void onClick(View v) {
                 mItemSelectedCallback.onItemSelected(mItemAdapter.getSelectedItemKey());
                 mDialog.setOnDismissListener(null);
                 mDialog.dismiss();
             }
         });
 
         mItemAdapter = new ItemAdapter(mActivity, R.layout.item_chooser_dialog_row);
         mItemAdapter.setNotifyOnChange(true);
         mListView.setAdapter(mItemAdapter);
+        mListView.setChoiceMode(ListView.CHOICE_MODE_SINGLE);
         mListView.setEmptyView(mEmptyMessage);
         mListView.setOnItemClickListener(mItemAdapter);
         mListView.setDivider(null);
         setState(State.STARTING);
 
         // The list is the main element in the dialog and it should grow and
         // shrink according to the size of the screen available.
         View listViewContainer = dialogContainer.findViewById(R.id.container);
         listViewContainer.setLayoutParams(new LinearLayout.LayoutParams(
                 LayoutParams.MATCH_PARENT,
@@ skipping 45 matching lines @@
         }
 
         mDialog.show();
     }
 
     public void dismiss() {
         mDialog.dismiss();
     }
 
     /**
-    * Add an item to the end of the list to show in the dialog if the item
-    * was not in the chooser. Otherwise update the items description.
-    *
-    * @param item The item to be added to the end of the chooser or updated.
-    */
-    public void addOrUpdateItem(ItemChooserRow item) {
+     * Adds an item to the end of the list to show in the dialog if the item
+     * was not in the chooser. Otherwise updates the items description.
+     *
+     * @param key Unique identifier for that item.
+     * @param description Text in the row.
+     */
+    public void addOrUpdateItem(String key, String description) {
         mProgressBar.setVisibility(View.GONE);
-        mItemAdapter.addOrUpdate(item);
+        mItemAdapter.addOrUpdate(key, description);
         setState(State.PROGRESS_UPDATE_AVAILABLE);
     }
 
     /**
-    * Remove an item that is shown in the dialog.
-    *
-    * @param item The item to be removed in the chooser.
-    */
-    public void removeItemFromList(ItemChooserRow item) {
-        mItemAdapter.remove(item);
+     * Removes an item that is shown in the dialog.
+     *
+     * @param key Unique identifier for the item.
+     */
+    public void removeItemFromList(String key) {
+        mItemAdapter.remove(key);
         setState(State.DISCOVERY_IDLE);
     }
 
     /**
      * Indicates the chooser that no more items will be added.
      */
     public void setIdleState() {
         mProgressBar.setVisibility(View.GONE);
         setState(State.DISCOVERY_IDLE);
     }
 
     /**
      * Sets whether the item is enabled.
-     * @param id The id of the item to affect.
+     * @param key Unique indetifier for the item.
      * @param enabled Whether the item should be enabled or not.
      */
-    public void setEnabled(String id, boolean enabled) {
-        mItemAdapter.setEnabled(id, enabled);
+    public void setEnabled(String key, boolean enabled) {
+        mItemAdapter.setEnabled(key, enabled);
     }
 
     /**
      * Clear all items from the dialog.
      */
     public void clear() {
         mItemAdapter.clear();
         setState(State.STARTING);
     }
 
@@ skipping 40 matching lines @@
     }
 
     /**
      * Returns the ItemAdapter associated with this class. For use with tests only.
      */
     @VisibleForTesting
     public ItemAdapter getItemAdapterForTesting() {
         return mItemAdapter;
     }
 }