Skip to content

Commit

Permalink
Issue mozilla-mobile#21236: Fixes empty tray visibility logic
Browse files Browse the repository at this point in the history
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 <royang@mozilla.com>
  • Loading branch information
jonalmeida and rocketsroger committed Sep 17, 2021
1 parent 6ac10d5 commit 2c84385
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 4 deletions.
Expand Up @@ -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 {
Expand Down
Expand Up @@ -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.
Expand All @@ -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<out RecyclerView.ViewHolder>? = null

abstract val emptyStringText: String

init {
Expand All @@ -48,14 +51,53 @@ abstract class AbstractBrowserPageViewHolder(
adapter: RecyclerView.Adapter<out RecyclerView.ViewHolder>,
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
Expand Down
Expand Up @@ -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<out RecyclerView.ViewHolder>
)

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

0 comments on commit 2c84385

Please sign in to comment.