Refresh key is not preserved when PagingSource is invalidated before first load [170027530]

Fixed

Bug

Status Update

No update yet.

Description

me...@yhpark.io

created issue #1

Oct 5, 2020 04:17AM

Component used: androidx.paging:paging-runtime

Version used: 3.0.0-alpha07

When a PagingSource is invalidated before it has loaded any page, the subsequent PagingSource does not get getRefreshKey called and initial load key is passed as null.

Sample project: Issue B from https://github.com/yhpark/androidx-paging3-issues

Comments

du...@google.com <du...@google.com> #2Oct 5, 2020 09:59PM

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> #3Oct 30, 2020 06:50PM

Took a look at your sample - looks like you aren't passing initialKey to Pager, which is where the first refresh key comes from (before any item access is made). Passing in initial key will correctly get picked up replacing null as the default value.

However there's a good point here that's its very easy to confuse when initialKey or getRefreshKey is used and we should probably see if it might be worth combining them and simply passing null for anchorPosition to getRefreshKey

so...@gmail.com <so...@gmail.com> #4Nov 2, 2020 03:00AM

I was not referring to the initialKey passed to Pager, but the refresh load key passed to the PagingSource.load(...) after the Pager is invalidated.

For instance, when user scrolled through position 123 in a recycler view then the PagingSource is invalidated, the first Refresh key given to the first load call to the newly created PagingSource should be around 123. However, this doesn't happen when a PagingSource (A) is invalidated and the next PagingSource (B) is also invalidated before it (B) has completed its first load, the subsequent PagingSource (C) gets the refresh key as null (or possibly the initialKey set to the Pager).

This is easily reproduced as presented in the demo project, and can also happen in real situations like multiple database updates within a short time interval.
This is a user facing bug. When this happens, the user's scroll position suddenly moves to the top of the list since the last anchor position is lost.

du...@google.com <du...@google.com> Nov 10, 2020 04:23AM

Accepted by du...@google.com.

ap...@google.com <ap...@google.com> #5Nov 10, 2020 07:20PM

Marked as fixed.

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

commit 6d4cbbf5ef0bd0145c5f88f11ff9f4204ac827a3
Author: Dustin Lam <dustinlam@google.com>
Date: Mon Nov 09 19:29:47 2020

Preserve PagingState across generations if no hint received

This change resolves an issue where getRefreshKey may not be called if
the most recent generation was invalidated before it had a chance to
load data and receive a hint. This makes it impossible to preserve
anchorPosition in cases where invalidates may happen quickly.

Relnote: "Fixed an issue where getRefreshKey does not get called on
subsequent generations of PagingSource when invalidations occur rapidly."
Fixes: 170027530
Test: ./gradlew paging:paging-common:test
Change-Id: I4546070c31ed73fd285754707b8b480cba206b93

M paging/common/src/main/kotlin/androidx/paging/PageFetcher.kt
M paging/common/src/main/kotlin/androidx/paging/PagingState.kt
M paging/common/src/test/kotlin/androidx/paging/PageFetcherTest.kt
M testutils/testutils-paging/src/main/java/androidx/paging/TestPagingSource.kt

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

me...@yhpark.io <me...@yhpark.io> #6Nov 12, 2020 04:16AM

Thanks!

Issue 170027530