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] Consider updating "Reacting to page changes" docs to simulate Foundation Pager's settledPage #1546

Closed curioustechizen closed 1 year ago

curioustechizen commented 1 year ago

Description

This documentation recommends simply using currentPage to react to page changes. However, this is not functionally equivalent to Foundation Pager's settledPage API. Specifically, you often want to react to page changes only when a page has settled.

Since Pager is now deprecated, instead of providing an equivalent of the settledPage API, would you consider updating the above documentation with a code snipper that simulated the settledPage API?

LaunchedEffect(pagerState) {
    snapshotFlow { pagerState.isScrollInProgress }
        .filter { !it }
        .drop(1) // Initial scroll is false
        .collect { AnalyticsService.sendPageSelectedEvent(pagerState.currentPage) }
}

Additional context

This difference in functionality between Accompanist Pager and Foundation Pager can have real-world consequences. Specifically, it led to a bug in Accompanist pager where a button on pages would not receive tap events (instead the pager would try to perform a DragInteraction) immediately after doing a programmatic scrollToPage.

Here is some code to demonstrate the problem

@Composable
internal fun ClickablePage(
    pageNumber: Int,
    onClick: () -> Unit,
    modifier: Modifier = Modifier
) {
    Box(
        modifier = modifier
            .fillMaxSize()
            .background(
                color = colorResource(id = R.color.teal_200),
                shape = RoundedCornerShape(16.dp)
            )
            .padding(24.dp)
    ) {
        Text(
            text = "Page number $pageNumber",
            style = MaterialTheme.typography.h4,
            textAlign = TextAlign.Center
        )
        IconButton(
            onClick = onClick,
            modifier = Modifier
                .size(88.dp)
                .background(color = colorResource(id = R.color.purple_700), shape = CircleShape)
                .align(Alignment.Center)
        ) {
            Icon(
                Icons.Filled.PlayArrow,
                contentDescription = null,
                tint = Color.White,
                modifier = Modifier.size(48.dp)
            )
        }
    }
}

@Composable
internal fun AccompanistPager(
    items: List<Int>,
    onPageClick: (Int) -> Unit,
    modifier: Modifier = Modifier
) {
    val pagerState = rememberPagerState()
    var pageToBeSelected by remember { mutableStateOf(0) }
    LaunchedEffect(pagerState) {
        snapshotFlow { pagerState.currentPage }
            .collect { pageToBeSelected = it }
    }
    LaunchedEffect(pageToBeSelected) {
        pagerState.animateScrollToPage(pageToBeSelected)
    }
    HorizontalPager(
        modifier = modifier,
        count = items.size,
        contentPadding = PaddingValues(horizontal = 24.dp),
        itemSpacing = 16.dp,
        state = pagerState
    ) { page ->
        ClickablePage(pageNumber =  items[page], onClick = { onPageClick(items[page]) })
    }
}

With this code, if you scroll to a page and then try to tap the Play button, nothing happens. You need to tap twice for it to take effect.

The problem can be fixed by simulating the settledPage API as follows

@Composable
internal fun AccompanistPager(
    items: List<Int>,
    onPageClick: (Int) -> Unit,
    modifier: Modifier = Modifier
) {
    val pagerState = rememberPagerState()
    var pageToBeSelected by remember { mutableStateOf(0) }
    LaunchedEffect(pagerState) {
        snapshotFlow { pagerState.isScrollInProgress }
            .filter { !it }
            .drop(1) // Initial scroll is false
            .collect { pageToBeSelected = pagerState.currentPage }
    }
    LaunchedEffect(pageToBeSelected) {
        pagerState.animateScrollToPage(pageToBeSelected)
    }
    HorizontalPager(
        modifier = modifier,
        count = items.size,
        contentPadding = PaddingValues(horizontal = 24.dp),
        itemSpacing = 16.dp,
        state = pagerState
    ) { page ->
        ClickablePage(pageNumber =  items[page], onClick = { onPageClick(items[page]) })
    }
}

The problem does not occur while using Foundation pager

@Composable
internal fun FoundationPager(
    items: List<Int>,
    onPageClick: (Int) -> Unit,
    modifier: Modifier = Modifier
) {
    val pagerState = rememberPagerState()
    var pageToBeSelected by remember { mutableStateOf(0) }
    LaunchedEffect(pagerState) {
        snapshotFlow { pagerState.settledPage }
            .collectLatest { pageToBeSelected = it }
    }
    LaunchedEffect(pageToBeSelected) {
        pagerState.animateScrollToPage(pageToBeSelected)
    }
    HorizontalPager(
        modifier = modifier,
        pageCount = items.size,
        contentPadding = PaddingValues(horizontal = 24.dp),
        pageSpacing = 16.dp,
        state = pagerState
    ) { page ->
        ClickablePage(pageNumber = items[page], onClick = { onPageClick(items[page]) })
    }
}
andkulikov commented 1 year ago

Thank you, but we are not planning updatinf the documentation for the deprecated library, as instead we recommend migrating to the Pager from foundation library, where there is no this issue, as you mentioned