HorizontalPager and PagerState don't expose pageCount [266965072]

Fixed

Bug

Status Update

No update yet.

Description

yu...@schimke.ee

created issue #1

Jan 29, 2023 04:13PM

Jetpack Compose version: 1.4.0-alpha05
Jetpack Compose component used: compose foundation

It's not possible to migrate off Accompanist Horizontal Pager to Compose Foundation, because the page count is not exposed as state.

https://github.com/google/horologist/blob/0ba46cee202c84154b4e20db4850d29757c7e776/compose-layout/src/main/java/com/google/android/horologist/compose/pager/PagerScreen.kt#L99

This is needed to implement androidx.wear.compose.material.PageIndicatorState from Wear Compose

Comments

ma...@google.com <ma...@google.com> Jan 30, 2023 09:41AM

Reassigned to le...@google.com.

le...@google.com <le...@google.com> #2Jan 30, 2023 11:15AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019

Change the way to detect mismatch between POJO and query

This fixes cursor mismatch warnings with expandProjection.

Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8

M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt

https://android-review.googlesource.com/1123258

https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1

yu...@schimke.ee <yu...@schimke.ee> #3Jan 30, 2023 12:14PM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

le...@google.com <le...@google.com> #4Jan 30, 2023 02:08PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020

Re-factor how errors are dismissed when query is re-written

This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (

b/140759491

).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.

This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.

Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f

M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt

https://android-review.googlesource.com/1288456

yu...@schimke.ee <yu...@schimke.ee> #5Jan 30, 2023 06:49PM

I'm using rememberUpdatedState for the count now, so I'm certainly not blocked.

https://github.com/google/horologist/blob/4a9bcff9bd165ba9223758a1efd3c6f7b7f53027/compose-layout/src/main/java/com/google/android/horologist/compose/pager/PagerScreen.kt#L83-L87

val countState = rememberUpdatedState(newValue = count)
val pagerScreenState = remember { PageScreenIndicatorState(state, countState) }
HorizontalPageIndicator(
modifier = Modifier.padding(6.dp),
pageIndicatorState = pagerScreenState
)

ap...@google.com <ap...@google.com> #6Apr 13, 2023 01:23PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit a9fe9fa615abc65c607ff14f7e638a4afbb8e8cb
Author: Levi Albuquerque <levima@google.com>
Date: Mon Mar 20 19:52:53 2023

Update pageCount use and application in Pager

pageCount should be used inside PagerState. This allows for better flexibility in the public API (it can be accessed from state), it also allows for usage in composition.

Relnote: Remove pageCount from Horizontal/VerticalPager. This should be provided at the state creation. Updated PagerState and rememberPagerState to accept the pageCount.

Test: Previous tests should pass.
Fixes: 266965072
Change-Id: Ifa3cb3f0444ed1493efbeb3e6268f50684dec44a

M compose/foundation/foundation/api/public_plus_experimental_current.txt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/pager/PagerCarrouselDemos.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/pager/PagerDemos.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/pager/PagerStateInteractionsDemos.kt
M compose/foundation/foundation/samples/src/main/java/androidx/compose/foundation/samples/PagerSamples.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/BasePagerTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/EmptyPagerTests.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PageLayoutPositionOnScrollingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PageSizeTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerAccessibilityTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerContentPaddingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerContentTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerCrossAxisTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerLayoutInfoTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerNestedScrollContentTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerOffscreenPageLimitPlacingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerPinnableContainerTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerPrefetcherTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerScrollingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerStateTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerSwipeEdgeTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/LazyLayoutPager.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/Pager.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/PagerMeasure.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/PagerMeasurePolicy.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/PagerState.kt
M compose/integration-tests/macrobenchmark-target/src/main/java/androidx/compose/integration/macrobenchmark/target/PagerActivity.kt
M compose/integration-tests/macrobenchmark-target/src/main/java/androidx/compose/integration/macrobenchmark/target/PagerAsCarouselActivity.kt
M paging/paging-compose/samples/src/main/java/androidx/paging/compose/samples/PagingFoundationSample.kt

https://android-review.googlesource.com/2490595

ap...@google.com <ap...@google.com> #7Apr 17, 2023 02:00PM

Project: platform/frameworks/support
Branch: androidx-main

commit 64c80bd7c114fc4a1ec29441277b6efa644a79dd
Author: Levi Albuquerque <levima@google.com>
Date: Thu Apr 13 18:03:40 2023

Update pageCount use and application in Pager

pageCount should be used inside PagerState. This allows for better flexibility in the public API (it can be accessed from state), it also allows for usage in composition.

Relnote: Remove pageCount from Horizontal/VerticalPager. This should be provided at the state creation. Updated PagerState and rememberPagerState to accept the pageCount.

Test: Previous tests should pass.
Fixes: 266965072
Change-Id: Ieb52d10f08c4a45beaa4ce0a72734a44afb60ce7

M compose/foundation/foundation/api/public_plus_experimental_current.txt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/pager/PagerCarrouselDemos.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/pager/PagerDemos.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/pager/PagerStateInteractionsDemos.kt
M compose/foundation/foundation/samples/src/main/java/androidx/compose/foundation/samples/PagerSamples.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/BasePagerTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/EmptyPagerTests.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PageLayoutPositionOnScrollingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PageSizeTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerAccessibilityTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerContentPaddingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerContentTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerCrossAxisTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerLayoutInfoTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerNestedScrollContentTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerOffscreenPageLimitPlacingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerPinnableContainerTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerPrefetcherTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerScrollingTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerStateTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerSwipeEdgeTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/pager/PagerTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/LazyLayoutPager.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/Pager.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/PagerMeasure.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/PagerMeasurePolicy.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/PagerState.kt
M compose/integration-tests/macrobenchmark-target/src/main/java/androidx/compose/integration/macrobenchmark/target/PagerActivity.kt
M compose/integration-tests/macrobenchmark-target/src/main/java/androidx/compose/integration/macrobenchmark/target/PagerAsCarouselActivity.kt
M paging/paging-compose/samples/src/main/java/androidx/paging/compose/samples/PagingFoundationSample.kt

https://android-review.googlesource.com/2535728

ys...@google.com <ys...@google.com> #8Apr 25, 2023 06:08PM

Nice, works well. https://github.com/google/horologist/pull/1231/files

One footgun is partially updating the API usage, I removed the count from the HorizontalPager, but missed one location calling rememberPagerState, was confused. But working after that. I wonder if that invalid combination should fail with a more obvious error.

But looks good.

na...@google.com <na...@google.com> #9May 3, 2023 06:24PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.paging:paging-compose:1.0.0-alpha19

na...@google.com <na...@google.com> #10May 10, 2023 07:09PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.foundation:foundation:1.5.0-alpha04

[Deleted User] <[Deleted User]> #11Jun 2, 2023 08:03PM

Started using this recently. But running into issues when using key on the HorizontalPager. If an update occurs on the data set the count seems to be getting mismatched? Get an "IndexOutOfBoundsException" when trying to access data set with index for the keys.

le...@google.com <le...@google.com> #12Jun 5, 2023 09:12AM

Hi, could you open another ticket with a reproducible sample? Thank you :D

[Deleted User] <[Deleted User]> #13Jun 5, 2023 09:27PM

Thanks for the reply, will do!