Ensure adapter position APIs work in ConcatAdapter for recycled VHs
This CL fixes a bug in ConcatAdapter where it was changing its internal
structure before calling onViewRecycled / onFailedToRecycle APIs. This
breaks documented API in onViewRecycled / onFailedToRecycle APIs where
it should be able to retrieve adapter position if the item is not
removed from the adapter.
I've moved callbacks before changing internal structure to fix the
issue.
Bug: 187339376
Test: ConcatAdapterTest
Change-Id: If5c9c182abed6d85df3b35e86ef79beed497c901
diff --git a/recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/ConcatAdapterTest.kt b/recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/ConcatAdapterTest.kt
index 38f728d..8c12195 100644
--- a/recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/ConcatAdapterTest.kt
+++ b/recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/ConcatAdapterTest.kt
@@ -146,9 +146,9 @@
adapter1.removeItems(3, 2)
assertThat(recyclerView.isLayoutRequested).isTrue()
measureAndLayout(100, 100)
- assertThat(adapter1.recycledViewHolders()).hasSize(2)
+ assertThat(adapter1.recycleEvents()).hasSize(2)
assertThat(adapter1.attachedViewHolders()).hasSize(8)
- assertThat(adapter1.attachedViewHolders()).containsNoneIn(adapter1.recycledViewHolders())
+ assertThat(adapter1.attachedViewHolders()).containsNoneIn(adapter1.recycleEvents())
}
@UiThreadTest
@@ -163,7 +163,7 @@
assertThat(recyclerView.isLayoutRequested).isTrue()
measureAndLayout(100, 100)
assertThat(adapter1.attachedViewHolders()).hasSize(3)
- assertThat(adapter1.recycledViewHolders()).hasSize(0)
+ assertThat(adapter1.recycleEvents()).hasSize(0)
}
@UiThreadTest
@@ -187,8 +187,14 @@
assertThat(recyclerView.isLayoutRequested).isTrue()
measureAndLayout(100, 200)
assertThat(adapter2.attachedViewHolders()).hasSize(3)
- assertThat(adapter2.failedToRecycleViewHolders()).contains(viewHolder)
- assertThat(adapter2.failedToRecycleViewHolders()).hasSize(1)
+ assertThat(adapter2.failedToRecycleEvents()).contains(
+ RecycledViewHolderEvent(
+ itemId = 12,
+ absoluteAdapterPosition = NO_POSITION,
+ bindingAdapterPosition = NO_POSITION
+ )
+ )
+ assertThat(adapter2.failedToRecycleEvents()).hasSize(1)
assertThat(adapter2.attachedViewHolders()).doesNotContain(viewHolder)
}
@@ -324,6 +330,252 @@
@UiThreadTest
@Test
+ public fun scrollView() {
+ val adapter1 = NestedTestAdapter(
+ count = 10,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 40)
+ }
+ )
+ val adapter2 = NestedTestAdapter(
+ count = 5,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 10)
+ }
+ )
+ val concatenated =
+ ConcatAdapter(adapter1, adapter2)
+ recyclerView.adapter = concatenated
+ measureAndLayout(100, 100)
+ recyclerView.scrollBy(0, 90)
+ assertThat(adapter1.attachedViewHolders()).hasSize(3)
+ assertThat(
+ adapter1.attachedViewHolders().map { it.bindingAdapterPosition }
+ ).containsExactly(2, 3, 4)
+ }
+
+ @UiThreadTest
+ @Test
+ public fun recycledViewPositions() {
+ val adapter1 = NestedTestAdapter(
+ count = 5,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 40)
+ }
+ )
+ val adapter2 = NestedTestAdapter(
+ count = 10,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 10)
+ }
+ )
+ val concatenated = ConcatAdapter(adapter1, adapter2)
+ recyclerView.setItemViewCacheSize(0)
+ recyclerView.adapter = concatenated
+ measureAndLayout(100, 100)
+ // trigger recycle
+ recyclerView.scrollBy(0, 90)
+ // two views are recycled
+ assertThat(adapter1.recycleEvents()).containsExactly(
+ RecycledViewHolderEvent(
+ itemId = 0,
+ absoluteAdapterPosition = 0,
+ bindingAdapterPosition = 0
+ ),
+ RecycledViewHolderEvent(
+ itemId = 1,
+ absoluteAdapterPosition = 1,
+ bindingAdapterPosition = 1
+ )
+ ).inOrder()
+ }
+
+ @UiThreadTest
+ @Test
+ @SdkSuppress(minSdkVersion = 16)
+ public fun recycledViewPositions_failedRecycle() {
+ val adapter1 = NestedTestAdapter(
+ count = 5,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 40)
+ }
+ )
+ val adapter2 = NestedTestAdapter(
+ count = 10,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 10)
+ }
+ )
+ val concatenated = ConcatAdapter(adapter1, adapter2)
+ recyclerView.setItemViewCacheSize(0)
+ recyclerView.adapter = concatenated
+ measureAndLayout(100, 100)
+ // give second view transient state
+ val viewHolder = recyclerView.findViewHolderForAdapterPosition(1)
+ check(viewHolder != null) {
+ "should have that view holder for position 1"
+ }
+ // give it transient state so that it won't be recycled
+ viewHolder.itemView.setHasTransientState(true)
+ // trigger recycle
+ recyclerView.scrollBy(0, 90)
+ // two views are recycled but one of them fails to recycle
+ assertThat(adapter1.failedToRecycleEvents()).containsExactly(
+ RecycledViewHolderEvent(
+ itemId = 1,
+ absoluteAdapterPosition = 1,
+ bindingAdapterPosition = 1
+ )
+ )
+ assertThat(adapter1.recycleEvents()).containsExactly(
+ RecycledViewHolderEvent(
+ itemId = 0,
+ absoluteAdapterPosition = 0,
+ bindingAdapterPosition = 0
+ )
+ )
+ }
+
+ @UiThreadTest
+ @Test
+ public fun recycledViewPositions_withAdapterChanges() {
+ val adapter1 = NestedTestAdapter(
+ count = 5,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 20)
+ }
+ )
+ val adapter2 = NestedTestAdapter(
+ count = 10,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 10)
+ }
+ )
+ val concatenated =
+ ConcatAdapter(adapter1, adapter2)
+ recyclerView.setItemViewCacheSize(0)
+ recyclerView.adapter = concatenated
+ val layoutManager = (recyclerView.layoutManager!! as LinearLayoutManager)
+ measureAndLayout(100, 100)
+ // remove items 3 and 1
+ adapter1.removeItems(3, 1)
+ adapter1.removeItems(1, 1)
+
+ // scroll to the beginning of the second adapter to trigger recycle of all items in adapter
+ // 1
+ layoutManager.scrollToPositionWithOffset(3, 0)
+ measureAndLayout(100, 100)
+ assertThat(adapter1.recycleEvents()).containsExactly(
+ RecycledViewHolderEvent(
+ itemId = 0,
+ bindingAdapterPosition = 0,
+ absoluteAdapterPosition = 0
+ ),
+ RecycledViewHolderEvent(
+ itemId = 1,
+ bindingAdapterPosition = NO_POSITION,
+ absoluteAdapterPosition = NO_POSITION
+ ),
+ RecycledViewHolderEvent(
+ itemId = 2,
+ bindingAdapterPosition = 1,
+ absoluteAdapterPosition = 1
+ ),
+ RecycledViewHolderEvent(
+ itemId = 3,
+ bindingAdapterPosition = NO_POSITION,
+ absoluteAdapterPosition = NO_POSITION
+ ),
+ RecycledViewHolderEvent(
+ itemId = 4,
+ bindingAdapterPosition = 2,
+ absoluteAdapterPosition = 2
+ )
+ )
+ }
+
+ @UiThreadTest
+ @Test
+ public fun recycledViewPositions_withAdapterChanges_secondAdapter() {
+ val adapter1 = NestedTestAdapter(
+ count = 5,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 20)
+ }
+ )
+ val adapter2 = NestedTestAdapter(
+ count = 10,
+ getLayoutParams = {
+ LayoutParams(MATCH_PARENT, 10)
+ }
+ )
+ val concatenated =
+ ConcatAdapter(adapter1, adapter2)
+ recyclerView.setItemViewCacheSize(0)
+ val layoutManager = (recyclerView.layoutManager!! as LinearLayoutManager)
+
+ recyclerView.adapter = concatenated
+ // start from the second adapter
+ layoutManager.scrollToPositionWithOffset(adapter1.itemCount, 0)
+ measureAndLayout(100, 100)
+ assertThat(adapter1.attachedViewHolders()).isEmpty()
+ assertThat(adapter2.attachedViewHolders()).hasSize(10)
+ // remove items 3 and 1 from first adapter
+ adapter1.removeItems(3, 1)
+ adapter1.removeItems(1, 1)
+ // remove items 2, 4 from the second adapter
+ adapter2.removeItems(4, 1)
+ adapter2.removeItems(2, 1)
+ // scroll to the top of the list
+ layoutManager.scrollToPositionWithOffset(0, 0)
+ measureAndLayout(100, 100)
+ assertThat(adapter1.attachedViewHolders()).hasSize(3)
+ assertThat(adapter2.attachedViewHolders()).hasSize(4)
+ // the UI will have (item ids)
+ // 0, 2, 4, 5, 6
+ // nothing is recycled in adapter 1
+ assertThat(adapter1.recycleEvents()).isEmpty()
+ // all items but first two are recycled in adapter2
+ assertThat(adapter2.recycleEvents()).containsExactly(
+ // first two items, 5 and 6, are not recycled as they are still visible
+ // items 7 and 9 are recycled (removed)
+ // items 8, 10 are still visible
+ RecycledViewHolderEvent(
+ itemId = 7,
+ bindingAdapterPosition = NO_POSITION,
+ absoluteAdapterPosition = NO_POSITION
+ ),
+ // pos 4 (item id 9) is removed from adapter 2
+ RecycledViewHolderEvent(
+ itemId = 9,
+ bindingAdapterPosition = NO_POSITION,
+ absoluteAdapterPosition = NO_POSITION
+ ),
+ RecycledViewHolderEvent(
+ itemId = 11,
+ bindingAdapterPosition = 4,
+ absoluteAdapterPosition = 7
+ ),
+ RecycledViewHolderEvent(
+ itemId = 12,
+ bindingAdapterPosition = 5,
+ absoluteAdapterPosition = 8
+ ),
+ RecycledViewHolderEvent(
+ itemId = 13,
+ bindingAdapterPosition = 6,
+ absoluteAdapterPosition = 9
+ ),
+ RecycledViewHolderEvent(
+ itemId = 14,
+ bindingAdapterPosition = 7,
+ absoluteAdapterPosition = 10
+ ),
+ )
+ }
+
+ @UiThreadTest
+ @Test
fun attachDetachTest_multipleRecyclerViews() {
val recyclerView2 = RecyclerView(ApplicationProvider.getApplicationContext())
val adapter1 = NestedTestAdapter(10)
@@ -1199,8 +1451,8 @@
val itemTypeLookup: ((TestItem, position: Int) -> Int)? = null
) : RecyclerView.Adapter<ConcatAdapterViewHolder>() {
private val attachedViewHolders = mutableListOf<ConcatAdapterViewHolder>()
- private val recycledViewHolders = mutableListOf<ConcatAdapterViewHolder>()
- private val failedToRecycleViewHolders = mutableListOf<ConcatAdapterViewHolder>()
+ private val recycledViewHolderEvents = mutableListOf<RecycledViewHolderEvent>()
+ private val failedToRecycleEvents = mutableListOf<RecycledViewHolderEvent>()
private var attachedRecyclerViews = mutableListOf<RecyclerView>()
private var observers = mutableListOf<RecyclerView.AdapterDataObserver>()
@@ -1219,6 +1471,7 @@
override fun onViewDetachedFromWindow(holder: ConcatAdapterViewHolder) {
assertThat(attachedViewHolders).contains(holder)
+ holder.onDetached()
attachedViewHolders.remove(holder)
}
@@ -1321,30 +1574,30 @@
}
override fun onViewRecycled(holder: ConcatAdapterViewHolder) {
- recycledViewHolders.add(holder)
+ recycledViewHolderEvents.add(RecycledViewHolderEvent(holder))
holder.onRecycled()
}
override fun getItemCount() = items.size
override fun onFailedToRecycleView(holder: ConcatAdapterViewHolder): Boolean {
- failedToRecycleViewHolders.add(holder)
+ failedToRecycleEvents.add(RecycledViewHolderEvent(holder))
return super.onFailedToRecycleView(holder)
}
fun getItemAt(localPosition: Int) = items[localPosition]
- fun recycledViewHolders(): List<ConcatAdapterViewHolder> = recycledViewHolders
- fun failedToRecycleViewHolders(): List<ConcatAdapterViewHolder> = failedToRecycleViewHolders
+ fun recycleEvents(): List<RecycledViewHolderEvent> = recycledViewHolderEvents
+ fun failedToRecycleEvents(): List<RecycledViewHolderEvent> = failedToRecycleEvents
}
- class ConcatAdapterViewHolder(
+ internal class ConcatAdapterViewHolder(
context: Context,
val localViewType: Int
) : RecyclerView.ViewHolder(View(context)) {
- private var boundItem: Any? = null
+ private var boundItem: TestItem? = null
private var boundAdapter: RecyclerView.Adapter<*>? = null
private var boundPosition: Int? = null
- fun bindTo(adapter: RecyclerView.Adapter<*>, item: Any, position: Int) {
+ fun bindTo(adapter: RecyclerView.Adapter<*>, item: TestItem, position: Int) {
boundAdapter = adapter
boundPosition = position
boundItem = item
@@ -1353,11 +1606,30 @@
fun boundItem() = boundItem
fun boundLocalPosition() = boundPosition
fun boundAdapter() = boundAdapter
+ fun onDetached() {
+ assertPosition()
+ }
fun onRecycled() {
+ assertPosition()
boundItem = null
boundPosition = -1
boundAdapter = null
}
+
+ private fun assertPosition() {
+ val shouldHavePosition = !isRemoved() && isBound() &&
+ !isAdapterPositionUnknown() && !isInvalid()
+ assertWithMessage(
+ "binding adapter position $this"
+ ).that(shouldHavePosition).isEqualTo(
+ bindingAdapterPosition != NO_POSITION
+ )
+ assertWithMessage(
+ "binding adapter position $this"
+ ).that(shouldHavePosition).isEqualTo(
+ absoluteAdapterPosition != NO_POSITION
+ )
+ }
}
class LoggingAdapterObserver(
@@ -1472,4 +1744,16 @@
val value: Int,
val viewType: Int = 0
)
+
+ internal data class RecycledViewHolderEvent(
+ val itemId: Int?,
+ val absoluteAdapterPosition: Int,
+ val bindingAdapterPosition: Int,
+ ) {
+ constructor(viewHolder: ConcatAdapterViewHolder) : this(
+ itemId = viewHolder.boundItem()?.id,
+ absoluteAdapterPosition = viewHolder.absoluteAdapterPosition,
+ bindingAdapterPosition = viewHolder.bindingAdapterPosition
+ )
+ }
}
\ No newline at end of file
diff --git a/recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/ConcatAdapterController.java b/recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/ConcatAdapterController.java
index b30e6e9..1d20c06 100644
--- a/recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/ConcatAdapterController.java
+++ b/recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/ConcatAdapterController.java
@@ -389,21 +389,24 @@
}
public void onViewRecycled(ViewHolder holder) {
- NestedAdapterWrapper wrapper = mBinderLookup.remove(holder);
+ NestedAdapterWrapper wrapper = mBinderLookup.get(holder);
if (wrapper == null) {
throw new IllegalStateException("Cannot find wrapper for " + holder
+ ", seems like it is not bound by this adapter: " + this);
}
wrapper.adapter.onViewRecycled(holder);
+ mBinderLookup.remove(holder);
}
public boolean onFailedToRecycleView(ViewHolder holder) {
- NestedAdapterWrapper wrapper = mBinderLookup.remove(holder);
+ NestedAdapterWrapper wrapper = mBinderLookup.get(holder);
if (wrapper == null) {
throw new IllegalStateException("Cannot find wrapper for " + holder
+ ", seems like it is not bound by this adapter: " + this);
}
- return wrapper.adapter.onFailedToRecycleView(holder);
+ final boolean result = wrapper.adapter.onFailedToRecycleView(holder);
+ mBinderLookup.remove(holder);
+ return result;
}
@NonNull