Refactor synthetic move event sending #456

igordmn · 2023-03-28T11:45:22Z

Create SyntheticEventSender, purpose of which to generate synthetic events based on the previous one. Extract necessary functionality from PointerPositionUpdater.
PointerPositionUpdater main purpose is update cursor position for the scene via reusing SyntheticEventSender
Remove deprecated functions

It is needed for my next PR with multitouch implementation where we need to send synthetic presses/releases

API Changes:

Remove deprecated SkiaRootForTest.processPointerInput
Remove deprecated PointerEvent.awtEvent, KeyEvent.awtEvent

igordmn · 2023-03-28T11:47:30Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/SyntheticEventSender.kt

+            eventType == PointerEventType.Enter ||
+            eventType == PointerEventType.Exit
+
+    private fun PointerInputEvent.isSamePosition(previousEvent: PointerInputEvent?): Boolean {


Here the difference compared to the previous code. Now we support multitouch for this function - compare by ids

igordmn · 2023-03-28T12:19:36Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

@@ -479,8 +480,8 @@ class ComposeScene internal constructor(
        )
        needLayout = false
        forEachOwner { it.measureAndLayout() }
-        pointerPositionUpdater.beforeEvent(event)
-        processPointerInput(event)
+        pointerPositionUpdater.update()


Here the difference with the previous code.

In case of relayout, we send always one additional move that is based on the last Move event, not on the current. Even if after that we send one more Move (that was passed to this function).

It was a missing event without which we can have some inconsistent behavior. Imagine a game that has a square that jumps to random position after we hover it:

move event 1 (300, 300) -> processPointerInput -> hover the square -> square receives Enter, and we set squarePosition = (750, 100) move event 2 (750, 100) -> measureAndLayout, layout square to (750, 100) -> needUpdate() -> update -> send Move (300, 300) -> square recieves Exit -> ... processPointerInput -> 750, 100 -> square receives Enter.

The example is very synthtic thought, and in reality we don't need this. But besides that, I remember that Compose nodes don't work well if we send a different Move after the node changes its position (it is probably a bug of Compose node), so I would keep it just as a precaution.

1. Create SyntheticEventSender, purpose of which to generate synthetic events based on the previous one. Extract necessary functionality from PointerPositionUpdater. 2. PointerPositionUpdater main purpose is update cursor position for the scene via reusing SyntheticEventSender 3. Remove deprecated functions It is needed for my next PR with multitouch implementation where we need to send synthetic presses/releases API Changes: 1. Removed deprecated SkiaRootForTest.processPointerInput

m-sasha

Is this just a refactoring (keeping the existing behavior) to prepare for multi-touch, or are there behavior changes here too?
Because I remember that we realized the synthetic events we generate and the event processing in HitPathTracker were conflicting with each other, and we said that maybe it all should be done in one place.

m-sasha · 2023-03-30T14:33:17Z

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ComposeSceneInputTest.kt

@@ -65,6 +65,7 @@ class ComposeSceneInputTest {
            overlappedPopup.Content()
            independentPopup.Content()
        }
+        scene.render() // Popup has 2-frame layout passes. Call it to avoid synthetic events


My solution (below in pressed popup should own received moves outside popup) was a bit different. Maybe make them the same, whether yours or mine.

Your solution is better, rewrote to it.

m-sasha · 2023-03-30T14:41:38Z

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/EventTestUtils.kt

+)
+
+internal infix fun List<PointerInputEvent>.shouldEqual(expected: List<PointerInputEvent>) {
+    assertContentEquals(expected.toList().map { it.formatEssential() }, toList().map { it.formatEssential() })


Why is the toList() needed?

Removed it. It is added by mistake.

m-sasha · 2023-03-30T14:48:13Z

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/EventTestUtils.kt

+    assertContentEquals(expected.toList().map { it.formatEssential() }, toList().map { it.formatEssential() })
+}
+
+internal fun PointerInputEvent.formatEssential(): String {


Some of the fields are not used here (buttons, keyboardModifiers etc.). If that's intentional, it should be documented in shouldEqual, or just call it something like positionAndDownShouldEqual.
Someone could use it without reading it fully, expecting it to do a full equality check.

m-sasha · 2023-03-30T19:06:52Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/SyntheticEventSender.kt

+            previousEvent.copySynthetic(PointerEventType.Move) {
+                copySynthetic(position = idToPosition[id] ?: position)
+            }


I think it's confusing to use a trailing lambda like this - it's not clear what that lambda is operating on.
I'd call it with an explicit argument (and rename the argument):

previousEvent.copySynthetic( type = PointerEventType.Move, pointerMapper = { ... } )

m-sasha · 2023-03-30T19:07:36Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/SyntheticEventSender.kt

+    // we don't use copy here to not forget to nullify properties that shouldn't be in a synthetic event
+    private fun PointerInputEvent.copySynthetic(
+        type: PointerEventType,
+        pointer: PointerInputEventData.() -> PointerInputEventData,


Feels like it PointerInputEventData should be a regular argument, not a receiver.

m-sasha · 2023-03-30T19:14:29Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/SyntheticEventSender.kt

+        sendSyntheticMove(previousEvent)
+    }
+
+    private fun sendSyntheticMove(pointerSourceEvent: PointerInputEvent) {


I think just sourceEvent is enough.

The pointerSource here tells us that we take this event as "a source of pointers", not that it is "source pointer event". Renamed it to pointersSourceEvent and added a doc.

m-sasha · 2023-03-30T19:16:06Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/SyntheticEventSender.kt

+        )
+    }
+
+    private fun sendMissingMove(currentEvent: PointerInputEvent) {


Maybe sendMoveIfMissing, or just inline it into send. It's just two lines of code...

I add sendMissingPresses, sendMissingReleases in the next PR, where is a lot of code. For consistency, better to keep this code in a separate function.

Also, if we rename to sendMoveIfMissing, we have to rename the other to sendPressesIfMissing, which is not a right sentence (it tells us that we either send 3 presses if missing, or 0 presses, not in between).

m-sasha · 2023-03-30T19:22:36Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/PointerPositionUpdater.skiko.kt

+ *
+ * For example, it can be needed when we scroll content without moving the pointer, and we need
+ * to highlight the items under the pointer.
+ */
 internal class PointerPositionUpdater(


Consider folding PointerPositionUpdater into SyntheticEventSender. They seem to be dealing with the same problem.

It was extracted because in reality they dealing with different problems.

PointerPositionUpdater informs Compose content about the updated position after relayout. Now it informs via sending synthetic events, but that just one of the options.

SyntheticEventSender encapsulates generation of synthetic events based on the previous events. Besides, in the next PR I add generation of Press/Release, which doesn't fit PointerPositionUpdater

m-sasha · 2023-03-30T19:28:28Z

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/SyntheticEventSenderTest.kt

+        )
+    }
+
+    private fun syntheticEvents(


Maybe call this eventsSentBy or something like that, because it returns all the events, not just the synthetic ones.

Yes, it is better. Renamed it.

m-sasha · 2023-03-30T19:33:48Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/AwtEvents.desktop.kt

 /**
 * The original raw native event from AWT.
 *
 * Null if:
 * - the native event is sent by another framework (when Compose UI is embed into it)
- * - there no native event (in tests, for example)
+ * - there is no native event (in tests, for example)
 * - there was a synthetic move event sent by compose on relayout


"re-layout"

igordmn · 2023-04-03T06:56:27Z

Is this just a refactoring (keeping the existing behavior)

It is refactoring + one side effect of refactoring (see the second comment) + one additional change for multitouch (see my first comment. I should put this in the next PR, but it is late to move it there).

I remember that we realized the synthetic events we generate and the event processing in HitPathTracker were conflicting with each other

Regarding this nothing changed, just a refactoring of the old code, to prepare it for multitouch

…r-synthetic-events # Conflicts: # compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

igordmn commented Mar 28, 2023

View reviewed changes

igordmn force-pushed the igor.demin/refactor-synthetic-events branch from cfc54c2 to 6436b1d Compare March 29, 2023 05:13

igordmn requested a review from m-sasha March 29, 2023 08:03

Base automatically changed from igor.demin/multiplatform-demo-example to jb-main March 29, 2023 13:48

igordmn force-pushed the igor.demin/refactor-synthetic-events branch from 1567999 to e7c18eb Compare March 29, 2023 13:53

igordmn force-pushed the igor.demin/refactor-synthetic-events branch from e7c18eb to e1eb6b5 Compare March 29, 2023 13:57

m-sasha requested changes Mar 30, 2023

View reviewed changes

igordmn added 3 commits April 3, 2023 09:21

Discussions

36b9366

Merge remote-tracking branch 'origin/jb-main' into igor.demin/refacto…

5c572f8

…r-synthetic-events # Conflicts: # compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

Discussions

9914d08

m-sasha approved these changes Apr 3, 2023

View reviewed changes

igordmn merged commit 7d26cf5 into jb-main Apr 3, 2023
1 check failed

igordmn deleted the igor.demin/refactor-synthetic-events branch April 3, 2023 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor synthetic move event sending #456

Refactor synthetic move event sending #456

igordmn commented Mar 28, 2023 •

edited

igordmn Mar 28, 2023

igordmn Mar 28, 2023 •

edited

m-sasha left a comment

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

m-sasha Mar 30, 2023

m-sasha Mar 30, 2023

igordmn Apr 3, 2023 •

edited

m-sasha Mar 30, 2023

igordmn Apr 3, 2023 •

edited

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

m-sasha Mar 30, 2023

igordmn Apr 3, 2023

igordmn commented Apr 3, 2023

Refactor synthetic move event sending #456

Refactor synthetic move event sending #456

Conversation

igordmn commented Mar 28, 2023 • edited

Choose a reason for hiding this comment

igordmn Mar 28, 2023 • edited

Choose a reason for hiding this comment

m-sasha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn Apr 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn Apr 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn commented Apr 3, 2023

igordmn commented Mar 28, 2023 •

edited

igordmn Mar 28, 2023 •

edited

igordmn Apr 3, 2023 •

edited

igordmn Apr 3, 2023 •

edited