From 538a3ce36c7947a3f9921e6497b209116a92506c Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Wed, 3 Nov 2021 12:14:13 -0400 Subject: [PATCH] For #22078 Selecting search group in Jump back in switches active tab --- .../controller/RecentTabController.kt | 7 ++- .../fenix/tabstray/TabsTrayFragment.kt | 3 +- .../mozilla/fenix/tabstray/TabsTrayStore.kt | 11 +++- .../NormalBrowserPageViewHolder.kt | 50 ++++++++++++------- app/src/main/res/navigation/nav_graph.xml | 5 ++ .../tabstray/TabsTrayStoreReducerTest.kt | 23 +++++++++ 6 files changed, 78 insertions(+), 21 deletions(-) create mode 100644 app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayStoreReducerTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt index 9648375702e6..b37de99fdc3c 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recenttabs/controller/RecentTabController.kt @@ -68,9 +68,12 @@ class DefaultRecentTabsController( } override fun handleRecentSearchGroupClicked(tabId: String) { - selectTabUseCase.invoke(tabId) metrics.track(Event.JumpBackInGroupTapped) - navController.navigate(HomeFragmentDirections.actionGlobalTabsTrayFragment()) + navController.navigate( + HomeFragmentDirections.actionGlobalTabsTrayFragment( + focusGroupTabId = tabId + ) + ) } @VisibleForTesting(otherwise = PRIVATE) diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt index cbc8791d9ddb..d92e697b7037 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt @@ -123,7 +123,8 @@ class TabsTrayFragment : AppCompatDialogFragment() { tabsTrayStore = StoreProvider.get(this) { TabsTrayStore( initialState = TabsTrayState( - mode = initialMode + mode = initialMode, + focusGroupTabId = args.selectedSearchGroupTabId ) ) } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayStore.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayStore.kt index cba2707dd182..57237e40f622 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayStore.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayStore.kt @@ -18,11 +18,13 @@ import mozilla.components.lib.state.Store * currently selected tabs. * @property syncing Whether the Synced Tabs feature should fetch the latest tabs from paired * devices. + * @property focusGroupTabId The search group tab id to focus. Defaults to null. */ data class TabsTrayState( val selectedPage: Page = Page.NormalTabs, val mode: Mode = Mode.Normal, - val syncing: Boolean = false + val syncing: Boolean = false, + val focusGroupTabId: String? = null ) : State { /** @@ -119,6 +121,11 @@ sealed class TabsTrayAction : Action { * no sync action was able to be performed. */ object SyncCompleted : TabsTrayAction() + + /** + * Removes the [TabsTrayState.focusGroupTabId] of the [TabsTrayState]. + */ + object ConsumeSelectedSearchGroupTabIdAction : TabsTrayAction() } /** @@ -149,6 +156,8 @@ internal object TabsTrayReducer { state.copy(syncing = true) is TabsTrayAction.SyncCompleted -> state.copy(syncing = false) + is TabsTrayAction.ConsumeSelectedSearchGroupTabIdAction -> + state.copy(focusGroupTabId = null) } } } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt index 6bbe5b0bd795..39411cde807c 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/NormalBrowserPageViewHolder.kt @@ -17,6 +17,7 @@ import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.ext.settings import org.mozilla.fenix.selection.SelectionHolder +import org.mozilla.fenix.tabstray.TabsTrayAction import org.mozilla.fenix.tabstray.TabsTrayInteractor import org.mozilla.fenix.tabstray.TabsTrayStore import org.mozilla.fenix.tabstray.browser.containsTabId @@ -84,6 +85,10 @@ class NormalBrowserPageViewHolder( val searchTermTabGroupsAreEnabled = containerView.context.settings().searchTermTabGroupsAreEnabled val selectedTab = browserStore.state.selectedNormalTab ?: return + // It's safe to read the state directly (i.e. won't cause bugs because of the store actions + // processed on a separate thread) instead of observing it because this value is only set during + // the initialState of the TabsTrayStore being created. + val selectedSearchGroupTabId = tabsTrayStore.state.focusGroupTabId // Update tabs into the inactive adapter. if (inactiveTabsAreEnabled && selectedTab.isNormalTabInactive(maxActiveTime)) { @@ -106,7 +111,13 @@ class NormalBrowserPageViewHolder( } // Updates tabs into the search term group adapter. - if (searchTermTabGroupsAreEnabled && selectedTab.isNormalTabActiveWithSearchTerm(maxActiveTime)) { + if (searchTermTabGroupsAreEnabled && ( + !selectedSearchGroupTabId.isNullOrEmpty() || + selectedTab.isNormalTabActiveWithSearchTerm(maxActiveTime) + ) + ) { + val tabId = selectedSearchGroupTabId ?: selectedTab.id + tabGroupAdapter.observeFirstInsert { // With a grouping, we need to use the list of the adapter that is already grouped // together for the UI, so we know the final index of the grouping to scroll to. @@ -117,35 +128,40 @@ class NormalBrowserPageViewHolder( // [DiffUtil.calculateDiff] directly to submit a changed list which evades the `ListAdapter` from being // notified of updates, so it therefore returns an empty list. tabGroupAdapter.currentList.forEachIndexed { groupIndex, group -> - if (group.containsTabId(selectedTab.id)) { + if (group.containsTabId(tabId)) { // Index is based on tabs above (inactive) with our calculated index. val indexToScrollTo = inactiveTabAdapter.itemCount + groupIndex layoutManager.scrollToPosition(indexToScrollTo) + if (selectedSearchGroupTabId != null) { + tabsTrayStore.dispatch(TabsTrayAction.ConsumeSelectedSearchGroupTabIdAction) + } return@observeFirstInsert } } } } - // Updates tabs into the normal browser tabs adapter. - browserAdapter.observeFirstInsert { - val activeTabsList = browserStore.state.getNormalTrayTabs( - searchTermTabGroupsAreEnabled, - inactiveTabsAreEnabled - ) - activeTabsList.forEachIndexed { tabIndex, trayTab -> - if (trayTab.id == selectedTab.id) { - - // Index is based on tabs above (inactive + groups + header) with our calculated index. - val indexToScrollTo = inactiveTabAdapter.itemCount + - tabGroupAdapter.itemCount + - headerAdapter.itemCount + tabIndex + if (selectedSearchGroupTabId.isNullOrEmpty()) { + // Updates tabs into the normal browser tabs adapter. + browserAdapter.observeFirstInsert { + val activeTabsList = browserStore.state.getNormalTrayTabs( + searchTermTabGroupsAreEnabled, + inactiveTabsAreEnabled + ) + activeTabsList.forEachIndexed { tabIndex, trayTab -> + if (trayTab.id == selectedTab.id) { + + // Index is based on tabs above (inactive + groups + header) with our calculated index. + val indexToScrollTo = inactiveTabAdapter.itemCount + + tabGroupAdapter.itemCount + + headerAdapter.itemCount + tabIndex - layoutManager.scrollToPosition(indexToScrollTo) + layoutManager.scrollToPosition(indexToScrollTo) - return@observeFirstInsert + return@observeFirstInsert + } } } } diff --git a/app/src/main/res/navigation/nav_graph.xml b/app/src/main/res/navigation/nav_graph.xml index e0a1173c7a69..6d9b80bb8f6e 100644 --- a/app/src/main/res/navigation/nav_graph.xml +++ b/app/src/main/res/navigation/nav_graph.xml @@ -150,6 +150,11 @@ android:name="enterMultiselect" android:defaultValue="false" app:argType="boolean" /> +