google / accompanist

A collection of extension libraries for Jetpack Compose
https://google.github.io/accompanist
Apache License 2.0
7.43k stars 598 forks source link

[Pager] Pager skipping #1534

Closed lighttigerXIV closed 1 year ago

lighttigerXIV commented 1 year ago

Description When dragging the pager if user keeps the finger it will detect the change but when user releases the finger it will skip to the other page and won't detect the change.

Steps to reproduce Drag to another page and keep the finger. (This only happens when slowly dragging the pager)

Expected behavior Don't skip the page.

Additional context

I don't know if anyone has this issue but i can't find anything about this. The following gif shows what happens:

pager-skipping

andkulikov commented 1 year ago

In order to help we need to see the code with how exactly do you use the Pager, thanks

lighttigerXIV commented 1 year ago

In order to help we need to see the code with how exactly do you use the Pager, thanks

Sure

This is my Pager:

HorizontalPager(
    state = songsPager,
    count = songsPagerQueue.size,
    key = { it }

) { currentSongPage ->

    val songAlbumArt = songsPagerArts!![currentSongPage]

    Image(
        modifier = Modifier
            .fillMaxHeight()
            .aspectRatio(1f)
            .clip(RoundedCornerShape(14.dp)),
        bitmap = songAlbumArt?.asImageBitmap() ?: defaultAlbumArt.asImageBitmap(),
        colorFilter = if (songAlbumArt == null) ColorFilter.tint(MaterialTheme.colorScheme.primary) else null,
        contentDescription = "Album Art"
    )
}

This are the LaunchedEffects related to it:

LaunchedEffect(songsPagerQueue) {

    selectedSong?.let {

        songsPager.scrollToPage(mainVM.getSongsPagerPosition())
    }
}

LaunchedEffect(songsPager.currentPage) {

    selectedSong?.let {

        if (songsPager.currentPage > mainVM.getSongsPagerPosition()) {

            mainVM.selectNextSong()
        }

        if (songsPager.currentPage < mainVM.getSongsPagerPosition()) {

            mainVM.selectPreviousSong()
        }
    }
}

I don't know if it helps but my compose version is 1.4.3 and the pager version is com.google.accompanist:accompanist-pager-indicators:0.17.0

If needed i have the code in my dev branch.

andkulikov commented 1 year ago

I think it could be that the issue is related to the last LaunchedEffect. songsPager.currentPage is updated not when the swipe is finished, but when we scrolled over the half to the current page.

lighttigerXIV commented 1 year ago

I think it could be that the issue is related to the last LaunchedEffect. songsPager.currentPage is updated not when the swipe is finished, but when we scrolled over the half to the current page.

Thanks. I used the page offset instead of the current page and now it works. I will leave the code here if anyone in the future as the same issue:

LaunchedEffect(songsPager.currentPageOffset) {

  if(songsPager.currentPageOffset == 0f){

      selectedSong?.let {

          if (songsPager.currentPage > mainVM.getSongsPagerPosition()) {

              mainVM.selectNextSong()
          }

          if (songsPager.currentPage < mainVM.getSongsPagerPosition()) {

              mainVM.selectPreviousSong()
          }
      }
  }
}
andkulikov commented 1 year ago

I do not recommend exactly this solution, offset is changing every frame and this means the composition will rerun on each scroll frame, it is not efficient. Instead I recommend doing something similar to: https://github.com/google/accompanist/issues/1347#issuecomment-1257432199

lighttigerXIV commented 1 year ago

I do not recommend exactly this solution, offset is changing every frame and this means the composition will rerun on each scroll frame, it is not efficient. Instead I recommend doing something similar to: #1347 (comment)

Is this ok then?

LaunchedEffect(songsPager){
    snapshotFlow(songsPager::isScrollInProgress)
        .drop(1)
        .collect{ scrollInProgress ->

            if(!scrollInProgress){

                selectedSong?.let {

                    if (songsPager.currentPage > mainVM.getSongsPagerPosition()) {

                        mainVM.selectNextSong()
                    }

                    if (songsPager.currentPage < mainVM.getSongsPagerPosition()) {

                        mainVM.selectPreviousSong()
                    }
                }
            }
        }
}
andkulikov commented 1 year ago

yes, it is what i meant

lighttigerXIV commented 1 year ago

yes, it is what i meant

Tanks :P