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.

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

Desktop pager physics #3447

Closed
alexzhirkevich opened this issue Aug 2, 2023 · 8 comments · Fixed by JetBrains/compose-multiplatform-core#1100
Closed

Desktop pager physics #3447

alexzhirkevich opened this issue Aug 2, 2023 · 8 comments · Fixed by JetBrains/compose-multiplatform-core#1100
Assignees
Labels
enhancement New feature or request

Comments

@alexzhirkevich
Copy link
Contributor

alexzhirkevich commented Aug 2, 2023

For some reason desktop and web pagers don't align with next or prev page after end of a scroll. Compose version doesn't matter. It was from the 1st version that includes pager.

It that an intended behaviour or a bug?

Screen.Recording.2023-08-02.at.10.30.41.mov

I expect it to work like on Android/iOS:

Screen.Recording.2023-08-02.at.10.51.48.mov
@alexzhirkevich alexzhirkevich added enhancement New feature or request submitted labels Aug 2, 2023
@eymar
Copy link
Collaborator

eymar commented Aug 3, 2023

How did you provide the input on the desktop? I guess by using a mouse wheel?

It seems that the pager implementaion is expecting some kind of a gesture (a fling, swipe, drag). The desktop's use case was not properly thought of.
Ideally, pager should never stop between two pages (by design I guess). So it's something that we can improve.

@garretyoder
Copy link

garretyoder commented Oct 4, 2023

I wrote a quick very dirty workaround to achieve this behavior by listening to the scroll offset and setting up both overscroll thresholds and a coroutine timeout that will kick it back to the original page if there is no further input.

var sJob: Job? = null
LaunchedEffect(pagerState) {
    snapshotFlow { pagerState.currentPageOffsetFraction }.collect { offset ->
        if (!pagerState.isScrollInProgress) {
            sJob?.cancel()
            if (offset > 0.2f) {
                pagerState.animateScrollToPage(pagerState.currentPage+1)
            } else if (offset < -0.2f) {
                pagerState.animateScrollToPage(pagerState.currentPage-1)
            } else if (offset!=0f) {
                sJob = scope.launch {
                    delay(500)
                    pagerState.animateScrollBy((pagerHeight*offset)*-1)
                }
            }
        }
    }
}

Please feel free to use the code/merge it, but I'm sure there is a cleaner solution - this was just my dirty five minute fix.

@HuixingWong
Copy link

Any update guys ?

@HuixingWong
Copy link

Maybe the reason is #653

@dima-avdeev-jb
Copy link
Contributor

@HuixingWong Sorry, no updates yet

@arkivanov
Copy link
Contributor

Also, while scrolling via Shift + Mouse wheel, PagerState#currentPage is always behind PagerState#targetPage by one, i.e. currentPage always equals to targetPage - 1.

@gleb-skobinsky
Copy link

gleb-skobinsky commented Nov 6, 2023

The workaround posted above did not work well enough for me (if the automatic fling is still in progress and you try to scroll the pager, it breaks and stops adjusting automatically), but I found my own dirty fix, maybe it will help some folks here.
The first step is to detect when the user stops scrolling. Ideally it should work like the onDragStopped of the draggable Modifier.
We can try to detect stop of the scroll like this:

// common
actual fun Modifier.onScrollCancel(action: () -> Unit): Modifier 
// desktop and JS
@OptIn(ExperimentalComposeUiApi::class)
actual fun Modifier.onScrollCancel(action: () -> Unit): Modifier = composed {
    var currentEventCount by remember { mutableStateOf(0) }
    LaunchedEffect(currentEventCount) {
        if (currentEventCount != 0) {
            delay(50L)
            action()
        }
    }
    return@composed onPointerEvent(PointerEventType.Scroll) {
        currentEventCount += 1
    }
}
// iOS and Android
actual fun Modifier.onScrollCancel(action: () -> Unit): Modifier = this

Then the snapping behaviour can be reproduced like this:

private var sJob: Job? = null

@OptIn(ExperimentalFoundationApi::class)
fun Modifier.desktopSnapFling(pagerState: PagerState, scrollScope: CoroutineScope) = onScrollCancel {
    val offset = pagerState.currentPageOffsetFraction
    if (!pagerState.isScrollInProgress) {
        sJob?.cancel()
        if (offset > 0.2f) {
            sJob = scrollScope.launch {
                pagerState.animateScrollToPage(pagerState.currentPage + 1)
            }
        } else if (offset < -0.2f) {
            sJob = scrollScope.launch {
                pagerState.animateScrollToPage(pagerState.currentPage - 1)
            }
        } else if (offset != 0f) {
            sJob = scrollScope.launch {
                pagerState.animateScrollToPage(pagerState.currentPage, 0f)
            }
        }
    }
}

Finally, we can apply the modifier to any composable that forms the content of the pager:

val scope = rememberCoroutineScope()
...
Image(
    bitmap = image.imageBitmap,
    contentDescription = "Gallery view image",
    contentScale = ContentScale.Fit,
    modifier = Modifier
        .fillMaxSize()
        .desktopSnapFling(pagerState, scope)
)

The main disadvantage of this is that the snap is based on the offset fraction and not scroll velocity. This is not in line with the mobile implementations, if I'm not mistaken.

MatkovIvan added a commit to JetBrains/compose-multiplatform-core that referenced this issue Feb 14, 2024
## Proposed Changes

- Based on #1055
- Trigger `onScrollStopped` callback after mouse wheel scroll
- Ignore `ScrollableDefaultFlingBehavior`s during mouse scroll to avoid
breaking changes
- Remove scrolling via `dispatchRawDelta`, `isSmoothScrollingEnabled =
false` will just disable animation instead
- Add pager test page in mpp demo

## Testing

Test: run pager page in mpp demo

## Issues Fixed

Fixes JetBrains/compose-multiplatform#3447
Fixes JetBrains/compose-multiplatform#3454
@okushnikov
Copy link

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants