Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pivot not being constrained to RecyclerView's state #179

Merged
merged 4 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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()
}
}

Expand Down Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ internal class PivotLayout(
return
}

pivotSelector.consumePendingSelectionChanges(state)
structureEngineer.onLayoutStarted(state)
pivotSelector.consumePendingSelectionChanges()

if (state.isPreLayout) {
preLayoutChildren(pivotSelector.position, recycler, state)
Expand Down Expand Up @@ -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()
}

Expand Down