Status Update
Comments
ad...@google.com <ad...@google.com>
ma...@google.com <ma...@google.com> #2
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
ma...@google.com <ma...@google.com> #3
sv...@google.com <sv...@google.com> #4
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
ma...@google.com <ma...@google.com> #5
quickly looked at the test failures. It seems like our regular non-velocity testing with
down()
moveBy(40f, 0f)
up()
Is no longer results in 0 velocity, hence many of the tests failed that relied on that.
I think (cc-ed Jelle to verify) that the way we ensured this is my having moveBy() and up() reporting the same pointer position to velocity tacking, where velocity tracker
Is that an intentional change in the algorithm or a cornercase we need to handle?
sv...@google.com <sv...@google.com> #6
Right, the problem is caused by VelocityPathFinder
. The 'lsq2' algorithm is essentially hardcoded there. I looked into it a bit, I'm able to pass some tests by changing the impl to use a couple of lines instead of parabola. However, it still does not work for all cases.
To solve the arbitrary "here's a start x and an end x, a time, and a velocity, figure out the path to achieve the desired velocity" problem is not very straightforward. It's not possible to solve with just 1 or 2 lines. You may need a non-linear curve. That would require re-deriving a differential equation for the impulse algorithm to solve for a specific function. Im still looking into it.
je...@google.com <je...@google.com> #7
Yes, when testing swipes with specific velocities, the sequence of touch events is very much dependent on the algorithm of the velocity tracker. If that algorithm changes, so must the injected touch events.
Thanks for taking a look at the VelocityPathFinder
, Siarhei. The current implementation basically reverse engineered the algorithm of VelocityTracker and derived an equation for it. If that is no longer possible, or is blocking you unreasonably, I think it's okay to relax the guarantees made by the VelocityPathFinder, as long as we have (1) some kind of approximation that works in most cases and (2) if it doesn't work we can make suggestions how to adjust the parameters to make it work (e.g. "try a shorter duration" or "do the swipe over a longer distance"). But even then, the latter requirement can be postponed if it is infeasible. I scheduled a meeting this Friday so we can discuss.
sv...@google.com <sv...@google.com> #8
Perfect, thanks Jelle.
I thought about this problem some more.
First, you can always achieve a certain velocity between two points if you are able to control time, by using a straight line with a fixed velocity going from start to finish.
However, with the current APIs, a time is provided, as well. That means we must add some more complex behaviour, similar to the current "flat line followed by a parabola". Adding two lines (a flat line with another straight line) works in most cases. However, it won't always work:
- If you are doing sampling, and you don't catch the entirety of the rising line over 100 ms, then it's possible that the velocity tracker would effectively see a rising line for less than 100 ms, and will thus not achieve the same velocity.
- Even without sampling, some of the provided inputs are such that the motion might have to first go backwards sufficiently (so instead of initial flat line, it would be a negative-sloping line) in order to achieve the desired final velocity, if you want a high velocity. This would not satisfy the requirement that the function is monotonically increasing.
- A piecewise-defined linear function also won't work due to sampling, since we are bound to miss some of the kinks in such a curve.
I will upload a "two-line" solution tomorrow that at least solves some of the presubmit failures to provide a better idea of what that looks like.
However, if we wanted to just change the VelocityPathFinder impl without the other tests, we would have to find some smooth function that ideally could be arbitrarily sampled, and would still result in roughly the requested velocity. From my investigations, it would have to satisfy the following: vfinal = integral_over_path(f'f'')(some constants). It's pretty easy to solve this for the case of f(t) = ax^2+bx+c, but I was not sure how to deal with the initial condition, since it's harder to define it in the smooth approximation. I think a simpler approach could be to do some numerical fitting, like picking a few monotonically increasing smooth functions (polynomial/sqrt, sin, exp, etc), and then tune them to achieve the desired velocity. Since this is only used in tests, we don't care about performance, so we can use some brute force approach. I haven't tried this yet, but I think it should work, if we want to go down that path. One benefit here would be that this approach would be immune to any future velocitytracker impl changes. Let's discuss more on Friday.
ap...@google.com <ap...@google.com> #9
Branch: androidx-main
commit 5b42a22514d0847dbce104a9f12216afa555b2bd
Author: Levi Albuquerque <levima@google.com>
Date: Fri May 13 13:06:45 2022
Improvements on how to calculate velocity based on pointer position
In this CL I updated the way we add data from an InputEventChange into
the velocity tracker. Now we add points deltas instead of event positions.
This will guarantee that points are consistent even if the target element
is moving, which was not considering when adding the input event position.
Test: Added tests to check the behaviour in scrolling.
Fixes: 216582726
Bug: 223440806
Bug: 227709803
Relnote: "When adding InputEventChange events to Velocity Tracker we will
consider now deltas instead of positions, this will guarantee the velocity
is correctly calculated for all cases even if the target element moves"
Change-Id: I51ec384a0424829b680b4666c7d3ce49227f45de
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/nestedscroll/NestedScrollModifierTest.kt
ap...@google.com <ap...@google.com> #10
Branch: androidx-main
commit a66e75c2cb06d1d754055093e47cbdb6162d7aeb
Author: Levi Albuquerque <levima@google.com>
Date: Fri May 27 10:59:21 2022
Improvements on how to calculate velocity based on pointer position
In this CL I updated the way we add data from an InputEventChange into
the velocity tracker. Now we add points deltas instead of event positions.
This will guarantee that points are consistent even if the target element
is moving, which was not considering when adding the input event position.
Test: Added tests to check the behaviour in scrolling.
Fixes: 216582726
Bug: 223440806
Bug: 227709803
Relnote: "When adding InputEventChange events to Velocity Tracker we will
consider now deltas instead of positions, this will guarantee the velocity
is correctly calculated for all cases even if the target element moves"
Change-Id: If9ef3b9069e8f15b9b049bc3cd8b08bfd91edd49
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/nestedscroll/NestedScrollModifierTest.kt
ap...@google.com <ap...@google.com> #11
Branch: androidx-main
commit 4e1e7aa76fe04d05362fa5cc319bd490401dfa61
Author: Levi Albuquerque <levima@google.com>
Date: Thu Jun 09 13:47:56 2022
Improvements on how to calculate velocity based on pointer position
In this CL I updated the way we add data from an InputEventChange into
the velocity tracker. Now we add points deltas instead of event positions.
This will guarantee that points are consistent even if the target element
is moving, which was not considering when adding the input event position.
Test: Added tests to check the behaviour in scrolling.
Fixes: 216582726
Bug: 223440806
Bug: 227709803
Relnote: "When adding InputEventChange events to Velocity Tracker we will
consider now deltas instead of positions, this will guarantee the velocity
is correctly calculated for all cases even if the target element moves"
Change-Id: Icea9d76a43643a6b17da11f3c539d27cb8fa6f6e
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/lazy/list/LazyListTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/lazy/list/BaseLazyListTestWithOrientation.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/nestedscroll/NestedScrollModifierTest.kt
Description
Jetpack Compose version: 1.1.1 Jetpack Compose component used: LazyColumn Android Studio Build: Kotlin version: 1.6.10
When I use nestedScroll in this case, If the list can not scrll up, I expect to move LazyColumn self. So I use nestedScroll and offset. But I found that the velocity is negative sometimes when I scrolled up that should be positive.
I think it's because of offset change the LazyColumn's layout coordinates, but VelocityTracker does't know this, so touch position's coordinates collected by VelocityTracker are not same. This make the wrong velocity calculation.