[Pager] Request for keeping targetPage api

[ekeitho]

This commit is removing targetPage which we were using!

Describe the bug

Over here at Square we are building our own version of Tabs and it is packaged with the HorizontalPager for showing the content for each tab. We have a requirement when the user is swiping the pager, we want the indicator under the tab to animate as the user is swiping. (gif below)

We were using the targetPage as a look-ahead value which helped us understand the direction the user was scrolling & then used that to identify what tab/page the user was scrolling towards. With this we are able to animate the indicator under the Tab as the user is swiping. Without this value, we'd need to essentially copy the code that's being removed.

Expected behavior

To keep the targetPage api.

Screenshots?

Environment:

• Android OS version: 12
• Device: Emulator, Pixel XL
• Accompanist version: 0.24.13-rc

Additional context

Here's some code of it's usage, hopefully this helps. :)

// called in a DrawScope
if (pagerState.isScrollInProgress) {
   val isSwipingForward = pagerState.currentPageOffset > 0
   // tabRanges is a list of IntRanges which specifies the start & end values of each tab
   val upcomingTabRange = tabRanges[pagerState.targetPage]
   val currentTabRange = tabRanges[pagerState.currentPage]
   val currentRangeTabWidth = currentTabRange.let { it.last - it.first }
   val nextRangeTabWidth = upcomingTabRange.let { it.last - it.first }
   val widthDifference = currentRangeTabWidth - nextRangeTabWidth
   // basically a percentage of completion towards the target page
   // don't worry we handle / 0 - just a simplified example to communicate usage
   val fraction = pagerState.currentPageOffset.absoluteValue / (pagerState.currentPage - pagerState.targetPage).absoluteValue

      .....
      // use these values to animate the indicator position & indicator width as user scrolls
      .....
    }
  }

[Nek-12]

I'm using targetPage to create a two-way pager connection that both emits user page changes to the viewmodel and scrolls automatically to the page if it has been changed in a viewmodel or if the user has clicked a tab. Removing targetPage api will force me to use currentPage + debounce since currentPage emits intermediate values during the animation of the pager, which breaks scrolling halfway through the animation.

[dmitrytavpeko]

I'm using targetPage to load data on pages as the user scrolls, which is perfect for this use case. Example:

LaunchedEffect(pagerState) {
     snapshotFlow { pagerState.targetPage }.collect { targetPageIndex ->
         stateHolder.onPageRequested(targetPageIndex)
     }
}

If targetPage is removed, I will need to do something like this to achive the same behaviour:

LaunchedEffect(pagerState) {
     snapshotFlow { pagerState.currentPage to pagerState.currentPageOffset}.collect { currentPageWithOffset ->
         val targetPageIndex = 'extra logic here'
         stateHolder.onPageRequested(targetPageIndex)
     }
}

[andkulikov]

The actual idea of deprecating this param was the idea that now I think that having "animationTargetPage" and "flingAnimationTarget" here was a mistake and overcomplication. And if we remove that part then the logic is essentially just val targetPage: Int get() = currentPageOffset.absoluteValue < 0.001f -> currentPage // If we're offset towards the start, guess the previous page currentPageOffset < 0f -> (currentPage - 1).coerceAtLeast(0) // If we're offset towards the end, guess the next page else -> (currentPage + 1).coerceAtMost(pageCount - 1) } This is a pretty simple logic which is not using any internal apis, so it could be an extension function you can write in your code if you think you need something like that

[dmitrytavpeko]

I would say animationTargetPage and flingAnimationTarget are more important than the logic in "when".

Here's a demo of the proposed solution:

val PagerState.newTargetPage: Int
    get() = when {
        // If a scroll isn't in progress, return the current page
        !isScrollInProgress -> currentPage
        // If the offset is 0f (or very close), return the current page
        currentPageOffset.absoluteValue < 0.001f -> currentPage
        // If we're offset towards the start, guess the previous page
        currentPageOffset < 0f -> (currentPage - 1).coerceAtLeast(0)
        // If we're offset towards the end, guess the next page
        else -> (currentPage + 1).coerceAtMost(pageCount - 1)
    }
    

