desktop: harden hover cursor fix after adversarial review: refresh on release, ignore button-held events, reset icon state on exit, cache canvas lookup failures

2026-07-31 00:50:11 +00:00 · 2026-07-14 17:17:45 +00:00
parent 92fae914bb
commit 2ad80d576d
6 changed files with 99 additions and 37 deletions
@@ -3,6 +3,7 @@ package chat.simplex.common.platform
 import androidx.compose.runtime.Composable
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.graphics.painter.Painter
+import androidx.compose.ui.input.pointer.PointerIcon
 import java.io.File

@Composable
@@ -17,6 +18,6 @@ actual fun Modifier.onRightClick(action: () -> Unit): Modifier = this

 actual fun Modifier.desktopPointerHoverIconHand(): Modifier = this

-actual fun desktopSetHoverCursor(hand: Boolean) {}
+actual fun desktopSetHoverCursor(icon: PointerIcon) {}

 actual fun Modifier.desktopOnHovered(action: (Boolean) -> Unit): Modifier = Modifier
@@ -7,6 +7,7 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.BlurredEdgeTreatment
 import androidx.compose.ui.draw.blur
 import androidx.compose.ui.graphics.painter.Painter
+import androidx.compose.ui.input.pointer.PointerIcon
 import androidx.compose.ui.unit.dp
 import chat.simplex.common.model.ChatController.appPrefs
 import chat.simplex.common.views.helpers.KeyChangeEffect
@@ -27,13 +28,15 @@ expect fun Modifier.onRightClick(action: () -> Unit): Modifier
 expect fun Modifier.desktopPointerHoverIconHand(): Modifier

 /**
- * Directly sets the mouse cursor (hand or text) on desktop, bypassing Compose's pointerHoverIcon.
- * pointerHoverIcon is edge-triggered (it displays the icon only on a per-node Enter event, or on an
- * icon change while the node is marked in-bounds) and those edges can be silently lost when chat
- * items shift/recompose under the cursor, leaving a stale cursor. Calling this on every hover move
- * keeps the cursor self-correcting. No-op on Android.
+ * Directly sets the mouse cursor on desktop ([PointerIcon.Hand], [PointerIcon.Text], anything else
+ * means default). Compose's pointerHoverIcon displays icons only on Enter/Exit edges or on icon
+ * change while marked in-bounds — edges that are silently lost when chat items shift/recompose
+ * under the cursor, leaving a stale cursor. Calling this on every hover move (and resetting on
+ * exit) keeps it self-correcting. No-op on Android.
+ * Verified against Compose 1.8.2, re-verify on upgrade (JetBrains/compose-multiplatform #2091,
+ * #1314, #3750). Details: plans/2026-07-13-fix-command-hover-cursor.md
 */
-expect fun desktopSetHoverCursor(hand: Boolean)
+expect fun desktopSetHoverCursor(icon: PointerIcon)

 expect fun Modifier.desktopOnHovered(action: (Boolean) -> Unit): Modifier

