accompanist icon indicating copy to clipboard operation
accompanist copied to clipboard

[Pager] Shows incorrect page

Open zoltish opened this issue 3 years ago • 9 comments

Description

I have a setup where I show a list of items, tapping one leads to its detail screen which is really a pager with the tapped item pre-selected.

In most cases it works fine, but Ive had an increasing amount of reports stating that the incorrect page is shown. Sometimes it happens in a row, sometimes just once; personally Ive been unable to recreate it at all.

The incorrect page is seemingly random, but whenever the bug happens in a row its always the same index. The pager leaves the composition when the user navigates back to the list screen, which imo makes it odd that the same index is still selected the next time around.

I can verify that the correct information is being sent through to the pager from my end of things. I have some additional pager logic, Ill share it below in case it helps.

Additional context

All reports thus far have been from users rocking Android 12/13 on a Pixel 6. I dont have a Pixel 6.

This is how I notify about a page being selected.

 LaunchedEffect(state) {
    snapshotFlow(state::isScrollInProgress)
        .drop(1)
        .collect { scrollInProgress ->
            if (!scrollInProgress) {
                updatedOnSelected(state.currentPage)
            }
        }
}

This is how I animate to the current page whenever its been changed by external state.

  LaunchedEffect(currentPage) {
      if (state.currentPage != currentPage && state.pageCount > 0) {
          state.animateScrollToPage(
              page = currentPage
          )
      }
  }

zoltish avatar Jul 21 '22 07:07 zoltish

Heres a video of how it looks.

zoltish avatar Jul 21 '22 10:07 zoltish

What could be a reason for this potentially is that state.pageCount is 0 when this code is executed even that this will be updated soon. pageCount is only updated when the layout pass happened. While the LaunchedEffects are executed after composition, but before layout. And in fact you don't really need to have this check with the latest version of Pager, animateScrollToPage will also automatically wait for the first layout to happen before starting scrolling as it needs the sizes calculated during the layout.

andkulikov avatar Jul 21 '22 11:07 andkulikov

That makes sense, but wouldnt it just result in the initial page (0) being shown in my example? I cant see how/why it would scroll to something like index=5 seemingly randomly.

zoltish avatar Jul 21 '22 11:07 zoltish

I don't your full code to say for sure. Just noticed this issue for now. Also how do you navigate between pages, are you sure the screen with pager is really recreated maybe it reuses the previous state?

andkulikov avatar Jul 21 '22 11:07 andkulikov

I can verify that a remember block is ran again when navigating back to the pager screen.

Im using AnimatedContent to animate between list/pager, could that have an effect on this? Its basically targetState=mode (list/pager) and in the content block a SaveableStateHolder wraps the actual content with the mode as key. When navigating back to the list, the pager state is removed via SaveableStateHolder.removeState(key).

zoltish avatar Jul 21 '22 12:07 zoltish

ok, should work then.

another idea is that maybe your scroll is canceled by some reason. to check if that happens you can try to add try/catch with CancellationException. but I assume it should be only happening if the user starts scrolling via fingers.

andkulikov avatar Jul 21 '22 13:07 andkulikov

I guess its going to be quite hard to test whether its being cancelled since I cant recreate this myself :( But Ill tweak it such that it uses a regular scrollTo for the initial call, no need to animate it at that point anyways.

Ive gotten some more feedback, and it seems to always point to the last page whenever the bug occurs, so probably something state related?

zoltish avatar Jul 22 '22 10:07 zoltish

If you don't really need an animation for the first scroll then the correct way to initialize the initial page is to pass the needed page into rememberPagerState(initialPage = ...)

andkulikov avatar Jul 22 '22 12:07 andkulikov

Ill start doing that, but I dont think that will fix the issue itself?

zoltish avatar Jul 23 '22 11:07 zoltish

I think I made a discovery!

I just noticed that a screen with a BottomSheetScaffold would always return to its last state after navigating away and coming back to it. The screen is disposed (DisposableEffect.onDispose is called).

I have a SaveableStateHolder, and temporarily removing it causes the screen to behave as expected when returning => a new scaffold state is always created. I take it the same thing is happening with the pager state, and for whatever reason that results in the pager showing the restored page rather than what my logic tells it to.

I still dont understand how this isnt affecting everyone using my app (Ive only had 2 reports thus far, and I cant recreate it whatsoever). But Im fairly certain this is the chain of events that cause it, and Ill fix my SaveableStateHolder usage.

zoltish avatar Aug 05 '22 15:08 zoltish

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.

github-actions[bot] avatar Sep 05 '22 04:09 github-actions[bot]