From d1345e8f93608278af9946dfda1cdc9263075abd Mon Sep 17 00:00:00 2001 From: Christian Sadilek Date: Mon, 7 Dec 2020 17:35:21 -0500 Subject: [PATCH] Closes #11290: Refactor ToolbarMenu to use browser store --- .../components/toolbar/BrowserToolbarView.kt | 7 +- .../components/toolbar/DefaultToolbarMenu.kt | 89 ++++++-------- .../fenix/customtabs/CustomTabToolbarMenu.kt | 23 ++-- .../fenix/customtabs/CustomTabsIntegration.kt | 4 +- .../customtabs/ExternalAppBrowserFragment.kt | 1 + .../fenix/toolbar/DefaultToolbarMenuTest.kt | 115 ++++++++++++++++++ 6 files changed, 173 insertions(+), 66 deletions(-) create mode 100644 app/src/test/java/org/mozilla/fenix/toolbar/DefaultToolbarMenuTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt index 882dc3df59..aa240d1510 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt @@ -160,9 +160,9 @@ class BrowserToolbarView( val menuToolbar: ToolbarMenu if (isCustomTabSession) { menuToolbar = CustomTabToolbarMenu( - this, - sessionManager, - customTabSession?.id, + context = this, + store = components.core.store, + sessionId = customTabSession?.id, shouldReverseItems = toolbarPosition == ToolbarPosition.TOP, onItemTapped = { it.performHapticIfNeeded(view) @@ -179,7 +179,6 @@ class BrowserToolbarView( interactor.onBrowserToolbarMenuItemTapped(it) }, lifecycleOwner = lifecycleOwner, - sessionManager = sessionManager, store = components.core.store, bookmarksStorage = bookmarkStorage, isPinningSupported = isPinningSupported diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt index 93a4517d08..0fc1a35445 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt @@ -6,10 +6,14 @@ package org.mozilla.fenix.components.toolbar import android.content.Context import androidx.annotation.ColorRes +import androidx.annotation.VisibleForTesting import androidx.core.content.ContextCompat.getColor import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.lifecycleScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.launch import mozilla.components.browser.menu.BrowserMenuHighlight import mozilla.components.browser.menu.WebExtensionBrowserMenuBuilder @@ -19,13 +23,15 @@ import mozilla.components.browser.menu.item.BrowserMenuImageSwitch import mozilla.components.browser.menu.item.BrowserMenuImageText import mozilla.components.browser.menu.item.BrowserMenuItemToolbar import mozilla.components.browser.menu.item.WebExtensionPlaceholderMenuItem -import mozilla.components.browser.session.Session -import mozilla.components.browser.session.SessionManager import mozilla.components.browser.state.selector.findTab +import mozilla.components.browser.state.selector.selectedTab +import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.feature.webcompat.reporter.WebCompatReporterFeature +import mozilla.components.lib.state.ext.flowScoped import mozilla.components.support.ktx.android.content.getColorFromAttr +import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifAnyChanged import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode @@ -36,7 +42,7 @@ import org.mozilla.fenix.theme.ThemeManager /** * Builds the toolbar object used with the 3-dot menu in the browser fragment. - * @param sessionManager Reference to the session manager that contains all tabs. + * @param store reference to the application's [BrowserStore]. * @param hasAccountProblem If true, there was a problem signing into the Firefox account. * @param shouldReverseItems If true, reverse the menu items. * @param onItemTapped Called when a menu item is tapped. @@ -44,9 +50,9 @@ import org.mozilla.fenix.theme.ThemeManager * @param bookmarksStorage Used to check if a page is bookmarked. */ @Suppress("LargeClass", "LongParameterList") +@ExperimentalCoroutinesApi class DefaultToolbarMenu( private val context: Context, - private val sessionManager: SessionManager, private val store: BrowserStore, hasAccountProblem: Boolean = false, shouldReverseItems: Boolean, @@ -59,8 +65,7 @@ class DefaultToolbarMenu( private var currentUrlIsBookmarked = false private var isBookmarkedJob: Job? = null - /** Gets the current browser session */ - private val session: Session? get() = sessionManager.selectedSession + private val selectedSession: TabSessionState? get() = store.state.selectedTab override val menuBuilder by lazy { WebExtensionBrowserMenuBuilder( @@ -81,7 +86,7 @@ class DefaultToolbarMenu( primaryContentDescription = context.getString(R.string.browser_menu_back), primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - session?.canGoBack ?: true + selectedSession?.content?.canGoBack ?: true }, secondaryImageTintResource = ThemeManager.resolveAttribute(R.attr.disabled, context), disableInSecondaryState = true, @@ -95,7 +100,7 @@ class DefaultToolbarMenu( primaryContentDescription = context.getString(R.string.browser_menu_forward), primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - session?.canGoForward ?: true + selectedSession?.content?.canGoForward ?: true }, secondaryImageTintResource = ThemeManager.resolveAttribute(R.attr.disabled, context), disableInSecondaryState = true, @@ -109,7 +114,7 @@ class DefaultToolbarMenu( primaryContentDescription = context.getString(R.string.browser_menu_refresh), primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - session?.loading == false + selectedSession?.content?.loading == false }, secondaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_stop, secondaryContentDescription = context.getString(R.string.browser_menu_stop), @@ -117,7 +122,7 @@ class DefaultToolbarMenu( disableInSecondaryState = false, longClickListener = { onItemTapped.invoke(ToolbarMenu.Item.Reload(bypassCache = true)) } ) { - if (session?.loading == true) { + if (selectedSession?.content?.loading == true) { onItemTapped.invoke(ToolbarMenu.Item.Stop) } else { onItemTapped.invoke(ToolbarMenu.Item.Reload(bypassCache = false)) @@ -157,19 +162,19 @@ class DefaultToolbarMenu( // Predicates that need to be repeatedly called as the session changes private fun canAddToHomescreen(): Boolean = - session != null && isPinningSupported && + selectedSession != null && isPinningSupported && !context.components.useCases.webAppUseCases.isInstallable() private fun canInstall(): Boolean = - session != null && isPinningSupported && + selectedSession != null && isPinningSupported && context.components.useCases.webAppUseCases.isInstallable() - private fun shouldShowOpenInApp(): Boolean = session?.let { session -> + private fun shouldShowOpenInApp(): Boolean = selectedSession?.let { session -> val appLink = context.components.useCases.appLinksUseCases.appLinkRedirect - appLink(session.url).hasExternalApp() + appLink(session.content.url).hasExternalApp() } ?: false - private fun shouldShowReaderAppearance(): Boolean = session?.let { + private fun shouldShowReaderAppearance(): Boolean = selectedSession?.let { store.state.findTab(it.id)?.readerState?.active } ?: false // End of predicates // @@ -234,7 +239,7 @@ class DefaultToolbarMenu( imageResource = R.drawable.ic_desktop, label = context.getString(R.string.browser_menu_desktop_site), initialState = { - session?.desktopMode ?: false + selectedSession?.content?.desktopMode ?: false } ) { checked -> onItemTapped.invoke(ToolbarMenu.Item.RequestDesktop(checked)) @@ -353,44 +358,28 @@ class DefaultToolbarMenu( } @ColorRes - private fun primaryTextColor() = ThemeManager.resolveAttribute(R.attr.primaryText, context) - - private var currentSessionObserver: Pair? = null - - private fun registerForIsBookmarkedUpdates() { - session?.let { - registerForUrlChanges(it) - } - - val sessionManagerObserver = object : SessionManager.Observer { - override fun onSessionSelected(session: Session) { - // Unregister any old session observer before registering a new session observer - currentSessionObserver?.let { - it.first.unregister(it.second) + @VisibleForTesting + internal fun primaryTextColor() = ThemeManager.resolveAttribute(R.attr.primaryText, context) + + @VisibleForTesting + internal fun registerForIsBookmarkedUpdates() { + store.flowScoped(lifecycleOwner) { flow -> + flow.mapNotNull { state -> state.selectedTab } + .ifAnyChanged { tab -> + arrayOf( + tab.id, + tab.content.url + ) + } + .collect { + currentUrlIsBookmarked = false + updateCurrentUrlIsBookmarked(it.content.url) } - currentUrlIsBookmarked = false - updateCurrentUrlIsBookmarked(session.url) - registerForUrlChanges(session) - } - } - - sessionManager.register(sessionManagerObserver, lifecycleOwner) - } - - private fun registerForUrlChanges(session: Session) { - val sessionObserver = object : Session.Observer { - override fun onUrlChanged(session: Session, url: String) { - currentUrlIsBookmarked = false - updateCurrentUrlIsBookmarked(url) - } } - - currentSessionObserver = Pair(session, sessionObserver) - updateCurrentUrlIsBookmarked(session.url) - session.register(sessionObserver, lifecycleOwner) } - private fun updateCurrentUrlIsBookmarked(newUrl: String) { + @VisibleForTesting + internal fun updateCurrentUrlIsBookmarked(newUrl: String) { isBookmarkedJob?.cancel() isBookmarkedJob = lifecycleOwner.lifecycleScope.launch { currentUrlIsBookmarked = bookmarksStorage diff --git a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt index dd9a3baead..8fe71a71ef 100644 --- a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt @@ -17,8 +17,9 @@ import mozilla.components.browser.menu.item.BrowserMenuImageSwitch import mozilla.components.browser.menu.item.BrowserMenuImageText import mozilla.components.browser.menu.item.BrowserMenuItemToolbar import mozilla.components.browser.menu.item.SimpleBrowserMenuItem -import mozilla.components.browser.session.Session -import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.selector.findTab +import mozilla.components.browser.state.state.TabSessionState +import mozilla.components.browser.state.store.BrowserStore import org.mozilla.fenix.R import org.mozilla.fenix.components.toolbar.ToolbarMenu import org.mozilla.fenix.ext.components @@ -29,14 +30,14 @@ import java.util.Locale /** * Builds the toolbar object used with the 3-dot menu in the custom tab browser fragment. - * @param sessionManager Reference to the session manager that contains all tabs. + * @param store reference to the application's [BrowserStore]. * @param sessionId ID of the open custom tab session. * @param shouldReverseItems If true, reverse the menu items. * @param onItemTapped Called when a menu item is tapped. */ class CustomTabToolbarMenu( private val context: Context, - private val sessionManager: SessionManager, + private val store: BrowserStore, private val sessionId: String?, private val shouldReverseItems: Boolean, private val onItemTapped: (ToolbarMenu.Item) -> Unit = {} @@ -45,7 +46,7 @@ class CustomTabToolbarMenu( override val menuBuilder by lazy { BrowserMenuBuilder(menuItems) } /** Gets the current custom tab session */ - private val session: Session? get() = sessionId?.let { sessionManager.findSessionById(it) } + private val session: TabSessionState? get() = sessionId?.let { store.state.findTab(it) } private val appName = context.getString(R.string.app_name) override val menuToolbar by lazy { @@ -54,7 +55,7 @@ class CustomTabToolbarMenu( primaryContentDescription = context.getString(R.string.browser_menu_back), primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - session?.canGoBack ?: true + session?.content?.canGoBack ?: true }, secondaryImageTintResource = ThemeManager.resolveAttribute( R.attr.disabled, @@ -71,7 +72,7 @@ class CustomTabToolbarMenu( primaryContentDescription = context.getString(R.string.browser_menu_forward), primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - session?.canGoForward ?: true + session?.content?.canGoForward ?: true }, secondaryImageTintResource = ThemeManager.resolveAttribute( R.attr.disabled, @@ -88,7 +89,7 @@ class CustomTabToolbarMenu( primaryContentDescription = context.getString(R.string.browser_menu_refresh), primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - session?.loading == false + session?.content?.loading == false }, secondaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_stop, secondaryContentDescription = context.getString(R.string.browser_menu_stop), @@ -96,7 +97,7 @@ class CustomTabToolbarMenu( disableInSecondaryState = false, longClickListener = { onItemTapped.invoke(ToolbarMenu.Item.Reload(bypassCache = true)) } ) { - if (session?.loading == true) { + if (session?.content?.loading == true) { onItemTapped.invoke(ToolbarMenu.Item.Stop) } else { onItemTapped.invoke(ToolbarMenu.Item.Reload(bypassCache = false)) @@ -108,7 +109,7 @@ class CustomTabToolbarMenu( private fun shouldShowOpenInApp(): Boolean = session?.let { session -> val appLink = context.components.useCases.appLinksUseCases.appLinkRedirect - appLink(session.url).hasExternalApp() + appLink(session.content.url).hasExternalApp() } ?: false private val menuItems by lazy { @@ -132,7 +133,7 @@ class CustomTabToolbarMenu( private val desktopMode = BrowserMenuImageSwitch( imageResource = R.drawable.ic_desktop, label = context.getString(R.string.browser_menu_desktop_site), - initialState = { session?.desktopMode ?: false } + initialState = { session?.content?.desktopMode ?: false } ) { checked -> onItemTapped.invoke(ToolbarMenu.Item.RequestDesktop(checked)) } diff --git a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt index d34b038dda..4752744723 100644 --- a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt +++ b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt @@ -7,6 +7,7 @@ package org.mozilla.fenix.customtabs import android.app.Activity import androidx.appcompat.content.res.AppCompatResources.getDrawable import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.browser.toolbar.BrowserToolbar import mozilla.components.browser.toolbar.display.DisplayToolbar import mozilla.components.feature.customtabs.CustomTabsToolbarFeature @@ -18,6 +19,7 @@ import org.mozilla.fenix.ext.settings class CustomTabsIntegration( sessionManager: SessionManager, + store: BrowserStore, toolbar: BrowserToolbar, sessionId: String, activity: Activity, @@ -74,7 +76,7 @@ class CustomTabsIntegration( private val customTabToolbarMenu by lazy { CustomTabToolbarMenu( activity, - sessionManager, + store, sessionId, shouldReverseItems, onItemTapped = onItemTapped diff --git a/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserFragment.kt index b9844ff44d..dc983ff128 100644 --- a/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserFragment.kt @@ -65,6 +65,7 @@ class ExternalAppBrowserFragment : BaseBrowserFragment(), UserInteractionHandler customTabsIntegration.set( feature = CustomTabsIntegration( sessionManager = requireComponents.core.sessionManager, + store = requireComponents.core.store, toolbar = toolbar, sessionId = customTabSessionId, activity = activity, diff --git a/app/src/test/java/org/mozilla/fenix/toolbar/DefaultToolbarMenuTest.kt b/app/src/test/java/org/mozilla/fenix/toolbar/DefaultToolbarMenuTest.kt new file mode 100644 index 0000000000..54d4f06b14 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/toolbar/DefaultToolbarMenuTest.kt @@ -0,0 +1,115 @@ +/* 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.toolbar + +import android.content.Context +import android.net.Uri +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.spyk +import io.mockk.unmockkStatic +import io.mockk.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineDispatcher +import mozilla.components.browser.state.action.ContentAction +import mozilla.components.browser.state.action.TabListAction +import mozilla.components.browser.state.selector.findTab +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.createTab +import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.concept.storage.BookmarksStorage +import mozilla.components.support.test.ext.joinBlocking +import mozilla.components.support.test.rule.MainCoroutineRule +import org.junit.After +import org.junit.Assert.assertNotNull +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.mozilla.fenix.components.toolbar.DefaultToolbarMenu + +@ExperimentalCoroutinesApi +class DefaultToolbarMenuTest { + + private lateinit var store: BrowserStore + private lateinit var lifecycleOwner: MockedLifecycleOwner + private lateinit var toolbarMenu: DefaultToolbarMenu + private lateinit var context: Context + private lateinit var bookmarksStorage: BookmarksStorage + + private val testDispatcher = TestCoroutineDispatcher() + + @get:Rule + val coroutinesTestRule = MainCoroutineRule(testDispatcher) + + @Before + fun setUp() { + mockkStatic(Uri::class) + every { Uri.parse(any()) } returns mockk(relaxed = true) + + context = mockk(relaxed = true) + every { context.theme } returns mockk(relaxed = true) + bookmarksStorage = mockk(relaxed = true) + store = BrowserStore( + BrowserState( + tabs = listOf( + createTab(url = "https://firefox.com", id = "1"), + createTab(url = "https://getpocket.com", id = "2") + ), selectedTabId = "1" + ) + ) + lifecycleOwner = MockedLifecycleOwner(Lifecycle.State.STARTED) + + toolbarMenu = spyk(DefaultToolbarMenu( + context = context, + store = store, + hasAccountProblem = false, + shouldReverseItems = false, + onItemTapped = { }, + lifecycleOwner = lifecycleOwner, + bookmarksStorage = bookmarksStorage, + isPinningSupported = false + )) + + every { toolbarMenu.updateCurrentUrlIsBookmarked(any()) } returns Unit + } + + @After + fun tearDown() { + unmockkStatic(Uri::class) + } + + @Test + fun `WHEN url changes THEN bookmarked state is updated`() { + toolbarMenu.registerForIsBookmarkedUpdates() + + val newUrl = "https://mozilla.org" + + store.dispatch(ContentAction.UpdateUrlAction("1", newUrl)).joinBlocking() + verify(exactly = 1) { toolbarMenu.updateCurrentUrlIsBookmarked(newUrl) } + } + + @Test + fun `WHEN selected tab changes THEN bookmarked state is updated`() { + toolbarMenu.registerForIsBookmarkedUpdates() + + val newSelectedTab = store.state.findTab("2") + assertNotNull(newSelectedTab) + + store.dispatch(TabListAction.SelectTabAction(newSelectedTab!!.id)).joinBlocking() + verify(exactly = 1) { toolbarMenu.updateCurrentUrlIsBookmarked(newSelectedTab.content.url) } + } + + internal class MockedLifecycleOwner(initialState: Lifecycle.State) : LifecycleOwner { + val lifecycleRegistry = LifecycleRegistry(this).apply { + currentState = initialState + } + + override fun getLifecycle(): Lifecycle = lifecycleRegistry + } +}