From 110c92e3fad9f4162d4115fdda83f7813d555434 Mon Sep 17 00:00:00 2001 From: Greg Pomerantz Date: Tue, 18 Aug 2026 15:13:05 -0400 Subject: [PATCH] Selection handles: capture base line at grab (fix jitter runaway) The relative-line mapping resolved the anchor's base line from the anchor's CURRENT byte on every drag event. Once the anchor crossed a line, its own movement fed back into its target: a finger jittering near a line boundary added one more line per event and raced the anchor to the bottom of the file (reported: tiny vertical movements jump the anchor off-screen). Capture the anchor's visual line once at the grab (SelDragPressLine) and compute the target from that fixed base. A jitter test (TestSelDrag_EndHandle_JitterNearLineBoundary_Stable) pins the runaway: mutation-verified against the per-event re-resolution. --- doc/development_plan.md | 46 +++++++++++++++----- internal/editor/state.go | 31 ++++++++++---- internal/editor/touch_selection_test.go | 57 +++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 19 deletions(-) diff --git a/doc/development_plan.md b/doc/development_plan.md index 5d6863d..0739ae3 100644 --- a/doc/development_plan.md +++ b/doc/development_plan.md @@ -87,12 +87,31 @@ on an idle window and every menu tap was swallowed. ## 2. Evidence (verified against gioui.org v0.10.0 sources, the live repo's version) -1. **`widget.Editor` is not virtualized** — it shapes the *entire* document per - invalidation and keeps a ~200 B/rune in-memory `glyphIndex`. Measured on this - box: 1 MB ≈ 17 ms/keystroke & ~0.32 GB; 3 MB ≈ 1.7 GB; 4 MB OOM-killed the host. - Planning figure **~0.5 GB of RAM per 1 MB of text**. A phone cannot edit 10 MB - in `widget.Editor`. This is *why* the live repo went the chunked-buffer route, - and it's the reason v1's "just use the widget" was wrong for large files. +1. **`widget.Editor` is not virtualized** — `textView.layoutText` seeks to the + start and shapes the *entire* document on every invalidation (text edit, + size/param change), with an infinite viewport; the shaper's `document.reset()` + is `lines = lines[:0]`, so the backing array of the largest layout is retained + forever. **Re-benchmarked 2026-08-18 against v0.10.0** (plain `text.Shaper`, + whole-document shape, the widget's exact call pattern; x86_64 VM): + + | doc | shape time (per edit/keystroke) | retained heap | + |---|---|---| + | 1 MB | ~120 ms | ~5 MB | + | 2 MB | ~0.4 s | ~7 MB | + | 5 MB | ~3.4 s | ~15 MB | + | 10 MB | ~14 s | ~29 MB | + + The **CPU wall is the decision argument**: a full re-shape on every keystroke + is already sluggish at 1 MB (~8 fps of edits) and unusable above ~2 MB; a + phone CPU (3–10× slower than this VM) is worse. Memory grows only ~3× the + text size with the raw shaper — the earlier "~0.5 GB per MB" figures (which + likely included Android PSS overhead / an older version) do not reproduce, but + they are not needed: the linear per-edit shape cost alone rules out + `widget.Editor` for multi-MB files. This is *why* the live repo went the + chunked-buffer route, and it's the reason v1's "just use the widget" was + wrong for large files. (For small files — sub-MB notes — `widget.Editor` + would be adequate; the custom editor is only strictly required above ~1 MB, + but the app's 50 MB target is 50× that.) 2. **The Android IME works through the op layer, not the widget.** The `window`'s editor state (`app.Window` → `input.EditorState{Selection, Snippet}`) is what `GioInputConnection` (Android's `InputConnection`) reads. It is fed by @@ -975,13 +994,18 @@ downward, dragging the start handle up extends it upward. The mapping is relative (displacement from the grab, not the finger's absolute line) precisely because the grab is usually a line or more below the anchor: tracking the finger's absolute line would first drag the anchor the wrong -way, through a collapse, before it ever reached the anchor's line. -Regressions in `touch_selection_test.go`: +way, through a collapse, before it ever reached the anchor's line. The +base line is captured **once at the grab** and never re-resolved from the +anchor's current byte: that would feed the anchor's own movement back +into its target line, and a finger jittering near a line boundary would +race the anchor off the screen (one extra line per event — reported as +"tiny vertical movements jump the anchor to the top/bottom of the +screen"). Regressions in `touch_selection_test.go`: `TestSelDrag_StartHandle_PressOnLineBelow_KeepsSelection` (the original bug's geometry), `TestSelDrag_EndHandle_DragDownExtendsAcrossLines`, -`TestSelDrag_StartHandle_DragUpExtendsToLineAbove` — all -mutation-verified against both the pre-fix mapping and the -absolute-finger-line variant. +`TestSelDrag_StartHandle_DragUpExtendsToLineAbove`, +`TestSelDrag_EndHandle_JitterNearLineBoundary_Stable` (the runaway) — all +mutation-verified against the pre-fix variants. **Menu anchors to the stable end.** While a start-handle drag is in progress the selection start is the moving end, so the menu anchors to diff --git a/internal/editor/state.go b/internal/editor/state.go index 0f155f2..c382f81 100644 --- a/internal/editor/state.go +++ b/internal/editor/state.go @@ -108,10 +108,15 @@ type EditorState struct { SelDragWhich int SelDragRel int // SelDragPressY is the text-local Y of the first event of an in-progress - // handle/body drag (the grab). Start/end handles move relative to this - // (see handleAnchorPos). - SelDragPressY float64 - Filename string + // handle/body drag (the grab), and SelDragPressLine the visual line of + // the dragged anchor AT THE GRAB. Start/end handles move relative to + // these (see handleAnchorPos). The base line must be captured once at the + // grab: re-resolving it from the anchor's current byte on every event + // makes the anchor's own movement feed back into its target, and a + // jittering finger near a line boundary races the anchor off the screen. + SelDragPressY float64 + SelDragPressLine int + Filename string // TooLarge is set when an opened file exceeds MaxEditableFileSize. The // editor shows a "too large to edit" notice instead of content (the // browser can still list the file). @@ -1109,6 +1114,16 @@ func selDragMove(which int, x, y ui.Dp) { e.SelDragging = true e.SelDragWhich = which e.SelDragPressY = localY + if which == 0 { + if l, ok2 := visualLineOfByte(e.SelectionStart - glyphBase()); ok2 { + e.SelDragPressLine = l + } + } + if which == 1 { + if l, ok2 := visualLineOfByte(e.SelectionEnd - glyphBase()); ok2 { + e.SelDragPressLine = l + } + } if which == 2 && ok { // Body drag: the grab fixes the finger's offset from the selection // start; the selection itself moves on later events. @@ -1173,10 +1188,10 @@ func selDragMove(which int, x, y ui.Dp) { // a deliberate vertical drag crosses lines only after the finger has moved // past half a line height from the grab. func handleAnchorPos(e *EditorState, anchorByte int, localX, localY float64) (int, bool) { - line, ok := visualLineOfByte(anchorByte - glyphBase()) - if !ok { - return textPosFromLocalPoint(localX, localY) - } + // Base line is the anchor's line at the GRAB (captured in selDragMove), + // never the anchor's current line: the anchor's own movement must not + // feed back into its target. + line := e.SelDragPressLine lh := float64(EffectiveLineHeight()) if lh > 0 { target := line + int(math.Round((localY-e.SelDragPressY)/lh)) diff --git a/internal/editor/touch_selection_test.go b/internal/editor/touch_selection_test.go index 5494f10..a486fb6 100644 --- a/internal/editor/touch_selection_test.go +++ b/internal/editor/touch_selection_test.go @@ -479,6 +479,63 @@ func checkSelection12_18(t *testing.T) { } } +// touchSelState3Lines is like touchSelState2Lines with a third line. +func touchSelState3Lines() { + content := "hello world\nsecond line\nthird line here\n" + TheState = NewState() + TheState.Editor.Buffer = content + TheState.Editor.CursorPosition = len(content) + TheState.EditorRegion = ui.Region{X: 16, Y: 100, W: 379, H: 700} + TheState.ScrollOffset = 0 + TheState.Editor.IMEWindowStartByte = 0 + TheState.Editor.IMEWindowText = content + lh := ui.Dp(16.8) + bo := []int{} + xs := []ui.Dp{} + s := []ui.Dp{} + ad := []ui.Dp{} + lines := []struct{ start, n int }{{0, 11}, {12, 11}, {24, 15}} + for li, l := range lines { + for j := 0; j < l.n; j++ { + bo = append(bo, l.start+j) + xs = append(xs, ui.Dp(10*j)) + s = append(s, ui.Dp(float64(lh)*float64(li))) + ad = append(ad, 10) + } + } + TheState.Editor.GlyphLayout = ui.GlyphLayout{ + LineHeight: lh, + ByteOffsets: bo, + X: xs, + Y: s, + Advance: ad, + } +} + +// TestSelDrag_EndHandle_JitterNearLineBoundary_Stable is a regression test +// for a feedback runaway: the target line must be computed from the +// anchor's line AT THE GRAB. Re-resolving the base line from the anchor's +// current byte on every event makes the anchor's own movement feed into its +// target: once the anchor crosses a line, every further event adds another +// line, and a jittering finger near a boundary races the anchor to the +// bottom of the file. +func TestSelDrag_EndHandle_JitterNearLineBoundary_Stable(t *testing.T) { + touchSelState3Lines() + HandleLongPressAt(18, 108) // selects "hello" [0,5) + checkInitialSelection(t) + // End handle (which=1), anchor byte 5 on line 0. Grab at local (52, 25). + HandleSelDragEvt(ui.SelectionDragEvent{Which: 1, X: 16 + 52, Y: 100 + 25}) + checkInitialSelection(t) + // The finger hovers just past the half-line boundary (dY = 10, 11, 10 + // around the 8.4 threshold) and jitters. The anchor must settle one line + // down (byte 17, the 'n' of "second") and STAY there — not creep to + // line 2 (byte 29+) or further with each event. + for _, dy := range []int{35, 36, 35, 36, 35} { + HandleSelDragEvt(ui.SelectionDragEvent{Which: 1, X: 16 + 52, Y: ui.Dp(100 + dy)}) + assertSelection(t, 0, 17) + } +} + func checkInitialSelection(t *testing.T) { t.Helper() s := TheState.Editor