Material3 PullToRefresh implementation has a number of rough areas [317177683]

Fixed

Bug

Status Update

No update yet.

Description

za...@gmail.com

created issue #1

Dec 20, 2023 08:21PM

Jetpack Compose version: 1.6.0-beta03
Jetpack Compose component used: androidx.compose.material3:material3:1.2.0-beta01
Android Studio Build: N/A
Kotlin version: 1.9.21

I've been testing the new PullToRefresh implementation in M3 and finding a number of issues, both functional and API ergonomics.

1. No exit animation when endRefresh() is called

This one is fairly straightforward and visible in the attached example. I would expect this to animate the progress indicator back to rest.

2. The indicator is always drawn above the content, including on top of other content

This is more serious bug, as it results in what looks like a floating artifact in the UI. The indicator implementation seems to assume that the container is always pushed up against the top of the display, and thus wouldn't worry about drawing the indicator. However, when used in even simple scaffolds with a top app bar of some sort, it will draw the indicator (even when not refreshing) on top of the app bar. This is visible in the attached examples as well.

3. The container intercepts upward scroll events in nested scrolling conditions

This is visible in the attachment as well. In a scaffold with a collapsing/moving top bar, the container will actively intercept upward scroll events in some cases and prevent expansion of the top app bar.

4. The API is awkward and a regression from the M2 implementation

The M2 implementation offered an onRefresh: () -> Unit callback that made using this dead simple. It also better aligned with what this UI ultimately is - a glorified refresh button implemented as a gesture. In the new M3 API, there is no such hook. Instead, one has to directly observe the isRefreshing boolean state. In simple cases (such as the samples) where the UI is directly suspending/blocking on some refresh call to the data layer, this works ok. However, it quickly breaks down when interacting with more real-world cases where a refresh event is signaled to the data layer (such as via LazyPagingItems.refresh()) and awaiting an updated load state.

The result is that the user has to manually implement this synchronization of states. If it's refreshing but the pager load state isn't loading, you now have to independently track if

You previously kicked off a refresh and now the source is done loading
The user initiated a refresh and you need to tell the source to kick off a new refresh

I've attached a screenshot example of what this synchronization tedium looks like in practice

Wed Dec 20 2023 15:20:10 GMT-0500 (Eastern Standard Time).png

278 KB

View

Download

screen-20231217-235324.mp4

5.1 MB

Download

screen-20231217-235654.mp4

5.5 MB

Download

Screenshot_20231217-235551.png

2.6 MB

View

Download

Comments

ma...@google.com <ma...@google.com> Dec 27, 2023 09:38PM

Reassigned to ma...@google.com.

jo...@veeva.com <jo...@veeva.com> #2Feb 6, 2024 05:18PM

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

commit a330c0d3bcdd41326f37968a60e6084ad4a2e32c
Author: Chet Haase <chet@google.com>
Date: Wed Jul 05 07:26:46 2023

Convert APIs using PointF to use Float instead

PointF is a convenient mechanism for passing around x.y values
representing 2D points. But there are downsides, including:
- Converting to PointF: You may not have the data in PointF form
to begin with, so using an API which takes PointF requires converting
the data to that form (including allocating a PointF object every time)
- Mutability: Point structures can be mutated internally, causing
unpredictability in what that mutation means. Should the library
react to those changes? Ignore them? Do defensive copies (requiring
even more allocations)? Using primitive types like Float make the
behavior more obvious (by making the data inherently immutable).
- Allocations: Whenever we use object types, there are necessarily
allocations on the Java heap for them. This puts pressure on the GC
at both allocation and collection time. Given the amount of points
being passed around (especially at morph creation time, when curves
are being split and created), this causes a lot of PointF objects to
be allocated (even temporarily). Using Float avoids that problem.

Also fixed bug with unclosed paths causing discontinuity at the
start/end point.

Bug: 276466399
Bug: 290254314
Test: integration and unit tests pass
Relnote: PointF parameters changed to Float pairs
Change-Id: Id4705d27c7be31b26ade8186b99fffe2e2f8450e

