diff --git a/FINDINGS.md b/FINDINGS.md index b87ae8a..1597cf6 100644 --- a/FINDINGS.md +++ b/FINDINGS.md @@ -264,34 +264,50 @@ A second bug in the same few lines: `Modifier.safeDrawingPadding()` in the activity **already includes the IME inset**, and the screen then applied `imePadding()` on top, so the keyboard inset was counted twice. -## An unconsumed press means waitForUpOrCancellation never returns +## Pointer coordinates cannot locate text in a scrolling BasicTextField -Tap to toggle did nothing at all, while a diagnostic log showed the gesture -handler firing on every tap and resolving the correct offset. Tapping the line -`- [ ] Take some time to build out...` produced offsets 289 through 292, and -that line's bracket span is exactly 289..292, so detection was never the -problem. +PLAN.md section 7 specifies tap to toggle as `onTextLayout` plus `pointerInput` +plus `getOffsetForPosition`. That does not work, and the reason is a coordinate +mismatch: `getOffsetForPosition` expects **layout** coordinates while +`pointerInput` reports **viewport** coordinates. `BasicTextField` scrolls its +content internally and does not expose the offset, so the two agree only while +the document has not been scrolled. -The problem was two lines further on. `BasicTextField` handles the tap itself -and consumes the pointer, and `waitForUpOrCancellation` returns **null** for a -pointer somebody else has consumed. The press arrived, the release never did, -and nothing ever toggled. +Measured, on a 1561 character note. Every tap resolved to an offset of 253 or +less, wherever on the screen it landed: -This was self inflicted. The press was deliberately left unconsumed so that -ordinary cursor placement and scrolling would keep working, and that choice -handed the gesture to the text field. The fix is to claim the press in -`PointerEventPass.Initial`, which runs outer first, but only once -`checkboxSpanAt` has confirmed the press actually landed on a box. Every other -tap stays unclaimed and behaves exactly as before. +``` +down offset=232 span=CheckboxSpan(start=230, end=233, checked=true) works, near the top +down offset=249 span=null tapped well below the fold +down offset=114 span=null tapped well below the fold +``` -The cost is that a scroll begun with a finger already on a `[ ]` will not -scroll. The target is three characters wide, so this is a fair trade. +The fix is to stop using pointer coordinates. The text field resolves taps +correctly because it is the thing that knows its own scroll, so it places the +cursor and that offset is read from `onValueChange` instead. The press is +watched only to tell a cursor move caused by a tap from one caused by anything +else, and is never consumed, so scrolling and ordinary cursor placement are +untouched. -Worth noting what was *not* wrong. `getOffsetForPosition` takes layout -coordinates while `pointerInput` reports viewport coordinates, so a scrolled -field looked like it should mis-resolve taps. The logged offsets show it does -not. Instrumenting first turned a plausible theory into a measured fact and -saved rewriting a correct piece of code. +One consequence worth writing down. After a toggle the cursor is parked past the +box rather than left inside it. Left inside, a second tap on the same box +resolves to the offset the cursor already holds, and a selection that does not +change reports nothing, so the box could not be unticked by tapping it twice. + +Two earlier attempts failed for a different reason each, both about consumption: + +- Leaving the press unconsumed let `BasicTextField` consume it, and + `waitForUpOrCancellation` returns null for a consumed pointer, so the release + never arrived. +- Consuming the press in `PointerEventPass.Initial` and then waiting in the + default Main pass fails too, because the same event reaches Main already + marked consumed, which again reads as a cancellation. + +A process note that cost a round trip. After the first diagnostic showed correct +offsets, this coordinate problem was written off as disproved. It was not: every +tap in that sample happened to be in the unscrolled top of the document, where +the two coordinate systems coincide. The evidence was consistent with both +explanations, and only looked decisive. ## Reconciliation, phase 0, completed 2026-09-09 diff --git a/android/app/src/main/java/com/example/todo/ui/note/NoteScreen.kt b/android/app/src/main/java/com/example/todo/ui/note/NoteScreen.kt index 92f0d21..10d986b 100644 --- a/android/app/src/main/java/com/example/todo/ui/note/NoteScreen.kt +++ b/android/app/src/main/java/com/example/todo/ui/note/NoteScreen.kt @@ -1,8 +1,8 @@ package com.example.todo.ui.note +import android.os.SystemClock import androidx.compose.foundation.gestures.awaitEachGesture import androidx.compose.foundation.gestures.awaitFirstDown -import androidx.compose.foundation.gestures.waitForUpOrCancellation import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row @@ -25,7 +25,6 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.SolidColor import androidx.compose.ui.input.pointer.PointerEventPass import androidx.compose.ui.input.pointer.pointerInput -import androidx.compose.ui.text.TextLayoutResult import androidx.compose.ui.text.TextRange import androidx.compose.ui.text.TextStyle import androidx.compose.ui.text.input.TextFieldValue @@ -34,9 +33,12 @@ import androidx.compose.ui.unit.sp import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.viewmodel.compose.viewModel import com.example.todo.editor.MarkdownHighlighter -import com.example.todo.theme.JetBrainsMono import com.example.todo.editor.checkboxSpanAt import com.example.todo.editor.toggleCheckbox +import com.example.todo.theme.JetBrainsMono + +/** A cursor move this long after a press is taken to be that press landing. */ +private const val TAP_WINDOW_MS = 700L @Composable fun NoteScreen(viewModel: NoteViewModel = viewModel(), modifier: Modifier = Modifier) { @@ -57,8 +59,8 @@ fun NoteScreen(viewModel: NoteViewModel = viewModel(), modifier: Modifier = Modi } // The editor owns cursor and selection; the view model owns the text that - // gets synced. They are only forced back together when the text changes - // from underneath, which means a fetch replaced it. + // gets synced. They are forced back together only when the text changes + // underneath, which means a fetch replaced it. val field = remember { mutableStateOf(TextFieldValue(state.text)) } LaunchedEffect(state.text) { if (state.text != field.value.text) { @@ -66,9 +68,28 @@ fun NoteScreen(viewModel: NoteViewModel = viewModel(), modifier: Modifier = Modi field.value = TextFieldValue(state.text, TextRange(where)) } } - val layout = remember { mutableStateOf(null) } + val pressedAt = remember { mutableStateOf(0L) } - fun update(new: TextFieldValue) { + fun onChange(new: TextFieldValue) { + val old = field.value + val movedCursor = new.text == old.text && + new.selection.collapsed && + new.selection != old.selection + val fromTap = SystemClock.uptimeMillis() - pressedAt.value < TAP_WINDOW_MS + val span = if (movedCursor && fromTap) { + checkboxSpanAt(new.text, new.selection.start) + } else { + null + } + if (span != null) { + val toggled = toggleCheckbox(new.text, span) + // Park the cursor past the box. Left inside, a second tap on the + // same box would land on the offset it already holds, and a + // selection that does not change reports nothing to toggle. + field.value = TextFieldValue(toggled, TextRange(span.end)) + viewModel.onTextChange(toggled) + return + } field.value = new viewModel.onTextChange(new.text) } @@ -91,34 +112,24 @@ fun NoteScreen(viewModel: NoteViewModel = viewModel(), modifier: Modifier = Modi BasicTextField( value = field.value, - onValueChange = ::update, - onTextLayout = { layout.value = it }, + onValueChange = ::onChange, visualTransformation = highlighter, modifier = Modifier .fillMaxSize() + // Watches for a press without ever consuming it. Pointer + // coordinates are useless here: they are relative to the + // viewport while getOffsetForPosition expects layout + // coordinates, and the field scrolls internally without + // exposing by how much, so a tap below the fold resolves to + // somewhere near the top of the document. The field itself + // knows its own scroll, so the offset it puts the cursor at is + // the one to trust. This only records that a press happened, + // so that an arriving cursor move can be told from one caused + // by anything else. .pointerInput(Unit) { awaitEachGesture { - // Claimed in the Initial pass, which runs outer first, - // and only for a press that actually lands on a box. - // Leaving it unclaimed lets the text field consume it, - // and a consumed pointer makes waitForUpOrCancellation - // return null, so the release never arrives here. - val down = awaitFirstDown( - requireUnconsumed = false, - pass = PointerEventPass.Initial, - ) - val at = layout.value ?: return@awaitEachGesture - val text = field.value.text - val span = checkboxSpanAt(text, at.getOffsetForPosition(down.position)) - ?: return@awaitEachGesture - - down.consume() - val up = waitForUpOrCancellation() ?: return@awaitEachGesture - up.consume() - // Same box on release, so a tap rather than a drag. - if (checkboxSpanAt(text, at.getOffsetForPosition(up.position)) == span) { - update(field.value.copy(text = toggleCheckbox(text, span))) - } + awaitFirstDown(requireUnconsumed = false, pass = PointerEventPass.Initial) + pressedAt.value = SystemClock.uptimeMillis() } }, textStyle = TextStyle(