@Composable
private fun TestContent() {
    val pagerState = rememberPagerState()
    HorizontalPager(count = 2, modifier = Modifier.fillMaxSize(), state = pagerState) { page ->
        Text(text = page.toString())
    }

    LaunchedEffect(pagerState) {
        snapshotFlow { pagerState.targetPage }.collect {
            Log.d("PAGERTEST", "Target page = ${pagerState.targetPage}")
        }
    }
    LaunchedEffect(pagerState) {
        snapshotFlow { pagerState.currentPage to pagerState.currentPageOffset }.collect {
            Log.d(
                "PAGERTEST",
                "Current page = ${it.first}, offset = ${it.second}. New target page = ${pagerState.newTargetPage}"
            )
        }
    }
}

When you scroll from the first page to the second you will see the following logs: D/PAGERTEST: Target page = 0 D/PAGERTEST: Current page = 0, offset = 0.0. New target page = 0 D/PAGERTEST: Target page = 1 D/PAGERTEST: Current page = 0, offset = 0.009027778. New target page = 1 D/PAGERTEST: Current page = 0, offset = 0.05347222. New target page = 1 D/PAGERTEST: Current page = 0, offset = 0.12777779. New target page = 1 D/PAGERTEST: Current page = 1, offset = -0.2625. New target page = 0 D/PAGERTEST: Current page = 1, offset = -0.06736111. New target page = 0 D/PAGERTEST: Current page = 1, offset = 0.0. New target page = 1

As you can see, the logic does not work.

In order to calculate the targetPage properly, we need to know the scrolling direction. If you want to get rid of animationTargetPage and flingAnimationTarget, I see these options:

1. remove targetPage and that's it. This option makes the users to determine scrolling direction in the apps manually. For expample, we can combine two latest emits from flow to calculate diff in offset. I believe it's not very efficient and in the end I would come to you with feature request 😄
2. remove targetPage and expose the scrolling direction. We can install something like "scrolling observer" on the underlying lazy list and expose scrolling state:

enum class ScrollingState {
   Idle,
   ScrollingBackward,
   ScrollingForward,
}

isScrollingInProgress can be replaced by this state as well.

3. Keep targetPage 😄

[andkulikov]

I think you can reproduce what you just reproduced even with the current implementation of targetPage. Because flingAnimationTarget is only available when the fling is happening. if you overscroll through the center of the next page without flinging the target page will still point to the previous page after that. I see what you mean and maybe scrollingDirection is indeed what we need. It just could be not accurate as well in some cases when the system records some small noisy scroll in the backward direction while in fact user though they scroll in the forward direction. It could happen during the slow scroll I think, because of how the finger is moving.

[dmitrytavpeko]

You are right, thanks for pointing this out. As for scrollingDirection, I would implement it this way (just a prototype):

enum class ScrollingDirection {
    Backward,
    Forward
}

private const val NoisyOffsetThreshold: Float = 4f
private const val NoisyOffsetAccumulatorThreshold: Float = NoisyOffsetThreshold * 3

private var noisyOffsetAccumulator: Float = 0f
private var lastScrollOffset: Float by mutableStateOf(0f)

val scrollingDirection: ScrollingDirection by derivedStateOf {
    if (lastScrollOffset > 0) ScrollingDirection.Backward else ScrollingDirection.Forward
}

