mirror of
https://github.com/storytold/pdfcraft.git
synced 2026-10-24 10:08:00 -06:00
fix(forms): cache field appearances drawn for display, redraw only what changed (#3470)
Review follow-up (CodeRabbit on #3475). A form edit refreshes the display copy on every keystroke, and with /NeedAppearances that redrew every field of the form: 68 ms per edit on a 1000-field form against 7 ms on main. FieldAppearanceCache keeps each widget's drawing with the objects it was drawn from: the widget and its field ancestors, the form dictionary and the fonts of its /DR, each with the objects its entries refer to. An entry is used only while each of them is still the same object (Arc::ptr_eq); the document replaces an object it edits, so a changed input is a miss, never a stale hit. The engine keeps one per editor. A field edit on that form now takes 13 ms. The repair note on a failed display write now names field appearances too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
b661ba4584
commit
da351cda8f
@@ -316,6 +316,10 @@ struct Editor {
|
||||
/// runs (content edits leave every annotation dictionary alone). Cleared on any refresh
|
||||
/// that may change annotations; see [`AppearanceCache`].
|
||||
appearances: AppearanceCache,
|
||||
/// Display appearances drawn for form fields the file leaves to the viewer (#3470), reused
|
||||
/// for each widget while the objects it was drawn from are unchanged; see
|
||||
/// [`pdfcraft_forms::FieldAppearanceCache`].
|
||||
field_appearances: pdfcraft_forms::FieldAppearanceCache,
|
||||
/// The text blocks last computed for one page, reused while the document is unchanged.
|
||||
text_blocks: TextBlocksCache,
|
||||
}
|
||||
@@ -882,13 +886,14 @@ fn is_placeholder_title(title: &str) -> bool {
|
||||
fn display_bytes(editor: Option<&mut Editor>, bytes: &Arc<Vec<u8>>) -> Arc<Vec<u8>> {
|
||||
let Some(e) = editor else { return bytes.clone() };
|
||||
let comments = pdfcraft_annot::with_missing_appearances_cached(&e.cos, &mut e.appearances);
|
||||
let Some(display) = with_field_appearances(comments, &e.cos) else {
|
||||
let fields = pdfcraft_forms::with_field_appearances_cached(comments.as_ref().unwrap_or(&e.cos), &mut e.field_appearances);
|
||||
let Some(display) = fields.or(comments) else {
|
||||
return bytes.clone();
|
||||
};
|
||||
match write_incremental(&display, &SaveOptions::default()) {
|
||||
Ok(written) => Arc::new(written),
|
||||
Err(err) => {
|
||||
e.cos.note_repair(format!("missing comment appearances could not be drawn ({err})"));
|
||||
e.cos.note_repair(format!("missing comment or field appearances could not be drawn ({err})"));
|
||||
bytes.clone()
|
||||
}
|
||||
}
|
||||
@@ -3576,6 +3581,7 @@ impl Session {
|
||||
redo: Vec::new(),
|
||||
keys,
|
||||
appearances: AppearanceCache::default(),
|
||||
field_appearances: pdfcraft_forms::FieldAppearanceCache::default(),
|
||||
text_blocks: TextBlocksCache::default(),
|
||||
}),
|
||||
None,
|
||||
|
||||
@@ -6160,6 +6160,11 @@ fn fields_left_to_the_viewer_are_displayed_from_their_values() {
|
||||
let n = widget("a").as_dict().and_then(|d| d.get(b"AP").and_then(|ap| saved.resolve(ap).as_dict().and_then(|ap| ap.reference(b"N")))).unwrap();
|
||||
let pdfcraft_cos::Object::Stream(n) = &*saved.get(n) else { panic!("A's appearance is a stream") };
|
||||
assert_eq!(n.decoded().unwrap().trim_ascii_end(), b"q\nQ", "A keeps its empty appearance");
|
||||
// Typing in one field redraws that field only, not the whole form, on each keystroke.
|
||||
let builds = |s: &Session| s.get(id).unwrap().editor.as_ref().unwrap().field_appearances.builds();
|
||||
let before = builds(&s);
|
||||
s.apply(id, Edit::SetFieldValue { name: "a".into(), value: pdfcraft_forms::FieldValue::Text("Typed".into()) }).unwrap();
|
||||
assert_eq!(builds(&s), before + 1);
|
||||
|
||||
// Without the flag, the empty appearance is the file's own, and only B is drawn.
|
||||
let id = s.open("form.pdf", None, Arc::new(pdf(false)), None).unwrap();
|
||||
|
||||
@@ -45,7 +45,9 @@ delete_field(&mut doc, "email")?;
|
||||
- Displaying a form that leaves its appearances to the viewer (`/NeedAppearances`, or a text or
|
||||
choice widget with no `/AP /N`): `with_field_appearances` returns a copy with those widgets
|
||||
drawn from their values, as typing them would draw them; the engine shows and prints that copy
|
||||
and saves the file as it was (#3470).
|
||||
and saves the file as it was (#3470). `with_field_appearances_cached` keeps each widget's
|
||||
drawing while the objects it was drawn from are the same (`FieldAppearanceCache`), so typing in
|
||||
one field redraws that field, not the form.
|
||||
- Check boxes and radio buttons switch `/V` and each widget's `/AS`. Widgets with no appearances
|
||||
get PdfCraft's own (a drawn check mark or dot; no symbol font needed).
|
||||
- Values are validated (options, `MaxLen`, single vs multi-select, NoToggleToOff, read-only) and a
|
||||
|
||||
+126
-1
@@ -1238,6 +1238,12 @@ pub fn need_appearances(doc: &Document) -> bool {
|
||||
/// and otherwise each widget without a normal appearance (`/AP /N`). Drawn as typing the value
|
||||
/// would draw it (#3470). `doc` is not changed: saving writes the file as it was.
|
||||
pub fn with_field_appearances(doc: &Document) -> Option<Document> {
|
||||
with_field_appearances_cached(doc, &mut FieldAppearanceCache::default())
|
||||
}
|
||||
|
||||
/// [`with_field_appearances`], reusing the appearances `cache` drew for widgets whose inputs are
|
||||
/// the same objects as then (see [`FieldAppearanceCache`]).
|
||||
pub fn with_field_appearances_cached(doc: &Document, cache: &mut FieldAppearanceCache) -> Option<Document> {
|
||||
let all = need_appearances(doc);
|
||||
let mut copy: Option<Document> = None;
|
||||
for f in fields(doc).iter().filter(|f| matches!(f.kind, FieldKind::Text | FieldKind::Combo | FieldKind::List)) {
|
||||
@@ -1247,7 +1253,15 @@ pub fn with_field_appearances(doc: &Document) -> Option<Document> {
|
||||
if drawn && !all {
|
||||
continue;
|
||||
}
|
||||
let (stream, _) = appearance::field_appearance_drawn(doc, f, w, &f.value, true);
|
||||
let inputs = appearance_inputs(doc, w);
|
||||
let stream = match cache.get(w.obj, &inputs) {
|
||||
Some(stream) => stream,
|
||||
None => {
|
||||
let (stream, _) = appearance::field_appearance_drawn(doc, f, w, &f.value, true);
|
||||
cache.insert(w.obj, inputs, &stream);
|
||||
stream
|
||||
}
|
||||
};
|
||||
let target = copy.get_or_insert_with(|| doc.clone());
|
||||
let ap = target.add(Object::Stream(stream));
|
||||
let mut fresh = Dict::new();
|
||||
@@ -1260,6 +1274,117 @@ pub fn with_field_appearances(doc: &Document) -> Option<Document> {
|
||||
copy
|
||||
}
|
||||
|
||||
/// The objects a widget's drawn appearance is built from, as the document holds them: the widget
|
||||
/// and the fields above it (value, `/DA`, `/Q`, flags, options, `/MK`, formatting actions), the
|
||||
/// form dictionary (`/DA`) and the fonts of its `/DR`, each with the objects its entries refer to
|
||||
/// (an indirect `/MK` or `/V`). Not the page: a field's rotation is its own `/MK /R`.
|
||||
fn appearance_inputs(doc: &Document, w: &Widget) -> Vec<std::sync::Arc<Object>> {
|
||||
// The objects a dictionary's entries refer to. The tree links and the drawn appearance itself
|
||||
// are not inputs, and `/Kids` and `/Fields` can be long.
|
||||
let referred = |d: &Dict, inputs: &mut Vec<std::sync::Arc<Object>>| {
|
||||
for (k, v) in d.iter() {
|
||||
if !matches!(&k[..], b"Kids" | b"Fields" | b"P" | b"Parent" | b"AP")
|
||||
&& let Some(r) = v.as_ref()
|
||||
{
|
||||
inputs.push(doc.get(r));
|
||||
}
|
||||
}
|
||||
};
|
||||
let mut inputs = Vec::new();
|
||||
let mut seen = std::collections::HashSet::new();
|
||||
let mut node = Some(w.obj);
|
||||
// The /Parent chain; the seen-set and the cap end a cycle or an absurd depth.
|
||||
while let Some(r) = node.filter(|r| seen.insert(*r) && seen.len() <= 64) {
|
||||
let obj = doc.get(r);
|
||||
if let Some(d) = obj.as_dict() {
|
||||
node = d.reference(b"Parent");
|
||||
referred(d, &mut inputs);
|
||||
} else {
|
||||
node = None;
|
||||
}
|
||||
inputs.push(obj);
|
||||
}
|
||||
let Some(root) = doc.root() else { return inputs };
|
||||
let catalog = doc.get(root);
|
||||
let form = match catalog.as_dict().and_then(|c| c.reference(b"AcroForm")) {
|
||||
Some(r) => doc.get(r),
|
||||
None => catalog,
|
||||
};
|
||||
if let Some(f) = form.as_dict() {
|
||||
referred(f, &mut inputs);
|
||||
// The fonts of /DR. An indirect /DR or /Font is listed through the entries that refer to
|
||||
// it; a direct one is part of the object that holds it. `held` only finds them: a copy
|
||||
// of a direct object is a new one each time, never an input.
|
||||
let held = |o: &Object| o.as_ref().map_or_else(|| std::sync::Arc::new(o.clone()), |r| doc.get(r));
|
||||
if let Some(dr) = f.get(b"DR").map(held)
|
||||
&& let Some(dr) = dr.as_dict()
|
||||
{
|
||||
referred(dr, &mut inputs);
|
||||
if let Some(font) = dr.get(b"Font").map(held)
|
||||
&& let Some(fonts) = font.as_dict()
|
||||
{
|
||||
referred(fonts, &mut inputs);
|
||||
}
|
||||
}
|
||||
}
|
||||
inputs.push(form);
|
||||
inputs
|
||||
}
|
||||
|
||||
/// Field appearances drawn for the display copy ([`with_field_appearances_cached`]), so a refresh
|
||||
/// redraws only the widgets whose inputs changed: a form that leaves every appearance to the
|
||||
/// viewer otherwise redrew all of its fields on each keystroke in any one of them (#3470). An
|
||||
/// entry holds the input objects it was drawn from and is used only while each is still the same
|
||||
/// object (`Arc::ptr_eq`); the document replaces an object it edits, so a changed input is a
|
||||
/// miss, never a stale hit.
|
||||
#[derive(Clone, Debug, Default)]
|
||||
pub struct FieldAppearanceCache {
|
||||
entries: std::collections::HashMap<ObjRef, (Vec<std::sync::Arc<Object>>, pdfcraft_cos::Stream)>,
|
||||
bytes: usize,
|
||||
hits: u64,
|
||||
builds: u64,
|
||||
}
|
||||
|
||||
impl FieldAppearanceCache {
|
||||
/// Cached appearance data restarts past this instead of growing without bound.
|
||||
const MAX_BYTES: usize = 64 << 20;
|
||||
|
||||
/// Forget every entry.
|
||||
pub fn clear(&mut self) {
|
||||
*self = Self::default();
|
||||
}
|
||||
|
||||
fn get(&mut self, w: ObjRef, inputs: &[std::sync::Arc<Object>]) -> Option<pdfcraft_cos::Stream> {
|
||||
let (from, stream) = self.entries.get(&w)?;
|
||||
if from.len() != inputs.len() || !from.iter().zip(inputs).all(|(a, b)| std::sync::Arc::ptr_eq(a, b)) {
|
||||
return None;
|
||||
}
|
||||
self.hits += 1;
|
||||
Some(stream.clone())
|
||||
}
|
||||
|
||||
fn insert(&mut self, w: ObjRef, inputs: Vec<std::sync::Arc<Object>>, stream: &pdfcraft_cos::Stream) {
|
||||
self.builds += 1;
|
||||
if self.bytes.saturating_add(stream.raw.len()) > Self::MAX_BYTES {
|
||||
self.clear();
|
||||
}
|
||||
self.bytes = self.bytes.saturating_add(stream.raw.len());
|
||||
if let Some((_, old)) = self.entries.insert(w, (inputs, stream.clone())) {
|
||||
self.bytes = self.bytes.saturating_sub(old.raw.len());
|
||||
}
|
||||
}
|
||||
|
||||
/// Cache hits so far (reused appearances); for tests.
|
||||
pub fn hits(&self) -> u64 {
|
||||
self.hits
|
||||
}
|
||||
|
||||
/// Appearances drawn so far; for tests.
|
||||
pub fn builds(&self) -> u64 {
|
||||
self.builds
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether the document's XMP metadata declares PDF/A conformance (`pdfaid:part`).
|
||||
pub fn declares_pdfa(doc: &Document) -> bool {
|
||||
let Some(root) = doc.root() else { return false };
|
||||
|
||||
@@ -2338,3 +2338,110 @@ fn without_need_appearances_only_missing_appearances_are_drawn() {
|
||||
assert!(ap(©, &field(&all, "g").widgets[0]).contains("(Value G) Tj"), "a down appearance alone is no normal one");
|
||||
assert!(with_field_appearances(©).is_none(), "every widget has one now");
|
||||
}
|
||||
|
||||
/// #3470: refreshing the display copy redraws only the widgets whose inputs changed. Typing in
|
||||
/// one field of a form that leaves every appearance to the viewer otherwise redrew all of them.
|
||||
#[test]
|
||||
fn the_field_appearance_cache_redraws_only_what_changed() {
|
||||
let mut doc = left_to_the_viewer(true);
|
||||
let mut cache = FieldAppearanceCache::default();
|
||||
with_field_appearances_cached(&doc, &mut cache).expect("fields to draw");
|
||||
let drawn = cache.builds();
|
||||
assert_eq!(drawn, 6, "a, b, c, d, f and g");
|
||||
with_field_appearances_cached(&doc, &mut cache).expect("fields to draw");
|
||||
assert_eq!((cache.builds(), cache.hits()), (drawn, drawn), "nothing changed, nothing redrawn");
|
||||
|
||||
set_value(&mut doc, "b", &FieldValue::Text("Typed".into())).unwrap();
|
||||
let copy = with_field_appearances_cached(&doc, &mut cache).expect("fields to draw");
|
||||
assert_eq!(cache.builds(), drawn + 1, "only b is redrawn");
|
||||
assert!(ap(©, &field(&fields(©), "b").widgets[0]).contains("(Typed) Tj"), "and shows its new value");
|
||||
assert!(ap(©, &field(&fields(©), "a").widgets[0]).contains("(Value A) Tj"), "a reuses its drawing");
|
||||
}
|
||||
|
||||
/// #3470: each kind of input a field's drawing depends on invalidates its cached appearance when
|
||||
/// it changes, wherever it lives: the parent field's value, the form's `/DA`, an indirect `/MK`
|
||||
/// and a `/DR` font. Field H is a parent with one kid widget; I is a plain field.
|
||||
#[test]
|
||||
fn the_field_appearance_cache_sees_every_input() {
|
||||
let mut doc = document(&[
|
||||
"<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(),
|
||||
"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(),
|
||||
"<< /Type /Page /Parent 2 0 R /Annots [6 0 R 7 0 R] >>".into(),
|
||||
"<< /Fields [5 0 R 7 0 R] /NeedAppearances true /DA (/Helv 12 Tf 0 g) /DR << /Font << /Helv 8 0 R >> >> >>".into(),
|
||||
"<< /FT /Tx /T (h) /V (Kid value) /Kids [6 0 R] >>".into(),
|
||||
"<< /Type /Annot /Subtype /Widget /Parent 5 0 R /Rect [50 700 350 720] /P 3 0 R /MK 9 0 R >>".into(),
|
||||
"<< /Type /Annot /Subtype /Widget /FT /Tx /T (i) /V (Plain) /Rect [50 650 350 670] /P 3 0 R >>".into(),
|
||||
"<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".into(),
|
||||
"<< /BG [1 1 1] >>".into(),
|
||||
]);
|
||||
let r = |n: u32| pdfcraft_cos::ObjRef::new(n, 0);
|
||||
let mut cache = FieldAppearanceCache::default();
|
||||
let mut redraw = |doc: &Document| {
|
||||
let before = cache.builds();
|
||||
let copy = with_field_appearances_cached(doc, &mut cache).expect("fields to draw");
|
||||
(cache.builds() - before, copy)
|
||||
};
|
||||
assert_eq!(redraw(&doc).0, 2);
|
||||
assert_eq!(redraw(&doc).0, 0, "unchanged");
|
||||
|
||||
doc.update_dict(r(5), |d| d.set(b"V".to_vec(), Object::String(PdfString::text("New value")))).unwrap();
|
||||
let (n, copy) = redraw(&doc);
|
||||
assert_eq!(n, 1, "the parent's value: only its kid");
|
||||
assert!(ap(©, &field(&fields(©), "h").widgets[0]).contains("(New value) Tj"));
|
||||
|
||||
doc.update_dict(r(9), |d| d.set(b"BG".to_vec(), Object::Array(vec![Object::Int(1), Object::Int(0), Object::Int(0)]))).unwrap();
|
||||
let (n, copy) = redraw(&doc);
|
||||
assert_eq!(n, 1, "the kid's indirect /MK");
|
||||
assert!(ap(©, &field(&fields(©), "h").widgets[0]).contains("1 0 0 rg"), "the new background");
|
||||
|
||||
doc.update_dict(r(4), |d| d.set(b"DA".to_vec(), Object::String(PdfString::text("/Helv 9 Tf 0 g")))).unwrap();
|
||||
let (n, copy) = redraw(&doc);
|
||||
assert_eq!(n, 2, "the form's /DA: both fields");
|
||||
assert!(ap(©, &field(&fields(©), "i").widgets[0]).contains("/Helv 9 Tf"));
|
||||
|
||||
doc.update_dict(r(8), |d| d.set(b"BaseFont".to_vec(), Object::Name(b"Courier".to_vec()))).unwrap();
|
||||
assert_eq!(redraw(&doc).0, 2, "a /DR font: both fields");
|
||||
}
|
||||
|
||||
/// #3470: an indirect `/DR` and an indirect `/Font` inside it are inputs too, with a font written
|
||||
/// directly into the `/Font` dictionary, and so is the form's `/DA` when it is an indirect string.
|
||||
#[test]
|
||||
fn the_field_appearance_cache_sees_indirect_resources() {
|
||||
let mut doc = document(&[
|
||||
"<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(),
|
||||
"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(),
|
||||
"<< /Type /Page /Parent 2 0 R /Annots [5 0 R] >>".into(),
|
||||
"<< /Fields [5 0 R] /NeedAppearances true /DA 8 0 R /DR 6 0 R >>".into(),
|
||||
"<< /Type /Annot /Subtype /Widget /FT /Tx /T (i) /V (Plain) /Rect [50 650 350 670] /P 3 0 R >>".into(),
|
||||
"<< /Font 7 0 R >>".into(),
|
||||
"<< /Helv << /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >> >>".into(),
|
||||
"(/Helv 12 Tf 0 g)".into(),
|
||||
]);
|
||||
let r = |n: u32| pdfcraft_cos::ObjRef::new(n, 0);
|
||||
let mut cache = FieldAppearanceCache::default();
|
||||
let mut redraw = |doc: &Document| {
|
||||
let before = cache.builds();
|
||||
with_field_appearances_cached(doc, &mut cache).expect("a field to draw");
|
||||
cache.builds() - before
|
||||
};
|
||||
assert_eq!(redraw(&doc), 1);
|
||||
assert_eq!(redraw(&doc), 0);
|
||||
let courier = || {
|
||||
let mut f = Dict::new();
|
||||
f.set(b"Type".to_vec(), Object::Name(b"Font".to_vec()));
|
||||
f.set(b"Subtype".to_vec(), Object::Name(b"Type1".to_vec()));
|
||||
f.set(b"BaseFont".to_vec(), Object::Name(b"Courier".to_vec()));
|
||||
Object::Dict(f)
|
||||
};
|
||||
doc.update_dict(r(7), |d| d.set(b"Helv".to_vec(), courier())).unwrap();
|
||||
assert_eq!(redraw(&doc), 1, "the indirect /Font dictionary");
|
||||
doc.update_dict(r(6), |d| {
|
||||
let mut fonts = Dict::new();
|
||||
fonts.set(b"Helv".to_vec(), courier());
|
||||
d.set(b"Font".to_vec(), Object::Dict(fonts));
|
||||
})
|
||||
.unwrap();
|
||||
assert_eq!(redraw(&doc), 1, "the indirect /DR");
|
||||
doc.set(r(8), Object::String(PdfString::text("/Helv 9 Tf 0 g")));
|
||||
assert_eq!(redraw(&doc), 1, "the form's indirect /DA");
|
||||
}
|
||||
|
||||
@@ -806,8 +806,8 @@ milestone = "M2"
|
||||
status = "shipped"
|
||||
commands = []
|
||||
tools = []
|
||||
evidence = ["crates/render/src/raster.rs::widget_appearance_states", "crates/render/src/raster.rs::highlights_without_an_appearance_are_drawn", "crates/engine/src/tests.rs::comments_without_appearances_are_drawn_but_not_saved", "crates/engine/src/tests.rs::cli_display_adds_missing_appearances_without_changing_the_source", "crates/engine/src/tests.rs::fields_left_to_the_viewer_are_displayed_from_their_values", "crates/forms/src/tests.rs::need_appearances_draws_every_text_and_choice_field_in_the_display_copy"]
|
||||
notes = "comments without /AP (FreeText, Ink, notes, stamps, shapes, markup) are drawn from their dictionaries in the page view, page_render and page images, including the CLI render command, never saved; text and choice fields are drawn from their values when the form sets /NeedAppearances (every one) or a widget has no /N (#3470), also display-only. Not cached: a form with 1000 such fields adds about 80 ms to each refresh (a read-only document still gets the renderer's Highlight fallback only); Link borders without /AP and printing are not covered yet"
|
||||
evidence = ["crates/render/src/raster.rs::widget_appearance_states", "crates/render/src/raster.rs::highlights_without_an_appearance_are_drawn", "crates/engine/src/tests.rs::comments_without_appearances_are_drawn_but_not_saved", "crates/engine/src/tests.rs::cli_display_adds_missing_appearances_without_changing_the_source", "crates/engine/src/tests.rs::fields_left_to_the_viewer_are_displayed_from_their_values", "crates/forms/src/tests.rs::need_appearances_draws_every_text_and_choice_field_in_the_display_copy", "crates/forms/src/tests.rs::the_field_appearance_cache_sees_every_input"]
|
||||
notes = "comments without /AP (FreeText, Ink, notes, stamps, shapes, markup) are drawn from their dictionaries in the page view, page_render and page images, including the CLI render command, never saved; text and choice fields are drawn from their values when the form sets /NeedAppearances (every one) or a widget has no /N (#3470), also display-only and cached per widget (a field edit on a 1000-field /NeedAppearances form takes ~13 ms against ~7 ms on a form that needs none) (a read-only document still gets the renderer's Highlight fallback only); Link borders without /AP and printing are not covered yet"
|
||||
|
||||
[[feature]]
|
||||
id = "view.render-error-isolation"
|
||||
|
||||
Reference in New Issue
Block a user