M graphics/graphics-shapes/api/current.txt
M graphics/graphics-shapes/api/restricted_current.txt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicShapeTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonMeasureTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/RoundedPolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/ShapesTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/TestUtils.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Cubic.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/CubicShape.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FeatureMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FloatMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Morph.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/PolygonMeasure.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/RoundedPolygon.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Shapes.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Utils.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/DebugDraw.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/ShapeEditor.kt
M graphics/integration-tests/testapp/src/main/java/androidx/graphics/shapes/test/MaterialShapes.kt

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

Message last modified on Feb 6, 2024 05:24PM

ar...@physitrack.com <ar...@physitrack.com> #3Feb 13, 2024 01:03PM

PointF was the main (possibly only) mutability issue, marking this as fixed

ha...@gmail.com <ha...@gmail.com> #4Mar 8, 2024 12:48PM

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

androidx.graphics:graphics-shapes:1.0.0-alpha04

an...@sigma.software <an...@sigma.software> #5Mar 18, 2024 02:16PM

It's impossible to migrate to M3 pull to refresh if you're using M2. API is so narrowly designed to just run async OPs directly inside @Composable functions so that you can call endRefresh() in the end. If you have adequate (can't find more suitable word) codebase with ViewModels, there is literally no way to control refresh state using outer state. It's such a terrible release that only wastes developers time to try it out and disappoint.

ma...@google.com <ma...@google.com> Apr 3, 2024 05:41PM

Reassigned to ma...@google.com.

co...@google.com <co...@google.com> Apr 4, 2024 06:39PM

Accepted by ma...@google.com.

ap...@google.com <ap...@google.com> #6Apr 18, 2024 06:28PM

Marked as fixed.

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

commit a1154475dd101bcf9867c11ceb567f9511dc7c1c
Author: Mariano Martin <marianomartin@google.com>
Date: Fri Apr 05 12:00:06 2024

[PullToRefresh] Update Material 3 PullToRefresh APIs

Test: updated existing tests

Fixes: 314496282, 317177684, 323787138, 324573502, 317177683

Relnote: "New pull-to-refresh APIs:

- Simplified PullToRefreshState to use fractional values instead of Dp units.
- isRefreshing state is controlled by the user instead of PullToRefreshState.
- Separated out the nested scroll connection from PullToRefreshState. It is handled by the new PullToRefreshBox or Modifier.pullToRefresh.

- This update is a breaking change to previous experimental APIs."

Change-Id: I0adeb950063988d1a05aca7aa135ccd982431423

M compose/material3/material3/api/current.txt
M compose/material3/material3/api/restricted_current.txt
M compose/material3/material3/integration-tests/material3-catalog/src/main/java/androidx/compose/material3/catalog/library/model/Examples.kt
M compose/material3/material3/integration-tests/material3-demos/src/main/java/androidx/compose/material3/demos/PullToRefreshDemo.kt
M compose/material3/material3/samples/src/main/java/androidx/compose/material3/samples/PullToRefreshSamples.kt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/pulltorefresh/PullToRefreshIndicatorScreenshotTest.kt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/pulltorefresh/PullToRefreshIndicatorTest.kt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/pulltorefresh/PullToRefreshStateImplTest.kt
M compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/pulltorefresh/PullToRefresh.kt

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

Issue 317177683

Description

Issue summary

Comments

ma...@google.com <ma...@google.com> Dec 27, 2023 09:38PM

jo...@veeva.com <jo...@veeva.com> #2Feb 6, 2024 05:18PM

ar...@physitrack.com <ar...@physitrack.com> #3Feb 13, 2024 01:03PM

ha...@gmail.com <ha...@gmail.com> #4Mar 8, 2024 12:48PM

an...@sigma.software <an...@sigma.software> #5Mar 18, 2024 02:16PM

ma...@google.com <ma...@google.com> Apr 3, 2024 05:41PM

co...@google.com <co...@google.com> Apr 4, 2024 06:39PM

ap...@google.com <ap...@google.com> #6Apr 18, 2024 06:28PM

Add comment

Issue metadata