andodeki
00f1dfbc12
Three defects found by auditing the ADR merge criteria against the code rather than against memory. 1. The scheduled fuzz job ran five hardcoded targets. Four had been added since and were never fuzzed: parse_revision_chain, decrypt, eval_function and parse_colorspace. eval_function is the sharpest of those - it executes PostScript taken verbatim from an untrusted file. The list now comes from `cargo fuzz list` and the job fails rather than passing vacuously if it comes back empty. `cargo fuzz list` reads the manifest, so a target file added without its [[bin]] entry would still be skipped silently. The engine job, which runs on every push, now checks the two agree. Both directions of that guard were exercised before committing. 2. ADR 0004 rule 3 promises an annotation whose appearance cannot be generated "keeps its original /AP and is reported as skipped". The keeping worked - to_dict clones the source dictionary - but nothing reported it: SaveReport only tracked skipped appearances for form fields. A caller who moved a stamp was never told its artwork still showed the old position. Adds SaveReport::annotation_appearances_skipped and appearance_is_generated, which enumerates the out-of-scope types explicitly so a new AnnotationType fails to compile until classified. 3. SetContents and SetFlags had no round-trip test. Both were implemented and unit-tested against the in-memory model, but neither was ever reparsed from written bytes - the assertion ADR 0004 calls central. New fixture annotations/stamp.pdf carries real /AP artwork for a Stamp (undrawable: must be preserved and reported) beside a Square (drawable: must not be reported), so the reporting cannot pass by reporting everything. The stamp test was mutation-checked: it fails when the reporting line is removed. ADR 0003 and 0004 merge criteria are now ticked. 0003's were genuine paperwork - every box traced to an existing named test. 0004's were not, and its ADR now records what was missing rather than implying it always worked. TEST_TARGET=pdf 447 -> 451 passing. rustfmt and clippy -D warnings clean.
152 lines
6.8 KiB
Markdown
152 lines
6.8 KiB
Markdown
# ADR 0004: PDF annotation editing — typed edits on document-owned state
|
|
|
|
- **Status:** Accepted
|
|
- **Date:** 2026-07-28
|
|
- **Review item:** Phase 8, `DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md`
|
|
- **Supersedes:** nothing
|
|
- **Related:** ADR 0003 (incremental save), review phases 3 and 4
|
|
|
|
## Context
|
|
|
|
The gap analysis lists annotation editing twice:
|
|
|
|
> `PdfAnnotationEditing` — move, resize, restyle, rotate annotations —
|
|
> **MISSING**
|
|
|
|
Its prerequisites are "document model, appearance generation, incremental
|
|
save". All three now exist: the document model landed in Phase 3,
|
|
appearance generation with it, and incremental save in ADR 0003.
|
|
|
|
Annotations today are strictly read-only. `PdfAnnotation` exposes public
|
|
fields and `from_dict`, and there is not a single mutator or `&mut self`
|
|
method in the module. The viewer can find a link and report a click on it
|
|
(Phase 4), but nothing can move a highlight, restyle a square, or delete a
|
|
stamp.
|
|
|
|
This is chosen ahead of the other unblocked feature, **AcroForm full
|
|
support**, for one reason: the review lists that feature's prerequisites as
|
|
"document model, appearance regeneration, **JavaScript actions**". A
|
|
scripting engine is a large new dependency and a security surface in its own
|
|
right — running document-supplied code is exactly the class of decision that
|
|
deserves its own ADR and threat review, not a side effect of finishing a
|
|
form feature. Annotation editing needs nothing that does not already exist.
|
|
|
|
## Decision
|
|
|
|
Mirror the shape that already works for forms. `DocumentFormEditor` takes
|
|
typed edit commands, validates them, mutates document-owned state and
|
|
returns real errors; annotation editing gets the same contract rather than a
|
|
second, differently-shaped API.
|
|
|
|
### The editing model
|
|
|
|
`AnnotationEditor` applies typed `AnnotationEdit` values to a document-owned
|
|
`PageAnnotations` collection:
|
|
|
|
- `Move { to }` and `Resize { rect }` — geometry.
|
|
- `SetColor`, `SetBorderWidth`, `SetOpacity` — appearance.
|
|
- `SetContents` — the note text.
|
|
- `SetFlags` — hidden, print, read-only.
|
|
- `Delete` — removal.
|
|
|
|
Every edit is validated before anything changes, so a rejected edit leaves
|
|
the annotation untouched. This is the property the form editor already has
|
|
and the one that makes an editor safe to wire to a UI.
|
|
|
|
### Identity
|
|
|
|
Annotations are keyed by `ObjRef`, as fields are. A name or an index into a
|
|
`/Annots` array is not stable across a save, and Phase 3 already established
|
|
that page-0-style fallbacks are how the wrong object gets edited.
|
|
|
|
`PdfAnnotation` currently carries no `obj_ref`. It gains one, populated by
|
|
`PdfDocument::page_annotations`, which already resolves each entry through
|
|
the xref.
|
|
|
|
### Scope
|
|
|
|
**In scope:** move, resize, restyle, set contents, set flags, delete;
|
|
saving those edits through the ADR 0003 incremental writer; regenerating
|
|
appearance streams for the annotation types whose look this crate can
|
|
actually draw.
|
|
|
|
**Explicitly out of scope**, recorded rather than implied:
|
|
|
|
- **Creating** new annotations. Editing existing ones is the review's
|
|
wording, and creation needs a page `/Annots` rewrite plus placement
|
|
policy that belongs with the UI work.
|
|
- Appearance generation for types this crate cannot draw: Stamp, 3D,
|
|
Screen, Sound, Movie, Redact. An edit to one of these saves its
|
|
dictionary and **keeps the document's existing `/AP`** rather than
|
|
replacing it with something wrong.
|
|
- Redaction *application*. Marking a redact annotation is an edit; burning
|
|
it into the page content is a destructive operation needing its own
|
|
design.
|
|
- Rich text (`/RC`) and measurement annotations.
|
|
|
|
## Non-negotiable rules
|
|
|
|
1. **A rejected edit changes nothing.** Validated before mutation, as with
|
|
forms.
|
|
2. **Deleting an annotation removes it from the page's `/Annots`**, not just
|
|
from an in-memory list. A save that leaves a dangling reference produces
|
|
a file other readers reject.
|
|
3. **An annotation whose appearance cannot be generated keeps its existing
|
|
`/AP`.** Writing a blank appearance would erase a stamp's artwork.
|
|
4. **Read-only annotations refuse edits** unless the caller explicitly
|
|
overrides, mirroring the form editor's `ReadOnly` error.
|
|
5. **Geometry is validated.** A rectangle with zero or negative extent is
|
|
refused, not normalised silently: it usually means a UI bug upstream.
|
|
|
|
## Merge criteria
|
|
|
|
- [x] Move, resize, restyle, contents, flags and delete all apply and are
|
|
observable on the in-memory model.
|
|
- [x] Each is refused with a typed error where invalid, leaving state
|
|
untouched.
|
|
- [x] Edits save through the incremental writer and survive a reparse.
|
|
- [x] A deleted annotation is absent from the reparsed page's annotations.
|
|
- [x] An annotation type with no appearance generator keeps its original
|
|
`/AP` and is reported as skipped.
|
|
- [x] Corpus round trip on `annotations/links.pdf` and `hidden.pdf`.
|
|
- [x] Malformed corpus fixtures never panic an annotation edit or save.
|
|
- [x] `TEST_TARGET=pdf ./tools/test-rust-clean.sh` passes, rustfmt and
|
|
clippy `-D warnings` clean.
|
|
|
|
All criteria met, but not all of them were met when this ADR was written.
|
|
The audit that ticked these boxes found two genuine gaps rather than
|
|
paperwork:
|
|
|
|
1. **"reported as skipped" was never implemented for annotations.** Rule 3's
|
|
*preservation* worked — `EditableAnnotation::to_dict` clones the source
|
|
dictionary, so a stamp's `/AP` was always kept — but `SaveReport` had no
|
|
field for it. Only the form path reported skipped appearances. A caller
|
|
who moved a stamp was never told its artwork still showed the old
|
|
position. Closed by `SaveReport::annotation_appearances_skipped` and
|
|
`appearance_is_generated`, which enumerates the out-of-scope types
|
|
explicitly so a newly added `AnnotationType` fails to compile until it is
|
|
classified.
|
|
2. **`SetContents` and `SetFlags` had no round-trip test.** Both were
|
|
implemented and unit-tested against the in-memory model, but neither was
|
|
ever reparsed from written bytes — the assertion this ADR calls central.
|
|
|
|
New fixture `annotations/stamp.pdf` carries real `/AP` artwork for a Stamp
|
|
(undrawable, must be preserved and reported) alongside a Square (drawable,
|
|
must *not* be reported) so the reporting cannot pass by reporting
|
|
everything.
|
|
|
|
## Consequences
|
|
|
|
**Positive.** The viewer becomes an annotator. The save path generalises
|
|
from "form edits" to "document edits", which is the shape the remaining
|
|
Phase 8 features need. Phase 4's interaction layer gains something to call
|
|
beyond form fields.
|
|
|
|
**Negative.** Two editors now exist, for forms and annotations, over the
|
|
same document. They are deliberately kept separate because their validation
|
|
rules differ, but a future document-wide edit journal would subsume both.
|
|
|
|
**Risk.** The likeliest defect is a deleted annotation left referenced by
|
|
its page, producing a file this viewer reads and Acrobat rejects. The
|
|
mitigation is a round-trip test asserting the reference is gone from the
|
|
reparsed page, not merely from the model.
|