LazyPagingItems should not be initialized with initial .loadState set to NotLoading.Incomplete [224855902]

Fixed

Bug

Status Update

No update yet.

Description

du...@google.com

created issue #1

Mar 15, 2022 10:39PM

In Paging 3.1 we made the behavior change to always wait for real LoadState updates from PagingSource / RemoteMediator before sending them downstream to user. This allows a developer to react on page loads without needing to filter out the initial / intermediate NotLoading states. This also prevents sending a CombinedLoadStates where mediator states is null even though we are using RemoteMediator.

Ideally we would not send any "initial" load state downstream, I'm not sure if that means we need to make .loadState nullable in LazyPagingItems as it may not be idiomatic for Compose.

Note that this initial state will also end up affecting invalidation, retry, recompose, etc.

Comments

an...@google.com <an...@google.com> #2Mar 16, 2022 01:15PM

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

du...@google.com <du...@google.com> #3Mar 16, 2022 07:49PM

Ideally we would not pass any initial state because it will always have some case where it is wrong - we should ideally wait for a real event from Paging before sending it downstream to user.

For example the initial state:

CombinedLoadStates(
            refresh = InitialLoadStates.refresh,
            prepend = InitialLoadStates.prepend,
            append = InitialLoadStates.append,
            source = InitialLoadStates
        )

...will always have null mediator states, even if they are using RemoteMediator. This can mess up loadstate logic that is waiting for some combination of source + mediator loads to happen. It's easy to filter the initial case, but it gets complicated if they want to switchMap between multiple streams and they all start with this initial state. Or also consider the case when coming from cached state and needing to filter this out... it's bad developer UX.

So we could either set this to nullable to denote "no state" or we could have some constant we use that can be easily filtered out like "BogusLoadState" and then people can do .filter { it !== BogusLoadState }

du...@google.com <du...@google.com> #4Mar 16, 2022 09:05PM

Alternatively, if we can read whether we have RemoteMediator or not, we could try this for initial state which would be a lot more accurate:

private val InitialLoadStates = LoadStates(
    LoadState.Loading,
    IncompleteLoadState,
    IncompleteLoadState
)

CombinedLoadStates(
  refresh = InitialLoadStates.refresh,
  prepend = InitialLoadStates.prepend,
  append = InitialLoadStates.append,
  source = InitialLoadStates,
  mediatr = ??
)

an...@google.com <an...@google.com> #5Mar 17, 2022 12:46PM

Reassigned to du...@google.com.

The latest proposal of starting in a reasonable Loading state looks great to me!
Don't you mind trying it out? You are better person for figuring out how to get RemoteMediator from PagingDataDiffer. To be honest I don't even know what RemoteMediator is used for

il...@google.com <il...@google.com> Apr 6, 2022 08:37PM

Reassigned to sa...@google.com.

ap...@google.com <ap...@google.com> #6Apr 29, 2022 10:43PM

Marked as fixed.

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

commit b7f9085d0b13af5a346d0712bbf3bc5a680c2fc9
Author: Sanura N'Jaka <sanura@google.com>
Date: Thu Apr 28 22:10:36 2022

Changing LazyPagingItem's inital LoadState refresh to Loading

Rather than LazyPagingItem having the inital LoadState
refresh being set to LoadState.NotLoading, we want it
to be LoadState.Loading

RelNote: "`LazyPagingItems` now sets the initial
`loadState` to have a `LoadState.Loading` refresh."
Test: lazyPagingInitialLoadState
Fixes: 224855902

Change-Id: I55043244f92c8029e4667df2e23c5813dcf2f729

M paging/paging-compose/src/main/java/androidx/paging/compose/LazyPagingItems.kt
M paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt

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

ki...@protonmail.com <ki...@protonmail.com> #7May 20, 2022 03:50PM

Can we receive an updated library with this fix since it has been marked as fixed?

al...@mercari.com <al...@mercari.com> #8Jun 16, 2022 10:56AM

Is this the same issue reported in https://issuetracker.google.com/issues/190788885 which was set as Won't Fix? Because we did follow the recommendation from there to skip the first state, so we will have to revert that code.

du...@google.com <du...@google.com> #9Jun 16, 2022 04:11PM

Is this the same issue reported in https://issuetracker.google.com/issues/190788885 which was set as Won't Fix? Because we did follow the recommendation from there to skip the first state, so we will have to revert that code.

Yes, part of the intention here is that people will no longer need to add this workaround in their code.

ha...@gmail.com <ha...@gmail.com> #10Aug 20, 2022 12:04PM

I don't get this change. If the user did not trigger the loading action, why would the state be Loading.
Let's take a search screen example. When the user first enters the search screen, nothing is loading until the user enters its search in the textfield then clicks the search button. Then the state should be Loading.

I guess we should use the workaround provided here

https://issuetracker.google.com/issues/190788885, by "dropping" the first value of loadState ? How would we even do that ?

Message last modified on Aug 20, 2022 05:15PM

ma...@gmail.com <ma...@gmail.com> #11Apr 24, 2023 09:44AM

It might be too late for some people, but if people want to know how to tackle this I've played around with it on a pet project ( didn't test this in a prod environment, so a heads up there ):

@Composable
fun <T : Any> LazyPagingItems<T>.refreshLoadState(): State<LoadState> = produceState<LoadState>(
  LoadState.NotLoading(false),
) {
  snapshotFlow { loadState }
    .drop(1)
    .map { it.refresh }
    .collect { value = it }
}

cl...@google.com <cl...@google.com> #12Apr 24, 2023 05:45PM

Re comment#10, as explained in comment#2 compose requires an initial state.

Given the most common use case displays paged data right away, defaulting LoadState to Loading means the most common use case will not have to drop first LoadState (versus if it were NotLoading).

re...@gmail.com <re...@gmail.com> #13Feb 16, 2024 11:47AM

I don't get how initial state being anything would affect the use case of displaying paged data right away. Wouldn't the LoadState change before the user has time to notice?

IMHO, initial state being NotLoading would be the sanest choice. In the case of waiting before starting to load data, the correct state would be shown to the user and in the case of loading data right away, it would immediately change to Loading, so the correct state would be shown to the user in this case as well.

du...@google.com <du...@google.com> Feb 16, 2024 05:26PM

Reassigned to cl...@google.com.

cl...@google.com <cl...@google.com> #14Feb 16, 2024 06:01PM

Re comment#13, when you call collectAsLazyPagingItems compose launches collection on Pager right away. By the time pager.collectAsLazyPagingItems() returns, data is loading, so initial Loading is the correct state.

Initializing with NotLoading also causes other issues (which is why Paging with Views was also refactored to not initialize with NotLoading):

For users who rely on first NotLoading state as indication of refresh complete, they have to manually filter out the first NotLoading
If loading animation (i.e. spinners) are based on LoadStates, you could end up seeing a blank screen --> spinner --> data loaded in, and that is not a good user experience.

Message last modified on Feb 16, 2024 06:01PM