Verified
Status Update
No update yet.
Comments
cc...@google.com
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
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
tw...@googlemail.com
Hi,
I uploaded a modified version of the PagingSample tohttps://github.com/twanschik/PagingSample .
Changed the code to not fill the database on startup but consecutively by batch-adding items when tapping on a button. Items are added in reverse order to ensure that the items will be "pre-pended" with regard to the select query.
Moreover, I uploaded two videos demonstrating the issue:
https://youtu.be/M0LmetV9pdc
https://youtu.be/6vyAA7v_1uM
The only difference is that in the second video LinearLayoutManager.stackFromEnd = true is set. This somehow results in even worse behavior.
Hope this helps clarifying the issue.
Thank you!
I uploaded a modified version of the PagingSample to
Changed the code to not fill the database on startup but consecutively by batch-adding items when tapping on a button. Items are added in reverse order to ensure that the items will be "pre-pended" with regard to the select query.
Moreover, I uploaded two videos demonstrating the issue:
The only difference is that in the second video LinearLayoutManager.stackFromEnd = true is set. This somehow results in even worse behavior.
Hope this helps clarifying the issue.
Thank you!
tw...@googlemail.com
Did the sample help you in any way to verify that the problem exists?
Cheers,
Thomas
Cheers,
Thomas
cc...@google.com
Yes, thank you, the sample clarified the problem,
The problem happens when a new PagedList with a prepend is received by the UI, but the items in the viewport aren’t updated. The stream of PagedLists keeps a guess of what key to use across PagedList generations based on last load position. However, if nothing in the viewport is updated, PagedListAdapter#getItem() isn't called. This means we don’t update that last load position to account for prepended items. This only affects PositionalDataSource, since the old fetch position becomes stale, whereas an item key would not.
Fix will be to update this key when a new PagedList arrives by triggering a fetch.
In the meantime, here's an adapter-side hack to workaround the problem. Add the below to CheeseAdapter.kt, and the sample works fine:
var rv : RecyclerView? = null
override fun onAttachedToRecyclerView(recyclerView: RecyclerView) {
rv = recyclerView // stash ref to RV, so we can get layout manager ref
}
override fun onDetachedFromRecyclerView(recyclerView: RecyclerView) {
rv = null
}
override fun onCurrentListChanged(currentList: PagedList<Cheese>?) {
if (currentList != null && currentList.isNotEmpty()) {
// have to post so findFirstVisibleItemPosition() returns position relative to currentList
rv?.post {
if (getCurrentList() == currentList) {
val visPos = (rv?.layoutManager as LinearLayoutManager)
.findFirstVisibleItemPosition()
val offset = currentList.positionOffset
// loadAround will update the key in the currentList,
// which will be used for next reload from DB
currentList.loadAround(visPos + offset)
}
}
}
}
The problem happens when a new PagedList with a prepend is received by the UI, but the items in the viewport aren’t updated. The stream of PagedLists keeps a guess of what key to use across PagedList generations based on last load position. However, if nothing in the viewport is updated, PagedListAdapter#getItem() isn't called. This means we don’t update that last load position to account for prepended items. This only affects PositionalDataSource, since the old fetch position becomes stale, whereas an item key would not.
Fix will be to update this key when a new PagedList arrives by triggering a fetch.
In the meantime, here's an adapter-side hack to workaround the problem. Add the below to CheeseAdapter.kt, and the sample works fine:
var rv : RecyclerView? = null
override fun onAttachedToRecyclerView(recyclerView: RecyclerView) {
rv = recyclerView // stash ref to RV, so we can get layout manager ref
}
override fun onDetachedFromRecyclerView(recyclerView: RecyclerView) {
rv = null
}
override fun onCurrentListChanged(currentList: PagedList<Cheese>?) {
if (currentList != null && currentList.isNotEmpty()) {
// have to post so findFirstVisibleItemPosition() returns position relative to currentList
rv?.post {
if (getCurrentList() == currentList) {
val visPos = (rv?.layoutManager as LinearLayoutManager)
.findFirstVisibleItemPosition()
val offset = currentList.positionOffset
// loadAround will update the key in the currentList,
// which will be used for next reload from DB
currentList.loadAround(visPos + offset)
}
}
}
}
tw...@googlemail.com
Hi,
thank you for your explanation. It total makes sense.I tried your suggested workaround, however, the list still doesn't keep the correct state. When scrolling a bit around and then pre-pending items, I still get different items.
Cheers,
Thomas
thank you for your explanation. It total makes sense.I tried your suggested workaround, however, the list still doesn't keep the correct state. When scrolling a bit around and then pre-pending items, I still get different items.
Cheers,
Thomas
tw...@googlemail.com
Do you have another suggestions how to workaround the problem?
Cheers,
Thomas
Cheers,
Thomas
cc...@google.com
Experimented more with the sample, was able to get this issue to reproduce:
1) Clear DB
2) Batch prepend x2
3) Scroll to top of loaded content
4) Batch prepend x5
The rest of this issue is as you described in your initial report - the initial load position doesn't account for prepend offsetting list content. (There's also the additional factor of centering at an arbitrary position in the viewport, not the center.)
Both of these make the initial load guess less precise. If any viewport content is outside the bounds of that initial load, those items will disappear until you scroll back to them.
Ideally, the initial load is large enough to account for this. Setting pageSize = 50, or initialLoadSize = 150 make the problem go away, and really the sample should have had a bigger page size like this from the beginning. 20 is only trivially bigger than what's in viewport on a mid-size phone, and we generally recommend around 3 viewports of content as a page size minimum.
The larger of removals/inserts you do with positional data, the larger the initial load needs to be to avoid this disappearing problem. In this case, it's just raising it to a reasonable value.
1) Clear DB
2) Batch prepend x2
3) Scroll to top of loaded content
4) Batch prepend x5
The rest of this issue is as you described in your initial report - the initial load position doesn't account for prepend offsetting list content. (There's also the additional factor of centering at an arbitrary position in the viewport, not the center.)
Both of these make the initial load guess less precise. If any viewport content is outside the bounds of that initial load, those items will disappear until you scroll back to them.
Ideally, the initial load is large enough to account for this. Setting pageSize = 50, or initialLoadSize = 150 make the problem go away, and really the sample should have had a bigger page size like this from the beginning. 20 is only trivially bigger than what's in viewport on a mid-size phone, and we generally recommend around 3 viewports of content as a page size minimum.
The larger of removals/inserts you do with positional data, the larger the initial load needs to be to avoid this disappearing problem. In this case, it's just raising it to a reasonable value.
cc...@google.com
Fix to incorrect initial fetch position in #5 merged, will go out with next AndroidX paging release.
cc...@google.com
Released with Paging 2.0.0-beta01 (Currently revision is 2.0.0 stable)
Description
Version used:all versions
Devices/Android versions reproduced on: all versions
Hi,
thanks for the great work.
I use a slightly modified version of the PagingSample. Instead of just adding one item when tapping on the "Add" button, I add 20 copies of the same item:
fun insert(text: CharSequence) = ioThread {
dao.insert(listOf(
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString()),
Cheese(id = 0, name = text.toString())))
}
When adding items which will be pre-pended to the list, the PositionalDataSource fails to re-load the visible items. This is a general problem.
My guess:
The PositionalDatasource doesn't know if newly inserted items will be pre-pended by the query or appended. Let's say we have the following scenario:
- initially loading items from position 20
- pre-pending 50 new items
- results in datasource invalidation and new initial load from position 20
- now there are completely different items at position 20
- async diff sees changes and triggers animations
Somehow this happens to be related to the page size used. When increasing the page size, it happens after more inserts. When decreasing the page size, this behavior happen after just the first inserts.
My question is:
How to handle pre-pending inserts with the PositionalDataSource?