rememberPagerState doesn't allow a reset on pageCount changed

[ch4rl3x]

Describe the bug

pagerTabIndicatorOffset crashes when page count changes.

Expected behavior

No crash should occur when page count changes

Additional context

rememberPagerState should be recreated on data changed. Use of the missing paramter inputs in rememberSavable fixed the issue.

inputs - A set of inputs such that, when any of them have changed, will cause the state to reset and init to be rerun

[andkulikov]

I think it is not a pattern we want to follow, none of our other rememberFoo() functions in Compose UI have the inputs param, which can feel confusing given that remember() and rememberSaveable() do have such param. However in reality this is added mostly for making the common use cases simpler, but in fact
remember(input1) { Foo() }
is the same as
key(input1) { remember { Foo() } }
so you can write something like
val pagerState = key(yourInput) { rememberPagerState() }

Also, getting back to your use case, are you sure you want to completely reset the state when the pageCount changes? Imagine the case where you had 10 items and was displaying the 5th item after scroll. Then a new item was added in the end so you now have 11 items, if you reset the state you will end up displaying the 1th item, didn't you want to continue displaying the 5th item? I think Pager state will be updated with the new pageCount once the next measurement happens. Can you maybe a share a whole sample which crashes?

[ch4rl3x]

Hey,

in advance, the solution val pagerState = key(yourInput) { rememberPagerState() } works

Here is a simple example which results in the crash when you scroll to page > "Test2" and press the button. In my real world szenario i have a sreen, which shows multiple graphs (AndroidView+MPAndroidChart) for measurement values for a selected aquarium. You are able to select another aquarium in the same screen. The new one could possible have less graphs to show. I attached a screenshot of the real app. If i select the aquarium "Test Aquarium", the app crashes because the new aquarium only has one graph to show.

Crash example:

var listItems by remember { mutableStateOf(listOf("Test1", "Test2", "Test3", "Test4", "Test5")) }

val state = rememberPagerState()
val coroutineScope = rememberCoroutineScope()

Surface(
    modifier = Modifier.statusBarsPadding()
) {
    Column(
        modifier = Modifier.fillMaxSize()
    ) {
        Button(
            modifier = Modifier.padding(bottom = 10.dp),
            onClick = {
                listItems = listOf("Test New 1", "Test New 2")
            }
        ) {
            Text("Load another context")
        }
        ScrollableTabRow(
            selectedTabIndex = state.currentPage,
            indicator = { tabPositions ->
                TabRowDefaults.Indicator(
                    modifier = Modifier.pagerTabIndicatorOffset(state, tabPositions),
                )
            }
        ) {
            // Add tabs for all of our pages
            listItems.forEachIndexed { index, s ->
                Tab(
                    text = {
                        Text(
                            text = s
                        )
                    },
                    selected = state.currentPage == index,
                    onClick = {
                        // Animate to the selected page when clicked
                        coroutineScope.launch {
                            state.animateScrollToPage(index)
                        }
                    }
                )
            }
        }

        HorizontalPager(
            count = listItems.count(),
            state = state,
        ) { page ->
            Text(text = "Content for page: ${page}")
        }
    }
}

The crash:

java.lang.IndexOutOfBoundsException: Index: 4, Size: 2
        at java.util.ArrayList.get(ArrayList.java:437)
        at com.google.accompanist.pager.PagerTabKt$pagerTabIndicatorOffset$1.invoke(PagerTab.kt:52)
        at com.google.accompanist.pager.PagerTabKt$pagerTabIndicatorOffset$1.invoke(PagerTab.kt:45)
        at androidx.compose.ui.ComposedModifierKt$materialize$result$1.invoke(ComposedModifier.kt:73)
        at androidx.compose.ui.ComposedModifierKt$materialize$result$1.invoke(ComposedModifier.kt:68)
        at androidx.compose.ui.Modifier$Element$DefaultImpls.foldIn(Modifier.kt:107)
        at androidx.compose.ui.ComposedModifier.foldIn(ComposedModifier.kt:46)
        at androidx.compose.ui.CombinedModifier.foldIn(Modifier.kt:149)
        at androidx.compose.ui.CombinedModifier.foldIn(Modifier.kt:149)
        at androidx.compose.ui.CombinedModifier.foldIn(Modifier.kt:149)
        at androidx.compose.ui.ComposedModifierKt.materialize(ComposedModifier.kt:68)
        at androidx.compose.ui.layout.LayoutKt$materializerOf$1.invoke-Deg8D_g(Layout.kt:177)
        at androidx.compose.ui.layout.LayoutKt$materializerOf$1.invoke(Layout.kt:176)
        at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:116)
        at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:34)
        at androidx.compose.foundation.layout.BoxKt.Box(Box.kt:348)
        at androidx.compose.material.TabRowDefaults.Indicator-9IZ8Weo(TabRow.kt:379)
        at de.charlex.aquacalculator.MainActivity$onResume$1$1$1$2.invoke(MainActivity.kt:155)

[andkulikov]

Thanks, we need to fix it in Modifier.pagerTabIndicatorOffset() to not crash in such situations. It is possible that the PagerState is not aware about the change in the data set so will have an incorrect index. We first need to use minOf(tabPositions.lastIndex, pagerState.currentPage); second to move the offset calculation out of the composition via using the other Modifier.offset() overload which accepts lambda

[ch4rl3x]

I think the main reason for these issue is that pagerState.currentPage stays at index 4 (If you selected the 5th tab) even if the the list size changed to 3. If you use minOf(tabPositions.lastIndex, pagerState.currentPage), the issue seems to be fixed, because the TabIndicator doesn't crash anymore (tab 3 will be selected), but if you change the list item size back to 5, the selection jumps back to index 4 BUT the HorizontalPager stays at index 2, which seems to be the correct behaviour for the HorizontalPager.

Increase the item count -> index stays at the same. Decrease item count -> index decreased until the highest possible index.

The pagerState.currentPage stays at index 4 all the time

[andkulikov]

Indeed, I agree it is wrong. pagerState.currentPage should be updated in this situation. We currently update this variable only after scroll finished, but we need to also update it when the items count changes

[ch4rl3x]

Would be

LaunchedEffect(count) {
    state.currentPage = minOf(count-1, state.currentPage)
}

the correct part within the Pager?

[andkulikov]

Yes, it is one of the ways to solve that. Feel free to create a pull request

[ch4rl3x]

#783