Bug 1278644 - Stable getItemID implementation for ClientsAdapter. r=sebastian, a=sledru
authorGrigory Kruglov <gkruglov@mozilla.com>
Wed, 13 Jul 2016 15:14:50 -0700
changeset 325588 e3d202c9c23067c2e3f18ee0d53a32ba98d4bcea
parent 325587 822a68704ed4ab149303597be3a8b2870a9a9f77
child 325589 db67e1188e8dd1d3a798d1bb9dae43c8a5e41abc
push id9843
push userryanvm@gmail.com
push dateFri, 29 Jul 2016 20:53:22 +0000
treeherdermozilla-aurora@42c3abe40b3b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssebastian, sledru
bugs1278644
milestone49.0a2
Bug 1278644 - Stable getItemID implementation for ClientsAdapter. r=sebastian, a=sledru
mobile/android/base/java/org/mozilla/gecko/home/ClientsAdapter.java
mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java
mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
--- a/mobile/android/base/java/org/mozilla/gecko/home/ClientsAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/ClientsAdapter.java
@@ -61,16 +61,18 @@ public class ClientsAdapter extends Recy
         // This races when multiple Fragments are created. That's okay: one
         // will win, and thereafter, all will be okay. If we create and then
         // drop an instance the shared SharedPreferences backing all the
         // instances will maintain the state for us. Since everything happens on
         // the UI thread, this doesn't even need to be volatile.
         if (sState == null) {
             sState = new RemoteTabsExpandableListState(GeckoSharedPrefs.forProfile(context));
         }
+
+        this.setHasStableIds(true);
     }
 
     @Override
     public CombinedHistoryItem onCreateViewHolder(ViewGroup parent, int viewType) {
         final LayoutInflater inflater = LayoutInflater.from(parent.getContext());
         final View view;
 
         final CombinedHistoryItem.ItemType itemType = CombinedHistoryItem.ItemType.viewTypeToItemType(viewType);
@@ -140,16 +142,60 @@ public class ClientsAdapter extends Recy
         }
     }
 
     @Override
     public int getItemViewType(int position) {
         return CombinedHistoryItem.ItemType.itemTypeToViewType(getItemTypeForPosition(position));
     }
 
+    @Override
+    public long getItemId(int position) {
+        // RecyclerView.NO_ID is -1, so start our hard-coded IDs at -2.
+        final int NAVIGATION_BACK_ID = -2;
+        final int HIDDEN_DEVICES_ID = -3;
+
+        final String clientGuid;
+        // adapterList is a list of tuples (clientGuid, tabId).
+        final Pair<String, Integer> pair = adapterList.get(position);
+
+        switch (getItemTypeForPosition(position)) {
+            case NAVIGATION_BACK:
+                return NAVIGATION_BACK_ID;
+
+            case HIDDEN_DEVICES:
+                return HIDDEN_DEVICES_ID;
+
+            // For Clients, return hashCode of their GUIDs.
+            case CLIENT:
+                clientGuid = pair.first;
+                return clientGuid.hashCode();
+
+            // For Tabs, return hashCode of their URLs.
+            case CHILD:
+                clientGuid = pair.first;
+                final Integer tabId = pair.second;
+
+                final RemoteClient remoteClient = visibleClients.get(clientGuid);
+                if (remoteClient == null) {
+                    return RecyclerView.NO_ID;
+                }
+
+                final RemoteTab remoteTab = remoteClient.tabs.get(tabId);
+                if (remoteTab == null) {
+                    return RecyclerView.NO_ID;
+                }
+
+                return remoteTab.url.hashCode();
+
+            default:
+                throw new IllegalStateException("Unexpected Home Panel item type");
+        }
+    }
+
     @UiThread
     public void setClients(List<RemoteClient> clients) {
         adapterList.clear();
         adapterList.add(null);
 
         hiddenClients.clear();
         visibleClients.clear();
 
--- a/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java
@@ -42,16 +42,17 @@ public class CombinedHistoryAdapter exte
 
     // We use a sparse array to store each section header's position in the panel [more cheaply than a HashMap].
     private final SparseArray<SectionHeader> sectionHeaders;
 
     public CombinedHistoryAdapter(Resources resources) {
         super();
         sectionHeaders = new SparseArray<>();
         HistorySectionsHelper.updateRecentSectionOffset(resources, sectionDateRangeArray);
+        this.setHasStableIds(true);
     }
 
     public void setHistory(Cursor history) {
         historyCursor = history;
         populateSectionHeaders(historyCursor, sectionHeaders);
         notifyDataSetChanged();
     }
 
@@ -157,16 +158,62 @@ public class CombinedHistoryAdapter exte
 
     @Override
     public int getItemCount() {
         final int historySize = historyCursor == null ? 0 : historyCursor.getCount();
         return historySize + sectionHeaders.size() + CombinedHistoryPanel.NUM_SMART_FOLDERS;
     }
 
     /**
+     * Returns stable ID for each position. Data behind historyCursor is a sorted Combined view.
+     *
+     * @param position view item position for which to generate a stable ID
+     * @return stable ID for given position
+     */
+    @Override
+    public long getItemId(int position) {
+        // Two randomly selected large primes used to generate non-clashing IDs.
+        final long PRIME_BOOKMARKS = 32416189867L;
+        final long PRIME_SECTION_HEADERS = 32416187737L;
+
+        // RecyclerView.NO_ID is -1, so let's start from -2 for our hard-coded IDs.
+        final int RECENT_TABS_ID = -2;
+        final int SYNCED_DEVICES_ID = -3;
+
+        switch (getItemTypeForPosition(position)) {
+            case RECENT_TABS:
+                return RECENT_TABS_ID;
+            case SYNCED_DEVICES:
+                return SYNCED_DEVICES_ID;
+            case SECTION_HEADER:
+                // We might have multiple section headers, so we try get unique IDs for them.
+                return position * PRIME_SECTION_HEADERS;
+            case HISTORY:
+                if (!historyCursor.moveToPosition(position)) {
+                    return RecyclerView.NO_ID;
+                }
+
+                final int historyIdCol = historyCursor.getColumnIndexOrThrow(BrowserContract.Combined.HISTORY_ID);
+                final long historyId = historyCursor.getLong(historyIdCol);
+
+                if (historyId != -1) {
+                    return historyId;
+                }
+
+                final int bookmarkIdCol = historyCursor.getColumnIndexOrThrow(BrowserContract.Combined.BOOKMARK_ID);
+                final long bookmarkId = historyCursor.getLong(bookmarkIdCol);
+
+                // Avoid clashing with historyId.
+                return bookmarkId * PRIME_BOOKMARKS;
+            default:
+                throw new IllegalStateException("Unexpected Home Panel item type");
+        }
+    }
+
+    /**
      * Add only the SectionHeaders that have history items within their range to a SparseArray, where the
      * array index is the position of the header in the history-only (no clients) ordering.
      * @param c data Cursor
      * @param sparseArray SparseArray to populate
      */
     private static void populateSectionHeaders(Cursor c, SparseArray<SectionHeader> sparseArray) {
         sparseArray.clear();
 
--- a/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
@@ -521,16 +521,29 @@ public class TopSitesPanel extends HomeF
             return Math.max(0, super.getCount() - mMaxGridEntries);
         }
 
         @Override
         public Object getItem(int position) {
             return super.getItem(position + mMaxGridEntries);
         }
 
+        /**
+         * We have to override default getItemId implementation, since for a given position, it returns
+         * value of the _id column. In our case _id is always 0 (see Combined view).
+         */
+        @Override
+        public long getItemId(int position) {
+            final int adjustedPosition = position + mMaxGridEntries;
+            final Cursor cursor = getCursor();
+
+            cursor.moveToPosition(adjustedPosition);
+            return getItemIdForTopSitesCursor(cursor);
+        }
+
         @Override
         public void bindView(View view, Context context, Cursor cursor) {
             final int position = cursor.getPosition();
             cursor.moveToPosition(position + mMaxGridEntries);
 
             final TwoLinePageRow row = (TwoLinePageRow) view;
             row.updateFromCursor(cursor);
         }
@@ -580,33 +593,26 @@ public class TopSitesPanel extends HomeF
                 // This will force each view to load favicons for the missing
                 // thumbnails if necessary.
                 gridItem.markAsDirty();
             }
 
             notifyDataSetChanged();
         }
 
