From 03fb63863c5ab50fd25ff24efa15d1bf37afd793 Mon Sep 17 00:00:00 2001 From: Greg Pomerantz Date: Sat, 15 Aug 2026 22:18:04 -0400 Subject: [PATCH] Fix test build after NewLogic signature change; add development plan draft NewLogic now takes (FileSystem, path, openfunc); update the e2e harness and editor tests accordingly. The harness uses a no-op openfunc, matching impl_other.go (the real app's OpenFile is a platform bridge, no-op off Android). Also adds doc/development_plan.md (draft, with corrections note re: the live repo state). --- doc/development_plan.md | 280 ++++++++++++++++++++++++++++ internal/editor/auto_save_test.go | 2 +- internal/editor/e2e_test.go | 6 +- internal/editor/integration_test.go | 2 +- internal/test/e2e/harness.go | 2 +- 5 files changed, 286 insertions(+), 6 deletions(-) create mode 100644 doc/development_plan.md diff --git a/doc/development_plan.md b/doc/development_plan.md new file mode 100644 index 0000000..eecec1e --- /dev/null +++ b/doc/development_plan.md @@ -0,0 +1,280 @@ +# Development Plan: from skeleton to lean, usable app + +Status: DRAFT — written 2026-08-15 against the stale Backup snapshot (May 31). +The live repo (~/pad) is 47 commits ahead; see the "Corrections" note at the +bottom before acting on anything in this document. + +## 1. Decision summary + +- **Tooling:** delete the hand-rolled `internal/ui` entirely. Build on Gio's native + widgets: `widget.Editor`, `widget.List`, `layout.List`, `widget.Button`, + `widget.RichText`. This removes the entire bug class the project has been + fighting (positioning/scroll/clipping/gesture) and is the *only* path to Android + IME input (swipe typing, autocorrect) without writing Android InputConnection + code ourselves. +- **Concurrency:** one main goroutine owns all app state and does all drawing. + File I/O runs in background goroutines; results come back over a channel and are + applied on the main goroutine. No logic goroutine, no unbuffered + display-channels, no globals (`editor.TheState`, `browser.currentBrowserState`, + `ui.OpenFile` all die). +- **Target size limit:** 50 MB files — but stock `widget.Editor` cannot do this + (evidence below). The plan stages this as an explicit decision point with three + options; recommendation: ship with stock editor at a documented limit, and do + windowed shaping as a separate, isolated project if 50 MB is still required. + +## 2. Evidence (verified against gioui.org v0.9.0 sources) + +1. **IME works through `widget.Editor` only.** `GioView.java` implements + `onCreateInputConnection` → `GioInputConnection` (commitText / + setComposingText / setComposingRegion); `app/ime.go` exposes + `editorState.Replace`; `widget.Editor` pushes `key.SnippetCmd` every frame. + Our current custom UI never receives IME input at all. +2. **The IME snippet is selection-scoped** (`updateSnippet` reads only the + selected range; empty selection → empty snippet at caret). Consequence: + windowing the editor's backing text does *not* break IME behavior. +3. **`widget.Editor` is not virtualized.** `textView.layoutText` shapes the + *entire* document on every invalidation (each keystroke, IME commit, wrap + toggle, width change — *not* on scroll; scroll only moves a clip) and builds + an in-memory `glyphIndex` of **~200 bytes per rune** (`combinedPos` + + `text.Glyph` per rune). +4. **Measured on this desktop (shaper.Layout + full glyph iteration, = the + per-keystroke cost of `widget.Editor`):** + + | text size | time/keystroke | peak RAM | + |---|---|---| + | 1 MB | 17 ms | ~0.32 GB | + | 2 MB | ~35 ms (extrapolated) | ~0.9 GB | + | 3 MB | — | ~1.7 GB (measured, incl. GC churn) | + | 4 MB | — | OOM-killed the benchmark host | + + Planning figure: **~0.5 GB of RAM per 1 MB of text** (editor index + + shaper store + buffer + undo copies, with reallocation churn). Android app + memory ceilings (lmkd) are typically ~1.5–2 GB on 8 GB phones. + +Therefore: comfortable editing up to **~2 MB**, a hard guard at **5 MB** +(≈ 2.5 GB — survivable on most phones, heavy), 10 MB ≈ 5 GB (OOM), 50 MB is +not deliverable with stock `widget.Editor`. 10 MB editing requires windowed +shaping (fork-level work on Gio). See §6. + +## 3. What gets deleted + +| Item | Where | Why | +|---|---|---| +| Whole custom toolkit | `internal/ui/` (element.go, render.go, layout.go, unit.go, icons/) ~1,500 L | replaced by Gio widgets | +| Worker pool + mock FS service | `internal/io/pool/worker_pool.go`, `mock/` (429 L) | replaced by plain background goroutines | +| 10 of 11 task types | `internal/io/pool/task.go` | only Read/Write are ever used, as plain functions | +| On-disk JSON index cache | `internal/browser/index.go` `.pad/indices/` machinery | cache-in-indexed-dir is the fragile design behind the failing mtime test; a sync directory needs no cache (re-scan is ms) | +| Dead browser state | `state.go`: `VisibleCount` (never set → list rendered 0 rows), `RowFilenames`, `Pages`, `Page`, `SortIndex`, `SortMode` (duplicate of browser's), `PaginationState`, `PageSize` | replaced by `widget.List` + real filenames | +| Untyped handlers | `browser/handlers.go`, `editor/logic.go` `func(any)` closures + globals | replaced by direct calls on the main goroutine | +| e2e harness | `internal/test/e2e/` | written against the dead architecture; replaced by widget-level tests after Phase 1 | +| Stale docs | `element_model.md`, `layout_rendering.md`, `touch.md`, `conflict_resolution.md`, `browser_implementation_plan.md` | describe deleted systems; `architecture.md` rewritten last | + +**Kept (ported, not rewritten):** `browser/types.go` (Entry), `sort.go` + tests, +`search.go` + tests, directory scan from `index.go` (minus cache), the wrap / +autosave / per-file-offset concepts from `editor/state.go`. + +## 4. Target architecture (single diagram, no channels except I/O) + +``` +main goroutine (Gio frame loop, owns ALL state) + app.State { + page: browser | editor + dir: string // current browser directory + entries: []browser.Entry // sorted+filtered, rebuilt on demand + sort: browser.SortMode + query: string + file: openFile{path, modTime, wrap, editor widget.Editor, dirty, lastEdit} + restore: state.RestoreData // persisted JSON + } + on FrameEvent: + - handle I/O results from chan result + - draw current page + background goroutines (one per op): + os.ReadFile / os.WriteFile / dir scan → send result on chan +``` + +- **Browser page:** top bar (path, sort menu) + search field + + `layout.List` over `entries` (virtualized; 1,000-file directory is a + non-issue). Tap → start background read → on result, switch to editor page. +- **Editor page:** `widget.Editor` bound to the file's text; toolbar (filename, + wrap toggle, close). Autosave: `dirty` + 1 s debounce in the frame loop, + background write, status shows saved/dirty. Undo/redo in-session: built into + `widget.Editor` (unexported history, triggered by normal editing) — no work. +- **State restore** (spec: survive close/reopen): small JSON in the app data + dir: `{openFile, wrap, autosave, offsets: {path: {line, col}}}`. On open: + `SetText` + `SetCaret(line, col)`. `SetCaret` auto-scrolls to the caret. + Offsets are rune offsets — invalid if the file changed; validate against + persisted mtime and fall back to top on mismatch. +- **External change detection:** on open and on app resume, compare mtime to + the opened modTime; if changed and we're not dirty → reload; if dirty → + one-line banner with Keep / Reload (no full conflict UI in v1). +- **File-size guard:** on open, if `size > limit` → show "file too large to + edit in v1" (limit per §6 decision). +- **Testability:** state transitions (open file, sort, search, autosave due, + external-change decision) are plain functions on `app.State` → directly + unit-testable. Device behavior covered by manual checklist + `-race` in CI. + +## 5. Phases + +### Phase 0 — deletion-only (no new features) +1. Delete everything in §3. +2. `cmd/pad` keeps an `app.Window` with a placeholder screen; build green. +3. Keep passing tests by porting sort/search tests to the trimmed `browser` + package; delete the on-disk-cache test (feature deleted) — this removes the + flaky `TestBuildIndex_CacheInvalidatedOnMtimeChange` by deleting the cache. + +**Done when:** `go build ./... && go vet ./... && go test -race ./...` green; +repo ≤ ~2,500 lines of Go; no `func(any)`, no package-level mutable state, +no channels except (added in Phase 1) the I/O result channel. + +### Phase 1 — browser +1. `layout.List` browser page: rows = name + modified time; directories + distinguished; tap on dir descends, breadcrumb back; tap on file opens it + (Phase 2 stub: toast "no editor yet"). +2. Port sort (name/modified, asc/desc) + search filter onto the page. +3. Background dir-scan for large directories; version counter to drop stale + results. + +**Done when:** on-device: browse a real 1,000+ file directory at full frame +rate; sort and search correct (unit tests); no data races. + +### Phase 2 — editor +1. Open file → background read → `widget.Editor` with `SetText`; toolbar with + wrap toggle (maps to `WrapPolicy`) and close (back to browser). +2. Autosave: debounce, background write, dirty indicator. +3. State restore + per-file offset memory + external-change check (§4). +4. IME verification on device: swipe typing, autocorrect, multi-line commits, + caret jump after commit. + +**Done when:** edit + close + reopen restores file and caret; kill app, +reopen, same; external modification handled per §4; `-race` clean. + +### Phase 3 — the 50 MB decision (see §6) +Either (A) document the limit and ship, or (C) build and integrate the +windowed editor. (B, full chunked streaming, is out of scope — it implies a +custom editor and forfeits the IME advantage.) + +### Phase 4 — hardening +1. Rewrite `doc/architecture.md` to match reality; amend `spec.md` per §7. +2. `README.md`: what it is, how to build/install (Go 1.24, gioui.org, NDK). +3. Device test checklist in-repo. + +## 6. The 50 MB decision (explicit fork) + +| Option | What | IME | Effort | Outcome | +|---|---|---|---|---| +| **A. Accept a documented limit** (2 MB soft warn, 5 MB hard guard — §2.4) | nothing more than Phase 2 | full | done | the large majority of real text files in a sync dir; honest limit in UI | +| **C. Windowed editor** | isolated project: shape only viewport ± window; `glyphIndex` covers the window; offset map rune→window; scroll triggers re-window; undo history kept as per-operation edits, not full copies | full (snippet is selection-scoped, verified) | large — touches Gio internals (`textView`, `Editor`), upstreamable but expect divergence | 10 MB+ editing at 60 fps, *if* the PoC holds | +| B. Chunked streaming editor (spec's original) | build our own editor on raw ops | **lost** (no InputConnection) | largest | rejected: forfeits the core reason for moving to native | + +**Recommendation: A now, C later as its own branch.** Reasoning: A delivers a +usable app; the Phase 0–2 work is identical under either choice, so nothing is +locked in. C is only worth starting if, after real usage, the 5 MB guard bites +(i.e. we genuinely need to *edit* larger files). If C is started, gate it +behind a PoC first: windowed shaping of a 10 MB file must sustain 60 fps +scroll + sub-100 ms keystroke on the target phone before any integration work. + +## 7. Spec deltas (to write into spec.md after Phase 2) + +1. "No practical limits / file never fully in memory" → "files up to N MB + (measured); larger files openable in browser, not editable." +2. Undo across sessions → dropped (in-session undo/redo is provided by the + editor widget). +3. Add: IME/swipe/autocorrect support (was an implicit requirement, now + explicit and delivered). +4. Add: external-change detection behavior (§4) as the conflict policy, + replacing `conflict_resolution.md`. + +## 8. Non-goals (v1) + +- File-system watcher / live browser refresh (re-scan on return to browser). +- Multi-file views, tabs, split. +- Syntax highlighting, search-in-file, diff/merge UI. +- Desktop/other platforms (Android-first; window sized per existing + `unit.Dp(390) × unit.Dp(844)`). + +## 9. Testing strategy (replaces `internal/test/e2e`) + +The old e2e harness is deleted: it existed to test the channel/frame +choreography, which no longer exists. E2E regression still works, in three +layers: + +**Layer 1 — Go unit tests on pure state functions (the bulk).** The design +keeps every behavior as a plain function on `app.State` (no goroutines, no +widgets), so the regression surface is directly testable on a host in +milliseconds: +- sort (all modes) / search / directory scan — port the existing + `browser` package tests; +- `applyReadResult` / `applyWriteResult` / `applyScanResult`, including + **stale/out-of-order results** (version counters) — the exact race the old + channel design was prone to, now a synchronous decision under test; +- autosave debounce and dirty-flag decisions (clock injected, no real timers); +- restore JSON encode/decode; offset validation against mtime; external-change + decision table (changed+clean → reload; changed+dirty → prompt; + unchanged → open); +- file-size guard (2 MB warn / 5 MB reject). + +CI gate: `go build && go vet && go test -race ./...` on every commit. + +**Layer 2 — UI code thin by construction.** Gio has no headless/test window +(verified in v0.9 and v0.10), so widget-level Go tests are not possible. +Mitigation is structural, not test-based: draw functions are ~30-line wirings +of `layout.List` / `widget.Editor` over `app.State`, and all positioning math +lives inside Gio. The bug class the old UI had (positioning/scroll/clipping) +is *deleted*, not covered by tests. + +**Layer 3 — scripted on-device e2e: an in-app selftest.** A debug-build flag +(`pad --selftest`, or launched via `am start` with an extra) drives the **real +state machine through the real draw path**: +1. create a fixture tree under the app's data dir; +2. browser: assert entry count/names in the rendered state; +3. open a file: assert editor text == fixture content; +4. force the autosave deadline: assert the file on disk is rewritten; +5. save restore state, simulate reopen: assert file + caret offset restored; +6. touch the file's mtime externally, reopen: assert the reload-prompt + decision fires. +Each step logs PASS/FAIL; a summary is written to a file the host pulls via +adb and asserts on. Deterministic, seconds long, runnable in CI against a +farm/one attached device. + +**Manual-only (unavoidable):** swipe typing, autocorrect, caret jump after an +IME commit — `adb input text` bypasses the IME entirely, so these cannot be +scripted by any tool. They live in a short checklist in the repo, run before +releases, alongside the §9/DoD journey. + +## 10. Definition of done for the whole effort + +On a real Android phone: launch → previous file and caret restored → browse a +large directory smoothly → open a real file → swipe-type with autocorrect → +close and reopen, caret where you left it → modify file externally, get the +reload prompt → autosave lands on disk within ~1 s. Repo: ≤ ~3,000 lines of +Go, one channel, zero globals, `go test -race ./...` green. + +--- + +## Corrections (live repo, ~/pad, as of 2026-08-15) + +The live repo has advanced far beyond the snapshot this document analyzed: +real filesystem (`internal/io/pool/real/`), a built Android APK (JNI + +permissions), a working editor (click-to-cursor, Enter/Backspace, autosave), +and a **chunked buffer** (`internal/editor/chunked_buffer.go`) implementing the +spec's "file never fully in memory" approach that this plan assumed would +require fork-level work. Concretely: + +- `VisibleCount` is now assigned (`internal/editor/state.go:174`); the + "browser renders 0 rows" finding is stale. +- The IME path is half-wired at the raw-op layer: `cmd/pad/main.go` consumes + `key.EditEvent`/`key.SnippetEvent` and fires `key.SoftKeyboardCmd`, but + **nothing creates the window-level editor-state op** (`app.EditorState`), so + IME *text commits* (swipe, autocorrect) likely still have nowhere to land; + raw key events (Enter/Backspace) work. Completing this wiring is the + pivotal experiment (see emulator decision below). +- The live test suite is RED: `internal/editor` and `internal/test/e2e` fail + to build after `NewLogic`'s signature changed to + `(pool.FileSystem, string, func(string))`. +- The A/B fork is now: **(A)** this plan's widget rebuild, vs **(B)** finish + the live path (complete IME wiring, stabilize chunked buffer, fix tests). + The emulator swipe-typing experiment decides. + +Decision pending: Path A vs Path B. Until decided, this document is a +reference for the widget-rebuild option, not an active plan. diff --git a/internal/editor/auto_save_test.go b/internal/editor/auto_save_test.go index b8b1992..3b8aa9b 100644 --- a/internal/editor/auto_save_test.go +++ b/internal/editor/auto_save_test.go @@ -33,7 +33,7 @@ func TestWriteFailureTracking(t *testing.T) { // TestAutoSave_RetryFails verifies that failed writes trigger retry mechanism. func TestAutoSave_RetryFails(t *testing.T) { // Setup: New Logic, set mock FS to fail - l := NewLogic(nil) + l := NewLogic(nil, "/", func(string) {}) // Start a goroutine to drain frameChan to prevent deadlocks go func() { diff --git a/internal/editor/e2e_test.go b/internal/editor/e2e_test.go index 74c2022..c341c7b 100644 --- a/internal/editor/e2e_test.go +++ b/internal/editor/e2e_test.go @@ -16,7 +16,7 @@ func TestAutoSaveE2E(t *testing.T) { initialContent := "Hello" mockFS.AddFile(filename, []byte(initialContent), time.Now()) - l := NewLogic(mockFS) + l := NewLogic(mockFS, "/", func(string) {}) go l.Run() defer l.Done() @@ -93,7 +93,7 @@ func TestLargeFileChunkBoundary(t *testing.T) { } mockFS.AddFile(filename, initialContent, time.Now()) - l := NewLogic(mockFS) + l := NewLogic(mockFS, "/", func(string) {}) go l.Run() defer l.Done() @@ -232,7 +232,7 @@ func TestFlushOnExitE2E(t *testing.T) { initialContent := "Hello" mockFS.AddFile(filename, []byte(initialContent), time.Now()) - l := NewLogic(mockFS) + l := NewLogic(mockFS, "/", func(string) {}) go l.Run() defer l.Done() diff --git a/internal/editor/integration_test.go b/internal/editor/integration_test.go index fb5cd51..da1b420 100644 --- a/internal/editor/integration_test.go +++ b/internal/editor/integration_test.go @@ -14,7 +14,7 @@ func TestOpenFileIntegration(t *testing.T) { content := "Hello World" mockFS.AddFile(filename, []byte(content), time.Now()) - l := NewLogic(mockFS) + l := NewLogic(mockFS, "/", func(string) {}) go l.Run() defer l.Done() diff --git a/internal/test/e2e/harness.go b/internal/test/e2e/harness.go index cd590b2..25e571d 100644 --- a/internal/test/e2e/harness.go +++ b/internal/test/e2e/harness.go @@ -23,7 +23,7 @@ type HarnessOption func(*Harness) // NewHarness creates a test harness with the given options. func NewHarness(opts ...HarnessOption) *Harness { h := &Harness{ - logic: editor.NewLogic(nil), + logic: editor.NewLogic(nil, "/", func(string) {}), // no-op openfunc (matches impl_other.go) capture: NewFrameCapture(), frameReceiverDone: make(chan struct{}), }