@@ -357,9 +357,15 @@ fun MarkdownText (
          onHover = { offset ->
            val hasAnnotation: (String) -> Boolean = { tag -> annotatedText.hasStringAnnotations(tag, start = offset, end = offset) }
            val hand = hasAnnotation("WEB_URL") || hasAnnotation("SIMPLEX_URL") || hasAnnotation("OTHER_URL") || hasAnnotation("SIMPLEX_NAME") || hasAnnotation("SECRET") || hasAnnotation("COMMAND")
-            icon.value = if (hand) PointerIcon.Hand else PointerIcon.Text
+            val newIcon = if (hand) PointerIcon.Hand else PointerIcon.Text
+            icon.value = newIcon
            // pointerHoverIcon alone loses updates when items shift/recompose under the cursor, see desktopSetHoverCursor
-            desktopSetHoverCursor(hand)
+            desktopSetHoverCursor(newIcon)
+          },
+          onHoverExit = {
+            // also reset icon.value, or the pointerHoverIcon node re-displays a stale Hand on its next Enter
+            icon.value = PointerIcon.Text
+            desktopSetHoverCursor(PointerIcon.Default)
          },
          shouldConsumeEvent = { offset ->
            annotatedText.hasStringAnnotations(tag = "WEB_URL", start = offset, end = offset)
@@ -392,6 +398,7 @@ fun ClickableText(
  onClick: (Int) -> Unit,
  onLongClick: (Int) -> Unit = {},
  onHover: (Int) -> Unit = {},
+  onHoverExit: () -> Unit = {},
  shouldConsumeEvent: (Int) -> Boolean
 ) {
  val layoutResult = remember { mutableStateOf<TextLayoutResult?>(null) }
@@ -415,9 +422,9 @@ fun ClickableText(
      consume
    }
    )
-  }.pointerInput(onHover) {
+  }.pointerInput(onHover, onHoverExit) {
    if (appPlatform.isDesktop) {
-      detectCursorMove { pos ->
+      detectCursorMove(onExit = onHoverExit) { pos ->
        layoutResult.value?.let { layoutResult ->
          onHover(layoutResult.getOffsetForPosition(pos))
        }
@@ -92,15 +92,20 @@ suspend fun PointerInputScope.detectGesture(
  }
 }

-suspend fun PointerInputScope.detectCursorMove(onMove: (Offset) -> Unit = {},) {
-  // One never-exiting scope: exiting/re-entering awaitPointerEventScope per event (as forEachGesture did)
-  // drops events that arrive between scopes, leaving a stale cursor icon.
-  // Enter matters too: when content shifts under a stationary cursor, the newly hovered item gets Enter, not Move.
+suspend fun PointerInputScope.detectCursorMove(onExit: () -> Unit = {}, onMove: (Offset) -> Unit = {},) {
+  // Single scope: re-entering awaitPointerEventScope per event (as forEachGesture did) drops events.
+  // Enter/Release matter too: a stationary cursor gets no Move when content shifts or after a click.
  awaitPointerEventScope {
    while (true) {
      val event = awaitPointerEvent()
-      if (event.type == PointerEventType.Move || event.type == PointerEventType.Enter) {
-        onMove(event.changes[0].position)
+      if (event.type == PointerEventType.Move || event.type == PointerEventType.Enter || event.type == PointerEventType.Release) {
+        val pos = event.changes[0].position
+        // while a button is held the pressed node keeps receiving events even outside its bounds
+        if (event.changes.none { it.pressed } && pos.x >= 0 && pos.y >= 0 && pos.x < size.width && pos.y < size.height) {
+          onMove(pos)
+        }
+      } else if (event.type == PointerEventType.Exit) {
+        onExit()
      }
    }
  }
@@ -13,12 +13,14 @@ import java.awt.Component
 import java.awt.Container
 import java.awt.Cursor
 import java.awt.Image
-import javax.swing.SwingUtilities
+import java.awt.Window
 import java.awt.datatransfer.DataFlavor
 import java.awt.datatransfer.Transferable
 import java.awt.image.BufferedImage
 import java.io.File
+import java.lang.ref.WeakReference
 import java.net.URI
+import javax.swing.SwingUtilities

@Composable
 actual fun Modifier.desktopOnExternalDrag(
@@ -86,24 +88,35 @@ actual fun Modifier.onRightClick(action: () -> Unit): Modifier = contextMenuOpen

 actual fun Modifier.desktopPointerHoverIconHand(): Modifier = Modifier.pointerHoverIcon(PointerIcon.Hand)

-// This runs on every mouse move over clickable text, so the canvas lookup is cached:
-// steady-state cost is a parent-chain check plus an int comparison, no allocations.
-private var cachedSkiaCanvas: Component? = null
+// Runs on every mouse move, so the canvas lookup (and its failure) is cached per window;
+// weak refs avoid retaining a recreated window's scene graph.
+private var cachedSkiaCanvas: WeakReference<Component>? = null
+private var skiaCanvasMissingIn: WeakReference<Window>? = null

-actual fun desktopSetHoverCursor(hand: Boolean) {
+actual fun desktopSetHoverCursor(icon: PointerIcon) {
+  // clickable message text only exists in the main window
  val window = simplexWindowState.window ?: return
-  val type = if (hand) Cursor.HAND_CURSOR else Cursor.TEXT_CURSOR
-  var canvas = cachedSkiaCanvas
-  if (canvas == null || !canvas.isDisplayable || SwingUtilities.getWindowAncestor(canvas) !== window) {
-    canvas = findSkiaCanvas(window) ?: return
-    cachedSkiaCanvas = canvas
+  val type = when (icon) {
+    PointerIcon.Hand -> Cursor.HAND_CURSOR
+    PointerIcon.Text -> Cursor.TEXT_CURSOR
+    else -> Cursor.DEFAULT_CURSOR
+  }
+  var canvas = cachedSkiaCanvas?.get()
+  if (canvas == null || SwingUtilities.getWindowAncestor(canvas) !== window) {
+    if (skiaCanvasMissingIn?.get() === window) return
+    canvas = findSkiaCanvas(window)
+    if (canvas == null) {
+      Log.w(TAG, "desktopSetHoverCursor: Skia canvas not found, hover cursor workaround disabled")
+      skiaCanvasMissingIn = WeakReference(window)
+      return
+    }
+    cachedSkiaCanvas = WeakReference(canvas)
  }
  if (canvas.cursor.type != type) canvas.cursor = Cursor.getPredefinedCursor(type)
 }

-// Compose writes the pointer icon to its skia canvas component (ComposeSceneMediator.setPointerIcon
-// -> contentComponent.cursor, a SkiaLayer/SkiaSwingLayer). Writing to the same component keeps
-// last-write-wins consistent with the framework's own cursor updates (e.g. reset to default on exit).
+// Compose writes pointer icons to its skia canvas component (ComposeSceneMediator.setPointerIcon);
+// writing to the same component keeps last-write-wins consistent with the framework's own updates.
 private fun findSkiaCanvas(c: Component): Component? = when {
  c.javaClass.name.contains("Skia") -> c
  c is Container -> c.components.firstNotNullOfOrNull { findSkiaCanvas(it) }
@@ -30,7 +30,17 @@ silently dropped. It also ignored `Enter` events entirely, so when the chat list
 stationary cursor (every command click appends the sent message), the newly hovered item — which
 receives `Enter`, not `Move` — never updated the icon state.

-**Fix:** one never-exiting `awaitPointerEventScope` loop reacting to both `Move` and `Enter`.
+**Fix:** one never-exiting `awaitPointerEventScope` loop reacting to `Move`, `Enter` and `Release`
+(plus an optional `onExit` callback used by the cursor fix below). `Release` refreshes hover at
+the final pointer position — after clicking a command the list shifts and the release is the only
+event a stationary pointer gets. Button-held events are ignored (gated on *no* pointer pressed,
+matching the old `forEachGesture` semantics which waited for all pointers up between events), and
+so are out-of-bounds positions: while a button is pressed the hit path is locked, so the pressed
+node keeps receiving events after the pointer leaves it — acting on those would apply hover
+effects at clamped positions (e.g. a wrong cursor stuck after drag-releasing outside the text).
+The other `detectCursorMove` call sites (scrollbar reveal in `ScrollableColumn.desktop.kt` and
+`OperatorView.desktop.kt`) were audited: reacting to `Enter`/`Release` there is harmless or an
+improvement, and `onExit` defaults to a no-op for them.

 ### Defect 2: display layer silently swallows updates (commit "set hover cursor directly…")

@@ -60,12 +70,35 @@ Clicking many commands maximizes exposure: each click inserts a message → reco
 list shift + (for `/join`) group-state changes — each transition can hit one of these edges.
 Related upstream issues: JetBrains/compose-multiplatform #2091, #1314, #3750.

-**Fix:** in `onHover` — which now fires reliably on every Move/Enter over the text — set the AWT
-cursor imperatively on desktop (`desktopSetHoverCursor`), in addition to the existing
-`pointerHoverIcon` modifier (kept for the normal reset-to-default on exit). The write targets the
-same Skia canvas component Compose writes to (`ComposeSceneMediator.setPointerIcon` →
-`contentComponent.cursor`), so last-write-wins stays consistent with the framework's own updates.
-No framework edge can leave the cursor stuck, because it is re-asserted on every mouse move.
+**Fix:** in `onHover` — which now fires reliably on every Move/Enter/Release over the text — set
+the AWT cursor imperatively on desktop (`desktopSetHoverCursor`), and symmetrically reset both the
+cursor and the `icon` state on the per-node `Exit` event (delivered reliably to the `pointerInput`
+node even when the hover-icon node is desynced and its own exit reset is a guarded no-op; the state
+reset prevents the hover-icon node from re-displaying a stale Hand on its next Enter). The set and
+reset live together in `MarkdownText` (`onHover`/`onHoverExit`), so other `ClickableText` callers
+are untouched. The existing `pointerHoverIcon` modifier is kept, so the normal framework path still
+works when its edges fire — and it remains the only path covering disposal-without-Exit and Android
+mice. The imperative write targets the same Skia canvas component Compose writes to
+(`ComposeSceneMediator.setPointerIcon` → `contentComponent.cursor`), so last-write-wins stays
+consistent with the framework's own updates; the canvas lookup is cached weakly (including negative
+results, with a one-time warning log if the component is not found after a Compose upgrade — the
+workaround then degrades to the framework-only path). No framework edge can leave the cursor
+stuck, because it is re-asserted on every mouse move and released on exit.
+
+The same lost-edge class also applies in principle to static `pointerHoverIcon` sites whose content
+shifts under a stationary cursor (e.g. the chat list link previews using
+`desktopPointerHoverIconHand`); those are low-exposure and left on the framework path — if a
+second report arrives, extract a shared reliable-hover-icon modifier from this wiring.
+
+### Residual limitation (known, strictly smaller than the fixed bug)
+
+If a hovered clickable text leaves the composition without the pointer ever getting an `Exit`
+event (e.g. the hovered message is deleted, or a fast fling disposes the item within a frame), the
+`onExit` reset does not run — the coroutine is simply cancelled. The retained `pointerHoverIcon`
+modifier's `onDetach` reset covers this when its node is in the normal (non-desynced) state; in the
+rare desynced state the cursor may stay a hand until the next hover write. This is the same event
+class the fix reduces, bounded to one stale frame region, and self-corrects on any subsequent
+hover.

 ## Performance