From 0f1bff74026bb85c9b7bb1c0078ba4ecd1465788 Mon Sep 17 00:00:00 2001 From: Christian Sadilek Date: Mon, 6 Apr 2020 17:18:41 -0400 Subject: [PATCH] No issue: Refactor readerview to use browser-state --- .../mozilla/fenix/browser/BrowserFragment.kt | 9 ++++-- .../java/org/mozilla/fenix/components/Core.kt | 4 ++- .../toolbar/BrowserToolbarController.kt | 7 +++-- .../components/toolbar/BrowserToolbarView.kt | 1 + .../components/toolbar/DefaultToolbarMenu.kt | 17 ++++++++--- .../fenix/components/toolbar/MenuPresenter.kt | 8 +++--- .../components/toolbar/ToolbarIntegration.kt | 6 +++- .../DefaultBrowserToolbarControllerTest.kt | 28 ++++++++++++++++++- .../toolbar/DefaultToolbarMenuTest.kt | 17 +++++++++-- 9 files changed, 77 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt index f933c8802..6493c4d1b 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -29,6 +29,7 @@ import mozilla.components.support.base.feature.ViewBoundFeatureWrapper import org.mozilla.fenix.FeatureFlags import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R +import org.mozilla.fenix.addons.runIfFragmentIsAttached import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.components.TabCollectionStorage import org.mozilla.fenix.components.metrics.Event @@ -62,7 +63,6 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { override fun initializeUI(view: View): Session? { val context = requireContext() - val sessionManager = context.components.core.sessionManager val components = context.components return super.initializeUI(view)?.also { @@ -70,12 +70,15 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { feature = ReaderViewFeature( context, components.core.engine, - sessionManager, + components.core.store, view.readerViewControlsBar - ) { available -> + ) { available, _ -> if (available) { components.analytics.metrics.track(Event.ReaderModeAvailable) } + runIfFragmentIsAttached { + browserToolbarView.toolbarIntegration.invalidateMenu() + } }, owner = this, view = view diff --git a/app/src/main/java/org/mozilla/fenix/components/Core.kt b/app/src/main/java/org/mozilla/fenix/components/Core.kt index df8580180..9b1b23037 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Core.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Core.kt @@ -31,6 +31,7 @@ import mozilla.components.feature.media.RecordingDevicesNotificationFeature import mozilla.components.feature.media.middleware.MediaMiddleware import mozilla.components.feature.pwa.ManifestStorage import mozilla.components.feature.pwa.WebAppShortcutManager +import mozilla.components.feature.readerview.ReaderViewMiddleware import mozilla.components.feature.session.HistoryDelegate import mozilla.components.feature.webcompat.WebCompatFeature import mozilla.components.feature.webnotifications.WebNotificationFeature @@ -99,7 +100,8 @@ class Core(private val context: Context) { val store by lazy { BrowserStore( middleware = listOf( - MediaMiddleware(context, MediaService::class.java) + MediaMiddleware(context, MediaService::class.java), + ReaderViewMiddleware() ) ) } diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt index 098efa550..b9e34cf4d 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.selector.findTab import mozilla.components.concept.engine.EngineView import mozilla.components.concept.engine.prompt.ShareData import mozilla.components.support.ktx.kotlin.isUrl @@ -275,9 +276,9 @@ class DefaultBrowserToolbarController( is ToolbarMenu.Item.ReaderMode -> { activity.settings().readerModeOpened = true - val enabled = currentSession?.readerMode - ?: activity.components.core.sessionManager.selectedSession?.readerMode - ?: false + val enabled = currentSession?.let { + activity.components.core.store.state.findTab(it.id)?.readerState?.active + } ?: false if (enabled) { readerModeController.hideReaderView() 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 6fa6e5bfb..577639cdf 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 @@ -207,6 +207,7 @@ class BrowserToolbarView( onItemTapped = { interactor.onBrowserToolbarMenuItemTapped(it) }, lifecycleOwner = lifecycleOwner, sessionManager = sessionManager, + store = components.core.store, bookmarksStorage = bookmarkStorage ) view.display.setMenuDismissAction { 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 1e2959fa2..155c86e17 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 @@ -21,6 +21,8 @@ import mozilla.components.browser.menu.item.BrowserMenuImageText import mozilla.components.browser.menu.item.BrowserMenuItemToolbar import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.selector.findTab +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.support.ktx.android.content.getColorFromAttr import org.mozilla.fenix.Config @@ -47,6 +49,7 @@ import org.mozilla.fenix.utils.Settings class DefaultToolbarMenu( private val context: Context, private val sessionManager: SessionManager, + private val store: BrowserStore, hasAccountProblem: Boolean = false, shouldReverseItems: Boolean, private val onItemTapped: (ToolbarMenu.Item) -> Unit = {}, @@ -64,7 +67,7 @@ class DefaultToolbarMenu( WebExtensionBrowserMenuBuilder( menuItems, endOfMenuAlwaysVisible = !shouldReverseItems, - store = context.components.core.store, + store = store, appendExtensionActionAtStart = !shouldReverseItems ) } @@ -150,14 +153,18 @@ class DefaultToolbarMenu( private fun shouldShowAddToHomescreen(): Boolean = session != null && context.components.useCases.webAppUseCases.isPinningSupported() - private fun shouldShowReaderMode(): Boolean = session?.readerable ?: false + private fun shouldShowReaderMode(): Boolean = session?.let { + store.state.findTab(it.id)?.readerState?.readerable + } ?: false private fun shouldShowOpenInApp(): Boolean = session?.let { session -> val appLink = context.components.useCases.appLinksUseCases.appLinkRedirect appLink(session.url).hasExternalApp() } ?: false - private fun shouldShowReaderAppearance(): Boolean = session?.readerMode ?: false + private fun shouldShowReaderAppearance(): Boolean = session?.let { + store.state.findTab(it.id)?.readerState?.active + } ?: false // End of predicates // private val menuItems by lazy { @@ -300,7 +307,9 @@ class DefaultToolbarMenu( label = context.getString(R.string.browser_menu_read), startImageResource = R.drawable.ic_readermode, initialState = { - session?.readerMode ?: false + session?.let { + store.state.findTab(it.id)?.readerState?.active + } ?: false }, highlight = BrowserMenuHighlight.LowPriority( label = context.getString(R.string.browser_menu_read), diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/MenuPresenter.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/MenuPresenter.kt index e87fa7fb8..de6d1105e 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/MenuPresenter.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/MenuPresenter.kt @@ -22,20 +22,20 @@ class MenuPresenter( /** Redraw the refresh/stop button */ override fun onLoadingStateChanged(session: Session, loading: Boolean) { - menuToolbar.invalidateActions() + invalidateActions() } /** Redraw the back and forward buttons */ override fun onNavigationStateChanged(session: Session, canGoBack: Boolean, canGoForward: Boolean) { - menuToolbar.invalidateActions() + invalidateActions() } /** Redraw the install web app button */ override fun onWebAppManifestChanged(session: Session, manifest: WebAppManifest?) { - menuToolbar.invalidateActions() + invalidateActions() } - override fun onReaderableStateUpdated(session: Session, readerable: Boolean) { + fun invalidateActions() { menuToolbar.invalidateActions() } } diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarIntegration.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarIntegration.kt index 5f90f09f5..c8ac9d62a 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarIntegration.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarIntegration.kt @@ -45,7 +45,7 @@ abstract class ToolbarIntegration( ) ) - private var menuPresenter = + private val menuPresenter = MenuPresenter(toolbar, context.components.core.sessionManager, sessionId) init { @@ -62,6 +62,10 @@ abstract class ToolbarIntegration( menuPresenter.stop() toolbarPresenter.stop() } + + fun invalidateMenu() { + menuPresenter.invalidateActions() + } } class DefaultToolbarIntegration( diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt index 1a4245bdc..4bd2936d7 100644 --- a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt @@ -27,6 +27,10 @@ import kotlinx.coroutines.test.runBlockingTest import kotlinx.coroutines.test.setMain import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.ReaderState +import mozilla.components.browser.state.state.createTab +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.engine.EngineView import mozilla.components.feature.search.SearchUseCases import mozilla.components.feature.session.SessionUseCases @@ -43,6 +47,7 @@ import org.mozilla.fenix.browser.BrowserAnimator import org.mozilla.fenix.browser.BrowserFragment import org.mozilla.fenix.browser.BrowserFragmentDirections import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager +import org.mozilla.fenix.browser.readermode.ReaderModeController import org.mozilla.fenix.collections.SaveCollectionStep import org.mozilla.fenix.components.Analytics import org.mozilla.fenix.components.FenixSnackbar @@ -82,6 +87,13 @@ class DefaultBrowserToolbarControllerTest { private val snackbar = mockk(relaxed = true) private val tabCollectionStorage = mockk(relaxed = true) private val topSiteStorage = mockk(relaxed = true) + private val readerModeController = mockk(relaxed = true) + private val store: BrowserStore = BrowserStore(initialState = BrowserState( + listOf( + createTab("https://www.mozilla.org", id = "reader-inactive-tab"), + createTab("https://www.mozilla.org", id = "reader-active-tab", readerState = ReaderState(active = true)) + )) + ) private lateinit var controller: DefaultBrowserToolbarController @@ -104,7 +116,7 @@ class DefaultBrowserToolbarControllerTest { tabCollectionStorage = tabCollectionStorage, topSiteStorage = topSiteStorage, bookmarkTapped = mockk(), - readerModeController = mockk(), + readerModeController = readerModeController, sessionManager = mockk(), store = mockk(), sharedViewModel = mockk() @@ -125,6 +137,7 @@ class DefaultBrowserToolbarControllerTest { every { activity.components.useCases.sessionUseCases } returns sessionUseCases every { activity.components.useCases.searchUseCases } returns searchUseCases every { activity.components.core.sessionManager.selectedSession } returns currentSession + every { activity.components.core.store } returns store val onComplete = slot<() -> Unit>() every { browserAnimator.captureEngineViewAndDrawStatically(capture(onComplete)) } answers { onComplete.captured.invoke() } @@ -559,4 +572,17 @@ class DefaultBrowserToolbarControllerTest { verify { deleteAndQuit(activity, testScope, null) } } + + @Test + fun handleToolbarReaderModePress() { + val item = ToolbarMenu.Item.ReaderMode(false) + + every { currentSession.id } returns "reader-inactive-tab" + controller.handleToolbarItemInteraction(item) + verify { readerModeController.showReaderView() } + + every { currentSession.id } returns "reader-active-tab" + controller.handleToolbarItemInteraction(item) + verify { readerModeController.hideReaderView() } + } } diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenuTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenuTest.kt index 14b064237..7a28ef9af 100644 --- a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenuTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenuTest.kt @@ -8,6 +8,10 @@ import io.mockk.mockkStatic import io.mockk.spyk import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.ReaderState +import mozilla.components.browser.state.state.createTab +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.feature.app.links.AppLinkRedirect import mozilla.components.feature.app.links.AppLinksUseCases @@ -28,12 +32,19 @@ class DefaultToolbarMenuTest { private val lifecycleOwner: LifecycleOwner = mockk(relaxed = true) private val bookmarksStorage: BookmarksStorage = mockk(relaxed = true) + private val store: BrowserStore = BrowserStore(initialState = BrowserState( + listOf( + createTab("https://www.mozilla.org", id = "readerable-tab", readerState = ReaderState(readerable = true)) + ) + )) + @Before fun setUp() { defaultToolbarMenu = spyk( DefaultToolbarMenu( context, sessionManager, + store, hasAccountProblem = false, shouldReverseItems = false, onItemTapped = onItemTapped, @@ -59,7 +70,7 @@ class DefaultToolbarMenuTest { every { getAppLinkRedirect(any()) } returns appLinkRedirect val session: Session = mockk(relaxed = true) - every { session.readerable } returns true + every { session.id } returns "readerable-tab" every { sessionManager.selectedSession } returns session val list = defaultToolbarMenu.getLowPrioHighlightItems() @@ -84,7 +95,7 @@ class DefaultToolbarMenuTest { every { getAppLinkRedirect(any()) } returns appLinkRedirect val session: Session = mockk(relaxed = true) - every { session.readerable } returns true + every { session.id } returns "readerable-tab" every { sessionManager.selectedSession } returns session val list = defaultToolbarMenu.getLowPrioHighlightItems() @@ -114,7 +125,7 @@ class DefaultToolbarMenuTest { every { context.components.useCases.webAppUseCases.isInstallable() } returns true val session: Session = mockk(relaxed = true) - every { session.readerable } returns true + every { session.id } returns "readerable-tab" every { sessionManager.selectedSession } returns session val getAppLinkRedirect: AppLinksUseCases.GetAppLinkRedirect = mockk(relaxed = true)