From 7aed481b30a1c1fe328fc7653faed5d9891f7bf6 Mon Sep 17 00:00:00 2001 From: Andrey Kulikov Date: Mon, 12 Sep 2022 19:57:35 +0100 Subject: [PATCH] [Pager] Fix pageOffset calculation when itemSpacing is not 0 --- .../com/google/accompanist/pager/Pager.kt | 23 ++----------------- .../google/accompanist/pager/PagerState.kt | 21 +++++++++-------- .../pager/BaseHorizontalPagerTest.kt | 11 +++++---- .../pager/BaseVerticalPagerTest.kt | 11 +++++---- .../com/google/accompanist/pager/PagerTest.kt | 7 +++--- 5 files changed, 31 insertions(+), 42 deletions(-) diff --git a/pager/src/main/java/com/google/accompanist/pager/Pager.kt b/pager/src/main/java/com/google/accompanist/pager/Pager.kt index 0dac998dc..d32810ee6 100644 --- a/pager/src/main/java/com/google/accompanist/pager/Pager.kt +++ b/pager/src/main/java/com/google/accompanist/pager/Pager.kt @@ -26,7 +26,6 @@ import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.calculateEndPadding -import androidx.compose.foundation.layout.calculateStartPadding import androidx.compose.foundation.layout.wrapContentSize import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.LazyRow @@ -42,7 +41,6 @@ import androidx.compose.ui.input.nestedscroll.NestedScrollConnection import androidx.compose.ui.input.nestedscroll.NestedScrollSource import androidx.compose.ui.input.nestedscroll.nestedScroll import androidx.compose.ui.platform.LocalDensity -import androidx.compose.ui.platform.LocalLayoutDirection import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.LayoutDirection import androidx.compose.ui.unit.Velocity @@ -53,7 +51,6 @@ import dev.chrisbanes.snapper.SnapperFlingBehavior import dev.chrisbanes.snapper.SnapperFlingBehaviorDefaults import dev.chrisbanes.snapper.SnapperLayoutInfo import dev.chrisbanes.snapper.rememberSnapperFlingBehavior -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.filter @@ -357,24 +354,8 @@ internal fun Pager( .collect { state.updateCurrentPageBasedOnLazyListState() } } val density = LocalDensity.current - val layoutDirection = LocalLayoutDirection.current - LaunchedEffect(density, contentPadding, isVertical, layoutDirection, reverseLayout, state) { - with(density) { - // this should be exposed on LazyListLayoutInfo instead. b/200920410 - state.afterContentPadding = if (isVertical) { - if (!reverseLayout) { - contentPadding.calculateBottomPadding() - } else { - contentPadding.calculateTopPadding() - } - } else { - if (!reverseLayout) { - contentPadding.calculateEndPadding(layoutDirection) - } else { - contentPadding.calculateStartPadding(layoutDirection) - } - }.roundToPx() - } + LaunchedEffect(density, state, itemSpacing) { + with(density) { state.itemSpacing = itemSpacing.roundToPx() } } val pagerScope = remember(state) { PagerScopeImpl(state) } diff --git a/pager/src/main/java/com/google/accompanist/pager/PagerState.kt b/pager/src/main/java/com/google/accompanist/pager/PagerState.kt index 89c6fee2c..a2fb3307f 100644 --- a/pager/src/main/java/com/google/accompanist/pager/PagerState.kt +++ b/pager/src/main/java/com/google/accompanist/pager/PagerState.kt @@ -85,12 +85,15 @@ class PagerState( val layoutInfo = lazyListState.layoutInfo return layoutInfo.visibleItemsInfo.maxByOrNull { val start = maxOf(it.offset, 0) - val end = minOf(it.offset + it.size, layoutInfo.viewportEndOffset - afterContentPadding) + val end = minOf( + it.offset + it.size, + layoutInfo.viewportEndOffset - layoutInfo.afterContentPadding + ) end - start } } - internal var afterContentPadding = 0 + internal var itemSpacing by mutableStateOf(0) private val currentPageLayoutInfo: LazyListItemInfo? get() = lazyListState.layoutInfo.visibleItemsInfo.lastOrNull { @@ -138,9 +141,7 @@ class PagerState( */ val currentPageOffset: Float by derivedStateOf { currentPageLayoutInfo?.let { - // We coerce since itemSpacing can make the offset > 1f. - // We don't want to count spacing in the offset so cap it to 1f - (-it.offset / it.size.toFloat()).coerceIn(-1f, 1f) + (-it.offset / (it.size + itemSpacing).toFloat()).coerceIn(-0.5f, 0.5f) } ?: 0f } @@ -232,11 +233,11 @@ class PagerState( // offset from the size lazyListState.animateScrollToItem( index = page, - scrollOffset = (target.size * pageOffset).roundToInt() + scrollOffset = ((target.size + itemSpacing) * pageOffset).roundToInt() ) } else if (layoutInfo.visibleItemsInfo.isNotEmpty()) { // If we don't, we use the current page size as a guide - val currentSize = layoutInfo.visibleItemsInfo.first().size + val currentSize = layoutInfo.visibleItemsInfo.first().size + itemSpacing lazyListState.animateScrollToItem( index = page, scrollOffset = (currentSize * pageOffset).roundToInt() @@ -245,13 +246,13 @@ class PagerState( // The target should be visible now target = lazyListState.layoutInfo.visibleItemsInfo.firstOrNull { it.index == page } - if (target != null && target.size != currentSize) { + if (target != null && target.size + itemSpacing != currentSize) { // If the size we used for calculating the offset differs from the actual // target page size, we need to scroll again. This doesn't look great, // but there's not much else we can do. lazyListState.animateScrollToItem( index = page, - scrollOffset = (target.size * pageOffset).roundToInt() + scrollOffset = ((target.size + itemSpacing) * pageOffset).roundToInt() ) } } @@ -291,7 +292,7 @@ class PagerState( if (pageOffset.absoluteValue > 0.0001f) { currentPageLayoutInfo?.let { scroll { - scrollBy(it.size * pageOffset) + scrollBy((it.size + itemSpacing) * pageOffset) } } } diff --git a/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseHorizontalPagerTest.kt b/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseHorizontalPagerTest.kt index d4da9ee48..62e64e479 100644 --- a/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseHorizontalPagerTest.kt +++ b/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseHorizontalPagerTest.kt @@ -75,7 +75,8 @@ abstract class BaseHorizontalPagerTest( override fun SemanticsNodeInteraction.assertLaidOutItemPosition( page: Int, currentPage: Int, - offset: Float, + pageCount: Int, + offset: Float ): SemanticsNodeInteraction { val rootBounds = composeTestRule.onRoot().getUnclippedBoundsInRoot() val expectedItemSize = ( @@ -83,6 +84,7 @@ abstract class BaseHorizontalPagerTest( contentPadding.calculateLeftPadding(layoutDirection) - contentPadding.calculateRightPadding(layoutDirection) ) * itemWidthFraction + val expectedItemSizeWithSpacing = expectedItemSize + itemSpacingDp.dp // The expected coordinates. This uses the implicit fact that VerticalPager by // use Alignment.CenterVertically by default, and that we're using items @@ -94,16 +96,17 @@ abstract class BaseHorizontalPagerTest( expectedItemSize - contentPadding.calculateRightPadding(layoutDirection) ) + - (expectedItemSize * offset) + (expectedItemSizeWithSpacing * offset) } else { - contentPadding.calculateLeftPadding(layoutDirection) - (expectedItemSize * offset) + contentPadding.calculateLeftPadding(layoutDirection) - + (expectedItemSizeWithSpacing * offset) } return assertWidthIsEqualTo(expectedItemSize) .assertHeightIsAtLeast(expectedItemSize) .assertTopPositionInRootIsEqualTo(expectedTop) .run { - val pageDelta = ((expectedItemSize + itemSpacingDp.dp) * (page - currentPage)) + val pageDelta = (expectedItemSizeWithSpacing * (page - currentPage)) if (laidOutRtl) { assertLeftPositionInRootIsEqualTo(expectedFirstItemLeft - pageDelta) } else { diff --git a/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseVerticalPagerTest.kt b/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseVerticalPagerTest.kt index 62c180b1b..79b538f28 100644 --- a/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseVerticalPagerTest.kt +++ b/pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseVerticalPagerTest.kt @@ -65,25 +65,28 @@ abstract class BaseVerticalPagerTest( override fun SemanticsNodeInteraction.assertLaidOutItemPosition( page: Int, currentPage: Int, - offset: Float, + pageCount: Int, + offset: Float ): SemanticsNodeInteraction { val rootBounds = composeTestRule.onRoot().getUnclippedBoundsInRoot() val expectedItemHeight = rootBounds.height - contentPadding.calculateTopPadding() - contentPadding.calculateBottomPadding() + val expectedItemHeightWithSpacing = expectedItemHeight + itemSpacingDp.dp val expectedItemWidth = rootBounds.width val expectedLeft = (rootBounds.width - expectedItemWidth) / 2 val expectedFirstItemTop = when (reverseLayout) { - true -> (rootBounds.height - contentPadding.calculateBottomPadding() - expectedItemHeight) + (expectedItemHeight * offset) - false -> contentPadding.calculateTopPadding() - (expectedItemHeight * offset) + true -> (rootBounds.height - contentPadding.calculateBottomPadding() - expectedItemHeight) + + (expectedItemHeightWithSpacing * offset) + false -> contentPadding.calculateTopPadding() - (expectedItemHeightWithSpacing * offset) } return assertWidthIsEqualTo(expectedItemWidth) .assertHeightIsAtLeast(expectedItemHeight) .assertLeftPositionInRootIsEqualTo(expectedLeft) .run { - val pageDelta = ((expectedItemHeight + itemSpacingDp.dp) * (page - currentPage)) + val pageDelta = (expectedItemHeightWithSpacing * (page - currentPage)) if (reverseLayout) { assertTopPositionInRootIsEqualTo(expectedFirstItemTop - pageDelta) } else { diff --git a/pager/src/sharedTest/kotlin/com/google/accompanist/pager/PagerTest.kt b/pager/src/sharedTest/kotlin/com/google/accompanist/pager/PagerTest.kt index f7c869650..29068ee56 100644 --- a/pager/src/sharedTest/kotlin/com/google/accompanist/pager/PagerTest.kt +++ b/pager/src/sharedTest/kotlin/com/google/accompanist/pager/PagerTest.kt @@ -620,14 +620,14 @@ abstract class PagerTest { // Assert that the 'current page' exists and is laid out in the correct position composeTestRule.onNodeWithTag(currentPage.toString()) .assertExists() - .assertLaidOutItemPosition(currentPage, currentPage, offset) + .assertLaidOutItemPosition(currentPage, currentPage, pageCount, offset) // Go through all of the pages, and assert the expected layout state (if it exists) (0 until pageCount).forEach { page -> // If this exists assert that it is laid out in the correct position composeTestRule.onNodeWithTag(page.toString()).apply { if (exists && isLaidOut) { - assertLaidOutItemPosition(page, currentPage, offset) + assertLaidOutItemPosition(page, currentPage, pageCount, offset) } } } @@ -636,7 +636,8 @@ abstract class PagerTest { protected abstract fun SemanticsNodeInteraction.assertLaidOutItemPosition( page: Int, currentPage: Int, - offset: Float, + pageCount: Int, + offset: Float ): SemanticsNodeInteraction private fun setPagerContent(