Skip to content

Commit

Permalink
Remove pointerId and isMousePressed states from ComposeScene
Browse files Browse the repository at this point in the history 
1. Remove pointerId

pointerId was introduced in https://android-review.googlesource.com/c/platform/frameworks/support/+/1402607, because double click didn't work

But it messes with hover and clicking multiple mouse buttons at the same time.

Double clicking still works after removing it.

Fixes JetBrains/compose-multiplatform#1176

2. Remove isMousePressed

mousePressed is unreliable on Windows (we can miss the release event), and doesn't work well with multiple buttons (the second pressed button resets the state).

Fixes JetBrains/compose-multiplatform#1426

Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
Change-Id: Idd9d17302f41dfcb6eaa7d1259034200d107bcca

# Conflicts:
#	compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/awt/ComposeWindowTest.kt
  • Loading branch information
@igordmn
igordmn committed Mar 28, 2022
1 parent 12f6319 commit 9ba813e
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 63 deletions.
41 changes: 22 additions & 19 deletions compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/awt/ComposeWindowTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,27 +45,30 @@ import org.junit.Test
class ComposeWindowTest {
// bug https://github.com/JetBrains/compose-jb/issues/1448
@Test
fun `dispose window inside event handler`() = runApplicationTest {
var isClickHappened = false

fun `dispose window in event handler`() = runApplicationTest {
val window = ComposeWindow()
window.isUndecorated = true
window.size = Dimension(200, 200)
window.setContent {
Box(modifier = Modifier.fillMaxSize().background(Color.Blue).clickable {
isClickHappened = true
window.dispose()
})
try {
var isClickHappened = false
window.size = Dimension(300, 400)
window.setContent {
Box(modifier = Modifier.fillMaxSize().background(Color.Blue).clickable {
isClickHappened = true
window.dispose()
})
}
window.isVisible = true
window.sendMouseEvent(MOUSE_ENTERED, 100, 50)
awaitIdle()
window.sendMouseEvent(MOUSE_MOVED, 100, 50)
awaitIdle()
window.sendMouseEvent(MOUSE_PRESSED, 100, 50, modifiers = BUTTON1_DOWN_MASK)
awaitIdle()
window.sendMouseEvent(MOUSE_RELEASED, 100, 50)
awaitIdle()
assertThat(isClickHappened).isTrue()
} finally {
window.dispose()
}

window.isVisible = true
awaitIdle()

window.sendMouseEvent(MOUSE_PRESSED, x = 100, y = 50)
window.sendMouseEvent(MOUSE_RELEASED, x = 100, y = 50)
awaitIdle()

assertThat(isClickHappened).isTrue()
}

@Test
Expand Down
73 changes: 43 additions & 30 deletions ...se/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/window/window/WindowInputEventTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
* limitations under the License.
*/

@file:Suppress("DEPRECATION") // https://github.com/JetBrains/compose-jb/issues/1514

package androidx.compose.ui.window.window

import androidx.compose.foundation.layout.Box
Expand Down Expand Up @@ -49,11 +47,19 @@ import androidx.compose.ui.window.launchApplication
import androidx.compose.ui.window.rememberWindowState
import androidx.compose.ui.window.runApplicationTest
import com.google.common.truth.Truth.assertThat
import org.junit.Test
import java.awt.Toolkit
import java.awt.event.KeyEvent
import java.awt.event.MouseEvent
import java.awt.event.MouseWheelEvent
import java.awt.event.MouseEvent.BUTTON1_DOWN_MASK
import java.awt.event.MouseEvent.BUTTON3_DOWN_MASK
import java.awt.event.MouseEvent.CTRL_DOWN_MASK
import java.awt.event.MouseEvent.MOUSE_DRAGGED
import java.awt.event.MouseEvent.MOUSE_PRESSED
import java.awt.event.MouseEvent.MOUSE_RELEASED
import java.awt.event.MouseEvent.MOUSE_WHEEL
import java.awt.event.MouseEvent.SHIFT_DOWN_MASK
import java.awt.event.MouseWheelEvent.WHEEL_UNIT_SCROLL
import org.junit.Test

@OptIn(ExperimentalComposeUiApi::class)
class WindowInputEventTest {
Expand Down Expand Up @@ -215,8 +221,8 @@ class WindowInputEventTest {

Box(
Modifier.fillMaxSize().pointerInput(events) {
while (true) {
awaitPointerEventScope {
awaitPointerEventScope {
while (true) {
events += awaitPointerEvent()
}
}
Expand All @@ -229,21 +235,28 @@ class WindowInputEventTest {
awaitIdle()
assertThat(events.size).isEqualTo(0)

window.sendMouseEvent(MouseEvent.MOUSE_PRESSED, x = 100, y = 50)
window.sendMouseEvent(MouseEvent.MOUSE_ENTERED, x = 100, y = 50)
awaitIdle()
assertThat(events.size).isEqualTo(1)
assertThat(events.last().pressed).isEqualTo(true)
assertThat(events.last().pressed).isEqualTo(false)
assertThat(events.last().position).isEqualTo(Offset(100 * density, 50 * density))

window.sendMouseEvent(MouseEvent.MOUSE_DRAGGED, x = 90, y = 40)
window.sendMouseEvent(MOUSE_PRESSED, 100, 50, modifiers = BUTTON1_DOWN_MASK)
awaitIdle()
assertThat(events.size).isEqualTo(2)
assertThat(events.last().pressed).isEqualTo(true)
assertThat(events.last().position).isEqualTo(Offset(90 * density, 40 * density))
assertThat(events.last().position).isEqualTo(Offset(100 * density, 50 * density))

window.sendMouseEvent(MouseEvent.MOUSE_RELEASED, x = 80, y = 30)
window.sendMouseEvent(MOUSE_DRAGGED, 90, 40, modifiers = BUTTON1_DOWN_MASK)
awaitIdle()
assertThat(events.size).isEqualTo(3)
assertThat(events.last().pressed).isEqualTo(true)
assertThat(events.last().position).isEqualTo(Offset(90 * density, 40 * density))

window.sendMouseEvent(MOUSE_RELEASED, 80, 30)
awaitIdle()
assertThat(events.size).isEqualTo(4)
assertThat(events.last().type).isEqualTo(PointerEventType.Release)
assertThat(events.last().pressed).isEqualTo(false)
assertThat(events.last().position).isEqualTo(Offset(80 * density, 30 * density))

Expand Down Expand Up @@ -296,9 +309,9 @@ class WindowInputEventTest {
assertThat(onEnters).isEqualTo(1)
assertThat(onExits).isEqualTo(0)

window.sendMouseEvent(MouseEvent.MOUSE_PRESSED, x = 90, y = 50)
window.sendMouseEvent(MouseEvent.MOUSE_DRAGGED, x = 80, y = 50)
window.sendMouseEvent(MouseEvent.MOUSE_RELEASED, x = 80, y = 50)
window.sendMouseEvent(MOUSE_PRESSED, x = 90, y = 50, modifiers = BUTTON1_DOWN_MASK)
window.sendMouseEvent(MOUSE_DRAGGED, x = 80, y = 50, modifiers = BUTTON1_DOWN_MASK)
window.sendMouseEvent(MOUSE_RELEASED, x = 80, y = 50)
awaitIdle()
assertThat(onMoves.size).isEqualTo(2)
assertThat(onMoves.last()).isEqualTo(Offset(80 * density, 50 * density))
Expand Down Expand Up @@ -342,21 +355,21 @@ class WindowInputEventTest {
assertThat(deltas.size).isEqualTo(0)

window.sendMouseWheelEvent(
MouseEvent.MOUSE_WHEEL,
MOUSE_WHEEL,
x = 100,
y = 50,
scrollType = MouseWheelEvent.WHEEL_UNIT_SCROLL,
scrollType = WHEEL_UNIT_SCROLL,
wheelRotation = 1
)
awaitIdle()
assertThat(deltas.size).isEqualTo(1)
assertThat(deltas.last()).isEqualTo(Offset(0f, 1f))

window.sendMouseWheelEvent(
MouseEvent.MOUSE_WHEEL,
MOUSE_WHEEL,
x = 100,
y = 50,
scrollType = MouseWheelEvent.WHEEL_UNIT_SCROLL,
scrollType = WHEEL_UNIT_SCROLL,
wheelRotation = -1
)
awaitIdle()
Expand Down Expand Up @@ -396,10 +409,10 @@ class WindowInputEventTest {

repeat(eventCount) {
window.sendMouseWheelEvent(
MouseEvent.MOUSE_WHEEL,
MOUSE_WHEEL,
x = 100,
y = 50,
scrollType = MouseWheelEvent.WHEEL_UNIT_SCROLL,
scrollType = WHEEL_UNIT_SCROLL,
wheelRotation = 1
)
}
Expand Down Expand Up @@ -440,10 +453,10 @@ class WindowInputEventTest {

repeat(eventCount) {
window.sendMouseWheelEvent(
MouseEvent.MOUSE_WHEEL,
MOUSE_WHEEL,
x = 100,
y = 50,
scrollType = MouseWheelEvent.WHEEL_UNIT_SCROLL,
scrollType = WHEEL_UNIT_SCROLL,
wheelRotation = 1
)
}
Expand Down Expand Up @@ -486,11 +499,11 @@ class WindowInputEventTest {
awaitIdle()

window.sendMouseEvent(
MouseEvent.MOUSE_PRESSED,
MOUSE_PRESSED,
x = 100,
y = 50,
modifiers = MouseEvent.SHIFT_DOWN_MASK or MouseEvent.CTRL_DOWN_MASK or
MouseEvent.BUTTON1_DOWN_MASK or MouseEvent.BUTTON3_DOWN_MASK
modifiers = SHIFT_DOWN_MASK or CTRL_DOWN_MASK or
BUTTON1_DOWN_MASK or BUTTON3_DOWN_MASK
)

awaitIdle()
Expand All @@ -513,13 +526,13 @@ class WindowInputEventTest {
)

window.sendMouseWheelEvent(
MouseEvent.MOUSE_WHEEL,
MOUSE_WHEEL,
x = 100,
y = 50,
scrollType = MouseWheelEvent.WHEEL_UNIT_SCROLL,
scrollType = WHEEL_UNIT_SCROLL,
wheelRotation = 1,
modifiers = MouseEvent.SHIFT_DOWN_MASK or MouseEvent.CTRL_DOWN_MASK or
MouseEvent.BUTTON1_DOWN_MASK or MouseEvent.BUTTON3_DOWN_MASK
modifiers = SHIFT_DOWN_MASK or CTRL_DOWN_MASK or
BUTTON1_DOWN_MASK or BUTTON3_DOWN_MASK
)

awaitIdle()
Expand Down Expand Up @@ -596,4 +609,4 @@ class WindowInputEventTest {

private val PointerEvent.pressed get() = changes.first().pressed
private val PointerEvent.position get() = changes.first().position
}
}
17 changes: 3 additions & 14 deletions compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import androidx.compose.ui.input.pointer.PointerInputEvent
import androidx.compose.ui.input.pointer.PointerInputEventData
import androidx.compose.ui.input.pointer.PointerKeyboardModifiers
import androidx.compose.ui.input.pointer.PointerType
import androidx.compose.ui.input.pointer.areAnyPressed
import androidx.compose.ui.node.LayoutNode
import androidx.compose.ui.platform.AccessibilityController
import androidx.compose.ui.platform.PlatformComponent
Expand Down Expand Up @@ -166,9 +167,6 @@ class ComposeScene internal constructor(

private val defaultPointerStateTracker = DefaultPointerStateTracker()

private var pointerId = 0L
private var isMousePressed = false

private val job = Job()
private val coroutineScope = CoroutineScope(coroutineContext + job)
// We use FlushCoroutineDispatcher for effectDispatcher not because we need `flush` for
Expand Down Expand Up @@ -403,18 +401,12 @@ class ComposeScene internal constructor(
val actualKeyboardModifiers =
keyboardModifiers ?: defaultPointerStateTracker.keyboardModifiers

when (eventType) {
PointerEventType.Press -> isMousePressed = true
PointerEventType.Release -> isMousePressed = false
}
val event = pointerInputEvent(
eventType,
position,
timeMillis,
nativeEvent,
type,
isMousePressed,
pointerId,
scrollDelta,
actualButtons,
actualKeyboardModifiers
Expand Down Expand Up @@ -455,7 +447,6 @@ class ComposeScene internal constructor(
private fun onMouseReleased(event: PointerInputEvent) {
val owner = hoveredOwner ?: focusedOwner
owner?.processPointerInput(event)
pointerId += 1
}

private var pointLocation = Offset.Zero
Expand Down Expand Up @@ -494,8 +485,6 @@ private fun pointerInputEvent(
timeMillis: Long,
nativeEvent: Any?,
type: PointerType,
isMousePressed: Boolean,
pointerId: Long,
scrollDelta: Offset,
buttons: PointerButtons,
keyboardModifiers: PointerKeyboardModifiers
Expand All @@ -505,11 +494,11 @@ private fun pointerInputEvent(
timeMillis,
listOf(
PointerInputEventData(
PointerId(pointerId),
PointerId(0),
timeMillis,
position,
position,
isMousePressed,
buttons.areAnyPressed,
type,
scrollDelta = scrollDelta
)
Expand Down

0 comments on commit 9ba813e

Please sign in to comment.