Verified
Status Update
No update yet.
Comments
cc...@google.com
Project: platform/frameworks/support
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
https://android-review.googlesource.com/1123258
https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
tw...@googlemail.com
tw...@googlemail.com
Project: platform/frameworks/support
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
https://android-review.googlesource.com/1288456
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
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?