Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

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

Merged
merged 17 commits into from Oct 2, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -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
Expand Down Expand Up @@ -78,7 +77,7 @@ fun RecentTabs(
)
}
is RecentTab.SearchGroup -> {
if (FeatureFlags.tabGroupFeature) {
if (components.settings.searchTermTabGroupsAreEnabled) {
RecentSearchGroupItem(
searchTerm = tab.searchTerm,
tabId = tab.tabId,
Expand Down
Expand Up @@ -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)
Expand All @@ -58,6 +59,10 @@ 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 {
Copy link
Contributor

@csadilek csadilek Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MozillaNoah We have to make sure we only show this preference when the feature flag is turned on. Did you verify this? I think this needs a it.visible = FeatureFlags.tabGroupFeature?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I did it for the inactive tabs toggle but not here. Will update shortly.

it.isChecked = it.context.settings().searchTermTabGroupsAreEnabled
it.onPreferenceChangeListener = SharedPreferenceUpdater()
}

radioManual = requirePreference(R.string.pref_key_close_tabs_manually)
radioOneDay = requirePreference(R.string.pref_key_close_tabs_after_one_day)
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
Expand Up @@ -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)
Expand Down
Expand Up @@ -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

/**
Expand Down Expand Up @@ -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
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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.
Expand All @@ -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) {

Expand Down
9 changes: 9 additions & 0 deletions app/src/main/java/org/mozilla/fenix/utils/Settings.kt
Expand Up @@ -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()

Expand Down
10 changes: 10 additions & 0 deletions app/src/main/res/drawable-night/ic_all_tabs.xml
@@ -0,0 +1,10 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for adding this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also include the license

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I don't think my original icon have it either. Since we need this change in, @MozillaNoah can you please create another PR to add license on both ic_all_tabs? Thanks

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>
1 change: 1 addition & 0 deletions app/src/main/res/values/preference_keys.xml
Expand Up @@ -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>

Expand Down
4 changes: 4 additions & 0 deletions app/src/main/res/values/strings.xml
Expand Up @@ -639,6 +639,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 -->
Expand Down
6 changes: 6 additions & 0 deletions app/src/main/res/xml/tabs_preferences.xml
Expand Up @@ -18,6 +18,12 @@
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" />
</androidx.preference.PreferenceCategory>

<androidx.preference.PreferenceCategory
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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()

Expand Down