Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't send synthetic Move events before Press/Release for touch #870

Merged
merged 3 commits into from Oct 12, 2023

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Oct 11, 2023

These kind of synthetics were added to fix missed Enter/Exit events:

Box1 | Box2

If we have this chain of native events:

Move on Box1
Press on Box1
Release on Box2
Move on Box2

Without synthetic events, Compose receives:

Enter on Box1
Press on Box1
Release on Box1 (because Box1 takes ownership of the all events)
Enter on Box2

With synthetic events:

Enter on Box1
Press on Box1
Exit on Box1
Release on Box1
Enter on Box2

Touch doesn't have Enter/Exit, so we don't have to send them. Furthermore, such kind of synthetics can bring issues when calculating the end fling velocity (the end position should be excluded from the velocity calculation on iOS). This isn't an issue for mouse.

Testing

  • run existing tests
  • run iOS demo and check that click, and scrolling works

Issues Fixed

https://youtrack.jetbrains.com/issue/COMPOSE-474/Fix-synthetic-events-before-end-event-with-different-position

These kind of synthetics were added to fix missed Enter/Exit events:

Box1 | Box2

If we have this chain of native events:
```
Move on Box1
Press on Box1
Release on Box2
Move on Box2
```
Without synthetic events, Compose receives:
```
Enter on Box1
Press on Box1
Release on Box1 (because Box1 takes ownership of the all events)
Enter on Box2
```
With synthetic events:
```
Enter on Box1
Press on Box1
Exit on Box1
Release on Box1
Enter on Box2
```

Touch doesn't have Enter/Exit, so we don't have to send them. Furthermore, such kind of synthetics can bring issues when calculating the end fling velocity (the end position should be excluded from the velocity calculation on iOS). This isn't an issue for mouse.

## Testing

- run existing tests
- run iOS demo and try usual cases

## Issues Fixed

https://youtrack.jetbrains.com/issue/COMPOSE-474/Fix-synthetic-events-before-end-event-with-different-position
@@ -972,6 +972,7 @@ private fun pointerInputEvent(
it.pressed,
it.pressure,
it.type,
issuesEnterExit = it.type == PointerType.Mouse,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need issuesEnterExit field at all? It seems that type == Mouse should be enough.

If we really need it, I suggest adding a doc comment with the reason why it's needed and probably rename to something like issuesHover

Copy link
Collaborator Author

@igordmn igordmn Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't a new field, it was added by Google in commonMain and it means that the pointer can move between nodes and generate Enter/Exit. It exists and used by the internal common code, so we have to set it anyway.

As for checking type == Mouse instead, it is always better to check a characteristic of the pointer, not a specific type of it. Imagine, we add a new pointer VR Controller, we have to add it everywhere where we added Mouse. Stylus also can support hover without pressing the screen.

I don't like issuesEnterExit, issuesHover sounds better, but we already have it, so we can't change it easely. Added a comment in SyntheticEventSender.

The same is applicable for our other checks. For example, for primary press checks we should add something like isPrimaryButtonCausesDown (naming is hard).

@igordmn igordmn merged commit e0bdb0e into jb-main Oct 12, 2023
3 checks passed
@igordmn igordmn deleted the igor.demin/fix-end-synthetic-event branch October 12, 2023 11:01
background.events.assertReceived(
PointerEventType.Release,
touch(1f, 1f, pressed = false, id = 1),
touch(1f, 2f, pressed = true, id = 2),
touch(10f, 55f, pressed = false, id = 1),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The position of synthetic Release equals the position of the last Move. Because we don't have a synthetic move before Release, we take the real's Move position, not synthetic one.

@@ -313,6 +313,11 @@ class DragGestureTest {
assertFalse(dragCanceled)
assertFalse(dragEnded)

scene.sendPointerEvent(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sent because we test a drag to Offset(5f + viewConfiguration.touchSlop, 15f), position. But because it is different from the previous Move position and we don't send a synthetic now, we send a real Move now

mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
These kind of synthetics were added to fix missed Enter/Exit events:

Box1 | Box2

If we have this chain of native events:
```
Move on Box1
Press on Box1
Release on Box2
Move on Box2
```
Without synthetic events, Compose receives:
```
Enter on Box1
Press on Box1
Release on Box1 (because Box1 takes ownership of the all events)
Enter on Box2
```
With synthetic events:
```
Enter on Box1
Press on Box1
Exit on Box1
Release on Box1
Enter on Box2
```

Touch doesn't have Enter/Exit, so we don't have to send them.
Furthermore, such kind of synthetics can bring issues when calculating
the end fling velocity (the end position should be excluded from the
velocity calculation on iOS). This isn't an issue for mouse.

## Testing

- run existing tests
- run iOS demo and check that click, and scrolling works

## Issues Fixed


https://youtrack.jetbrains.com/issue/COMPOSE-474/Fix-synthetic-events-before-end-event-with-different-position
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants