From c2d385f14225f0e290fc9909bdfe1d2f9455a005 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Fri, 17 Sep 2021 12:38:45 -0400 Subject: [PATCH] [fenix] Issue https://github.com/mozilla-mobile/fenix/issues/21236: Fixes empty tray visibility logic This is a bug we noticed after landing search term grouping. An adapter can submit an empty list of items to the `ConcatAdapter` early. This has the side-effect of triggering our `observeFirstInsert` too soon and therefore updating the visibility to show the empty tray placeholder and never switches back. Our solution is to keep a constant observer on the adapter so we can perform the visibility check on every insert/remove. Co-authored-by: Roger Yang --- .../fenix/tabstray/TrayPagerAdapter.kt | 8 +++ .../AbstractBrowserPageViewHolder.kt | 50 +++++++++++++++++-- .../viewholders/AbstractPageViewHolder.kt | 14 ++++++ .../viewholders/SyncedTabsPageViewHolder.kt | 3 ++ .../AbstractBrowserPageViewHolderTest.kt | 2 + 5 files changed, 73 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt index 472a0a5967..74fac7abd9 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TrayPagerAdapter.kt @@ -106,6 +106,14 @@ class TrayPagerAdapter( } } + override fun onViewAttachedToWindow(holder: AbstractPageViewHolder) { + holder.attachedToWindow() + } + + override fun onViewDetachedFromWindow(holder: AbstractPageViewHolder) { + holder.detachedFromWindow() + } + override fun getItemCount(): Int = TRAY_TABS_COUNT companion object { diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolder.kt index 50cbbb65bc..e3a5db8a0b 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolder.kt @@ -13,8 +13,8 @@ import androidx.recyclerview.widget.RecyclerView import org.mozilla.fenix.R import org.mozilla.fenix.tabstray.TabsTrayInteractor import org.mozilla.fenix.tabstray.TabsTrayStore +import org.mozilla.fenix.tabstray.TrayPagerAdapter import org.mozilla.fenix.tabstray.browser.AbstractBrowserTrayList -import org.mozilla.fenix.tabstray.ext.observeFirstInsert /** * A shared view holder for browser tabs tray list. @@ -27,6 +27,9 @@ abstract class AbstractBrowserPageViewHolder( private val trayList: AbstractBrowserTrayList = itemView.findViewById(R.id.tray_list_item) private val emptyList: TextView = itemView.findViewById(R.id.tab_tray_empty_view) + private var adapterObserver: RecyclerView.AdapterDataObserver? = null + private var adapterRef: RecyclerView.Adapter? = null + abstract val emptyStringText: String init { @@ -48,14 +51,53 @@ abstract class AbstractBrowserPageViewHolder( adapter: RecyclerView.Adapter, layoutManager: RecyclerView.LayoutManager ) { - adapter.observeFirstInsert { - updateTrayVisibility(adapter.itemCount) - } + adapterRef = adapter + scrollToTab(adapter, layoutManager) + trayList.layoutManager = layoutManager trayList.adapter = adapter } + /** + * When the [RecyclerView.Adapter] is attached to the window we register a data observer to + * always check whether to call [updateTrayVisibility]. + * + * We keep a constant observer instead of using [RecyclerView.Adapter.observeFirstInsert], + * because some adapters can insert empty lists and trigger the one-shot observer too soon. + * + * See also [AbstractPageViewHolder.attachedToWindow]. + */ + override fun attachedToWindow() { + adapterRef?.let { adapter -> + adapterObserver = object : RecyclerView.AdapterDataObserver() { + override fun onItemRangeInserted(positionStart: Int, itemCount: Int) { + updateTrayVisibility(adapter.itemCount) + } + + override fun onItemRangeRemoved(positionstart: Int, itemcount: Int) { + updateTrayVisibility(adapter.itemCount) + } + } + adapterObserver?.let { + adapter.registerAdapterDataObserver(it) + } + } + } + + /** + * [RecyclerView.AdapterDataObserver]s are responsible to be unregistered when they are done, + * so we do that here when our [TrayPagerAdapter] page is detached from the window. + * + * See also [AbstractPageViewHolder.detachedFromWindow]. + */ + override fun detachedFromWindow() { + adapterObserver?.let { + adapterRef?.unregisterAdapterDataObserver(it) + adapterObserver = null + } + } + private fun updateTrayVisibility(size: Int) { if (size == 0) { trayList.visibility = GONE diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractPageViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractPageViewHolder.kt index 870b6ab321..3e922ca1e2 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractPageViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/AbstractPageViewHolder.kt @@ -15,7 +15,21 @@ abstract class AbstractPageViewHolder constructor( val containerView: View ) : RecyclerView.ViewHolder(containerView) { + /** + * Invoked when the nested [RecyclerView.Adapter] is bound to the [RecyclerView.ViewHolder]. + */ abstract fun bind( adapter: RecyclerView.Adapter ) + + /** + * Invoked when the [RecyclerView.ViewHolder] is attached from the window. This could have + * previously been bound and is now attached again. + */ + abstract fun attachedToWindow() + + /** + * Invoked when the [RecyclerView.ViewHolder] is detached from the window. + */ + abstract fun detachedFromWindow() } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt index 68c2cea3e9..c695b71109 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/viewholders/SyncedTabsPageViewHolder.kt @@ -27,6 +27,9 @@ class SyncedTabsPageViewHolder( binding.syncedTabsTrayLayout.tabsTrayStore = tabsTrayStore } + override fun detachedFromWindow() = Unit // no-op + override fun attachedToWindow() = Unit // no-op + companion object { const val LAYOUT_ID = R.layout.component_sync_tabs_tray_layout } diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolderTest.kt index 8f4e5a9e66..7bbff9acc3 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/viewholders/AbstractBrowserPageViewHolderTest.kt @@ -41,6 +41,7 @@ class AbstractBrowserPageViewHolderTest { val emptyList: TextView = itemView.findViewById(R.id.tab_tray_empty_view) viewHolder.bind(adapter) + viewHolder.attachedToWindow() adapter.updateTabs( Tabs( @@ -65,6 +66,7 @@ class AbstractBrowserPageViewHolderTest { val emptyList: TextView = itemView.findViewById(R.id.tab_tray_empty_view) viewHolder.bind(adapter) + viewHolder.attachedToWindow() adapter.updateTabs( Tabs(