Suspending gesture detecting functions should be combinable [181577176]

WAI

Bug

Status Update

No update yet.

Description

da...@danielzfranklin.org

created issue #1

Mar 1, 2021 08:15PM

I had code that required detecting a number of different gestures that were possible only in certain states. I started off using the gesture detecting functions and global variables like so:

var isInTurn = false
var isInSelect = false

detectDragGesturesAfterLongPress { change, _ ->
    if (!isInTurn) {
        isInSelect = true
        /* ... */
    }
}

detectDragGestures { change, _ ->
    if (!isInSelect) {
        isInTurn = true
        /* ... */
    }
}
// ...

I eventually traced a number of bugs down to updating the state non-atomically, so I was planning to add a mutex. That reminded me of how coroutines can often be used to solve concurrency issues, and I came up with

while (true) {
    val event = awaitPointerEventScope { awaitFirstDown() }

    when {
        isStartOfTurn(event) ->
            awaitTurnOrCancel(event)

        isStartOfSelect(event) ->
            awaitSelectOrCancel(event)

        // ...
    }
}

I defined a bunch of my own functions on PointerInputScope, and the approach solved my concurrency issues in a relatively clean way.

The problem is that most of the framework detectors block forever, so they can't be combined. I ended up copying a framework function, editing it to remove the line forEachGesture {, and copying over all their private/internal dependencies.

I'm suggesting that framework gesture detector functions should (when it makes sense) delegate to another public function that detects only one gesture.

fun detectDragGesturesAfterLongPress(/* ... */) {
   forEachGesture {
      detectOneDragGestureAfterLongPress(/* ... */)
    }
}

fun detectOneDragGestureAfterLongPress(/* ... */)

I'm also suggesting that more of the private helpers you use in gesture detection be made public, such as awaitLongPressOrCancellation, to enable users to write code that combines small suspending functions to detect complex gestures as cleanly as the framework.

Comments

ma...@google.com <ma...@google.com> #2Mar 2, 2021 02:33PM

Status: Won't Fix (Intended Behavior)

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

da...@danielzfranklin.org <da...@danielzfranklin.org> #3Mar 2, 2021 04:07PM

Thank you for going into depth on your thought processes. I think awaitLongPressOrCancellation would be a useful building block to open up.

Copying awaitLongPressOrCancellation and it's private/internal dependencies to my project led to me copying about 40 lines of code (excluding comments/blank). (In comparison, if forEachGesture was private, I would have had to copy about 20 lines to use it.)

If I hadn't seen awaitLongPressOrCancellation's implementation while reading the source code of another function I suspect I would have incorrectly assumed I could solve the problem with a simpler approach, and had to figure out some bugs (such as around pointer events out of bounds).

Here's what I copied into my project:


// Copied from Compose
private suspend fun PointerInputScope.awaitLongPressOrCancellation(
    initialDown: PointerInputChange
): PointerInputChange? {
    var longPress: PointerInputChange? = null
    var currentDown = initialDown
    val longPressTimeout = viewConfiguration.longPressTimeoutMillis
    return try {
        // wait for first tap up or long press
        withTimeout(longPressTimeout) {
            awaitPointerEventScope {
                var finished = false
                while (!finished) {
                    val event = awaitPointerEvent(PointerEventPass.Main)
                    if (event.changes.fastAll { it.changedToUpIgnoreConsumed() }) {
                        // All pointers are up
                        finished = true
                    }

                    if (
                        event.changes.fastAny { it.consumed.downChange || it.isOutOfBounds(size) }
                    ) {
                        finished = true // Canceled
                    }

                    // Check for cancel by position consumption. We can look on the Final pass of
                    // the existing pointer event because it comes after the Main pass we checked
                    // above.
                    val consumeCheck = awaitPointerEvent(PointerEventPass.Final)
                    if (consumeCheck.changes.fastAny { it.positionChangeConsumed() }) {
                        finished = true
                    }
                    if (!event.isPointerUp(currentDown.id)) {
                        longPress = event.changes.firstOrNull { it.id == currentDown.id }
                    } else {
                        val newPressed = event.changes.fastFirstOrNull { it.pressed }
                        if (newPressed != null) {
                            currentDown = newPressed
                            longPress = currentDown
                        } else {
                            // should technically never happen as we checked it above
                            finished = true
                        }
                    }
                }
            }
        }
        null
    } catch (_: TimeoutCancellationException) {
        longPress ?: initialDown
    }
}

// Copied from Compose
private fun PointerEvent.isPointerUp(pointerId: PointerId): Boolean =
    changes.firstOrNull { it.id == pointerId }?.pressed != true

ma...@google.com <ma...@google.com> #4Mar 3, 2021 04:07PM

Filed b/181764910 to track the awaitLongPressOrCancellation separately, thanks!

ap...@google.com <ap...@google.com> #5Aug 12, 2022 06:10PM

Project: platform/frameworks/support
Branch: androidx-main

commit 4192f52e326cf6e81e6e30202ff00496743b6af1
Author: Jeremy Walker <jewalker@google.com>
Date: Wed Aug 10 15:18:18 2022

Exposing long press await fun as building block for gesture detection.

Bug: 181577176
Test: Manually tested.
Relnote: "Exposed `AwaitPointerEventScope#awaitLongPressOrCancellation` as additional building block for more complex gesture detection"
Change-Id: I04374246211122f86e1235a82a98fa4484381e69

M compose/foundation/foundation/api/public_plus_experimental_current.txt
M compose/foundation/foundation/api/current.txt
M compose/foundation/foundation/api/restricted_current.txt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/DragGestureDetector.kt

https://android-review.googlesource.com/2182320

je...@google.com <je...@google.com> #6Aug 12, 2022 07:45PM

awaitLongPressOrCancellation() is now public per issuetracker/181764910.

Issue 181577176