From 721316f905425a0ec3a5c3ab23b46795e5e9010f Mon Sep 17 00:00:00 2001 From: Noah Bond Date: Fri, 6 Jan 2023 13:22:21 -0800 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/28264 - Refactor synced tabs handling out of `NavigationInteractor` --- .../fenix/tabstray/NavigationInteractor.kt | 20 --------- .../fenix/tabstray/SyncedTabsController.kt | 19 ++++++++ .../fenix/tabstray/SyncedTabsInteractor.kt | 19 ++++++++ .../fenix/tabstray/TabsTrayController.kt | 45 ++++++++++++++++--- .../fenix/tabstray/TabsTrayFragment.kt | 2 +- .../fenix/tabstray/TabsTrayInteractor.kt | 12 ++++- .../fenix/tabstray/TrayPagerAdapter.kt | 2 +- .../viewholders/SyncedTabsPageViewHolder.kt | 8 ++-- .../tabstray/DefaultTabsTrayControllerTest.kt | 37 +++++++++++++++ .../tabstray/NavigationInteractorTest.kt | 34 -------------- 10 files changed, 131 insertions(+), 67 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsController.kt create mode 100644 app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsInteractor.kt diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt b/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt index 51538d9c03..79f6b27a73 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt @@ -15,11 +15,9 @@ import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.engine.prompt.ShareData import mozilla.components.service.fxa.manager.FxaAccountManager import mozilla.components.service.glean.private.NoExtras -import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.GleanMetrics.Collections import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.TabsTray -import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.collections.CollectionsDialog import org.mozilla.fenix.collections.show import org.mozilla.fenix.components.TabCollectionStorage @@ -28,7 +26,6 @@ import org.mozilla.fenix.home.HomeFragment import org.mozilla.fenix.tabstray.ext.getTabSessionState import org.mozilla.fenix.tabstray.ext.isActiveDownload import kotlin.coroutines.CoroutineContext -import mozilla.components.browser.storage.sync.Tab as SyncTab /** * An interactor that helps with navigating to different parts of the app from the tabs tray. @@ -85,11 +82,6 @@ interface NavigationInteractor { * Used when adding [TabSessionState]s as bookmarks. */ fun onSaveToBookmarks(tabs: Collection) - - /** - * Called when clicking on a SyncedTab item. - */ - fun onSyncedTabClicked(tab: SyncTab) } /** @@ -98,7 +90,6 @@ interface NavigationInteractor { @Suppress("LongParameterList", "TooManyFunctions") class DefaultNavigationInteractor( private val context: Context, - private val activity: HomeActivity, private val browserStore: BrowserStore, private val navController: NavController, private val dismissTabTray: () -> Unit, @@ -244,15 +235,4 @@ class DefaultNavigationInteractor( showBookmarkSnackbar(tabs.size) } - - override fun onSyncedTabClicked(tab: SyncTab) { - Events.syncedTabOpened.record(NoExtras()) - - dismissTabTray() - activity.openToBrowserAndLoad( - searchTermOrURL = tab.active().url, - newTab = true, - from = BrowserDirection.FromTabsTray, - ) - } } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsController.kt b/app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsController.kt new file mode 100644 index 0000000000..27336f6343 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsController.kt @@ -0,0 +1,19 @@ +/* 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 + +import mozilla.components.browser.storage.sync.Tab + +/** + * Controller for handling any actions on synced tabs in the tabs tray. + */ +interface SyncedTabsController { + /** + * Handles a synced tab item click. + * + * @param tab The synced [Tab] that was clicked. + */ + fun handleSyncedTabClicked(tab: Tab) +} diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsInteractor.kt b/app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsInteractor.kt new file mode 100644 index 0000000000..153a0f48d7 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/tabstray/SyncedTabsInteractor.kt @@ -0,0 +1,19 @@ +/* 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 + +import mozilla.components.browser.storage.sync.Tab + +/** + * Interactor for responding to any actions on synced tabs in the tabs tray. + */ +interface SyncedTabsInteractor { + /** + * Invoked when the user clicks on a synced [Tab]. + * + * @param tab The synced [Tab] that was clicked. + */ + fun onSyncedTabClicked(tab: Tab) +} 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 56a61f0959..31cefa7e68 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt @@ -10,16 +10,20 @@ import mozilla.components.browser.state.action.DebugAction import mozilla.components.browser.state.action.LastAccessAction import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.selector.getNormalOrPrivateTabs +import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.SessionState import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.browser.storage.sync.Tab import mozilla.components.concept.base.profiler.Profiler import mozilla.components.concept.engine.mediasession.MediaSession.PlaybackState import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.lib.state.DelicateAction import mozilla.telemetry.glean.private.NoExtras -import org.mozilla.fenix.GleanMetrics.Tab +import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.TabsTray +import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager @@ -28,8 +32,12 @@ import org.mozilla.fenix.ext.DEFAULT_ACTIVE_DAYS import org.mozilla.fenix.home.HomeFragment import org.mozilla.fenix.tabstray.ext.isActiveDownload import java.util.concurrent.TimeUnit +import org.mozilla.fenix.GleanMetrics.Tab as GleanTab -interface TabsTrayController { +/** + * Controller for handling any actions in the tabs tray. + */ +interface TabsTrayController : SyncedTabsController { /** * Called to open a new tab. @@ -107,8 +115,25 @@ interface TabsTrayController { fun handleMediaClicked(tab: SessionState) } +/** + * Default implementation of [TabsTrayController]. + * + * @property activity [HomeActivity] used to perform top-level app actions. + * @property trayStore [TabsTrayStore] used to read/update the [TabsTrayState]. + * @property browserStore [BrowserStore] used to read/update the current [BrowserState]. + * @property browsingModeManager [BrowsingModeManager] used to read/update the current [BrowsingMode]. + * @property navController [NavController] used to navigate away from the tabs tray. + * @property navigateToHomeAndDeleteSession Lambda used to return to the Homescreen and delete the current session. + * @property navigationInteractor [NavigationInteractor] used to perform navigation actions with side effects. + * @property tabsUseCases Use case wrapper for interacting with tabs. + * @property selectTabPosition Lambda used to scroll the tabs tray to the desired position. + * @property dismissTray Lambda used to dismiss/minimize the tabs tray. + * @property showUndoSnackbarForTab Lambda used to display an UNDO Snackbar. + * @property showCancelledDownloadWarning Lambda used to display a cancelled download warning. + */ @Suppress("TooManyFunctions", "LongParameterList") class DefaultTabsTrayController( + private val activity: HomeActivity, private val trayStore: TabsTrayStore, private val browserStore: BrowserStore, private val browsingModeManager: BrowsingModeManager, @@ -121,7 +146,6 @@ class DefaultTabsTrayController( private val dismissTray: () -> Unit, private val showUndoSnackbarForTab: (Boolean) -> Unit, internal val showCancelledDownloadWarning: (downloadCount: Int, tabId: String?, source: String?) -> Unit, - ) : TabsTrayController { override fun handleOpeningNewTab(isPrivate: Boolean) { @@ -279,12 +303,12 @@ class DefaultTabsTrayController( override fun handleMediaClicked(tab: SessionState) { when (tab.mediaSessionState?.playbackState) { PlaybackState.PLAYING -> { - Tab.mediaPause.record(NoExtras()) + GleanTab.mediaPause.record(NoExtras()) tab.mediaSessionState?.controller?.pause() } PlaybackState.PAUSED -> { - Tab.mediaPlay.record(NoExtras()) + GleanTab.mediaPlay.record(NoExtras()) tab.mediaSessionState?.controller?.play() } else -> throw AssertionError( @@ -292,4 +316,15 @@ class DefaultTabsTrayController( ) } } + + override fun handleSyncedTabClicked(tab: Tab) { + Events.syncedTabOpened.record(NoExtras()) + + dismissTray() + activity.openToBrowserAndLoad( + searchTermOrURL = tab.active().url, + newTab = true, + from = BrowserDirection.FromTabsTray, + ) + } } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt index 045957a4e4..262f51a8da 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt @@ -197,7 +197,6 @@ class TabsTrayFragment : AppCompatDialogFragment() { navigationInteractor = DefaultNavigationInteractor( context = requireContext(), - activity = activity, tabsTrayStore = tabsTrayStore, browserStore = requireComponents.core.store, navController = findNavController(), @@ -213,6 +212,7 @@ class TabsTrayFragment : AppCompatDialogFragment() { ) tabsTrayController = DefaultTabsTrayController( + activity = activity, trayStore = tabsTrayStore, browserStore = requireComponents.core.store, browsingModeManager = activity.browsingModeManager, diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt index a5721ecce4..a51aaa439f 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt @@ -5,8 +5,12 @@ package org.mozilla.fenix.tabstray import mozilla.components.browser.state.state.TabSessionState +import mozilla.components.browser.storage.sync.Tab -interface TabsTrayInteractor { +/** + * Interactor for responding to all user actions in the tabs tray. + */ +interface TabsTrayInteractor : SyncedTabsInteractor { /** * Set the current tray item to the clamped [position]. * @@ -53,7 +57,7 @@ interface TabsTrayInteractor { } /** - * Interactor to be called for any tabs tray user actions. + * Default implementation of [TabsTrayInteractor]. * * @property controller [TabsTrayController] to which user actions can be delegated for actual app update. */ @@ -91,4 +95,8 @@ class DefaultTabsTrayInteractor( override fun onInactiveDebugClicked(tabs: Collection) { controller.forceTabsAsInactive(tabs) } + + override fun onSyncedTabClicked(tab: Tab) { + controller.handleSyncedTabClicked(tab) + } } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt index f61f05d6a6..5d28ff0749 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt @@ -91,7 +91,7 @@ class TrayPagerAdapter( ) }, tabsTrayStore = tabsTrayStore, - navigationInteractor = navInteractor, + interactor = tabsTrayInteractor, ) } else -> throw IllegalStateException("Unknown viewType.") diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt index c0e7bf5cf2..27fdcfcf19 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt @@ -9,7 +9,7 @@ import androidx.compose.ui.platform.ComposeView import androidx.recyclerview.widget.RecyclerView import mozilla.components.lib.state.ext.observeAsComposableState import org.mozilla.fenix.ext.settings -import org.mozilla.fenix.tabstray.NavigationInteractor +import org.mozilla.fenix.tabstray.SyncedTabsInteractor import org.mozilla.fenix.tabstray.TabsTrayState import org.mozilla.fenix.tabstray.TabsTrayStore import org.mozilla.fenix.tabstray.syncedtabs.SyncedTabsList @@ -21,12 +21,12 @@ import org.mozilla.fenix.theme.Theme * * @param composeView Root ComposeView passed-in from TrayPagerAdapter. * @param tabsTrayStore Store used as a Composable State to listen for changes to [TabsTrayState.syncedTabs]. - * @param navigationInteractor The lambda for handling clicks on synced tabs. + * @param interactor [SyncedTabsInteractor] used to respond to interactions with synced tabs. */ class SyncedTabsPageViewHolder( private val composeView: ComposeView, private val tabsTrayStore: TabsTrayStore, - private val navigationInteractor: NavigationInteractor, + private val interactor: SyncedTabsInteractor, ) : AbstractPageViewHolder(composeView) { fun bind() { @@ -36,7 +36,7 @@ class SyncedTabsPageViewHolder( SyncedTabsList( syncedTabs = tabs ?: emptyList(), taskContinuityEnabled = composeView.context.settings().enableTaskContinuityEnhancements, - onTabClick = navigationInteractor::onSyncedTabClicked, + onTabClick = interactor::onSyncedTabClicked, ) } } 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 6b6fcea9cd..e786bad45b 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt @@ -22,6 +22,8 @@ import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.browser.state.state.createTab import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.browser.storage.sync.Tab +import mozilla.components.browser.storage.sync.TabEntry import mozilla.components.concept.base.profiler.Profiler import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.service.glean.testing.GleanTestRule @@ -35,7 +37,10 @@ import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.TabsTray +import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -64,6 +69,9 @@ class DefaultTabsTrayControllerTest { @MockK(relaxed = true) private lateinit var tabsUseCases: TabsUseCases + @MockK(relaxed = true) + private lateinit var activity: HomeActivity + @get:Rule val gleanTestRule = GleanTestRule(testContext) @@ -475,6 +483,34 @@ class DefaultTabsTrayControllerTest { assertTrue(navigateToHomeAndDeleteSessionInvoked) } + @Test + fun `WHEN a synced tab is clicked THEN the metrics are reported and the tab is opened`() { + val tab = mockk() + val entry = mockk() + assertNull(Events.syncedTabOpened.testGetValue()) + + every { tab.active() }.answers { entry } + every { entry.url }.answers { "https://mozilla.org" } + + var dismissTabTrayInvoked = false + createController( + dismissTray = { + dismissTabTrayInvoked = true + }, + ).handleSyncedTabClicked(tab) + + assertTrue(dismissTabTrayInvoked) + assertNotNull(Events.syncedTabOpened.testGetValue()) + + verify { + activity.openToBrowserAndLoad( + searchTermOrURL = "https://mozilla.org", + newTab = true, + from = BrowserDirection.FromTabsTray, + ) + } + } + private fun createController( navigateToHomeAndDeleteSession: (String) -> Unit = { }, selectTabPosition: (Int, Boolean) -> Unit = { _, _ -> }, @@ -483,6 +519,7 @@ class DefaultTabsTrayControllerTest { showCancelledDownloadWarning: (Int, String?, String?) -> Unit = { _, _, _ -> }, ): DefaultTabsTrayController { return DefaultTabsTrayController( + activity, trayStore, browserStore, browsingModeManager, diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt index f47127d328..100ed970eb 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt @@ -20,7 +20,6 @@ import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.browser.state.store.BrowserStore -import mozilla.components.browser.storage.sync.TabEntry import mozilla.components.service.fxa.manager.FxaAccountManager import mozilla.components.service.glean.testing.GleanTestRule import mozilla.components.support.test.robolectric.testContext @@ -35,17 +34,14 @@ import org.junit.Rule import org.junit.Test import org.junit.rules.RuleChain import org.junit.runner.RunWith -import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.TabsTray -import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.collections.CollectionsDialog import org.mozilla.fenix.collections.show import org.mozilla.fenix.components.TabCollectionStorage import org.mozilla.fenix.components.bookmarks.BookmarksUseCase import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import mozilla.components.browser.state.state.createTab as createStateTab -import mozilla.components.browser.storage.sync.Tab as SyncTab @RunWith(FenixRobolectricTestRunner::class) // for gleanTestRule class NavigationInteractorTest { @@ -57,7 +53,6 @@ class NavigationInteractorTest { private val context: Context = mockk(relaxed = true) private val collectionStorage: TabCollectionStorage = mockk(relaxed = true) private val accountManager: FxaAccountManager = mockk(relaxed = true) - private val activity: HomeActivity = mockk(relaxed = true) val coroutinesTestRule: MainCoroutineRule = MainCoroutineRule() val gleanTestRule = GleanTestRule(testContext) @@ -220,34 +215,6 @@ class NavigationInteractorTest { Assert.assertEquals("1", snapshot.single().extra?.getValue("tab_count")) } - @Test - fun `onSyncedTabsClicked sets metrics and opens browser`() { - val tab = mockk() - val entry = mockk() - assertNull(Events.syncedTabOpened.testGetValue()) - - every { tab.active() }.answers { entry } - every { entry.url }.answers { "https://mozilla.org" } - - var dismissTabTrayInvoked = false - createInteractor( - dismissTabTray = { - dismissTabTrayInvoked = true - }, - ).onSyncedTabClicked(tab) - - assertTrue(dismissTabTrayInvoked) - assertNotNull(Events.syncedTabOpened.testGetValue()) - - verify { - activity.openToBrowserAndLoad( - searchTermOrURL = "https://mozilla.org", - newTab = true, - from = BrowserDirection.FromTabsTray, - ) - } - } - @Suppress("LongParameterList") private fun createInteractor( browserStore: BrowserStore = store, @@ -259,7 +226,6 @@ class NavigationInteractorTest { ): NavigationInteractor { return DefaultNavigationInteractor( context, - activity, browserStore, navController, dismissTabTray,