Fixed
Status Update
Comments
ap...@google.com <ap...@google.com> #2
Project: platform/frameworks/support
Branch: androidx-main
commit f958564f327d27b02f262d56fa3ebb29fd04ad05
Author: Dustin Lam <dustinlam@google.com>
Date: Fri Aug 27 17:10:23 2021
Do not track remote states locally within PageFetcher for PageEvents
Previously from fetcher side, we locally tracked remote load states
per-generation to inject them into the page event stream, however this
leads to issues between generations, where the local state is incorrect
before it receives an event from either the source or remote stream.
This CL changes .injectRemoteEvents into a custom .combine operator so
that we always wait for a real event from both remote and source flows,
so that we are guaranteed to have valid states for both. We also now
immediately send REFRESH=Loading from source so that it is less likely
to drop remote events due to waiting on source flow from .combine.
The reason we use a custom .combine operator is to remove the "batching"
logic in .combine, which would normally output the latest values from
each Flow and can miss emissiosn when we get multiple emissions on the
same Flow, as opposed to alternating emissions.
Since it also affected some tests, this CL also fixes a longstanding
issue with cancellation on invalidation. Previously, a collector
would continue to receive remote events after invalidation, which
can make it difficult for Java users or those not using collectLatest to
switch to the next generation, since submitData would never return until
cancelled. We now eagerly cancel collection on the remote stream (within
a generation) to close the Flow<PageEvent> from a PagingData as soon as
we receive a new generation.
RelNote: "LoadStates from Paging now await valid values from both
PagingSource and RemoteMediator before emitting downstream between
generations. This prevents new generations of PagingData from sending
NotLoading in CombinedLoadStates.source.refresh if it was already
Loading; new generations of PagingData will now always correctly begin
with Loading for refresh state instead of first resetting to NotLoading
incorrectly in some cases.
Cancellation on past generations now happen eagerly on invalidation /
new generations. It should no longer be required to use .collectLatest
on Flow<PagingData>, although it is still highly recommended to do so."
Fixes: 177351336
Fixes: 195028524
Test: ./gradlew paging:paging-common:test
Change-Id: I0b2b5e0120e2e3a677f65d40d3955a9bbbe2e1a9
M paging/paging-common/src/main/kotlin/androidx/paging/PageFetcher.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PagingDataDifferTest.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PageFetcherSnapshotStateTest.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PageFetcherSnapshotTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/FlowExt.kt
M testutils/testutils-paging/src/main/java/androidx/paging/TestPagingSource.kt
A paging/paging-common/src/test/kotlin/androidx/paging/FlowExtTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/PageFetcherSnapshot.kt
D paging/paging-common/src/test/kotlin/androidx/paging/SimpleFlowExtTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/PageFetcherSnapshotState.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PageFetcherTest.kt
https://android-review.googlesource.com/1812164
Branch: androidx-main
commit f958564f327d27b02f262d56fa3ebb29fd04ad05
Author: Dustin Lam <dustinlam@google.com>
Date: Fri Aug 27 17:10:23 2021
Do not track remote states locally within PageFetcher for PageEvents
Previously from fetcher side, we locally tracked remote load states
per-generation to inject them into the page event stream, however this
leads to issues between generations, where the local state is incorrect
before it receives an event from either the source or remote stream.
This CL changes .injectRemoteEvents into a custom .combine operator so
that we always wait for a real event from both remote and source flows,
so that we are guaranteed to have valid states for both. We also now
immediately send REFRESH=Loading from source so that it is less likely
to drop remote events due to waiting on source flow from .combine.
The reason we use a custom .combine operator is to remove the "batching"
logic in .combine, which would normally output the latest values from
each Flow and can miss emissiosn when we get multiple emissions on the
same Flow, as opposed to alternating emissions.
Since it also affected some tests, this CL also fixes a longstanding
issue with cancellation on invalidation. Previously, a collector
would continue to receive remote events after invalidation, which
can make it difficult for Java users or those not using collectLatest to
switch to the next generation, since submitData would never return until
cancelled. We now eagerly cancel collection on the remote stream (within
a generation) to close the Flow<PageEvent> from a PagingData as soon as
we receive a new generation.
RelNote: "LoadStates from Paging now await valid values from both
PagingSource and RemoteMediator before emitting downstream between
generations. This prevents new generations of PagingData from sending
NotLoading in CombinedLoadStates.source.refresh if it was already
Loading; new generations of PagingData will now always correctly begin
with Loading for refresh state instead of first resetting to NotLoading
incorrectly in some cases.
Cancellation on past generations now happen eagerly on invalidation /
new generations. It should no longer be required to use .collectLatest
on Flow<PagingData>, although it is still highly recommended to do so."
Fixes: 177351336
Fixes: 195028524
Test: ./gradlew paging:paging-common:test
Change-Id: I0b2b5e0120e2e3a677f65d40d3955a9bbbe2e1a9
M paging/paging-common/src/main/kotlin/androidx/paging/PageFetcher.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PagingDataDifferTest.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PageFetcherSnapshotStateTest.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PageFetcherSnapshotTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/FlowExt.kt
M testutils/testutils-paging/src/main/java/androidx/paging/TestPagingSource.kt
A paging/paging-common/src/test/kotlin/androidx/paging/FlowExtTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/PageFetcherSnapshot.kt
D paging/paging-common/src/test/kotlin/androidx/paging/SimpleFlowExtTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/PageFetcherSnapshotState.kt
M paging/paging-common/src/test/kotlin/androidx/paging/PageFetcherTest.kt
ap...@google.com <ap...@google.com> #3
Project: platform/frameworks/support
Branch: androidx-main
commit ba33f5f07155179c480bfbe4dfc4c250518ac546
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Feb 15 17:39:25 2022
Avoid intermediate IDLE events
This CL fixes a bug in page caching where we were injecting an IDLE
state after each invalidation. Even though this is technically not
"wrong", it is unexpected and requires developers to handle it in more
cases. Technically they should always handle it (since other things
might affect state) but this bug makes it too prominent for no reason.
The fix makes CachedPageEventFlow check if it ever received any upstream
events before synthesizing a state event. This behavior was added in
Ica5bf1f2bcdb6ac56ccaaba3f29b317ea92bec55 but became obsolete in
I0b2b5e0120e2e3a677f65d40d3955a9bbbe2e1a9.
I've updated PagingDataDifferTest to also run with caching enabled to
ensure the behavior does not change between cached and non-cached cases.
Fixes: 216647650
Bug: 177351336
Bug: 195028524
Bug: 194408854
Test: PagingDataDifferTest, AsyncPagingDataDifferTest, CachingTest
Change-Id: Ic4bca6ffd085dcb2cdd7a991d56367aad275cd12
M paging/paging-common/src/test/kotlin/androidx/paging/PagingDataDifferTest.kt
M paging/paging-common/src/test/kotlin/androidx/paging/FlattenedPageEventStorageTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/CachedPageEventFlow.kt
M paging/paging-common/src/test/kotlin/androidx/paging/CachedPageEventFlowTest.kt
M paging/paging-runtime/src/androidTest/java/androidx/paging/AsyncPagingDataDifferTest.kt
https://android-review.googlesource.com/1987328
Branch: androidx-main
commit ba33f5f07155179c480bfbe4dfc4c250518ac546
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Feb 15 17:39:25 2022
Avoid intermediate IDLE events
This CL fixes a bug in page caching where we were injecting an IDLE
state after each invalidation. Even though this is technically not
"wrong", it is unexpected and requires developers to handle it in more
cases. Technically they should always handle it (since other things
might affect state) but this bug makes it too prominent for no reason.
The fix makes CachedPageEventFlow check if it ever received any upstream
events before synthesizing a state event. This behavior was added in
Ica5bf1f2bcdb6ac56ccaaba3f29b317ea92bec55 but became obsolete in
I0b2b5e0120e2e3a677f65d40d3955a9bbbe2e1a9.
I've updated PagingDataDifferTest to also run with caching enabled to
ensure the behavior does not change between cached and non-cached cases.
Fixes: 216647650
Bug: 177351336
Bug: 195028524
Bug: 194408854
Test: PagingDataDifferTest, AsyncPagingDataDifferTest, CachingTest
Change-Id: Ic4bca6ffd085dcb2cdd7a991d56367aad275cd12
M paging/paging-common/src/test/kotlin/androidx/paging/PagingDataDifferTest.kt
M paging/paging-common/src/test/kotlin/androidx/paging/FlattenedPageEventStorageTest.kt
M paging/paging-common/src/main/kotlin/androidx/paging/CachedPageEventFlow.kt
M paging/paging-common/src/test/kotlin/androidx/paging/CachedPageEventFlowTest.kt
M paging/paging-runtime/src/androidTest/java/androidx/paging/AsyncPagingDataDifferTest.kt
Description
i.e. we want to prevent this:
Refresh.Loading --> [new generation] --> Refresh.NotLoading --> Refresh.Loading
and want this:
Refresh.Loading --> [new generation] --> Refresh.Loading
To do that reset, Fetcher needs to keep track of multi-gen local LoadStates which it currently doesn't do so.
An idea is to have fetcher hoist the state up in a separate operator to inject the correct source state.