Merge pull request #2 from jonalmeida/tabs_group

Issue mozilla-mobile#21236: Scroll to selected tab + various tab fixes for groupings

app/src/main/java/org/mozilla/fenix/FeatureFlags.kt

@@ -67,5 +67,5 @@ object FeatureFlags {
     /**
      * Identifies and separates the tabs list with a group containing search term tabs.
      */
-    val tabGroupFeature = Config.channel.isDebug
+    val tabGroupFeature = Config.channel.isNightlyOrDebug
 }

app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt

@@ -10,9 +10,6 @@ import android.view.ViewGroup
 import androidx.annotation.VisibleForTesting
 import androidx.recyclerview.widget.ConcatAdapter
 import androidx.recyclerview.widget.RecyclerView
-import mozilla.components.browser.state.selector.normalTabs
-import mozilla.components.browser.state.selector.privateTabs
-import mozilla.components.browser.state.selector.selectedTab
 import mozilla.components.browser.state.store.BrowserStore
 import org.mozilla.fenix.R
 import org.mozilla.fenix.sync.SyncedTabsAdapter
@@ -29,50 +26,62 @@ import org.mozilla.fenix.tabstray.viewholders.SyncedTabsPageViewHolder
 
 class TrayPagerAdapter(
     @VisibleForTesting internal val context: Context,
-    @VisibleForTesting internal val store: TabsTrayStore,
+    @VisibleForTesting internal val tabsTrayStore: TabsTrayStore,
     @VisibleForTesting internal val browserInteractor: BrowserTrayInteractor,
     @VisibleForTesting internal val navInteractor: NavigationInteractor,
     @VisibleForTesting internal val interactor: TabsTrayInteractor,
     @VisibleForTesting internal val browserStore: BrowserStore
 ) : RecyclerView.Adapter<AbstractPageViewHolder>() {
 
+    /**
+     * ⚠️ N.B: Scrolling to the selected tab depends on the order of these adapters. If you change
+     * the ordering or add/remove an adapter, please update [NormalBrowserPageViewHolder.scrollToTab] and
+     * the layout manager.
+     */
     private val normalAdapter by lazy {
         ConcatAdapter(
             InactiveTabsAdapter(context, browserInteractor, INACTIVE_TABS_FEATURE_NAME),
-            TabGroupAdapter(context, browserInteractor, store, TAB_GROUP_FEATURE_NAME),
-            TitleHeaderAdapter(context.getString(R.string.tab_tray_header_title)),
-            BrowserTabsAdapter(context, browserInteractor, store, TABS_TRAY_FEATURE_NAME)
+            TabGroupAdapter(context, browserInteractor, tabsTrayStore, TAB_GROUP_FEATURE_NAME),
+            TitleHeaderAdapter(browserStore, R.string.tab_tray_header_title),
+            BrowserTabsAdapter(context, browserInteractor, tabsTrayStore, TABS_TRAY_FEATURE_NAME)
         )
     }
-    private val privateAdapter by lazy { BrowserTabsAdapter(context, browserInteractor, store, TABS_TRAY_FEATURE_NAME) }
-    private val syncedTabsAdapter by lazy { SyncedTabsAdapter(TabClickDelegate(navInteractor)) }
+    private val privateAdapter by lazy {
+        BrowserTabsAdapter(
+            context,
+            browserInteractor,
+            tabsTrayStore,
+            TABS_TRAY_FEATURE_NAME
+        )
+    }
+    private val syncedTabsAdapter by lazy {
+        SyncedTabsAdapter(TabClickDelegate(navInteractor))
+    }
 
     override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): AbstractPageViewHolder {
         val itemView = LayoutInflater.from(parent.context).inflate(viewType, parent, false)
 
-        val selectedTab = browserStore.state.selectedTab
-
         return when (viewType) {
             NormalBrowserPageViewHolder.LAYOUT_ID -> {
                 NormalBrowserPageViewHolder(
                     itemView,
-                    store,
-                    interactor,
-                    browserStore.state.normalTabs.indexOf(selectedTab)
+                    tabsTrayStore,
+                    browserStore,
+                    interactor
                 )
             }
             PrivateBrowserPageViewHolder.LAYOUT_ID -> {
                 PrivateBrowserPageViewHolder(
                     itemView,
-                    store,
-                    interactor,
-                    browserStore.state.privateTabs.indexOf(selectedTab)
+                    tabsTrayStore,
+                    browserStore,
+                    interactor
                 )
             }
             SyncedTabsPageViewHolder.LAYOUT_ID -> {
                 SyncedTabsPageViewHolder(
                     itemView,
-                    store
+                    tabsTrayStore
                 )
             }
             else -> throw IllegalStateException("Unknown viewType.")

app/src/main/java/org/mozilla/fenix/tabstray/browser/NormalBrowserTrayList.kt

@@ -12,12 +12,13 @@ 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
 import org.mozilla.fenix.tabstray.ext.inactiveTabsAdapter
 import org.mozilla.fenix.tabstray.ext.isNormalTabActive
 import org.mozilla.fenix.tabstray.ext.isNormalTabActiveWithoutSearchTerm
 import org.mozilla.fenix.tabstray.ext.isNormalTabInactive
-import org.mozilla.fenix.tabstray.ext.isNormalTabWithSearchTerm
+import org.mozilla.fenix.tabstray.ext.isNormalTabActiveWithSearchTerm
 import org.mozilla.fenix.tabstray.ext.tabGroupAdapter
 import java.util.concurrent.TimeUnit
 
@@ -26,17 +27,17 @@ import java.util.concurrent.TimeUnit
  */
 const val DEFAULT_ACTIVE_DAYS = 4L
 
+/**
+ * The maximum time from when a tab was created or accessed until it is considered "inactive".
+ */
+val maxActiveTime = TimeUnit.DAYS.toMillis(DEFAULT_ACTIVE_DAYS)
+
 class NormalBrowserTrayList @JvmOverloads constructor(
     context: Context,
     attrs: AttributeSet? = null,
     defStyleAttr: Int = 0
 ) : AbstractBrowserTrayList(context, attrs, defStyleAttr) {
 
-    /**
-     * The maximum time from when a tab was created or accessed until it is considered "inactive".
-     */
-    private var maxActiveTime = TimeUnit.DAYS.toMillis(DEFAULT_ACTIVE_DAYS)
-
     private val concatAdapter by lazy { adapter as ConcatAdapter }
 
     override val tabsFeature by lazy {
@@ -68,7 +69,7 @@ class NormalBrowserTrayList @JvmOverloads constructor(
             if (!FeatureFlags.tabGroupFeature) {
                 return@filter false
             }
-            it.isNormalTabWithSearchTerm(maxActiveTime)
+            it.isNormalTabActiveWithSearchTerm(maxActiveTime)
         }
         val tabsAdapter = concatAdapter.tabGroupAdapter
 
@@ -123,9 +124,9 @@ class NormalBrowserTrayList @JvmOverloads constructor(
     override fun onAttachedToWindow() {
         super.onAttachedToWindow()
 
-        tabsFeature.start()
-        searchTermFeature.start()
         inactiveFeature.start()
+        searchTermFeature.start()
+        tabsFeature.start()
 
         touchHelper.attachToRecyclerView(this)
     }

app/src/main/java/org/mozilla/fenix/tabstray/browser/SelectedItemAdapterBinding.kt

@@ -4,10 +4,10 @@
 
 package org.mozilla.fenix.tabstray.browser
 
+import androidx.recyclerview.widget.RecyclerView
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.collect
-import kotlinx.coroutines.flow.drop
 import kotlinx.coroutines.flow.map
 import mozilla.components.browser.tabstray.TabsAdapter.Companion.PAYLOAD_DONT_HIGHLIGHT_SELECTED_ITEM
 import mozilla.components.browser.tabstray.TabsAdapter.Companion.PAYLOAD_HIGHLIGHT_SELECTED_ITEM
@@ -23,19 +23,21 @@ import org.mozilla.fenix.tabstray.TabsTrayStore
 @OptIn(ExperimentalCoroutinesApi::class)
 class SelectedItemAdapterBinding(
     store: TabsTrayStore,
-    val adapter: BrowserTabsAdapter
+    val adapter: RecyclerView.Adapter<out RecyclerView.ViewHolder>
 ) : AbstractBinding<TabsTrayState>(store) {
 
     override suspend fun onState(flow: Flow<TabsTrayState>) {
         flow.map { it.mode }
-            // ignore initial mode update; the adapter is already in an updated state.
-            .drop(1)
             .ifChanged()
             .collect { mode ->
                 notifyAdapter(mode)
             }
     }
 
+    /**
+     * N.B: This method should be made more performant to find the position of the multi-selected tab that has
+     * changed in the adapter, and then [RecyclerView.Adapter.notifyItemChanged].
+     */
     private fun notifyAdapter(mode: Mode) = with(adapter) {
         if (mode == Mode.Normal) {
             notifyItemRangeChanged(0, itemCount, PAYLOAD_HIGHLIGHT_SELECTED_ITEM)

app/src/main/java/org/mozilla/fenix/tabstray/browser/TabGroupAdapter.kt

@@ -10,17 +10,22 @@ import android.view.LayoutInflater
 import android.view.ViewGroup
 import androidx.recyclerview.widget.DiffUtil
 import androidx.recyclerview.widget.ListAdapter
+import androidx.recyclerview.widget.RecyclerView
 import androidx.recyclerview.widget.RecyclerView.HORIZONTAL
 import androidx.recyclerview.widget.RecyclerView.VERTICAL
 import mozilla.components.concept.tabstray.Tabs
-import mozilla.components.concept.tabstray.Tab as TabsTrayTab
 import mozilla.components.concept.tabstray.TabsTray
 import mozilla.components.support.base.observer.ObserverRegistry
 import org.mozilla.fenix.components.Components
 import org.mozilla.fenix.ext.components
+import org.mozilla.fenix.selection.SelectionHolder
 import org.mozilla.fenix.tabstray.TabsTrayStore
+import org.mozilla.fenix.tabstray.browser.TabGroupAdapter.Group
 import kotlin.math.max
-import mozilla.components.support.base.observer.Observable as ComponentObservable
+import mozilla.components.concept.tabstray.Tab as TabsTrayTab
+import mozilla.components.support.base.observer.Observable
+
+typealias TrayObservable = Observable<TabsTray.Observer>
 
 /**
  * The [ListAdapter] for displaying the list of search term tabs.
@@ -30,38 +35,74 @@ import mozilla.components.support.base.observer.Observable as ComponentObservabl
  * @param featureName [String] representing the name of the feature displaying tabs. Used in telemetry reporting.
  * @param delegate [Observable]<[TabsTray.Observer]> for observing tabs tray changes. Defaults to [ObserverRegistry].
  */
+@Suppress("TooManyFunctions")
 class TabGroupAdapter(
     private val context: Context,
     private val browserTrayInteractor: BrowserTrayInteractor,
     private val store: TabsTrayStore,
     private val featureName: String,
-    delegate: ComponentObservable<TabsTray.Observer> = ObserverRegistry()
-) : ListAdapter<TabGroupAdapter.Group, TabGroupViewHolder>(DiffCallback),
-    TabsTray,
-    ComponentObservable<TabsTray.Observer> by delegate {
+    delegate: TrayObservable = ObserverRegistry()
+) : ListAdapter<Group, TabGroupViewHolder>(DiffCallback), TabsTray, TrayObservable by delegate {
+
+    data class Group(
+        /**
+         * A title for the tab group.
+         */
+        val title: String,
+
+        /**
+         * The list of tabs belonging to this tab group.
+         */
+        val tabs: List<TabsTrayTab>,
+
+        /**
+         * The last time tabs in this group was accessed.
+         */
+        val lastAccess: Long
+    )
+
+    /**
+     * Tracks the selected tabs in multi-select mode.
+     */
+    var selectionHolder: SelectionHolder<TabsTrayTab>? = null
 
     override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): TabGroupViewHolder {
         val view = LayoutInflater.from(parent.context).inflate(viewType, parent, false)
 
         return when {
             context.components.settings.gridTabView -> {
-                TabGroupViewHolder(view, HORIZONTAL)
+                TabGroupViewHolder(view, HORIZONTAL, browserTrayInteractor, store, selectionHolder)
             }
             else -> {
-                TabGroupViewHolder(view, VERTICAL)
+                TabGroupViewHolder(view, VERTICAL, browserTrayInteractor, store, selectionHolder)
             }
         }
     }
 
     override fun onBindViewHolder(holder: TabGroupViewHolder, position: Int) {
         val group = getItem(position)
-        holder.bind(group, browserTrayInteractor, store, this)
+        holder.bind(group, this)
+    }
+
+    override fun getItemViewType(position: Int) = TabGroupViewHolder.LAYOUT_ID
+
+    /**
+     * Notify the nested [RecyclerView] when this view has been attached.
+     */
+    override fun onViewAttachedToWindow(holder: TabGroupViewHolder) {
+        holder.rebind()
     }
 
-    override fun getItemViewType(position: Int): Int {
-        return TabGroupViewHolder.LAYOUT_ID
+    /**
+     * Notify the nested [RecyclerView] when this view has been detached.
+     */
+    override fun onViewDetachedFromWindow(holder: TabGroupViewHolder) {
+        holder.unbind()
     }
 
+    /**
+     * Creates a grouping of data classes for how groupings will be structured.
+     */
     override fun updateTabs(tabs: Tabs) {
         val data = tabs.list.groupBy { it.searchTerm.lowercase() }
 
@@ -82,37 +123,21 @@ class TabGroupAdapter(
         submitList(grouping)
     }
 
-    data class Group(
-        /**
-         * A title for the tab group.
-         */
-        val title: String,
-
-        /**
-         * The list of tabs belonging to this tab group.
-         */
-        val tabs: List<TabsTrayTab>,
-
-        /**
-         * The last time tabs in this group was accessed.
-         */
-        val lastAccess: Long
-    )
-
-    override fun isTabSelected(tabs: Tabs, position: Int): Boolean =
-        tabs.selectedIndex == position
+    /**
+     * Not implemented; handled by nested [RecyclerView].
+     */
+    override fun isTabSelected(tabs: Tabs, position: Int): Boolean = false
     override fun onTabsChanged(position: Int, count: Int) = Unit
     override fun onTabsInserted(position: Int, count: Int) = Unit
     override fun onTabsMoved(fromPosition: Int, toPosition: Int) = Unit
     override fun onTabsRemoved(position: Int, count: Int) = Unit
 
     private object DiffCallback : DiffUtil.ItemCallback<Group>() {
-        override fun areItemsTheSame(oldItem: Group, newItem: Group): Boolean {
-            return oldItem.title == newItem.title
-        }
-
-        override fun areContentsTheSame(oldItem: Group, newItem: Group): Boolean {
-            return oldItem == newItem
-        }
+        override fun areItemsTheSame(oldItem: Group, newItem: Group) = oldItem.title == newItem.title
+        override fun areContentsTheSame(oldItem: Group, newItem: Group) = oldItem == newItem
     }
 }
+
+internal fun Group.containsTabId(tabId: String): Boolean {
+    return tabs.firstOrNull { it.id == tabId } != null
+}