+        /**
+         * We have to override default getItemId implementation, since for a given position, it returns
+         * value of the _id column. In our case _id is always 0 (see Combined view).
+         */
         @Override
         public long getItemId(int position) {
-            // We are trying to return stable ids so that Android can recycle views appropriately:
-            // * If we have a history id then we return it
-            // * If we only have a bookmark id then we negate it and return it. We negate it in order
-            //   to avoid clashing/conflicting with history ids.
-
             final Cursor cursor = getCursor();
             cursor.moveToPosition(position);
 
-            final long historyId = cursor.getLong(cursor.getColumnIndexOrThrow(TopSites.HISTORY_ID));
-            if (historyId != 0) {
-                return historyId;
-            }
-
-            final long bookmarkId = cursor.getLong(cursor.getColumnIndexOrThrow(TopSites.BOOKMARK_ID));
-            return -1 * bookmarkId;
+            return getItemIdForTopSitesCursor(cursor);
         }
 
         @Override
         public void bindView(View bindView, Context context, Cursor cursor) {
             final String url = cursor.getString(cursor.getColumnIndexOrThrow(TopSites.URL));
             final String title = cursor.getString(cursor.getColumnIndexOrThrow(TopSites.TITLE));
             final int type = cursor.getInt(cursor.getColumnIndexOrThrow(TopSites.TYPE));
 
@@ -960,9 +966,30 @@ public class TopSitesPanel extends HomeF
 
         @Override
         public void onLoaderReset(Loader<Map<String, ThumbnailInfo>> loader) {
             if (mGridAdapter != null) {
                 mGridAdapter.updateThumbnails(null);
             }
         }
     }
+
+    /**
+     * We are trying to return stable IDs so that Android can recycle views appropriately:
+     * - If we have a history ID then we return it
+     * - If we only have a bookmark ID then we negate it and return it. We negate it in order
+     *   to avoid clashing/conflicting with history IDs.
+     *
+     * @param cursorInPosition Cursor already moved to position for which we're getting a stable ID
+     * @return Stable ID for a given cursor
+     */
+    private static long getItemIdForTopSitesCursor(final Cursor cursorInPosition) {
+        final int historyIdCol = cursorInPosition.getColumnIndexOrThrow(TopSites.HISTORY_ID);
+        final long historyId = cursorInPosition.getLong(historyIdCol);
+        if (historyId != 0) {
+            return historyId;
+        }
+
+        final int bookmarkIdCol = cursorInPosition.getColumnIndexOrThrow(TopSites.BOOKMARK_ID);
+        final long bookmarkId = cursorInPosition.getLong(bookmarkIdCol);
+        return -1 * bookmarkId;
+    }
 }