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

Refactor synthetic move event sending #456

Merged
merged 4 commits into from Apr 3, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -3,48 +3,18 @@ package androidx.compose.ui.awt
import androidx.compose.ui.input.key.KeyEvent
import androidx.compose.ui.input.pointer.PointerEvent

/**
* The original raw native event from AWT
*/
@Deprecated(
"Use awtEventOrNull. `awtEvent` will be removed in Compose 1.3",
replaceWith = ReplaceWith("awtEventOrNull")
)
val PointerEvent.awtEvent: java.awt.event.MouseEvent get() {
require(nativeEvent is java.awt.event.MouseEvent) {
"nativeEvent wasn't sent by AWT. Make sure, that you use AWT backed API" +
" (from androidx.compose.ui.awt.* or from androidx.compose.ui.window.*)"
}
return nativeEvent
}

/**
* The original raw native event from AWT
*/
@Deprecated(
"Use awtEventOrNull. `awtEvent` will be removed in Compose 1.3",
replaceWith = ReplaceWith("awtEventOrNull")
)
val KeyEvent.awtEvent: java.awt.event.KeyEvent get() {
require(nativeKeyEvent is java.awt.event.KeyEvent) {
"nativeKeyEvent wasn't sent by AWT. Make sure, that you use AWT backed API" +
" (from androidx.compose.ui.awt.* or from androidx.compose.ui.window.*)"
}
return nativeKeyEvent
}

/**
* 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
Copy link

Choose a reason for hiding this comment

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

"re-layout"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

* - there was a synthetic move event sent by compose when move is missing between two non-move events
*
* Always check for null when you want to handle the native event.
*/
@Suppress("DEPRECATION")
val PointerEvent.awtEventOrNull: java.awt.event.MouseEvent? get() {
if (nativeEvent is SyntheticMouseEvent) return null
return nativeEvent as? java.awt.event.MouseEvent
}

Expand All @@ -53,7 +23,9 @@ val PointerEvent.awtEventOrNull: java.awt.event.MouseEvent? get() {
*
* 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)
*
* Always check for null when you want to handle the native event.
*/
val KeyEvent.awtEventOrNull: java.awt.event.KeyEvent? get() {
return nativeKeyEvent as? java.awt.event.KeyEvent
Expand Down
Expand Up @@ -172,8 +172,7 @@ internal class ComposeLayer(
MainUIDispatcher + coroutineExceptionHandler,
platform,
Density(1f),
_component::needRedraw,
createSyntheticNativeMoveEvent = _component::createSyntheticMouseEvent,
_component::needRedraw
)

private val density get() = _component.density.density
Expand Down Expand Up @@ -289,21 +288,6 @@ internal class ComposeLayer(
}
}

@Suppress("DEPRECATION")
fun createSyntheticMouseEvent(sourceEvent: Any?, positionSourceEvent: Any?): Any {
sourceEvent as MouseEvent
positionSourceEvent as MouseEvent

return SyntheticMouseEvent(
sourceEvent.source as Component,
MouseEvent.MOUSE_MOVED,
sourceEvent.`when`,
sourceEvent.modifiersEx,
positionSourceEvent.x,
positionSourceEvent.y
)
}

private fun refreshWindowFocus() {
platform.windowInfo.isWindowFocused = window?.isFocused ?: false
keyboardModifiersRequireUpdate = true
Expand Down Expand Up @@ -557,14 +541,3 @@ private val MouseEvent.isMacOsCtrlClick
((modifiersEx and InputEvent.BUTTON1_DOWN_MASK) != 0) &&
((modifiersEx and InputEvent.CTRL_DOWN_MASK) != 0)
)

@Deprecated("Will be removed in Compose 1.3")
internal class SyntheticMouseEvent(
source: Component,
id: Int,
`when`: Long,
modifiers: Int,
x: Int,
y: Int
) : MouseEvent(source, id, `when`, modifiers, x, y, 0, false)

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your solution is better, rewrote to it.


scene.sendPointerEvent(PointerEventType.Enter, Offset(-10f, -10f))
background.events.assertReceivedNoEvents()
Expand Down Expand Up @@ -258,7 +259,6 @@ class ComposeSceneInputTest {
independentPopup.events.assertReceivedNoEvents()
}


