Implement the Android-native touch selection model, verified on-device: - long-press selects the word under the finger (blank -> caret + paste-only menu); double-tap selects the word; drag handles resize the selection, drag the highlighted body to move it; floating menu offers copy/cut/paste (selection) or paste (bare caret), closing on any item tap. - Renderer reports finger positions (app-local Dp) as tap/double-tap/ long-press/selection-drag events; the logic goroutine owns all geometry (EditorRegion, menu rect, hit-testing, handles) and the renderer only draws the frame snapshot. - Long press: 400 ms still-press on the editor, cancelled by movement (non-grabbing raw pointer probe) or by a scroll/handle grab. The main loop keeps invalidating while a press is pending (Gio renders on demand; a stationary finger produces no frames). - Clipboard crosses the goroutine boundary via buffered channels (clipboardSetChan/pasteReqChan logic->main, pasteChan main->logic); main executes the Gio ops and, on Android, invalidates after ReadCmd because a queued transfer.DataEvent schedules no frame of its own. Renderer fixes found while validating on-device: - clickReg was stored by value in a map; range yielded copies so per-frame press bookkeeping (long-press state) was silently discarded. Now pointers. - On Android a tap's press+release arrive in the same frame and gesture.Click/Drag return one event per Update call; without draining each gesture's queue every frame the release was lost on an idle window and every menu tap was swallowed (needed a second tap to 'rescue' it). Click and drag loops now drain to exhaustion (scroll already does). - pointer.Filter queries must name Kinds: a zero-kinds filter matches nothing (the press-probe query was dead). - Menu.Draw offsets items by the menu origin; the clippable drawElement branch registers SelDrag (handles now draw for the TextField). Tests: internal/editor/touch_selection_test.go (word range, long-press, double-tap, tap/menu guards, handle drags, menu actions, selection edits) and internal/test/e2e/touch_selection_e2e_test.go; full suite green under -race. Docs: spec.md §2.2 + §7, architecture.md §6.3a, development_plan.md Phases 8-9.
581 lines
36 KiB
Markdown
581 lines
36 KiB
Markdown
# Development Plan: reach a lean, usable Android text editor
|
||
|
||
Status: v8, 2026-08-17 (Phases 0–3 + doc reorg + Phase 6 scroll-perf/clamping
|
||
verification + tap-to-position-cursor fix + selection + real-file e2e +
|
||
Android arrow/shift workaround + touch selection). 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`,
|
||
`conflict_resolution.md`, `bugs.txt`) were deleted; `spec.md` and
|
||
`architecture.md` were rewritten to describe the actual app; `doc/README.md`
|
||
adds the doc policy + build/install recipe. See `doc/README.md`.
|
||
Phases 0–3 done: single-owner no-lock architecture, Android IME wiring, on-device
|
||
IME validation (passing), viewport-on-open fix, chunked-buffer drift fix, the
|
||
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). Phase 7 (2026-08-16) verified
|
||
tap-to-position-cursor on-device and found + fixed two real bugs: (1) 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); fixed with `tapLocalY`. (2) the `GlyphLayout` byte offsets are
|
||
window-relative but the four cursor functions (tap, Home, End, vertical move)
|
||
treated them as absolute, so the cursor snapped to the window top; fixed by adding
|
||
the `IMEWindowStartByte` window base at each cursor boundary (`glyphBase`). Both
|
||
have regression tests.
|
||
Phase 8 (2026-08-16/17) added **text selection** (shift+arrow extend; insert/
|
||
backspace/delete replace the selection; IME unions its range with the active
|
||
selection) with rendering + IME wiring. It also added **real-file e2e tests**
|
||
(`internal/test/e2e/real_file_*_test.go`: open/edit/autosave on real on-disk
|
||
files, incl. chunk-boundary and multi-byte edits) and found + fixed two real
|
||
bugs: (1) `UpdateLineIndexAfterEdit` only shifted offsets — any edit involving
|
||
newlines left the line index permanently inconsistent; replaced with
|
||
newline-aware `UpdateLineIndexAfterInsert`/`UpdateLineIndexAfterDelete`.
|
||
(2) `HandleBackspace`/`HandleDelete` deleted **one byte**, corrupting
|
||
multi-byte UTF-8 characters; now rune-granular.
|
||
On-device validation (2026-08-17) exposed a **third, platform-level gap**:
|
||
Gio v0.10 on Android (a) drops modifier state in the JNI bridge, and (b)
|
||
wraps plain arrow-key *presses* in `input.SystemEvent` for focus navigation,
|
||
so arrow keys never reached the editor and shift+arrow was impossible.
|
||
Fixed in main.go: explicit named `key.Filter`s for the four arrows (delivers
|
||
the press and suppresses the focus jump) plus app-side shift tracking.
|
||
Verified end-to-end on the emulator: plain arrows move the caret,
|
||
shift+arrow shows a highlight, typing replaces the selection. Remaining:
|
||
real-device swipe/autocorrect sign-off (the emulator's AOSP/Gboard keyboard
|
||
is a proxy).
|
||
Phase 9 (2026-08-17) added **touch selection** — the Android-native selection
|
||
model: long-press selects the word under the finger (blank → caret + paste-only
|
||
menu), double-tap selects the word, drag handles resize the selection, drag the
|
||
highlighted body moves it, and a floating copy/cut/paste menu is hit-tested in
|
||
logic and drawn by the renderer. All flows verified on-device, including the
|
||
full copy→paste and cut cycles through the real Android clipboard. Debugging
|
||
this surfaced two renderer bugs worth knowing about: (1) a map of click-registry
|
||
**values** (not pointers) silently discarded per-frame mutations, so the
|
||
long-press never fired; (2) on Android a tap's press and release arrive in the
|
||
**same frame**, and `gesture.Click`/`gesture.Drag` return one event per Update
|
||
call, so without draining each gesture's queue every frame the release was lost
|
||
on an idle window and every menu tap was swallowed.
|
||
|
||
## 1. Decision summary (updated)
|
||
|
||
- **Primary path: B — finish the live custom path, do NOT rebuild on widgets.**
|
||
The live repo already has the two hardest, most-asset-heavy pieces that a
|
||
rebuild would throw away: a **real filesystem** backend (`internal/io/pool/real/`)
|
||
and a **working Android APK** (JNI + permissions, built + signed). It also has
|
||
a **chunked buffer** (`internal/editor/chunked_buffer.go`) that already implements
|
||
the spec's "file never fully in memory" — the thing v1 said would need fork-level
|
||
work on `widget.Editor`. So the only blocker to the headline feature (Android IME:
|
||
swipe typing, autocorrect) is a **small, precisely-defined wiring gap** (§4), not a
|
||
rewrite.
|
||
- **Concurrency:** keep the existing logic-goroutine + channels model for now. It's
|
||
proven to build and (after this plan) pass tests. The v1 "one main goroutine"
|
||
simplification is a *later* cleanup, not a prerequisite for usability.
|
||
- **File-size limit:** the chunked buffer targets large files directly; we validate
|
||
it against a real 10 MB file on-device and set the honest limit from that, rather
|
||
than inheriting `widget.Editor`'s ~0.5 GB/MB wall (§2).
|
||
|
||
## 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.
|
||
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
|
||
`key.SelectionCmd` and `key.SnippetCmd` ops tagged with a **focusable** handler.
|
||
A handler becomes focusable by emitting a `key.FocusFilter{Target: tag}` (that's
|
||
all `widget.Editor` does — it never touches `io/input` at all). The live code
|
||
already emits `key.FocusCmd{Tag}` + `key.FocusFilter{Target}` and consumes
|
||
`key.EditEvent`/`key.SnippetEvent`, so the *skeleton* is present; the stateful
|
||
ops are missing (§4).
|
||
3. **The IME snippet is selection-scoped** — so a chunked/virtualized editor can
|
||
still feed a correct snippet (only the visible/selected window), which the live
|
||
chunked-buffer design is compatible with.
|
||
4. **No headless test window** in v0.9 or v0.10 — widget-level Go tests are
|
||
impossible; on-device e2e is required (§9).
|
||
5. **The dev agent has no vision** (verified) — the emulator debug loop is
|
||
data-based (state dumps, logcat, in-app frame-delta logs, PIL/OCR), not
|
||
screenshot-judgment
|
||
(§9.3).
|
||
|
||
## 3. Current state of the live repo (post housekeeping, 2026-08-16)
|
||
|
||
- `go build ./...` green. **`go test ./...` and `go test -race ./...` green.**
|
||
- `03fb638` fixed the red build (harness + 5 call sites after `NewLogic` gained
|
||
a `path` and `openfunc` parameter); the e2e harness uses a no-op `openfunc`
|
||
(matching `impl_other.go`, whose real `OpenFile` is a no-op off-Android).
|
||
- `7240b62` committed the JNI/Termux open-file bridge + word-wrap-aware
|
||
viewport/scroll WIP as the working-tree baseline.
|
||
- `58725a5` made state single-owner and turned the race detector green
|
||
(see §13).
|
||
- The working tree is a clean, race-clean baseline. Ready for the IME work.
|
||
|
||
## 4. The IME wiring gap (verified, testable) — THE central work item
|
||
|
||
Android IME text (swipe, autocorrect) arrives as `key.EditEvent{Range, Text}`
|
||
routed to the focused tag. Four things are missing or wrong:
|
||
|
||
1. **No `key.SelectionCmd` emitted.** Without it the driver's
|
||
`EditorState.Selection.Caret` stays at origin, so the IME doesn't know where the
|
||
caret/selection is → suggestions anchor wrongly and the caret jumps after a
|
||
commit. **Fix:** in the editor's draw/update pass, emit
|
||
`gtx.Execute(key.SelectionCmd{Tag, Range:{cursor,cursor}, Caret:{Pos,Ascent,Descent}})`
|
||
whenever the caret moves. The caret's **pixel** position already exists in
|
||
`internal/ui/render.go:534-554` (the code that draws the caret) — it must be
|
||
plumbed into `app.State` (caret pos in px) and read back here. This is the one
|
||
item coupled to the cursor-positioning work, but it reuses the same geometry.
|
||
2. **No `key.SnippetCmd` emitted.** Without it the IME receives an empty snippet →
|
||
no autocorrect context and no in-place swipe replacement. **Fix:** emit
|
||
`gtx.Execute(key.SnippetCmd{Tag, Snippet:{Range: selection, Text: selectedText}})`;
|
||
with no selection, an empty snippet at the caret (exactly what `widget.Editor`
|
||
sends). Feeding it from the chunked buffer is O(selection size), not O(file).
|
||
3. **`HandleKeyDown` ignores `EditEvent.Range`.** `state.go:313` does
|
||
`HandleInsert(v.Text)` for any non-backspace text and never deletes
|
||
`v.Range.Start..End`. A *replacement* commit (the normal swipe/autocorrect case)
|
||
therefore inserts the new text **without removing the old** → duplication/corruption.
|
||
**Fix:** `Replace(v.Range.Start, v.Range.End, v.Text)` on the chunked buffer and
|
||
advance the cursor to `Start + len(Text)`. (~10 lines; `ChunkedBuffer` already
|
||
has `Insert`/`Delete`.)
|
||
4. **No `key.InputHintOp{Tag, Hint: key.HintText}`.** Cosmetic but correct to add —
|
||
hints the on-screen keyboard into text/autocorrect mode.
|
||
|
||
Acceptance (all verifiable headlessly, §9): after an IME commit, the buffer equals
|
||
the expected post-replacement text, the caret is at the commit end, and the next
|
||
frame's `SelectionCmd`/`SnippetCmd` reflect that state. On a real IME, swipe +
|
||
autocorrect produce clean, non-duplicated text.
|
||
|
||
## 5. Phases (Path B)
|
||
|
||
### Phase 0 — clean baseline (small) — DONE (`58725a5`)
|
||
1. Commit the uncommitted JNI/chunk-buffer work. — `7240b62`.
|
||
2. `go test -race ./...` green (add `-race`; the channel model may surface races —
|
||
fix any found). — `58725a5`. Details in §13.
|
||
|
||
### Phase 1 — complete the IME (§4) — DONE (`9b78219`, `72b3c3f`)
|
||
All four items implemented and tested:
|
||
- **Item 3** (`9b78219`): `HandleKeyDown` honors `key.EditEvent.Range` via
|
||
`HandleReplaceRange` (rune→byte via UTF-8 leading-byte scan, string + chunked
|
||
paths). Also fixed a multi-chunk `Delete` corruption and a `FullContent`
|
||
truncation bug found while testing it.
|
||
- **Items 1/2/4** (`72b3c3f`): `TextField.Draw` emits, when focused,
|
||
`key.InputHintOp{HintText}`, `key.SnippetCmd` (the **visible window** as the
|
||
snippet, `Range {0,len}` so the IME reports `EditEvent.Range` window-relative),
|
||
and `key.SelectionCmd` (caret, window-relative rune index). `HandleReplaceRange`
|
||
offsets the window-relative range by `IMEWindowStartByte` (new `EditorState`
|
||
fields set during layout) to address the buffer.
|
||
|
||
Tests: `ime_range_test.go` covers string + chunked + unicode + swapped-bounds +
|
||
windowed (scrolled) paths for item 3. Items 1/2/4 are renderer-side op emission
|
||
(not observable headlessly); they are verified by build + on-device (Phase 2).
|
||
|
||
Note: pushing the visible window (not the whole file) as the snippet keeps IME
|
||
traffic small for large files. An `EditEvent.Range` that falls outside the
|
||
window (the IME discarding and re-anchoring the snippet — a rare case) is
|
||
clamped to the window end by the byte-conversion, so it degrades rather than
|
||
corrupting; the common within-window path is exact.
|
||
|
||
### Phase 2 — emulator verification — DONE (IME commit path validated on-device)
|
||
Environment (this 16 GB VM): Android SDK + NDK 27 + platform-tools + emulator +
|
||
API 35 `google_apis/x86_64` system image installed to `~/android-sdk`; AVD
|
||
`pad_avd` (Pixel 6 profile) boots with KVM (`/dev/kvm` chmod 666). Build recipe:
|
||
`gogio -target android -targetsdk 35 -arch amd64` → inject
|
||
`MANAGE_EXTERNAL_STORAGE` via apktool → sign with the debug keystore →
|
||
`adb install -r` (scripted in `scripts/build_emu.sh`).
|
||
|
||
**On-device IME validation — PASSING** (Gboard, API 35; observed via the gated
|
||
`imeDebugLog` trace + logcat + autosaved file diff + screenshots):
|
||
- Browser lists `/storage/emulated/0/Notes`; a row tap opens the file in the
|
||
in-app editor (see the open-file fix below).
|
||
- Editor focus → `key.FocusCmd`/`FocusFilter` → Gboard shows (soft input up).
|
||
- **Single-char commit** (tap a Gboard key): one `EditEvent{Range:{c,c},Text}` →
|
||
`HandleReplaceRange` inserts at the caret; buffer + cursor + autosaved file all
|
||
correct.
|
||
- **Multi-char commit** (tap keys with human ~0.6 s gaps): cursor advances in sync
|
||
(c→c+1→c+2…), each char lands at the right byte offset, file correct.
|
||
- **Deletion** (Gboard backspace): routed through the IME as
|
||
`EditEvent{Range:{i,i+1},Text:""}` (i.e. `deleteSurroundingText`) →
|
||
`HandleReplaceRange` deletes the range correctly.
|
||
- **Unicode** content (`héllo wörld 日本語`) displays and the buffer stays
|
||
consistent through edits around it.
|
||
- Conclusion: §4 (the IME wiring gap) works end to end for a real IME's commit
|
||
path. Swipe-typing and autocorrect use the *same* commit path
|
||
(`commitText`/`setComposingText` → `EditEvent`), so they are covered by this;
|
||
a real-device final sign-off with a swipe/autocorrect IME is still worth doing.
|
||
|
||
**Bugs found + fixed on-device (Phase 2):**
|
||
1. **Open-file crash (fixed, `c9c0d47`):** browser-row taps were routed through
|
||
the Android Termux bridge (`ui.OpenFile = openfunc`), which built a `file://`
|
||
Intent URI and crashed on Android 7+ with `FileUriExposedException`, and it
|
||
bypassed the editor entirely. Fix: `ui.OpenFile` now calls the in-app
|
||
`editor.OpenFile(path)`; the Termux bridge is retained as a dormant
|
||
`TheState.open` hook.
|
||
2. **Viewport opens at EOF (open bug, Phase 3):** after a file opens, the
|
||
*opening* tap leaks into the now-visible editor and `SetCursorFromPoint`/scroll
|
||
moves the viewport to the tapped row (below a short file's content) → the
|
||
content area renders blank (`VisibleByteRange` lands on a past-EOF visual
|
||
line; `visibleContent (0)`). Workaround used during testing: swipe to top.
|
||
Root cause is the tap that switches `page=EditorPage` also being delivered as an
|
||
editor tap. Needs: ignore the opening tap in the editor (or clamp
|
||
`SetCursorFromPoint`/scroll so a short file that fits the viewport never
|
||
scrolls).
|
||
3. **Rapid synthetic IME commits desync (edge case) — FIXED:** `adb shell input
|
||
text "…"` fires per-char commits faster than a frame; the per-frame
|
||
`SnippetCmd`/`SelectionCmd` re-push reset the IME's cursor, so fast commits
|
||
interleaved/corrupted (observed `" IME123"` → `E123IM…`). Fixed by deduping the
|
||
IME ops in `TextField.Draw` — push the snippet/selection only when they change
|
||
(and force a fresh push on focus (re)gain), mirroring `widget.Editor`'s
|
||
`updateSnippet`/selection gating. State lives in the main-owned `Renderer`
|
||
(commit `46383c1`). On-device, rapid commits (0.08–0.1 s cadence) now land
|
||
cleanly.
|
||
|
||
Observation loop that worked (no state-dump flag needed in the end): logcat
|
||
(`imeDebugLog` + `HandleKeyDown` + `VisibleByteRange`), autosaved-file diff via
|
||
`adb shell cat`, and screenshots (vision, auxiliary).
|
||
|
||
### Phase 3 — large-file validation + honest size limit — DONE
|
||
1. **Open a real 10 MB file** via the chunked buffer: opens in ~121 ms
|
||
(stat→read→index), renders correctly, smooth scroll. ✓
|
||
2. **Fixed the whole-file shaper leak** (the real 1 GB memory bug): with word
|
||
wrap on (the default), `VisibleByteRange` used the previously-shaped
|
||
`GlyphLayout.VisualLineStarts` to bound the visible range. That layout only
|
||
covers the visible window (~50 lines), not the document, so once the
|
||
viewport's line count exceeded the window's visual-line count the range fell
|
||
back to `end=fileLen` and the shaper laid out the **entire file** every frame.
|
||
Gio's shaper `document.reset()` keeps the backing-array cap, so memory grew
|
||
to the largest layout ever shaped and never shrank (~1.1 GB PSS for 10 MB,
|
||
OOM-killed under scroll). Fix: always derive the range from the real-line
|
||
`LineIndex`; word wrap needs no separate path (each real line yields ≥1
|
||
visual line). Now ~150 MB PSS / ~230 MB RSS at steady state, flat under
|
||
scroll. (commit `c79c142`)
|
||
3. **Fixed the pre-existing LineIndex storage mismatch**: `EditorLayout` read
|
||
`TheState.Editor.LineIndex` (never set; `SetLineIndex` had zero callers) for
|
||
`maxScroll`/`scrollByteOffset`, always using the huge pre-index estimate. Now
|
||
reads `cb.LineIndex`; the dead field + setter are removed.
|
||
4. **Honest size limit = 50 MB** (`MaxEditableFileSize`), measured on-device
|
||
(10 MB → ~150 MB PSS; 50 MB extrapolates to a few hundred MB, fine on a
|
||
phone). Larger files get a "too large to edit" state; the browser still
|
||
lists them. ✓
|
||
5. **IME rapid-commit desync fix**: `TextField.Draw` now dedups the IME
|
||
`SnippetCmd`/`SelectionCmd` (push only on change, fresh push on focus
|
||
(re)gain; state in the main-owned `Renderer`), mirroring `widget.Editor`.
|
||
Rapid commits (0.08–0.1 s cadence) land cleanly on-device (commit
|
||
`46383c1`). ✓
|
||
6. **Remaining (not blocking usability):** real-device swipe/autocorrect
|
||
sign-off (the emulator's AOSP/Gboard keyboard is a proxy for real IMEs).
|
||
|
||
### Phase 4 — the v1 simplifications, now that it's usable (optional, later)
|
||
Only after the app is usable: replace globals (`TheState`, `ui.OpenFile`) with
|
||
explicit state, prune dead task types, and *then* consider collapsing the
|
||
logic-goroutine/channel model to a single owner. These reduce future bug surface
|
||
but are not needed for a usable v1.
|
||
|
||
### Phase 5 — hardening (mostly done)
|
||
- ✓ Rewrite `doc/architecture.md` to match reality (2026-08-16: now describes
|
||
the single-owner/no-lock model, channel topology, Frame contract, ownership
|
||
rules; over-detailed companion docs deleted — see `doc/README.md` policy).
|
||
- ✓ Amend `spec.md` per §7 (2026-08-16: rewritten; unbuilt features moved to an
|
||
explicit “deferred” table; see the corrections noted under §7 below).
|
||
- ✓ Build/install recipe (2026-08-16: in `doc/README.md`; self-contained
|
||
in-repo script `scripts/build_emu.sh`).
|
||
- ☐ In-repo device test checklist (the adb tap/swipe/IME sequences used in
|
||
Phases 2–3 and Phase 6 are in this plan's phase notes but not a standalone
|
||
checklist).
|
||
|
||
### Phase 6 — scroll performance & clamping verification — DONE (2026-08-16)
|
||
|
||
Goal: (a) confirm scroll does not degrade at large offsets in a large file,
|
||
(b) confirm scroll clamping is correct across file sizes. Enabled the default-off
|
||
in-app profiler (architecture.md §11) and drove deterministic `frac` scroll
|
||
jumps + real swipes on the emulator.
|
||
|
||
**Scroll performance at large offsets (10 MB file, 130,955 lines):** swept
|
||
`frac` 0.02→1.0 with swipes. Logic-frame cadence is **flat across all offsets**
|
||
(p50 ~38–44 ms, p90 ~51–75 ms, no trend up at 0.9/0.98/1.0) — **no large-offset
|
||
degradation**. The visible byte range stays **≤ 4,274 B (0.04% of the file)** at
|
||
every offset, confirming the shaper never lays out the whole file. `ScreenRecord`
|
||
and `gfxinfo` were ruled out as metrics (downsampled / View-layer-only, since Pad
|
||
renders into a `SurfaceView`); the in-app profiler is the instrument.
|
||
|
||
**Scroll clamping (6 sizes, 2→130,955 lines):** for each, `top` and `bottom`
|
||
commands set the offset to exactly 0 and to exactly `maxScroll`; **no frame ever
|
||
exceeded `maxScroll` or went negative**. Sub-viewport files (`tiny_1line` 2 lines,
|
||
`small_10` 11 lines) correctly have `maxScroll = 0` and cannot scroll; larger
|
||
files' `maxScroll` scales correctly with content. No blank viewport at any size
|
||
(content verified on-screen).
|
||
|
||
**Memory (bonus):** PSS **plateaus ~250 MB** for the 10 MB file under sustained
|
||
scroll (bounded high-water mark from the shaper glyph cache + Go heap; grows
|
||
~13 MB over the first ~30 scrolls then flat) — **no leak**, well under the 2.5 GB
|
||
OOM line. The profiler's own overhead is negligible (same plateau with it off).
|
||
|
||
**Test-harness gotcha:** on-screen tap coordinates are not the naive
|
||
Dp→px map — see `README.md` §Screen coordinates (screen px vs display px
|
||
vs app-local; the tap rule is "read off the screenshot, ×1.2"). Don't
|
||
memorize content positions (file rows etc.); measure them from the current
|
||
screenshot. Verify file opens from the title bar / file content, not
|
||
logcat (the open log line was later removed as noise).
|
||
|
||
### 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).
|
||
|
||
### Phase 8 — selection on-device + Gio Android key limitation — DONE (2026-08-17)
|
||
|
||
Verified shift+arrow selection on the emulator, then root-caused why plain
|
||
hardware arrows never reached the editor on Android (see the Phase 8 note in the
|
||
status header): Gio v0.10's Android JNI drops modifier state, and plain arrow
|
||
*presses* are wrapped in `input.SystemEvent` for focus navigation. Fix in
|
||
main.go: explicit named `key.Filter`s for the four arrows (delivers the press
|
||
and suppresses the focus jump) plus app-side shift tracking (`NameShift`
|
||
press/release, reset on focus loss); forwarded events carry
|
||
`Shift: Modifiers.Contain(ModShift) || shiftDown`. Verified on-device: `keyevent
|
||
22` moves the caret, `keycombination 59 22` extends the selection, typing
|
||
replaces it. Documented as a Gio-version-sensitive workaround (architecture.md
|
||
§2.1).
|
||
|
||
### Phase 9 — touch selection — DONE (2026-08-17)
|
||
|
||
Implemented the Android-native touch selection model (v1 scope: word selection
|
||
+ resize + move + floating menu; no handle-tap-to-caret, no marquee, no
|
||
auto-scroll-to-cursor). See spec.md §2.2 and architecture.md §6.3 for the
|
||
contracts; the highlights:
|
||
|
||
- **Renderer reports finger positions, logic owns all geometry.** Touch events
|
||
are delivered as `ui.Point` (tap), `ui.DoubleTapPoint`, `ui.LongPressPoint`
|
||
(app-local Dp); the logic goroutine converts them to text coordinates with
|
||
the stored `EditorRegion` + scroll offset, hit-tests menu items itself, and
|
||
owns the menu rect, highlight range, and handle positions. The renderer only
|
||
draws what the frame snapshot says.
|
||
- **Long press needs invalidation to elapse.** Gio renders on demand; a
|
||
stationary finger produces no events and no frames, so the 400 ms threshold
|
||
could never be checked. The main loop polls `Renderer.PendingLongPress()`
|
||
and keeps `w.Invalidate()`-ing while a press is held still on the editor.
|
||
A non-grabbing raw pointer probe (`event.Op` tag) watches the press for
|
||
movement so a scroll cancels the pending long press; scroll/drag gestures
|
||
grab the pointer (`pointer.GrabCmd`) and cancel it the same way.
|
||
- **One event per Update is a trap on Android.** A tap's down+up routinely
|
||
land in one frame; `gesture.Click`/`gesture.Drag` return at most one event
|
||
per `Update` call, so the release sat in the queue until the *next* redraw —
|
||
which on an idle window never comes. The renderer now drains each gesture's
|
||
queue to exhaustion every frame (scroll already drains internally).
|
||
- **Clipboard crosses the goroutine boundary via channels; the ops run on the
|
||
main/Gio frame path** (`clipboardSetChan`/`pasteReqChan` logic→main,
|
||
`pasteChan` main→logic, all buffered so the harness never blocks logic).
|
||
On Android, `clipboard.ReadCmd` is answered synchronously during op flush
|
||
with a queued `transfer.DataEvent` that schedules **no** frame of its own,
|
||
so main invalidates the window after each read to guarantee a follow-up
|
||
frame in which the DataEvent is consumed and forwarded to logic.
|
||
- **Bugs found while validating on-device:** `clickReg` stored by value in a
|
||
map (per-frame press bookkeeping lost → long press never fired); `Menu.Draw`
|
||
didn't offset items by the menu origin; the clippable `drawElement` branch
|
||
skipped `SelDrag` registration (handles never drew); the press-probe query
|
||
omitted `Kinds` (a `pointer.Filter` with zero kinds matches nothing).
|
||
- **Verification:** unit tests (`touch_selection_test.go`, ~18) + e2e
|
||
(`touch_selection_e2e_test.go`) green under `-race`; on-device: long-press
|
||
word/blank, double-tap, handle-drag resize, single-tap copy/cut/paste
|
||
(including copy→paste and cut cycles through the real Android clipboard),
|
||
menu close-on-tap, plain-tap caret placement, and scroll-drag not firing a
|
||
long press.
|
||
|
||
## 6. File-size decision (re-framed)
|
||
|
||
v1 framed this as "accept a limit vs build a windowed editor." The live repo
|
||
already chose the better option — a **chunked buffer** (only viewport ± window
|
||
chunks resident, async prefetch, dirty-chunk protection, async line index). So the
|
||
decision is now: **validate the existing chunked buffer against a real 10 MB file
|
||
(Phase 3) and set the limit from the measurement.** No fork of Gio is on the
|
||
critical path. The chunked buffer proved out on a real 10 MB file (Phase 3, §5):
|
||
10 MB+ editing works with ~linear, bounded memory, without the 0.5 GB/MB wall
|
||
that kills `widget.Editor`.
|
||
|
||
## 7. Spec deltas (WRITTEN into spec.md, 2026-08-16 rewrite)
|
||
|
||
1. ✓ "No practical limits / file never fully in memory" → replaced by the
|
||
measured 50 MB hard limit + `TooLarge` state (spec.md §2.3).
|
||
2. ✓ Correction: undo is **not implemented at all** (not "in-session only") —
|
||
there is no undo stack in the code; listed as deferred (spec.md §7).
|
||
3. ✓ IME/swipe/autocorrect support is explicit (delivered Phase 1; spec.md
|
||
§2.2). Real-device swipe sign-off remains the one open validation item.
|
||
4. ✓ Correction: external-change detection is **not implemented** (no mtime
|
||
compare on open/resume, no watcher) — listed as deferred (spec.md §7), not
|
||
as a current requirement.
|
||
|
||
## 8. Non-goals (v1)
|
||
|
||
- File-system watcher / live browser refresh (re-scan on return to browser).
|
||
- Tabs, split view, search-in-file, syntax highlighting, diff/merge.
|
||
- Desktop/other platforms (Android-first; window `390×844` dp).
|
||
- Rewriting on `widget.Editor` (v1 plan) — kept only as fallback (§12).
|
||
|
||
## 9. Testing strategy
|
||
|
||
The **existing** e2e harness is **kept** (it builds and passes now) and extended —
|
||
v1's "delete the harness" is wrong for Path B. E2E regression works in three layers:
|
||
|
||
### Layer 1 — Go unit tests on pure logic (the bulk)
|
||
- Port/keep `browser` sort/search/scan tests; keep `editor` chunked-buffer,
|
||
cursor, autosave tests.
|
||
- **New:** range-replace on `ChunkedBuffer` (§4.3) — pure, directly testable; the
|
||
exact corruption bug becomes a regression test.
|
||
- **New:** IME state transitions — after an `EditEvent{Range,Text}`, assert
|
||
buffer + caret; assert the emitted `SelectionCmd`/`SnippetCmd` reflect state.
|
||
- **New:** restore JSON round-trip; offset-vs-mtime validation; external-change
|
||
decision table; file-size guard.
|
||
- CI gate: `go build && go vet && go test -race ./...`.
|
||
|
||
### Layer 2 — UI thin by construction
|
||
Draw functions are wirings of the existing element model over `app.State`;
|
||
positioning math stays in one place (`render.go`). The cursor/caret geometry that
|
||
§4.1 reuses is the same code path as drawing, so IME correctness and visual
|
||
correctness share a foundation.
|
||
|
||
### Layer 3 — scripted on-device e2e (emulator now available)
|
||
The in-app selftest drives the **real state machine through the real draw path**
|
||
(browser entries → open file → edit → autosave deadline → reload-prompt decision),
|
||
logging PASS/FAIL to a file the host `adb pull`s and asserts.
|
||
|
||
### 9.3 Emulator observation layer (this 16 GB VM, KVM verified)
|
||
Gio renders into one GL surface, so Android's view hierarchy exposes nothing about
|
||
our UI. The debug loop is data-based — *more* precise than pixels for this codebase.
|
||
Vision is now enabled and verified in-session (screenshots can be read), but it is
|
||
only an **auxiliary** check: state dumps, logcat, in-app frame-delta logs, and
|
||
file diffs remain the authoritative correctness signals.
|
||
1. **State-dump debug flag** (add in Phase 1): debug builds write `app.State`
|
||
(browser entries, editor text length, cursor px, scroll, dirty, row geometry)
|
||
as JSON to the app data dir on a magic tap or 2 s interval; host `adb pull`s
|
||
and asserts. Primary "eyes."
|
||
2. **logcat** — Go panics + our logs.
|
||
3. **Frame timing for the 60 fps claims — `dumpsys gfxinfo` DOES NOT WORK
|
||
here (verified 2026-08-16): it reported 0 frames while the app was visibly
|
||
rendering, because Pad draws into a `SurfaceView` and gfxinfo only measures
|
||
the View-layer pipeline. Use temporary in-app instrumentation instead:
|
||
timestamp each frame in the main loop, log deltas, report p50/p90/p99/max.
|
||
`screenrecord` is a human artifact, not an agent metric (it captures ~22
|
||
fps with encoder-confounded timestamps).
|
||
4. **Screenshot → PIL color histogram / tesseract OCR** — coarse on-screen checks.
|
||
5. **File diffs** via `adb pull` — autosave/restore correctness.
|
||
6. **Driving** — `input tap/swipe/keyevent/text`, `am start/force-stop`,
|
||
`ime`/`settings` to pick the AOSP keyboard; swipe-typing exercised via
|
||
`input swipe` over the keyboard area and verified **behaviorally** (did the
|
||
commit land, non-duplicated, cursor correct?), not visually.
|
||
|
||
## 10. Definition of done (whole effort)
|
||
|
||
On an emulator (and a real phone for final IME sign-off): launch → previous file
|
||
+ caret restored → browse a large directory smoothly → open a real file →
|
||
**swipe-type with autocorrect, clean and non-duplicated** → close and reopen, caret
|
||
where you left it → external modification → reload prompt → autosave lands on disk
|
||
within ~1 s. Repo: `go test -race ./...` green.
|
||
|
||
## 11. Risks / watch-items
|
||
- **§4.1 caret plumbing** is coupled to the cursor-positioning geometry; if that
|
||
geometry is itself buggy, the IME will inherit it. Mitigation: the state-dump
|
||
reports caret px, so we can assert it against expected values in tests.
|
||
- **Channel model + `-race`** (Phase 0) — **resolved** in `58725a5` (§13);
|
||
`-race` is green and is now a standing regression gate.
|
||
- **Chunked buffer at 10 MB** (Phase 3) — **resolved**: opens a real 10 MB file
|
||
in ~121 ms, renders and scrolls smoothly, ~150 MB PSS at steady state. The 50 MB
|
||
limit is set from this measurement (§5 Phase 3, commit `c79c142`).
|
||
- **Chunked buffer fixed-slot drift** (Phase 3) — **resolved**: the buffer was
|
||
rewritten off the fixed `[i*chunkSize, (i+1)*chunkSize)` slot model. It now
|
||
holds an ordered `[][]byte` chunk slice with **prefix-sum byte offsets** and
|
||
full-loads in-range files on open (no lazy loading, no eviction), so byte↔chunk
|
||
mapping stays correct after length-changing edits. A rope is no longer needed
|
||
for the 50 MB target.
|
||
- **Whole-file shaper memory leak** (Phase 3) — **resolved**: `VisibleByteRange`
|
||
no longer falls back to `end=fileLen` via the window-only `VisualLineStarts`;
|
||
the range is always bounded by the real-line `LineIndex`. Memory is now stable
|
||
and scales ~linearly with file size (commit `c79c142`).
|
||
- **AOSP keyboard** is a proxy for real IMEs (Gboard, etc.); final swipe/autocorrect
|
||
sign-off needs a real device with a real IME.
|
||
- **Viewport opens at EOF** (Phase 2) — **resolved**: the opening tap/scroll is
|
||
swallowed for ~300 ms via a `justOpenedAt` window and `ScrollOffset` is
|
||
re-clamped in `EditorLayout` as a safety net (commit `3460ef3`).
|
||
|
||
## 12. Why v1 (widget rebuild) is now a fallback, not the plan
|
||
v1 was written against a stale snapshot (May 31) and concluded "delete
|
||
`internal/ui`, build on `widget.Editor`." Two later-verified facts changed that:
|
||
(a) the live repo already solved the two hardest parts a rebuild would discard
|
||
(real FS, APK, and the chunked buffer that `widget.Editor` cannot do), and (b) the
|
||
Android IME is reachable from the op layer with only the §4 gap — so the "free IME"
|
||
that motivated the rebuild is actually ~4 small wirings away on the existing code.
|
||
**Path A (widget rebuild) remains the fallback if the Phase 1 IME wiring proves
|
||
fragile on-device** (e.g., the caret/snippet plumbing drags in a long tail of
|
||
cursor-positioning bugs that are cheaper to not own). The emulator (Phase 2) is the
|
||
instrument that makes that call.
|
||
|
||
## 13. Single-owner state refactor (`58725a5`)
|
||
|
||
`go test -race ./...` was red. The races were architectural, not incidental —
|
||
several violated `doc/architecture.md §1` (logic goroutine = sole owner of
|
||
mutable state). Fixes, in order of impact:
|
||
|
||
1. **Frame is the only cross-goroutine state carrier.** `editor.Frame` now
|
||
carries `Elems`, `Scale`, `FocusedElementID`, and `Query`. The main goroutine
|
||
reads *only* the frame-receiver-stored snapshot (under its mutex); it no
|
||
longer calls `logic.State()` for scale/focus/search. `Renderer.Draw` takes
|
||
the scale as a parameter; the `ScaleProvider` indirection is gone.
|
||
2. **Gio-mutable widgets are main-owned.** `browser.BrowserState.SearchEditor`
|
||
(a `widget.Editor`) was removed. Main owns the search `widget.Editor` and
|
||
forwards its text via `SearchQueryChan`; the logic stores the result in
|
||
`Browser.Query`. More generally, `Renderer` now owns all `widget.Editor`
|
||
instances (registered by element ID) because Gio mutates them during draw.
|
||
3. **Autosave is owner-mediated.** The 1 s debounce timer goroutine no longer
|
||
reads editor state; it sends a `struct{}` token on `autosaveChan` and the
|
||
owner reconstructs content and dispatches the write.
|
||
4. **Shutdown is ordered and waitable.** `Shutdown()` = `Done()` →
|
||
`WaitForExit()` → `FlushAll()` → `workerPool.Stop()`, so `FlushAll` (lock-free)
|
||
only ever runs after the owner has exited or from the owner itself.
|
||
5. **Tests inspect via the owner.** `Harness.Inspect/WithState/FileLoaded/
|
||
FullContent/CursorPosition` (and the in-package `withState`/`l.Inspect`
|
||
helpers) execute callbacks on the logic goroutine. `Harness.Run` now panics
|
||
if called twice — `TestTypeAtStartOfBuffer` and
|
||
`TestEditorClickToMoveCursorWithScroll` had a *double `Run()`* (two logic
|
||
loops on one state) that was the largest race source.
|
||
6. **Test-only flakes fixed:** `TestWorkerPool_PriorityPreemption` was rewritten
|
||
deterministically (gate task holds the worker while both priorities queue —
|
||
the old version raced the worker's task pickup; Go's `select` is random when
|
||
both channels are ready, so the old ordering guarantee was unimplementable as
|
||
written). `TestLazyLoadingLargeDirectory` timeout raised 2 s → 15 s (a 10k
|
||
entry index build under `-race` exceeds 2 s).
|
||
|
||
Residual (accepted) invariants:
|
||
- `editor.TheState` is a global used by package-level mutators (`GoToEditor`,
|
||
`OpenFile`, `HandleInsert`, …); it is only safe to call them from the owner
|
||
goroutine (input-handler closures run on the owner via `SendInput`).
|
||
- `Logic.FlushAll` is lock-free by design; it must not run concurrently with
|
||
`Run` (guaranteed by the `Shutdown` order).
|