Pad/doc/development_plan.md
Greg Pomerantz 4cfabec7a4 Doc: record Phase 2 on-device IME validation (PASSING) + 3 bugs found
- IME commit path validated on emulator (Gboard/API 35): single-char,
  multi-char (human cadence), deletion (deleteSurroundingText->EditEvent),
  and Unicode all commit correctly with cursor in sync; autosave persists.
- Build recipe scripted (gogio + apktool MANAGE_EXTERNAL_STORAGE + apksigner).
- Bugs: (1) open-file crash via Termux bridge -> fixed (c9c0d47); (2) viewport
  opens at EOF because the opening tap leaks into the editor (open, Phase 3);
  (3) rapid synthetic IME commits desync via per-frame snippet re-push (edge).
2026-08-16 09:45:51 -04:00

362 lines
22 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Development Plan: reach a lean, usable Android text editor
Status: v2, 2026-08-16 (Phase 2 on-device IME validation complete). Written
against the **live** repo `/home/gmp/pad`. v1 (the widget-rebuild plan) is
superseded — see §12 for why. Phases 02 done; Phase 3 (large-file + the
viewport-on-open bug) is next.
## 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, gfxinfo, 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 `/tmp/build_pad.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):** `adb shell input text
"…"` fires per-char commits faster than a frame; the per-frame `SnippetCmd`
re-push resets the IME's cursor, so fast commits interleave/corrupt
(observed `" IME123"``E123IM…`). Real typing (human cadence) stays in sync
(validated above). Robustness note for Phase 3: only re-push the snippet when
it actually changes, or suppress re-push mid-composition.
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
1. Open a real **10 MB** file via the chunked buffer; verify smooth scroll + edit
and measure RAM on-device.
2. Set the documented limit from that measurement; guard larger files with a
clear "too large to edit" state (browser can still list/preview).
### 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
Rewrite `doc/architecture.md` to match reality; amend `spec.md` per §7; README
(build/install recipe); in-repo device test checklist.
## 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. If the chunked buffer proves out (likely), 10 MB+ editing is in
reach without the 0.5 GB/MB wall that kills `widget.Editor`.
## 7. Spec deltas (to write into spec.md)
1. "No practical limits / file never fully in memory" → backed by the chunked
buffer; add a measured hard limit from Phase 3.
2. Undo across sessions → dropped (in-session only; chunked buffer does not keep
full-document undo copies).
3. Add: IME/swipe/autocorrect support (now explicit, delivered in Phase 1).
4. Add: external-change detection as the conflict policy (mtime compare on open
+ on resume; changed+clean → reload, changed+dirty → Keep/Reload prompt).
## 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, gfxinfo, 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. **`dumpsys gfxinfo <pkg>`** — per-frame timing/jank for the 60 fps claims.
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): unproven on-device; the measurement is
the gate for the size claim.
- **Chunked buffer fixed-slot drift** (Phase 3, discovered during IME work): the
buffer models chunk *i* as bytes `[i*chunkSize, (i+1)*chunkSize)`. That holds
for a freshly loaded file and for read-only display, but a **length-changing
edit** (insert/delete) shifts the bytes after the edit point, so the
fixed-slot offsets no longer line up for any *not-yet-loaded* tail chunk. In
practice: (a) files fully in memory are fine (all chunks loaded, `FullContent`
concatenates by index); (b) the caret region is always loaded, so **IME commits
at the caret are correct**; (c) a *large* file with an *unloaded* tail that is
then edited and scrolled into is where it breaks (lazy `loadChunk` reads stale
disk, since the file is only rewritten whole on save). A proper fix is a rope
or an offset-index structure. Phase 3 must either avoid this case (cap edits
to loaded region / flush before loading a shifted tail) or replace the slot
model. Until then the honest size limit should assume the file is small enough
to stay fully in memory (the ~50 MB target is borderline; see §6).
- **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, open): the opening tap leaks into the editor
and scrolls a short file past its content → blank editor until you scroll to top.
Blocks a clean first impression; fix in Phase 3 (ignore the opening tap / clamp
scroll for files that fit the viewport). See §5 Phase 2 bugs.
## 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).