@Test
fun scroll() = ImageComposeScene(100, 100).use { scene ->
val background = FillBox()
Expand Down
Expand Up @@ -21,8 +21,13 @@ import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.requiredSize
import androidx.compose.runtime.Composable
import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.input.pointer.PointerButtons
import androidx.compose.ui.input.pointer.PointerEvent
import androidx.compose.ui.input.pointer.PointerEventType
import androidx.compose.ui.input.pointer.PointerId
import androidx.compose.ui.input.pointer.PointerInputEvent
import androidx.compose.ui.input.pointer.PointerInputEventData
import androidx.compose.ui.input.pointer.PointerType
import androidx.compose.ui.input.pointer.pointerInput
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.unit.IntRect
Expand All @@ -32,6 +37,7 @@ import androidx.compose.ui.unit.toOffset
import androidx.compose.ui.window.Popup
import androidx.compose.ui.window.PopupPositionProvider
import com.google.common.truth.Truth.assertThat
import kotlin.test.assertContentEquals

fun Events.assertReceivedNoEvents() = assertThat(list).isEmpty()

Expand Down Expand Up @@ -120,4 +126,52 @@ fun Modifier.collectEvents(events: Events) = pointerInput(Unit) {
events.add(awaitPointerEvent())
}
}
}

@OptIn(ExperimentalComposeUiApi::class)
internal fun mouseEvent(
type: PointerEventType,
x: Float,
y: Float,
pressed: Boolean
) = PointerInputEvent(
type,
0,
listOf(
PointerInputEventData(
id = PointerId(0),
uptime = 0,
Offset(x, y),
Offset(x, y),
down = pressed,
pressure = 1f,
type = PointerType.Mouse,
scrollDelta = Offset.Zero
)
),
buttons = PointerButtons(isPrimaryPressed = pressed)
)

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

Choose a reason for hiding this comment

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

Why is the toList() needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it. It is added by mistake.

}

internal fun PointerInputEvent.formatEssential(): String {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

val pointers = if (pointers.size == 1) {
pointers.first().formatEssential()
} else {
pointers.joinToString(" ") {
val id = it.id.value
val data = it.formatEssential()
"$id-$data"
}
}
return "$eventType $pointers"
}

