No issue: Refactor readerview to use browser-state

2020-04-06 17:18:41 -04:00 · 2020-04-06 17:18:41 -04:00 · 0f1bff7402
parent c427b0a70b
commit 0f1bff7402
9 changed files with 77 additions and 20 deletions
--- 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
--- 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()
            )
        )
    }
--- 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()
--- 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 {
--- 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),
--- 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()
    }
 }
--- 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(
--- 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<FenixSnackbar>(relaxed = true)
    private val tabCollectionStorage = mockk<TabCollectionStorage>(relaxed = true)
    private val topSiteStorage = mockk<TopSiteStorage>(relaxed = true)
+    private val readerModeController = mockk<ReaderModeController>(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() }
+    }
 }
--- 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)