For #21360 - Added toggle for search term tab groups (#21615)

* For #21360 - Added toggle for search term tab groups * For #21360 - Lint cleanup * PR: Added missing licenses and possibly fixed UI test * PR: Added a "scrollTo" to potentially fix a UI test * PR: Added potential fix for alwaysStartOnHomeTest * PR: Added temporary ignore to alwaysStartOnHomeTest * PR: added missing ignore comment * For #21360 - Added missing feature flag driven visibility logic Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mozilla-mobile · Oct 2, 2021 · aa28b6f · aa28b6f
1 parent c7067a5
commit aa28b6f
Show file tree

Hide file tree

Showing 15 changed files with 82 additions and 19 deletions.
diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt
@@ -1438,6 +1438,7 @@ class SmokeTest {
         }
     }
 
+    @Ignore // to be fixed here https://github.com/mozilla-mobile/fenix/issues/21644
     @Test
     fun alwaysStartOnHomeTest() {
         val settings = activityTestRule.activity.applicationContext.settings()

diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/SettingsSubMenuTabsRobot.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/SettingsSubMenuTabsRobot.kt
@@ -16,6 +16,7 @@ import androidx.test.espresso.matcher.ViewMatchers.withText
 import androidx.test.platform.app.InstrumentationRegistry
 import androidx.test.uiautomator.UiDevice
 import org.hamcrest.CoreMatchers.allOf
+import org.mozilla.fenix.helpers.TestHelper.scrollToElementByText
 import org.mozilla.fenix.helpers.click
 
 /**
@@ -29,7 +30,10 @@ class SettingsSubMenuTabsRobot {
 
     fun verifyStartOnHomeOptions() = assertStartOnHomeOptions()
 
-    fun clickAlwaysStartOnHomeToggle() = alwaysStartOnHomeToggle().click()
+    fun clickAlwaysStartOnHomeToggle() {
+        scrollToElementByText("Always")
+        alwaysStartOnHomeToggle().click()
+    }
 
     class Transition {
         val mDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())
@@ -51,6 +55,8 @@ private fun assertTabViewOptions() {
         .check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
     gridToggle()
         .check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
+    searchTermTabGroupsToggle()
+        .check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
 }
 
 private fun assertCloseTabsOptions() {
@@ -65,6 +71,8 @@ private fun assertCloseTabsOptions() {
 }
 
 private fun assertStartOnHomeOptions() {
+    // Scroll to ensure all the items are visible.
+    scrollToElementByText("Never")
     startOnHomeHeading()
         .check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
     afterFourHoursToggle()
@@ -79,6 +87,8 @@ private fun listToggle() = onView(withText("List"))
 
 private fun gridToggle() = onView(withText("Grid"))
 
+private fun searchTermTabGroupsToggle() = onView(withText("Search groups"))
+
 private fun closeTabsHeading() = onView(withText("Close tabs"))
 
 private fun manuallyToggle() = onView(withText("Manually"))

diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabs.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabs.kt
@@ -43,7 +43,6 @@ import mozilla.components.browser.icons.compose.Placeholder
 import mozilla.components.browser.icons.compose.WithIcon
 import mozilla.components.support.ktx.kotlin.getRepresentativeSnippet
 import mozilla.components.ui.colors.PhotonColors
-import org.mozilla.fenix.FeatureFlags
 import org.mozilla.fenix.R
 import org.mozilla.fenix.components.components
 import org.mozilla.fenix.home.recenttabs.RecentTab
@@ -78,7 +77,7 @@ fun RecentTabs(
                     )
                 }
                 is RecentTab.SearchGroup -> {
-                    if (FeatureFlags.tabGroupFeature) {
+                    if (components.settings.searchTermTabGroupsAreEnabled) {
                         RecentSearchGroupItem(
                             searchTerm = tab.searchTerm,
                             tabId = tab.tabId,

diff --git a/app/src/main/java/org/mozilla/fenix/settings/TabsSettingsFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/TabsSettingsFragment.kt
@@ -34,6 +34,7 @@ class TabsSettingsFragment : PreferenceFragmentCompat() {
     private lateinit var startOnHomeRadioNever: RadioButtonPreference
     private lateinit var inactiveTabsCategory: PreferenceCategory
     private lateinit var inactiveTabs: SwitchPreference
+    private lateinit var searchTermTabGroups: SwitchPreference
 
     override fun onCreatePreferences(savedInstanceState: Bundle?, rootKey: String?) {
         setPreferencesFromResource(R.xml.tabs_preferences, rootKey)
@@ -58,6 +59,11 @@ class TabsSettingsFragment : PreferenceFragmentCompat() {
         // pref_key_tab_view_grid and look into using the native RadioGroup in the future.
         listRadioButton = requirePreference(R.string.pref_key_tab_view_list_do_not_use)
         gridRadioButton = requirePreference(R.string.pref_key_tab_view_grid)
+        searchTermTabGroups = requirePreference<SwitchPreference>(R.string.pref_key_search_term_tab_groups).also {
+            it.isVisible = FeatureFlags.tabGroupFeature
+            it.isChecked = it.context.settings().searchTermTabGroupsAreEnabled
+            it.onPreferenceChangeListener = SharedPreferenceUpdater()
+        }
 
         radioManual = requirePreference(R.string.pref_key_close_tabs_manually)
         radioOneMonth = requirePreference(R.string.pref_key_close_tabs_after_one_month)

diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt
@@ -10,7 +10,6 @@ import androidx.recyclerview.widget.ConcatAdapter
 import mozilla.components.browser.state.state.TabSessionState
 import mozilla.components.browser.tabstray.TabViewHolder
 import mozilla.components.feature.tabs.tabstray.TabsFeature
-import org.mozilla.fenix.FeatureFlags
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.tabstray.ext.browserAdapter
@@ -45,14 +44,15 @@ class NormalBrowserTrayList @JvmOverloads constructor(
     override val tabsFeature by lazy {
         val tabsAdapter = concatAdapter.browserAdapter
         val inactiveTabsEnabled = context.settings().inactiveTabsAreEnabled
+        val searchTermTabGroupsAreEnabled = context.settings().searchTermTabGroupsAreEnabled
         val tabFilter: (TabSessionState) -> Boolean = {
             when {
-                FeatureFlags.tabGroupFeature && inactiveTabsEnabled ->
+                searchTermTabGroupsAreEnabled && inactiveTabsEnabled ->
                     it.isNormalTabActiveWithoutSearchTerm(maxActiveTime)
 
                 inactiveTabsEnabled -> it.isNormalTabActive(maxActiveTime)
 
-                FeatureFlags.tabGroupFeature -> it.isNormalTabWithoutSearchTerm()
+                searchTermTabGroupsAreEnabled -> it.isNormalTabWithoutSearchTerm()
 
                 else -> !it.content.private
             }
@@ -71,11 +71,13 @@ class NormalBrowserTrayList @JvmOverloads constructor(
     private val searchTermFeature by lazy {
         val store = context.components.core.store
         val inactiveTabsEnabled = context.settings().inactiveTabsAreEnabled
+        val searchTermTabGroupsAreEnabled = context.settings().searchTermTabGroupsAreEnabled
         val tabFilter: (TabSessionState) -> Boolean = {
             when {
-                FeatureFlags.tabGroupFeature && inactiveTabsEnabled -> it.isNormalTabActiveWithSearchTerm(maxActiveTime)
+                searchTermTabGroupsAreEnabled && inactiveTabsEnabled ->
+                    it.isNormalTabActiveWithSearchTerm(maxActiveTime)
 
-                FeatureFlags.tabGroupFeature -> it.isNormalTabWithSearchTerm()
+                searchTermTabGroupsAreEnabled -> it.isNormalTabWithSearchTerm()
 
                 else -> false
             }

diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/TitleHeaderBinding.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/TitleHeaderBinding.kt
@@ -25,8 +25,12 @@ class TitleHeaderBinding(
     private val showHeader: (Boolean) -> Unit
 ) : AbstractBinding<BrowserState>(store) {
     override suspend fun onState(flow: Flow<BrowserState>) {
-        flow.map { it.getNormalTrayTabs(settings.inactiveTabsAreEnabled) }
-            .ifChanged { it.size }
+        flow.map {
+            it.getNormalTrayTabs(
+                settings.searchTermTabGroupsAreEnabled,
+                settings.inactiveTabsAreEnabled
+            )
+        }.ifChanged { it.size }
             .collect {
                 if (it.isEmpty()) {
                     showHeader(false)

diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/ext/TabSelectors.kt b/app/src/main/java/org/mozilla/fenix/tabstray/ext/TabSelectors.kt
@@ -8,7 +8,6 @@ import mozilla.components.browser.state.selector.normalTabs
 import mozilla.components.browser.state.selector.privateTabs
 import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.state.TabSessionState
-import org.mozilla.fenix.FeatureFlags
 import org.mozilla.fenix.tabstray.browser.maxActiveTime
 
 /**
@@ -41,13 +40,16 @@ val BrowserState.inactiveTabs: List<TabSessionState>
 /**
  * The list of normal tabs in the tabs tray filtered appropriately based on feature flags.
  */
-fun BrowserState.getNormalTrayTabs(inactiveTabsEnabled: Boolean): List<TabSessionState> {
+fun BrowserState.getNormalTrayTabs(
+    searchTermTabGroupsAreEnabled: Boolean,
+    inactiveTabsEnabled: Boolean
+): List<TabSessionState> {
     return normalTabs.run {
-        if (FeatureFlags.tabGroupFeature && inactiveTabsEnabled) {
+        if (searchTermTabGroupsAreEnabled && inactiveTabsEnabled) {
             filter { it.isNormalTabActiveWithoutSearchTerm(maxActiveTime) }
         } else if (inactiveTabsEnabled) {
             filter { it.isNormalTabActive(maxActiveTime) }
-        } else if (FeatureFlags.tabGroupFeature) {
+        } else if (searchTermTabGroupsAreEnabled) {
             filter { it.isNormalTabWithSearchTerm() }
         } else {
             this

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
@@ -12,7 +12,6 @@ import androidx.recyclerview.widget.RecyclerView
 import mozilla.components.browser.state.selector.selectedNormalTab
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.concept.tabstray.Tab
-import org.mozilla.fenix.FeatureFlags
 import org.mozilla.fenix.R
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.selection.SelectionHolder
@@ -80,6 +79,7 @@ class NormalBrowserPageViewHolder(
         val inactiveTabAdapter = concatAdapter.inactiveTabsAdapter
         val tabGroupAdapter = concatAdapter.tabGroupAdapter
         val inactiveTabsAreEnabled = containerView.context.settings().inactiveTabsAreEnabled
+        val searchTermTabGroupsAreEnabled = containerView.context.settings().searchTermTabGroupsAreEnabled
 
         val selectedTab = browserStore.state.selectedNormalTab ?: return
 
@@ -102,7 +102,7 @@ class NormalBrowserPageViewHolder(
         }
 
         // Updates tabs into the search term group adapter.
-        if (FeatureFlags.tabGroupFeature && selectedTab.isNormalTabActiveWithSearchTerm(maxActiveTime)) {
+        if (searchTermTabGroupsAreEnabled && selectedTab.isNormalTabActiveWithSearchTerm(maxActiveTime)) {
             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.
@@ -127,7 +127,10 @@ class NormalBrowserPageViewHolder(
 
         // Updates tabs into the normal browser tabs adapter.
         browserAdapter.observeFirstInsert {
-            val activeTabsList = browserStore.state.getNormalTrayTabs(inactiveTabsAreEnabled)
+            val activeTabsList = browserStore.state.getNormalTrayTabs(
+                searchTermTabGroupsAreEnabled,
+                inactiveTabsAreEnabled
+            )
             activeTabsList.forEachIndexed { tabIndex, trayTab ->
                 if (trayTab.id == selectedTab.id) {
 

diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt
@@ -421,6 +421,15 @@ class Settings(private val appContext: Context) : PreferencesHolder {
         featureFlag = FeatureFlags.inactiveTabs
     )
 
+    /**
+     * Indicates if the user has enabled the search term tab groups feature.
+     */
+    var searchTermTabGroupsAreEnabled by featureFlagPreference(
+        appContext.getPreferenceKey(R.string.pref_key_search_term_tab_groups),
+        default = FeatureFlags.tabGroupFeature,
+        featureFlag = FeatureFlags.tabGroupFeature
+    )
+
     @VisibleForTesting
     internal fun timeNowInMillis(): Long = System.currentTimeMillis()
 

diff --git a/app/src/main/res/drawable-night/ic_all_tabs.xml b/app/src/main/res/drawable-night/ic_all_tabs.xml
@@ -0,0 +1,13 @@
+<!-- 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/. -->
+<vector xmlns:android="http://schemas.android.com/apk/res/android"
+    android:width="18dp"
+    android:height="18dp"
+    android:viewportWidth="18"
+    android:viewportHeight="18">
+  <path
+      android:pathData="M0,2.5C0,1.119 1.119,0 2.5,0H15.5C16.881,0 18,1.119 18,2.5V10.5C18,11.881 16.881,13 15.5,13H2.5C1.119,13 0,11.881 0,10.5V2.5ZM15.7,11.5L16.5,10.7V2.3L15.7,1.5H2.3L1.5,2.3V10.7L2.3,11.5H15.7ZM1.5,15.7L2.3,16.5H15.7L16.5,15.7V14.75C16.5,14.336 16.836,14 17.25,14C17.664,14 18,14.336 18,14.75V15.5C18,16.881 16.881,18 15.5,18H2.5C1.119,18 0,16.881 0,15.5V14.75C0,14.336 0.336,14 0.75,14C1.164,14 1.5,14.336 1.5,14.75V15.7Z"
+      android:fillColor="#FBFBFE"
+      android:fillType="evenOdd"/>
+</vector>
diff --git a/app/src/main/res/drawable/ic_all_tabs.xml b/app/src/main/res/drawable/ic_all_tabs.xml
@@ -1,3 +1,6 @@
+<!-- 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/. -->
 <vector xmlns:android="http://schemas.android.com/apk/res/android"
     android:width="18dp"
     android:height="18dp"

diff --git a/app/src/main/res/values/preference_keys.xml b/app/src/main/res/values/preference_keys.xml
@@ -270,6 +270,7 @@
     <string name="pref_key_camera_permissions_needed" translatable="false">pref_key_camera_permissions_needed</string>
     <string name="pref_key_inactive_tabs_category" translatable="false">pref_key_inactive_tabs_category</string>
     <string name="pref_key_inactive_tabs" translatable="false">pref_key_inactive_tabs</string>
+    <string name="pref_key_search_term_tab_groups" translatable="false">pref_key_search_term_tab_groups</string>
 
     <string name="pref_key_return_to_browser" translatable="false">pref_key_return_to_browser</string>
 

diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml
@@ -661,6 +661,10 @@
     <string name="tab_view_list">List</string>
     <!-- Option for a grid tab view -->
     <string name="tab_view_grid">Grid</string>
+    <!-- Option for search term tab groups -->
+    <string name="tab_view_search_term_tab_groups">Search groups</string>
+    <!-- Summary text for search term tab groups -->
+    <string name="tab_view_search_term_tab_groups_summary">Group related sites together</string>
     <!-- Title of preference that allows a user to auto close tabs after a specified amount of time -->
     <string name="preferences_close_tabs">Close tabs</string>
     <!-- Option for auto closing tabs that will never auto close tabs, always allows user to manually close tabs -->

diff --git a/app/src/main/res/xml/tabs_preferences.xml b/app/src/main/res/xml/tabs_preferences.xml
@@ -18,6 +18,13 @@
             android:defaultValue="true"
             android:key="@string/pref_key_tab_view_grid"
             android:title="@string/tab_view_grid" />
+
+        <SwitchPreference
+            android:defaultValue="true"
+            android:key="@string/pref_key_search_term_tab_groups"
+            android:title="@string/tab_view_search_term_tab_groups"
+            android:summary="@string/tab_view_search_term_tab_groups_summary"
+            app:isPreferenceVisible="false"/>
     </androidx.preference.PreferenceCategory>
 
     <androidx.preference.PreferenceCategory

diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/browser/TitleHeaderBindingTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/browser/TitleHeaderBindingTest.kt
@@ -18,7 +18,6 @@ import mozilla.components.support.test.libstate.ext.waitUntilIdle
 import mozilla.components.support.test.rule.MainCoroutineRule
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
-import org.junit.Ignore
 import org.junit.Rule
 import org.junit.Test
 import org.mozilla.fenix.utils.Settings
@@ -47,7 +46,6 @@ class TitleHeaderBindingTest {
         assertTrue(result)
     }
 
-    @Ignore // To be fixed with https://github.com/mozilla-mobile/fenix/issues/21360
     @Test
     fun `WHEN grouped tabs are added to the list THEN return false`() = runBlockingTest {
         var result = false
@@ -56,6 +54,7 @@ class TitleHeaderBindingTest {
         val binding = TitleHeaderBinding(store, settings) { result = it }
 
         every { settings.inactiveTabsAreEnabled } returns true
+        every { settings.searchTermTabGroupsAreEnabled } returns true
 
         binding.start()