diff --git a/doc/development_plan.md b/doc/development_plan.md index abdf173..aa80ea7 100644 --- a/doc/development_plan.md +++ b/doc/development_plan.md @@ -1,7 +1,8 @@ # Development Plan: reach a lean, usable Android text editor -Status: v5, 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 +Status: v6, 2026-08-16 (Phases 0–3 + doc reorg + Phase 6 scroll-perf/clamping +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 docs (`*_implementation_plan.md`, `touch.md`, `element_model.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 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 -files, and memory plateaus ~250 MB (no leak). Remaining: real-device -swipe/autocorrect sign-off (the emulator's AOSP/Gboard keyboard is a proxy). +files, and memory plateaus ~250 MB (no leak). Phase 7 (2026-08-16) verified +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) @@ -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/`, not a loose `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) v1 framed this as "accept a limit vs build a windowed editor." The live repo diff --git a/internal/editor/cursor_test.go b/internal/editor/cursor_test.go index 92352a4..06dea1b 100644 --- a/internal/editor/cursor_test.go +++ b/internal/editor/cursor_test.go @@ -8,8 +8,8 @@ import ( // TestCursorPositioning_Basic verifies cursor positioning via GlyphLayout. func TestCursorPositioning_Basic(t *testing.T) { -TheState = NewState() -TheState.Editor.Buffer = "hello\nworld" + TheState = NewState() + TheState.Editor.Buffer = "hello\nworld" // Two lines: "hello\n" (bytes 0-5) and "world" (bytes 6-10) // 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. func TestCursorPositioning_EmptyDocument(t *testing.T) { -TheState = NewState() -TheState.Editor.Buffer = "" -TheState.Editor.GlyphLayout = ui.GlyphLayout{} + TheState = NewState() + TheState.Editor.Buffer = "" + TheState.Editor.GlyphLayout = ui.GlyphLayout{} // Should not panic on empty layout SetCursorFromPoint(50, 50) @@ -49,8 +49,8 @@ TheState.Editor.GlyphLayout = ui.GlyphLayout{} // TestCursorPositioning_IncompleteLayout verifies cursor positioning handles missing layout slices. func TestCursorPositioning_IncompleteLayout(t *testing.T) { -TheState = NewState() -TheState.Editor.Buffer = "test" + TheState = NewState() + TheState.Editor.Buffer = "test" // GlyphLayout with ByteOffsets and X but no Advance (partial data) TheState.Editor.GlyphLayout = ui.GlyphLayout{ ByteOffsets: []int{0, 1, 2, 3}, @@ -65,9 +65,9 @@ TheState.Editor.Buffer = "test" // TestHandleCursorMove_Bounds verifies cursor movement stays within buffer bounds. func TestHandleCursorMove_Bounds(t *testing.T) { -TheState = NewState() -TheState.Editor.Buffer = "hello" -TheState.Editor.CursorPosition = 0 + TheState = NewState() + TheState.Editor.Buffer = "hello" + TheState.Editor.CursorPosition = 0 // Move right 3 times HandleCursorMove(1) @@ -89,3 +89,57 @@ TheState.Editor.CursorPosition = 0 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) + } +} diff --git a/internal/editor/state.go b/internal/editor/state.go index 29e90a0..60d82b6 100644 --- a/internal/editor/state.go +++ b/internal/editor/state.go @@ -971,10 +971,11 @@ func EditorLayout(screenWidth, screenHeight ui.Dp, wordWrap bool) []ui.Element { return } if pt, ok := data.(ui.Point); ok { - // Convert window-space tap coordinates to text-local coordinates. - // layout.X is relative to the text region left, and layout.Y is relative to the text region top. + // Window-space tap -> window-relative text-local coordinates. + // (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) - localY := float64(pt.Y - editorRegion.Y + TheState.ScrollOffset) + localY := tapLocalY(pt.Y, editorRegion.Y, TheState.ScrollOffset) SetCursorFromPoint(localX, localY) } }}, @@ -987,6 +988,18 @@ func EditorLayout(screenWidth, screenHeight ui.Dp, wordWrap bool) []ui.Element { 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). func SetCursorFromPoint(x, y float64) { layout := TheState.Editor.GlyphLayout @@ -1002,8 +1015,6 @@ func SetCursorFromPoint(x, y float64) { // So y is the position in the *content*. 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 type lineGroup struct { y float64 @@ -1076,7 +1087,6 @@ func SetCursorFromPoint(x, y float64) { 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 if rightmostIdx != -1 && x > rightmostX { @@ -1116,15 +1126,12 @@ func SetCursorFromPoint(x, y float64) { if dist < 0 { dist = -dist } - log.Printf("Checking glyph %d: x=%f, centerX=%f, dist=%f", i, x, glyphCenterX, dist) if dist < minDist { minDist = dist bestIdx = i } } - log.Printf("BestIdx=%d, ByteOffset=%d", bestIdx, layout.ByteOffsets[bestIdx]) if bestIdx != -1 { TheState.Editor.CursorPosition = layout.ByteOffsets[bestIdx] - log.Printf("After SetCursor: CursorPosition=%d", TheState.Editor.CursorPosition) } }