diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt index 0c3b39af2..c50b70df4 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt @@ -38,7 +38,7 @@ class NormalBrowserTrayList @JvmOverloads constructor( private val swipeDelegate = SwipeToDeleteDelegate() private val concatAdapter by lazy { adapter as ConcatAdapter } - private val tabSorter by lazy { TabSorter(context, concatAdapter, context.components.core.store) } + private val tabSorter by lazy { TabSorter(context.settings(), concatAdapter, context.components.core.store) } private val inactiveTabsFilter: (TabSessionState) -> Boolean = filter@{ if (!context.settings().inactiveTabsAreEnabled) { return@filter false diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabSorter.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabSorter.kt index de9e1232b..8b2c98de7 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabSorter.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TabSorter.kt @@ -4,7 +4,6 @@ package org.mozilla.fenix.tabstray.browser -import android.content.Context import androidx.recyclerview.widget.ConcatAdapter import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.tabstray.Tab @@ -13,23 +12,25 @@ import mozilla.components.concept.tabstray.TabsTray import mozilla.components.feature.tabs.tabstray.TabsFeature import mozilla.components.support.base.observer.Observable import mozilla.components.support.base.observer.ObserverRegistry -import org.mozilla.fenix.ext.settings import org.mozilla.fenix.tabstray.ext.browserAdapter import org.mozilla.fenix.tabstray.ext.inactiveTabsAdapter import org.mozilla.fenix.tabstray.ext.tabGroupAdapter +import org.mozilla.fenix.utils.Settings import kotlin.math.max /** * An intermediary layer to consume tabs from [TabsFeature] for sorting into the various adapters. */ class TabSorter( - private val context: Context, + private val settings: Settings, private val concatAdapter: ConcatAdapter, private val store: BrowserStore ) : TabsTray, Observable by ObserverRegistry() { + private val groupsSet = mutableSetOf() + override fun updateTabs(tabs: Tabs) { - val inactiveTabs = tabs.list.getInactiveTabs(context) - val searchTermTabs = tabs.list.getSearchGroupTabs(context) + val inactiveTabs = tabs.list.getInactiveTabs(settings) + val searchTermTabs = tabs.list.getSearchGroupTabs(settings) val normalTabs = tabs.list - inactiveTabs - searchTermTabs val selectedTabId = store.state.selectedTabId @@ -40,7 +41,11 @@ class TabSorter( // Tab groups // We don't need to provide a selectedId, because the [TabGroupAdapter] has that built-in with support from // NormalBrowserPageViewHolder.scrollToTab. - val (groups, remainderTabs) = searchTermTabs.toSearchGroups() + val (groups, remainderTabs) = searchTermTabs.toSearchGroups(groupsSet) + + groupsSet.clear() + groupsSet.addAll(groups.map { it.title }) + concatAdapter.tabGroupAdapter.submitList(groups) // Normal tabs. @@ -60,8 +65,8 @@ private fun List.findSelectedIndex(tabId: String?): Int { /** * Returns a list of inactive tabs based on our preferences. */ -private fun List.getInactiveTabs(context: Context): List { - val inactiveTabsEnabled = context.settings().inactiveTabsAreEnabled +private fun List.getInactiveTabs(settings: Settings): List { + val inactiveTabsEnabled = settings.inactiveTabsAreEnabled return if (inactiveTabsEnabled) { filter { !it.isActive(maxActiveTime) } } else { @@ -72,9 +77,9 @@ private fun List.getInactiveTabs(context: Context): List { /** * Returns a list of search term tabs based on our preferences. */ -private fun List.getSearchGroupTabs(context: Context): List { - val inactiveTabsEnabled = context.settings().inactiveTabsAreEnabled - val tabGroupsEnabled = context.settings().searchTermTabGroupsAreEnabled +private fun List.getSearchGroupTabs(settings: Settings): List { + val inactiveTabsEnabled = settings.inactiveTabsAreEnabled + val tabGroupsEnabled = settings.searchTermTabGroupsAreEnabled return when { tabGroupsEnabled && inactiveTabsEnabled -> filter { it.searchTerm.isNotBlank() && it.isActive(maxActiveTime) } @@ -107,7 +112,7 @@ private fun Tab.isActive(maxActiveTime: Long): Boolean { * * See also: https://github.com/mozilla-mobile/android-components/issues/11012 */ -private fun List.toSearchGroups(): Pair, List> { +private fun List.toSearchGroups(groupSet: Set): Pair, List> { val data = groupBy { it.searchTerm.lowercase() } val groupings = data.map { mapEntry -> @@ -127,7 +132,7 @@ private fun List.toSearchGroups(): Pair, List 1 }.sortedBy { it.lastAccess } + val groups = groupings.filter { it.tabs.size > 1 || groupSet.contains(it.title) }.sortedBy { it.lastAccess } val remainderTabs = (groupings - groups).flatMap { it.tabs } return groups to remainderTabs diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/browser/Tab.kt b/app/src/test/java/org/mozilla/fenix/tabstray/browser/Tab.kt index 738ee7fca..45638e716 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/browser/Tab.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/browser/Tab.kt @@ -11,8 +11,12 @@ import java.util.UUID * Helper for writing tests that need a [Tab]. */ fun createTab( - tabId: String = UUID.randomUUID().toString() + tabId: String = UUID.randomUUID().toString(), + lastAccess: Long = 0L, + searchTerm: String = "" ) = Tab( - tabId, - "https://mozilla.org" + id = tabId, + url = "https://mozilla.org", + lastAccess = lastAccess, + searchTerm = searchTerm ) diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabSorterTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabSorterTest.kt new file mode 100644 index 000000000..91f0b5d8e --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TabSorterTest.kt @@ -0,0 +1,315 @@ +/* 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 androidx.recyclerview.widget.ConcatAdapter +import io.mockk.every +import io.mockk.mockk +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.concept.tabstray.Tabs +import mozilla.components.support.test.mock +import mozilla.components.support.test.robolectric.testContext +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import org.mozilla.fenix.tabstray.TrayPagerAdapter.Companion.INACTIVE_TABS_FEATURE_NAME +import org.mozilla.fenix.tabstray.TrayPagerAdapter.Companion.TABS_TRAY_FEATURE_NAME +import org.mozilla.fenix.tabstray.TrayPagerAdapter.Companion.TAB_GROUP_FEATURE_NAME +import org.mozilla.fenix.tabstray.ext.browserAdapter +import org.mozilla.fenix.tabstray.ext.inactiveTabsAdapter +import org.mozilla.fenix.tabstray.ext.tabGroupAdapter +import org.mozilla.fenix.tabstray.ext.titleHeaderAdapter +import org.mozilla.fenix.utils.Settings + +@RunWith(FenixRobolectricTestRunner::class) +class TabSorterTest { + private val context = testContext + private val settings: Settings = mockk() + private var inactiveTimestamp = 0L + + @Before + fun setUp() { + every { settings.inactiveTabsAreEnabled }.answers { true } + every { settings.searchTermTabGroupsAreEnabled }.answers { true } + } + + @Test + fun `WHEN updated with one normal tab THEN adapter have only one normal tab and no header`() { + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis()) + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 1) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 0) + assertEquals(adapter.titleHeaderAdapter.itemCount, 0) + assertEquals(adapter.browserAdapter.itemCount, 1) + } + + @Test + fun `WHEN updated with one normal tab and two search term tab THEN adapter have normal tab and a search group`() { + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis()), + createTab("tab2", System.currentTimeMillis(), searchTerm = "mozilla"), + createTab("tab3", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 2) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 1) + assertEquals(adapter.browserAdapter.itemCount, 1) + } + + @Test + fun `WHEN updated with one normal tab, one inactive tab and two search term tab THEN adapter have normal tab, inactive tab and a search group`() { + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis()), + createTab("tab2", inactiveTimestamp), + createTab("tab3", System.currentTimeMillis(), searchTerm = "mozilla"), + createTab("tab4", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 3) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 1) + assertEquals(adapter.tabGroupAdapter.itemCount, 1) + assertEquals(adapter.browserAdapter.itemCount, 1) + } + + @Test + fun `WHEN inactive tabs is off THEN adapter have no inactive tab`() { + every { settings.inactiveTabsAreEnabled }.answers { false } + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis()), + createTab("tab2", inactiveTimestamp), + createTab("tab3", System.currentTimeMillis(), searchTerm = "mozilla"), + createTab("tab4", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 3) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 1) + assertEquals(adapter.browserAdapter.itemCount, 2) + } + + @Test + fun `WHEN search term tabs is off THEN adapter have no search term group`() { + every { settings.searchTermTabGroupsAreEnabled }.answers { false } + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis()), + createTab("tab2", inactiveTimestamp), + createTab("tab3", System.currentTimeMillis(), searchTerm = "mozilla"), + createTab("tab4", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 4) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 1) + assertEquals(adapter.tabGroupAdapter.itemCount, 0) + assertEquals(adapter.browserAdapter.itemCount, 3) + } + + @Test + fun `WHEN both inactive tabs and search term tabs are off THEN adapter have only normal tabs`() { + every { settings.inactiveTabsAreEnabled }.answers { false } + every { settings.searchTermTabGroupsAreEnabled }.answers { false } + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis()), + createTab("tab2", inactiveTimestamp), + createTab("tab3", System.currentTimeMillis(), searchTerm = "mozilla"), + createTab("tab4", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 4) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 0) + assertEquals(adapter.browserAdapter.itemCount, 4) + } + + @Test + fun `WHEN only one search term tab THEN there is no search group`() { + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 1) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 0) + assertEquals(adapter.browserAdapter.itemCount, 1) + } + + @Test + fun `WHEN remove second last one search term tab THEN search group is kept even if there's only one tab`() { + val store = BrowserStore( + BrowserState( + tabs = emptyList() + ) + ) + val adapter = ConcatAdapter( + InactiveTabsAdapter(context, mock(), mock(), INACTIVE_TABS_FEATURE_NAME, settings), + TabGroupAdapter(context, mock(), mock(), TAB_GROUP_FEATURE_NAME), + TitleHeaderAdapter(store, context.settings()), + BrowserTabsAdapter(context, mock(), mock(), TABS_TRAY_FEATURE_NAME) + ) + val tabSorter = TabSorter(settings, adapter, store) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis(), searchTerm = "mozilla"), + createTab("tab2", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 1) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 1) + assertEquals(adapter.browserAdapter.itemCount, 0) + + tabSorter.updateTabs( + Tabs( + list = listOf( + createTab("tab1", System.currentTimeMillis(), searchTerm = "mozilla") + ), + selectedIndex = 0 + ) + ) + + assertEquals(adapter.itemCount, 1) + assertEquals(adapter.inactiveTabsAdapter.inActiveTabsCount, 0) + assertEquals(adapter.tabGroupAdapter.itemCount, 1) + assertEquals(adapter.browserAdapter.itemCount, 0) + } +}