Fix ANR and view pool resolution in nested group (#1101)

* Fix infinite loop when no RV found in ancestors * Use the correct parent for the sub models, one of sub models could be another group * Add nested group in test coverage Co-authored-by: Emmanuel Boudrant <[email protected]>
airbnb · Dec 2, 2020 · 12b4cda · 12b4cda
1 parent 860e049
commit 12b4cda
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 9 deletions.
diff --git a/epoxy-adapter/src/main/java/com/airbnb/epoxy/ModelGroupHolder.kt b/epoxy-adapter/src/main/java/com/airbnb/epoxy/ModelGroupHolder.kt
@@ -9,12 +9,12 @@ import androidx.recyclerview.widget.RecyclerView
 import com.airbnb.viewmodeladapter.R
 import java.util.ArrayList
 
-class ModelGroupHolder(parent: ViewParent) : EpoxyHolder() {
+class ModelGroupHolder(private val modelGroupParent: ViewParent) : EpoxyHolder() {
     val viewHolders = ArrayList<EpoxyViewHolder>(4)
 
     /** Use parent pool or create a local pool */
     @VisibleForTesting
-    val viewPool = findViewPool(parent) ?: LocalGroupRecycledViewPool()
+    val viewPool = findViewPool(modelGroupParent)
 
     /**
      * Get the root view group (aka
@@ -150,6 +150,7 @@ class ModelGroupHolder(parent: ViewParent) : EpoxyHolder() {
 
         return recycledView as? EpoxyViewHolder
             ?: HELPER_ADAPTER.createViewHolder(
+                modelGroupParent,
                 model,
                 parent,
                 viewType
@@ -185,7 +186,7 @@ class ModelGroupHolder(parent: ViewParent) : EpoxyHolder() {
 
         private val HELPER_ADAPTER = HelperAdapter()
 
-        private fun findViewPool(view: ViewParent): RecyclerView.RecycledViewPool? {
+        private fun findViewPool(view: ViewParent): RecyclerView.RecycledViewPool {
             var viewPool: RecyclerView.RecycledViewPool? = null
             while (viewPool == null) {
                 viewPool = if (view is RecyclerView) {
@@ -196,7 +197,7 @@ class ModelGroupHolder(parent: ViewParent) : EpoxyHolder() {
                         findViewPool(parent)
                     } else {
                         // This model group is is not in a RecyclerView
-                        null
+                        LocalGroupRecycledViewPool()
                     }
                 }
             }
@@ -251,16 +252,24 @@ private class LocalGroupRecycledViewPool : RecyclerView.RecycledViewPool()
 private class HelperAdapter : RecyclerView.Adapter<EpoxyViewHolder>() {
 
     private var model: EpoxyModel<*>? = null
-
-    fun createViewHolder(model: EpoxyModel<*>, parent: ViewGroup, viewType: Int): EpoxyViewHolder {
+    private var modelGroupParent: ViewParent? = null
+
+    fun createViewHolder(
+        modelGroupParent: ViewParent,
+        model: EpoxyModel<*>,
+        parent: ViewGroup,
+        viewType: Int
+    ): EpoxyViewHolder {
         this.model = model
+        this.modelGroupParent = modelGroupParent
         val viewHolder = createViewHolder(parent, viewType)
         this.model = null
+        this.modelGroupParent = null
         return viewHolder
     }
 
     override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): EpoxyViewHolder {
-        return EpoxyViewHolder(parent, model!!.buildView(parent), model!!.shouldSaveViewState())
+        return EpoxyViewHolder(modelGroupParent, model!!.buildView(parent), model!!.shouldSaveViewState())
     }
 
     override fun onBindViewHolder(holder: EpoxyViewHolder, position: Int) {

diff --git a/epoxy-integrationtest/src/test/java/com/airbnb/epoxy/EpoxyModelGroupRecyclingTest.kt b/epoxy-integrationtest/src/test/java/com/airbnb/epoxy/EpoxyModelGroupRecyclingTest.kt
@@ -50,6 +50,18 @@ class EpoxyModelGroupRecyclingTest {
                     id("1")
                     layout(R.layout.vertical_linear_group)
                     onBind(assertOnModelBound)
+
+                    group {
+                        id("1.1")
+                        layout(R.layout.vertical_linear_group)
+                        onBind(assertOnModelBound)
+                    }
+
+                    group {
+                        id("1.1.1")
+                        layout(R.layout.vertical_linear_group)
+                        onBind(assertOnModelBound)
+                    }
                 }
             },
             buildModels2 = {
@@ -60,7 +72,7 @@ class EpoxyModelGroupRecyclingTest {
                 }
             }
         )
-        Assert.assertEquals(2, modelsBound)
+        Assert.assertEquals(4, modelsBound)
     }
 
     /**
@@ -88,6 +100,18 @@ class EpoxyModelGroupRecyclingTest {
                     id("1")
                     layout(R.layout.vertical_linear_group)
                     onBind(assertOnModelBound1)
+
+                    group {
+                        id("1.1")
+                        layout(R.layout.vertical_linear_group)
+                        onBind(assertOnModelBound1)
+
+                        group {
+                            id("1.1.1")
+                            layout(R.layout.vertical_linear_group)
+                            onBind(assertOnModelBound1)
+                        }
+                    }
                 }
             },
             buildModels2 = {
@@ -98,7 +122,7 @@ class EpoxyModelGroupRecyclingTest {
                 }
             }
         )
-        Assert.assertEquals("2 models should have been bound", 2, modelsBound)
+        Assert.assertEquals(4, modelsBound)
     }
 
     /** Sets the models in the [recyclerView1] and [recyclerView2]. */