From 21bd9fe02e2cb0fa2ecf039f467297c53c58dc04 Mon Sep 17 00:00:00 2001 From: Greg Pomerantz Date: Sun, 16 Aug 2026 01:33:39 -0400 Subject: [PATCH] Doc: mark Phase 0 done; record single-owner refactor and vision status --- doc/development_plan.md | 76 +++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 15 deletions(-) diff --git a/doc/development_plan.md b/doc/development_plan.md index cb8cdc7..b9cce04 100644 --- a/doc/development_plan.md +++ b/doc/development_plan.md @@ -49,14 +49,15 @@ Status: v2, 2026-08-16. Written against the **live** repo `/home/gmp/pad` ## 3. Current state of the live repo (post housekeeping, 2026-08-16) -- `go build ./...` green. **`go test ./...` now green** (was red: `internal/editor` - and `internal/test/e2e` failed to build after `NewLogic` gained a `path` and - `openfunc` parameter). Commit `03fb638` fixed the harness + 5 call sites; the e2e - harness now uses a no-op `openfunc` (matching `impl_other.go`, whose real - `OpenFile` is a no-op off-Android). -- Uncommitted (yours, left untouched): JNI/Termux bridge, chunk-buffer + logic - changes. **Recommend committing these** so the working tree is a clean baseline - before the IME work. +- `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 @@ -94,10 +95,10 @@ autocorrect produce clean, non-duplicated text. ## 5. Phases (Path B) -### Phase 0 — clean baseline (small) -1. Commit the uncommitted JNI/chunk-buffer work. +### 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). + fix any found). — `58725a5`. Details in §13. ### Phase 1 — complete the IME (§4) Implement items 1–4, with **Go unit tests** for item 3 (range-replace on the @@ -186,9 +187,12 @@ 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; agent has NO vision) +### 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: +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 @@ -214,8 +218,8 @@ within ~1 s. Repo: `go test -race ./...` green. - **§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): the existing goroutine/channel design may - have latent races; running `-race` before IME work avoids conflating them. +- **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. - **AOSP keyboard** is a proxy for real IMEs (Gboard, etc.); final swipe/autocorrect @@ -232,3 +236,45 @@ that motivated the rebuild is actually ~4 small wirings away on the existing cod 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).