editor: fix tap-to-position-cursor clamping on large files
Tapping in the editor repositions the cursor, but it was never actually verified (the old unit test only checked in-bounds/no-panic). On-device verification found that on a large file scrolled deep, the cursor clamped to the bottom of the viewport regardless of tap position. Root cause: the tap handler computed the tap's text-local Y in content space (pt.Y - region.Y + full ScrollOffset) and passed it to SetCursorFromPoint, whose visualLine = y/lineHeight then produced a huge content-line number (e.g. 91,640). But the GlyphLayout is window-relative (layout.Y==0 is the top of the visible window), so the number far exceeded the window's line count and clamped to the last group (bottom line). The Phase 3 windowing refactor introduced the windowed layout without updating the tap handler. Fix: tapLocalY() converts the tap Y to window-relative space by adding only the sub-line remainder (ScrollOffset mod lineHeight), never the full scroll. Extracted as a named helper so it is unit-testable. Added two regression tests that fail on the pre-fix formula (cursor clamps to the bottom line) and pass on the fix. Verified on-device: taps now map linearly across the viewport. Also removed the per-tap/per-glyph log.Printf debug lines in SetCursorFromPoint.
This commit is contained in:
parent
be48ad8157
commit
6dedffcba7
|
|
@ -1,7 +1,8 @@
|
||||||
# Development Plan: reach a lean, usable Android text editor
|
# Development Plan: reach a lean, usable Android text editor
|
||||||
|
|
||||||
Status: v5, 2026-08-16 (Phases 0–3 + doc reorg + Phase 6 scroll-perf/clamping
|
Status: v6, 2026-08-16 (Phases 0–3 + doc reorg + Phase 6 scroll-perf/clamping
|
||||||
verification complete). Written against the **live** repo `/home/gmp/pad`. v1 (the widget-rebuild plan) is
|
verification + tap-to-position-cursor fix). Written against the **live** repo
|
||||||
|
`/home/gmp/pad`. v1 (the widget-rebuild plan) is
|
||||||
superseded — see §12 for why. Doc reorganization (2026-08-16): the over-detailed
|
superseded — see §12 for why. Doc reorganization (2026-08-16): the over-detailed
|
||||||
docs (`*_implementation_plan.md`, `touch.md`, `element_model.md`,
|
docs (`*_implementation_plan.md`, `touch.md`, `element_model.md`,
|
||||||
`layout_rendering.md`, `virtual_scroll_render_optimization.md`,
|
`layout_rendering.md`, `virtual_scroll_render_optimization.md`,
|
||||||
|
|
@ -14,8 +15,13 @@ whole-file shaper memory-leak fix, a measured 50 MB size limit, and the IME
|
||||||
rapid-commit desync fix (snippet/selection dedup). Phase 6 (2026-08-16) added a
|
rapid-commit desync fix (snippet/selection dedup). Phase 6 (2026-08-16) added a
|
||||||
default-off in-app performance profiler and verified: scroll does not degrade at
|
default-off in-app performance profiler and verified: scroll does not degrade at
|
||||||
large offsets (10 MB file), scroll clamping is exact across 2→130,955-line
|
large offsets (10 MB file), scroll clamping is exact across 2→130,955-line
|
||||||
files, and memory plateaus ~250 MB (no leak). Remaining: real-device
|
files, and memory plateaus ~250 MB (no leak). Phase 7 (2026-08-16) verified
|
||||||
swipe/autocorrect sign-off (the emulator's AOSP/Gboard keyboard is a proxy).
|
tap-to-position-cursor on-device and found + fixed a real bug: for large files
|
||||||
|
scrolled deep, `SetCursorFromPoint` clamped the cursor to the bottom of the
|
||||||
|
viewport (a content-space vs window-relative Y mismatch from the Phase 3 windowing
|
||||||
|
refactor); it only worked on small files by luck. Fixed (`tapLocalY`), with
|
||||||
|
regression tests. Remaining: real-device swipe/autocorrect sign-off (the
|
||||||
|
emulator's AOSP/Gboard keyboard is a proxy).
|
||||||
|
|
||||||
## 1. Decision summary (updated)
|
## 1. Decision summary (updated)
|
||||||
|
|
||||||
|
|
@ -280,6 +286,38 @@ at ~`(86, 264)` px, not the glyph's apparent 1:1 position. Browser file rows
|
||||||
logcat line `Logic: OpenFileChan /storage/emulated/0/Notes/<file>`, not a loose
|
logcat line `Logic: OpenFileChan /storage/emulated/0/Notes/<file>`, not a loose
|
||||||
`OpenFileChan` match.
|
`OpenFileChan` match.
|
||||||
|
|
||||||
|
### Phase 7 — tap-to-position-cursor verification + fix — DONE (2026-08-16)
|
||||||
|
|
||||||
|
Asked "does tapping in the editor reposition the cursor, and has it been
|
||||||
|
verified?" — it had **not** been verified (the old unit test only checked
|
||||||
|
in-bounds/no-panic, never the landed offset). On-device verification found a
|
||||||
|
real bug:
|
||||||
|
|
||||||
|
- **Symptom:** on a large file scrolled deep (e.g. `big10mb.txt` at 70% down),
|
||||||
|
tapping anywhere in the viewport placed the cursor on the **bottom** line of
|
||||||
|
the viewport, regardless of where you tapped. It worked on small files only
|
||||||
|
because their visible window happened to be wide enough to keep the line number
|
||||||
|
in range.
|
||||||
|
- **Root cause:** the tap handler computed the tap's text-local Y in
|
||||||
|
**content space** (`pt.Y - region.Y + full ScrollOffset`) and passed it to
|
||||||
|
`SetCursorFromPoint`, whose `visualLine = y/lineHeight` then produced a huge
|
||||||
|
content-line number (e.g. 91,640). But the `GlyphLayout` is **window-relative**
|
||||||
|
(`layout.Y==0` is the top of the *visible window*, not the file), so the line
|
||||||
|
number far exceeded the window's ~86 lines and clamped to the last group (the
|
||||||
|
bottom line). The Phase 3 windowing refactor introduced the windowed layout
|
||||||
|
but the tap handler was never updated to match it.
|
||||||
|
- **Fix:** `tapLocalY()` converts the tap Y to window-relative space by adding
|
||||||
|
only the sub-line remainder of the scroll (`ScrollOffset mod lineHeight`),
|
||||||
|
never the full scroll. Extracted as a named helper so it is unit-testable.
|
||||||
|
- **Verification:** on-device, taps now map linearly across the viewport (top tap
|
||||||
|
→ window line 3; y=430/750/1000/1200 → lines 3/11/16/21, cursor 132/464/677/924),
|
||||||
|
and a screenshot confirms the cursor bar lands on the tapped line, not the
|
||||||
|
bottom. Two regression tests added (`TestTapLocalY_WindowRelative`,
|
||||||
|
`TestTapToPosition_LargeFileScrolled`) — both fail on the pre-fix formula
|
||||||
|
(cursor clamps to byte 78 = bottom line) and pass on the fix.
|
||||||
|
- Also removed the per-tap `log.Printf` debug lines in `SetCursorFromPoint`
|
||||||
|
(per-tap, per-glyph logcat noise).
|
||||||
|
|
||||||
## 6. File-size decision (re-framed)
|
## 6. File-size decision (re-framed)
|
||||||
|
|
||||||
v1 framed this as "accept a limit vs build a windowed editor." The live repo
|
v1 framed this as "accept a limit vs build a windowed editor." The live repo
|
||||||
|
|
|
||||||
|
|
@ -8,8 +8,8 @@ import (
|
||||||
|
|
||||||
// TestCursorPositioning_Basic verifies cursor positioning via GlyphLayout.
|
// TestCursorPositioning_Basic verifies cursor positioning via GlyphLayout.
|
||||||
func TestCursorPositioning_Basic(t *testing.T) {
|
func TestCursorPositioning_Basic(t *testing.T) {
|
||||||
TheState = NewState()
|
TheState = NewState()
|
||||||
TheState.Editor.Buffer = "hello\nworld"
|
TheState.Editor.Buffer = "hello\nworld"
|
||||||
|
|
||||||
// Two lines: "hello\n" (bytes 0-5) and "world" (bytes 6-10)
|
// Two lines: "hello\n" (bytes 0-5) and "world" (bytes 6-10)
|
||||||
// Line 0 glyphs at Y=70, Line 1 glyphs at Y=140
|
// Line 0 glyphs at Y=70, Line 1 glyphs at Y=140
|
||||||
|
|
@ -35,9 +35,9 @@ TheState.Editor.Buffer = "hello\nworld"
|
||||||
|
|
||||||
// TestCursorPositioning_EmptyDocument verifies cursor positioning handles empty documents.
|
// TestCursorPositioning_EmptyDocument verifies cursor positioning handles empty documents.
|
||||||
func TestCursorPositioning_EmptyDocument(t *testing.T) {
|
func TestCursorPositioning_EmptyDocument(t *testing.T) {
|
||||||
TheState = NewState()
|
TheState = NewState()
|
||||||
TheState.Editor.Buffer = ""
|
TheState.Editor.Buffer = ""
|
||||||
TheState.Editor.GlyphLayout = ui.GlyphLayout{}
|
TheState.Editor.GlyphLayout = ui.GlyphLayout{}
|
||||||
|
|
||||||
// Should not panic on empty layout
|
// Should not panic on empty layout
|
||||||
SetCursorFromPoint(50, 50)
|
SetCursorFromPoint(50, 50)
|
||||||
|
|
@ -49,8 +49,8 @@ TheState.Editor.GlyphLayout = ui.GlyphLayout{}
|
||||||
|
|
||||||
// TestCursorPositioning_IncompleteLayout verifies cursor positioning handles missing layout slices.
|
// TestCursorPositioning_IncompleteLayout verifies cursor positioning handles missing layout slices.
|
||||||
func TestCursorPositioning_IncompleteLayout(t *testing.T) {
|
func TestCursorPositioning_IncompleteLayout(t *testing.T) {
|
||||||
TheState = NewState()
|
TheState = NewState()
|
||||||
TheState.Editor.Buffer = "test"
|
TheState.Editor.Buffer = "test"
|
||||||
// GlyphLayout with ByteOffsets and X but no Advance (partial data)
|
// GlyphLayout with ByteOffsets and X but no Advance (partial data)
|
||||||
TheState.Editor.GlyphLayout = ui.GlyphLayout{
|
TheState.Editor.GlyphLayout = ui.GlyphLayout{
|
||||||
ByteOffsets: []int{0, 1, 2, 3},
|
ByteOffsets: []int{0, 1, 2, 3},
|
||||||
|
|
@ -65,9 +65,9 @@ TheState.Editor.Buffer = "test"
|
||||||
|
|
||||||
// TestHandleCursorMove_Bounds verifies cursor movement stays within buffer bounds.
|
// TestHandleCursorMove_Bounds verifies cursor movement stays within buffer bounds.
|
||||||
func TestHandleCursorMove_Bounds(t *testing.T) {
|
func TestHandleCursorMove_Bounds(t *testing.T) {
|
||||||
TheState = NewState()
|
TheState = NewState()
|
||||||
TheState.Editor.Buffer = "hello"
|
TheState.Editor.Buffer = "hello"
|
||||||
TheState.Editor.CursorPosition = 0
|
TheState.Editor.CursorPosition = 0
|
||||||
|
|
||||||
// Move right 3 times
|
// Move right 3 times
|
||||||
HandleCursorMove(1)
|
HandleCursorMove(1)
|
||||||
|
|
@ -89,3 +89,57 @@ TheState.Editor.CursorPosition = 0
|
||||||
t.Errorf("expected cursor at 0, got %d", TheState.Editor.CursorPosition)
|
t.Errorf("expected cursor at 0, got %d", TheState.Editor.CursorPosition)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestTapLocalY_WindowRelative is a regression test for the tap-to-position-cursor
|
||||||
|
// coordinate bug: the tap's text-local Y must be WINDOW-relative (layout.Y==0 is
|
||||||
|
// the top of the visible window), so it must NOT include the full scroll offset.
|
||||||
|
// The old code added the full ScrollOffset, which made visualLine a huge
|
||||||
|
// content-line number that clamped the cursor to the bottom of the viewport on
|
||||||
|
// any large file.
|
||||||
|
func TestTapLocalY_WindowRelative(t *testing.T) {
|
||||||
|
lh := float64(EditorLineHeight())
|
||||||
|
// Tap 100 Dp below a region top that starts at 114 Dp, with a deep scroll
|
||||||
|
// (a large file scrolled far down).
|
||||||
|
got := tapLocalY(214, 114, 1539464)
|
||||||
|
// Expected: (214-114) + (1539464 mod lineHeight) => in [100, 100+lineHeight).
|
||||||
|
if got < 100 || got >= 100+lh {
|
||||||
|
t.Errorf("tapLocalY = %v, want in [100, %v) (window-relative)", got, 100+lh)
|
||||||
|
}
|
||||||
|
// Regression: the buggy version returned ~1,539,564 (full scroll leaked in).
|
||||||
|
if got > 1000 {
|
||||||
|
t.Errorf("tapLocalY = %v, full scroll offset leaked in (must be window-relative)", got)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestTapToPosition_LargeFileScrolled is the end-to-end regression: with a
|
||||||
|
// window-relative GlyphLayout and a deep scroll, a tap a few lines below the top
|
||||||
|
// of the window must land on that window line, NOT be clamped to the last window
|
||||||
|
// line (the pre-fix behavior on large files).
|
||||||
|
func TestTapToPosition_LargeFileScrolled(t *testing.T) {
|
||||||
|
lh := float64(EditorLineHeight())
|
||||||
|
const lines = 40
|
||||||
|
gl := ui.GlyphLayout{LineHeight: EditorLineHeight()}
|
||||||
|
for i := 0; i < lines; i++ {
|
||||||
|
for _, x := range []ui.Dp{10, 20} {
|
||||||
|
gl.ByteOffsets = append(gl.ByteOffsets, i*2)
|
||||||
|
gl.X = append(gl.X, x)
|
||||||
|
gl.Y = append(gl.Y, ui.Dp(float64(i+1)*lh))
|
||||||
|
gl.Advance = append(gl.Advance, 10)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
TheState = NewState()
|
||||||
|
TheState.Editor.GlyphLayout = gl
|
||||||
|
TheState.ScrollOffset = 1539464 // deep into a large file
|
||||||
|
|
||||||
|
// Tap ~3 lines below the top of the window.
|
||||||
|
regionTop := ui.Dp(114)
|
||||||
|
ptY := regionTop + ui.Dp(3*lh)
|
||||||
|
localY := tapLocalY(ptY, regionTop, TheState.ScrollOffset)
|
||||||
|
SetCursorFromPoint(15, localY)
|
||||||
|
|
||||||
|
// Should land on window line 3 (byte offset 6), NOT be clamped to the last
|
||||||
|
// window line (line 39, byte offset 78).
|
||||||
|
if TheState.Editor.CursorPosition < 4 || TheState.Editor.CursorPosition > 8 {
|
||||||
|
t.Errorf("cursor = %d, want window line 3 (byte 6); large-file scroll clamp regression?", TheState.Editor.CursorPosition)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -971,10 +971,11 @@ func EditorLayout(screenWidth, screenHeight ui.Dp, wordWrap bool) []ui.Element {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
if pt, ok := data.(ui.Point); ok {
|
if pt, ok := data.(ui.Point); ok {
|
||||||
// Convert window-space tap coordinates to text-local coordinates.
|
// Window-space tap -> window-relative text-local coordinates.
|
||||||
// layout.X is relative to the text region left, and layout.Y is relative to the text region top.
|
// (See tapLocalY: the GlyphLayout is window-relative, so the Y must
|
||||||
|
// be too — never add the full scroll offset here.)
|
||||||
localX := float64(pt.X - editorRegion.X)
|
localX := float64(pt.X - editorRegion.X)
|
||||||
localY := float64(pt.Y - editorRegion.Y + TheState.ScrollOffset)
|
localY := tapLocalY(pt.Y, editorRegion.Y, TheState.ScrollOffset)
|
||||||
SetCursorFromPoint(localX, localY)
|
SetCursorFromPoint(localX, localY)
|
||||||
}
|
}
|
||||||
}},
|
}},
|
||||||
|
|
@ -987,6 +988,18 @@ func EditorLayout(screenWidth, screenHeight ui.Dp, wordWrap bool) []ui.Element {
|
||||||
return []ui.Element{statusBar, editorElem, bottomBar}
|
return []ui.Element{statusBar, editorElem, bottomBar}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// tapLocalY converts a tap's screen Y (Dp) to a text-local Y in the
|
||||||
|
// window-relative coordinate space of the GlyphLayout, where layout.Y==0 is the
|
||||||
|
// top of the VISIBLE WINDOW, not the top of the file. It adds only the sub-line
|
||||||
|
// remainder of the scroll (ScrollOffset mod lineHeight), never the full scroll:
|
||||||
|
// the full scroll would make visualLine a huge content-line number far past the
|
||||||
|
// window's line count, clamping the cursor to the bottom line of the viewport on
|
||||||
|
// any large file (it only worked by luck on small files whose window spanned the
|
||||||
|
// tapped content-line number).
|
||||||
|
func tapLocalY(ptY, regionTopY ui.Dp, scrollOffset ui.Dp) float64 {
|
||||||
|
return float64(ptY-regionTopY) + math.Mod(float64(scrollOffset), float64(EditorLineHeight()))
|
||||||
|
}
|
||||||
|
|
||||||
// SetCursorFromPoint updates the cursor position based on screen coordinates (Dp).
|
// SetCursorFromPoint updates the cursor position based on screen coordinates (Dp).
|
||||||
func SetCursorFromPoint(x, y float64) {
|
func SetCursorFromPoint(x, y float64) {
|
||||||
layout := TheState.Editor.GlyphLayout
|
layout := TheState.Editor.GlyphLayout
|
||||||
|
|
@ -1002,8 +1015,6 @@ func SetCursorFromPoint(x, y float64) {
|
||||||
// So y is the position in the *content*.
|
// So y is the position in the *content*.
|
||||||
visualLine := int(y / lineHeight)
|
visualLine := int(y / lineHeight)
|
||||||
|
|
||||||
log.Printf("SetCursorFromPoint: y=%f, scroll=%f, visualLine=%d, lineHeight=%f", y, float64(TheState.ScrollOffset), visualLine, lineHeight)
|
|
||||||
|
|
||||||
// Group glyphs by their Y-baseline
|
// Group glyphs by their Y-baseline
|
||||||
type lineGroup struct {
|
type lineGroup struct {
|
||||||
y float64
|
y float64
|
||||||
|
|
@ -1076,7 +1087,6 @@ func SetCursorFromPoint(x, y float64) {
|
||||||
rightmostIdx = i
|
rightmostIdx = i
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
log.Printf("TargetGroup: Y=%f, Indices=%v, rightmostIdx=%d, x=%f, rightmostX=%f", targetGroup.y, targetGroup.indices, rightmostIdx, x, rightmostX)
|
|
||||||
|
|
||||||
// 4. Check if tap is to the right of the last character
|
// 4. Check if tap is to the right of the last character
|
||||||
if rightmostIdx != -1 && x > rightmostX {
|
if rightmostIdx != -1 && x > rightmostX {
|
||||||
|
|
@ -1116,15 +1126,12 @@ func SetCursorFromPoint(x, y float64) {
|
||||||
if dist < 0 {
|
if dist < 0 {
|
||||||
dist = -dist
|
dist = -dist
|
||||||
}
|
}
|
||||||
log.Printf("Checking glyph %d: x=%f, centerX=%f, dist=%f", i, x, glyphCenterX, dist)
|
|
||||||
if dist < minDist {
|
if dist < minDist {
|
||||||
minDist = dist
|
minDist = dist
|
||||||
bestIdx = i
|
bestIdx = i
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
log.Printf("BestIdx=%d, ByteOffset=%d", bestIdx, layout.ByteOffsets[bestIdx])
|
|
||||||
if bestIdx != -1 {
|
if bestIdx != -1 {
|
||||||
TheState.Editor.CursorPosition = layout.ByteOffsets[bestIdx]
|
TheState.Editor.CursorPosition = layout.ByteOffsets[bestIdx]
|
||||||
log.Printf("After SetCursor: CursorPosition=%d", TheState.Editor.CursorPosition)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user