From db26eb266d534bbc4cbfdc73296cba08f47cd9a1 Mon Sep 17 00:00:00 2001 From: cyfung1031 <44498510+cyfung1031@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:21:02 +0900 Subject: [PATCH] M14: Reuse form enumeration during document open (#1011) Co-authored-by: Brandon Thomas --- ROADMAP.md | 1 + crates/engine/src/lib.rs | 14 +++++----- crates/engine/src/tests.rs | 4 ++- crates/forms/README.md | 3 +++ crates/forms/src/lib.rs | 19 ++++++++++++-- crates/forms/src/tests.rs | 52 ++++++++++++++++++++++++++++++++++++++ crates/organize/src/lib.rs | 3 ++- 7 files changed, 86 insertions(+), 10 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index faa2d955..556fe492 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -146,6 +146,7 @@ Newest first: the date, what moved, and the new overall estimate. - **2026-10-11 (M13):** CLI run and MCP reject missing, empty, repeated and option-valued --root arguments before any tool/script execution or MCP initialization. A single-hyphen directory name remains valid; synthetic CLI regressions preserve a would-be overwritten file. Estimates unchanged. +- **2026-10-11 (M14, #307, Plan 2):** Forms now return terminal fields and the page-widget adoption count from one enumeration, and engine open reuses that result while retaining XFA script-button marking. The 12,000-page/no-form PDF (1,442,052 bytes) and 2,500-orphan-widget PDF (673,432 bytes) produce byte-identical `doc_open`/`doc_info` outputs against upstream `33c14e07cfd742a82bf03baa6b103c01008fa68c`, including the orphan repair note. After one warm-up per binary and three interleaved base/candidate rounds, median open times were 0.16/0.15 s and 0.08/0.07 s, with median peak RSS 62.69/59.98 MiB and 33.28/32.20 MiB. Workspace tests, Clippy, formatting, focused forms/engine/organize tests, and the WASM check pass. A behavior-preserving local binding in `organize` also lets the workspace Clippy gate pass. No estimate change. - **2026-10-10 (M14, interface size):** Preferences ▸ Interface size scales the whole interface (50–250 %, setting `ui_scale`, control option `ui-scale`); 50 % brings an interface the system draws at 200 % back to 100 % (#456). Auto, the default, follows the desktop's text scaling: on Linux and the BSDs the desktop app reads GNOME's `text-scaling-factor` once at launch, so with Large Text (1.25) on a 1× display the interface no longer draws 13 px labels that show their pixels next to the system's text. The size is egui's zoom factor, so `pixels_per_point` grows with it and page rasters stay one texel per screen pixel; egui's own ⌘+/⌘− interface zoom is turned off so those keys stay page zoom. Still missing: KDE's font DPI and Windows' "Make text bigger" aren't read (both scale only text there), and egui's text is still greyscale-antialiased without the system's gamma, so small text looks rougher than GTK's at 100 %. - **2026-10-10 (M14, #496):** Shortcut displays share a UI formatter across toolbar search, registry menus, command palette, Help and tooltips. Modifier and key names use a dedicated catalog context in every bundled translation; missing entries keep the English key name rather than a command translation. German shows Strg/Umschalt/Entf, macOS retains its symbols, and runtime language changes refresh the display. Regression coverage checks localized layout widths, English/German switching, platform conventions and unchanged command execution. Milestone and effort estimates unchanged. diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 26f14fe6..585c79c8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -2390,12 +2390,14 @@ impl Session { }; let display = display_bytes(editor.as_ref(), &bytes); let renderer = RenderPool::new(display.clone(), render_threads(), config.clone()); - let mut form = editor.as_ref().map(|e| pdfcraft_forms::fields(&e.cos)).unwrap_or_default(); - let mut adopted = 0; - if let Some(e) = editor.as_ref() { - xfa::mark_script_buttons(&e.cos, &mut form); - adopted = pdfcraft_forms::adopted_page_fields(&e.cos); - } + let (form, adopted) = editor + .as_ref() + .map(|e| { + let mut fields = pdfcraft_forms::fields_with_adopted(&e.cos); + xfa::mark_script_buttons(&e.cos, &mut fields.items); + (fields.items, fields.adopted_page_fields) + }) + .unwrap_or_default(); // Fields the file lists only as page widgets are adopted leniently; the file says so. if adopted > 0 && let Some(e) = &mut editor diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 9babc4fb..768e0ccb 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2400,6 +2400,8 @@ fn xfa_scripts_initialize_calculate_validate_toggle_and_add_rows() { let bytes = Arc::new(pdfcraft_xfa::fixtures::shell(&pdfcraft_xfa::fixtures::scripted_template())); let mut s = Session::new().with_clock(|| 1_700_000_000); let id = s.open("scripted.pdf", None, bytes, None).expect("opens"); + let add_row = s.get(id).unwrap().form.iter().find(|f| f.name == "addRow").expect("the XFA button is in the form snapshot"); + assert!(matches!(add_row.button.as_ref(), Some(pdfcraft_forms::af::ButtonAction::Script(_)))); let value = |s: &Session, n: &str| { s.get(id).unwrap().form.iter().find(|f| f.name == n).map(|f| f.value.clone()).unwrap_or_else(|| panic!("no field {n}")) }; @@ -2828,7 +2830,7 @@ fn adopting_page_only_fields_is_noted_as_a_repair() { let doc = s.get(id).unwrap(); let names: Vec<&str> = doc.form.iter().map(|f| f.name.as_str()).collect(); assert_eq!(names, ["alpha", "beta"]); - assert!(doc.repair_log().iter().any(|l| l.contains("2 fields") && l.contains("page annotations")), "{:?}", doc.repair_log()); + assert_eq!(doc.repair_log(), vec!["the form's /Fields list omits 2 fields; they were read from the page annotations".to_string()]); } /// A page too large for the renderer at the asked resolution exports at the most it allows (and diff --git a/crates/forms/README.md b/crates/forms/README.md index fa475fa7..9af549ad 100644 --- a/crates/forms/README.md +++ b/crates/forms/README.md @@ -7,6 +7,9 @@ Interactive forms (AcroForm, ISO 32000-2 §12.7). Layer L3; depends on `pdfcraft ```rust let all: Vec = fields(&doc); // terminal fields with widgets, options, flags, /DA +let snapshot = fields_with_adopted(&doc); // fields and page-only adoption count in one pass +let all = snapshot.items; +let adopted = snapshot.adopted_page_fields; set_value(&mut doc, "name", &FieldValue::Text("Ada".into()))?; set_value(&mut doc, "agree", &FieldValue::Check(true))?; set_value(&mut doc, "size", &FieldValue::Radio(Some("L".into())))?; diff --git a/crates/forms/src/lib.rs b/crates/forms/src/lib.rs index 4735474e..b8be06c3 100644 --- a/crates/forms/src/lib.rs +++ b/crates/forms/src/lib.rs @@ -487,13 +487,28 @@ struct Inherited { /// Every terminal field: the `/Fields` tree in order, then the fields reachable only through the /// page annotations (see [`adopt_page_fields`]). pub fn fields(doc: &Document) -> Vec { - enumerate(doc).0 + fields_with_adopted(doc).items } /// How many of [`fields`] were reachable only through the page annotations: the form's `/Fields` /// list names none or only some of them, so the leniency that adopts them is worth recording. pub fn adopted_page_fields(doc: &Document) -> usize { - enumerate(doc).1 + fields_with_adopted(doc).adopted_page_fields +} + +/// The terminal fields and page-only fields adopted while reading a form. +#[derive(Clone, Debug, PartialEq)] +pub struct Fields { + /// Terminal fields in tree order, followed by adopted page-only fields. + pub items: Vec, + /// How many fields were reachable only through page annotations. + pub adopted_page_fields: usize, +} + +/// Read all terminal fields and the page-only adoption count in one enumeration. +pub fn fields_with_adopted(doc: &Document) -> Fields { + let (items, adopted_page_fields) = enumerate(doc); + Fields { items, adopted_page_fields } } fn enumerate(doc: &Document) -> (Vec, usize) { diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index 354bbf5b..45c55e24 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -62,6 +62,20 @@ fn reopen(doc: &Document) -> Document { Document::open(Arc::new(bytes)).expect("reopens") } +fn legacy_two_call_enumeration(doc: &Document) -> (Vec, usize) { + (enumerate(doc).0, enumerate(doc).1) +} + +fn assert_combined_matches_legacy(doc: &Document, adopted: usize) { + let (old_fields, old_adopted) = legacy_two_call_enumeration(doc); + let combined = fields_with_adopted(doc); + assert_eq!(combined.items, old_fields); + assert_eq!(combined.adopted_page_fields, old_adopted); + assert_eq!(combined.adopted_page_fields, adopted); + assert_eq!(fields(doc), old_fields, "the compatibility wrapper keeps the old fields"); + assert_eq!(adopted_page_fields(doc), old_adopted, "the compatibility wrapper keeps the old count"); +} + fn field<'a>(all: &'a [Field], name: &str) -> &'a Field { all.iter().find(|f| f.name == name).unwrap_or_else(|| panic!("no field {name}")) } @@ -94,6 +108,7 @@ fn appearance_text(doc: &Document, w: &Widget, font_name: &[u8]) -> (Object, Vec #[test] fn the_field_tree_is_read_with_inheritance() { let doc = fixture(); + assert_combined_matches_legacy(&doc, 0); let all = fields(&doc); let names: Vec<&str> = all.iter().map(|f| f.name.as_str()).collect(); assert_eq!(names, ["name", "notes", "zip", "pin", "id", "address.city", "agree", "size", "country", "toppings", "go", "bare"]); @@ -158,6 +173,7 @@ fn fields_listed_only_on_the_pages_are_adopted() { "<< /Type /Annot /Subtype /Widget /Rect [400 700 415 715] /P 3 0 R >>".into(), // 15 no /FT, /T or /Parent: not a field ]; let mut doc = document(&objs); + assert_combined_matches_legacy(&doc, 3); let all = fields(&doc); let names: Vec<&str> = all.iter().map(|f| f.name.as_str()).collect(); assert_eq!(names, ["listed", "alpha", "beta", "pair"]); @@ -175,6 +191,7 @@ fn fields_listed_only_on_the_pages_are_adopted() { let mut objs = objs; objs[3] = "<< /DA (/Helv 0 Tf 0 g) /DR << /Font << /Helv 6 0 R >> >> >>".into(); let doc = document(&objs); + assert_combined_matches_legacy(&doc, 4); let all = fields(&doc); let names: Vec<&str> = all.iter().map(|f| f.name.as_str()).collect(); assert_eq!(names, ["listed", "alpha", "beta", "pair"]); @@ -499,10 +516,44 @@ fn reset_restores_defaults() { #[test] fn documents_without_forms_say_so() { let mut doc = Document::new_empty(); + assert_combined_matches_legacy(&doc, 0); assert!(fields(&doc).is_empty()); assert_eq!(set_value(&mut doc, "x", &FieldValue::Text("y".into())), Err(FormError::NoForm)); } +#[test] +fn combined_enumeration_keeps_page_order_across_pages() { + let objs: Vec = vec![ + "<< /Type /Catalog /Pages 2 0 R /AcroForm 5 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 /MediaBox [0 0 600 800] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [6 0 R] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [7 0 R] >>".into(), + "<< /Fields [6 0 R] >>".into(), + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (listed) /Rect [50 700 250 720] /P 3 0 R >>".into(), + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (adopted) /Rect [50 700 250 720] /P 4 0 R >>".into(), + ]; + let doc = document(&objs); + assert_combined_matches_legacy(&doc, 1); + + let combined = fields_with_adopted(&doc); + assert_eq!(combined.items.iter().map(|f| f.name.as_str()).collect::>(), ["listed", "adopted"]); + assert_eq!(combined.items.iter().map(|f| f.widgets[0].page).collect::>(), [Some(0), Some(1)]); +} + +#[test] +fn combined_enumeration_keeps_leniency_for_dangling_field_entries() { + let objs: Vec = vec![ + "<< /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 [] >>".into(), + "<< /Fields [99 0 R null 5 0 R] >>".into(), + "<< /FT /Tx /T (valid) >>".into(), + ]; + let doc = document(&objs); + assert_combined_matches_legacy(&doc, 0); + assert_eq!(fields_with_adopted(&doc).items.iter().map(|f| f.name.as_str()).collect::>(), ["valid"]); +} + #[test] fn every_field_type_can_be_added_named_and_drawn() { let mut doc = Document::new_empty(); @@ -1010,6 +1061,7 @@ fn field_actions_round_trip_on_every_trigger() { assert_eq!(field_actions(&doc, &name).unwrap(), [(Trigger::MouseExit, FieldAction::Named("NextPage".into()))]); // The Mouse Up script is what the button runs. set_field_actions(&mut doc, &name, &[(Trigger::MouseUp, FieldAction::JavaScript("this.print();".into()))]).unwrap(); + assert_combined_matches_legacy(&doc, 0); assert_eq!(fields(&doc).iter().find(|f| f.name == "go").unwrap().button, Some(af::ButtonAction::Named("Print".into()))); } diff --git a/crates/organize/src/lib.rs b/crates/organize/src/lib.rs index 3c6bf759..8f7bdac7 100644 --- a/crates/organize/src/lib.rs +++ b/crates/organize/src/lib.rs @@ -274,7 +274,8 @@ fn drop_named_destinations(doc: &mut Document, gone: &[ObjRef]) -> Result { - if prune_legacy(doc, &mut d, gone, &mut dead) { + let changed = prune_legacy(doc, &mut d, gone, &mut dead); + if changed { doc.update_dict(root, |c| c.set(b"Dests".to_vec(), Object::Dict(d)))?; } }