462 lines
58 KiB
Markdown
462 lines
58 KiB
Markdown
|
|
# BadNote Phase 1 — Smooth Editing Core + Infinite Annotation Space
|
|||
|
|
|
|||
|
|
**Status:** PLAN (ralplan consensus — Architect APPROVE-WITH-MUST-FIXES applied 2026-06-21; pending Critic + M1 spike verification)
|
|||
|
|
**Date:** 2026-06-21
|
|||
|
|
**Mode:** DELIBERATE (high-risk: full PDF-backend swap + new input architecture + 60fps target)
|
|||
|
|
**Owner plan file:** `docs/plans/2026-06-21-badnote-phase1.md`
|
|||
|
|
|
|||
|
|
> Grounding note: this plan was written after reading the actual sources
|
|||
|
|
> (`pdf_annotator_screen.dart`, `ink_canvas.dart`, `pdf_annotation_layer.dart`,
|
|||
|
|
> `database_service.dart`, `undo_manager.dart`, `pdf_service.dart`,
|
|||
|
|
> `split_view_screen.dart`, `stroke_rasterizer.dart`, `stroke_stabilizer.dart`,
|
|||
|
|
> models) and after fetching pdfrx **v2.4.4** API facts from Context7 / GitHub.
|
|||
|
|
>
|
|||
|
|
> **pdfrx is NOT yet installed in this repo's pub-cache**, so every pdfrx API
|
|||
|
|
> claim below is **doc-derived inference, not compile-checked.** Such claims are
|
|||
|
|
> tagged **[VERIFY-IN-M1]** (doc-derived; must be source-pinned during the
|
|||
|
|
> Milestone-1 spike) or **[UNCONFIRMED]** (doc could not even establish an
|
|||
|
|
> inference). The very first build task (M1) installs pdfrx and converts every
|
|||
|
|
> **[VERIFY-IN-M1]** and **[UNCONFIRMED]** tag into a source-pinned,
|
|||
|
|
> compile-checked fact. No tag in this document should be read as "already
|
|||
|
|
> verified in code."
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 1. Goals / Non-goals
|
|||
|
|
|
|||
|
|
### 1.1 Phase 1 Goals (the P1 boundary)
|
|||
|
|
1. **G1 — pdfrx render core.** Replace `SfPdfViewer` + Stack-overlay with **pdfrx** (`PdfViewer.file`). Continuous vertical scroll, pinch-zoom + pan, tiled hi-res rendering. Re-home page management (delete/insert/rotate), bookmarks, thumbnails, and export onto pdfrx's page-coordinate model.
|
|||
|
|
2. **G2 — High-performance inking engine.** Static layer baked into a cached `ui.Picture` rebuilt only on a per-host `revision` bump (O(1) `shouldRepaint`); live layer paints only the in-progress stroke; per-page `RepaintBoundary`; zero per-frame object allocation for committed strokes.
|
|||
|
|
3. **G3 — Pen-first input arbitration.** Stylus draws; inverted-stylus erases; touch scrolls/pinches via pdfrx; palm rejection (ignore touch while stylus down); mouse behaves per active mode. Implemented as an explicit state machine.
|
|||
|
|
4. **G4 — Infinite annotation side-canvas (toggle).** A blank, vertically-infinite ink board sharing the **same** ink engine through a `CoordinateSpaceHost` abstraction. Independent scroll/zoom from the PDF pane.
|
|||
|
|
5. **G5 — OneNote-like editable text boxes.** Real in-place editable text (replacing the modal-dialog text tool), placeable/movable, stored as plain text in host coordinates.
|
|||
|
|
6. **G6 — Keyboard/mouse modes.** A `Browse` mode (wheel scroll, Ctrl+wheel zoom, space/middle-drag pan, no accidental ink) and a `Type` mode (focus/edit text boxes). Tool number shortcuts (1=pen,2=eraser,…). Keep existing Ctrl+Z/Y/F/S.
|
|||
|
|
|
|||
|
|
### 1.2 Phase 1 Non-goals (explicitly out of scope; context only)
|
|||
|
|
- **N1** Search / review UI and **handwriting formula OCR → searchable** (P2). P1 *stores* text-box plain text so P2 can index it, but builds no search UI.
|
|||
|
|
- **N2** Server sync, `llm_wiki`, VLM/LLM note refinement (P3).
|
|||
|
|
- **N3** Ink-recognition CAS assisted computation (P4).
|
|||
|
|
- **N4** PPT/image-doc ingestion. P1 targets PDF only (the `docType` column stays but only `pdf` is exercised).
|
|||
|
|
- **N5** Cross-device migration of existing data. **Existing data may be reset.** No migration code; a clean schema is acceptable (see §5).
|
|||
|
|
- **N6** Existing text-line ONNX OCR pipeline (`StrokeRasterizer`, `ctc_decoder`) is *retained as-is* and not wired into the new editor in P1.
|
|||
|
|
|
|||
|
|
### 1.3 P2–P4 forward-compatibility constraints (so P1 doesn't paint us into a corner)
|
|||
|
|
- Text boxes and strokes are stored with stable IDs + page/board coordinates so P2 search can reference them.
|
|||
|
|
- The `CoordinateSpaceHost` abstraction (G4) is the seam P4's CAS overlay will also attach to.
|
|||
|
|
- Persistence keeps a per-page text content string addressable by `(documentId, hostId, pageIndex)` for future FTS indexing.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 2. Architecture Overview
|
|||
|
|
|
|||
|
|
### 2.1 Coordinate model — single source of truth
|
|||
|
|
**Truth = host content coordinates.** Two host kinds:
|
|||
|
|
- **PDF page host:** coordinates are **normalized [0,1]** relative to that page's unrotated content box. Stored per `(documentId, pageIndex)`.
|
|||
|
|
- **Board host:** coordinates are **absolute logical units** on an unbounded canvas (origin top-left, +y down), independent of zoom.
|
|||
|
|
|
|||
|
|
Rendering maps truth → screen at paint time **via a canvas transform**, never by allocating transformed point objects (this kills the `_scaledStrokes` deep-copy and the `pdf_annotation_layer` rotation re-mapping).
|
|||
|
|
|
|||
|
|
For a PDF page, the device-space rect is supplied by pdfrx:
|
|||
|
|
- `pageOverlaysBuilder(context, pageRect, page)` gives `pageRect` **already scaled to current zoom** in viewer-local coords **[VERIFY-IN-M1]**.
|
|||
|
|
- **Correction (do NOT overstate "no manual zoom math"):** `pageOverlaysBuilder` returns *raw widgets*, and per the pdfrx docs in-page offsets normally still need `* controller.currentZoom`. The `canvas.scale(size.width, size.height)` trick is correct **only if** the ink `CustomPaint` is laid out at **exactly `pageRect.size`** — otherwise `size` ≠ the zoomed page box and strokes mis-scale. We therefore require: wrap `PageAnnotationLayer` in `SizedBox.fromSize(size: pageRect.size)` (equivalently `Positioned.fromRect(rect: Offset.zero & pageRect.size, ...)` inside the page-local stack) so the `CustomPaint`'s `size` *is* the zoomed page box, then `canvas.scale(size.width, size.height)` maps normalized [0,1] → zoomed pixels. With that constraint satisfied, the **point positions** follow scroll+zoom without per-point math; the constraint itself is the "zoom math," made explicit and verified once. **M1 assertion:** a stroke stored at normalized `(0.5, 0.5)` renders at the visual page center across **3 zoom levels** (e.g. fit, 2×, 4×) — golden/coordinate check, not eyeballed.
|
|||
|
|
- Page rotation: pdfrx renders the rotated page and reports `pageRect` for the rotated box; `PdfRect.toRect(page:, scaledPageSize:)` handles rotation **[VERIFY-IN-M1]**. We therefore store strokes in **unrotated normalized page space** and let pdfrx's reported geometry carry rotation. (This removes the bespoke `_forwardRotate`/`_inverseRotate` math.)
|
|||
|
|
|
|||
|
|
### 2.2 Widget tree (PDF editor, single document)
|
|||
|
|
```
|
|||
|
|
EditorScreen (ConsumerStatefulWidget)
|
|||
|
|
└─ ProviderScope override: editorControllerProvider(documentId)
|
|||
|
|
├─ EditorToolbar (tool/color/width/mode, undo/redo, board toggle)
|
|||
|
|
└─ Body (Row)
|
|||
|
|
├─ [optional] PageThumbnailSidebar (re-homed; renders via pdfrx)
|
|||
|
|
├─ Expanded → EditorPdfPane
|
|||
|
|
│ └─ PdfViewer.file(
|
|||
|
|
│ params: PdfViewerParams(
|
|||
|
|
│ pageOverlaysBuilder: → [ PageAnnotationLayer(pageIndex, page, pageRect) ],
|
|||
|
|
│ viewerOverlayBuilder: → [ InputArbiterOverlay(...) ], // stylus capture
|
|||
|
|
│ panAxis: PanAxis.vertical, // [VERIFY-IN-M1] see note below
|
|||
|
|
│ ...perf params (see §7)
|
|||
|
|
│ ))
|
|||
|
|
└─ [optional, board toggle on] BoardPane
|
|||
|
|
└─ InteractiveViewer(constrained:false)
|
|||
|
|
└─ BoardAnnotationLayer(boardHost) // same ink engine
|
|||
|
|
```
|
|||
|
|
`PageAnnotationLayer` and `BoardAnnotationLayer` are thin adapters over one shared `AnnotationLayer` widget parameterized by a `CoordinateSpaceHost`.
|
|||
|
|
|
|||
|
|
> **[VERIFY-IN-M1] `panAxis`:** `PanAxis.vertical` locks panning to vertical, which is what we want for continuous reading, but it may **block the horizontal component of a pinch-zoom pan** (zooming in then dragging sideways to inspect). M1 must verify whether `PanAxis.free` is required while zoomed (and `vertical` only at fit-width), or whether pdfrx already exempts pinch from the axis lock. Resolve before M3.
|
|||
|
|
|
|||
|
|
### 2.3 Data flow
|
|||
|
|
```
|
|||
|
|
PointerEvent
|
|||
|
|
→ InputArbiter (state machine; classifies device + mode)
|
|||
|
|
├─ stylus/pen-draw → EditorController.beginStroke / extendStroke / commitStroke
|
|||
|
|
├─ inverted-stylus → EditorController.eraseAt
|
|||
|
|
└─ touch/mouse-nav → (not consumed) → pdfrx gesture recognizers
|
|||
|
|
→ EditorController mutates per-host stroke list + bumps host.revision
|
|||
|
|
├─ live stroke held in a ValueNotifier<LiveStroke?> (drives LiveInkPainter only)
|
|||
|
|
└─ on commit: append to committed list, revision++, schedule debounced save
|
|||
|
|
→ AnnotationLayer rebuilds:
|
|||
|
|
├─ StaticInkPainter (revision-gated → rebuilds ui.Picture only on change)
|
|||
|
|
├─ LiveInkPainter (listens to ValueNotifier; repaints current stroke only)
|
|||
|
|
└─ TextBoxLayer (Positioned EditableTextBox widgets)
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
### 2.4 Why this fixes the four root causes
|
|||
|
|
| Root cause (current) | P1 mechanism |
|
|||
|
|
| --- | --- |
|
|||
|
|
| 1. Full repaint + O(n²) live `getStroke` every PointerMove | Static `ui.Picture` cache (committed) + `LiveInkPainter` that incrementally extends a raw polyline and runs `getStroke` **once** at commit (§6.4); `RepaintBoundary` per page. |
|
|||
|
|
| 2. `_scaledStrokes` deep-copies all strokes each build | No copies; paint-time `canvas.scale`/`transform` over stored points. |
|
|||
|
|
| 3. `_loadAllAnnotations` serial per-page DB await on open | Single batched query (`WHERE document_id=?`) → group in memory; lazy-hydrate per page on first paint. |
|
|||
|
|
| 4. Overlay Stack doesn't share viewer transform | Ink lives **inside** `pageOverlaysBuilder` (page coords); with the `CustomPaint` sized to `pageRect.size` (§2.1), point positions track scroll+zoom without per-point math. |
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 3. Component-by-Component Implementation Plan
|
|||
|
|
|
|||
|
|
> File status legend: **[ADD]** new, **[MODIFY]** edit existing, **[DELETE]** remove, **[KEEP]** unchanged.
|
|||
|
|
|
|||
|
|
### 3.1 Coordinate-space host abstraction
|
|||
|
|
- **[ADD] `lib/editor/hosts/coordinate_space_host.dart`**
|
|||
|
|
- `abstract class CoordinateSpaceHost`
|
|||
|
|
- `String get hostId;` (e.g. `"page:3"`, `"board"`)
|
|||
|
|
- `int get pageIndex;` (board → a sentinel, e.g. `-1`)
|
|||
|
|
- `Offset toContent(Offset deviceLocal, Size deviceSize);` — device→truth
|
|||
|
|
- `Offset toDevice(Offset content, Size deviceSize);` — truth→device (for hit-test / text-box placement)
|
|||
|
|
- `void applyContentToCanvas(Canvas canvas, Size deviceSize);` — sets transform so painters draw in truth units
|
|||
|
|
- `class NormalizedPageHost implements CoordinateSpaceHost` — truth ∈ [0,1]; transform = `scale(size.width, size.height)`.
|
|||
|
|
- `class BoardHost implements CoordinateSpaceHost` — truth = absolute logical px; transform = identity (the `InteractiveViewer` supplies pan/zoom).
|
|||
|
|
- **Deps:** none. **Used by:** `AnnotationLayer`, painters, `InputArbiter`, `EditorController`.
|
|||
|
|
|
|||
|
|
### 3.2 Editor state (single source of truth)
|
|||
|
|
- **[ADD] `lib/editor/state/editor_models.dart`**
|
|||
|
|
- `class StrokeStore` — holds `List<InkStroke> committed` + `int revision`; `add()/remove()/replace()` bump `revision`.
|
|||
|
|
- `class HostState` — `{ CoordinateSpaceHost host, StrokeStore strokes, List<TextBoxModel> textBoxes }`. (Undo is **global**, not per-host — see §3.2 undo scope below; a single `UndoManager` lives on `EditorController`, with each action tagged by `hostId`+`pageIndex`.)
|
|||
|
|
- `class LiveStroke` — `{ List<InkPoint> rawPoints, PenTool tool, Color color, double width }` (the in-progress polyline).
|
|||
|
|
- **[ADD] `lib/editor/state/editor_controller.dart`**
|
|||
|
|
- `class EditorController extends ChangeNotifier` (or Riverpod `Notifier`):
|
|||
|
|
- State: `Map<int, HostState> pageHosts`, `HostState? boardHost`, tool/color/width/filled/mode, `ValueNotifier<LiveStroke?> liveStroke`.
|
|||
|
|
- **Store retention vs Picture eviction are DECOUPLED (MAJOR-4 resolution).** The `PictureCache` LRU (§3.3) evicts only the rendered `ui.Picture` — it NEVER drops the `HostState`/`StrokeStore`. A `HostState` is retained in `pageHosts` (not garbage-collected) for any host that has **undo history referencing it OR unsaved/dirty changes**, regardless of whether its Picture is evicted or its overlay is unmounted. `pageHosts` entries may be discarded only when a host has no pending save and no undo/redo entry pointing at it. This guarantees the undo path below always finds a live store.
|
|||
|
|
- `void beginStroke(CoordinateSpaceHost host, InkPoint p)`
|
|||
|
|
- `void extendStroke(InkPoint p)`
|
|||
|
|
- `void commitStroke()` — runs `getStroke` once, appends `InkStroke`, `revision++`, pushes undo, schedules save.
|
|||
|
|
- `void eraseAt(CoordinateSpaceHost host, Offset content, double radius)` — reuses split logic from `ink_canvas._splitStroke` (extracted to a pure util, §3.7).
|
|||
|
|
- **Undo scope under continuous scroll (decided):** use a **single global undo stack across all hosts, ordered by commit time** — NOT per-page. On a continuous viewer the user may ink across several pages without an explicit "page change", so per-page undo stacks feel broken ("Ctrl+Z did nothing" because the active page silently changed). `undo()/redo()` pop the global stack; each entry records its `hostId`+`pageIndex` so the action is reversed on the correct host and (optionally) the viewer scrolls that host into view. The existing per-stroke `UndoManager` semantics are reused, but there is **one** manager keyed globally rather than `Map<int, UndoManager>`. Board-host commits enter the same global stack.
|
|||
|
|
- **Undo on an evicted/unmounted host (MAJOR-4 path):** because the `HostState`/`StrokeStore` is retained whenever undo history references it (§ State above), `undo()/redo()` for a host whose Picture was evicted simply (1) mutates that host's retained `StrokeStore` (bump `revision`), (2) calls `SaveScheduler.schedule(hostId)` to persist the change, and (3) optionally scrolls the host into view (`pdfController.goToPage`). If/when the host's overlay later re-mounts, `StaticInkPainter` rebuilds the `ui.Picture` from the (now-updated) retained store on first paint — no special case, because the store was never lost. The Picture being absent at undo time is irrelevant: undo operates on the store, not the Picture.
|
|||
|
|
- `void addTextBox / updateTextBox / moveTextBox / deleteTextBox`.
|
|||
|
|
- `void setMode(EditorMode)`, `setTool`, etc.
|
|||
|
|
- Debounced persistence via `SaveScheduler` (§3.6).
|
|||
|
|
- **Deps:** `DatabaseService`, hosts, models, `UndoManager` (kept), `getStroke`.
|
|||
|
|
- **Used by:** all layers + toolbar via Riverpod provider `editorControllerProvider(documentId)`.
|
|||
|
|
|
|||
|
|
### 3.3 Render layers
|
|||
|
|
- **[ADD] `lib/editor/render/annotation_layer.dart`**
|
|||
|
|
- `class AnnotationLayer extends StatelessWidget` params: `host`, `controller`. Builds a `RepaintBoundary` wrapping a `Stack`:
|
|||
|
|
- `CustomPaint(painter: StaticInkPainter(store, host))`
|
|||
|
|
- `CustomPaint(painter: LiveInkPainter(controller.liveStroke, host))` (repaints via `repaint: liveStroke` Listenable)
|
|||
|
|
- `TextBoxLayer(host, controller)`
|
|||
|
|
- **[ADD] `lib/editor/render/static_ink_painter.dart`**
|
|||
|
|
- Caches a `ui.Picture` keyed by `revision`. `shouldRepaint` = `old.revision != revision` → **O(1)**. On rebuild: `applyContentToCanvas`, draw all committed strokes (reuse `_drawStroke` body extracted to `lib/editor/render/stroke_drawing.dart`). Viewport culling retained (existing `_strokeInViewport`).
|
|||
|
|
- **[ADD] `lib/editor/render/picture_cache.dart` — Picture memory budget + eviction (gates P-1 vs P-3 contradiction).**
|
|||
|
|
- On a tablet, P-1 (300 pages at 60fps) and P-3 (a 2,000-stroke page never rebuilds its Picture) **conflict** if every page retains a `ui.Picture` forever — 300 retained Pictures blows the memory budget. Resolution: a bounded LRU `PictureCache` whose lifetime is **tied to pdfrx's visible + cache window**:
|
|||
|
|
- A page's `ui.Picture` is built/retained **only while its `AnnotationLayer` overlay is mounted** (i.e. pdfrx has it within visible + `verticalCacheExtent`). On overlay unmount (page scrolled far off-screen), `dispose()` the Picture and drop it from the cache.
|
|||
|
|
- LRU cap: retain at most `K` page Pictures (default `K` ≈ visible pages + 2×cache-extent, e.g. ~8–12) regardless of mount churn, evicting least-recently-painted and disposing.
|
|||
|
|
- Re-entry: a page re-entering the window rebuilds its Picture from committed strokes (cheap relative to scroll budget; revision unchanged so it's a one-time rebuild, not per-frame).
|
|||
|
|
- **DECOUPLED from stroke stores (MAJOR-4):** this cache holds `ui.Picture`s ONLY. It must never reach into or evict `HostState`/`StrokeStore`. Store lifetime is governed solely by §3.2 (retain while undo history or dirty state references the host). Evicting a Picture is always safe because it can be rebuilt from the retained store.
|
|||
|
|
- **Safe dispose (deferred to post-frame):** never call `ui.Picture.dispose()` synchronously on a Picture that may still be referenced by an in-flight raster frame (e.g. evicting during the same frame that painted it). Defer disposal via `SchedulerBinding.instance.addPostFrameCallback` (or a one-frame quarantine queue) so the raster thread is done with it first.
|
|||
|
|
- **Used by:** `StaticInkPainter` (asks the cache for the page's Picture by `(hostId, revision)`).
|
|||
|
|
- **[ADD] `lib/editor/render/live_ink_painter.dart`**
|
|||
|
|
- Paints only `liveStroke`. During draw: render a **raw pressure-polyline** (cheap) rather than recomputing `getStroke` every move (§6.4). `repaint:` bound to the `ValueNotifier<LiveStroke?>` so only this painter invalidates on pointer move.
|
|||
|
|
- **[ADD] `lib/editor/render/stroke_drawing.dart`** — pure functions extracted from `_InkPainter._drawStroke/_drawFreehand/_drawRect/...` (single source of truth shared by static painter, live painter raw-mode, and export).
|
|||
|
|
|
|||
|
|
### 3.4 PDF pane + overlay wiring
|
|||
|
|
- **[ADD] `lib/editor/pdf/editor_pdf_pane.dart`**
|
|||
|
|
- Wraps `PdfViewer.file(filePath, controller: pdfController, params: ...)`.
|
|||
|
|
- `pageOverlaysBuilder: (ctx, pageRect, page) => [ SizedBox.fromSize(size: pageRect.size, child: PageAnnotationLayer(pageIndex: page.pageNumber-1, pageSize: pageRect.size, controller: ctrl)) ]` — **the `SizedBox.fromSize(size: pageRect.size)` wrap is mandatory** so the child's `CustomPaint.size` equals the zoomed page box (see §2.1). If pdfrx positions overlay children at `pageRect.topLeft` already, this is sufficient; if it expects an absolutely-positioned child, use `Positioned.fromRect(rect: pageRect, child: …)`. **[VERIFY-IN-M1: exact builder return shape + whether children are page-local or viewer-local positioned].**
|
|||
|
|
- `viewerOverlayBuilder: (ctx, size, handleLinkTap) => [ InputArbiterOverlay(size: size, controller: ctrl, pdfController: pdfController) ]`. **[VERIFY-IN-M1: arg order/types + return type].**
|
|||
|
|
- Bridges page-change → controller current page; exposes `goToPage`, zoom controls. **[VERIFY-IN-M1: `PdfViewerController.{goToPage, currentZoom, layout, globalToDocument, documentToLocal}` signatures].**
|
|||
|
|
- **[ADD] `lib/editor/pdf/page_annotation_layer.dart`** — adapter: builds `NormalizedPageHost(pageIndex)`, takes `pageSize` (= `pageRect.size`), and renders `AnnotationLayer`. Its root `CustomPaint` must receive `size == pageSize` (guaranteed by the `SizedBox.fromSize` wrap above) so `canvas.scale(size.width, size.height)` is valid.
|
|||
|
|
|
|||
|
|
### 3.5 Input arbitration
|
|||
|
|
- **[ADD] `lib/editor/input/input_arbiter.dart`** — pure state machine (no Flutter widgets) — see §6. Unit-testable.
|
|||
|
|
- **[ADD] `lib/editor/input/input_arbiter_overlay.dart` — CONCRETE per-kind routing transport.**
|
|||
|
|
- **The contradiction to resolve:** a plain `Listener` wrapping an `IgnorePointer` child either (a) sits *above* pdfrx and, being non-opaque, may still let the parent `Listener` see events while hit-testing passes through to pdfrx — but `Listener` does **not** consume events from the gesture arena, so pdfrx's pan recognizer *also* sees stylus moves; or (b) if made opaque, swallows touch and pdfrx never scrolls. Neither alone gives "stylus→us, touch→pdfrx."
|
|||
|
|
- **Decision — Primary transport (custom RenderProxyBox):** implement a `RenderProxyBox` subclass (`_PenCaptureRenderBox`) placed in `viewerOverlayBuilder` whose `hitTestSelf`/`hitTest` returns **true only when the incoming pointer's `kind ∈ {stylus, invertedStylus}`**, and **false for touch/mouse** so the hit-test continues to pdfrx underneath. On a stylus hit it becomes the pointer's target and receives the full down/move/up stream (which it feeds to `InputArbiter`) while **never entering the gesture arena**; touch/mouse are not hit by us at all and reach pdfrx normally. This is the explicit, deterministic per-kind split. Exposed as a small `PenCaptureRegion({onPenEvent, child})` widget wrapping the `RenderProxyBox`.
|
|||
|
|
- **Fallback transport (if M1 shows the RenderProxyBox path fights pdfrx):** adopt pdfrx's sanctioned `PdfOverlayInteractionRegion` (**[VERIFY-IN-M1: full constructor + whether it coexists with pan/zoom and exposes raw pointer kind]**) for tap/stroke capture, or, last resort, the upstream `gestureDeviceFilter` PR (R1).
|
|||
|
|
- Modes: in `Type`/`Browse` the region's `hitTestSelf` predicate is adjusted (e.g. Browse → never capture pen; Draw/Type → capture pen). Mouse is never captured for drawing in any mode (pen-required, §6.2); the region's predicate matches only `{stylus, invertedStylus}`.
|
|||
|
|
|
|||
|
|
### 3.6 Persistence
|
|||
|
|
- **[MODIFY] `lib/services/database_service.dart`** — see §5. Add: `getAllAnnotationsForDocument(documentId) → Map<int,String>` (one query), text-box CRUD, board-strokes CRUD (reuse/rename `scratchpad`).
|
|||
|
|
- **[ADD] `lib/editor/state/save_scheduler.dart`** — `class SaveScheduler` debounced (≈800ms) + flush-on-dispose + flush-on-page-leave.
|
|||
|
|
- **Ordering invariant (decided):** `stylusUp → commitStroke → (revision++, push undo) → scheduleSave`. The save snapshot for a host is **serialized synchronously at schedule/flush time, before any `await`** — i.e. `jsonEncode(host.strokes.committed)` runs in the same synchronous frame, then the encoded string is handed to the async DB write. This preserves the existing wrong-page-saved guard from `_saveCurrentPageAnnotations` (currently it captures `targetPage`+serializes before the first `await`). Continuous scroll makes the race **more** likely (the "current host" can change mid-flush), so the snapshot must capture `(hostId, encodedJson)` synchronously and the async writer must use only those captured values, never re-reading controller state after an `await`.
|
|||
|
|
|
|||
|
|
### 3.7 Erase / split util
|
|||
|
|
- **[ADD] `lib/editor/input/stroke_eraser.dart`** — pure `List<InkStroke> splitStroke(InkStroke, Set<int> erasedIdx)` + `eraseHits(strokes, point, radius)` extracted from `ink_canvas._eraseAt/_splitStroke`. Unit-tested.
|
|||
|
|
|
|||
|
|
### 3.8 Text boxes
|
|||
|
|
> **Sanctioned exception to Principle 1 (single source of truth = paint-time transform).** Text boxes are real `Positioned` Flutter widgets (`EditableText`), not canvas paint — they **cannot** ride the `canvas.scale` transform the ink painters use. Their placement is therefore computed in **widget space**: position = `host.toDevice(contentRect.topLeft)` and the rendered box multiplies its content-space size by `controller.currentZoom` (font size scales with zoom too). This is an explicit, bounded carve-out; the *stored* truth remains content coordinates (so Principle 1 holds for persistence and for P2 search), only the *render path* differs. Documented here so it is not mistaken for a violation.
|
|||
|
|
|
|||
|
|
- **[ADD] `lib/editor/text/text_box_model.dart`** — `TextBoxModel { id, content, contentRect (truth coords), fontSize, color }` (freezed).
|
|||
|
|
- **[ADD] `lib/editor/text/editable_text_box.dart`** — a `Positioned` `TextField`/`EditableText` placed via `host.toDevice(rect.topLeft)` with size and font scaled by `controller.currentZoom` (widget-space, per the §3.8 exception); draggable handle to move; focus participates in `Type` mode.
|
|||
|
|
- **[ADD] `lib/editor/text/text_box_layer.dart`** — renders all text boxes for a host; hit-test add on tap in `Type` mode.
|
|||
|
|
|
|||
|
|
### 3.9 Toolbar + modes + shortcuts
|
|||
|
|
- **[MODIFY] `lib/widgets/annotation_toolbar.dart`** → re-home as `lib/editor/ui/editor_toolbar.dart` (or adapt in place). Add: `EditorMode` selector (Draw/Browse/Type), board toggle, tool number indicators. Keep tool/color/width/pressure/stabilization/undo/redo.
|
|||
|
|
- **[ADD] `lib/editor/ui/editor_shortcuts.dart`** — `CallbackShortcuts` map: 1..9 tool select, Ctrl+Z/Y, Ctrl+Shift+Z, Ctrl+F (no-op stub in P1 / scrolls to top), Ctrl+S (force flush), Esc → Browse mode, Space/middle-drag pan handled by pdfrx.
|
|||
|
|
|
|||
|
|
### 3.10 Screen assembly + retirement of old editor
|
|||
|
|
- **[ADD] `lib/screens/editor_screen.dart`** — the new top-level editor (replaces `pdf_annotator_screen.dart` usage).
|
|||
|
|
- **[MODIFY] navigation entry points** that push `PdfAnnotatorScreen` → push `EditorScreen`. (Grep for `PdfAnnotatorScreen(` and `SplitViewScreen(` constructors.)
|
|||
|
|
- **[DELETE]** after parity is reached and tests pass:
|
|||
|
|
- `lib/widgets/pdf_annotation_layer.dart` (replaced by `page_annotation_layer.dart`)
|
|||
|
|
- `lib/screens/pdf_annotator_screen.dart` (replaced by `editor_screen.dart`)
|
|||
|
|
- `lib/screens/split_view_screen.dart` (replaced by board pane inside `editor_screen.dart`)
|
|||
|
|
- `lib/widgets/ink_canvas.dart` **only** once its draw/erase/stabilizer logic is fully extracted to `stroke_drawing.dart` + `stroke_eraser.dart`. **"Keep until extracted" means: `ink_canvas.dart` stays compiled-in AND remains the live reference implementation until the extraction is *verified* (the new `stroke_drawing`/`stroke_eraser` units pass the ported unit tests with identical output, §8.1).** Do not fork/duplicate its logic into the new units and leave the old one drifting — extract, prove equivalence, then delete in one step to avoid silent divergence between two stroke renderers.
|
|||
|
|
|
|||
|
|
### 3.11 Re-homed PDF mutation/export
|
|||
|
|
- **[MODIFY] `lib/services/pdf_service.dart`** — keep (uses `syncfusion_flutter_pdf`, headless, no viewer). `exportAnnotatedPdf` and page delete/insert/rotate still operate on file bytes. Two required changes:
|
|||
|
|
1. Export `_renderStrokes` must use the **same** outline geometry as on-screen (call `stroke_drawing` outline builder, not the crude line-segments path) so export matches what the user saw. **Critically: `getStroke` returns a closed *fill polygon*, not a centerline.** The current export pen-*strokes* line segments, which would render a hairline outline instead of the filled nib shape. Export must build a `PdfPath` from the `getStroke` outline points and **fill** it with a `PdfBrush` (solid color), not stroke it with a `PdfPen`. (Shape/line/arrow/text tools keep their stroke/fill semantics as today.) This is what makes the R4 golden pass.
|
|||
|
|
2. Rotation: since strokes are now stored in **unrotated** page space, drop the `_rotateStroke90CW` in-memory transform from the editor; export reads pdfium/syncfusion page rotation and applies it once.
|
|||
|
|
- **[KEEP]** `thumbnail_service.dart`, `stroke_rasterizer.dart`, `ctc_decoder`, OCR assets (P2).
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 4. Dependency Changes
|
|||
|
|
|
|||
|
|
**`pubspec.yaml`:**
|
|||
|
|
- **ADD** `pdfrx: ^2.4.4` (latest stable observed on pub.dev at planning time; **[VERIFY-IN-M1]** the exact resolved version after `flutter pub add pdfrx`).
|
|||
|
|
- **REMOVE** `syncfusion_flutter_pdfviewer` and `syncfusion_pdfviewer_platform_interface` (viewer + thumbnail-render interface). Note: `thumbnail_service.dart` uses `syncfusion_pdfviewer_platform_interface` to render pages off-screen — **migrate thumbnails to pdfrx page rendering** before removing it. **[VERIFY-IN-M1 (SHOULD #6): confirm pdfrx exposes an off-screen `PdfPage.render` → RGBA bytes path** (e.g. `PdfPage.render(...) → PdfImage`/RGBA) **before scheduling the Syncfusion-viewer dep removal in M6.]** Keep this dep until thumbnails are ported and that path is proven.
|
|||
|
|
- **KEEP** `syncfusion_flutter_pdf` (headless export/page-mutation in `pdf_service.dart`) **unless** the spike confirms pdfrx/pdfium can do equivalent vector page delete/insert/rotate + ink draw at acceptable fidelity (decision deferred to §10 Milestone 1 spike; default = keep syncfusion_flutter_pdf for export).
|
|||
|
|
- **KEEP** `perfect_freehand`, `flutter_riverpod`, `riverpod_annotation`, `sqflite*`, `freezed`, `json_serializable`, `flutter_onnxruntime`, `file_picker`, `image_picker`, `google_fonts`.
|
|||
|
|
- **KEEP** the `sqlite3: 3.3.2` override + vendored-binary `hooks` block (offline/GFW build) unchanged.
|
|||
|
|
|
|||
|
|
**Windows build prerequisite [VERIFY-IN-M1]:** pdfrx docs state it requires **Windows Developer Mode enabled** (symlinks at build time). Confirm during the M1 Windows build; document in README + CI runner setup.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 5. Data Model + Persistence Changes
|
|||
|
|
|
|||
|
|
Because **data may reset**, prefer the cleanest schema over migrations. Bump DB to a fresh `version` with `_onCreate` only (no upgrade path required, but keep `_onUpgrade` harmless).
|
|||
|
|
|
|||
|
|
### 5.1 Strokes (normalized model)
|
|||
|
|
- `InkStroke` / `InkPoint` models **[KEEP]** (freezed, already JSON-serializable). Strokes stored as JSON blob per host (matches current `annotations.annotation_json`). Blob-per-page is the default for write simplicity.
|
|||
|
|
- **Blob-vs-per-stroke-rows is decided in M2 by a measured trigger, not deferred:** blob-per-page rewrites the whole page on each save, which collides with the P-3 2,000-stroke target. **Rule:** during M2, measure the `SaveScheduler` full-page flush at 2,000 strokes; **if flush > 50ms**, switch the strokes table to **per-stroke rows** (`strokes(id, document_id, host_id, page_number, stroke_json, ...)` with a `(document_id, host_id, page_number)` index) and incremental insert/delete on commit/erase instead of full-page rewrite. This keeps save off the inking-frame budget regardless of page density.
|
|||
|
|
|
|||
|
|
### 5.2 Tables (clean schema)
|
|||
|
|
- `documents` **[KEEP]** (drop `rotation` column reliance from editor logic; pdfium owns page rotation). Column may stay for compatibility.
|
|||
|
|
- `annotations(document_id, page_number, annotation_json, ...)` **[KEEP shape]**. **ADD** batch read `getAllAnnotationsForDocument`.
|
|||
|
|
- **[ADD]** `text_boxes(id, document_id, host_id, page_number, content, rect_json, font_size, color, created_at, updated_at)` — addressable by host; `content` is plain text for future FTS.
|
|||
|
|
- `scratchpads(document_id UNIQUE, strokes_json)` **[REPURPOSE]** → board host store. Optionally rename to `boards` and add `text_boxes` rows with `host_id='board'`. Keep `scratchpads` name to minimize churn; document the rename decision.
|
|||
|
|
- `bookmarks` **[KEEP]**. `ocr_results`, `document_fts`, `notes*` **[KEEP]** (untouched in P1).
|
|||
|
|
|
|||
|
|
### 5.3 Coordinate semantics on disk
|
|||
|
|
- PDF page strokes: normalized [0,1] **unrotated** page space (changed from current "possibly rotated" representation — acceptable because data resets).
|
|||
|
|
- Board strokes: absolute logical px (as today in `split_view`).
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 6. Input Arbitration Design
|
|||
|
|
|
|||
|
|
### 6.1 Modes (user-selectable, G6)
|
|||
|
|
- `Draw` — pen draws; touch scrolls; **mouse pans/selects (does not draw — pen required, see §6.2)**.
|
|||
|
|
- `Browse` — nothing draws; wheel scroll, Ctrl+wheel zoom, space/middle-drag pan; pen does nothing (prevents accidental ink during reading/复习).
|
|||
|
|
- `Type` — text boxes focusable/editable; pen still draws (so you can annotate around a text box); touch scrolls.
|
|||
|
|
|
|||
|
|
### 6.2 Device → action matrix
|
|||
|
|
| Device | Draw mode | Browse mode | Type mode |
|
|||
|
|
| --- | --- | --- | --- |
|
|||
|
|
| stylus (tip) | draw | ignore | draw |
|
|||
|
|
| invertedStylus | erase | ignore | erase |
|
|||
|
|
| touch (1–2 fingers) | pdfrx scroll/zoom | pdfrx scroll/zoom | pdfrx scroll/zoom |
|
|||
|
|
| mouse (left drag) | **pan/select (does NOT draw — pen required to draw)** | pan (if space/middle) else select | text caret / select |
|
|||
|
|
| mouse wheel | scroll (Ctrl=zoom) | scroll (Ctrl=zoom) | scroll (Ctrl=zoom) |
|
|||
|
|
|
|||
|
|
> **Decided (reconciles former Open Question 5):** in Draw mode, **mouse left-drag does NOT draw** — drawing requires a pen (stylus). Desktop review/复习 with a mouse pans/selects; this prevents accidental ink while a mouse user scrolls/reads, and keeps "pen = ink" unambiguous across devices. (`Draw` mode entry in the `mouse left drag` row reflects this.)
|
|||
|
|
|
|||
|
|
### 6.3 State machine (`InputArbiter`)
|
|||
|
|
States: `idle → inking → erasing → touchNav`.
|
|||
|
|
- `idle + stylusDown` (mode≠Browse) → `inking`; emit `beginStroke`. Set `_stylusActive=true`.
|
|||
|
|
- `idle + invertedStylusDown` → `erasing`; emit `eraseAt`.
|
|||
|
|
- `inking + stylusMove` → `extendStroke`. `inking + stylusUp` → `commitStroke` → `idle`.
|
|||
|
|
- **Palm rejection:** any `touchDown` while `_stylusActive` → **dropped** (not forwarded, not drawn). `touchDown` while `idle` → not consumed → falls through to pdfrx (`touchNav`, but we don't model it; pdfrx owns it).
|
|||
|
|
- `stylusCancel`/pointer-leave → discard live stroke → `idle`.
|
|||
|
|
- **Transport (see §3.5):** the per-kind split is done by a `RenderProxyBox` (`PenCaptureRegion`) whose hit-test returns true **only** for `{stylus, invertedStylus}`, so stylus events target us (outside the gesture arena) while touch/mouse are never hit by us and reach pdfrx's recognizer underneath. This replaces the earlier hand-wave that a bare `Listener`+`IgnorePointer` would route by kind — it does not (a `Listener` does not consume from the arena). **[VERIFY-IN-M1: that a stationary stylus-down does not trigger pdfrx pan AND single-finger touch still scrolls — pdfrx's recognizer `supportedDevices` is hardcoded, §6.5.]**
|
|||
|
|
|
|||
|
|
### 6.4 Live-stroke incremental rendering (perf correctness, root cause 1)
|
|||
|
|
- During `inking`, accumulate raw `InkPoint`s; `LiveInkPainter` draws a **pressure-aware polyline / quad path** (cheap, O(points added)), NOT `getStroke` over the whole stroke each move.
|
|||
|
|
- On `commitStroke`, run `getStroke(..., isComplete:true)` **once** to produce the final outline, store as committed, bump revision. Visual "pop" at commit must be imperceptible — verified by the perf/quality check (§8). If the polyline preview diverges too much, fallback option: run `getStroke` only over a trailing window of the last N points (incremental tail) — flagged as a tuning task, not a redesign.
|
|||
|
|
- **Mid-stroke resize safety (§2.1 interaction):** each live `InkPoint` is captured in **normalized content coords** at the moment of the pointer event — `host.toContent(event.localPosition, currentDeviceSize)` — NOT raw device pixels. So if `pageRect.size` changes mid-stroke (tile reflow / zoom between `beginStroke` and `commitStroke`), already-captured points stay correct (they are resolution-independent) and the committed stroke cannot mis-scale; only the in-flight *render* re-maps via the new transform. The `LiveInkPainter` likewise paints through `host.applyContentToCanvas` at the current size, so the live preview follows a resize too.
|
|||
|
|
|
|||
|
|
### 6.5 pdfrx gesture caveat **[RISK — gesture-device set is [VERIFY-IN-M1]]**
|
|||
|
|
The pdfrx-v2.4.4 docs indicate its gesture recognizer `supportedDevices` includes stylus+touch+mouse with **no param to restrict by pointer kind**; this is doc-derived and **[VERIFY-IN-M1]** by reading `~/.pub-cache/.../pdfrx-*/lib/src/` (the recognizer's `supportedDevices` set). Mitigation: the `RenderProxyBox` per-kind hit-test transport (§3.5) keeps stylus out of pdfrx's arena without a fork. **M1 must empirically confirm** that a stationary stylus-down inside `viewerOverlayBuilder` does not trigger pdfrx pan AND that single-finger touch still scrolls. If it conflicts, fallback = `PdfOverlayInteractionRegion` (**[VERIFY-IN-M1 capability]**) or an upstream `gestureDeviceFilter` PR. This is the #1 thing the spike de-risks; it is a **hard M1 exit gate** (see §10/M1).
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 7. Performance Strategy (measurable)
|
|||
|
|
|
|||
|
|
### 7.1 Targets
|
|||
|
|
|
|||
|
|
> **Sample protocol (applies to all frame-time targets P-1, P-2, P-4, and the M1 perf gates):** "median"/"p95" are computed over **N ≥ 120 frames during sustained programmatic scroll (a continuous fling driven by `tool/perf_scroll_bench.dart`), profile mode, warm cache** (discard the first 30 frames so tile/Picture caches are populated before sampling). Each run is repeated 3× and the median run is reported. P-5 (open latency) is a single cold-open measurement averaged over 3 runs.
|
|||
|
|
|
|||
|
|
- **P-1:** Continuous scroll/zoom on a **300-page** PDF: median frame build+raster ≤ **16.6ms** (60fps); p95 ≤ 22ms; zero sustained jank (>32ms) during steady scroll. *Sample: N ≥ 120 frames, sustained scroll, warm cache (per protocol above).*
|
|||
|
|
- **P-2:** Inking latency: a single stroke of 500 points keeps frame time ≤ 16.6ms on the live layer (static layer untouched during draw).
|
|||
|
|
- **P-3:** Page with **2,000 committed strokes** (page currently within the visible+cache window): pointer-move during a *new* stroke does not rebuild that page's static picture (assert `StaticInkPainter.shouldRepaint==false` while `revision` constant). Scope: applies to mounted pages; off-window pages are evicted by design (see P-6).
|
|||
|
|
- **P-4:** Board host with **5,000 strokes**: pan/zoom ≤ 16.6ms median (relies on static Picture cache + cull).
|
|||
|
|
- **P-5:** Document open: `getAllAnnotationsForDocument` is a single query; time-to-first-page-interactive < 500ms on a 300-page doc with annotations on 50 pages. *Sample: single cold-open, averaged over 3 runs (per protocol above).*
|
|||
|
|
- **P-6 (memory budget; reconciles P-1 vs P-3):** Total retained ink `ui.Picture` memory on the 300-page asset (annotations on 50 pages) ≤ **64 MB** at any time, enforced by the `PictureCache` LRU + unmount-dispose (§3.3). P-3's "no rebuild while revision constant" holds for **mounted** pages only; off-window pages are intentionally evicted, so P-1 and P-3 do not contradict — P-3 is scoped to the visible+cache window. Verify by sampling `ui.Picture` count × estimated bytes (or `dart:developer` memory snapshot) during a full scroll.
|
|||
|
|
|
|||
|
|
### 7.2 How to verify
|
|||
|
|
- **Benchmark asset:** add `test/assets/large_300p.pdf` (generate via a script using `syncfusion_flutter_pdf`; commit or generate in a `tool/gen_bench_pdf.dart`). Plus `test/assets/dense_strokes.json` (2k/5k synthetic strokes).
|
|||
|
|
- **Frame timing harness:** integration test using `WidgetController` + `SchedulerBinding.addTimingsCallback` (or `flutter run --profile --trace-skia`); record `FrameTiming.totalSpan`. A `tool/perf_scroll_bench.dart` drives programmatic scroll and prints median/p95.
|
|||
|
|
- **shouldRepaint assertion:** unit/widget test wraps `StaticInkPainter` and asserts O(1) behavior (P-3).
|
|||
|
|
- **Manual gate (Windows + Surface Pen):** documented checklist run on target hardware (subjective 60fps + no ink lag) since CI lacks a pen. Recorded in `docs/plans/phase1-perf-results.md`.
|
|||
|
|
- **Acceptance:** P-1..P-5 numeric targets met in profile mode on the dev Windows tablet; results pasted into the perf-results doc with the commit hash.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 8. Testing Strategy
|
|||
|
|
|
|||
|
|
> **sqlite test workaround [CONFIRMED in repo memory]:** `flutter test` fails to download sqlite3 locally; run DB-touching tests with system sqlite + `LD_LIBRARY_PATH` (see `badnote-local-test-sqlite.md`). Provide a `tool/test.sh` wrapper that sets `LD_LIBRARY_PATH` to the system sqlite and runs `flutter test`. Pure-logic tests (transforms, eraser, arbiter) must NOT touch the DB so they run without the workaround.
|
|||
|
|
|
|||
|
|
### 8.1 Unit (no DB, no Flutter binding where possible)
|
|||
|
|
- **Coordinate transforms:** `NormalizedPageHost`/`BoardHost` round-trip `toContent(toDevice(x))≈x`; rotation handled by pdfrx geometry (test our hosts assume unrotated).
|
|||
|
|
- **Stroke split/erase:** port + extend existing `undo_manager_test.dart` discipline; cover full-erase (empty replacements), mid-erase (2 segments), endpoint erase, <2-point dropping.
|
|||
|
|
- **Revision gating:** `StrokeStore.add/remove` bumps revision; `StaticInkPainter.shouldRepaint` true iff revision changed.
|
|||
|
|
- **InputArbiter state machine:** table-driven tests over the §6.2 matrix incl. palm rejection (touch dropped while stylus active) and mode transitions. **No widgets** → fast, deterministic.
|
|||
|
|
- **getStroke commit-equivalence:** committing a polyline produces a non-empty outline; live-polyline bounds ⊆ committed-outline bounds (sanity for §6.4 "no pop").
|
|||
|
|
- **SaveScheduler synchronous-snapshot invariant (§3.6):** with a fake DB whose write `await`s on a controllable completer, assert that mutating controller state / changing the "current host" *after* `scheduleSave` but *before* the write completes does NOT change what gets persisted — the snapshot `(hostId, encodedJson)` was captured synchronously. This is the regression test for the wrong-page-saved race under continuous scroll.
|
|||
|
|
|
|||
|
|
### 8.2 Widget tests
|
|||
|
|
- **Input routing:** pump editor with a fake pdfrx pane (or a `Listener` test harness) and synthesize `PointerEventKind.stylus` vs `.touch`; assert stylus → stroke committed, touch → not consumed.
|
|||
|
|
- **Text box edit:** tap in Type mode adds a box; typing updates model; drag moves it; persists via fake DB.
|
|||
|
|
- **Mode behavior:** Browse mode → stylus down produces no stroke; Draw mode → it does.
|
|||
|
|
|
|||
|
|
### 8.3 Perf/benchmark check
|
|||
|
|
- `tool/perf_scroll_bench.dart` (P-1) + `StaticInkPainter` no-rebuild assertion (P-3) run in profile mode; not a hard CI gate (CI has no GPU profile reliability) but required before milestone sign-off, output archived.
|
|||
|
|
|
|||
|
|
### 8.4 Regression
|
|||
|
|
- Keep `ctc_decoder_test.dart` green (OCR untouched). Update/replace `widget_test.dart` to boot `EditorScreen`.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 9. Risks & Mitigations
|
|||
|
|
|
|||
|
|
| # | Risk | Likelihood | Impact | Mitigation / Trigger |
|
|||
|
|
| --- | --- | --- | --- | --- |
|
|||
|
|
| R1 | **pdfrx stylus-vs-touch arbitration** infeasible without a fork (stylus triggers pdfrx pan, or touch stops scrolling). | Med | High | Milestone-1 spike proves it with a `Listener`. Fallback: upstream PR adding pointer-kind filter, or restrict pan region. Do not build the rest until R1 is GREEN. |
|
|||
|
|
| R2 | **60fps on 300 pages** not met even with pdfrx tiling. | Med | High | Measure pdfrx-alone first (no ink) at Milestone 1; tune `verticalCacheExtent`, `maxImageBytesCachedOnMemory`, `onePassRenderingSizeThreshold`. If pdfrx itself can't hit it, that invalidates the chosen backend → escalate (this is why perf is de-risked first). |
|
|||
|
|
| R3 | **Infinite-board perf** with thousands of strokes. | Med | Med | Static Picture cache + viewport cull + spatial tiling of the board into chunks if needed (deferred sub-task). |
|
|||
|
|
| R4 | **Export fidelity** drift: exported ink ≠ on-screen ink. | Med | Med | Share `stroke_drawing` outline geometry between screen + export; golden-image compare a known page. |
|
|||
|
|
| R5 | **perfect_freehand incremental correctness** (live polyline vs committed outline "pop"). | Med | Low | §6.4 commit-once; if visible, switch to trailing-window incremental getStroke. Covered by §8.1 equivalence test + manual check. |
|
|||
|
|
| R6 | **Thumbnail rendering** loses its syncfusion platform-interface backend. | High | Low | M1 (SHOULD #6) confirms pdfrx exposes an off-screen `PdfPage.render → RGBA` path **before** M6 schedules the syncfusion-viewer dep removal; port thumbnails to it; keep the dep until ported and proven. |
|
|||
|
|
| R7 | **pdfrx controller API drift** (all pdfrx APIs are [VERIFY-IN-M1], not compile-checked — pdfrx not yet installed). | Med | Low | M1 sub-task 1 installs pdfrx and source-pins exact signatures from `~/.pub-cache/.../pdfrx-*/lib/src/`, updating every [VERIFY-IN-M1] tag. |
|
|||
|
|
| R8 | **Windows Dev Mode** not enabled on CI/dev → build fails. | Med | Med | Document + add CI step to enable; covered in §4. |
|
|||
|
|
|
|||
|
|
### Pre-mortem (DELIBERATE mode — 3 failure scenarios)
|
|||
|
|
1. **"Six weeks in, scrolling a big PDF still janks."** Cause: we built ink/board/text first and only profiled pdfrx at the end. *Prevention:* Milestone 1 is pdfrx-only perf with a hard 60fps gate before any ink code lands.
|
|||
|
|
2. **"Pen draws but the page won't scroll with touch."** Cause: naive `Listener`/`IgnorePointer` layering swallowed touch (a `Listener` cannot route by pointer kind), or pdfrx arena conflict (R1). *Prevention:* **promoted to a hard M1 exit gate** (§10/M1 sub-task 3) with written PASS/FAIL — "pen draws AND single-finger touch scrolls AND pinch zooms in the same overlay" — tested on the **physical Surface Pen device**, using the `PenCaptureRegion` `RenderProxyBox` per-kind transport (§3.5). M2 is blocked until PASS.
|
|||
|
|
3. **"Export looks nothing like the screen."** Cause: export kept the old line-segment renderer while screen used `getStroke` outlines. *Prevention:* R4 shared-geometry task + golden test in Milestone 4.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 10. Milestones / Sequencing (de-risk perf FIRST)
|
|||
|
|
|
|||
|
|
**M1 — pdfrx spike + perf gate (de-risk R1, R2, R7). Hard gate — M2 does not begin until ALL of MUST #1–#5 below are GREEN (the two perf gates and the coordinate assertion can run on the Windows tablet without a pen; MUST #3 specifically requires the physical Surface Pen).**
|
|||
|
|
|
|||
|
|
*M1 sub-task 0 — bootstrap (no acceptance gate; creates the assets every later gate depends on; owner: implementer of M1):*
|
|||
|
|
- **[ADD] `tool/gen_bench_pdf.dart`** — generates `test/assets/large_300p.pdf` (300 pages, mixed text+vector content) using `syncfusion_flutter_pdf`. Run once; commit the asset (or document regenerating it).
|
|||
|
|
- **[ADD] `tool/gen_dense_strokes.dart`** → `test/assets/dense_strokes.json` — synthetic stroke sets at 300/2,000/5,000 strokes for P-3/P-4/P-6 and the M1 ink-overlay gate.
|
|||
|
|
- **[ADD] `tool/test.sh`** — wrapper that exports `LD_LIBRARY_PATH` to the system sqlite (per `badnote-local-test-sqlite.md`) then runs `flutter test "$@"`. All DB-touching tests run through it.
|
|||
|
|
- **[ADD] `docs/plans/phase1-perf-results.md`** — seed with empty result tables for each gate (pinned-API table, coordinate assertion, pdfrx-alone perf, ink-overlay perf, pen-arbitration PASS/FAIL), to be filled with device + commit hash.
|
|||
|
|
|
|||
|
|
*M1 acceptance sub-tasks (all blocking):*
|
|||
|
|
1. **Install + source-pin (MUST #1, blocking).** `flutter pub add pdfrx`, then read `~/.pub-cache/hosted/pub.dev/pdfrx-*/lib/src/` and pin EXACT signatures (arg order/types + return type) for: `pageOverlaysBuilder`, `viewerOverlayBuilder`, `PdfViewerController.{goToPage, currentZoom, layout, globalToDocument, documentToLocal}`, `PdfRect.toRect`, and the gesture recognizer's `supportedDevices` set. Record each in `phase1-perf-results.md`, replacing every **[VERIFY-IN-M1]** tag in this plan with the pinned fact (or with a corrected approach if the doc inference was wrong). Runs on Windows tablet or dev box (no pen needed).
|
|||
|
|
2. **Coordinate-correctness assertion (MUST #2, blocking).** With `PageAnnotationLayer` wrapped in `SizedBox.fromSize(size: pageRect.size)`, assert a stroke at normalized `(0.5,0.5)` lands at visual page center across **3 zoom levels** (fit / 2× / 4×). Automatable widget/golden test — no pen needed. **Blocking:** a wrong coordinate model invalidates the entire ink approach.
|
|||
|
|
3. **Pen/touch arbitration exit gate (MUST #3, blocking, requires physical Surface Pen):** using the `PenCaptureRegion` `RenderProxyBox` transport (§3.5), on the **physical Surface Pen tablet**: **PASS iff** (a) pen tip draws a stroke on the page overlay, AND (b) single-finger touch scrolls the document, AND (c) pinch zooms — all in the *same* overlay without mode switching. **FAIL** if pen triggers pan, or if touch stops scrolling. On FAIL, switch to the §3.5 fallback transport before re-testing; M2 blocked until PASS. **Owner/runner:** the developer with the Surface Pro tablet (the project's primary-target device). **Fallback if device unavailable:** CI has no pen — this gate is **not** automatable there; M2 stays blocked until a human runs it on real hardware. As an interim signal only (does NOT satisfy the gate), a Windows-mouse/synthesized-stylus smoke check may run in CI to catch gross regressions.
|
|||
|
|
4. **Perf gate — pdfrx alone (MUST #4, blocking; Principle 4 / pre-mortem #1 primary de-risk):** 300-page asset, fling-scroll in profile mode on the Windows tablet → median frame (build+raster) ≤ 16.6ms, p95 ≤ 22ms (sample protocol per §7.1). **Blocking:** if pdfrx alone cannot hit 60fps the chosen backend is invalidated — escalate before any further work. No pen needed.
|
|||
|
|
5. **Perf gate — WITH ink-overlay build cost (MUST #5, blocking):** add a throwaway `AnnotationLayer` stub that paints a **non-trivial `ui.Picture` on every visible page** (~300 synthetic strokes/page from `dense_strokes.json`), wrapped per the real cache/RepaintBoundary design. Fling-scroll the 300-page asset and assert frame **BUILD** time (not just raster) ≤ **16.6ms median** (sample protocol per §7.1). Proves overlay mounting/unmounting + Picture (re)build during scroll stays within budget — empty-pdfrx perf alone is insufficient. No pen needed.
|
|||
|
|
|
|||
|
|
*Deliverable:* the sub-task-0 assets above + a throwaway `lib/editor/pdf/editor_pdf_pane.dart` skeleton + `PenCaptureRegion` prototype + `docs/plans/phase1-perf-results.md` populated with pinned APIs, MUST #2/#4/#5 numeric results, and the MUST #3 pen-arbitration PASS/FAIL (device + commit hash). **No further work proceeds if ANY of MUST #1–#5 fails.**
|
|||
|
|
|
|||
|
|
**M2 — Ink engine on a single PDF page. Precondition: ALL of M1 MUST #1–#5 are GREEN (MUST #3 confirmed on the physical Surface Pen).** `CoordinateSpaceHost`, `EditorController`, `StaticInkPainter` + `PictureCache` (revision Picture cache w/ LRU + unmount-dispose, §3.3), `LiveInkPainter`, `stroke_drawing`, `RepaintBoundary`. *Exit:* draw/erase/undo on one page; P-2/P-3/P-6 met; ink follows scroll+zoom structurally (§2.1 sizing constraint honored). Includes the SHOULD #1 measured trigger: if full-page save flush > 50ms at 2,000 strokes, switch to per-stroke rows (§5.1).
|
|||
|
|
|
|||
|
|
**M3 — Full PDF editor parity.** Multi-page hosting, batched annotation load (root cause 3), `SaveScheduler`, re-home page mgmt/bookmarks/thumbnails, modes + shortcuts (G6), input arbiter integrated.
|
|||
|
|
- *Exit — every item in this parity CHECKLIST passes (each individually checkable; old-screen DELETE in §3.10 is gated on ALL passing):*
|
|||
|
|
|
|||
|
|
| # | Capability (current `pdf_annotator_screen.dart` / toolbar) | New-code location | Check |
|
|||
|
|
| --- | --- | --- | --- |
|
|||
|
|
| C1 | Page rotate 90° | `pdf_service.rotatePage` (kept) + editor re-render; strokes unrotated (§2.1) | rotate a page; ink stays aligned |
|
|||
|
|
| C2 | Page delete (+ remap annotations/bookmarks) | `pdf_service.deletePage` + `EditorController` remap + `database_service` remap fns (kept) | delete page; later pages' ink/bookmarks shift correctly |
|
|||
|
|
| C3 | Insert blank page | `pdf_service.insertBlankPage` + remap | insert; subsequent ink shifts |
|
|||
|
|
| C4 | Insert image page (camera/gallery) | `pdf_service.insertImageOnPage` + `camera_service` (kept) | image lands on page; ink overlays |
|
|||
|
|
| C5 | Bookmark add / toggle / jump | `EditorController` bookmark state + `database_service` bookmarks (kept) | add, toggle off, jump-to from drawer |
|
|||
|
|
| C6 | Thumbnail-sidebar nav | re-homed `PageThumbnailSidebar` rendering via pdfrx (§4 SHOULD #6) | tap thumbnail scrolls to page |
|
|||
|
|
| C7 | Text placement | `text_box_layer` / `editable_text_box` (§3.8) — delivered in M4; M3 exit notes dependency | place a text box (full edit lands M4) |
|
|||
|
|
| C8 | Undo / redo | global `UndoManager` on `EditorController` (§3.2) | Ctrl+Z/Y across pages reverses last commit |
|
|||
|
|
| C9 | Save-on-page-leave | `SaveScheduler` flush-on-page-leave (§3.6) | leave page; reopen; ink persisted |
|
|||
|
|
| C10 | Shortcuts Ctrl+Z/Y/F/S | `editor_shortcuts.dart` (§3.9) | each shortcut fires its action |
|
|||
|
|
| C11 | Zoom controls (in/out/fit) | `editor_pdf_pane` zoom wrappers over `PdfViewerController` | buttons change zoom; ink tracks |
|
|||
|
|
|
|||
|
|
*Note:* C7 full editing depends on M4; M3 may ship a placement stub, but the old screen is NOT deleted until C1–C11 (incl. C7 via M4) all pass. Old screen swapped in nav; widget tests green.
|
|||
|
|
|
|||
|
|
**M4 — Export + text boxes.** Shared-geometry export (R4 + golden), editable text boxes (G5), persistence (§5 text_boxes table). *Exit:* export matches screen; text boxes place/move/edit/persist.
|
|||
|
|
|
|||
|
|
**M5 — Infinite board.** `BoardHost`, `BoardPane`, board toggle, reuse ink engine; migrate `split_view` behavior; delete `split_view_screen.dart`. *Exit:* board draw at P-4; independent scroll/zoom; G4 done.
|
|||
|
|
|
|||
|
|
**M6 — Cleanup + sign-off.** Delete `pdf_annotation_layer.dart`, `pdf_annotator_screen.dart`, `ink_canvas.dart` (after extraction), remove syncfusion viewer deps, final perf-results doc, full test pass via `tool/test.sh`.
|
|||
|
|
|
|||
|
|
Each milestone ends with a verifier/critic pass and the perf-results doc updated with the commit hash.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 11. RALPLAN-DR Summary
|
|||
|
|
|
|||
|
|
### Principles (3–5)
|
|||
|
|
1. **Single source of truth = host content coordinates.** Screen mapping is a paint-time transform, never stored/duplicated geometry.
|
|||
|
|
2. **Separate static (committed) from live (in-progress) ink**, gated by an O(1) revision so steady-state inking touches only the live layer.
|
|||
|
|
3. **Pen-first, arena-free input.** A per-kind `RenderProxyBox` (`PenCaptureRegion`, §3.5) hit-tests true only for `{stylus, invertedStylus}` so stylus is captured outside the gesture arena while touch/mouse fall through to pdfrx's own recognizers; palm rejection is explicit. (Corrected from an earlier "raw `Listener` claims stylus" framing — a bare `Listener` cannot route by pointer kind.)
|
|||
|
|
4. **De-risk perf before features.** The PDF backend must prove 60fps on a large doc before any ink/board/text code is written.
|
|||
|
|
5. **Host-agnostic ink engine.** PDF page and infinite board are two `CoordinateSpaceHost`s behind one renderer, so P4's CAS overlay reuses the same seam.
|
|||
|
|
|
|||
|
|
### Decision Drivers (top 3)
|
|||
|
|
1. **D1 — "Ink follows PDF scroll/zoom" must be structural**, not manually synced (current arch makes it impossible; root cause 4).
|
|||
|
|
2. **D2 — 60fps on hundreds of pages with thousands of strokes** on Windows tablet + Surface Pen.
|
|||
|
|
3. **D3 — Reusable ink across PDF + infinite board** (G4) without duplicating the engine, and forward-compatible with P2 search / P4 CAS.
|
|||
|
|
|
|||
|
|
### Viable Options (≥2) with bounded pros/cons
|
|||
|
|
|
|||
|
|
**Option A — pdfrx + `pageOverlaysBuilder` (CHOSEN, pre-decided).**
|
|||
|
|
- Pros: page-coordinate overlay inherits scroll+zoom automatically (D1 ✔, structural); pdfium vector + tiled rendering targets D2; Windows supported; gives text-extraction API for P2; viewer-overlay seam for input arbiter.
|
|||
|
|
- Cons: gesture `supportedDevices` hardcoded (doc-derived, **[VERIFY-IN-M1]**) → stylus/touch arbitration must be proven (R1); several pdfrx APIs are **[VERIFY-IN-M1]** until source-pinned; new dependency + Windows Dev Mode build requirement.
|
|||
|
|
|
|||
|
|
**Option B — Custom pdfium wrapper inside `InteractiveViewer`.**
|
|||
|
|
- Pros: total control over gestures (cleanly solves R1); one transform owns both PDF tiles and ink (D1 trivially); board and PDF share the exact same viewer.
|
|||
|
|
- Cons: must hand-roll tiled hi-res rendering, page layout, text extraction, link handling → very large surface, high schedule risk against D2; reinvents what pdfrx already ships. **Rejected (see invalidation).**
|
|||
|
|
|
|||
|
|
**Option C — Keep `SfPdfViewer`, optimize the overlay only.**
|
|||
|
|
- Pros: smallest change; no new dependency; export path unchanged.
|
|||
|
|
- Cons: overlay Stack fundamentally cannot share the viewer transform → D1 unachievable (root cause 4 is structural, not a perf tweak); continuous-scroll Notability feel impossible; licensing/cost of Syncfusion. **Rejected (see invalidation).**
|
|||
|
|
|
|||
|
|
### Invalidation rationale for rejected options
|
|||
|
|
- **C rejected:** it cannot satisfy D1. The overlay is a sibling of `SfPdfViewer` with no access to its internal pan/zoom matrix, so ink can never track the page during continuous scroll/zoom — exactly the structural defect (root cause 4) this phase exists to fix. Optimizing the overlay improves frame cost but not the architecture.
|
|||
|
|
- **B rejected (deferred fallback, not chosen):** it *would* satisfy D1–D3 but at the cost of re-implementing tiled rendering, layout, and text extraction that pdfrx provides for free, putting D2's 60fps and the overall schedule at serious risk. It survives only as the **R1 fallback** if the M1 spike proves pdfrx's gesture arena truly cannot be worked around — in which case we reconsider B or an upstream pdfrx PR.
|
|||
|
|
- Net: **two options remain genuinely viable (A chosen, B as documented fallback)**; C is invalidated against the primary driver D1.
|
|||
|
|
|
|||
|
|
### ADR (Architect review applied; finalize after M1 spike)
|
|||
|
|
- **Status:** Architect verdict APPROVE-WITH-MUST-FIXES applied (2026-06-21). All 5 MUST-FIXes + 6 SHOULD-FIXes incorporated. Remaining gate: M1 spike must turn every **[VERIFY-IN-M1]** tag into a source-pinned fact and pass the pen-arbitration + perf gates.
|
|||
|
|
- **Decision:** Adopt pdfrx (`^2.4.4`, exact version [VERIFY-IN-M1]) with `pageOverlaysBuilder`-hosted ink and a **per-kind `RenderProxyBox` (`PenCaptureRegion`)** input transport (not a bare `Listener`); host-agnostic ink engine over `CoordinateSpaceHost`.
|
|||
|
|
- **Drivers:** D1 structural ink-follows-page, D2 60fps@300pages, D3 reusable ink across PDF/board.
|
|||
|
|
- **Alternatives considered:** B (custom pdfium/InteractiveViewer — retained as R1 fallback), C (optimize Syncfusion overlay — invalidated vs D1).
|
|||
|
|
- **Why chosen:** A is the only option that gives D1 *for free* while leveraging shipped tiled rendering + text extraction toward D2 and P2.
|
|||
|
|
- **Hardening from review:** (1) all pdfrx claims downgraded to [VERIFY-IN-M1], source-pinned in M1; (2) §2.1 "no zoom math" corrected — requires `CustomPaint` sized to `pageRect.size`; (3) bounded `PictureCache` LRU + unmount-dispose with memory target P-6 (reconciles P-1 vs P-3); (4) concrete per-kind `RenderProxyBox` transport replaces the `Listener`/`IgnorePointer` hand-wave; (5) M1 perf gate now includes ink-overlay BUILD cost, and the pen/touch arbitration is a hard M1 exit gate on the physical Surface Pen before M2.
|
|||
|
|
- **Consequences:** new dep + Windows Dev Mode build step; must prove gesture arbitration (R1) + ink-overlay build budget in M1; export keeps headless syncfusion_flutter_pdf and must **fill** getStroke outlines (not stroke them); global (not per-page) undo; text boxes are a sanctioned widget-space exception to Principle 1.
|
|||
|
|
- **Follow-ups:** finalize after M1 spike results; resolve the remaining Open Questions (export backend; board table rename; `panAxis`-during-zoom). (Mouse-in-Draw UX is now resolved: pen required to draw.)
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Open Questions (persist to `.omc/plans/open-questions.md`)
|
|||
|
|
- [x] ~~Per-page stroke blob vs per-stroke rows~~ — **RESOLVED into M2 with a measured trigger (§5.1): switch to per-stroke rows if 2,000-stroke flush > 50ms.** No longer an open question.
|
|||
|
|
- [ ] Keep `syncfusion_flutter_pdf` for export, or move export to pdfium? Decide at M1 from fidelity/feasibility. — *Affects §4 deps + R4.*
|
|||
|
|
- [ ] Board persistence: rename `scratchpads`→`boards` or keep name? — *Cosmetic; affects §5.*
|
|||
|
|
- [ ] Does the `PenCaptureRegion` `RenderProxyBox` (§3.5) over `viewerOverlayBuilder` reliably let single-finger touch scroll on the actual Surface Pen device? — *Hard exit criterion for M1 (MUST #3 / R1).*
|
|||
|
|
- [x] ~~Mouse-in-Draw-mode: should left-drag draw, or require pen?~~ — **RESOLVED (§6.1/§6.2): pen required to draw; mouse left-drag pans/selects in Draw mode.** No longer open.
|
|||
|
|
```
|