Merge pull request #211 from rubensousa/grid_crash

Invalidate span cache to prevent infinite layout loops after structure changes
rubensousa · May 30, 2024 · ef2a670 · ef2a670
2 parents 88ecc57 + 664f799
commit ef2a670
Show file tree

Hide file tree

Showing 9 changed files with 142 additions and 11 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -2,6 +2,24 @@
 
 ## Version 1.3.0
 
+### 1.3.0-alpha03
+
+2024-05-31
+
+#### New Features
+
+- Added `DpadStateRegistry` that assists in saving and restoring the scroll state or view state of ViewHolders ([#45](https://github.com/rubensousa/DpadRecyclerView/issues/45))
+
+#### Improvements
+
+- Disable layout passes during scroll events by default. This is an attempt to fix ([#207](https://github.com/rubensousa/DpadRecyclerView/issues/207))
+  To fallback to the previous behavior, please use `setLayoutWhileScrollingEnabled(true)`
+
+#### Bug fixes
+
+- Fixed issue with grid layouts with different spans after item removals. ([#210](https://github.com/rubensousa/DpadRecyclerView/issues/210))
+- Fixed `DpadRecyclerView` losing focus in some cases when adapter contents are updated during scroll events. ([#206](https://github.com/rubensousa/DpadRecyclerView/issues/206))
+
 ### 1.3.0-alpha02
 
 2024-03-23

diff --git a/.../src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/AbstractTestAdapter.kt b/.../src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/AbstractTestAdapter.kt
@@ -140,6 +140,6 @@ abstract class AbstractTestAdapter<VH : RecyclerView.ViewHolder>(
 
     override fun getItemCount(): Int = items.size
 
-    protected fun getItem(position: Int) = items[position]
+    fun getItem(position: Int) = items[position]
 
 }
diff --git a/...est/kotlin/com/rubensousa/dpadrecyclerview/test/tests/mutation/GridAdapterMutationTest.kt b/...est/kotlin/com/rubensousa/dpadrecyclerview/test/tests/mutation/GridAdapterMutationTest.kt
@@ -0,0 +1,96 @@
+/*
+ * Copyright 2024 Rúben Sousa
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.rubensousa.dpadrecyclerview.test.tests.mutation
+
+import androidx.recyclerview.widget.RecyclerView
+import com.google.common.truth.Truth.assertThat
+import com.rubensousa.dpadrecyclerview.ChildAlignment
+import com.rubensousa.dpadrecyclerview.DpadSpanSizeLookup
+import com.rubensousa.dpadrecyclerview.ParentAlignment
+import com.rubensousa.dpadrecyclerview.test.TestAdapterConfiguration
+import com.rubensousa.dpadrecyclerview.test.TestLayoutConfiguration
+import com.rubensousa.dpadrecyclerview.test.helpers.assertFocusAndSelection
+import com.rubensousa.dpadrecyclerview.test.helpers.assertItemAtPosition
+import com.rubensousa.dpadrecyclerview.test.helpers.getRelativeItemViewBounds
+import com.rubensousa.dpadrecyclerview.test.helpers.onRecyclerView
+import com.rubensousa.dpadrecyclerview.test.tests.AbstractTestAdapter
+import com.rubensousa.dpadrecyclerview.test.tests.DpadRecyclerViewTest
+import com.rubensousa.dpadrecyclerview.testing.KeyEvents
+import com.rubensousa.dpadrecyclerview.testing.rules.DisableIdleTimeoutRule
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+
+class GridAdapterMutationTest : DpadRecyclerViewTest() {
+
+    @get:Rule
+    val idleTimeoutRule = DisableIdleTimeoutRule()
+
+    override fun getDefaultLayoutConfiguration(): TestLayoutConfiguration {
+        return TestLayoutConfiguration(
+            spans = 4,
+            orientation = RecyclerView.VERTICAL,
+            parentAlignment = ParentAlignment(
+                edge = ParentAlignment.Edge.MIN_MAX,
+                fraction = 0.5f
+            ),
+            childAlignment = ChildAlignment(
+                fraction = 0.5f
+            )
+        )
+    }
+
+    override fun getDefaultAdapterConfiguration(): TestAdapterConfiguration {
+        return super.getDefaultAdapterConfiguration().copy(
+            numberOfItems = 40
+        )
+    }
+
+    @Before
+    fun setup() {
+        launchFragment()
+    }
+
+    @Test
+    fun testGridRemovalWithSpanLookupDoesNotCrash() {
+        onRecyclerView("Change span size") { recyclerView ->
+            recyclerView.setSpanSizeLookup(object : DpadSpanSizeLookup() {
+                override fun getSpanSize(position: Int): Int {
+                    val adapter = recyclerView.adapter as AbstractTestAdapter<*>
+                    val item = adapter.getItem(position)
+                    return if (item % 9 == 0) {
+                        recyclerView.getSpanCount()
+                    } else {
+                        1
+                    }
+                }
+            })
+        }
+        KeyEvents.pressDown()
+        val oldViewBounds = getRelativeItemViewBounds(position = 1)
+        mutateAdapter { adapter ->
+            adapter.removeAt(3)
+            adapter.removeAt(13)
+        }
+        assertFocusAndSelection(1)
+        assertItemAtPosition(position = 1, item = 1)
+
+        val newViewBounds = getRelativeItemViewBounds(position = 1)
+        assertThat(newViewBounds).isEqualTo(oldViewBounds)
+    }
+
+}
diff --git a/...lerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt b/...lerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt
@@ -260,24 +260,24 @@ class PivotLayoutManager(properties: Properties) : RecyclerView.LayoutManager()
     }
 
     override fun onItemsAdded(recyclerView: RecyclerView, positionStart: Int, itemCount: Int) {
-        configuration.spanSizeLookup.invalidateCache()
+        layoutInfo.invalidateSpanCache()
         pivotLayout.onItemsAdded(positionStart, itemCount)
         pivotSelector.onItemsAdded(positionStart, itemCount)
     }
 
     override fun onItemsChanged(recyclerView: RecyclerView) {
-        configuration.spanSizeLookup.invalidateCache()
+        layoutInfo.invalidateSpanCache()
         pivotSelector.onItemsChanged()
     }
 
     override fun onItemsRemoved(recyclerView: RecyclerView, positionStart: Int, itemCount: Int) {
-        configuration.spanSizeLookup.invalidateCache()
+        layoutInfo.invalidateSpanCache()
         pivotLayout.onItemsRemoved(positionStart, itemCount)
         pivotSelector.onItemsRemoved(positionStart, itemCount)
     }
 
     override fun onItemsMoved(recyclerView: RecyclerView, from: Int, to: Int, itemCount: Int) {
-        configuration.spanSizeLookup.invalidateCache()
+        layoutInfo.invalidateSpanCache()
         pivotLayout.onItemsMoved(from, to, itemCount)
         pivotSelector.onItemsMoved(from, to, itemCount)
     }

diff --git a/...clerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/LayoutInfo.kt b/...clerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/LayoutInfo.kt
@@ -126,14 +126,22 @@ internal class LayoutInfo(
         return configuration.spanSizeLookup.getSpanSize(position)
     }
 
-    fun getStartSpanIndex(position: Int): Int {
-        return configuration.spanSizeLookup.getCachedSpanIndex(position, configuration.spanCount)
+    fun getStartSpanIndex(position: Int, fromCache: Boolean = true): Int {
+        return if (fromCache) {
+            configuration.spanSizeLookup.getCachedSpanIndex(position, configuration.spanCount)
+        } else {
+            configuration.spanSizeLookup.getSpanIndex(position, configuration.spanCount)
+        }
     }
 
     fun getEndSpanIndex(position: Int): Int {
         return getStartSpanIndex(position) + configuration.spanSizeLookup.getSpanSize(position) - 1
     }
 
+    fun invalidateSpanCache() {
+        configuration.spanSizeLookup.invalidateCache()
+    }
+
     fun getSpanGroupIndex(position: Int): Int {
         return configuration.spanSizeLookup.getCachedSpanGroupIndex(
             position,

diff --git a/...w/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/StructureEngineer.kt b/...w/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/StructureEngineer.kt
@@ -424,6 +424,14 @@ internal abstract class StructureEngineer(
              */
             if (layoutResult.consumedSpace > 0) {
                 viewRecycler.recycleByLayoutRequest(recycler, layoutRequest)
+            } else if (viewProvider.hasNext(layoutRequest, state)) {
+                Log.w(
+                    DpadRecyclerView.TAG,
+                    "View at position ${layoutRequest.currentPosition} could not be laid out"
+                )
+                layoutRequest.moveToNextPosition()
+            } else {
+                remainingSpace = 0
             }
 
             layoutResult.reset()

diff --git a/...main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridLayoutEngineer.kt b/...main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridLayoutEngineer.kt
@@ -587,9 +587,10 @@ internal class GridLayoutEngineer(
                 DpadRecyclerView.TAG,
                 "Cannot find post layout position for pre layout position: $position"
             )
-            return layoutInfo.getStartSpanIndex(position)
+            return layoutInfo.getStartSpanIndex(position, fromCache = false)
         }
-        return layoutInfo.getStartSpanIndex(adapterPosition)
+        // Do not re-use span index cache since we are in pre-layout
+        return layoutInfo.getStartSpanIndex(adapterPosition, fromCache = false)
     }
 
     private fun getSpanSize(recycler: Recycler, state: State, position: Int): Int {

diff --git a/gradle.properties b/gradle.properties
@@ -22,4 +22,4 @@ kotlin.code.style=official
 # thereby reducing the size of the R class for that library
 android.nonTransitiveRClass=true
 android.enableR8.fullMode=true
-LIBRARY_VERSION=1.3.0-alpha02
+LIBRARY_VERSION=1.3.0-alpha03
diff --git a/mkdocs.yml b/mkdocs.yml
@@ -24,7 +24,7 @@ theme:
 
 extra:
   dpadrecyclerview:
-    version: '1.3.0-alpha02'
+    version: '1.3.0-alpha03'
   social:
     - icon: 'fontawesome/brands/github'
       link: 'https://github.com/rubensousa/DpadRecyclerView'