JetBrains / compose-multiplatform

Compose Multiplatform, a modern UI framework for Kotlin that makes building performant and beautiful user interfaces easy and enjoyable.
https://jetbrains.com/lp/compose-multiplatform
Apache License 2.0
15.73k stars 1.14k forks source link

Clickable - get rid of one frame lag after we release the button #2970

Open igordmn opened 1 year ago

igordmn commented 1 year ago

Compose 3fb4c2f7

After https://android-review.googlesource.com/c/platform/frameworks/support/+/2242251, the ComposeSceneTest.rendering of clickable fails in this line:

scene.sendPointerEvent(PointerEventType.Release, Offset(1f, 1f))
scene.sendPointerEvent(PointerEventType.Move, Offset(-1f, -1f))
scene.sendPointerEvent(PointerEventType.Exit, Offset(-1f, -1f))
awaitNextRender() // we still see the pressed state in this frame. Before the change it was applied immediately.

It is because of this change:

Subject: [PATCH] 1
---
Index: compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt
--- a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt    (revision 3fd58ef9861fa3bd457036896be122939e58175a)
+++ b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt    (date 1680599932775)
@@ -239,7 +239,9 @@
                 }
             } else {
                 up.consume()
-                pressScope.release()
+                launch {
+                    pressScope.release()
+                }
                 onTap?.invoke(up.position)
             }
         }

The frame rendering was scheduled before we pressScope.release(), and because of that we render one frame before pressScope.release().

Not sure, if it is needed to be fixed for real-world cases. maybe in some cases, where application developer tighttly controlled the state of the application (we send an event -> we need to change the screenshot immediately to save it somewhere)

m-sasha commented 1 year ago

I think lag is very important. It makes the experience of using an app less pleasant.

igordmn commented 1 year ago

Usually it is noticible when we continuously change the state (for example, when we drag something). When we change the state once (change indication on Release), it is probably can be noticible too, but less.

igordmn commented 1 year ago

Also, I am not sure, if we have a lag in a real application, when frames render on each vsync, not continuously.

malliaridis commented 1 year ago

@igordmn is this maybe the cause for the application lag I notice every time I hover over multiple buttons?

Simple reproduction steps:

  1. embed material 2 or material 3 buttons (2-3 should be enough)
  2. Hover over the buttons a couple seconds to change the state multiple times
  3. Move the window around

Tested it on Windows and noticed in a few applications already that I assume are written in Compose (JetBrains Toolbox and Fleet as well as my own Compose MPP apps). I was noticing a reduced pointer trail on these windows at first.