For mozilla-mobile#22078 Selecting search group in Jump back in switc…

…hes active tab
Amejia481 · Nov 3, 2021 · 6e15b5f · 6e15b5f
1 parent a360063
commit 6e15b5f
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 24 deletions.
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
@@ -68,7 +68,6 @@ class DefaultRecentTabsController(
     }
 
     override fun handleRecentSearchGroupClicked(tabId: String) {
-        selectTabUseCase.invoke(tabId)
         metrics.track(Event.JumpBackInGroupTapped)
         navController.navigate(
             HomeFragmentDirections.actionGlobalTabsTrayFragment(

diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayStore.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayStore.kt
@@ -18,6 +18,7 @@ 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 selectedSearchGroupTabId The current page selected search group tab id. Default to null.
  */
 data class TabsTrayState(
     val selectedPage: Page = Page.NormalTabs,
@@ -120,6 +121,11 @@ sealed class TabsTrayAction : Action {
      * no sync action was able to be performed.
      */
     object SyncCompleted : TabsTrayAction()
+
+    /**
+     * Removes the [TabsTrayState.selectedSearchGroupTabId] of the [TabsTrayState].
+     */
+    object ConsumeSelectedSearchGroupTabIdAction : TabsTrayAction()
 }
 
 /**
@@ -150,6 +156,8 @@ internal object TabsTrayReducer {
                 state.copy(syncing = true)
             is TabsTrayAction.SyncCompleted ->
                 state.copy(syncing = false)
+            is TabsTrayAction.ConsumeSelectedSearchGroupTabIdAction ->
+                state.copy(selectedSearchGroupTabId = 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
@@ -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,9 @@ 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.selectedSearchGroupTabId
 
         // Update tabs into the inactive adapter.
@@ -107,8 +111,11 @@ class NormalBrowserPageViewHolder(
         }
 
         // Updates tabs into the search term group adapter.
-        if (searchTermTabGroupsAreEnabled && ( selectedSearchGroupTabId.isNullOrEmpty() ||
-                selectedTab.isNormalTabActiveWithSearchTerm(maxActiveTime))) {
+        if (searchTermTabGroupsAreEnabled && (
+            !selectedSearchGroupTabId.isNullOrEmpty() ||
+                selectedTab.isNormalTabActiveWithSearchTerm(maxActiveTime)
+            )
+        ) {
             val tabId = selectedSearchGroupTabId ?: selectedTab.id
 
             tabGroupAdapter.observeFirstInsert {
@@ -127,29 +134,34 @@ class NormalBrowserPageViewHolder(
                         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
@@ -130,11 +130,6 @@
         app:destination="@id/tabsTrayFragment"
         app:popUpTo="@id/tabsTrayFragment"
         app:popUpToInclusive="true">
-        <argument
-            android:name="selectedSearchGroupTabId"
-            app:nullable="true"
-            android:defaultValue="null"
-            app:argType="string" />
     </action>
     <action
         android:id="@+id/action_global_savedLoginsAuthFragment"
@@ -159,7 +154,7 @@
         <argument
             android:name="selectedSearchGroupTabId"
             app:nullable="true"
-            android:defaultValue="null"
+            android:defaultValue="@null"
             app:argType="string" />
     </dialog>
 

diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayStoreReducerTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/TabsTrayStoreReducerTest.kt
@@ -0,0 +1,23 @@
+/* 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.tabstray
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+class TabsTrayStoreReducerTest {
+    @Test
+    fun `GIVEN selectedSearchGroupTabId WHEN ConsumeSelectedSearchGroupTabIdAction THEN selectedSearchGroupTabId must be consumed`() {
+        val initialState = TabsTrayState(selectedSearchGroupTabId = "id")
+        val expectedState = initialState.copy(selectedSearchGroupTabId = null)
+
+        val resultState = TabsTrayReducer.reduce(
+            initialState,
+            TabsTrayAction.ConsumeSelectedSearchGroupTabIdAction
+        )
+
+        assertEquals(expectedState, resultState)
+    }
+}