From 8a2d4cf6650e0c8fabbf2fde278fc0dc44e9517d Mon Sep 17 00:00:00 2001 From: rahulsainani Date: Tue, 27 Feb 2024 17:24:58 +0100 Subject: [PATCH] Bug 1882106 - Show undo snackbar when tab is closed via tab strip --- .../fenix/browser/BaseBrowserFragment.kt | 32 ++++++++--------- .../mozilla/fenix/browser/BrowserFragment.kt | 9 ++++- .../fenix/browser/tabstrip/TabStrip.kt | 22 +++++++----- .../fenix/components/appstate/AppAction.kt | 12 +++++++ .../fenix/components/appstate/AppState.kt | 3 ++ .../components/appstate/AppStoreReducer.kt | 4 +++ .../java/org/mozilla/fenix/ext/Context.kt | 12 +++++++ .../org/mozilla/fenix/home/HomeFragment.kt | 26 +++++++++----- .../components/appstate/TabStripActionTest.kt | 35 +++++++++++++++++++ 9 files changed, 119 insertions(+), 36 deletions(-) create mode 100644 app/src/test/java/org/mozilla/fenix/components/appstate/TabStripActionTest.kt 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 cc7cb1072e..07e36dea2d 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -161,6 +161,7 @@ import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.runIfFragmentIsAttached import org.mozilla.fenix.ext.secure import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.ext.tabClosedUndoMessage import org.mozilla.fenix.home.HomeScreenViewModel import org.mozilla.fenix.home.SharedViewModel import org.mozilla.fenix.library.bookmarks.BookmarksSharedViewModel @@ -399,23 +400,7 @@ abstract class BaseBrowserFragment : }, onCloseTab = { closedSession -> val closedTab = store.state.findTab(closedSession.id) ?: return@DefaultBrowserToolbarController - - val snackbarMessage = if (closedTab.content.private) { - requireContext().getString(R.string.snackbar_private_tab_closed) - } else { - requireContext().getString(R.string.snackbar_tab_closed) - } - - viewLifecycleOwner.lifecycleScope.allowUndo( - binding.dynamicSnackbarContainer, - snackbarMessage, - requireContext().getString(R.string.snackbar_deleted_undo), - { - requireComponents.useCases.tabsUseCases.undo.invoke() - }, - paddedForBottomToolbar = true, - operation = { }, - ) + showUndoSnackbar(requireContext().tabClosedUndoMessage(closedTab.content.private)) }, ) val browserToolbarMenuController = DefaultBrowserToolbarMenuController( @@ -1017,6 +1002,19 @@ abstract class BaseBrowserFragment : initializeEngineView(toolbarHeight) } + protected fun showUndoSnackbar(message: String) { + viewLifecycleOwner.lifecycleScope.allowUndo( + binding.dynamicSnackbarContainer, + message, + requireContext().getString(R.string.snackbar_deleted_undo), + { + requireComponents.useCases.tabsUseCases.undo.invoke() + }, + paddedForBottomToolbar = true, + operation = { }, + ) + } + /** * Show a [Snackbar] when data is set to the device clipboard. To avoid duplicate displays of * information only show a [Snackbar] for Android 12 and lower. 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 bbc9a9c5c6..bb0f710bb8 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -53,6 +53,7 @@ import org.mozilla.fenix.ext.nav import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.runIfFragmentIsAttached import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.ext.tabClosedUndoMessage import org.mozilla.fenix.home.HomeFragment import org.mozilla.fenix.nimbus.FxNimbus import org.mozilla.fenix.settings.quicksettings.protections.cookiebanners.getCookieBannerUIMode @@ -268,12 +269,18 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler { ), ) }, - onLastTabClose = { + onLastTabClose = { isPrivate -> + requireComponents.appStore.dispatch( + AppAction.TabStripAction.UpdateLastTabClosed(isPrivate), + ) findNavController().navigate( BrowserFragmentDirections.actionGlobalHome(), ) }, onSelectedTabClick = {}, + onCloseTabClick = { isPrivate -> + showUndoSnackbar(requireContext().tabClosedUndoMessage(isPrivate)) + }, ) } } diff --git a/app/src/main/java/org/mozilla/fenix/browser/tabstrip/TabStrip.kt b/app/src/main/java/org/mozilla/fenix/browser/tabstrip/TabStrip.kt index 571b141655..fc9d19554b 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/tabstrip/TabStrip.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/tabstrip/TabStrip.kt @@ -75,6 +75,7 @@ private val addTabIconSize = 20.dp * @param appStore The [AppStore] instance used to observe browsing mode. * @param tabsUseCases The [TabsUseCases] instance to perform tab actions. * @param onAddTabClick Invoked when the add tab button is clicked. + * @param onCloseTabClick Invoked when a tab is closed. * @param onLastTabClose Invoked when the last remaining open tab is closed. * @param onSelectedTabClick Invoked when a tab is selected. */ @@ -85,7 +86,8 @@ fun TabStrip( appStore: AppStore = components.appStore, tabsUseCases: TabsUseCases = components.useCases.tabsUseCases, onAddTabClick: () -> Unit, - onLastTabClose: () -> Unit, + onCloseTabClick: (isPrivate: Boolean) -> Unit, + onLastTabClose: (isPrivate: Boolean) -> Unit, onSelectedTabClick: () -> Unit, ) { val isPrivateMode by appStore.observeAsState(false) { it.mode.isPrivate } @@ -96,11 +98,12 @@ fun TabStrip( TabStripContent( state = state, onAddTabClick = onAddTabClick, - onCloseTabClick = { + onCloseTabClick = { id, isPrivate -> if (state.tabs.size == 1) { - onLastTabClose() + onLastTabClose(isPrivate) } - tabsUseCases.removeTab(it) + tabsUseCases.removeTab(id) + onCloseTabClick(isPrivate) }, onSelectedTabClick = { tabsUseCases.selectTab(it) @@ -118,7 +121,7 @@ fun TabStrip( private fun TabStripContent( state: TabStripState, onAddTabClick: () -> Unit, - onCloseTabClick: (id: String) -> Unit, + onCloseTabClick: (id: String, isPrivate: Boolean) -> Unit, onSelectedTabClick: (id: String) -> Unit, onMove: (tabId: String, targetId: String, placeAfter: Boolean) -> Unit, ) { @@ -153,7 +156,7 @@ private fun TabStripContent( private fun TabsList( state: TabStripState, modifier: Modifier = Modifier, - onCloseTabClick: (id: String) -> Unit, + onCloseTabClick: (id: String, isPrivate: Boolean) -> Unit, onSelectedTabClick: (id: String) -> Unit, onMove: (tabId: String, targetId: String, placeAfter: Boolean) -> Unit, ) { @@ -246,7 +249,7 @@ private fun LazyListState.isItemPartiallyVisible(itemInfo: LazyListItemInfo?) = private fun TabItem( state: TabStripItem, modifier: Modifier = Modifier, - onCloseTabClick: (id: String) -> Unit, + onCloseTabClick: (id: String, isPrivate: Boolean) -> Unit, onSelectedTabClick: (id: String) -> Unit, ) { TabStripCard( @@ -297,7 +300,7 @@ private fun TabItem( ) } - IconButton(onClick = { onCloseTabClick(state.id) }) { + IconButton(onClick = { onCloseTabClick(state.id, state.isPrivate) }) { Icon( painter = painterResource(R.drawable.mozac_ic_cross_20), tint = FirefoxTheme.colors.iconPrimary, @@ -411,7 +414,7 @@ private fun TabStripContentPreview(tabs: List) { tabs = tabs, ), onAddTabClick = {}, - onCloseTabClick = {}, + onCloseTabClick = { _, _ -> }, onSelectedTabClick = {}, onMove = { _, _, _ -> }, ) @@ -441,6 +444,7 @@ private fun TabStripPreview() { browserStore.dispatch(TabListAction.AddTabAction(tab)) }, onLastTabClose = {}, + onCloseTabClick = {}, onSelectedTabClick = {}, ) } diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt index 07eb907e7c..a2089f3515 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt +++ b/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt @@ -267,4 +267,16 @@ sealed class AppAction : Action { val key: ShoppingState.ProductRecommendationImpressionKey, ) : ShoppingAction() } + + /** + * [AppAction]s related to the tab strip. + */ + sealed class TabStripAction : AppAction() { + + /** + * [TabStripAction] used to update whether the last remaining tab that was closed was private. + * Null means the state should reset and no snackbar should be shown. + */ + data class UpdateLastTabClosed(val private: Boolean?) : TabStripAction() + } } diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppState.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppState.kt index a056a00e75..30e141a270 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppState.kt +++ b/app/src/main/java/org/mozilla/fenix/components/appstate/AppState.kt @@ -56,6 +56,8 @@ import org.mozilla.fenix.wallpapers.WallpaperState * @property wallpaperState The [WallpaperState] to display in the [HomeFragment]. * @property standardSnackbarError A snackbar error message to display. * @property shoppingState Holds state for shopping feature that's required to live the lifetime of a session. + * @property wasLastTabClosedPrivate Whether the last remaining tab that was closed in private mode. This is used to + * display an undo snackbar message relevant to the browsing mode. If null, no snackbar is shown. */ data class AppState( val isForeground: Boolean = true, @@ -81,4 +83,5 @@ data class AppState( val wallpaperState: WallpaperState = WallpaperState.default, val standardSnackbarError: StandardSnackbarError? = null, val shoppingState: ShoppingState = ShoppingState(), + val wasLastTabClosedPrivate: Boolean? = null, ) : State diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt index bf0ab3245f..deef63f40b 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt +++ b/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt @@ -239,6 +239,10 @@ internal object AppStoreReducer { ) is AppAction.ShoppingAction -> ShoppingStateReducer.reduce(state, action) + + is AppAction.TabStripAction.UpdateLastTabClosed -> state.copy( + wasLastTabClosedPrivate = action.private, + ) } } diff --git a/app/src/main/java/org/mozilla/fenix/ext/Context.kt b/app/src/main/java/org/mozilla/fenix/ext/Context.kt index edfc37ff12..11f37595d8 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/Context.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/Context.kt @@ -17,6 +17,7 @@ import android.view.accessibility.AccessibilityManager import androidx.annotation.StringRes import mozilla.components.support.locale.LocaleManager import org.mozilla.fenix.FenixApplication +import org.mozilla.fenix.R import org.mozilla.fenix.components.Components import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.settings.advanced.getSelectedLocale @@ -133,3 +134,14 @@ inline fun Context.startExternalActivitySafe(intent: Intent, onActivityNotPresen */ fun Context.isSystemInDarkTheme(): Boolean = resources.configuration.uiMode and Configuration.UI_MODE_NIGHT_MASK == Configuration.UI_MODE_NIGHT_YES + +/** + * Returns the message to be shown when a tab is closed based on whether the tab was private or not. + * @param private true if the tab was private, false otherwise. + */ +fun Context.tabClosedUndoMessage(private: Boolean): String = + if (private) { + getString(R.string.snackbar_private_tab_closed) + } else { + getString(R.string.snackbar_tab_closed) + } diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index 2b1963ec66..e64adaebe5 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -103,6 +103,7 @@ import org.mozilla.fenix.ext.nav import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.scaleToBottomOfView import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.ext.tabClosedUndoMessage import org.mozilla.fenix.home.pocket.DefaultPocketStoriesController import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesCategory import org.mozilla.fenix.home.privatebrowsing.controller.DefaultPrivateBrowsingController @@ -636,6 +637,11 @@ class HomeFragment : Fragment() { homeViewModel.sessionToDelete = null + requireComponents.appStore.state.wasLastTabClosedPrivate?.also { + showUndoSnackbar(requireContext().tabClosedUndoMessage(it)) + requireComponents.appStore.dispatch(AppAction.TabStripAction.UpdateLastTabClosed(null)) + } + tabCounterView?.update(requireComponents.core.store.state) if (bundleArgs.getBoolean(FOCUS_ON_ADDRESS_BAR)) { @@ -707,6 +713,9 @@ class HomeFragment : Fragment() { (requireActivity() as HomeActivity).openToBrowser(BrowserDirection.FromHome) }, onLastTabClose = {}, + onCloseTabClick = { isPrivate -> + showUndoSnackbar(requireContext().tabClosedUndoMessage(isPrivate)) + }, ) } } @@ -765,18 +774,14 @@ class HomeFragment : Fragment() { private fun removeTabAndShowSnackbar(sessionId: String) { val tab = store.state.findTab(sessionId) ?: return - requireComponents.useCases.tabsUseCases.removeTab(sessionId) + showUndoSnackbar(requireContext().tabClosedUndoMessage(tab.content.private)) + } - val snackbarMessage = if (tab.content.private) { - requireContext().getString(R.string.snackbar_private_tab_closed) - } else { - requireContext().getString(R.string.snackbar_tab_closed) - } - + private fun showUndoSnackbar(message: String) { viewLifecycleOwner.lifecycleScope.allowUndo( requireView(), - snackbarMessage, + message, requireContext().getString(R.string.snackbar_deleted_undo), { requireComponents.useCases.tabsUseCases.undo.invoke() @@ -1136,12 +1141,15 @@ class HomeFragment : Fragment() { } companion object { + // Used to set homeViewModel.sessionToDelete when all tabs of a browsing mode are closed const val ALL_NORMAL_TABS = "all_normal" const val ALL_PRIVATE_TABS = "all_private" + // Navigation arguments passed to HomeFragment private const val FOCUS_ON_ADDRESS_BAR = "focusOnAddressBar" - private const val SCROLL_TO_COLLECTION = "scrollToCollection" + + // Delay for scrolling to the collection header private const val ANIM_SCROLL_DELAY = 100L // Sponsored top sites titles and search engine names used for filtering diff --git a/app/src/test/java/org/mozilla/fenix/components/appstate/TabStripActionTest.kt b/app/src/test/java/org/mozilla/fenix/components/appstate/TabStripActionTest.kt new file mode 100644 index 0000000000..42516b95dc --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/components/appstate/TabStripActionTest.kt @@ -0,0 +1,35 @@ +/* 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.components.appstate + +import mozilla.components.support.test.ext.joinBlocking +import org.junit.Assert.assertEquals +import org.junit.Test +import org.mozilla.fenix.components.AppStore + +class TabStripActionTest { + + @Test + fun `WHEN the last remaining tab that was closed was private THEN state should reflect that`() { + val store = AppStore(initialState = AppState()) + + store.dispatch(AppAction.TabStripAction.UpdateLastTabClosed(true)).joinBlocking() + + val expected = AppState(wasLastTabClosedPrivate = true) + + assertEquals(expected, store.state) + } + + @Test + fun `WHEN the last remaining tab that was closed was not private THEN state should reflect that`() { + val store = AppStore(initialState = AppState()) + + store.dispatch(AppAction.TabStripAction.UpdateLastTabClosed(false)).joinBlocking() + + val expected = AppState(wasLastTabClosedPrivate = false) + + assertEquals(expected, store.state) + } +}