From 6a5020760e55728df0c31a46ce97d9d1829b3ca5 Mon Sep 17 00:00:00 2001 From: Grzegorz Martens Date: Fri, 23 Oct 2026 23:59:08 +0200 Subject: [PATCH] M6: calculation checks: formulas by field object, whole AFSimple lists only, no all-clear beside a broken formula - CalculatedField carries the field object: form_calculation_order and the Calculation order page find each row's formula and value by it, so two calculated fields sharing a name (a malformed form) each show their own. - An AFSimple_Calculate list counts as written out only when it ends the call: "row" + suffix and [...].concat(more) are names built at run time, and an array item counts only when it ends at a comma or the list's end, so ["row" + i] no longer reads, or renames, "row". - After a reorder, Undo is offered on the missing-field card instead of bringing back the all-clear beside it. From CodeRabbit's review. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BfMGHQNgdq5yf5tKZHGR3z --- crates/automation/src/forms.rs | 8 ++- crates/automation/tests/it/automation.rs | 37 ++++++++++++++ crates/forms/README.md | 3 +- crates/forms/src/af/calc.rs | 32 +++++++++--- crates/forms/src/af/tests.rs | 14 ++++++ crates/forms/src/calc_order.rs | 5 +- crates/forms/src/tests.rs | 6 +++ crates/ui-egui/src/tools/calc_order_ui.rs | 20 +++++--- crates/ui-egui/tests/it/prepare.rs | 61 +++++++++++++++++++++++ 9 files changed, 163 insertions(+), 23 deletions(-) diff --git a/crates/automation/src/forms.rs b/crates/automation/src/forms.rs index 1dec77bf03..d13f059a3f 100644 --- a/crates/automation/src/forms.rs +++ b/crates/automation/src/forms.rs @@ -667,11 +667,9 @@ impl Automation { CalculationIssue::NotInOrder => json!({ "issue": "not_in_order" }), CalculationIssue::BuiltNames => json!({ "issue": "built_names" }), }; + // By field object, not name: two calculated fields can share a name in a malformed form. let doc = self.doc(a)?; - let mut by_name: std::collections::HashMap<&str, &FormField> = std::collections::HashMap::with_capacity(doc.form.len()); - for f in doc.form.iter() { - by_name.entry(f.name.as_str()).or_insert(f); - } + let by_obj: std::collections::HashMap<_, &FormField> = doc.form.iter().map(|f| (f.obj, f)).collect(); out["calculation_order"] = json!(order.fields.iter().map(|f| f.name.as_str()).collect::>()); out["fields"] = order .fields @@ -679,7 +677,7 @@ impl Automation { .map(|f| { json!({ "name": f.name, - "formula": by_name.get(f.name.as_str()).and_then(|x| calculate_json(x)), + "formula": by_obj.get(&f.obj).and_then(|x| calculate_json(x)), "reads": f.reads, "listed": f.listed, "issues": f.issues.iter().map(issue).collect::>(), diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 22b5cd17e2..fd29cdce18 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -5058,6 +5058,43 @@ fn the_calculation_order_is_read_fixed_and_set() { assert_eq!((value(&mut a, "total"), value(&mut a, "tax")), (json!("0"), json!("0")), "undone with the values it recalculated"); } +/// A malformed form with two calculated fields named `dup`, one summing `src`, the other `x`. +fn two_calculations_with_one_name() -> Vec { + let widget = |name: &str, y: u32, reads: &str| { + format!( + "<< /Type /Annot /Subtype /Widget /FT /Tx /T ({name}) /Rect [50 {y} 250 {}] /P 3 0 R /AA << /C << /S /JavaScript /JS (AFSimple_Calculate\\(\"SUM\", \"{reads}\"\\);) >> >> >>", + y + 20 + ) + }; + pdf_of(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>", + "<< /Type /Page /Parent 2 0 R /Annots [5 0 R 6 0 R 7 0 R 8 0 R] >>", + "<< /Fields [5 0 R 6 0 R 7 0 R 8 0 R] /DA (/Helv 0 Tf 0 g) /CO [6 0 R 7 0 R 5 0 R] >>", + &widget("dup", 700, "x"), + &widget("dup", 660, "src"), + &widget("src", 620, "x"), + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (x) /Rect [50 580 250 600] /P 3 0 R /V (3) >>", + ]) +} + +/// Each row of the calculation order gets its own field's formula, even where two fields share a +/// name (a malformed form): the first dup sums src, the second x. +#[test] +fn the_calculation_order_gives_each_field_its_own_formula() { + let dir = workdir("calcdup"); + std::fs::write(dir.join("dup.pdf"), two_calculations_with_one_name()).unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "dup.pdf" }))["doc"].as_u64().unwrap(); + let r = ok(&mut a, "form_calculation_order", json!({ "doc": doc })); + assert_eq!(r["calculation_order"], json!(["dup", "src", "dup"])); + let formulas: Vec = r["fields"].as_array().unwrap().iter().map(|f| f["formula"].clone()).collect(); + assert_eq!( + formulas, + [json!({ "op": "sum", "fields": ["src"] }), json!({ "op": "sum", "fields": ["x"] }), json!({ "op": "sum", "fields": ["x"] })] + ); +} + /// The calculation order through the tool shows each formula, in form_set_props's shape or as a /// script, and flags a read of a field the form doesn't have and a script that builds the names /// it reads; form_fields shows a custom script too. diff --git a/crates/forms/README.md b/crates/forms/README.md index 3b5e46eee9..6d576bb7ee 100644 --- a/crates/forms/README.md +++ b/crates/forms/README.md @@ -87,7 +87,8 @@ let cells: Vec = grid.cells_for(names)?; // "Table1_1", "Table left alone. `calculations_changed` compares two readings of a form to say which formulas an edit rewrote. - The calculation order (`/CO`, ยง12.7.3): `calculation_order` lists the calculated fields in the - order `recalculate` runs them, with the fields each formula reads (`Calculate::reads`, + order `recalculate` runs them, each with its field object (two can share a name in a malformed + form), with the fields each formula reads (`Calculate::reads`, `af::calculation_reads`; names built at run time aren't known) and what is wrong with the order: a field reading one that calculates later, a loop, a field `/CO` leaves out, and `/CO` entries that calculate nothing. Two issues are about the formula itself: it reads a field the diff --git a/crates/forms/src/af/calc.rs b/crates/forms/src/af/calc.rs index 958b5d4d04..c6556476a6 100644 --- a/crates/forms/src/af/calc.rs +++ b/crates/forms/src/af/calc.rs @@ -277,12 +277,18 @@ fn calculation_sites(js: &str) -> Vec<(Range, Site)> { sites.extend(built(k)); continue; } + // One string of comma-separated names, when it is the whole list (not `"row" + suffix`). if let Some((range, list)) = string(k + 4) { - sites.push((range, Site::Commas(list))); + if punct(k + 5, ')') { + sites.push((range, Site::Commas(list))); + } else { + sites.extend(built(k)); + } continue; } - // `new Array("a", "b")` or `["a", "b"]`: its items, up to the first that isn't a string, - // which has to close the list for the list to be written out. Nothing else is read, so no + // `new Array("a", "b")` or `["a", "b"]`: each item that is one string, and the list has + // to end the call. An item that is more than a string (`"row" + i`), or anything after + // the list (`.concat(more)`), is a name built at run time. Nothing else is read, so no // token is read for two calls. let (first, close) = if punct(k + 4, '[') { (k + 5, ']') @@ -292,11 +298,21 @@ fn calculation_sites(js: &str) -> Vec<(Range, Site)> { sites.extend(built(k)); continue; }; - for (range, token) in tokens.iter().skip(first) { - match token { - Js::Str(name) => sites.push((range.clone(), Site::Literal(name.clone()))), - Js::Punct(',') => {} - Js::Punct(c) if *c == close => break, + let mut j = first; + loop { + match tokens.get(j) { + // An item ends at a comma or the list's end, or where a script cut short ends. + Some((range, Js::Str(name))) if punct(j + 1, ',') || punct(j + 1, close) || tokens.get(j + 1).is_none() => { + sites.push((range.clone(), Site::Literal(name.clone()))); + j += 1; + } + Some((_, Js::Punct(','))) => j += 1, + Some((_, Js::Punct(c))) if *c == close => { + if !punct(j + 1, ')') { + sites.extend(built(k)); + } + break; + } _ => { sites.extend(built(k)); break; diff --git a/crates/forms/src/af/tests.rs b/crates/forms/src/af/tests.rs index 5198fcc5ad..50b3029ada 100644 --- a/crates/forms/src/af/tests.rs +++ b/crates/forms/src/af/tests.rs @@ -168,9 +168,23 @@ fn names_built_at_run_time_are_noticed() { "AFSimple_Calculate(\"SUM\", [\"a\", prefix + \"b\"]);", "AFSimple_Calculate(\"SUM\", new Array(\"a\", \"b\" + c));", "AFSimple_Calculate(op, new Array(\"a\"));", + "AFSimple_Calculate(\"SUM\", \"row\" + suffix);", + "AFSimple_Calculate(\"SUM\", [\"a\"].concat(more));", + "AFSimple_Calculate(\"SUM\", [\"row\" + i]);", ] { assert!(reads_built_names(js), "{js}"); } + // A string that is only the start of a name or a list isn't read, or renamed, as one. + for (js, reads) in [ + ("AFSimple_Calculate(\"SUM\", \"row\" + suffix);", vec![]), + ("AFSimple_Calculate(\"SUM\", [\"row\" + i]);", vec![]), + ("AFSimple_Calculate(\"SUM\", [\"a\", \"row\" + i]);", vec!["a"]), + ("AFSimple_Calculate(\"SUM\", [\"a\", \"b\"].concat(more));", vec!["a", "b"]), + ] { + assert_eq!(parse_calculate(js), None, "{js} is a script"); + assert_eq!(calculation_reads(js), reads, "{js}"); + assert_eq!(rename_in_calculation(js, "row", "line"), None, "{js}"); + } for js in [ "event.value = getField(\"a\").value + getField('b').value;", "AFSimple_Calculate(\"SUM\", new Array(\"a\", \"b\"));", diff --git a/crates/forms/src/calc_order.rs b/crates/forms/src/calc_order.rs index c4a1b5bd37..ed69ac7c1b 100644 --- a/crates/forms/src/calc_order.rs +++ b/crates/forms/src/calc_order.rs @@ -17,6 +17,9 @@ const MAX_LINKS: usize = 1 << 20; pub struct CalculatedField { /// The field's full name. pub name: String, + /// The field object. Two calculated fields can share a name in a malformed form; this tells + /// them apart. + pub obj: ObjRef, /// The fields its formula reads, each once, as the formula names them: a name also stands for /// the fields under it. A script's names built at run time (`getField("row" + i)`) aren't here. pub reads: Vec, @@ -184,7 +187,7 @@ pub fn calculation_order(doc: &Document, fields: &[Field]) -> CalculationOrder { if f.actions.scripts.calculate.as_deref().is_some_and(crate::af::reads_built_names) { issues.push(CalculationIssue::BuiltNames); } - out.push(CalculatedField { name: f.name.clone(), reads, listed: *listed, issues }); + out.push(CalculatedField { name: f.name.clone(), obj: f.obj, reads, listed: *listed, issues }); } let fixed = fixed_order(&links, &component); CalculationOrder { fields: out, extra, fixed } diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index 6f848f9628..c9c01c0e13 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -1814,6 +1814,12 @@ fn two_fields_with_one_name_keep_their_places_in_the_order() { }; let row = |name: &str, reads: &str, ok: bool| (name.to_owned(), vec![reads.to_owned()], ok); assert_eq!(rows(&doc), [row("dup", "src", false), row("src", "x", true), row("dup", "x", true)]); + // Each row is its own field object, so a caller can find the right formula for each dup. + let all = fields(&doc); + for c in calculation_order(&doc, &all).fields { + let f = all.iter().find(|f| f.obj == c.obj).unwrap(); + assert_eq!((&f.name, f.actions.calculate.reads()), (&c.name, c.reads.clone())); + } let current: Vec = ["dup", "src", "dup"].map(String::from).into(); set_calculation_order(&mut doc, ¤t).unwrap(); assert_eq!(rows(&doc), [row("dup", "src", false), row("src", "x", true), row("dup", "x", true)], "the current order sets back unchanged"); diff --git a/crates/ui-egui/src/tools/calc_order_ui.rs b/crates/ui-egui/src/tools/calc_order_ui.rs index bc80983021..d2b952df3e 100644 --- a/crates/ui-egui/src/tools/calc_order_ui.rs +++ b/crates/ui-egui/src/tools/calc_order_ui.rs @@ -139,16 +139,14 @@ fn one_line(text: &str) -> String { text.split_whitespace().collect::>().join(" ").chars().take(200).collect() } -/// Each calculated field's value, formula and standing, in the order's order. +/// Each calculated field's value, formula and standing, in the order's order. Fields are found by +/// their object, not their name: two calculated fields can share a name in a malformed form. fn rows(calculated: &[CalculatedField], form: &[FormField]) -> Vec { - let mut by_name: HashMap<&str, &FormField> = HashMap::with_capacity(form.len()); - for f in form { - by_name.entry(f.name.as_str()).or_insert(f); - } + let by_obj: HashMap<_, &FormField> = form.iter().map(|f| (f.obj, f)).collect(); calculated .iter() .map(|c| { - let field = by_name.get(c.name.as_str()); + let field = by_obj.get(&c.obj); let value = match field { Some(f) if f.kind == FormFieldKind::Text => f.value.first().map(|v| format_value(&f.actions.format, v)).unwrap_or_default(), Some(f) => f.display_value(), @@ -451,6 +449,11 @@ fn cards(ui: &mut egui::Ui, t: &Tokens, pal: &Palette, order: &CalculationOrder, { act = Some(Act::EditFormula((*first).to_owned())); } + // The all-clear, which carries Undo after a reorder, stays hidden while a formula is + // broken, so Undo is offered here. + if undo && text_button(ui, tl!("Undo"), t) { + act = Some(Act::Undo); + } }); ui.add_space(6.0); } @@ -503,7 +506,7 @@ fn cards(ui: &mut egui::Ui, t: &Tokens, pal: &Palette, order: &CalculationOrder, ui.add(egui::Label::new(egui::RichText::new(tl!("Moves only what must move")).font(theme::regular(11.5)).color(t.text_muted)).wrap()); }); }); - } else if missing.is_empty() || undo { + } else if missing.is_empty() { let (fill, edge, colour) = pal.good; card(ui, fill, edge, |ui| { let said = if loops.is_empty() { @@ -965,7 +968,8 @@ mod tests { use super::*; fn field(name: &str, reads: &[&str], issues: Vec) -> CalculatedField { - CalculatedField { name: name.into(), reads: reads.iter().map(|r| (*r).to_owned()).collect(), listed: true, issues } + let obj = pdfcraft_cos::ObjRef { num: 0, generation: 0 }; + CalculatedField { name: name.into(), obj, reads: reads.iter().map(|r| (*r).to_owned()).collect(), listed: true, issues } } #[test] diff --git a/crates/ui-egui/tests/it/prepare.rs b/crates/ui-egui/tests/it/prepare.rs index d7bde224b4..3fad521e68 100644 --- a/crates/ui-egui/tests/it/prepare.rs +++ b/crates/ui-egui/tests/it/prepare.rs @@ -1354,6 +1354,56 @@ fn deleting_two_radio_buttons_of_a_group_at_once() { assert!(!names(&h).iter().any(|n| n == "shirt")); } +/// Two calculated fields that share a name (a malformed form) each show their own formula: the +/// first dup sums src, the second x. +#[test] +fn fields_sharing_a_name_each_show_their_own_formula() { + let widget = |name: &str, y: u32, reads: &str| { + format!( + "<< /Type /Annot /Subtype /Widget /FT /Tx /T ({name}) /Rect [50 {y} 250 {}] /P 3 0 R /AA << /C << /S /JavaScript /JS (AFSimple_Calculate\\(\"SUM\", \"{reads}\"\\);) >> >> >>", + y + 20 + ) + }; + 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 [5 0 R 6 0 R 7 0 R 8 0 R] >>".into(), + "<< /Fields [5 0 R 6 0 R 7 0 R 8 0 R] /DA (/Helv 0 Tf 0 g) /CO [6 0 R 7 0 R 5 0 R] >>".into(), + widget("dup", 700, "x"), + widget("dup", 660, "src"), + widget("src", 620, "x"), + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (x) /Rect [50 580 250 600] /P 3 0 R /V (3) >>".into(), + ]; + let mut bytes = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(bytes.len()); + bytes.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = bytes.len(); + bytes.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + bytes.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + bytes.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("dup.pdf", None, bytes.clone()).unwrap(); + app + }); + h.run_steps(4); + h.state_mut().execute("form.prepare"); + h.run_steps(3); + h.get_by_label("Calculation order").click(); + h.run_steps(3); + for (row, formula) in [("1. dup", "= sum(src)"), ("3. dup", "= sum(x)")] { + h.get_by_label_contains(row).hover(); + h.run_steps(30); + assert!(h.query_by_label_contains(formula).is_some(), "{row} shows {formula}"); + } +} + /// A formula that reads a field the form doesn't have gets a card of its own, in place of "every /// field calculates after the fields it reads", and its link opens that formula. A script that /// builds the names it reads gets a note. @@ -1383,6 +1433,17 @@ fn a_formula_reading_a_missing_field_has_its_own_card() { assert!(h.query_by_label("1 formula reads a field the form doesn't have").is_some()); assert!(h.query_by_label("sub reads discount, which the form doesn't have.").is_some()); assert!(h.query_by_label("Every field calculates after the fields it reads.").is_none(), "no all-clear while a formula is broken"); + // Reordered, the order can be undone from the missing-field card; the all-clear still stays away. + h.state_mut().apply_edit(Edit::SetCalculationOrder { names: vec!["rows".into(), "sub".into()] }); + h.run_steps(3); + assert!(h.query_by_label("Every field calculates after the fields it reads.").is_none(), "still no all-clear"); + h.get_by_label("Undo").click(); + h.run_steps(3); + let order: Vec = { + let s = h.state(); + s.session.get(s.views[0].id).unwrap().calculation_order().fields.into_iter().map(|f| f.name).collect() + }; + assert_eq!(order, ["sub", "rows"], "Undo put the order back"); h.get_by_label("Edit sub's formula").click(); h.run_steps(3); let draft = h.state().field_props.as_ref().map(|d| (d.field.clone(), d.tab));