From 2b40413dc300884aaee6e5d71d44233dbe2be930 Mon Sep 17 00:00:00 2001 From: Grzegorz Martens Date: Sun, 25 Oct 2026 10:16:10 +0100 Subject: [PATCH] M6: calculation checks: an Acrobat widget name is a missing field, as nothing here resolves it found accepted price.0 (up to the field's last widget), but links, recalculate and the scripts' getField all take a name as the field or the fields under it, so a formula reading price.0 got no value and no order check while the page said nothing was wrong. found now looks names up the same way; a widget name is flagged as missing until all of them resolve it (from CodeRabbit's review). Co-Authored-By: Claude Opus 5.5 --- crates/forms/README.md | 5 +++-- crates/forms/src/calc_order.rs | 41 +++++++++++++--------------------- crates/forms/src/tests.rs | 8 +++++-- parity/acrobat-features.toml | 2 +- 4 files changed, 26 insertions(+), 30 deletions(-) diff --git a/crates/forms/README.md b/crates/forms/README.md index 6d576bb7ee..e3c5f7824b 100644 --- a/crates/forms/README.md +++ b/crates/forms/README.md @@ -92,8 +92,9 @@ let cells: Vec = grid.cells_for(names)?; // "Table1_1", "Table `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 - form doesn't have (`MissingField`; the field of that name, a field under it and a widget of one, - `price.0`, all count as there), or its script builds names at run time (`BuiltNames`, + form doesn't have (`MissingField`; the field of that name and a field under it count as there, + as recalculation and `getField` find them, but not an Acrobat widget name such as `price.0`, + which neither resolves), or its script builds names at run time (`BuiltNames`, `af::reads_built_names`), so its reads are incomplete. `CalculationOrder::fixed` is Fix order: a stable topological sort that keeps the current order wherever it can and leaves loops as they are. `set_calculation_order` writes `/CO` afresh. A field that gets a calculation joins the end of diff --git a/crates/forms/src/calc_order.rs b/crates/forms/src/calc_order.rs index 3b4a684824..cbafd296e2 100644 --- a/crates/forms/src/calc_order.rs +++ b/crates/forms/src/calc_order.rs @@ -161,7 +161,7 @@ pub fn calculation_order(doc: &Document, fields: &[Field]) -> CalculationOrder { .collect(); let links = links(calculated.iter().map(|(f, _)| f.name.as_str()), &reads); let component = components(&links); - let mut all: Vec<(&str, usize)> = fields.iter().map(|f| (f.name.as_str(), f.widgets.len())).collect(); + let mut all: Vec<&str> = fields.iter().map(|f| f.name.as_str()).collect(); all.sort_unstable(); let name = |k: usize| calculated.get(k).map(|(f, _)| f.name.clone()).unwrap_or_default(); let mut out = Vec::with_capacity(calculated.len()); @@ -193,22 +193,14 @@ pub fn calculation_order(doc: &Document, fields: &[Field]) -> CalculationOrder { CalculationOrder { fields: out, extra, fixed } } -/// Whether a formula's read `name` finds a field in `fields` (names with their widget counts, -/// sorted): the field of that name, a field under it (`row` for `row.1`), or one of a field's -/// widgets (`price.0`, as Acrobat's `getField` numbers them, up to its last widget). -fn found(fields: &[(&str, usize)], name: &str) -> bool { - // The most widgets a field called `n` has (a malformed form can have two such fields). - let widgets = |n: &str| { - let at = fields.partition_point(|(f, _)| *f < n); - fields.get(at..).unwrap_or_default().iter().take_while(|(f, _)| *f == n).map(|&(_, w)| w).max() - }; +/// Whether a formula's read `name` finds a field in `names` (sorted): the field of that name or a +/// field under it (`row` for `row.1`), as [`links`], [`crate::recalculate`] and the scripts' +/// `getField` look names up. Acrobat's widget names (`price.0`) aren't fields to any of them, so +/// a read of one gets no value and is missing too. +fn found(names: &[&str], name: &str) -> bool { let prefix = format!("{name}."); - let under = fields.partition_point(|(f, _)| *f < prefix.as_str()); - // A field with one widget, or none, is still `.0` (digits only: `parse` would take `+0`). - let widget = name.rsplit_once('.').is_some_and(|(base, i)| { - i.bytes().all(|b| b.is_ascii_digit()) && i.parse::().is_ok_and(|i| widgets(base).is_some_and(|w| i < w.max(1))) - }); - widgets(name).is_some() || fields.get(under).is_some_and(|(f, _)| f.starts_with(&prefix)) || widget + let under = names.partition_point(|n| *n < prefix.as_str()); + names.binary_search(&name).is_ok() || names.get(under).is_some_and(|n| n.starts_with(&prefix)) } /// For each calculated field (by its place in `names`), the places of the calculated fields it @@ -417,17 +409,16 @@ mod tests { } #[test] - fn a_read_name_finds_a_field_a_group_or_a_widget() { - let mut fields = vec![("price", 1), ("qty", 3), ("row.1", 1), ("row.2", 1), ("rows", 1), ("total", 1), ("bare", 0)]; - fields.sort_unstable(); - for name in ["price", "row", "row.1", "rows", "price.0", "qty.0", "qty.2", "bare", "bare.0"] { - assert!(found(&fields, name), "{name}"); + fn a_read_name_finds_a_field_or_a_group() { + let mut names = vec!["price", "row.1", "row.2", "rows", "total"]; + names.sort_unstable(); + for name in ["price", "row", "row.1", "rows"] { + assert!(found(&names, name), "{name}"); } - // Past a field's last widget there is no widget to find. - for name in ["pric", "ro", "row.3", "price.", "price.x", "price.-1", "price.+0", "price.1", "price.12", "qty.3", "bare.1", "total.0.1", ""] { - assert!(!found(&fields, name), "{name}"); + // A widget name finds nothing: recalculation and getField don't resolve it either. + for name in ["pric", "ro", "row.3", "price.", "price.0", "price.12", "total.0.1", ""] { + assert!(!found(&names, name), "{name}"); } - assert!(!found(&fields, "qty.99999999999999999999999"), "a number too big for usize is no widget"); } /// Two thousand formulas each reading a group of two thousand fields would be four million diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index ae73da1a7d..0909132a0f 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -1727,7 +1727,8 @@ fn missing_fields_and_built_names_are_flagged() { let calculate = |c: Calculate| FieldProps { calculate: Some(c), ..FieldProps::default() }; set_props(&mut doc, "sum", &calculate(Calculate::Simple { op: CalcOp::Sum, fields: vec!["qty".into(), "gone".into()] })).unwrap(); set_props(&mut doc, "notation", &calculate(Calculate::Notation("qty * nope".into()))).unwrap(); - // qty has one widget: qty.0 is it, and there is no qty.1. + // qty has one widget, but a widget name is no field: recalculation and getField give qty.0 + // nothing, so it is as missing as qty.1. let script = "event.value = getField(\"qty\").value + getField(\"ghost\").value + getField(\"qty.0\").value + getField(\"qty.1\").value;"; set_field_script(&mut doc, "script", "calculate", Some(script)).unwrap(); set_field_script(&mut doc, "built", "calculate", Some("event.value = getField(\"qty\").value + getField(\"row\" + 1).value;")).unwrap(); @@ -1740,7 +1741,10 @@ fn missing_fields_and_built_names_are_flagged() { vec![ ("sum".to_owned(), sum), ("notation".to_owned(), vec![MissingField { field: "nope".into() }]), - ("script".to_owned(), vec![MissingField { field: "ghost".into() }, MissingField { field: "qty.1".into() }]), + ( + "script".to_owned(), + vec![MissingField { field: "ghost".into() }, MissingField { field: "qty.0".into() }, MissingField { field: "qty.1".into() }], + ), ("built".to_owned(), vec![BuiltNames]), ] }; diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 952b70445c..3b7713a3da 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -4679,7 +4679,7 @@ milestone = "M6" status = "shipped" commands = ["form.field.properties", "form.calculation_order"] tools = ["form_set_props", "form_fill", "form_calculation_order"] -evidence = ["crates/forms/src/tests.rs::formats_validation_and_calculations_run_like_acrobat", "crates/forms/src/tests.rs::the_calculation_order_is_checked_fixed_and_kept", "crates/forms/src/tests.rs::a_calculation_order_with_gaps_and_extra_entries", "crates/forms/src/tests.rs::two_fields_with_one_name_keep_their_places_in_the_order", "crates/engine/src/tests.rs::setting_the_calculation_order_recalculates_and_undoes", "crates/automation/tests/it/automation.rs::the_calculation_order_is_read_fixed_and_set", "crates/ui-egui/tests/it/prepare.rs::the_calculation_order_is_moved_dragged_and_fixed", "crates/engine/src/tests.rs::checkbox_and_rank_changes_recalculate_ordered_dependents", "crates/engine/src/tests.rs::dynamic_calculation_reads_and_side_effects_reach_later_fields", "crates/forms/src/tests.rs::missing_fields_and_built_names_are_flagged", "crates/forms/src/calc_order.rs::a_read_name_finds_a_field_a_group_or_a_widget", "crates/forms/src/af/tests.rs::names_built_at_run_time_are_noticed", "crates/automation/tests/it/automation.rs::the_calculation_order_shows_formulas_and_what_they_cannot_read", "crates/ui-egui/tests/it/prepare.rs::a_formula_reading_a_missing_field_has_its_own_card"] +evidence = ["crates/forms/src/tests.rs::formats_validation_and_calculations_run_like_acrobat", "crates/forms/src/tests.rs::the_calculation_order_is_checked_fixed_and_kept", "crates/forms/src/tests.rs::a_calculation_order_with_gaps_and_extra_entries", "crates/forms/src/tests.rs::two_fields_with_one_name_keep_their_places_in_the_order", "crates/engine/src/tests.rs::setting_the_calculation_order_recalculates_and_undoes", "crates/automation/tests/it/automation.rs::the_calculation_order_is_read_fixed_and_set", "crates/ui-egui/tests/it/prepare.rs::the_calculation_order_is_moved_dragged_and_fixed", "crates/engine/src/tests.rs::checkbox_and_rank_changes_recalculate_ordered_dependents", "crates/engine/src/tests.rs::dynamic_calculation_reads_and_side_effects_reach_later_fields", "crates/forms/src/tests.rs::missing_fields_and_built_names_are_flagged", "crates/forms/src/calc_order.rs::a_read_name_finds_a_field_or_a_group", "crates/forms/src/af/tests.rs::names_built_at_run_time_are_noticed", "crates/automation/tests/it/automation.rs::the_calculation_order_shows_formulas_and_what_they_cannot_read", "crates/ui-egui/tests/it/prepare.rs::a_formula_reading_a_missing_field_has_its_own_card"] notes = "/CO order, then other calculated fields; recalculated after every fill and Clear form. Order editing: Prepare a form ▸ Calculation order lists the calculated fields in order with their formulas and values, Up/Down arrows (as Acrobat's Set Field Calculation Order dialog), Alt+Up/Down and drag, says which fields read a field calculated later, which read each other in a loop and which /CO leaves out, and Fix order moves only what must move; on the page it numbers each calculated field's step and draws arrows from the fields the focused field reads and to the fields that read it; form_calculation_order reads, sets, moves and fixes it. Each formula is also checked against the form: a read of a field the form doesn't have (a field deleted from under a sum, a typo in notation, a script's getField) and a script that builds the names it reads at run time are flagged, in form_calculation_order (which also gives each formula) and on the page: a red Missing field mark on the row and a card naming those formulas with a link to edit the first (no all-clear while one is broken), a grey note for built names, and both in the summary above the page. A new calculation joins the end of /CO and an edited one keeps its place. Acrobat's dialog is known from Adobe's help summary only, not yet observed." [[feature]]