Merge pull request #1333 from andkulikov/main

[Pager] Fix pageOffset calculation when itemSpacing is not 0

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) }

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)
                     }
                 }
             }

pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseHorizontalPagerTest.kt

@@ -75,14 +75,16 @@ 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 = (
             rootBounds.width -
                 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 {

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 {

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(