internal fun PointerInputEventData.formatEssential(): String {
val x = position.x.toInt()
val y = position.y.toInt()
val down = if (down) "down" else "up"
return "$x:$y:$down"
}
@@ -0,0 +1,88 @@
/*
* Copyright 2023 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.compose.ui

import androidx.compose.ui.input.pointer.PointerEventType.Companion.Enter
import androidx.compose.ui.input.pointer.PointerEventType.Companion.Exit
import androidx.compose.ui.input.pointer.PointerEventType.Companion.Move
import androidx.compose.ui.input.pointer.PointerEventType.Companion.Press
import androidx.compose.ui.input.pointer.PointerEventType.Companion.Release
import androidx.compose.ui.input.pointer.PointerInputEvent
import kotlin.test.Test

class SyntheticEventSenderTest {
@Test
fun `mouse, shouldn't generate new events if order is correct`() {
syntheticEvents(
mouseEvent(Enter, 10f, 20f, pressed = false),
mouseEvent(Press, 10f, 20f, pressed = true),
mouseEvent(Move, 10f, 30f, pressed = true),
mouseEvent(Release, 10f, 30f, pressed = false),
mouseEvent(Move, 10f, 40f, pressed = false),
mouseEvent(Press, 10f, 40f, pressed = true),
mouseEvent(Release, 10f, 40f, pressed = false),
mouseEvent(Exit, -1f, -1f, pressed = false),
) shouldEqual listOf(
mouseEvent(Enter, 10f, 20f, pressed = false),
mouseEvent(Press, 10f, 20f, pressed = true),
mouseEvent(Move, 10f, 30f, pressed = true),
mouseEvent(Release, 10f, 30f, pressed = false),
mouseEvent(Move, 10f, 40f, pressed = false),
mouseEvent(Press, 10f, 40f, pressed = true),
mouseEvent(Release, 10f, 40f, pressed = false),
mouseEvent(Exit, -1f, -1f, pressed = false),
)
}

@Test
fun `mouse, should generate new move before non-move if position isn't the same`() {
syntheticEvents(
mouseEvent(Enter, 10f, 20f, pressed = false),
mouseEvent(Press, 10f, 25f, pressed = true),
mouseEvent(Move, 10f, 30f, pressed = true),
mouseEvent(Release, 10f, 35f, pressed = false),
mouseEvent(Move, 10f, 40f, pressed = false),
mouseEvent(Press, 10f, 45f, pressed = true),
mouseEvent(Release, 10f, 50f, pressed = false),
mouseEvent(Exit, -1f, -1f, pressed = false),
) shouldEqual listOf(
mouseEvent(Enter, 10f, 20f, pressed = false),
mouseEvent(Move, 10f, 25f, pressed = false),
mouseEvent(Press, 10f, 25f, pressed = true),
mouseEvent(Move, 10f, 30f, pressed = true),
mouseEvent(Move, 10f, 35f, pressed = true),
mouseEvent(Release, 10f, 35f, pressed = false),
mouseEvent(Move, 10f, 40f, pressed = false),
mouseEvent(Move, 10f, 45f, pressed = false),
mouseEvent(Press, 10f, 45f, pressed = true),
mouseEvent(Move, 10f, 50f, pressed = true),
mouseEvent(Release, 10f, 50f, pressed = false),
mouseEvent(Exit, -1f, -1f, pressed = false),
)
}

private fun syntheticEvents(
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is better. Renamed it.

vararg inputEvents: PointerInputEvent
): List<PointerInputEvent> {
val received = mutableListOf<PointerInputEvent>()
val sender = SyntheticEventSender(received::add)
for (inputEvent in inputEvents) {
sender.send(inputEvent)
}
return received
}
}
Expand Up @@ -93,10 +93,7 @@ class ComposeScene internal constructor(
coroutineContext: CoroutineContext = Dispatchers.Unconfined,
internal val platform: Platform,
density: Density = Density(1f),
private val invalidate: () -> Unit = {},
@Deprecated("Will be removed in Compose 1.3")
internal val createSyntheticNativeMoveEvent:
(sourceEvent: Any?, positionSourceEvent: Any?) -> Any? = { _, _ -> null }
private val invalidate: () -> Unit = {}
) {
/**
* Constructs [ComposeScene]
Expand Down Expand Up @@ -215,7 +212,10 @@ class ComposeScene internal constructor(

private val recomposer = Recomposer(coroutineContext + job + effectDispatcher)

internal val pointerPositionUpdater = PointerPositionUpdater(::invalidateIfNeeded, ::sendAsMove)
private val syntheticEventSender = SyntheticEventSender(::processPointerInput)
internal val pointerPositionUpdater = PointerPositionUpdater(
::invalidateIfNeeded, syntheticEventSender
)

internal var mainOwner: SkiaBasedOwner? = null
private var composition: Composition? = null
Expand Down Expand Up @@ -354,6 +354,7 @@ class ComposeScene internal constructor(
content: @Composable () -> Unit
) {
check(!isClosed) { "ComposeScene is closed" }
syntheticEventSender.reset()
pointerPositionUpdater.reset()
composition?.dispose()
mainOwner?.dispose()
Expand Down Expand Up @@ -442,7 +443,7 @@ class ComposeScene internal constructor(
* is platform-dependent.
* @param type The device type that produced the event, such as [mouse][PointerType.Mouse],
* or [touch][PointerType.Touch].
* @param buttons Contains the state of pointer buttons (e.g. mouse and stylus buttons).
* @param buttons Contains the state of pointer buttons (e.g. mouse and stylus buttons) after the event.
* @param keyboardModifiers Contains the state of modifier keys, such as Shift, Control,
* and Alt, as well as the state of the lock keys, such as Caps Lock and Num Lock.
* @param nativeEvent The original native event.
Expand Down Expand Up @@ -479,8 +480,8 @@ class ComposeScene internal constructor(
)
needLayout = false
forEachOwner { it.measureAndLayout() }
pointerPositionUpdater.beforeEvent(event)
processPointerInput(event)
pointerPositionUpdater.update()
Copy link
Collaborator Author

@igordmn igordmn Mar 28, 2023

Choose a reason for hiding this comment

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

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.

syntheticEventSender.send(event)
updatePointerPositions(event)
}

Expand All @@ -507,16 +508,6 @@ class ComposeScene internal constructor(
}
}

@Suppress("DEPRECATION")
private fun sendAsMove(sourceEvent: PointerInputEvent, positionSourceEvent: PointerInputEvent) {
val nativeEvent = createSyntheticNativeMoveEvent(
sourceEvent.nativeEvent,
positionSourceEvent.nativeEvent
)
processPointerInput(createMoveEvent(nativeEvent, sourceEvent, positionSourceEvent))
}

@OptIn(ExperimentalComposeUiApi::class)
private fun processPointerInput(event: PointerInputEvent) {
when (event.eventType) {
PointerEventType.Press -> processPress(event)
Expand Down