From e6653978904a690cafeb9cb3f3dd163e574a0f99 Mon Sep 17 00:00:00 2001 From: Mugurell Date: Tue, 16 Nov 2021 16:24:51 +0200 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/22442 - Don't display individual tab if part of the Jump back in tabs group We'll show as a recent tab the next most recent tab not part of the last active search group. --- .../org/mozilla/fenix/ext/BrowserState.kt | 27 +++++-- .../org/mozilla/fenix/ext/BrowserStateTest.kt | 62 +++++++++++++- .../fenix/home/RecentTabsListFeatureTest.kt | 81 ++++++++++--------- 3 files changed, 122 insertions(+), 48 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt b/app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt index 9843bdb49b..88c578d999 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt @@ -30,16 +30,24 @@ val maxActiveTime = TimeUnit.DAYS.toMillis(DEFAULT_ACTIVE_DAYS) /** * Get the last opened normal tab, last tab with in progress media and last search term group, if available. * - * @return A list of the last opened tab, last tab with in progress media and last search term group - * if distinct and available or an empty list. + * @return A list of the last opened tab not part of the last active search group and + * the last active search group if these are available or an empty list. */ fun BrowserState.asRecentTabs(): List { return mutableListOf().apply { - val lastOpenedNormalTab = lastOpenedNormalTab + val mostRecentTabsGroup = lastSearchGroup + val mostRecentTabNotInGroup = if (mostRecentTabsGroup == null) { + lastOpenedNormalTab + } else { + listOf(selectedNormalTab) + .plus(normalTabs.sortedByDescending { it.lastAccess }) + .minus(lastTabGroup?.tabs ?: emptyList()) + .firstOrNull() + } - lastOpenedNormalTab?.let { add(RecentTab.Tab(it)) } + mostRecentTabNotInGroup?.let { add(RecentTab.Tab(it)) } - lastSearchGroup?.let { add(it) } + mostRecentTabsGroup?.let { add(it) } } } @@ -67,12 +75,19 @@ val BrowserState.inProgressMediaTab: TabSessionState? .filter { it.hasMediaPlayed() } .maxByOrNull { it.lastMediaAccessState.lastMediaAccess } +/** + * Get the most recently accessed [TabGroup]. + * Result will be `null` if the currently open normal tabs are not part of a search group. + */ +val BrowserState.lastTabGroup: TabGroup? + get() = normalTabs.toSearchGroup().first.lastOrNull() + /** * Get the most recent search term group. */ val BrowserState.lastSearchGroup: RecentTab.SearchGroup? get() { - val tabGroup = normalTabs.toSearchGroup().first.lastOrNull() ?: return null + val tabGroup = lastTabGroup ?: return null val firstTab = tabGroup.tabs.firstOrNull() ?: return null return RecentTab.SearchGroup( diff --git a/app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt b/app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt index 90dfad04a1..e4fe509df8 100644 --- a/app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt +++ b/app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt @@ -15,6 +15,7 @@ import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test import org.mozilla.fenix.home.recenttabs.RecentTab +import org.mozilla.fenix.tabstray.browser.TabGroup import org.mozilla.fenix.utils.Settings class BrowserStateTest { @@ -158,7 +159,7 @@ class BrowserStateTest { } @Test - fun `GIVEN a tab group exists WHEN recentTabs is called THEN return a tab group`() { + fun `GIVEN only normal tabs from a search group are open WHEN recentTabs is called THEN return only the tab group`() { val searchGroupTab = createTab( url = "https://www.mozilla.org", id = "1", @@ -175,8 +176,36 @@ class BrowserStateTest { val result = browserState.asRecentTabs() + assertEquals(1, result.size) + assert(result[0] is RecentTab.SearchGroup) + assertEquals(searchGroupTab.historyMetadata?.searchTerm, (result[0] as RecentTab.SearchGroup).searchTerm) + assertEquals(searchGroupTab.id, (result[0] as RecentTab.SearchGroup).tabId) + assertEquals(searchGroupTab.content.url, (result[0] as RecentTab.SearchGroup).url) + assertEquals(searchGroupTab.content.thumbnail, (result[0] as RecentTab.SearchGroup).thumbnail) + assertEquals(2, (result[0] as RecentTab.SearchGroup).count) + } + + @Test + fun `GIVEN tabs with different search terms are opened WHEN recentTabs is called THEN return the most recent tab and tab group`() { + val searchGroupTab = createTab( + url = "https://www.mozilla.org", + id = "1", + historyMetadata = HistoryMetadataKey( + url = "https://www.mozilla.org", + searchTerm = "Test", + referrerUrl = "https://www.mozilla.org" + ) + ) + val otherTab = createTab(url = "https://www.mozilla.org/firefox", id = "2") + val browserState = BrowserState( + tabs = listOf(searchGroupTab, otherTab, searchGroupTab), + selectedTabId = searchGroupTab.id + ) + + val result = browserState.asRecentTabs() + assertEquals(2, result.size) - assertEquals(searchGroupTab, (result[0] as RecentTab.Tab).state) + assertEquals(otherTab, (result[0] as RecentTab.Tab).state) assert(result[1] is RecentTab.SearchGroup) assertEquals(searchGroupTab.historyMetadata?.searchTerm, (result[1] as RecentTab.SearchGroup).searchTerm) assertEquals(searchGroupTab.id, (result[1] as RecentTab.SearchGroup).tabId) @@ -413,4 +442,33 @@ class BrowserStateTest { assertEquals(2, result.size) assertTrue(result.containsAll(listOf(normalTab1, normalTab3))) } + + @Test + fun `GIVEN tabs exist with search terms WHEN lastTabGroup is called THEN return the last accessed TabGroup`() { + val tab1 = createTab(url = "url1", id = "id1", searchTerms = "test1", lastAccess = 10) + val tab2 = createTab(url = "url2", id = "id2", searchTerms = "test1", lastAccess = 11) + val tab3 = createTab(url = "url3", id = "id3", searchTerms = "test3", lastAccess = 1000) + val tab4 = createTab(url = "url4", id = "id4", searchTerms = "test3", lastAccess = 1111) + val tab5 = createTab(url = "url5", id = "id5", searchTerms = "test5", lastAccess = 100) + val tab6 = createTab(url = "url6", id = "id6", searchTerms = "test5", lastAccess = 111) + val browserState = BrowserState( + tabs = listOf(tab1, tab2, tab3, tab4, tab5, tab6) + ) + val expected = TabGroup("Test3", listOf(tab3, tab4), tab4.lastAccess) + + val result = browserState.lastTabGroup + + assertEquals(expected, result) + } + + @Test + fun `GIVEN no tabs exist with search terms WHEN lastTabGroup is called THEN return the last accessed TabGroup`() { + val tab1 = createTab(url = "url1", id = "id1", lastAccess = 10) + val tab2 = createTab(url = "url2", id = "id2", lastAccess = 11) + val browserState = BrowserState(tabs = listOf(tab1, tab2)) + + val result = browserState.lastTabGroup + + assertNull(result) + } } diff --git a/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt b/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt index 30c31ce530..a3cb9242ed 100644 --- a/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt @@ -386,7 +386,7 @@ class RecentTabsListFeatureTest { } @Test - fun `GIVEN a selected tab group WHEN the feature starts THEN dispatch the selected tab group as a recent tab list`() { + fun `GIVEN only tabs from a search group WHEN the feature starts THEN dispatch the selected tab group as a recent tab list`() { val tab1 = createTab( url = "https://www.mozilla.org", id = "1", @@ -422,9 +422,8 @@ class RecentTabsListFeatureTest { homeStore.waitUntilIdle() - assertEquals(2, homeStore.state.recentTabs.size) - assertTrue(homeStore.state.recentTabs[0] is RecentTab.Tab) - val searchGroup = (homeStore.state.recentTabs[1] as RecentTab.SearchGroup) + assertEquals(1, homeStore.state.recentTabs.size) + val searchGroup = (homeStore.state.recentTabs[0] as RecentTab.SearchGroup) assertEquals(searchGroup.searchTerm, "Test search term") assertEquals(searchGroup.tabId, "1") assertEquals(searchGroup.url, "https://www.mozilla.org") @@ -433,11 +432,17 @@ class RecentTabsListFeatureTest { } @Test - fun `GIVEN a tab group with one tab and a selected tab WHEN the feature starts THEN dispatch selected tab as a recent tab list`() { + fun `GIVEN tabs with different search terms are opened WHEN the feature starts THEN dispatch the last active tab and last active search group as recent tabs list`() { val tab1 = createTab( url = "https://www.mozilla.org", - id = "1" + id = "1", + historyMetadata = HistoryMetadataKey( + url = "https://www.mozilla.org", + searchTerm = "test search term", + referrerUrl = "https://www.mozilla.org" + ) ) + val tab2 = createTab( url = "https://www.mozilla.org", id = "2", @@ -447,7 +452,8 @@ class RecentTabsListFeatureTest { referrerUrl = "https://www.mozilla.org" ) ) - val tabs = listOf(tab1, tab2) + val tab3 = createTab(url = "https://www.mozilla.org/firefox", id = "3") + val tabs = listOf(tab1, tab2, tab3) val browserStore = BrowserStore( BrowserState( tabs = tabs, @@ -463,13 +469,19 @@ class RecentTabsListFeatureTest { homeStore.waitUntilIdle() - assertEquals(1, homeStore.state.recentTabs.size) + assertEquals(2, homeStore.state.recentTabs.size) assertTrue(homeStore.state.recentTabs[0] is RecentTab.Tab) - assertEquals(tab1, (homeStore.state.recentTabs[0] as RecentTab.Tab).state) + assertEquals(tab3, (homeStore.state.recentTabs[0] as RecentTab.Tab).state) + val searchGroup = (homeStore.state.recentTabs[1] as RecentTab.SearchGroup) + assertEquals(searchGroup.searchTerm, "Test search term") + assertEquals(searchGroup.tabId, "1") + assertEquals(searchGroup.url, "https://www.mozilla.org") + assertEquals(searchGroup.thumbnail, null) + assertEquals(searchGroup.count, 2) } @Test - fun `GIVEN a tab group with two tabs and a selected tab WHEN the feature starts THEN dispatch both the selected tab and the selected tab group as a recent tab list`() { + fun `GIVEN a tab group with one tab and a selected tab WHEN the feature starts THEN dispatch selected tab as a recent tab list`() { val tab1 = createTab( url = "https://www.mozilla.org", id = "1" @@ -483,17 +495,7 @@ class RecentTabsListFeatureTest { referrerUrl = "https://www.mozilla.org" ) ) - - val tab3 = createTab( - url = "https://www.mozilla.org", - id = "3", - historyMetadata = HistoryMetadataKey( - url = "https://www.mozilla.org", - searchTerm = "test search term", - referrerUrl = "https://www.mozilla.org" - ) - ) - val tabs = listOf(tab1, tab2, tab3) + val tabs = listOf(tab1, tab2) val browserStore = BrowserStore( BrowserState( tabs = tabs, @@ -509,42 +511,41 @@ class RecentTabsListFeatureTest { homeStore.waitUntilIdle() - assertEquals(2, homeStore.state.recentTabs.size) + assertEquals(1, homeStore.state.recentTabs.size) assertTrue(homeStore.state.recentTabs[0] is RecentTab.Tab) assertEquals(tab1, (homeStore.state.recentTabs[0] as RecentTab.Tab).state) - val searchGroup = (homeStore.state.recentTabs[1] as RecentTab.SearchGroup) - assertEquals(searchGroup.searchTerm, "Test search term") - assertEquals(searchGroup.tabId, "2") - assertEquals(searchGroup.url, "https://www.mozilla.org") - assertEquals(searchGroup.thumbnail, null) - assertEquals(searchGroup.count, 2) } @Test - fun `GIVEN a selected tab group with 2 tabs WHEN the feature starts THEN dispatch both tab in the selected tab group in the recent tab list`() { + fun `GIVEN a tab group with two tabs and a selected tab WHEN the feature starts THEN dispatch both the selected tab and the selected tab group as a recent tab list`() { val tab1 = createTab( url = "https://www.mozilla.org", - id = "1", + id = "1" + ) + val tab2 = createTab( + url = "https://www.mozilla.org", + id = "2", historyMetadata = HistoryMetadataKey( url = "https://www.mozilla.org", searchTerm = "test search term", referrerUrl = "https://www.mozilla.org" ) ) - val tab2 = createTab( - url = "https://www.getpocket.com", - id = "2", + + val tab3 = createTab( + url = "https://www.mozilla.org", + id = "3", historyMetadata = HistoryMetadataKey( - url = "https://www.getpocket.com", - searchTerm = "Test Search Term", - referrerUrl = "https://www.getpocket.com" + url = "https://www.mozilla.org", + searchTerm = "test search term", + referrerUrl = "https://www.mozilla.org" ) ) - val tabs = listOf(tab1, tab2) + val tabs = listOf(tab1, tab2, tab3) val browserStore = BrowserStore( BrowserState( tabs = tabs, - selectedTabId = "2" + selectedTabId = "1" ) ) val feature = RecentTabsListFeature( @@ -558,10 +559,10 @@ class RecentTabsListFeatureTest { assertEquals(2, homeStore.state.recentTabs.size) assertTrue(homeStore.state.recentTabs[0] is RecentTab.Tab) - assertEquals(tab2, (homeStore.state.recentTabs[0] as RecentTab.Tab).state) + assertEquals(tab1, (homeStore.state.recentTabs[0] as RecentTab.Tab).state) val searchGroup = (homeStore.state.recentTabs[1] as RecentTab.SearchGroup) assertEquals(searchGroup.searchTerm, "Test search term") - assertEquals(searchGroup.tabId, "1") + assertEquals(searchGroup.tabId, "2") assertEquals(searchGroup.url, "https://www.mozilla.org") assertEquals(searchGroup.thumbnail, null) assertEquals(searchGroup.count, 2)