From 6f8e85ac4e61f95c6c8aaf1e66188637ff0ca64f Mon Sep 17 00:00:00 2001 From: Eli Hart Date: Tue, 5 Feb 2019 08:46:41 -1000 Subject: [PATCH] Fix model group issues (#676) --- .../com/airbnb/epoxy/EpoxyModelGroup.java | 13 ++-- .../java/com/airbnb/epoxy/ModelGroupHolder.kt | 51 ++++++------ .../com/airbnb/epoxy/EpoxyModelGroupTest.kt | 78 ++++++++++++++++--- 3 files changed, 98 insertions(+), 44 deletions(-) diff --git a/epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java b/epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java index 53325c14ab..17fad17aef 100644 --- a/epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java +++ b/epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyModelGroup.java @@ -141,12 +141,15 @@ public void bind(@NonNull ModelGroupHolder holder, @NonNull EpoxyModel previo public void onModel(EpoxyModel model, EpoxyViewHolder viewHolder, int modelIndex) { setViewVisibility(model, viewHolder); - EpoxyModel previousModel = previousGroup.models.get(modelIndex); - if (previousModel.id() == model.id()) { - viewHolder.bind(model, previousModel, Collections.emptyList(), modelIndex); - } else { - viewHolder.bind(model, null, Collections.emptyList(), modelIndex); + if (modelIndex < previousGroup.models.size()) { + EpoxyModel previousModel = previousGroup.models.get(modelIndex); + if (previousModel.id() == model.id()) { + viewHolder.bind(model, previousModel, Collections.emptyList(), modelIndex); + return; + } } + + viewHolder.bind(model, null, Collections.emptyList(), modelIndex); } }); } diff --git a/epoxy-adapter/src/main/java/com/airbnb/epoxy/ModelGroupHolder.kt b/epoxy-adapter/src/main/java/com/airbnb/epoxy/ModelGroupHolder.kt index b2b32e1ef8..fc9431bfd6 100644 --- a/epoxy-adapter/src/main/java/com/airbnb/epoxy/ModelGroupHolder.kt +++ b/epoxy-adapter/src/main/java/com/airbnb/epoxy/ModelGroupHolder.kt @@ -58,40 +58,31 @@ class ModelGroupHolder : EpoxyHolder() { } private fun createViewStubData(viewGroup: ViewGroup): List { - val stubs = ArrayList(4) + return ArrayList(4).apply { - while (true) { - val stubData = getNextViewStub(viewGroup) - if (stubData != null) { - stubs.add(stubData) - stubData.removeView() - } else { - break - } - } + collectViewStubs(viewGroup, this) - if (stubs.isEmpty()) { - throw IllegalStateException( - "No view stubs found. If viewgroup is not empty it must contain ViewStubs." - ) + if (isEmpty()) { + throw IllegalStateException( + "No view stubs found. If viewgroup is not empty it must contain ViewStubs." + ) + } } - - return stubs } - private fun getNextViewStub(viewGroup: ViewGroup): ViewStubData? { - val childCount = viewGroup.childCount - for (i in 0 until childCount) { + private fun collectViewStubs( + viewGroup: ViewGroup, + stubs: ArrayList + ) { + for (i in 0 until viewGroup.childCount) { val child = viewGroup.getChildAt(i) if (child is ViewGroup) { - getNextViewStub(child)?.let { return it } + collectViewStubs(child, stubs) } else if (child is ViewStub) { - return ViewStubData(viewGroup, child, i) + stubs.add(ViewStubData(viewGroup, child, i)) } } - - return null } fun bindGroupIfNeeded(group: EpoxyModelGroup) { @@ -116,8 +107,7 @@ class ModelGroupHolder : EpoxyHolder() { if (usingStubs() && stubs.size < modelCount) { throw IllegalStateException( - "Insufficient view stubs for EpoxyModelGroup. " + modelCount + - " models were provided but only " + stubs.size + " view stubs exist." + "Insufficient view stubs for EpoxyModelGroup. $modelCount models were provided but only ${stubs.size} view stubs exist." ) } viewHolders.ensureCapacity(modelCount) @@ -180,7 +170,7 @@ class ModelGroupHolder : EpoxyHolder() { private fun removeAndRecycleView(modelPosition: Int) { if (usingStubs()) { - stubs[modelPosition].removeView() + stubs[modelPosition].resetStub() } else { childContainer.removeViewAt(modelPosition) } @@ -204,6 +194,8 @@ private class ViewStubData( ) { fun setView(view: View, useStubLayoutParams: Boolean) { + removeCurrentView() + // Carry over the stub id manually since we aren't inflating via the stub val inflatedId = viewStub.inflatedId if (inflatedId != View.NO_ID) { @@ -217,7 +209,12 @@ private class ViewStubData( } } - fun removeView() { + fun resetStub() { + removeCurrentView() + viewGroup.addView(viewStub, position) + } + + private fun removeCurrentView() { val view = viewGroup.getChildAt(position) ?: throw IllegalStateException("No view exists at position $position") viewGroup.removeView(view) diff --git a/epoxy-adapter/src/test/java/com/airbnb/epoxy/EpoxyModelGroupTest.kt b/epoxy-adapter/src/test/java/com/airbnb/epoxy/EpoxyModelGroupTest.kt index a91a00cafe..fa6479dfcb 100644 --- a/epoxy-adapter/src/test/java/com/airbnb/epoxy/EpoxyModelGroupTest.kt +++ b/epoxy-adapter/src/test/java/com/airbnb/epoxy/EpoxyModelGroupTest.kt @@ -38,11 +38,11 @@ class EpoxyModelGroupTest(val useViewStubs: Boolean) { assertEquals(0, modelGroupHolder.viewHolders.size) } - private fun bind(modelGroup: EpoxyModelGroup) { + private fun bind(modelGroup: EpoxyModelGroup, previousGroup: EpoxyModelGroup? = null) { if (topLevelHolder == null) { topLevelHolder = EpoxyViewHolder(modelGroup.buildView(recyclerView), false) } - topLevelHolder!!.bind(modelGroup, null, emptyList(), 0) + topLevelHolder!!.bind(modelGroup, previousGroup, emptyList(), 0) } private fun assertModelsBound(modelGroup: EpoxyModelGroup) { @@ -62,47 +62,53 @@ class EpoxyModelGroupTest(val useViewStubs: Boolean) { @Test fun bind_Unbind_Rebind_LinearLayoutWithLessModels() { - bind(createFrameLayoutGroup(3)) + val firstGroup = createFrameLayoutGroup(3) + bind(firstGroup) unbind() + createSpaceGroup(2).let { - bind(it) + bind(it, firstGroup) assertModelsBound(it) } } @Test fun bind_Unbind_Rebind_LinearLayoutWithMoreModels() { - bind(createFrameLayoutGroup(3)) + val firstGroup = createFrameLayoutGroup(3) + bind(firstGroup) unbind() createSpaceGroup(4).let { - bind(it) + bind(it, firstGroup) assertModelsBound(it) } } @Test fun rebind_LinearLayoutWithSameViewTypes() { - bind(createFrameLayoutGroup(3)) + val firstGroup = createFrameLayoutGroup(3) + bind(firstGroup) createFrameLayoutGroup(4).let { - bind(it) + bind(it, firstGroup) assertModelsBound(it) } } @Test fun rebind_LinearLayoutWithMoreModels() { - bind(createFrameLayoutGroup(3)) + val firstGroup = createFrameLayoutGroup(3) + bind(firstGroup) createSpaceGroup(4).let { - bind(it) + bind(it, firstGroup) assertModelsBound(it) } } @Test fun rebind_LinearLayoutWithLessModels() { - bind(createFrameLayoutGroup(3)) + val firstGroup = createFrameLayoutGroup(3) + bind(firstGroup) createSpaceGroup(2).let { - bind(it) + bind(it, firstGroup) assertModelsBound(it) } } @@ -120,6 +126,54 @@ class EpoxyModelGroupTest(val useViewStubs: Boolean) { assertEquals(firstHolders, secondHolders) } + @Test + fun viewStubsOutOfOrder() { + val models = (0 until 4).map { NestedModelFrameLayout().id(it) } + + val modelGroup = object : EpoxyModelGroup(0, models) { + public override fun buildView(parent: ViewGroup): View { + return LinearLayout(parent.context).apply { + + addView(ViewStub(parent.context).apply { + inflatedId = 0 + }) + + addView(LinearLayout(parent.context).apply { + addView(ViewStub(parent.context).apply { + inflatedId = 1 + }) + + addView(Space(parent.context)) + + addView(ViewStub(parent.context).apply { + inflatedId = 2 + }) + }) + + addView(ViewStub(parent.context).apply { + inflatedId = 3 + }) + } + } + } + + bind(modelGroup) + + modelGroupHolder.viewHolders.forEachIndexed { index, viewholder -> + + val view = viewholder.itemView + assertEquals(index, view.id) + + val indexInsideParentView = (view.parent as ViewGroup).indexOfChild(view) + when (view.id) { + 0 -> assertEquals(0, indexInsideParentView) + 1 -> assertEquals(0, indexInsideParentView) + 2 -> assertEquals(2, indexInsideParentView) + 3 -> assertEquals(2, indexInsideParentView) + } + } + } + private fun createFrameLayoutGroup(modelCount: Int): EpoxyModelGroup { val models = (0 until modelCount).map { NestedModelFrameLayout().id(it) } return if (useViewStubs) ViewStubsGroupModel(models) else LinerLayoutGroupModel(models)