Revert "Temporarily revert "Remove duplicate events."" #752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
9f1fffb
to
ac8ec35
Compare
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
b958f88
to
c86dace
Compare
igordmn
added a commit
that referenced
this pull request
Sep 26, 2023
After reverting 1b9c041 in [this PR](#752), enter/exit events don't have a proper order - we can have an enter without exiting another component first. The tests are written in that PR. In the past, we: 1. made some fixes for `isIn` without upstreaming 2. in the upstream added an optimization `1b9c0419` that filter Move's with the same position. 3. Because `1b9c0419` breaks some cases, we reverted it, but this revert is no longer needed, and we reverted it back 4. Because we reverted it back - we enabled the optimization again, but it doesn't work properly with hover 5. It doesn't work properly, because `isIn` shouldn't be cached, it should be reset to `false` every time we do a hit test (resetIsIn), and set to `true` (markIsIn) only for nodes in the hit path. 6. Previously `cleanUpHits` helped us, but it isn't called when we apply optimization (don't send moves with the same position). The order of calling: ``` addHitPath resetIsIn [for hit nodes] markIsIn dispatchChanges buildCache determine if it is enter/exit/move by `isIn` [if position is changed] send it to Compose [if position is changed] cleanUpHits ``` The bugs with hover probably exist in the upstream as well, we will upstream fixes later. # Test 1. Run tests in [this PR] - they succeed 2. Revert this fix, run tests again - some will fail. The test for the case fixed in this PR is "move between two overlapped components"
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
c86dace
to
3f60570
Compare
igordmn
changed the base branch from
jb-main
to
igor.demin/fix-enter-exit-with-caching
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
3f60570
to
e9fb3ef
Compare
igordmn
added a commit
that referenced
this pull request
Sep 27, 2023
After reverting 1b9c041 in [this PR](#752), enter/exit events don't have a proper order - we can have an enter without exiting another component first. The tests are written in that PR. In the past, we: 1. made some fixes for `isIn` without upstreaming 2. in the upstream added an optimization `1b9c0419` that filter Move's with the same position. 3. Because `1b9c0419` breaks some cases, we reverted it, but this revert is no longer needed, and we reverted it back 4. Because we reverted it back - we enabled the optimization again, but it doesn't work properly with hover 5. It doesn't work properly, because `isIn` shouldn't be cached, it should be reset to `false` every time we do a hit test (resetIsIn), and set to `true` (markIsIn) only for nodes in the hit path. 6. Previously `cleanUpHits` helped us, but it isn't called when we apply optimization (don't send moves with the same position). The order of calling: ``` addHitPath resetIsIn [for hit nodes] markIsIn dispatchChanges buildCache determine if it is enter/exit/move by `isIn` [if position is changed] send it to Compose [if position is changed] cleanUpHits ``` The bugs with hover probably exist in the upstream as well, we will upstream fixes later. # Test 1. Run tests in [this PR] - they succeed 2. Revert this fix, run tests again - some will fail. The test for the case fixed in this PR is "move between two overlapped components"
igordmn
force-pushed
the
igor.demin/fix-enter-exit-with-caching
branch
from
d0deb8e
to
37d72ed
Compare
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
1840a06
to
087dfa7
Compare
igordmn
added a commit
that referenced
this pull request
Sep 27, 2023
After reverting 1b9c041 in [this PR](#752), enter/exit events don't have a proper order - we can have an enter without exiting another component first. The tests are written in that PR. In the past, we: 1. made some fixes for `isIn` without upstreaming 2. in the upstream added an optimization `1b9c0419` that filter Move's with the same position. 3. Because `1b9c0419` breaks some cases, we reverted it, but this revert is no longer needed, and we reverted it back 4. Because we reverted it back - we enabled the optimization again, but it doesn't work properly with hover 5. It doesn't work properly, because `isIn` shouldn't be cached, it should be reset to `false` every time we do a hit test (resetIsIn), and set to `true` (markIsIn) only for nodes in the hit path. 6. Previously `cleanUpHits` helped us, but it isn't called when we apply optimization (don't send moves with the same position). The order of calling: ``` addHitPath resetIsIn [for hit nodes] markIsIn dispatchChanges buildCache determine if it is enter/exit/move by `isIn` [if position is changed] send it to Compose [if position is changed] cleanUpHits ``` The bugs with hover probably exist in the upstream as well, we will upstream fixes later. # Test 1. Run tests in [this PR] - they succeed 2. Revert this fix, run tests again - some will fail. The test for the case fixed in this PR is "move between two overlapped components"
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
087dfa7
to
56bebb9
Compare
igordmn
force-pushed
the
igor.demin/fix-enter-exit-with-caching
branch
from
37d72ed
to
11e4b46
Compare
igordmn
requested review from
m-sasha and
MatkovIvan
and removed request for
m-sasha
Merged
elijah-semyonov
added a commit
that referenced
this pull request
Sep 27, 2023
## PR scope changed The velocity calculation formula is a separate issue now, it will take all iOS platform specific datapoints into account, so no input data correction is needed. Duplicating data points will be fixed in #752. ## Proposed Changes 1. Refactor touch sending on iOS, throw SkikoPointerEvent out as redundant 2. Send coalesced touches(higher than display refresh rate touches) as historical data (don't send them for synthetics). Users can retrieve it now and use in case they need accurate touches data. 3. ~Adjust velocity calculator to filter duplicated data points in a single timestamp (because synthetic touches), leading to inadequate velocity (unexpected scrolls to top)~ 4. ~Override position reported in the `ended` phase with last known position of the touch. Solves two problems~: 4.1. ~Current velocity calculator plays bad with that value, because it's slightly exaggerated to help UIKit with proper fling calculation (opaque implementation detail) and since iOS can report two events in the same frame (`moved` and `ended` with exaggerated position with small timestamp delta, it causes awkward unexpected motions in the end of scroll which was not supposed to lead to a fling)~ 4.2. ~Delta in `ended` phase is not taken into consideration when scrolling UIScrollView (proved by reverse engineering). Passing that metadata in call tree of Draggable implementation will require internal common API changes.~ ## Testing Test: N/A ## Issues Fixed Fixes: [inadequate fling velocity during "quick-drag-and-stop" touch events sequence](JetBrains/compose-multiplatform#3335). ~Awkward motion caused by `ended` delta in the end of the drag without a fling~. ## API Change `ComposeScene.Pointer` now has a new constructor argument `val historical: List<HistoricalChange> = emptyList()` ## Video ~https://github.com/JetBrains/compose-multiplatform-core/assets/4167681/bd5f530f-d26e-4a65-ab54-8085b22b5e1d~ --------- Co-authored-by: dima.avdeev <dima.avdeev@jetbrains.com>
MatkovIvan
approved these changes
Sep 27, 2023
Base automatically changed from
igor.demin/fix-enter-exit-with-caching
to
jb-main
September 27, 2023 18:02
igordmn
added a commit
that referenced
this pull request
Sep 27, 2023
After reverting 1b9c041 in [this PR](#752), enter/exit events don't have a proper order - we can have an enter without exiting another component first. The tests are written in that PR. 1. In the past we made some fixes in HitPathTracker for hover without upstreaming (`isIn`, `hasEntered` are involved) 2. In the upstream there was an optimization that filter Move's with the same position. 3. Because it breaks some cases, we reverted it in 1b9c041, but this adds unneccesary moves, and breaks touch handling 4. We'll revert it back in #752 5. After we enable the optimization again, we still have an issue with hover (Enter/Exit aren't symmetric). This PR prepares the fix. 6. It doesn't work properly, because `isIn` is cached and don't reset to `false` after we return from `dispatchChanges` eagerly. It leads that the old hit nodes won't receive Exit after the new hit test. 7. To solve this, we always reset it to `false` as it was before the optimization (we extracted `cleanUpHover`) The order of calling after [the PR](#752): ``` addHitPath [for new hit nodes] create node with `isIn = true` [for old hit nodes] markIsIn: `isIn = true` dispatchChanges buildCache determine if it is enter/exit/move by `isIn` [if position is changed] send it to Compose [if position is changed] cleanUpHits cleanUpHover: `isIn = false` ``` The bugs with hover probably exist in the upstream as well, we will upstream fixes later. # Test 1. Run MouseMoveTest in [this PR](#752) - it succeed 2. Revert this fix, run tests again - some will fail. The test for the case fixed in this PR is "move between two overlapped components"
This reverts commit 1b9c041 This is no longer needed, it was reverted because it didn't supported by `Modifier.pointerHoverIcon`, it depended on the events each frame. Now `pointerHoverIcon` work differently.
igordmn
force-pushed
the
igor.demin/revert-hitpathtracker
branch
from
56bebb9
to
53cfaab
Compare
igordmn
pushed a commit
that referenced
this pull request
Nov 15, 2023
## PR scope changed The velocity calculation formula is a separate issue now, it will take all iOS platform specific datapoints into account, so no input data correction is needed. Duplicating data points will be fixed in #752. ## Proposed Changes 1. Refactor touch sending on iOS, throw SkikoPointerEvent out as redundant 2. Send coalesced touches(higher than display refresh rate touches) as historical data (don't send them for synthetics). Users can retrieve it now and use in case they need accurate touches data. 3. ~Adjust velocity calculator to filter duplicated data points in a single timestamp (because synthetic touches), leading to inadequate velocity (unexpected scrolls to top)~ 4. ~Override position reported in the `ended` phase with last known position of the touch. Solves two problems~: 4.1. ~Current velocity calculator plays bad with that value, because it's slightly exaggerated to help UIKit with proper fling calculation (opaque implementation detail) and since iOS can report two events in the same frame (`moved` and `ended` with exaggerated position with small timestamp delta, it causes awkward unexpected motions in the end of scroll which was not supposed to lead to a fling)~ 4.2. ~Delta in `ended` phase is not taken into consideration when scrolling UIScrollView (proved by reverse engineering). Passing that metadata in call tree of Draggable implementation will require internal common API changes.~ ## Testing Test: N/A ## Issues Fixed Fixes: [inadequate fling velocity during "quick-drag-and-stop" touch events sequence](JetBrains/compose-multiplatform#3335). ~Awkward motion caused by `ended` delta in the end of the drag without a fling~. ## API Change `ComposeScene.Pointer` now has a new constructor argument `val historical: List<HistoricalChange> = emptyList()` ## Video ~https://github.com/JetBrains/compose-multiplatform-core/assets/4167681/bd5f530f-d26e-4a65-ab54-8085b22b5e1d~ --------- Co-authored-by: dima.avdeev <dima.avdeev@jetbrains.com>
igordmn
added a commit
that referenced
this pull request
Nov 15, 2023
After reverting 1b9c041 in [this PR](#752), enter/exit events don't have a proper order - we can have an enter without exiting another component first. The tests are written in that PR. 1. In the past we made some fixes in HitPathTracker for hover without upstreaming (`isIn`, `hasEntered` are involved) 2. In the upstream there was an optimization that filter Move's with the same position. 3. Because it breaks some cases, we reverted it in 1b9c041, but this adds unneccesary moves, and breaks touch handling 4. We'll revert it back in #752 5. After we enable the optimization again, we still have an issue with hover (Enter/Exit aren't symmetric). This PR prepares the fix. 6. It doesn't work properly, because `isIn` is cached and don't reset to `false` after we return from `dispatchChanges` eagerly. It leads that the old hit nodes won't receive Exit after the new hit test. 7. To solve this, we always reset it to `false` as it was before the optimization (we extracted `cleanUpHover`) The order of calling after [the PR](#752): ``` addHitPath [for new hit nodes] create node with `isIn = true` [for old hit nodes] markIsIn: `isIn = true` dispatchChanges buildCache determine if it is enter/exit/move by `isIn` [if position is changed] send it to Compose [if position is changed] cleanUpHits cleanUpHover: `isIn = false` ``` The bugs with hover probably exist in the upstream as well, we will upstream fixes later. # Test 1. Run MouseMoveTest in [this PR](#752) - it succeed 2. Revert this fix, run tests again - some will fail. The test for the case fixed in this PR is "move between two overlapped components"
igordmn
added a commit
that referenced
this pull request
Nov 15, 2023
This reverts commit 1b9c041 It was reverted because: - it didn't supported by `Modifier.pointerHoverIcon`. In the past it is required that each move should result in an event, even if position is the same, otherwise we will rollback the cursor to the default state. Now `pointerHoverIcon` work differently - it doesn't reset the cursor, and - it didn't work properly with hover. It is fixed [here](#845) Because we fixed all the issues, we can rollback the revert, and apply the optimization that reduces duplicate Move's (Move's with the same position for the same node). Without this optimization, we can spam the client code with multiple Move's, generated by synthetic events, and break touch handling in some cases (in velocity calculation, for example) # Test Run all tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 1b9c041
It was reverted because:
Modifier.pointerHoverIcon. In the past it is required that each move should result in an event, even if position is the same, otherwise we will rollback the cursor to the default state. NowpointerHoverIconwork differently - it doesn't reset the cursor, andBecause we fixed all the issues, we can rollback the revert, and apply the optimization that reduces duplicate Move's (Move's with the same position for the same node). Without this optimization, we can spam the client code with multiple Move's, generated by synthetic events, and break touch handling in some cases (in velocity calculation, for example)
Test
Run all tests