From f40c19f4061dff2c78d3456422a9e32e6ac03caa Mon Sep 17 00:00:00 2001 From: Mugurell Date: Thu, 4 Nov 2021 16:15:23 +0200 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/22155 - New BrowserState.actualInactiveTabs public method This allows querying from all throughout the app which of the current tabs are inactive while taking into consideration whether the feature is enabled or not such that when the feature is disabled it will always return an empty result. --- .../org/mozilla/fenix/FenixApplication.kt | 4 +- .../org/mozilla/fenix/ext/BrowserState.kt | 34 ++++++- .../fenix/tabstray/TabsTrayController.kt | 6 +- .../TabsTrayInactiveTabsOnboardingBinding.kt | 4 +- .../tabstray/browser/NormalBrowserTrayList.kt | 12 +-- .../fenix/tabstray/browser/TabSorter.kt | 1 + .../fenix/tabstray/ext/TabSelectors.kt | 8 +- .../NormalBrowserPageViewHolder.kt | 6 +- .../org/mozilla/fenix/FenixApplicationTest.kt | 4 +- .../org/mozilla/fenix/ext/BrowserStateTest.kt | 95 +++++++++++++++++++ .../tabstray/DefaultTabsTrayControllerTest.kt | 8 +- .../tabstray/ext/TabSessionStateKtTest.kt | 2 +- 12 files changed, 148 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index fcd69da513..7cb154fa84 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -81,7 +81,7 @@ import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MozillaProductDetector import org.mozilla.fenix.components.toolbar.ToolbarPosition import org.mozilla.fenix.perf.MarkersActivityLifecycleCallbacks -import org.mozilla.fenix.tabstray.ext.inactiveTabs +import org.mozilla.fenix.ext.potentialInactiveTabs import org.mozilla.fenix.utils.Settings /** @@ -628,7 +628,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider { tabViewSetting.set(settings.getTabViewPingString()) closeTabSetting.set(settings.getTabTimeoutPingString()) - inactiveTabsCount.set(browserStore.state.inactiveTabs.size.toLong()) + inactiveTabsCount.set(browserStore.state.potentialInactiveTabs.size.toLong()) val installSourcePackage = if (SDK_INT >= Build.VERSION_CODES.R) { packageManager.getInstallSourceInfo(packageName).installingPackageName 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 69bacb9026..9843bdb49b 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt @@ -11,10 +11,22 @@ import mozilla.components.browser.state.state.TabSessionState import mozilla.components.feature.tabs.ext.hasMediaPlayed import org.mozilla.fenix.home.recenttabs.RecentTab import org.mozilla.fenix.tabstray.browser.TabGroup -import org.mozilla.fenix.tabstray.browser.maxActiveTime import org.mozilla.fenix.tabstray.ext.isNormalTabActiveWithSearchTerm +import org.mozilla.fenix.tabstray.ext.isNormalTabInactive +import org.mozilla.fenix.utils.Settings +import java.util.concurrent.TimeUnit import kotlin.math.max +/** + * The time until which a tab is considered in-active (in days). + */ +const val DEFAULT_ACTIVE_DAYS = 14L + +/** + * The maximum time from when a tab was created or accessed until it is considered "inactive". + */ +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. * @@ -109,3 +121,23 @@ fun List.toSearchGroup( return groups to remainderTabs } + +/** + * List of all inactive tabs based on [maxActiveTime]. + * The user may have disabled the feature so for user interactions consider using the [actualInactiveTabs] method + * or an in place check of the feature status. + */ +val BrowserState.potentialInactiveTabs: List + get() = normalTabs.filter { it.isNormalTabInactive(maxActiveTime) } + +/** + * List of all inactive tabs based on [maxActiveTime]. + * The result will be always be empty if the user disabled the feature. + */ +fun BrowserState.actualInactiveTabs(settings: Settings): List { + return if (settings.inactiveTabsAreEnabled) { + potentialInactiveTabs + } else { + emptyList() + } +} diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt index dc65ddeace..bce694ae82 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt @@ -21,8 +21,8 @@ import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.home.HomeFragment -import org.mozilla.fenix.tabstray.browser.DEFAULT_ACTIVE_DAYS -import org.mozilla.fenix.tabstray.ext.inactiveTabs +import org.mozilla.fenix.ext.DEFAULT_ACTIVE_DAYS +import org.mozilla.fenix.ext.potentialInactiveTabs import java.util.concurrent.TimeUnit interface TabsTrayController { @@ -218,7 +218,7 @@ class DefaultTabsTrayController( override fun handleDeleteAllInactiveTabs() { metrics.track(Event.TabsTrayCloseAllInactiveTabs) - browserStore.state.inactiveTabs.map { it.id }.let { + browserStore.state.potentialInactiveTabs.map { it.id }.let { tabsUseCases.removeTabs(it) } showUndoSnackbarForTab(false) diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInactiveTabsOnboardingBinding.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInactiveTabsOnboardingBinding.kt index a9414a7341..a4a382c27f 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInactiveTabsOnboardingBinding.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInactiveTabsOnboardingBinding.kt @@ -29,7 +29,7 @@ import org.mozilla.fenix.R import org.mozilla.fenix.browser.infobanner.InfoBanner import org.mozilla.fenix.databinding.ComponentTabstray2Binding import org.mozilla.fenix.databinding.OnboardingInactiveTabsCfrBinding -import org.mozilla.fenix.tabstray.ext.inactiveTabs +import org.mozilla.fenix.ext.potentialInactiveTabs import org.mozilla.fenix.utils.Settings @OptIn(ExperimentalCoroutinesApi::class) @@ -49,7 +49,7 @@ class TabsTrayInactiveTabsOnboardingBinding( .ifChanged() .collect { val inactiveTabsList = - if (settings.inactiveTabsAreEnabled) { store.state.inactiveTabs } else emptyList() + if (settings.inactiveTabsAreEnabled) { store.state.potentialInactiveTabs } else emptyList() if (inactiveTabsList.isNotEmpty() && shouldShowOnboardingForInactiveTabs()) { createInactiveCFR() } 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 79c9d94d62..18fb960848 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 @@ -11,21 +11,11 @@ import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.tabstray.TabViewHolder import mozilla.components.feature.tabs.tabstray.TabsFeature import org.mozilla.fenix.ext.components +import org.mozilla.fenix.ext.maxActiveTime 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.isNormalTabInactive -import java.util.concurrent.TimeUnit - -/** - * The time until which a tab is considered in-active (in days). - */ -const val DEFAULT_ACTIVE_DAYS = 14L - -/** - * The maximum time from when a tab was created or accessed until it is considered "inactive". - */ -val maxActiveTime = TimeUnit.DAYS.toMillis(DEFAULT_ACTIVE_DAYS) class NormalBrowserTrayList @JvmOverloads constructor( context: Context, 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 5ffc90e7b8..875a62e788 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 @@ -10,6 +10,7 @@ import mozilla.components.browser.tabstray.TabsTray import mozilla.components.feature.tabs.tabstray.TabsFeature import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController +import org.mozilla.fenix.ext.maxActiveTime import org.mozilla.fenix.ext.toSearchGroup import org.mozilla.fenix.tabstray.ext.browserAdapter import org.mozilla.fenix.tabstray.ext.hasSearchTerm diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/ext/TabSelectors.kt b/app/src/main/java/org/mozilla/fenix/tabstray/ext/TabSelectors.kt index 8a93a78004..86eba261ea 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/ext/TabSelectors.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/ext/TabSelectors.kt @@ -9,7 +9,7 @@ import mozilla.components.browser.state.selector.privateTabs import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.TabSessionState import org.mozilla.fenix.ext.toSearchGroup -import org.mozilla.fenix.tabstray.browser.maxActiveTime +import org.mozilla.fenix.ext.maxActiveTime /** * The currently selected tab if there's one that is private. @@ -32,12 +32,6 @@ fun BrowserState.findPrivateTab(tabId: String): TabSessionState? { return privateTabs.firstOrNull { it.id == tabId } } -/** - * The list of inactive tabs in the tabs tray filtered based on [maxActiveTime]. - */ -val BrowserState.inactiveTabs: List - get() = normalTabs.filter { it.isNormalTabInactive(maxActiveTime) } - /** * The list of normal tabs in the tabs tray filtered appropriately based on feature flags. */ diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt index dfe8318028..e5b0f3d340 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt @@ -21,11 +21,11 @@ import org.mozilla.fenix.tabstray.TabsTrayAction import org.mozilla.fenix.tabstray.TabsTrayInteractor import org.mozilla.fenix.tabstray.TabsTrayStore import org.mozilla.fenix.tabstray.browser.containsTabId -import org.mozilla.fenix.tabstray.browser.maxActiveTime +import org.mozilla.fenix.ext.maxActiveTime import org.mozilla.fenix.tabstray.ext.browserAdapter import org.mozilla.fenix.tabstray.ext.defaultBrowserLayoutColumns import org.mozilla.fenix.tabstray.ext.getNormalTrayTabs -import org.mozilla.fenix.tabstray.ext.inactiveTabs +import org.mozilla.fenix.ext.potentialInactiveTabs import org.mozilla.fenix.tabstray.ext.titleHeaderAdapter import org.mozilla.fenix.tabstray.ext.inactiveTabsAdapter import org.mozilla.fenix.tabstray.ext.isNormalTabActiveWithSearchTerm @@ -92,7 +92,7 @@ class NormalBrowserPageViewHolder( // Update tabs into the inactive adapter. if (inactiveTabsAreEnabled && selectedTab.isNormalTabInactive(maxActiveTime)) { - val inactiveTabsList = browserStore.state.inactiveTabs + val inactiveTabsList = browserStore.state.potentialInactiveTabs // We want to expand the inactive section first before we want to fire our scroll observer. appStore.dispatch(AppAction.UpdateInactiveExpanded(true)) diff --git a/app/src/test/java/org/mozilla/fenix/FenixApplicationTest.kt b/app/src/test/java/org/mozilla/fenix/FenixApplicationTest.kt index f666258589..cab85ee42c 100644 --- a/app/src/test/java/org/mozilla/fenix/FenixApplicationTest.kt +++ b/app/src/test/java/org/mozilla/fenix/FenixApplicationTest.kt @@ -33,7 +33,7 @@ import org.mozilla.fenix.GleanMetrics.SearchDefaultEngine import org.mozilla.fenix.components.metrics.MozillaProductDetector import org.mozilla.fenix.components.toolbar.ToolbarPosition import org.mozilla.fenix.helpers.FenixRobolectricTestRunner -import org.mozilla.fenix.tabstray.ext.inactiveTabs +import org.mozilla.fenix.ext.potentialInactiveTabs import org.mozilla.fenix.utils.BrowsersCache import org.mozilla.fenix.utils.Settings import org.robolectric.annotation.Config @@ -144,7 +144,7 @@ class FenixApplicationTest { every { application.reportHomeScreenMetrics(settings) } just Runs every { settings.inactiveTabsAreEnabled } returns true mockkStatic("org.mozilla.fenix.tabstray.ext.TabSelectorsKt") { - every { browserStore.state.inactiveTabs } returns listOf(mockk(), mockk()) + every { browserStore.state.potentialInactiveTabs } returns listOf(mockk(), mockk()) application.setStartupMetrics(browserStore, settings, browsersCache, mozillaProductDetector) 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 3b25265a0b..90dfad04a1 100644 --- a/app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt +++ b/app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.ext +import io.mockk.every import io.mockk.mockk import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.LastMediaAccessState @@ -11,8 +12,10 @@ import mozilla.components.browser.state.state.createTab import mozilla.components.concept.storage.HistoryMetadataKey import org.junit.Assert.assertEquals 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.utils.Settings class BrowserStateTest { @@ -318,4 +321,96 @@ class BrowserStateTest { ) assertEquals(normalTab3.id, browserState.secondToLastOpenedNormalTab!!.id) } + + @Test + fun `GIVEN a list of tabs WHEN potentialInactiveTabs is called THEN return the normal tabs which haven't been active lately`() { + // An inactive tab is one created or accessed more than [maxActiveTime] ago + // checked against [System.currentTimeMillis] + // + // createTab() sets the [createdTime] property to [System.currentTimeMillis] this meaning + // that the default tab is considered active. + + val normalTab1 = createTab(url = "url1", id = "1", createdAt = 1) + val normalTab2 = createTab(url = "url2", id = "2") + val normalTab3 = createTab(url = "url3", id = "3", createdAt = 1) + val normalTab4 = createTab(url = "url4", id = "4") + val privateTab1 = createTab(url = "url1", id = "6", private = true) + val privateTab2 = createTab(url = "url2", id = "7", private = true, createdAt = 1) + val privateTab3 = createTab(url = "url3", id = "8", private = true) + val privateTab4 = createTab(url = "url4", id = "9", private = true, createdAt = 1) + val browserState = BrowserState( + tabs = listOf( + normalTab1, normalTab2, normalTab3, normalTab4, + privateTab1, privateTab2, privateTab3, privateTab4 + ) + ) + + val result = browserState.potentialInactiveTabs + + assertEquals(2, result.size) + assertTrue(result.containsAll(listOf(normalTab1, normalTab3))) + } + + @Test + fun `GIVEN inactiveTabs feature is disabled WHEN actualInactiveTabs is called THEN return an empty result`() { + // An inactive tab is one created or accessed more than [maxActiveTime] ago + // checked against [System.currentTimeMillis] + // + // createTab() sets the [createdTime] property to [System.currentTimeMillis] this meaning + // that the default tab is considered active. + + val normalTab1 = createTab(url = "url1", id = "1", createdAt = 1) + val normalTab2 = createTab(url = "url2", id = "2") + val normalTab3 = createTab(url = "url3", id = "3", createdAt = 1) + val normalTab4 = createTab(url = "url4", id = "4") + val privateTab1 = createTab(url = "url1", id = "6", private = true) + val privateTab2 = createTab(url = "url2", id = "7", private = true, createdAt = 1) + val privateTab3 = createTab(url = "url3", id = "8", private = true) + val privateTab4 = createTab(url = "url4", id = "9", private = true, createdAt = 1) + val browserState = BrowserState( + tabs = listOf( + normalTab1, normalTab2, normalTab3, normalTab4, + privateTab1, privateTab2, privateTab3, privateTab4 + ) + ) + val settings: Settings = mockk() { + every { inactiveTabsAreEnabled } returns false + } + + val result = browserState.actualInactiveTabs(settings) + + assertTrue(result.isEmpty()) + } + + @Test + fun `GIVEN inactiveTabs feature is enabled WHEN actualInactiveTabs is called THEN return the normal tabs which haven't been active lately`() { + // An inactive tab is one created or accessed more than [maxActiveTime] ago + // checked against [System.currentTimeMillis] + // + // createTab() sets the [createdTime] property to [System.currentTimeMillis] this meaning + // that the default tab is considered active. + + val normalTab1 = createTab(url = "url1", id = "1", createdAt = 1) + val normalTab2 = createTab(url = "url2", id = "2") + val normalTab3 = createTab(url = "url3", id = "3", createdAt = 1) + val normalTab4 = createTab(url = "url4", id = "4") + val privateTab1 = createTab(url = "url1", id = "6", private = true) + val privateTab2 = createTab(url = "url2", id = "7", private = true, createdAt = 1) + val privateTab3 = createTab(url = "url3", id = "8", private = true) + val privateTab4 = createTab(url = "url4", id = "9", private = true, createdAt = 1) + val browserState = BrowserState( + tabs = listOf( + normalTab1, normalTab2, normalTab3, normalTab4, + privateTab1, privateTab2, privateTab3, privateTab4 + ) + ) + val settings: Settings = mockk() { + every { inactiveTabsAreEnabled } returns true + } + + val result = browserState.actualInactiveTabs(settings) + + assertEquals(2, result.size) + assertTrue(result.containsAll(listOf(normalTab1, normalTab3))) + } } diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt index 40b46dffc6..734b9a6252 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt @@ -33,8 +33,8 @@ import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.home.HomeFragment -import org.mozilla.fenix.tabstray.browser.maxActiveTime -import org.mozilla.fenix.tabstray.ext.inactiveTabs +import org.mozilla.fenix.ext.maxActiveTime +import org.mozilla.fenix.ext.potentialInactiveTabs class DefaultTabsTrayControllerTest { @MockK(relaxed = true) @@ -409,7 +409,7 @@ class DefaultTabsTrayControllerTest { every { browserStore.state } returns mockk() try { mockkStatic("mozilla.components.browser.state.selector.SelectorsKt") - every { browserStore.state.inactiveTabs } returns listOf(inactiveTab) + every { browserStore.state.potentialInactiveTabs } returns listOf(inactiveTab) controller.handleDeleteAllInactiveTabs() @@ -433,7 +433,7 @@ class DefaultTabsTrayControllerTest { every { browserStore.state } returns mockk() try { mockkStatic("mozilla.components.browser.state.selector.SelectorsKt") - every { browserStore.state.inactiveTabs } returns listOf(inactiveTab) + every { browserStore.state.potentialInactiveTabs } returns listOf(inactiveTab) createController().handleDeleteAllInactiveTabs() diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/ext/TabSessionStateKtTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/ext/TabSessionStateKtTest.kt index 32590fa74f..7959b0071b 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/ext/TabSessionStateKtTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/ext/TabSessionStateKtTest.kt @@ -10,7 +10,7 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test -import org.mozilla.fenix.tabstray.browser.DEFAULT_ACTIVE_DAYS +import org.mozilla.fenix.ext.DEFAULT_ACTIVE_DAYS import java.util.concurrent.TimeUnit class TabSessionStateKtTest {