From aed86b9d3086a0778450b2292428be0a563d37ba Mon Sep 17 00:00:00 2001 From: Zac McKenney Date: Tue, 6 Dec 2022 10:42:08 -0800 Subject: [PATCH] [fenix] Bug 1804107 - Fix for BaseBrowserFragment toolbar getter null crash and invalidating actions when modifying toolbar --- .../fenix/browser/BaseBrowserFragment.kt | 10 +++++++-- .../mozilla/fenix/browser/BrowserFragment.kt | 22 ++++++++++++------- .../fenix/browser/BrowserFragmentTest.kt | 21 ++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt index ec0d715fd0..12c12a2fba 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -33,7 +33,6 @@ import com.google.android.material.snackbar.Snackbar import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Dispatchers.Main import kotlinx.coroutines.Job -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.launch @@ -1400,6 +1399,11 @@ abstract class BaseBrowserFragment : binding.swipeRefresh.isEnabled = shouldPullToRefreshBeEnabled(inFullScreen) } + @CallSuper + internal open fun onUpdateToolbarForConfigurationChange(toolbar: BrowserToolbarView) { + toolbar.dismissMenu() + } + /* * Dereference these views when the fragment view is destroyed to prevent memory leaks */ @@ -1472,7 +1476,9 @@ abstract class BaseBrowserFragment : override fun onConfigurationChanged(newConfig: Configuration) { super.onConfigurationChanged(newConfig) - _browserToolbarView?.dismissMenu() + _browserToolbarView?.let { + onUpdateToolbarForConfigurationChange(it) + } } // This method is called in response to native web extension messages from 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 5aa5ac704f..c2760bd9ed 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -5,7 +5,6 @@ package org.mozilla.fenix.browser import android.content.Context -import android.content.res.Configuration import android.os.StrictMode import android.view.View import android.view.ViewGroup @@ -32,6 +31,7 @@ import org.mozilla.fenix.GleanMetrics.ReaderMode import org.mozilla.fenix.R import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.components.TabCollectionStorage +import org.mozilla.fenix.components.toolbar.BrowserToolbarView import org.mozilla.fenix.components.toolbar.ToolbarMenu import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.nav @@ -90,7 +90,7 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { browserToolbarView.view.addNavigationAction(homeAction) - setScreenSize(isTablet = resources.getBoolean(R.bool.tablet)) + updateToolbarActions(isTablet = resources.getBoolean(R.bool.tablet)) val readerModeAction = BrowserToolbar.ToggleButton( @@ -170,7 +170,14 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { } } - private fun setScreenSize(isTablet: Boolean) { + override fun onUpdateToolbarForConfigurationChange(toolbar: BrowserToolbarView) { + super.onUpdateToolbarForConfigurationChange(toolbar) + + updateToolbarActions(isTablet = resources.getBoolean(R.bool.tablet)) + } + + @VisibleForTesting + internal fun updateToolbarActions(isTablet: Boolean) { if (isTablet == this.isTablet) return if (isTablet) { @@ -280,6 +287,8 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { refreshAction?.let { browserToolbarView.view.addNavigationAction(it) } + + browserToolbarView.view.invalidateActions() } private fun removeTabletActions() { @@ -292,6 +301,8 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { refreshAction?.let { browserToolbarView.view.removeNavigationAction(it) } + + browserToolbarView.view.invalidateActions() } override fun onStart() { @@ -447,9 +458,4 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { internal fun updateLastBrowseActivity() { requireContext().settings().lastBrowseActivity = System.currentTimeMillis() } - - override fun onConfigurationChanged(newConfig: Configuration) { - super.onConfigurationChanged(newConfig) - setScreenSize(isTablet = resources.getBoolean(R.bool.tablet)) - } } diff --git a/app/src/test/java/org/mozilla/fenix/browser/BrowserFragmentTest.kt b/app/src/test/java/org/mozilla/fenix/browser/BrowserFragmentTest.kt index f79056199e..0009f21553 100644 --- a/app/src/test/java/org/mozilla/fenix/browser/BrowserFragmentTest.kt +++ b/app/src/test/java/org/mozilla/fenix/browser/BrowserFragmentTest.kt @@ -312,6 +312,21 @@ class BrowserFragmentTest { verify(exactly = 1) { toolbarIntegration.invalidateMenu() } } + @Test + fun `WHEN toolbar is initialized THEN onConfigurationChanged sets toolbar actions for size in fragment`() { + val browserToolbarView: BrowserToolbarView = mockk(relaxed = true) + + browserFragment._browserToolbarView = null + browserFragment.onConfigurationChanged(mockk(relaxed = true)) + verify(exactly = 0) { browserFragment.onUpdateToolbarForConfigurationChange(any()) } + verify(exactly = 0) { browserFragment.updateToolbarActions(any()) } + + browserFragment._browserToolbarView = browserToolbarView + browserFragment.onConfigurationChanged(mockk(relaxed = true)) + verify(exactly = 1) { browserFragment.onUpdateToolbarForConfigurationChange(any()) } + verify(exactly = 1) { browserFragment.updateToolbarActions(any()) } + } + @Test fun `WHEN fragment configuration changed THEN menu is dismissed`() { val browserToolbarView: BrowserToolbarView = mockk(relaxed = true) @@ -325,7 +340,9 @@ class BrowserFragmentTest { @Test fun `WHEN fragment configuration screen size changes between tablet and mobile size THEN tablet action items added and removed`() { + val browserToolbarView: BrowserToolbarView = mockk(relaxed = true) val browserToolbar: BrowserToolbar = mockk(relaxed = true) + browserFragment._browserToolbarView = browserToolbarView every { browserFragment.browserToolbarView.view } returns browserToolbar mockkObject(ThemeManager.Companion) @@ -348,7 +365,9 @@ class BrowserFragmentTest { @Test fun `WHEN fragment configuration change enables tablet size twice THEN tablet action items are only added once`() { + val browserToolbarView: BrowserToolbarView = mockk(relaxed = true) val browserToolbar: BrowserToolbar = mockk(relaxed = true) + browserFragment._browserToolbarView = browserToolbarView every { browserFragment.browserToolbarView.view } returns browserToolbar mockkObject(ThemeManager.Companion) @@ -370,7 +389,9 @@ class BrowserFragmentTest { @Test fun `WHEN fragment configuration change sets mobile size twice THEN tablet action items are not added or removed`() { + val browserToolbarView: BrowserToolbarView = mockk(relaxed = true) val browserToolbar: BrowserToolbar = mockk(relaxed = true) + browserFragment._browserToolbarView = browserToolbarView every { browserFragment.browserToolbarView.view } returns browserToolbar mockkObject(ThemeManager.Companion)