Skip to content

Commit

Permalink
Fix TapGestureDetector mutex is not locked error on Touchscreen (#824)
Browse files Browse the repository at this point in the history
Fixes JetBrains/compose-multiplatform#3655

The issue is introduced after migration to `awaitPointerEventScope`,
this code is indeed has a race condition, as it was assumed in the TODO.
When we send Press/Release without any moves - we go via this path:
```
// awaitPointerEventScope isn't called until it receives the first event
awaitPointerEventScope {
  // in the beginning of the method we have 2 events already in the queue

  launch { pressScope.reset() } // launch async, reset isn't called
  val down = awaitPress // don't suspend here, because we already have Press
  awaitReleaseOrCancelled() // don't suspend here, because we already have Release
  pressScope.release() // crash, because `pressScope.reset()` isn't called yet
}
```

To avoid races, we should always access `pressScope` in `launch` (this
way the calls maintain order).

The same approach is used in the original `TapGestureDetector.kt` (the
change in commit 32de9dd)

Also, the rest of the code doesn't have races with `pressScope` calls.

## Testing
- haven't reproduced with mouse
- reproduced with a simple test (added to OnClickTest)
  • Loading branch information
igordmn authored and elijah-semyonov committed Sep 21, 2023
1 parent cdc5b8e commit 13a0944
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,6 @@ suspend fun PointerInputScope.detectTapGestures(

while (currentCoroutineContext().isActive) {
awaitPointerEventScope {
// TODO: [1.4 Update] seems that launch may introduce races?
// TapGestureDetectorKt.detectTapGestures also use it
launch { pressScope.reset() }

val down = awaitPress(filter = filter, requireUnconsumed = true).apply { changes[0].consume() }
Expand All @@ -119,10 +117,10 @@ suspend fun PointerInputScope.detectTapGestures(
}

if (cancelled) {
pressScope.cancel()
launch { pressScope.cancel() }
return@awaitPointerEventScope
} else if (firstRelease != null) {
pressScope.release()
launch { pressScope.release() }
}

if (firstRelease == null) {
Expand All @@ -132,7 +130,7 @@ suspend fun PointerInputScope.detectTapGestures(
consumeUntilRelease = true,
filter = longClickReleaseFilter
)
pressScope.release()
launch { pressScope.release() }
}
} else if (onDoubleTap == null) {
onTap?.invoke(firstRelease.changes[0].position)
Expand All @@ -146,8 +144,6 @@ suspend fun PointerInputScope.detectTapGestures(
if (secondPress == null) {
onTap?.invoke(firstRelease.changes[0].position)
} else {
// TODO: [1.4 Update] seems that launch may introduce races?
// TapGestureDetectorKt.detectTapGestures also use it
launch { pressScope.reset() }
launch { pressScope.onPress(secondPress.changes[0].position) }

Expand All @@ -161,10 +157,10 @@ suspend fun PointerInputScope.detectTapGestures(
}

if (cancelled) {
pressScope.cancel()
launch { pressScope.cancel() }
return@awaitPointerEventScope
} else if (secondRelease != null) {
pressScope.release()
launch { pressScope.release() }
}

if (secondRelease == null) {
Expand All @@ -174,7 +170,7 @@ suspend fun PointerInputScope.detectTapGestures(
consumeUntilRelease = true,
filter = longClickReleaseFilter
)
pressScope.release()
launch { pressScope.release() }
}
} else if (!cancelled) {
onDoubleTap(secondRelease.changes[0].position)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package androidx.compose.foundation

import androidx.compose.foundation.gestures.detectTransformGestures
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.size
import androidx.compose.ui.ComposeScene
import androidx.compose.ui.ImageComposeScene
import androidx.compose.ui.Modifier
import androidx.compose.ui.geometry.Offset
Expand All @@ -32,11 +34,13 @@ import androidx.compose.ui.platform.LocalViewConfiguration
import androidx.compose.ui.platform.ViewConfiguration
import androidx.compose.ui.test.ExperimentalTestApi
import androidx.compose.ui.test.runSkikoComposeUiTest
import androidx.compose.ui.unit.Constraints
import androidx.compose.ui.unit.Density
import androidx.compose.ui.unit.dp
import androidx.compose.ui.use
import kotlin.test.Test
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.runTest

@ExperimentalCoroutinesApi
@OptIn(ExperimentalFoundationApi::class)
Expand Down Expand Up @@ -72,6 +76,37 @@ class OnClickTest {
assertThat(clicksCount).isEqualTo(2)
}

// https://github.com/JetBrains/compose-multiplatform/issues/3655
// Isn't reproducible with ImageComposeScene because of Dispatchers.Unconfined
@OptIn(ExperimentalFoundationApi::class, ExperimentalCoroutinesApi::class)
@Test
fun simpleClickWithoutMove() = runTest {
val scene = ComposeScene(coroutineContext = coroutineContext)
try {
scene.constraints = Constraints.fixed(100, 100)
scene.setContent {
Box(
Modifier
.onClick {}
.fillMaxSize()
)
}

scene.sendPointerEvent(
PointerEventType.Press,
Offset(30f, 10f),
button = PointerButton.Primary
)
scene.sendPointerEvent(
PointerEventType.Release,
Offset(30f, 10f),
button = PointerButton.Primary
)
} finally {
scene.close()
}
}

@Test
fun primaryClicks() = testClick(
button = PointerButton.Primary,
Expand Down

0 comments on commit 13a0944

Please sign in to comment.