Toggle from the cursor offset, not from pointer coordinates
PLAN.md section 7 specifies getOffsetForPosition on a pointer position. That cannot work: it expects layout coordinates while pointerInput reports viewport coordinates, and BasicTextField scrolls internally without exposing the offset. On a 1561 character note every tap resolved to an offset of 253 or less, so it worked at the top and nowhere else. The field is the thing that knows its own scroll, so it now places the cursor and that offset is read back from onValueChange. The press is watched only to distinguish a tap from any other cursor move, and is never consumed, so scrolling and ordinary cursor placement are untouched. That also retires the earlier trade off where a scroll starting on a box would not scroll. After toggling, the cursor is parked past the box. Left inside, a second tap resolves to the offset it already holds, and an unchanged selection reports nothing, so a box could not be unticked by tapping it twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PU5ZFfQFtDTFqqhvMTWdGH
This commit is contained in:
1 parent
bb66567725
commit
0812b7b2e7
2 files changed
+80
-53
No files matched your search
+39
-23
@@ -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
|
activity **already includes the IME inset**, and the screen then applied
|
||||||
`imePadding()` on top, so the keyboard inset was counted twice.
|
`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
|
PLAN.md section 7 specifies tap to toggle as `onTextLayout` plus `pointerInput`
|
||||||
handler firing on every tap and resolving the correct offset. Tapping the line
|
plus `getOffsetForPosition`. That does not work, and the reason is a coordinate
|
||||||
`- [ ] Take some time to build out...` produced offsets 289 through 292, and
|
mismatch: `getOffsetForPosition` expects **layout** coordinates while
|
||||||
that line's bracket span is exactly 289..292, so detection was never the
|
`pointerInput` reports **viewport** coordinates. `BasicTextField` scrolls its
|
||||||
problem.
|
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
|
Measured, on a 1561 character note. Every tap resolved to an offset of 253 or
|
||||||
and consumes the pointer, and `waitForUpOrCancellation` returns **null** for a
|
less, wherever on the screen it landed:
|
||||||
pointer somebody else has consumed. The press arrived, the release never did,
|
|
||||||
and nothing ever toggled.
|
|
||||||
|
|
||||||
This was self inflicted. The press was deliberately left unconsumed so that
|
```
|
||||||
ordinary cursor placement and scrolling would keep working, and that choice
|
down offset=232 span=CheckboxSpan(start=230, end=233, checked=true) works, near the top
|
||||||
handed the gesture to the text field. The fix is to claim the press in
|
down offset=249 span=null tapped well below the fold
|
||||||
`PointerEventPass.Initial`, which runs outer first, but only once
|
down offset=114 span=null tapped well below the fold
|
||||||
`checkboxSpanAt` has confirmed the press actually landed on a box. Every other
|
```
|
||||||
tap stays unclaimed and behaves exactly as before.
|
|
||||||
|
|
||||||
The cost is that a scroll begun with a finger already on a `[ ]` will not
|
The fix is to stop using pointer coordinates. The text field resolves taps
|
||||||
scroll. The target is three characters wide, so this is a fair trade.
|
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
|
One consequence worth writing down. After a toggle the cursor is parked past the
|
||||||
coordinates while `pointerInput` reports viewport coordinates, so a scrolled
|
box rather than left inside it. Left inside, a second tap on the same box
|
||||||
field looked like it should mis-resolve taps. The logged offsets show it does
|
resolves to the offset the cursor already holds, and a selection that does not
|
||||||
not. Instrumenting first turned a plausible theory into a measured fact and
|
change reports nothing, so the box could not be unticked by tapping it twice.
|
||||||
saved rewriting a correct piece of code.
|
|
||||||
|
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
|
## Reconciliation, phase 0, completed 2026-09-09
|
||||||
|
|
||||||
|
|||||||
@@ -1,8 +1,8 @@
|
|||||||
package com.example.todo.ui.note
|
package com.example.todo.ui.note
|
||||||
|
|
||||||
|
import android.os.SystemClock
|
||||||
import androidx.compose.foundation.gestures.awaitEachGesture
|
import androidx.compose.foundation.gestures.awaitEachGesture
|
||||||
import androidx.compose.foundation.gestures.awaitFirstDown
|
import androidx.compose.foundation.gestures.awaitFirstDown
|
||||||
import androidx.compose.foundation.gestures.waitForUpOrCancellation
|
|
||||||
import androidx.compose.foundation.layout.Arrangement
|
import androidx.compose.foundation.layout.Arrangement
|
||||||
import androidx.compose.foundation.layout.Column
|
import androidx.compose.foundation.layout.Column
|
||||||
import androidx.compose.foundation.layout.Row
|
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.graphics.SolidColor
|
||||||
import androidx.compose.ui.input.pointer.PointerEventPass
|
import androidx.compose.ui.input.pointer.PointerEventPass
|
||||||
import androidx.compose.ui.input.pointer.pointerInput
|
import androidx.compose.ui.input.pointer.pointerInput
|
||||||
import androidx.compose.ui.text.TextLayoutResult
|
|
||||||
import androidx.compose.ui.text.TextRange
|
import androidx.compose.ui.text.TextRange
|
||||||
import androidx.compose.ui.text.TextStyle
|
import androidx.compose.ui.text.TextStyle
|
||||||
import androidx.compose.ui.text.input.TextFieldValue
|
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.compose.collectAsStateWithLifecycle
|
||||||
import androidx.lifecycle.viewmodel.compose.viewModel
|
import androidx.lifecycle.viewmodel.compose.viewModel
|
||||||
import com.example.todo.editor.MarkdownHighlighter
|
import com.example.todo.editor.MarkdownHighlighter
|
||||||
import com.example.todo.theme.JetBrainsMono
|
|
||||||
import com.example.todo.editor.checkboxSpanAt
|
import com.example.todo.editor.checkboxSpanAt
|
||||||
import com.example.todo.editor.toggleCheckbox
|
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
|
@Composable
|
||||||
fun NoteScreen(viewModel: NoteViewModel = viewModel(), modifier: Modifier = Modifier) {
|
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
|
// 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
|
// gets synced. They are forced back together only when the text changes
|
||||||
// from underneath, which means a fetch replaced it.
|
// underneath, which means a fetch replaced it.
|
||||||
val field = remember { mutableStateOf(TextFieldValue(state.text)) }
|
val field = remember { mutableStateOf(TextFieldValue(state.text)) }
|
||||||
LaunchedEffect(state.text) {
|
LaunchedEffect(state.text) {
|
||||||
if (state.text != field.value.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))
|
field.value = TextFieldValue(state.text, TextRange(where))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
val layout = remember { mutableStateOf<TextLayoutResult?>(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
|
field.value = new
|
||||||
viewModel.onTextChange(new.text)
|
viewModel.onTextChange(new.text)
|
||||||
}
|
}
|
||||||
@@ -91,34 +112,24 @@ fun NoteScreen(viewModel: NoteViewModel = viewModel(), modifier: Modifier = Modi
|
|||||||
|
|
||||||
BasicTextField(
|
BasicTextField(
|
||||||
value = field.value,
|
value = field.value,
|
||||||
onValueChange = ::update,
|
onValueChange = ::onChange,
|
||||||
onTextLayout = { layout.value = it },
|
|
||||||
visualTransformation = highlighter,
|
visualTransformation = highlighter,
|
||||||
modifier = Modifier
|
modifier = Modifier
|
||||||
.fillMaxSize()
|
.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) {
|
.pointerInput(Unit) {
|
||||||
awaitEachGesture {
|
awaitEachGesture {
|
||||||
// Claimed in the Initial pass, which runs outer first,
|
awaitFirstDown(requireUnconsumed = false, pass = PointerEventPass.Initial)
|
||||||
// and only for a press that actually lands on a box.
|
pressedAt.value = SystemClock.uptimeMillis()
|
||||||
// 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)))
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
textStyle = TextStyle(
|
textStyle = TextStyle(
|
||||||
|
|||||||
Reference in new issue
Block a user