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.

Sign up for GitHub

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

Jump to bottom

[Pager] Fix pageOffset calculation when itemSpacing is not 0 #1333

Merged
merged 1 commit into from Sep 12, 2022
Merged

[Pager] Fix pageOffset calculation when itemSpacing is not 0 #1333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 2 additions & 21 deletions pager/src/main/java/com/google/accompanist/pager/Pager.kt
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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) }
Expand Down
21 changes: 11 additions & 10 deletions pager/src/main/java/com/google/accompanist/pager/PagerState.kt
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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()
Expand All @@ -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()
)
}
}
Expand Down Expand Up @@ -291,7 +292,7 @@ class PagerState(
if (pageOffset.absoluteValue > 0.0001f) {
currentPageLayoutInfo?.let {
scroll {
scrollBy(it.size * pageOffset)
scrollBy((it.size + itemSpacing) * pageOffset)
}
}
}
Expand Down
11 changes: 7 additions & 4 deletions pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseHorizontalPagerTest.kt
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
11 changes: 7 additions & 4 deletions pager/src/sharedTest/kotlin/com/google/accompanist/pager/BaseVerticalPagerTest.kt
View file
Open in desktop
Expand Up @@ -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 {
Expand Down
7 changes: 4 additions & 3 deletions pager/src/sharedTest/kotlin/com/google/accompanist/pager/PagerTest.kt
View file
Open in desktop
Expand Up @@ -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)
}
}
}
Expand All @@ -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(
Expand Down