From 5509b2029d23fd31d0b9f75ce49480ae3a01f245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BAben=20Sousa?= Date: Thu, 3 Aug 2023 01:29:41 +0200 Subject: [PATCH 1/5] Fix grid laying out views in wrong spans when scrolling in opposite direction --- .../tests/scrolling/VerticalGridScrollTest.kt | 35 +++++ .../dpadrecyclerview/DpadSpanSizeLookup.kt | 14 +- .../layoutmanager/LayoutConfiguration.kt | 5 + .../layoutmanager/PivotLayoutManager.kt | 4 + .../layoutmanager/focus/SpanFocusFinder.kt | 111 +++++++-------- .../layoutmanager/layout/StructureEngineer.kt | 5 +- .../layout/grid/GridLayoutEngineer.kt | 126 +++++++++--------- .../layoutmanager/layout/grid/GridRow.kt | 93 ++----------- .../spacing/DpadGridSpacingDecoration.kt | 16 +-- .../focus/SpanFocusFinderTest.kt | 98 +++++++++++++- .../test/layoutmanager/grid/GridRowTest.kt | 61 ++++----- 11 files changed, 306 insertions(+), 262 deletions(-) diff --git a/dpadrecyclerview/src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/scrolling/VerticalGridScrollTest.kt b/dpadrecyclerview/src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/scrolling/VerticalGridScrollTest.kt index 61ad0c0e..b763a829 100644 --- a/dpadrecyclerview/src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/scrolling/VerticalGridScrollTest.kt +++ b/dpadrecyclerview/src/androidTest/kotlin/com/rubensousa/dpadrecyclerview/test/tests/scrolling/VerticalGridScrollTest.kt @@ -18,6 +18,7 @@ package com.rubensousa.dpadrecyclerview.test.tests.scrolling import androidx.recyclerview.widget.RecyclerView import androidx.test.platform.app.InstrumentationRegistry +import com.google.common.truth.Truth.assertThat import com.rubensousa.dpadrecyclerview.ChildAlignment import com.rubensousa.dpadrecyclerview.DpadSpanSizeLookup import com.rubensousa.dpadrecyclerview.FocusableDirection @@ -271,6 +272,40 @@ class VerticalGridScrollTest : DpadRecyclerViewTest() { assertFocusAndSelection(position = 78) } + @Test + fun testLayoutStaysTheSameWhenScrollingInOppositeDirection() { + launchFragment() + onRecyclerView("Change span size lookup") { recyclerView -> + recyclerView.setParentAlignment(ParentAlignment(fraction = 0.0f)) + recyclerView.setChildAlignment(ChildAlignment(fraction = 0.0f)) + recyclerView.setSpanSizeLookup(object : DpadSpanSizeLookup() { + override fun getSpanSize(position: Int): Int { + return if (position == 0 || position.rem(9) == 0) { + spanCount + } else { + 1 + } + } + }) + } + waitForIdleScrollState() + + val childPositions = getChildrenBounds() + + // Scroll down until last uneven child is visible + repeat(5) { + KeyEvents.pressDown(delay = 500L) + } + + waitForIdleScrollState() + + repeat(5) { + KeyEvents.pressUp(delay = 500L) + } + + assertThat(getChildrenBounds()).isEqualTo(childPositions) + } + private fun scrollUp(grid: VerticalGridLayout) { KeyEvents.pressUp() grid.scrollUp() diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/DpadSpanSizeLookup.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/DpadSpanSizeLookup.kt index 1fa10f79..9ec53b8b 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/DpadSpanSizeLookup.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/DpadSpanSizeLookup.kt @@ -106,20 +106,8 @@ abstract class DpadSpanSizeLookup { cacheSpanGroupIndices = enabled } - - /** - * Clears the span index cache. GridLayoutManager automatically calls this method when - * adapter changes occur. - */ - fun invalidateSpanIndexCache() { + fun invalidateCache() { spanIndexCache.clear() - } - - /** - * Clears the span group index cache. GridLayoutManager automatically calls this method - * when adapter changes occur. - */ - fun invalidateSpanGroupIndexCache() { spanGroupIndexCache.clear() } diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/LayoutConfiguration.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/LayoutConfiguration.kt index 61bf8557..e766e4ec 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/LayoutConfiguration.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/LayoutConfiguration.kt @@ -169,10 +169,15 @@ internal class LayoutConfiguration(properties: Properties) { fun setSpanCount(count: Int) { spanCount = max(1, count) + spanSizeLookup.invalidateCache() } fun setSpanSizeLookup(spanSizeLookup: DpadSpanSizeLookup) { this.spanSizeLookup = spanSizeLookup + if (spanSizeLookup !== DpadSpanSizeLookup.DEFAULT) { + spanSizeLookup.setSpanIndexCacheEnabled(true) + spanSizeLookup.setSpanGroupIndexCacheEnabled(true) + } } fun setChildDrawingOrderEnabled(enabled: Boolean) { diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt index d828cfb5..4a673192 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/PivotLayoutManager.kt @@ -187,20 +187,24 @@ class PivotLayoutManager(properties: Properties) : RecyclerView.LayoutManager() } override fun onItemsAdded(recyclerView: RecyclerView, positionStart: Int, itemCount: Int) { + configuration.spanSizeLookup.invalidateCache() pivotLayout.onItemsAdded(positionStart, itemCount) pivotSelector.onItemsAdded(positionStart, itemCount) } override fun onItemsChanged(recyclerView: RecyclerView) { + configuration.spanSizeLookup.invalidateCache() pivotSelector.onItemsChanged() } override fun onItemsRemoved(recyclerView: RecyclerView, positionStart: Int, itemCount: Int) { + configuration.spanSizeLookup.invalidateCache() pivotLayout.onItemsRemoved(positionStart, itemCount) pivotSelector.onItemsRemoved(positionStart, itemCount) } override fun onItemsMoved(recyclerView: RecyclerView, from: Int, to: Int, itemCount: Int) { + configuration.spanSizeLookup.invalidateCache() pivotLayout.onItemsMoved(from, to, itemCount) pivotSelector.onItemsMoved(from, to, itemCount) } diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt index f37dcada..d707ea2b 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt @@ -65,7 +65,7 @@ internal class SpanFocusFinder { if (spanCount == 1) { return RecyclerView.NO_POSITION } - val positionDirection = if (forward) 1 else -1 + var positionDirection = if (forward) 1 else -1 val spanDirection = getSpanDirection(forward, reverseLayout) if (spanSizeLookup === DpadSpanSizeLookup.DEFAULT) { return findNextEvenSpanPosition( @@ -73,12 +73,10 @@ internal class SpanFocusFinder { ) } val focusedSpanIndex = spanSizeLookup.getCachedSpanIndex(focusedPosition, spanCount) - val focusedSpanSize = spanSizeLookup.getSpanSize(focusedPosition) - val currentSpanIndex = focusedSpanIndex + focusedSpanSize * spanDirection - spanDirection - // Move position to the start of the next span group val firstPositionInNextSpanGroup = moveToStartOfNextSpanGroup( - focusedPosition, currentSpanIndex, spanSizeLookup, spanDirection, positionDirection + focusedPosition, focusedSpanIndex, spanSizeLookup, + spanDirection, positionDirection, reverseLayout ) var currentPosition = firstPositionInNextSpanGroup @@ -94,9 +92,15 @@ internal class SpanFocusFinder { return currentPosition } + positionDirection = if (forward || reverseLayout) { + positionDirection + } else { + positionDirection * -1 + } + // Now search until we find the cached span index or we go outside the edge while (!isPositionOutOfBounds(currentPosition, edgePosition, forward)) { - if (isPositionAtCachedSpan(currentPosition, spanSizeLookup, spanDirection)) { + if (isPositionAtCachedSpan(currentPosition, spanSizeLookup, reverseLayout)) { return currentPosition } currentPosition += positionDirection @@ -134,10 +138,10 @@ internal class SpanFocusFinder { private fun isPositionAtCachedSpan( position: Int, spanSizeLookup: DpadSpanSizeLookup, - spanDirection: Int + reverseLayout: Boolean, ): Boolean { val spanIndex = spanSizeLookup.getCachedSpanIndex(position, spanCount) - return if (spanDirection > 0) { + return if (!reverseLayout) { spanIndex >= cachedSpanIndex } else { spanIndex <= cachedSpanIndex @@ -145,34 +149,60 @@ internal class SpanFocusFinder { } private fun moveToStartOfNextSpanGroup( - currentPosition: Int, - currentSpanIndex: Int, - spanSizeLookup: DpadSpanSizeLookup, - spanDirection: Int, - positionDirection: Int, + position: Int, + spanIndex: Int, + lookup: DpadSpanSizeLookup, + spanDir: Int, + posDir: Int, + reverseLayout: Boolean ): Int { - val targetSpanIndex = getEndSpanIndex(spanDirection) - val position = moveSpanIndexToTarget( - currentPosition, - currentSpanIndex, - targetSpanIndex, - spanSizeLookup, - spanDirection, - positionDirection - ) - return position + positionDirection + var currentPos = position + var currentSpan = spanIndex + val startSpanIndex = if (!reverseLayout) 0 else spanCount - 1 + + // First step: move to edge of current span group + while (fitsNextInCurrentSpanGroup(lookup, currentSpan, currentPos, spanDir, posDir)) { + currentPos += posDir + currentSpan = getNextSpanEnd(lookup, currentSpan, currentPos, spanDir, posDir) + } + + // Move to next span group + currentPos += posDir + + // Second step: move to start of next span group + currentSpan = lookup.getCachedSpanIndex(currentPos, spanCount) + while (currentSpan != startSpanIndex && currentSpan > 0 && currentSpan < spanCount) { + currentSpan += lookup.getSpanSize(currentPos) * spanDir + currentPos += posDir + } + + return currentPos } - private fun isPositionOutOfBounds(position: Int, edgePosition: Int, forward: Boolean): Boolean { - return (position > edgePosition && forward) || (position < edgePosition && !forward) + private fun fitsNextInCurrentSpanGroup( + lookup: DpadSpanSizeLookup, + spanIndex: Int, + currentPos: Int, + spanDir: Int, + posDir: Int + ): Boolean { + val nextSpanEnd = getNextSpanEnd(lookup, spanIndex, currentPos, spanDir, posDir) + return nextSpanEnd >= 0 && nextSpanEnd <= spanCount - 1 } - private fun getStartSpanIndex(spanDirection: Int): Int { - return if (spanDirection > 0) 0 else spanCount - 1 + private fun getNextSpanEnd( + spanSizeLookup: DpadSpanSizeLookup, + spanIndex: Int, + currentPos: Int, + spanDir: Int, + posDir: Int + ): Int { + val currentSpanEnd = spanIndex + (spanSizeLookup.getSpanSize(currentPos) - 1) * spanDir + return currentSpanEnd + spanSizeLookup.getSpanSize(currentPos + posDir) * spanDir } - private fun getEndSpanIndex(spanDirection: Int): Int { - return getStartSpanIndex(-spanDirection) + private fun isPositionOutOfBounds(position: Int, edgePosition: Int, forward: Boolean): Boolean { + return (position > edgePosition && forward) || (position < 0 && !forward) } private fun getSpanDirection(forward: Boolean, reverseLayout: Boolean): Int { @@ -184,27 +214,4 @@ internal class SpanFocusFinder { } } - private fun moveSpanIndexToTarget( - position: Int, - spanIndex: Int, - targetSpanIndex: Int, - spanSizeLookup: DpadSpanSizeLookup, - spanDirection: Int, - positionDirection: Int - ): Int { - if (spanIndex == targetSpanIndex) { - return position - } - var currentSpanIndex = spanIndex - var currentPosition = position - while (currentSpanIndex != targetSpanIndex - && currentSpanIndex >= 0 - && currentSpanIndex < spanCount - ) { - currentSpanIndex += spanSizeLookup.getSpanSize(position) * spanDirection - currentPosition += positionDirection - } - return currentPosition - } - } diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/StructureEngineer.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/StructureEngineer.kt index a26b5b03..39ec13bb 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/StructureEngineer.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/StructureEngineer.kt @@ -397,6 +397,7 @@ internal abstract class StructureEngineer( recycler: RecyclerView.Recycler, state: RecyclerView.State, ): Int { + var newSpace = 0 var remainingSpace = layoutRequest.fillSpace layoutResult.reset() @@ -411,6 +412,8 @@ internal abstract class StructureEngineer( layoutResult.consumedSpace * layoutRequest.direction.value ) + newSpace += layoutResult.consumedSpace + if (!layoutResult.skipConsumption) { remainingSpace -= layoutResult.consumedSpace } @@ -429,7 +432,7 @@ internal abstract class StructureEngineer( // Recycle once again after layout is done viewRecycler.recycleByLayoutRequest(recycler, layoutRequest) - return layoutRequest.fillSpace - remainingSpace + return newSpace } private fun updateLoopingState() { diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridLayoutEngineer.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridLayoutEngineer.kt index 3a931739..c15f7f48 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridLayoutEngineer.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridLayoutEngineer.kt @@ -243,9 +243,12 @@ internal class GridLayoutEngineer( layoutResult: LayoutResult ) { layoutRow.reset(keyline = layoutRequest.checkpoint) - val fillSpanCount = calculateSpansToFill(layoutRequest) - val viewCount = getViewsForRow(layoutRequest, viewProvider, recycler, state, fillSpanCount) - assignSpans(recycler, state, viewCount, appending = layoutRequest.isAppending()) + val anchorSpanIndex = getSpanIndex(recycler, state, layoutRequest.currentPosition) + val anchorSpanSize = getSpanSize(recycler, state, layoutRequest.currentPosition) + val availableSpans = calculateAvailableSpans(layoutRequest, anchorSpanIndex, anchorSpanSize) + val viewCount = getViewsForRow( + layoutRequest, viewProvider, recycler, state, anchorSpanIndex, availableSpans + ) val rowHeight = fillRow(viewCount, layoutRow, layoutRequest) layoutResult.consumedSpace = rowHeight @@ -266,12 +269,6 @@ internal class GridLayoutEngineer( repeat(viewCount) { index -> val view = getRowViewAt(index) val layoutParams = layoutInfo.getLayoutParams(view) - if (layoutRequest.isAppending() && !row.fitsEnd(layoutParams.spanSize)) { - return row.height - } - if (layoutRequest.isPrepending() && !row.fitsStart(layoutParams.spanSize)) { - return row.height - } addView(view, layoutRequest) layoutManager.calculateItemDecorationsForChild(view, insets) @@ -287,12 +284,14 @@ internal class GridLayoutEngineer( row.append( decoratedSize, layoutParams.viewLayoutPosition, + layoutParams.spanIndex, layoutParams.spanSize ) } else { row.prepend( decoratedSize, layoutParams.viewLayoutPosition, + layoutParams.spanIndex, layoutParams.spanSize ) } @@ -372,15 +371,30 @@ internal class GridLayoutEngineer( ) { val perpendicularSize = layoutInfo.getPerpendicularDecoratedSize(view) if (layoutRequest.isVertical) { - bounds.left = layoutManager.paddingLeft + layoutRow.getSpanBorder( - layoutParams.spanIndex - ) - bounds.right = bounds.left + perpendicularSize + if (!layoutRequest.reverseLayout) { + bounds.left = layoutManager.paddingLeft + layoutRow.getSpanBorder( + layoutParams.spanIndex + ) + bounds.right = bounds.left + perpendicularSize + } else { + bounds.right = layoutRow.getSpanBorder( + layoutRow.numberOfSpans - layoutParams.spanIndex + ) - layoutManager.paddingRight + bounds.left = bounds.right - perpendicularSize + } } else { - bounds.top = layoutManager.paddingTop + layoutRow.getSpanBorder( - layoutParams.spanIndex - ) - bounds.bottom = bounds.top + perpendicularSize + if (!layoutRequest.reverseLayout) { + bounds.top = layoutManager.paddingTop + layoutRow.getSpanBorder( + layoutParams.spanIndex, + ) + bounds.bottom = bounds.top + perpendicularSize + } else { + bounds.bottom = layoutRow.getSpanBorder( + layoutRow.numberOfSpans - layoutParams.spanIndex + ) - layoutManager.paddingBottom + bounds.top = bounds.bottom - perpendicularSize + } + } } @@ -489,11 +503,15 @@ internal class GridLayoutEngineer( } } - private fun calculateSpansToFill(request: LayoutRequest): Int { - return if (request.isAppending()) { - layoutRow.getAvailableAppendSpans() + private fun calculateAvailableSpans( + request: LayoutRequest, + anchorSpanIndex: Int, + anchorSpanSize: Int + ): Int { + return if (request.isAppending() != request.reverseLayout) { + layoutRow.numberOfSpans - anchorSpanIndex } else { - layoutRow.getAvailablePrependSpans() + anchorSpanIndex + anchorSpanSize } } @@ -502,9 +520,11 @@ internal class GridLayoutEngineer( viewProvider: ViewProvider, recycler: Recycler, state: State, - spansToFill: Int + anchorSpanIndex: Int, + availableSpans: Int ): Int { - var remainingSpans = spansToFill + var currentSpanIndex = anchorSpanIndex + var remainingSpans = availableSpans var viewCount = 0 while (isRowIncomplete(viewCount, viewProvider, layoutRequest, state, remainingSpans)) { val position = layoutRequest.currentPosition @@ -521,11 +541,22 @@ internal class GridLayoutEngineer( break } val view = viewProvider.next(layoutRequest, state) + val params = layoutInfo.getLayoutParams(view) + params.updateSpan( + index = currentSpanIndex, + groupIndex = getSpanGroupIndex(recycler, state, position), + size = spanSize + ) if (position == pivotLayoutPosition) { pivotView = view } rowViews[viewCount] = view viewCount++ + if (layoutRequest.isAppending() != layoutRequest.reverseLayout) { + currentSpanIndex += spanSize + } else { + currentSpanIndex -= spanSize + } } return viewCount } @@ -542,43 +573,6 @@ internal class GridLayoutEngineer( && remainingSpans > 0 } - /** - * Assigns a span index and span size to each view inside [rowViews]. - */ - private fun assignSpans( - recycler: Recycler, - state: State, - count: Int, - appending: Boolean - ) { - var start = 0 - var end = 0 - var increment = 0 - if (appending) { - start = 0 - end = count - increment = 1 - } else { - start = count - 1 - end = -1 - increment = -1 - } - var span = 0 - var i = start - while (i != end) { - val view = getRowViewAt(i) - val params = layoutInfo.getLayoutParams(view) - val layoutPosition = layoutInfo.getLayoutPositionOf(view) - params.updateSpan( - index = span, - groupIndex = getSpanGroupIndex(recycler, state, layoutPosition), - size = getSpanSize(recycler, state, layoutPosition) - ) - span += params.spanSize - i += increment - } - } - private fun getSpanIndex(recycler: Recycler, state: State, position: Int): Int { if (!state.isPreLayout) { return layoutInfo.getStartSpanIndex(position) @@ -591,9 +585,9 @@ internal class GridLayoutEngineer( if (adapterPosition == RecyclerView.NO_POSITION) { Log.w( DpadRecyclerView.TAG, - "Cannot find span index for pre layout position: $position" + "Cannot find post layout position for pre layout position: $position" ) - return 1 + return layoutInfo.getStartSpanIndex(position) } return layoutInfo.getStartSpanIndex(adapterPosition) } @@ -610,9 +604,9 @@ internal class GridLayoutEngineer( if (adapterPosition == RecyclerView.NO_POSITION) { Log.w( DpadRecyclerView.TAG, - "Cannot find span size for pre layout position: $position" + "Cannot find post layout position for pre layout position: $position" ) - return 1 + return layoutInfo.getSpanSize(position) } return layoutInfo.getSpanSize(adapterPosition) } @@ -629,9 +623,9 @@ internal class GridLayoutEngineer( if (adapterPosition == RecyclerView.NO_POSITION) { Log.w( DpadRecyclerView.TAG, - "Cannot find span size for pre layout position: $position" + "Cannot find post layout position for pre layout position: $position" ) - return 1 + return layoutInfo.getSpanGroupIndex(position) } return layoutInfo.getSpanGroupIndex(adapterPosition) } diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridRow.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridRow.kt index 42e15ca6..45d64923 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridRow.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/layout/grid/GridRow.kt @@ -90,30 +90,8 @@ internal class GridRow( return startIndex == RecyclerView.NO_POSITION && endIndex == RecyclerView.NO_POSITION } - fun getAvailableAppendSpans(): Int { - if (endIndex == RecyclerView.NO_POSITION) { - if (startIndex == RecyclerView.NO_POSITION) { - return numberOfSpans - } - return numberOfSpans - startIndex - } - // Spans: 3, filled: [X, -, -], - return numberOfSpans - endIndex - 1 - } - - fun getAvailablePrependSpans(): Int { - if (startIndex == RecyclerView.NO_POSITION) { - if (endIndex == RecyclerView.NO_POSITION) { - return numberOfSpans - } - return endIndex - } - // Spans: 3, filled: [X, -, -], - return startIndex - } - - fun getSpanBorder(index: Int): Int { - return spanBorders[index] + fun getSpanBorder(spanIndex: Int): Int { + return spanBorders[spanIndex] } fun getSpaceForSpanRange( @@ -134,28 +112,6 @@ internal class GridRow( endOffset += dy } - fun fitsEnd(spanSize: Int): Boolean { - if (endIndex == RecyclerView.NO_POSITION) { - return spanSize <= numberOfSpans - } - return endIndex + spanSize < numberOfSpans - } - - fun fitsStart(spanSize: Int): Boolean { - if (startIndex == RecyclerView.NO_POSITION) { - return spanSize <= numberOfSpans - } - return startIndex - spanSize >= 0 - } - - fun isEndComplete(): Boolean { - return endIndex == numberOfSpans - 1 - } - - fun isStartComplete(): Boolean { - return startIndex == 0 - } - fun getSpanSpace(): Int { return width / numberOfSpans } @@ -192,51 +148,22 @@ internal class GridRow( return getSpanBorder(endIndex + 1) } - fun append(viewSize: Int, viewPosition: Int, spanSize: Int): Int { - val viewSpanIndex = endIndex + 1 - val viewStart = getSpanSpace() * viewSpanIndex - updateSpans(viewSize, viewPosition, viewSpanIndex, spanSize) + fun append(viewSize: Int, viewPosition: Int, spanIndex: Int, spanSize: Int) { + endIndex = spanIndex + spanSize - 1 + updateSpans(viewSize, viewPosition, spanIndex, spanSize) endOffset = startOffset + height - endIndex += spanSize if (startIndex == RecyclerView.NO_POSITION) { - startIndex = 0 + startIndex = spanIndex } - return viewStart } - fun prepend(viewSize: Int, viewPosition: Int, spanSize: Int): Int { - if (startIndex == RecyclerView.NO_POSITION) { - startIndex = numberOfSpans - spanSize - } else { - startIndex -= spanSize - } - updateSpans(viewSize, viewPosition, startIndex, spanSize) + fun prepend(viewSize: Int, viewPosition: Int, spanIndex: Int, spanSize: Int) { + startIndex = spanIndex + updateSpans(viewSize, viewPosition, spanIndex, spanSize) startOffset = endOffset - height if (endIndex == RecyclerView.NO_POSITION) { - endIndex = numberOfSpans - 1 + endIndex = spanIndex + spanSize - 1 } - return getSpanSpace() * startIndex - } - - /** - * @return next item position - */ - fun moveToNext(): Int { - val nextPosition = getPositionAt(numberOfSpans - 1) + 1 - startOffset += height - endOffset = startOffset - resetSpans() - return nextPosition - } - - /** - * @return previous item position - */ - fun moveToPrevious(): Int { - val previousPosition = getPositionAt(0) - 1 - endOffset = startOffset - resetSpans() - return previousPosition } fun reset(keyline: Int) { diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/spacing/DpadGridSpacingDecoration.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/spacing/DpadGridSpacingDecoration.kt index 19c83f4a..f0b77d94 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/spacing/DpadGridSpacingDecoration.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/spacing/DpadGridSpacingDecoration.kt @@ -74,22 +74,22 @@ class DpadGridSpacingDecoration private constructor( val reverseLayout = parent.isLayoutReversed() val itemCount = state.itemCount val spanSize = layoutParams.spanSize - - val isAtStartEdge = layoutParams.spanGroupIndex == 0 - - val isAtEndEdge = if (!reverseLayout) { - (layoutPosition + spanCount - spanIndex - spanSize) >= itemCount - 1 + val realSpanIndex = if (!reverseLayout) { + spanIndex } else { - (layoutPosition + spanSize + spanIndex) >= itemCount + spanCount - 1 - spanIndex } + val isAtStartEdge = layoutParams.spanGroupIndex == 0 + val isAtEndEdge = layoutPosition + (spanCount - spanIndex - spanSize) >= itemCount - 1 + if (parent.getOrientation() == RecyclerView.VERTICAL) { applyVertically( - outRect, spanIndex, spanSize, spanCount, isAtStartEdge, isAtEndEdge, reverseLayout + outRect, realSpanIndex, spanSize, spanCount, isAtStartEdge, isAtEndEdge, reverseLayout ) } else { applyHorizontally( - outRect, spanIndex, spanSize, spanCount, isAtStartEdge, isAtEndEdge, reverseLayout + outRect, realSpanIndex, spanSize, spanCount, isAtStartEdge, isAtEndEdge, reverseLayout ) } } diff --git a/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt b/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt index c58e391c..baee076b 100644 --- a/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt +++ b/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt @@ -84,6 +84,21 @@ class SpanFocusFinderTest { } } + @Test + fun `previous span focus is set to header when focus is on any span`() { + repeat(spanCount) { spanIndex -> + assertThat( + finder.findNextSpanPosition( + focusedPosition = spanIndex + 1, + spanSizeLookup = headerSpanSizeLookup, + forward = false, + edgePosition = 0, + reverseLayout = false + ) + ).isEqualTo(0) + } + } + @Test fun `next span focus is correctly set for last possible view`() { assertThat( @@ -127,7 +142,7 @@ class SpanFocusFinderTest { edgePosition = spanCount + 2, reverseLayout = true ) - ).isEqualTo( spanCount + 2) + ).isEqualTo(spanCount + 2) } @Test @@ -260,4 +275,85 @@ class SpanFocusFinderTest { ).isEqualTo(RecyclerView.NO_POSITION) } + @Test + fun `next position is found for full span configuration`() { + val spanSizeLookup = object : DpadSpanSizeLookup() { + override fun getSpanSize(position: Int): Int { + return if (position == 0 || position.rem(9) == 0) { + spanCount + } else { + 1 + } + } + } + assertThat( + finder.findNextSpanPosition( + focusedPosition = 0, + spanSizeLookup = spanSizeLookup, + forward = true, + edgePosition = 199, + reverseLayout = false + ) + ).isEqualTo(1) + assertThat( + finder.findNextSpanPosition( + focusedPosition = 1, + spanSizeLookup = spanSizeLookup, + forward = true, + edgePosition = 199, + reverseLayout = false + ) + ).isEqualTo(6) + assertThat( + finder.findNextSpanPosition( + focusedPosition = 6, + spanSizeLookup = spanSizeLookup, + forward = true, + edgePosition = 199, + reverseLayout = false + ) + ).isEqualTo(9) + } + + @Test + fun `previous position is found for full span configuration`() { + val spanSizeLookup = object : DpadSpanSizeLookup() { + override fun getSpanSize(position: Int): Int { + return if (position == 0 || position.rem(9) == 0) { + spanCount + } else { + 1 + } + } + } + assertThat( + finder.findNextSpanPosition( + focusedPosition = 1, + spanSizeLookup = spanSizeLookup, + forward = false, + edgePosition = 199, + reverseLayout = false + ) + ).isEqualTo(0) + assertThat( + finder.findNextSpanPosition( + focusedPosition = 6, + spanSizeLookup = spanSizeLookup, + forward = false, + edgePosition = 199, + reverseLayout = false + ) + ).isEqualTo(1) + assertThat( + finder.findNextSpanPosition( + focusedPosition = 9, + spanSizeLookup = spanSizeLookup, + forward = false, + edgePosition = 199, + reverseLayout = false + ) + ).isEqualTo(6) + } + + } diff --git a/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/grid/GridRowTest.kt b/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/grid/GridRowTest.kt index 59d64a99..e8f1299a 100644 --- a/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/grid/GridRowTest.kt +++ b/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/grid/GridRowTest.kt @@ -44,7 +44,12 @@ class GridRowTest { @Test fun `constructor copies the state of another row`() { val startRow = createRow() - startRow.append(viewSize = 500, viewPosition = 0, spanSize = defaultNumberOfSpans) + startRow.append( + viewSize = 500, + viewPosition = 0, + spanIndex = 0, + spanSize = defaultNumberOfSpans + ) val endRow = GridRow(startRow) assertThat(endRow.startOffset).isEqualTo(startRow.startOffset) @@ -69,29 +74,6 @@ class GridRowTest { assertThat(startRow.startOffset).isEqualTo(0) } - @Test - fun `fits end only returns true if item can be appended to the row`() { - val row = createRow() - repeat(defaultNumberOfSpans) { index -> - assertThat(row.fitsEnd(spanSize = index + 1)).isTrue() - } - - row.append(viewSize = defaultViewSize, viewPosition = 0, spanSize = 1) - assertThat(row.fitsEnd(spanSize = defaultNumberOfSpans)).isFalse() - } - - @Test - fun `fits start only returns true if item can be prepended to the row`() { - val row = createRow() - repeat(defaultNumberOfSpans) { index -> - assertThat(row.fitsStart(spanSize = index + 1)).isTrue() - } - - row.prepend(viewSize = defaultViewSize, viewPosition = 0, spanSize = 1) - - assertThat(row.fitsStart(spanSize = defaultNumberOfSpans)).isFalse() - } - @Test fun `getSpanSpace returns the minimum size of a span`() { val row = createRow() @@ -104,10 +86,10 @@ class GridRowTest { // Row is empty, so offset should be 0 assertThat(row.getSpanStartOffset()).isEqualTo(0) - row.prepend(viewSize = defaultViewSize, viewPosition = 3, spanSize = 1) - assertThat(row.getSpanStartOffset()).isEqualTo(row.getSpanSpace() * 4) + row.prepend(viewSize = defaultViewSize, viewPosition = 3, spanIndex = 3, spanSize = 1) + assertThat(row.getSpanStartOffset()).isEqualTo(row.getSpanSpace() * 3) - row.prepend(viewSize = defaultViewSize, viewPosition = 2, spanSize = 4) + row.prepend(viewSize = defaultViewSize, viewPosition = 2, spanIndex = 0, spanSize = 4) assertThat(row.getSpanStartOffset()).isEqualTo(0) } @@ -118,10 +100,10 @@ class GridRowTest { assertThat(row.getSpanEndOffset()).isEqualTo(0) // Appending an item moves the end offset - row.append(viewSize = defaultViewSize, viewPosition = 0, spanSize = 1) + row.append(viewSize = defaultViewSize, viewPosition = 0, spanIndex = 0, spanSize = 1) assertThat(row.getSpanEndOffset()).isEqualTo(row.getSpanSpace()) - row.append(viewSize = defaultViewSize, viewPosition = 1, spanSize = 4) + row.append(viewSize = defaultViewSize, viewPosition = 1, spanIndex = 1, spanSize = 4) assertThat(row.getSpanEndOffset()).isEqualTo(row.getWidth()) } @@ -129,15 +111,13 @@ class GridRowTest { fun `append inserts item at the end of a row and updates row height`() { val row = createRow() - var viewLeft = row.append(viewSize = defaultViewSize, viewPosition = 0, spanSize = 1) - assertThat(viewLeft).isEqualTo(0) + row.append(viewSize = defaultViewSize, viewPosition = 0, spanIndex = 0, spanSize = 1) assertThat(row.startIndex).isEqualTo(0) assertThat(row.endIndex).isEqualTo(0) assertThat(row.height).isEqualTo(defaultViewSize) val newHeight = defaultViewSize + 50 - viewLeft = row.append(viewSize = newHeight, viewPosition = 1, spanSize = 1) - assertThat(viewLeft).isEqualTo(row.getSpanSpace()) + row.append(viewSize = newHeight, viewPosition = 1, spanIndex = 1, spanSize = 1) assertThat(row.height).isEqualTo(newHeight) assertThat(row.startIndex).isEqualTo(0) assertThat(row.endIndex).isEqualTo(1) @@ -147,13 +127,18 @@ class GridRowTest { fun `prepend inserts item at the start of a row and updates row height`() { val row = createRow() - var viewLeft = row.prepend(viewSize = defaultViewSize, viewPosition = 0, spanSize = 1) - assertThat(viewLeft).isEqualTo(row.getSpanSpace() * (defaultNumberOfSpans - 1)) + row.prepend( + viewSize = defaultViewSize, + viewPosition = 0, + spanIndex = 0, + spanSize = 1 + ) + + assertThat(row.getSpanStartOffset()).isEqualTo(0) assertThat(row.height).isEqualTo(defaultViewSize) val newHeight = defaultViewSize + 50 - viewLeft = row.prepend(viewSize = newHeight, viewPosition = 1, spanSize = 1) - assertThat(viewLeft).isEqualTo(row.getSpanSpace() * (defaultNumberOfSpans - 2)) + row.prepend(viewSize = newHeight, viewPosition = 1, spanIndex = 1, spanSize = 1) assertThat(row.height).isEqualTo(newHeight) } @@ -173,7 +158,7 @@ class GridRowTest { ): GridRow { val row = createRow(numberOfSpans, width) row.offsetBy(top) - row.append(viewSize, viewPosition = 0, spanSize) + row.append(viewSize, viewPosition = 0, spanIndex = 0, spanSize) return row } } \ No newline at end of file From fb91a84f13955709ebc70ff578f11e2762d92148 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BAben=20Sousa?= Date: Fri, 4 Aug 2023 00:31:19 +0200 Subject: [PATCH 2/5] Update public API --- dpadrecyclerview/api/dpadrecyclerview.api | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dpadrecyclerview/api/dpadrecyclerview.api b/dpadrecyclerview/api/dpadrecyclerview.api index 4ba4e012..51ad281b 100644 --- a/dpadrecyclerview/api/dpadrecyclerview.api +++ b/dpadrecyclerview/api/dpadrecyclerview.api @@ -189,8 +189,7 @@ public abstract class com/rubensousa/dpadrecyclerview/DpadSpanSizeLookup { public fun getSpanGroupIndex (II)I public fun getSpanIndex (II)I public abstract fun getSpanSize (I)I - public final fun invalidateSpanGroupIndexCache ()V - public final fun invalidateSpanIndexCache ()V + public final fun invalidateCache ()V public final fun setSpanGroupIndexCacheEnabled (Z)V public final fun setSpanIndexCacheEnabled (Z)V } From bbbe113e964def7dce34666f296c939547ddbb61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BAben=20Sousa?= Date: Fri, 4 Aug 2023 00:37:05 +0200 Subject: [PATCH 3/5] Bump to 1.1.0-alpha03 --- docs/changelog.md | 8 ++++++++ gradle.properties | 2 +- mkdocs.yml | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index d8fc639b..27ea547b 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,6 +2,14 @@ ## Version 1.1.0 +### 1.1.0-alpha03 + +2023-08-?? + +#### Bug fixes + +- Fixed grid layout not placing views in the correct spans when scrolling in opposite direction: ([#156](https://github.com/rubensousa/DpadRecyclerView/issues/156)) + ### 1.1.0-alpha02 2023-06-23 diff --git a/gradle.properties b/gradle.properties index a78d98ed..5ee65303 100644 --- 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.1.0-alpha02 \ No newline at end of file +LIBRARY_VERSION=1.1.0-alpha03 \ No newline at end of file diff --git a/mkdocs.yml b/mkdocs.yml index 436a4fa8..0004ef3d 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -24,7 +24,7 @@ theme: extra: dpadrecyclerview: - version: '1.1.0-alpha02' + version: '1.1.0-alpha03' social: - icon: 'fontawesome/brands/github' link: 'https://github.com/rubensousa/DpadRecyclerView' From 08d4ff49d34f491a898370c5d5b40677e0d4f7f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BAben=20Sousa?= Date: Fri, 4 Aug 2023 21:07:48 +0200 Subject: [PATCH 4/5] Check range of positions when searching other spans --- .../layoutmanager/focus/SpanFocusFinder.kt | 36 ++++++++++--- .../focus/SpanFocusFinderTest.kt | 54 ++++++++++++++++++- 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt index d707ea2b..59917117 100644 --- a/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt +++ b/dpadrecyclerview/src/main/java/com/rubensousa/dpadrecyclerview/layoutmanager/focus/SpanFocusFinder.kt @@ -66,7 +66,6 @@ internal class SpanFocusFinder { return RecyclerView.NO_POSITION } var positionDirection = if (forward) 1 else -1 - val spanDirection = getSpanDirection(forward, reverseLayout) if (spanSizeLookup === DpadSpanSizeLookup.DEFAULT) { return findNextEvenSpanPosition( spanSizeLookup, focusedPosition, edgePosition, positionDirection @@ -75,15 +74,22 @@ internal class SpanFocusFinder { val focusedSpanIndex = spanSizeLookup.getCachedSpanIndex(focusedPosition, spanCount) val firstPositionInNextSpanGroup = moveToStartOfNextSpanGroup( - focusedPosition, focusedSpanIndex, spanSizeLookup, - spanDirection, positionDirection, reverseLayout + position = focusedPosition, + spanIndex = focusedSpanIndex, + lookup = spanSizeLookup, + spanDir = getSpanDirection(forward, reverseLayout), + posDir = positionDirection, + edgePosition = edgePosition, + forward = forward, + reverseLayout = reverseLayout ) - var currentPosition = firstPositionInNextSpanGroup - if (isPositionOutOfBounds(currentPosition, edgePosition, forward)) { + if (firstPositionInNextSpanGroup == RecyclerView.NO_POSITION) { return RecyclerView.NO_POSITION } + var currentPosition = firstPositionInNextSpanGroup + // 1. If there's no cache, just return the current position that's sitting on an edge // 2. If the item takes the entire size, just return it since there's no other valid option if (cachedSpanIndex == RecyclerView.NO_POSITION @@ -154,6 +160,8 @@ internal class SpanFocusFinder { lookup: DpadSpanSizeLookup, spanDir: Int, posDir: Int, + edgePosition: Int, + forward: Boolean, reverseLayout: Boolean ): Int { var currentPos = position @@ -161,7 +169,9 @@ internal class SpanFocusFinder { val startSpanIndex = if (!reverseLayout) 0 else spanCount - 1 // First step: move to edge of current span group - while (fitsNextInCurrentSpanGroup(lookup, currentSpan, currentPos, spanDir, posDir)) { + while (!isPositionOutOfBounds(currentPos + posDir, edgePosition, forward) + && fitsNextInCurrentSpanGroup(lookup, currentSpan, currentPos, spanDir, posDir) + ) { currentPos += posDir currentSpan = getNextSpanEnd(lookup, currentSpan, currentPos, spanDir, posDir) } @@ -169,13 +179,25 @@ internal class SpanFocusFinder { // Move to next span group currentPos += posDir + if (isPositionOutOfBounds(currentPos, edgePosition, forward)) { + return RecyclerView.NO_POSITION + } + // Second step: move to start of next span group currentSpan = lookup.getCachedSpanIndex(currentPos, spanCount) - while (currentSpan != startSpanIndex && currentSpan > 0 && currentSpan < spanCount) { + while (currentSpan != startSpanIndex + && currentSpan > 0 + && currentSpan < spanCount + && !isPositionOutOfBounds(currentPos + posDir, edgePosition, forward) + ) { currentSpan += lookup.getSpanSize(currentPos) * spanDir currentPos += posDir } + if (isPositionOutOfBounds(currentPos, edgePosition, forward)) { + return RecyclerView.NO_POSITION + } + return currentPos } diff --git a/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt b/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt index baee076b..ede7aeb3 100644 --- a/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt +++ b/dpadrecyclerview/src/test/java/com/rubensousa/dpadrecyclerview/test/layoutmanager/focus/SpanFocusFinderTest.kt @@ -26,7 +26,7 @@ import org.junit.Test class SpanFocusFinderTest { private val itemCount = 1000 - private val spanCount = 5 + private var spanCount = 5 private val finder = SpanFocusFinder() private val headerPosition = 0 private val headerSpanSizeLookup = object : DpadSpanSizeLookup() { @@ -355,5 +355,57 @@ class SpanFocusFinderTest { ).isEqualTo(6) } + @Test + fun `finding next position does not query span size out of bounds`() { + spanCount = 3 + finder.setSpanCount(spanCount) + val spanSizeQueries = mutableSetOf() + val spanSizeLookup = object: DpadSpanSizeLookup() { + override fun getSpanSize(position: Int): Int { + spanSizeQueries.add(position) + return 1 + } + } + + assertThat( + finder.findNextSpanPosition( + focusedPosition = 0, + spanSizeLookup = spanSizeLookup, + forward = true, + edgePosition = 0, + reverseLayout = false + ) + ).isEqualTo(RecyclerView.NO_POSITION) + + assertThat(spanSizeQueries).hasSize(1) + assertThat(spanSizeQueries).contains(0) + } + + @Test + fun `finding previous position does not query span size out of bounds`() { + spanCount = 3 + finder.setSpanCount(spanCount) + val spanSizeQueries = mutableSetOf() + val spanSizeLookup = object: DpadSpanSizeLookup() { + override fun getSpanSize(position: Int): Int { + spanSizeQueries.add(position) + return 1 + } + } + + assertThat( + finder.findNextSpanPosition( + focusedPosition = 0, + spanSizeLookup = spanSizeLookup, + forward = false, + edgePosition = 0, + reverseLayout = false + ) + ).isEqualTo(RecyclerView.NO_POSITION) + + assertThat(spanSizeQueries).hasSize(1) + assertThat(spanSizeQueries).contains(0) + } + } From a58c4ecace7c0e22b9ac518110c194cec7261cd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BAben=20Sousa?= Date: Fri, 4 Aug 2023 21:09:11 +0200 Subject: [PATCH 5/5] Set release date --- docs/changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.md b/docs/changelog.md index 27ea547b..e0d9e60e 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -4,7 +4,7 @@ ### 1.1.0-alpha03 -2023-08-?? +2023-08-04 #### Bug fixes