diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt index 3a9757d51..7a9fd4672 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt @@ -45,10 +45,6 @@ class BrowserTabsAdapter( private val imageLoader = ThumbnailLoader(context.components.core.thumbnailStorage) - init { - setHasStableIds(true) - } - override fun getItemViewType(position: Int): Int { return if (context.settings().gridTabView) { ViewType.GRID.ordinal @@ -64,8 +60,6 @@ class BrowserTabsAdapter( } } - override fun getItemId(position: Int) = position.toLong() - override fun onBindViewHolder(holder: TabsTrayViewHolder, position: Int) { super.onBindViewHolder(holder, position) @@ -79,7 +73,7 @@ class BrowserTabsAdapter( } tracker?.let { - holder.showTabIsMultiSelectEnabled(it.isSelected(position.toLong())) + holder.showTabIsMultiSelectEnabled(it.isSelected(getItemId(position))) } } } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabAdapterIdStorage.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabAdapterIdStorage.kt new file mode 100644 index 000000000..e1760debe --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabAdapterIdStorage.kt @@ -0,0 +1,45 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.tabstray.browser + +import android.util.LruCache +import androidx.recyclerview.widget.RecyclerView +import mozilla.components.concept.tabstray.Tab + +internal const val INITIAL_NUMBER_OF_TABS = 20 +internal const val CACHE_SIZE_MULTIPLIER = 1.5 + +/** + * Storage for Browser tabs that need a stable ID for each item in a [RecyclerView.Adapter]. + * This ID is commonly needed by [RecyclerView.Adapter.getItemId] when + * enabling [RecyclerView.Adapter.setHasStableIds]. + */ +internal class TabAdapterIdStorage(initialSize: Int = INITIAL_NUMBER_OF_TABS) { + private val uniqueTabIds = LruCache(initialSize) + private var lastUsedSuggestionId = 0L + + /** + * Returns a unique tab ID for the given [Tab]. + */ + fun getStableId(tab: Tab): Long { + val key = tab.id + return uniqueTabIds[key] ?: run { + lastUsedSuggestionId += 1 + uniqueTabIds.put(key, lastUsedSuggestionId) + lastUsedSuggestionId + } + } + + /** + * Resizes the internal cache size if the [count] is larger than what is currently available. + */ + fun resizeCacheIfNeeded(count: Int) { + val currentMaxSize = uniqueTabIds.maxSize() + if (count > currentMaxSize) { + val newMaxSize = (count * CACHE_SIZE_MULTIPLIER).toInt() + uniqueTabIds.resize(newMaxSize) + } + } +} diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabsAdapter.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabsAdapter.kt index e8cbcb071..b0f1a1430 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabsAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabsAdapter.kt @@ -13,11 +13,17 @@ import mozilla.components.concept.tabstray.TabsTray import mozilla.components.support.base.observer.Observable import mozilla.components.support.base.observer.ObserverRegistry -// The previous tabs adapter was very restrictive and required Fenix to jump through -// may hoops to access and update certain methods. An abstract adapter is easier to manage -// for Android UI APIs. -// -// TODO Let's upstream this to AC with tests. +/** + * RecyclerView adapter implementation to display a list/grid of tabs. + * + * The previous tabs adapter was very restrictive and required Fenix to jump through + * may hoops to access and update certain methods. An abstract adapter is easier to manage + * for Android UI APIs. + * + * TODO Let's upstream this to AC with tests. + * + * @param delegate TabsTray.Observer registry to allow `TabsAdapter` to conform to `Observable`. + */ abstract class TabsAdapter( delegate: Observable = ObserverRegistry() ) : RecyclerView.Adapter(), TabsTray, Observable by delegate { @@ -25,10 +31,18 @@ abstract class TabsAdapter( protected var tabs: Tabs? = null protected var styling: TabsTrayStyling = TabsTrayStyling() + private val idStorage = TabAdapterIdStorage() + + init { + setHasStableIds(true) + } + @CallSuper override fun updateTabs(tabs: Tabs) { this.tabs = tabs + idStorage.resizeCacheIfNeeded(tabs.list.size) + notifyObservers { onTabsUpdated() } } @@ -39,6 +53,13 @@ abstract class TabsAdapter( holder.bind(tabs.list[position], isTabSelected(tabs, position), styling, this) } + override fun getItemId(position: Int): Long { + val key = tabs?.list?.get(position) + ?: throw IllegalStateException("Unknown tab for position $position") + + return idStorage.getStableId(key) + } + override fun getItemCount(): Int = tabs?.list?.size ?: 0 final override fun isTabSelected(tabs: Tabs, position: Int): Boolean = diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabAdapterIdStorageTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabAdapterIdStorageTest.kt new file mode 100644 index 000000000..881112946 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabAdapterIdStorageTest.kt @@ -0,0 +1,85 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.tabstray.browser + +import mozilla.components.concept.tabstray.Tab +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import java.util.UUID + +@RunWith(FenixRobolectricTestRunner::class) +class TabAdapterIdStorageTest { + + @Test + fun `the same ID is returned when queried multiple times`() { + val storage = TabAdapterIdStorage() + val tab = createTab() + + val id1 = storage.getStableId(tab) + val id2 = storage.getStableId(tab) + + assertEquals(id1, id2) + } + + @Test + fun `the same ID is returned when the cache is at max`() { + val storage = TabAdapterIdStorage(2) + val tab1 = createTab() + val tab2 = createTab() + + val id1 = storage.getStableId(tab1) + val id2 = storage.getStableId(tab2) + val id1Again = storage.getStableId(tab1) + + assertEquals(id1, id1Again) + assertNotEquals(id1, id2) + } + + @Test + fun `the same ID is NOT returned if the cache is over max`() { + val storage = TabAdapterIdStorage(2) + val tab1 = createTab() + val tab2 = createTab() + val tab3 = createTab() + + val id1 = storage.getStableId(tab1) + val id2 = storage.getStableId(tab2) + val id3 = storage.getStableId(tab3) + val id1Again = storage.getStableId(tab1) + + assertNotEquals(id1, id1Again) + assertNotEquals(id1, id2) + assertNotEquals(id1, id3) + } + + @Test + fun `the same ID is returned if the cache is resized when full`() { + val storage = TabAdapterIdStorage(2) + val tab1 = createTab() + val tab2 = createTab() + val tab3 = createTab() + + val id1 = storage.getStableId(tab1) + val id2 = storage.getStableId(tab2) + + storage.resizeCacheIfNeeded(3) + + val id3 = storage.getStableId(tab3) + val id1Again = storage.getStableId(tab1) + + assertEquals(id1, id1Again) + assertNotEquals(id1, id2) + assertNotEquals(id1, id3) + assertNotEquals(id2, id3) + } +} + +fun createTab() = Tab( + UUID.randomUUID().toString(), + "https://mozilla.org" +) diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabsAdapterTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabsAdapterTest.kt new file mode 100644 index 000000000..382502470 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabsAdapterTest.kt @@ -0,0 +1,76 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.tabstray.browser + +import android.view.View +import android.view.ViewGroup +import mozilla.components.browser.tabstray.TabViewHolder +import mozilla.components.browser.tabstray.TabsTrayStyling +import mozilla.components.concept.tabstray.Tab +import mozilla.components.concept.tabstray.Tabs +import mozilla.components.concept.tabstray.TabsTray +import mozilla.components.support.base.observer.Observable +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner + +@RunWith(FenixRobolectricTestRunner::class) +class TabsAdapterTest { + + lateinit var adapter: TabsAdapter + + @Before + fun setup() { + adapter = TestTabsAdapter() + } + + @Test + fun `getItemId gives a new ID for each position`() { + val (tab1, tab2, tab3) = Triple(createTab(), createTab(), createTab()) + val tabs = Tabs( + list = listOf(tab1, tab2, tab3), + selectedIndex = 0 + ) + + adapter.updateTabs(tabs) + + val id1 = adapter.getItemId(0) + val id2 = adapter.getItemId(1) + val id3 = adapter.getItemId(2) + val id1Again = adapter.getItemId(0) + + assertEquals(id1, id1Again) + assertNotEquals(id1, id2) + assertNotEquals(id1, id3) + assertNotEquals(id2, id3) + } + + @Test(expected = IllegalStateException::class) + fun `getItemId throws if a tab does not exist for the position`() { + adapter.getItemId(4) + } + + class TestTabsAdapter : TabsAdapter() { + + inner class ViewHolder(view: View) : TabViewHolder(view) { + override var tab: Tab? = null + + override fun bind( + tab: Tab, + isSelected: Boolean, + styling: TabsTrayStyling, + observable: Observable + ) = throw UnsupportedOperationException() + } + + override fun onCreateViewHolder( + parent: ViewGroup, + viewType: Int + ): ViewHolder = throw UnsupportedOperationException() + } +}