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 c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

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

It wasn't clear that it was only fixed pageCount. What is the API restricts that? or handles the pageCount changing? I had assumed that if you add additional pages, or remove them it would essentially add or remove from the end of the pages. If that's not the case, perhaps document that it must be fixed and never change.

But either way, it's an additional parameter to pass into my PagerScreenIndicatorState, or any foundation PageIndicatorState implementation, when it's obviously already known and available through PagerState > LazyListState. Making this public would just make it easy to not worry about whether pageCount is fixed or not, when implementing a generic layout or components around this API.

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

Sorry, what I mean by fixed is that, unlike LazyList, the number of pager is known upfront, as a parameter from the definition of the composable.

You can of course use a state backed parameter in this case. I understand that the pageCount inside the PagerState is available through LazyListState, but the end goal is that Pager will not be based off LazyLists so we couldn't open all APIs available in LazyListState for Pager. We're reconsidering this and I'll update the ticket with the result, thanks for the feedback btw :)

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!