Merge pull request #179 from rubensousa/adapter_update

Fix pivot not being constrained to RecyclerView's state
rubensousa · Jan 10, 2024 · 3d67616 · 3d67616
2 parents e925781 + db51848
commit 3d67616
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 18 deletions.
diff --git a/...oidTest/kotlin/com/rubensousa/dpadrecyclerview/test/helpers/RecyclerViewTestExtensions.kt b/...oidTest/kotlin/com/rubensousa/dpadrecyclerview/test/helpers/RecyclerViewTestExtensions.kt
@@ -58,11 +58,12 @@ fun selectPosition(
     position: Int,
     subPosition: Int = 0,
     smooth: Boolean = false,
+    waitForIdle: Boolean = smooth,
     id: Int = R.id.recyclerView
 ) {
     Espresso.onView(withId(id))
         .perform(DpadRecyclerViewActions.selectPosition(position, subPosition, smooth))
-    if (smooth) {
+    if (waitForIdle) {
         Espresso.onView(withId(id))
             .perform(DpadRecyclerViewActions.waitForIdleScroll())
     }

diff --git a/.../src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/AbstractTestAdapter.kt b/.../src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/AbstractTestAdapter.kt
@@ -74,6 +74,11 @@ abstract class AbstractTestAdapter<VH : RecyclerView.ViewHolder>(
         }
     }
 
+    fun setList(newList: MutableList<Int>) {
+        currentVersion++
+        items = newList
+    }
+
     private fun calculateDiff(
         oldList: List<Int>,
         newList: List<Int>

diff --git a/.../androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/selection/SelectionTest.kt b/.../androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/selection/SelectionTest.kt
@@ -32,6 +32,7 @@ import com.rubensousa.dpadrecyclerview.test.helpers.assertIsFocused
 import com.rubensousa.dpadrecyclerview.test.helpers.assertIsNotFocused
 import com.rubensousa.dpadrecyclerview.test.helpers.assertSelectedPosition
 import com.rubensousa.dpadrecyclerview.test.helpers.assertViewHolderSelected
+import com.rubensousa.dpadrecyclerview.test.helpers.onRecyclerView
 import com.rubensousa.dpadrecyclerview.test.helpers.selectPosition
 import com.rubensousa.dpadrecyclerview.test.helpers.waitForCondition
 import com.rubensousa.dpadrecyclerview.test.helpers.waitForIdleScrollState
@@ -257,4 +258,25 @@ class SelectionTest : DpadRecyclerViewTest() {
             )
     }
 
+    @Test
+    fun testSelectingPositionOutOfBoundsDoesNotCrash() {
+        val numberOfItems = 500
+        launchFragment(TestAdapterConfiguration(numberOfItems = numberOfItems))
+
+        selectPosition(numberOfItems + 100, smooth = true, waitForIdle = false)
+
+        Thread.sleep(1000L)
+
+        mutateAdapter { adapter ->
+            adapter.setList(MutableList(numberOfItems + 10) { it })
+            adapter.notifyItemRangeInserted(0, 100)
+        }
+
+        onRecyclerView("Request layout") { recyclerView ->
+            recyclerView.requestLayout()
+        }
+
+        assertFocusAndSelection(numberOfItems + 9)
+    }
+
 }
diff --git a/...lerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt b/...lerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt
@@ -124,6 +124,7 @@ class PivotLayoutManager(properties: Properties) : RecyclerView.LayoutManager()
     override fun onLayoutChildren(recycler: RecyclerView.Recycler, state: RecyclerView.State) {
         // If we have focus, save it temporarily since the views will change and we might lose it
         hadFocusBeforeLayout = hasFocus()
+        scroller.cancelSmoothScroller()
         pivotSelector.onLayoutChildren(state)
         pivotLayout.onLayoutChildren(recycler, state)
     }

diff --git a/...recyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotSelector.kt b/...recyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotSelector.kt
@@ -60,32 +60,52 @@ internal class PivotSelector(
     }
     private var selectedViewHolder: DpadViewHolder? = null
 
-    fun update(position: Int, subPosition: Int = 0): Boolean {
-        val changed = position != this.position || subPosition != this.subPosition
-        this.position = position
-        this.subPosition = subPosition
-        return changed
+    fun update(newPosition: Int, newSubPosition: Int = 0): Boolean {
+        val previousPosition = position
+        position = constrainPivotPosition(
+            position = newPosition,
+            itemCount = layoutManager.itemCount
+        )
+        subPosition = newSubPosition
+        return position != previousPosition || newSubPosition != subPosition
     }
 
-    fun consumePendingSelectionChanges(): Boolean {
+    fun consumePendingSelectionChanges(state: RecyclerView.State): Boolean {
         var consumed = false
         if (position != RecyclerView.NO_POSITION && positionOffset != OFFSET_DISABLED) {
-            applyPositionOffset()
+            applyPositionOffset(state.itemCount)
             subPosition = 0
             consumed = true
+        } else {
+            // If we didn't adjust the pivot, just ensure it's still within bounds
+            position = constrainPivotPosition(
+                position = position,
+                itemCount = state.itemCount
+            )
         }
         positionOffset = 0
         return consumed
     }
 
-    private fun applyPositionOffset() {
+    private fun applyPositionOffset(itemCount: Int) {
         val previousPosition = position
-        position += positionOffset
-        // Ensure selection is within bounds
-        position = max(0, min(layoutManager.itemCount - 1, position))
+        position = constrainPivotPosition(
+            position = position + positionOffset,
+            itemCount = itemCount
+        )
         if (position != previousPosition) {
-            isSelectionUpdatePending = true
+            setSelectionUpdatePending()
+        }
+    }
+
+    /**
+     * Calculates the pivot position so that is within bounds of the current layout state
+     */
+    private fun constrainPivotPosition(position: Int, itemCount: Int): Int {
+        if (itemCount == 0) {
+            return RecyclerView.NO_POSITION
         }
+        return max(0, min(itemCount - 1, position))
     }
 
     fun onLayoutChildren(state: RecyclerView.State) {
@@ -101,7 +121,7 @@ internal class PivotSelector(
             // Make sure the pivot is set to 0 by default whenever we have items
             position = 0
             positionOffset = 0
-            isSelectionUpdatePending = true
+            setSelectionUpdatePending()
         }
     }
 
@@ -167,9 +187,9 @@ internal class PivotSelector(
                 // If the focused position was removed,
                 // stop updating the offset until the next layout pass
                 positionOffset += positionStart - finalPosition
-                applyPositionOffset()
+                applyPositionOffset(layoutManager.itemCount)
                 positionOffset = Int.MIN_VALUE
-                isSelectionUpdatePending = true
+                setSelectionUpdatePending()
             } else {
                 positionOffset -= itemCount
             }

diff --git a/...lerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/PivotLayout.kt b/...lerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/PivotLayout.kt
@@ -78,8 +78,8 @@ internal class PivotLayout(
             return
         }
 
+        pivotSelector.consumePendingSelectionChanges(state)
         structureEngineer.onLayoutStarted(state)
-        pivotSelector.consumePendingSelectionChanges()
 
         if (state.isPreLayout) {
             preLayoutChildren(pivotSelector.position, recycler, state)
@@ -113,7 +113,7 @@ internal class PivotLayout(
 
     private fun layoutChildren(recycler: RecyclerView.Recycler, state: RecyclerView.State) {
         if (DpadRecyclerView.DEBUG) {
-            Log.i(TAG, "LayoutStart: ${state.asString()}")
+            Log.i(TAG, "LayoutStart for pivot ${pivotSelector.position}: ${state.asString()}")
             structureEngineer.logChildren()
         }