@Composable
private fun TestContent() {
    val lazyListState = rememberLazyListState()
    LazyRow(
        modifier = Modifier
            .fillMaxSize()
            .nestedScroll(object : NestedScrollConnection {
                override fun onPreScroll(available: Offset, source: NestedScrollSource): Offset {
                    // TODO: Calculate scrolling direction based on list type (horizontal or vertical) and LTR-RTL, if necessary.
                    val offset = available.x
                    if (offset < NoisyOffsetThreshold) {
                        noisyOffsetAccumulator += offset
                        if (noisyOffsetAccumulator.absoluteValue > NoisyOffsetAccumulatorThreshold) {
                            lastScrollOffset = noisyOffsetAccumulator
                            noisyOffsetAccumulator = 0f
                        }
                    } else {
                        noisyOffsetAccumulator = 0f
                        lastScrollOffset = offset
                    }

                    return Offset.Zero
                }
            }), state = lazyListState
    ) {
        items(100) { index ->
            Text(text = index.toString(), modifier = Modifier.fillParentMaxSize())
        }
    }

    LaunchedEffect(lazyListState) {
        snapshotFlow { lazyListState.isScrollInProgress to scrollingDirection }.collect {
            Log.d(
                "PAGERTEST",
                "Scrolling in progress = ${it.first}, scrolling direction = ${it.second}"
            )
        }
    }
}

Does that make sense or did you mean something advanced?

[ekeitho]

This is a pretty simple logic which is not using any internal apis, so it could be an extension function you can write in your code if you think you need something like that

@andkulikov i hear that and we can definitely do that, but if it's already valuable to a set of people - does it makes sense as it scales for lets say 100 of people to have an exact copy of this extension function?

[andkulikov]

@dmitrytavpeko I meant something like this:

@ExperimentalPagerApi
@Composable
fun PagerState.scrollDirection(): State<ScrollingDirection> {
    return produceState(initialValue = ScrollingDirection.Backward) {
        var lastPosition = currentPage + currentPageOffset
        snapshotFlow { currentPage + currentPageOffset }
            .collect { position ->
                if (position > lastPosition) {
                    value = ScrollingDirection.Forward
                } else if (position < lastPosition) {
                    value = ScrollingDirection.Backward
                }
                lastPosition = position
            }
    }
}

If you use nestedScroll this will not be covering scrolling happening programmatically

[andkulikov]

This is a pretty simple logic which is not using any internal apis, so it could be an extension function you can write in your code if you think you need something like that

@andkulikov i hear that and we can definitely do that, but if it's already valuable to a set of people - does it makes sense as it scales for lets say 100 of people to have an exact copy of this extension function?

So there are two issues with targetPage now:

1. It uses "animationTargetPage" and "flingAnimationTarget" which we don't want to have anymore
2. It just doesn't work as users expected, see https://github.com/google/accompanist/issues/1245#issuecomment-1191344726 Maybe scrollDirection is the api we need to expose instead, agree. But in fact it will be useful not only for Pager, so I filed it as a feature request for Compose UI, feel free to star: https://issuetracker.google.com/issues/240411671. In the meantime we can detect it as I showed in https://github.com/google/accompanist/issues/1245#issuecomment-1196793022

[dmitrytavpeko]

@dmitrytavpeko I meant something like this:

@ExperimentalPagerApi
@Composable
fun PagerState.scrollDirection(): State<ScrollingDirection> {
    return produceState(initialValue = ScrollingDirection.Backward) {
        var lastPosition = currentPage + currentPageOffset
        snapshotFlow { currentPage + currentPageOffset }
            .collect { position ->
                if (position > lastPosition) {
                    value = ScrollingDirection.Forward
                } else if (position < lastPosition) {
                    value = ScrollingDirection.Backward
                }
                lastPosition = position
            }
    }
}

If you use nestedScroll this will not be covering scrolling happening programmatically

Nice, thank you! Using this scrollDirection, currentPage, and currentPageOffset we can predict the next page. Considering that scrollDirection may be exposed by the LazyList in the future, I would like to have this api in the library. I'm not sure it's a good idea at the moment because for my use case (predict the target page to load data on demand) I will use scrollDirection in combination with currentPage and currentPageOffset anyway. So we can kill two birds with one stone: get rid of animationTargetPage and flingAnimationTarget and keep targetPage 😄

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.