From 81f38ae6182035f5f2e53dda9059777bb299325f Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:37:01 +0800 Subject: [PATCH 1/5] M6: list boxes show and pick the option /I names, and a single-select list shows one value Options are tracked by position as well as export value, so two options sharing an export value (ISO 32000-2 Table 234, /I) show and pick the right row; a single-select list given several values holds and shows the first; /I matches /V item for item, repeats included; values match options in linear time. Rebased onto main as one commit: main split the forms crate's lib.rs into read.rs and values.rs and moved forms_ui.rs into tools/. Folds the branch's four commits. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/automation/src/forms.rs | 3 + crates/automation/src/tools.rs | 2 +- crates/automation/tests/it/list_selection.rs | 81 +++++++ crates/automation/tests/it/main.rs | 1 + crates/forms/src/appearance.rs | 8 +- crates/forms/src/lib.rs | 61 ++++- crates/forms/src/read.rs | 87 ++++++- crates/forms/src/tests.rs | 224 +++++++++++++++++++ crates/forms/src/values.rs | 84 ++++--- crates/ui-egui/src/tools/forms_ui.rs | 48 ++-- crates/ui-egui/tests/it/forms.rs | 92 ++++++++ parity/acrobat-features.toml | 6 +- 12 files changed, 632 insertions(+), 65 deletions(-) create mode 100644 crates/automation/tests/it/list_selection.rs diff --git a/crates/automation/src/forms.rs b/crates/automation/src/forms.rs index adf0bd0884..fba7bb1b51 100644 --- a/crates/automation/src/forms.rs +++ b/crates/automation/src/forms.rs @@ -297,6 +297,9 @@ impl Automation { } FormFieldKind::Combo | FormFieldKind::List => { o.insert("options".into(), f.options.iter().map(|(e, d)| json!({ "value": e, "label": d })).collect()); + // The selected options as 1-based positions in `options`: two options can share + // an export value, and `value` alone does not tell them apart. + o.insert("selected".into(), json!(f.selected.iter().map(|i| i.saturating_add(1)).collect::>())); o.insert("multi_select".into(), json!(f.has(field_flags::MULTI_SELECT))); o.insert("editable".into(), json!(f.has(field_flags::EDIT))); } diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index c6c200cece..96f49814ce 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -391,7 +391,7 @@ pub fn tools() -> Vec { }), &["doc", "from", "to"], )), - t("form_fields", "List form fields", "Every interactive form field: name, type (text, checkbox, radio, combo, list, button, signature), value, options, page, rect (top-left-origin points) and rotation (0, 90, 180 or 270 degrees counterclockwise), read-only and required flags. A calculated field includes calculate: {op, fields} or {notation} as form_set_props takes them, or {script} for a custom Calculate script (set with form_set_script). Push buttons include caption (first widget's normal label) and captions [{widget, caption}] for all widgets (1-based).") + t("form_fields", "List form fields", "Every interactive form field: name, type (text, checkbox, radio, combo, list, button, signature), value, options, page, rect (top-left-origin points) and rotation (0, 90, 180 or 270 degrees counterclockwise), read-only and required flags. A calculated field includes calculate: {op, fields} or {notation} as form_set_props takes them, or {script} for a custom Calculate script (set with form_set_script). Push buttons include caption (first widget's normal label) and captions [{widget, caption}] for all widgets (1-based). Combo and list boxes include selected: the 1-based positions in options of the selected ones, which tells apart options that share an export value.") .ro() .with(schema(json!({ "doc": doc() }), &["doc"])), t( diff --git a/crates/automation/tests/it/list_selection.rs b/crates/automation/tests/it/list_selection.rs new file mode 100644 index 0000000000..2c2dc041ef --- /dev/null +++ b/crates/automation/tests/it/list_selection.rs @@ -0,0 +1,81 @@ +//! Which options a list box has selected, through the tool table (ISO 32000-2 Table 234, `/I`). + +use pdfcraft_automation::{Automation, Content}; +use serde_json::{Value, json}; + +fn tool(a: &mut Automation, name: &str, args: Value) -> Value { + match a.call(name, &args).unwrap().remove(0) { + Content::Json(value) => value, + other => panic!("expected JSON: {other:?}"), + } +} + +/// A PDF of the given objects (numbered from 1). +fn pdf_of(objs: &[&str]) -> Vec { + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + out +} + +fn open(test: &str) -> (Automation, u64, std::path::PathBuf) { + let dir = std::env::temp_dir().join(format!("pdfcraft-list-selection-{test}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let bytes = pdf_of(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R 6 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 6 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (D_dup) /Opt [[(1) (One-A)] [(1) (One-B)] [(2) (Two)]] /V (1) /I [1] /Rect [50 250 200 320] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (H_twovals) /Opt [[(TW) (Taiwan)] [(JP) (Japan)] [(US) (United States)]] /V [(TW) (JP)] /Rect [50 150 200 220] /P 3 0 R /F 4 >>", + ]); + std::fs::write(dir.join("choice.pdf"), bytes).unwrap(); + let mut a = Automation::new().with_root(&dir).unwrap(); + let id = tool(&mut a, "doc_open", json!({ "path": "choice.pdf" }))["doc"].as_u64().unwrap(); + (a, id, dir) +} + +fn field(a: &mut Automation, doc: u64, name: &str) -> Value { + let all = tool(a, "form_fields", json!({ "doc": doc })); + all["fields"].as_array().unwrap().iter().find(|f| f["name"] == name).unwrap().clone() +} + +/// Two options share the export value `1`: `selected` says which one `/I` picks, and filling the +/// field with an option's label picks that option. +#[test] +fn form_fields_reports_which_of_two_options_with_one_export_value_is_selected() { + let (mut a, doc, dir) = open("dup"); + let d = field(&mut a, doc, "D_dup"); + assert_eq!(d["value"], "1"); + assert_eq!(d["selected"], json!([2]), "One-B, the second option: {d}"); + tool(&mut a, "form_fill", json!({ "doc": doc, "values": { "D_dup": "One-A" } })); + let d = field(&mut a, doc, "D_dup"); + assert_eq!((d["value"].clone(), d["selected"].clone()), (json!("1"), json!([1])), "{d}"); + tool(&mut a, "form_fill", json!({ "doc": doc, "values": { "D_dup": "One-B" } })); + let d = field(&mut a, doc, "D_dup"); + assert_eq!(d["selected"], json!([2])); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A list box without MultiSelect given `/V [(TW) (JP)]` reports Taiwan alone (ISO 32000-2 +/// Table 233: "at most one item shall be selected"), and filling it writes one value. +#[test] +fn form_fields_reports_one_value_for_a_single_select_list_given_two() { + let (mut a, doc, dir) = open("twovals"); + let h = field(&mut a, doc, "H_twovals"); + assert_eq!((h["value"].clone(), h["selected"].clone()), (json!("TW"), json!([1])), "{h}"); + tool(&mut a, "form_fill", json!({ "doc": doc, "values": { "H_twovals": "United States" } })); + let h = field(&mut a, doc, "H_twovals"); + assert_eq!((h["value"].clone(), h["selected"].clone()), (json!("US"), json!([3])), "{h}"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/automation/tests/it/main.rs b/crates/automation/tests/it/main.rs index d36dbe93a9..3e0dcc822f 100644 --- a/crates/automation/tests/it/main.rs +++ b/crates/automation/tests/it/main.rs @@ -8,6 +8,7 @@ mod batch; mod button_caption; mod form_bulk; mod hostile_args; +mod list_selection; mod optimize_content_boundaries; mod redact_after_stray_q_form; mod redact_text_after_q; diff --git a/crates/forms/src/appearance.rs b/crates/forms/src/appearance.rs index c9ff86d17a..4780001ebb 100644 --- a/crates/forms/src/appearance.rs +++ b/crates/forms/src/appearance.rs @@ -814,11 +814,15 @@ fn field_appearance_fit(doc: &Document, f: &Field, w: &Widget, values: &[String] size = rows.size; let line = rows.row; let mut y = height - rows.inset; - for (export, display) in f.options.iter().skip(rows.top) { + // The field's own selection (its `/I`) when these are its values; values it doesn't + // hold yet select by export value alone. + let picked = if values == f.value.as_slice() { f.selected.clone() } else { crate::selected_options(&f.options, values, None) }; + for (i, (_, display)) in f.options.iter().enumerate().skip(rows.top) { if y - line < 0.0 { break; } - if values.contains(export) { + // Both sources list positions in ascending order. + if picked.binary_search(&i).is_ok() { c.push_str(&format!("0.6 0.75 0.86 rg\n{} {} {} {} re f\n", n(bw), n(y - line), n(width - 2.0 * bw), n(line))); } show(&mut body, pad, y - line + size * 0.25, display); diff --git a/crates/forms/src/lib.rs b/crates/forms/src/lib.rs index 775306d865..d6cf3a2012 100644 --- a/crates/forms/src/lib.rs +++ b/crates/forms/src/lib.rs @@ -42,10 +42,11 @@ mod values; pub use appearances::{FieldAppearanceCache, declares_pdfa, need_appearances, undrawn_fields, with_field_appearances, with_field_appearances_cached}; pub(crate) use read::page_refs; +pub use read::selected_options; pub use read::{Fields, acroform, adopted_page_fields, fields, fields_with_adopted}; #[cfg(test)] use read::{MAX_SCRIPT_BYTES, PageWalk, enumerate, form_reads}; -use read::{script, script_budget}; +use read::{first_by_export, script, script_budget}; pub use tab_order::{TabOrder, move_in_tab_order, set_tab_order, tab_orders}; #[cfg(test)] use values::has_password_flag; @@ -159,6 +160,10 @@ pub struct Field { pub max_len: Option, /// Choice options: (export value, display text). pub options: Vec<(String, String)>, + /// Combo and list boxes: the selected options, as ascending positions in `options` (see + /// [`selected_options`]). Two options can share an export value, so `value` alone can't say + /// which of them is selected. Empty for other kinds. + pub selected: Vec, /// Default appearance string (`/DA`, inherited from the form if absent). pub da: String, /// Quadding: 0 left, 1 centred, 2 right. @@ -229,18 +234,56 @@ impl Field { } } + /// Combo and list boxes: the selected options in the order the value names them (`/V` keeps + /// the order they were picked in), so the last is the latest pick. + pub fn picked_in_order(&self) -> Vec { + let mut out: Vec = Vec::with_capacity(self.selected.len()); + let mut taken: std::collections::HashSet = std::collections::HashSet::with_capacity(self.selected.len()); + let mut unpicked = self.selected_by_export(); + for v in &self.value { + if let Some(i) = unpicked.get_mut(v.as_str()).and_then(std::collections::VecDeque::pop_front) { + taken.insert(i); + out.push(i); + } + } + // Positions `/I` holds beyond what the value names (none while the two agree). + out.extend(self.selected.iter().copied().filter(|i| !taken.contains(i))); + out + } + /// The value as one string: text, the state name, or the selected display texts. pub fn display_value(&self) -> String { match self.kind { - FieldKind::Combo | FieldKind::List => self - .value - .iter() - .map(|v| self.options.iter().find(|(e, _)| e == v).map_or(v.clone(), |(_, d)| d.clone())) - .collect::>() - .join(", "), + FieldKind::Combo | FieldKind::List => { + // Each value as the selected option it names (options can share an export + // value), in the value's order; past those, the first option exporting it. + let mut unpicked = self.selected_by_export(); + let first = first_by_export(&self.options); + self.value + .iter() + .map(|v| { + let i = + unpicked.get_mut(v.as_str()).and_then(std::collections::VecDeque::pop_front).or_else(|| first.get(v.as_str()).copied()); + i.and_then(|i| self.options.get(i)).map_or(v.clone(), |(_, d)| d.clone()) + }) + .collect::>() + .join(", ") + } _ => self.value.join(", "), } } + + /// The selected positions grouped by the export value of their option, each group + /// ascending, so a value takes the next selected option exporting it in one lookup. + fn selected_by_export(&self) -> std::collections::HashMap<&str, std::collections::VecDeque> { + let mut out: std::collections::HashMap<&str, std::collections::VecDeque> = std::collections::HashMap::new(); + for &i in &self.selected { + if let Some((e, _)) = self.options.get(i) { + out.entry(e.as_str()).or_default().push_back(i); + } + } + out + } } /// A value to put in a field. @@ -253,6 +296,10 @@ pub enum FieldValue { Radio(Option), /// Combo box or list box: export values (several only for multi-select lists). Choice(Vec), + /// Combo box or list box: options by their zero-based position in `/Opt` (as `/I` numbers + /// them), which tells apart options that share an export value. Several only for + /// multi-select lists; `/V` takes them in this order. + ChoiceIndices(Vec), } // ── reading ───────────────────────────────────────────────────────────────────────────────── diff --git a/crates/forms/src/read.rs b/crates/forms/src/read.rs index e689c0e813..177e32fd1d 100644 --- a/crates/forms/src/read.rs +++ b/crates/forms/src/read.rs @@ -201,6 +201,69 @@ fn values_of(doc: &Document, o: Option<&Object>) -> Vec { } } +/// A choice field's `/I` (ISO 32000-2 Table 234) as option positions, when it is usable: an array +/// of integers "sorted in ascending order", each naming one of `count` options. `None` when it is +/// absent or anything in it is not (a non-integer, a position out of range, out of order or +/// repeated), so the field falls back to `/V`. +pub(crate) fn listed_indices(doc: &Document, d: &Dict, count: usize) -> Option> { + let listed = doc.resolve(d.get(b"I")?); + let entries = listed.as_array()?; + // Strictly ascending positions below `count` number at most `count`. + if entries.len() > count { + return None; + } + let mut out: Vec = Vec::with_capacity(entries.len()); + for e in entries { + let i = usize::try_from(doc.resolve(e).as_int()?).ok().filter(|&i| i < count)?; + if out.last().is_some_and(|&last| last >= i) { + return None; + } + out.push(i); + } + Some(out) +} + +/// The options a choice field shows selected, as ascending positions in `options`, given its +/// values (`/V`) and its usable `/I` ([`listed_indices`]). +/// +/// `/I` decides when it names the same export values as `/V`: it "shall be used when two or more +/// elements in the Opt array have different names but the same export value", and "if the items +/// identified by this entry differ from those in the V entry …, the V entry shall be used" (ISO +/// 32000-2 Table 234). Otherwise each value selects the first option with that export value, as +/// the writer numbers `/I` when it is given only an export value, so a pick keeps showing the same +/// option when `/I` is lost. A value no option exports selects nothing. +pub fn selected_options(options: &[(String, String)], values: &[String], listed: Option<&[usize]>) -> Vec { + if let Some(listed) = listed { + // The same items, each as many times: `/V [(1) (1)]` names two items, which an `/I [1]` + // naming one doesn't, so `/V` decides then. + let mut named: Vec<&str> = values.iter().map(String::as_str).collect(); + named.sort_unstable(); + let exports: Option> = listed.iter().map(|&i| options.get(i).map(|(e, _)| e.as_str())).collect(); + if let Some(mut exports) = exports { + exports.sort_unstable(); + if exports == named { + return listed.to_vec(); + } + } + } + let first = first_by_export(options); + let mut out: Vec = values.iter().filter_map(|v| first.get(v.as_str()).copied()).collect(); + out.sort_unstable(); + out.dedup(); + out +} + +/// Each export value in `options` with the position of the first option exporting it, so +/// matching values against options takes one pass over each rather than one over the options per +/// value (a file can hold many of both). +pub(crate) fn first_by_export(options: &[(String, String)]) -> std::collections::HashMap<&str, usize> { + let mut first: std::collections::HashMap<&str, usize> = std::collections::HashMap::with_capacity(options.len()); + for (i, (e, _)) in options.iter().enumerate() { + first.entry(e.as_str()).or_insert(i); + } + first +} + fn nums(doc: &Document, o: Option<&Object>) -> Option> { let o = doc.resolve(o?); o.as_array()?.iter().map(|x| doc.resolve(x).as_f64()).collect() @@ -544,16 +607,34 @@ fn walk( Object::Array(pair) if pair.len() >= 2 => Some((text_of(&doc.resolve(&pair[0]))?, text_of(&doc.resolve(&pair[1]))?)), other => text_of(other).map(|t| (t.clone(), t)), }) - .collect(); + .collect::>(); + let mut value = values_of(doc, inh.v.as_ref()); + // A list box without MultiSelect selects one option: "if clear, at most one item shall be + // selected" (ISO 32000-2 Table 233). Given an array /V or /DV anyway, it holds the first + // value only, and shows and resets to it; the file keeps its entries until the field is filled. + let mut default = values_of(doc, inh.dv.as_ref()); + let single = kind == FieldKind::List && ff & flags::MULTI_SELECT == 0; + if single { + value.truncate(1); + default.truncate(1); + } + let mut selected = match kind { + FieldKind::Combo | FieldKind::List => selected_options(&options, &value, listed_indices(doc, d, options.len()).as_deref()), + _ => Vec::new(), + }; + if single { + selected.truncate(1); + } out.push(Field { name: inh.name.clone(), obj: r, kind, - value: values_of(doc, inh.v.as_ref()), - default: values_of(doc, inh.dv.as_ref()), + value, + default, flags: ff, max_len: inh.max_len, options, + selected, da: inh.da.clone().unwrap_or_else(|| "/Helv 0 Tf 0 g".into()), quadding: inh.q.unwrap_or(0), tooltip: d.get(b"TU").and_then(|o| text_of(&doc.resolve(o))), diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index cca4cb4881..c76f934553 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -5485,3 +5485,227 @@ fn locked_button_export_edits_leave_the_document_unchanged() { assert_eq!(write_incremental(&doc, &SaveOptions::default()).unwrap(), before); } } + +/// A one-page form with list box `dup` (single select unless `ff` says otherwise) whose first two +/// options share the export value `1`, plus `extra` entries (`/V`, `/I`) on the field. +fn duplicate_exports(ff: u32, extra: &str) -> Document { + 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 [6 0 R] >>".into(), + "<< /Fields [6 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >>".into(), + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".into(), + format!( + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (dup) /Ff {ff} /Opt [[(1) (One-A)] [(1) (One-B)] [(2) (Two)]] {extra} /Rect [50 700 200 746] /P 3 0 R >>" + ), + ]; + document(&objs) +} + +/// The bottom edges of the highlighted rows in a list box appearance, top row first. +fn highlighted_rows(appearance: &str) -> Vec { + let lines: Vec<&str> = appearance.lines().collect(); + lines.windows(2).filter(|w| w[0] == "0.6 0.75 0.86 rg").map(|w| w[1].split(' ').nth(1).unwrap_or_default().to_string()).collect() +} + +/// The rows the display copy (`/NeedAppearances`) highlights in `dup`. +fn shown_rows(doc: &Document) -> Vec { + let copy = with_field_appearances(doc).expect("the form leaves its appearances to the viewer"); + highlighted_rows(&ap(©, &field(&fields(©), "dup").widgets[0])) +} + +/// ISO 32000-2 Table 234: `/I` "shall be used when two or more elements in the Opt array have +/// different names but the same export value". `/V (1)` with `/I [1]` selects One-B alone, not +/// every option exporting `1`. +#[test] +fn a_list_box_with_duplicate_export_values_selects_the_option_i_names() { + let doc = duplicate_exports(0, "/V (1) /I [1]"); + let dup = field(&fields(&doc), "dup").clone(); + assert_eq!(dup.selected, [1]); + assert_eq!(dup.display_value(), "One-B"); + let one_b = shown_rows(&doc); + assert_eq!(one_b.len(), 1, "one highlighted row"); + // The same field with /I [0] highlights the row above it. + let one_a = shown_rows(&duplicate_exports(0, "/V (1) /I [0]")); + assert_eq!(one_a.len(), 1); + assert_ne!(one_a, one_b, "One-A and One-B are different rows"); + let row = |s: &[String]| s.first().and_then(|y| y.parse::().ok()).unwrap(); + assert!(row(&one_b) < row(&one_a), "One-B is drawn below One-A"); +} + +/// Without a usable `/I`, `/V` decides (Table 234: "If the items identified by this entry differ +/// from those in the V entry …, the V entry shall be used"), and an export value two options +/// share selects the first of them. A malformed `/I` never panics. +#[test] +fn an_unusable_i_falls_back_to_the_first_option_with_the_value() { + for (extra, multi, want) in [ + ("/V (1)", 0, vec![0]), // no /I: the first option exporting 1 + ("/V (1) /I [5]", 0, vec![0]), // out of range + ("/V (1) /I [-1]", 0, vec![0]), // negative + ("/V (1) /I [(1)]", 0, vec![0]), // not an integer + ("/V (1) /I [1.5]", 0, vec![0]), // not a whole number + ("/V (1) /I 1", 0, vec![0]), // not an array + ("/V (1) /I [2]", 0, vec![0]), // names Two, which /V doesn't + ("/V (1) /I []", 0, vec![0]), // names nothing + ("/V [(1) (2)] /I [2 1]", 1 << 21, vec![0, 2]), // not ascending + ("/V [(1) (2)] /I [1 1 2]", 1 << 21, vec![0, 2]), // repeated + ("/V [(1) (2)] /I [1 2]", 1 << 21, vec![1, 2]), // usable: One-B and Two + ("/V [(1) (1)] /I [0 1]", 1 << 21, vec![0, 1]), // usable: both options exporting 1 + ("/V [(1) (1)] /I [1]", 1 << 21, vec![0]), // names one item where /V names two + ("/V [] /I [1]", 0, vec![]), // an empty value selects nothing + ("/V (9)", 0, vec![]), // no option exports 9 + ] { + let doc = duplicate_exports(multi, extra); + assert_eq!(field(&fields(&doc), "dup").selected, want, "{extra}"); + assert_eq!(shown_rows(&doc).len(), want.len(), "{extra}"); + } + // Pure: positions past the options or unlike the values are ignored. + let options = [("1".to_string(), "One-A".to_string()), ("1".to_string(), "One-B".to_string())]; + assert_eq!(selected_options(&options, &["1".into()], Some(&[1])), [1]); + assert_eq!(selected_options(&options, &["1".into()], Some(&[7])), [0]); + assert_eq!(selected_options(&options, &[], Some(&[])), Vec::::new()); +} + +/// Picking an option by position writes its export value to `/V` and its position to `/I`, so a +/// pick of One-A or One-B survives a save and reopen; picking by display text does the same. +#[test] +fn picking_one_of_two_options_with_the_same_export_value_round_trips() { + for (value, want) in [ + (FieldValue::ChoiceIndices(vec![0]), 0), + (FieldValue::ChoiceIndices(vec![1]), 1), + (FieldValue::Text("One-B".into()), 1), + (FieldValue::Text("One-A".into()), 0), + (FieldValue::Choice(vec!["1".into()]), 0), + ] { + let mut doc = duplicate_exports(0, "/V (1) /I [1]"); + set_value(&mut doc, "dup", &value).unwrap(); + let doc = reopen(&doc); + let dup = field(&fields(&doc), "dup").clone(); + let d = doc.get(dup.obj).as_dict().cloned().unwrap(); + assert_eq!(d.get(b"V").and_then(|v| v.as_string()).map(|s| s.to_text()).as_deref(), Some("1"), "{value:?}: /V is one string"); + let i: Vec = d.get(b"I").and_then(|i| i.as_array()).unwrap().iter().filter_map(Object::as_int).collect(); + assert_eq!(i, [want as i64], "{value:?}"); + assert_eq!(dup.selected, [want], "{value:?}"); + assert_eq!(highlighted_rows(&ap(&doc, &dup.widgets[0])).len(), 1, "{value:?}: its saved look highlights one row"); + } + // The saved look highlights the row picked, not the first with its value. + let saved = |pick: usize| { + let mut doc = duplicate_exports(0, ""); + set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![pick])).unwrap(); + highlighted_rows(&ap(&doc, &field(&fields(&doc), "dup").widgets[0])) + }; + assert_ne!(saved(0), saved(1)); + // Positions are checked: out of range, or several in a single-select list. + let mut doc = duplicate_exports(0, ""); + assert!(matches!(set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![3])), Err(FormError::Invalid(_)))); + assert!(matches!(set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![usize::MAX])), Err(FormError::Invalid(_)))); + assert!(matches!(set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![0, 1])), Err(FormError::Invalid(_)))); + // A multi-select list takes both options that share the value, in ascending /I. + let mut doc = duplicate_exports(1 << 21, ""); + set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![1, 0])).unwrap(); + let doc = reopen(&doc); + let dup = field(&fields(&doc), "dup").clone(); + assert_eq!(dup.value, ["1", "1"]); + assert_eq!(dup.selected, [0, 1]); + assert_eq!(dup.picked_in_order(), [0, 1]); + // A check box can't take positions. + let mut doc = fixture(); + assert!(matches!(set_value(&mut doc, "agree", &FieldValue::ChoiceIndices(vec![0])), Err(FormError::Invalid(_)))); +} + +/// A file can hold a list box with very many options and values. Matching values to options +/// takes one lookup per value, not a scan of the options (or of the selection) for each, so +/// reading and showing such a field stays fast. Two options share each export value, and every +/// result still names the first option exporting a value, or the next one selected. +#[test] +fn many_options_and_values_match_in_linear_time() { + const N: usize = 100_000; + let options: Vec<(String, String)> = (0..2 * N).map(|i| (format!("e{}", i / 2), format!("d{i}"))).collect(); + let values: Vec = (0..N).rev().map(|k| format!("e{k}")).collect(); + let started = std::time::Instant::now(); + let selected = selected_options(&options, &values, None); + assert_eq!(selected, (0..N).map(|k| 2 * k).collect::>(), "the first option exporting each value"); + let took = started.elapsed(); + assert!(took < std::time::Duration::from_secs(2), "selected_options took {took:?}"); + + let mut list = field(&fields(&fixture()), "toppings").clone(); + list.options = options; + list.value = values; + list.selected = (0..2 * N).collect(); + let started = std::time::Instant::now(); + let order = list.picked_in_order(); + let shown = list.display_value(); + let took = started.elapsed(); + assert!(took < std::time::Duration::from_secs(2), "picked_in_order and display_value took {took:?}"); + // The values name e{N-1} first: its first option, then down to e0; the second option of + // each value is selected without a value naming it, so it follows in ascending order. + assert_eq!(order.first(), Some(&(2 * (N - 1)))); + assert_eq!(order.get(N - 1), Some(&0)); + assert_eq!(order.get(N..), Some(&(0..N).map(|k| 2 * k + 1).collect::>()[..])); + assert!(shown.starts_with(&format!("d{}, d{}", 2 * (N - 1), 2 * (N - 2))), "{}", shown.get(..40).unwrap_or(&shown)); +} + +/// A one-page form with list box `pair` (single select unless `ff` says otherwise) of three +/// countries, plus `extra` entries (`/V`, `/DV`, `/I`) on the field. +fn countries(ff: u32, extra: &str) -> Document { + 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 [6 0 R] >>".into(), + "<< /Fields [6 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >>".into(), + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".into(), + format!( + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (pair) /Ff {ff} /Opt [[(TW) (Taiwan)] [(JP) (Japan)] [(US) (United States)]] {extra} /Rect [50 700 200 746] /P 3 0 R >>" + ), + ]; + document(&objs) +} + +/// ISO 32000-2 Table 233, MultiSelect: "if clear, at most one item shall be selected". A list box +/// without it given `/V [(TW) (JP)]` holds and shows Taiwan alone, and opening it changes nothing +/// in the file. +#[test] +fn a_single_select_list_given_several_values_shows_the_first() { + for (extra, multi, value, want) in [ + ("/V [(TW) (JP)]", 0, vec!["TW"], vec![0]), + ("/V [(JP) (TW)]", 0, vec!["JP"], vec![1]), + ("/V [(TW) (JP)] /I [0 1]", 0, vec!["TW"], vec![0]), // /I names more than the one value + ("/V [(XX) (JP)]", 0, vec!["XX"], vec![]), // the first value is no option + ("/V []", 0, vec![], vec![]), + ("/V [(TW) (JP)]", 1 << 21, vec!["TW", "JP"], vec![0, 1]), // multi-select keeps both + ] { + let doc = countries(multi, extra); + let pair = field(&fields(&doc), "pair").clone(); + assert_eq!(pair.value, value, "{extra}"); + assert_eq!(pair.selected, want, "{extra}"); + let copy = with_field_appearances(&doc).expect("left to the viewer"); + assert_eq!(highlighted_rows(&ap(©, &field(&fields(©), "pair").widgets[0])).len(), want.len(), "{extra}"); + // Reading and drawing the display copy leaves the field's own /V as it was. + let v = doc.get(pair.obj).as_dict().and_then(|d| d.get(b"V").cloned()).unwrap(); + assert!(matches!(v, Object::Array(_)), "{extra}: {v:?}"); + } + // Two options selected by /I alone in a single-select list: the first. + let doc = duplicate_exports(0, "/V (1) /I [0 1]"); + assert_eq!(field(&fields(&doc), "dup").selected, [0]); +} + +/// Picking a row in such a list writes one string to `/V` and that option's position to `/I`, +/// and Clear form goes back to the first value of an array `/DV` instead of refusing it. +#[test] +fn picking_in_a_single_select_list_given_several_values_writes_one() { + for (pick, export, index) in [(FieldValue::ChoiceIndices(vec![2]), "US", 2), (FieldValue::Choice(vec!["JP".into()]), "JP", 1)] { + let mut doc = countries(0, "/V [(TW) (JP)] /I [0 1]"); + set_value(&mut doc, "pair", &pick).unwrap(); + let doc = reopen(&doc); + let pair = field(&fields(&doc), "pair").clone(); + let d = doc.get(pair.obj).as_dict().cloned().unwrap(); + assert_eq!(d.get(b"V").and_then(|v| v.as_string()).map(|s| s.to_text()).as_deref(), Some(export), "{pick:?}: /V is one string"); + let i: Vec = d.get(b"I").and_then(|i| i.as_array()).unwrap().iter().filter_map(Object::as_int).collect(); + assert_eq!(i, [index], "{pick:?}"); + assert_eq!((pair.value, pair.selected), (vec![export.to_string()], vec![index as usize])); + } + let mut doc = countries(0, "/V (US) /DV [(JP) (TW)]"); + assert_eq!(reset(&mut doc, None).unwrap(), 1); + let pair = field(&fields(&doc), "pair").clone(); + assert_eq!((pair.value, pair.selected), (vec!["JP".to_string()], vec![1])); +} diff --git a/crates/forms/src/values.rs b/crates/forms/src/values.rs index e8615567b6..fc4263b345 100644 --- a/crates/forms/src/values.rs +++ b/crates/forms/src/values.rs @@ -1,5 +1,5 @@ use super::appearances::{declares_pdfa, need_appearances}; -use super::read::{acroform, field_flags, fields, page_refs}; +use super::read::{acroform, field_flags, fields, listed_indices, page_refs, selected_options}; use super::{ Dict, Document, Field, FieldEvent, FieldKind, FieldValue, FormError, NoScripts, ObjRef, Object, PdfString, Scripts, Widget, af, appearance, author, calc_order, flags, invalid, name_obj, scripting, @@ -133,7 +133,13 @@ fn share_value(doc: &mut Document, source: ObjRef, twin: &Field, value: &[String } d.remove(b"RV"); })?; - redraw(doc, twin, value, scripts) + // Drawn with the options the copied `/I` selects, not only the export values. + let mut shown = twin.clone(); + shown.value = value.to_vec(); + if matches!(twin.kind, FieldKind::Combo | FieldKind::List) { + shown.selected = selected_options(&twin.options, value, listed_indices(doc, &src, twin.options.len()).as_deref()); + } + redraw(doc, &shown, value, scripts) } FieldKind::PushButton | FieldKind::Signature => Ok(()), } @@ -237,7 +243,7 @@ pub(crate) fn write_value(doc: &mut Document, f: &Field, value: &FieldValue, scr } } FieldValue::Radio(None) => false, - FieldValue::Choice(_) => return invalid(format!("{:?} is a check box: use true or false", f.name)), + FieldValue::Choice(_) | FieldValue::ChoiceIndices(_) => return invalid(format!("{:?} is a check box: use true or false", f.name)), }; set_states(doc, f, on.then_some(state.as_str())) } @@ -262,37 +268,60 @@ pub(crate) fn write_value(doc: &mut Document, f: &Field, value: &FieldValue, scr } set_states(doc, f, choice.as_deref()) } - (FieldKind::Combo | FieldKind::List, FieldValue::Choice(_) | FieldValue::Text(_)) => { - let vals: Vec = match value { - FieldValue::Choice(v) => v.clone(), - FieldValue::Text(t) if t.is_empty() => Vec::new(), - FieldValue::Text(t) => vec![t.clone()], - other => return invalid(format!("{:?} can't take {other:?}", f.name)), + (FieldKind::Combo | FieldKind::List, FieldValue::Choice(_) | FieldValue::Text(_) | FieldValue::ChoiceIndices(_)) => { + let count = match value { + FieldValue::Choice(v) => v.len(), + FieldValue::ChoiceIndices(v) => v.len(), + _ => 1, }; - if vals.len() > 1 && !(f.kind == FieldKind::List && f.has(flags::MULTI_SELECT)) { + if count > 1 && !(f.kind == FieldKind::List && f.has(flags::MULTI_SELECT)) { return invalid(format!("{:?} takes a single value", f.name)); } - // Accept export values or display texts; free text only in editable combo boxes. + // Each value as its export value and, when it is one of the options, that option's + // position: two options can share an export value, and the position (`/I`) says + // which one is picked (ISO 32000-2 Table 234). let mut exports = Vec::new(); - for v in &vals { - match f.options.iter().find(|(e, d)| e == v || d == v) { - Some((e, _)) => exports.push(e.clone()), - None if f.kind == FieldKind::Combo && f.has(flags::EDIT) => exports.push(v.clone()), - None if f.options.is_empty() => exports.push(v.clone()), - None => { - let opts: Vec<&str> = f.options.iter().map(|(_, d)| d.as_str()).collect(); - return invalid(format!("{:?} has no option {v:?} (options: {})", f.name, opts.join(", "))); + let mut picked: Vec = Vec::new(); + match value { + FieldValue::ChoiceIndices(positions) => { + for &i in positions { + let Some((e, _)) = f.options.get(i) else { + return invalid(format!("{:?} has no option {} (it has {})", f.name, i.saturating_add(1), f.options.len())); + }; + exports.push(e.clone()); + picked.push(i); + } + } + _ => { + let vals: Vec = match value { + FieldValue::Choice(v) => v.clone(), + FieldValue::Text(t) if t.is_empty() => Vec::new(), + FieldValue::Text(t) => vec![t.clone()], + other => return invalid(format!("{:?} can't take {other:?}", f.name)), + }; + // Accept export values or display texts; free text only in editable combo boxes. + for v in &vals { + match f.options.iter().position(|(e, d)| e == v || d == v) { + Some(i) => { + exports.push(f.options.get(i).map(|(e, _)| e.clone()).unwrap_or_else(|| v.clone())); + picked.push(i); + } + None if f.kind == FieldKind::Combo && f.has(flags::EDIT) => exports.push(v.clone()), + None if f.options.is_empty() => exports.push(v.clone()), + None => { + let opts: Vec<&str> = f.options.iter().map(|(_, d)| d.as_str()).collect(); + return invalid(format!("{:?} has no option {v:?} (options: {})", f.name, opts.join(", "))); + } + } } } } - let mut indices: Vec = - exports.iter().filter_map(|e| f.options.iter().position(|(x, _)| x == e)).map(|i| Object::Int(i as i64)).collect(); - // /I lists option indices in ascending order; /V keeps the caller's value order. - // Every entry above is an integer, so this sorts by its numeric option index. - indices.sort_unstable_by_key(Object::as_int); + // /I lists option indices in ascending order, each once; /V keeps the caller's order. + picked.sort_unstable(); + picked.dedup(); + let indices: Vec = picked.iter().map(|&i| Object::Int(i64::try_from(i).unwrap_or(i64::MAX))).collect(); // A list box scrolls (`/TI`) so the picked options are among the rows it draws, as // in other viewers: a pick below the last visible row would otherwise not show (#1524). - let picked: Vec = indices.iter().filter_map(|i| i.as_int()).filter_map(|i| usize::try_from(i).ok()).collect(); let top = list_scroll(f, &picked, f.options.len()); // An empty value of its own keeps a parent field's /V from showing through. let shadow = exports.is_empty() && crate::author::ancestor_sets(doc, f.obj, b"V"); @@ -317,7 +346,10 @@ pub(crate) fn write_value(doc: &mut Document, f: &Field, value: &FieldValue, scr } })?; scroll_widgets(doc, f, &picked, f.options.len())?; - redraw(doc, f, &exports, scripts) + let mut shown = f.clone(); + shown.value = exports.clone(); + shown.selected = picked; + redraw(doc, &shown, &exports, scripts) } (FieldKind::PushButton, _) => invalid(format!("{:?} is a button; it has no value", f.name)), (FieldKind::Signature, _) => invalid(format!("{:?} is a signature field; sign it with Fill & Sign or a digital ID", f.name)), diff --git a/crates/ui-egui/src/tools/forms_ui.rs b/crates/ui-egui/src/tools/forms_ui.rs index 7232b1169b..1cf6a1b610 100644 --- a/crates/ui-egui/src/tools/forms_ui.rs +++ b/crates/ui-egui/src/tools/forms_ui.rs @@ -259,31 +259,33 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b let zoom = (r.height() as f64) / height_pt; let y_pt = (p.y - r.min.y) as f64 / zoom; let rows = w.list.unwrap_or(form_appearance::ListRows { top: 0, inset: 2.0, row: 12.0 * 1.15, size: 12.0 }); - let picked_indices: Vec = f.value.iter().filter_map(|v| f.options.iter().position(|(e, _)| e == v)).collect(); - let top = form_appearance::ListRows::top_showing(rows.top, &picked_indices, rows.visible(height_pt), f.options.len()); + let top = form_appearance::ListRows::top_showing(rows.top, &f.selected, rows.visible(height_pt), f.options.len()); if let Some(opt_idx) = rows.option_at(top, y_pt).filter(|&i| i < f.options.len()) { - let export = &f.options[opt_idx].0; let ctrl = ui.input(|i| i.modifiers.command || i.modifiers.ctrl); let shift = ui.input(|i| i.modifiers.shift); let multi = f.has(field_flags::MULTI_SELECT); - let new_value = if multi && ctrl { - let mut picked = f.value.clone(); - if let Some(pos) = picked.iter().position(|x| x == export) { + // Options by position: two options can share an export value (ISO 32000-2 + // Table 234, `/I`), and the click picks the row, not the first option with its value. + let in_order = f.picked_in_order(); + let picked: Vec = if multi && ctrl { + let mut picked = in_order.clone(); + if let Some(pos) = picked.iter().position(|&x| x == opt_idx) { picked.remove(pos); } else { - picked.push(export.clone()); + picked.push(opt_idx); } - FieldValue::Choice(picked) - } else if multi && shift && !f.value.is_empty() { - let anchor = f.value.last().and_then(|last| f.options.iter().position(|(e, _)| e == last)).unwrap_or(opt_idx); + picked + } else if multi + && shift + && let Some(&anchor) = in_order.last() + { let (min, max) = (anchor.min(opt_idx), anchor.max(opt_idx)); - let picked = f.options[min..=max].iter().map(|(e, _)| e.clone()).collect(); - FieldValue::Choice(picked) + (min..=max).collect() } else { - FieldValue::Choice(vec![export.clone()]) + vec![opt_idx] }; - if new_value != FieldValue::Choice(f.value.clone()) { - view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: new_value }); + if picked != in_order { + view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::ChoiceIndices(picked) }); } } } @@ -597,19 +599,19 @@ fn paint_unbaked_widget(painter: &egui::Painter, xf: &PageXform, f: &FormField, let zoom = (xf.rect.width() / unrotated_w.max(1.0)) as f64; let rows = w.list.unwrap_or(form_appearance::ListRows { top: 0, inset: 2.0, row: 12.0 * 1.15, size: 12.0 }); let height_pt = if matches!(w.rotation, 90 | 270) { (w.rect[2] - w.rect[0]).max(1.0) } else { (w.rect[3] - w.rect[1]).max(1.0) }; - let picked_indices: Vec = f.value.iter().filter_map(|v| f.options.iter().position(|(e, _)| e == v)).collect(); - let top = form_appearance::ListRows::top_showing(rows.top, &picked_indices, rows.visible(height_pt), f.options.len()); + let top = form_appearance::ListRows::top_showing(rows.top, &f.selected, rows.visible(height_pt), f.options.len()); let font_size = (rows.size * zoom).clamp(6.0, 64.0) as f32; let row_h = (rows.row * zoom) as f32; let inset = (rows.inset * zoom) as f32; let p = painter.with_clip_rect(r); let mut y = r.min.y + inset; - for (export, display) in f.options.iter().skip(top) { + for (i, (_, display)) in f.options.iter().enumerate().skip(top) { if y + row_h > r.max.y { break; } let row_rect = Rect::from_min_size(egui::pos2(r.min.x + 1.0, y), egui::vec2(r.width() - 2.0, row_h)); - if f.value.contains(export) { + // By position: options can share an export value. + if f.selected.contains(&i) { p.rect_filled(row_rect, CornerRadius::ZERO, Color32::from_rgb(0x99, 0xC0, 0xDC)); } p.text( @@ -924,11 +926,11 @@ pub(crate) fn overlay( } else if tab { next = Some(!shift); } else if (up || down) && !f.options.is_empty() { - let current_idx = f.value.last().and_then(|v| f.options.iter().position(|(e, _)| e == v)).unwrap_or(0); - let next_idx = if up { current_idx.saturating_sub(1) } else { (current_idx + 1).min(f.options.len().saturating_sub(1)) }; + let current_idx = f.picked_in_order().last().copied().unwrap_or(0); + let next_idx = if up { current_idx.saturating_sub(1) } else { current_idx.saturating_add(1).min(f.options.len().saturating_sub(1)) }; if next_idx != current_idx && next_idx < f.options.len() { - let export = f.options[next_idx].0.clone(); - edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::Choice(vec![export]) }); + // By position, so the arrows step through options that share an export value. + edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::ChoiceIndices(vec![next_idx]) }); } } if clicked_outside() { diff --git a/crates/ui-egui/tests/it/forms.rs b/crates/ui-egui/tests/it/forms.rs index fa828a8f9f..f089b33729 100644 --- a/crates/ui-egui/tests/it/forms.rs +++ b/crates/ui-egui/tests/it/forms.rs @@ -1294,6 +1294,98 @@ fn list_box_clicks_select_directly_and_ctrl_click_toggles_multi() { assert_eq!(value(&h, "ListMulti"), ["Tue", "Thu"], "Ctrl-clicking Thu toggled it on, keeping Tue"); } +/// The selected option positions of `name` (`/I`, or `/V` without a usable one). +fn selected(h: &Harness<'static, PdfCraftApp>, name: &str) -> Vec { + let s = h.state(); + s.session.get(s.views[0].id).unwrap().form.iter().find(|f| f.name == name).unwrap().selected.clone() +} + +/// ISO 32000-2 Table 234: `/I` tells apart options that share an export value. A list box whose +/// One-A and One-B both export `1` shows the one `/I` names, and a click picks the row clicked: +/// `/V (1)` with `/I` naming it, so the pick survives a save and reopen. +#[test] +fn a_list_box_click_picks_one_of_two_options_with_the_same_export_value() { + let bytes = pdf_from_objects(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (dup) /Opt [[(1) (One-A)] [(1) (One-B)] [(2) (Two)]] /V (1) /I [1] /Rect [50 250 200 320] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + ]); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("dup.pdf", None, bytes).unwrap(); + app.set_option("left", "closed").unwrap(); + app.set_option("zoom", "100").unwrap(); + app + }); + h.run_steps(4); + assert_eq!(selected(&h, "dup"), [1], "One-B is selected, not One-A too"); + + let r = { + let s = h.state(); + let doc = s.session.get(s.views[0].id).unwrap(); + let f = doc.form.iter().find(|f| f.name == "dup").unwrap(); + pdfcraft_ui_egui::forms_ui::field_screen_rect(&s.views[0], &doc.info, f, 0).expect("on screen") + }; + let zoom = r.height() / 70.0; + // Row 0 (One-A), then row 1 (One-B): each click changes the selection though /V stays (1). + click_at(&mut h, egui::pos2(r.center().x, r.min.y + 8.0 * zoom)); + assert_eq!(value(&h, "dup"), ["1"]); + assert_eq!(selected(&h, "dup"), [0], "clicking One-A picks One-A"); + click_at(&mut h, egui::pos2(r.center().x, r.min.y + 19.0 * zoom)); + assert_eq!(selected(&h, "dup"), [1], "clicking One-B picks One-B again"); + click_at(&mut h, egui::pos2(r.center().x, r.min.y + 8.0 * zoom)); + assert_eq!(selected(&h, "dup"), [0]); + + let dir = scratch("duplicate-exports"); + let out = dir.join("saved.pdf"); + h.state_mut().save_override = Some(out.to_string_lossy().into_owned()); + h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::S); + wait_for_file(&mut h, &out); + let mut app = PdfCraftApp::new(); + app.open_bytes("saved.pdf", None, std::fs::read(&out).unwrap()).unwrap(); + let reopened = app.session.get(app.views[0].id).unwrap().form.iter().find(|f| f.name == "dup").unwrap().clone(); + assert_eq!(reopened.value, ["1"], "/V is the one export value"); + assert_eq!(reopened.selected, [0], "/I keeps One-A through the save"); + let _ = std::fs::remove_dir_all(&dir); +} + +/// ISO 32000-2 Table 233: a list box without MultiSelect selects "at most one item". Given +/// `/V [(TW) (JP)]` it shows Taiwan alone, and a click writes the one option clicked. +#[test] +fn a_single_select_list_box_given_two_values_shows_and_takes_one() { + let bytes = pdf_from_objects(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (pair) /Opt [[(TW) (Taiwan)] [(JP) (Japan)] [(US) (United States)]] /V [(TW) (JP)] /Rect [50 250 200 320] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + ]); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("pair.pdf", None, bytes).unwrap(); + app.set_option("left", "closed").unwrap(); + app.set_option("zoom", "100").unwrap(); + app + }); + h.run_steps(4); + assert_eq!((value(&h, "pair"), selected(&h, "pair")), (vec!["TW".to_string()], vec![0]), "Taiwan alone"); + + let r = { + let s = h.state(); + let doc = s.session.get(s.views[0].id).unwrap(); + let f = doc.form.iter().find(|f| f.name == "pair").unwrap(); + pdfcraft_ui_egui::forms_ui::field_screen_rect(&s.views[0], &doc.info, f, 0).expect("on screen") + }; + let zoom = r.height() / 70.0; + // Row 1 (Japan), which /V also named: clicking it makes it the one value. + click_at(&mut h, egui::pos2(r.center().x, r.min.y + 19.0 * zoom)); + assert_eq!((value(&h, "pair"), selected(&h, "pair")), (vec!["JP".to_string()], vec![1])); +} + /// Auto-size (`0 Tf`, inherited from the AcroForm) single-line and multiline fields, and a fixed 10 pt one. fn auto_size_form() -> Vec { pdf_from_objects(&[ diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 0b6f658dd6..1f095bed92 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -4534,9 +4534,9 @@ tier = "P0" milestone = "M6" status = "shipped" commands = ["form.fields"] -tools = ["form_fill"] -evidence = ["crates/ui-egui/tests/it/forms.rs::check_boxes_radios_and_choices", "crates/forms/src/tests.rs::choices_accept_exports_or_display_text", "crates/ui-egui/tests/it/forms.rs::an_editable_combo_box_takes_a_typed_value", "crates/ui-egui/tests/it/forms.rs::a_combo_box_without_edit_takes_no_typing", "crates/automation/tests/it/automation.rs::form_fill_takes_free_text_only_in_an_editable_combo_box"] -notes = "combo and list boxes, multi-select lists; an editable combo box (Edit flag) has a text box over the field as well as its list, and takes typed text on the page and through form_fill; a combo box without Edit takes only its options. No type-ahead or spell check in the typed box" +tools = ["form_fill", "form_fields"] +evidence = ["crates/ui-egui/tests/it/forms.rs::check_boxes_radios_and_choices", "crates/forms/src/tests.rs::choices_accept_exports_or_display_text", "crates/ui-egui/tests/it/forms.rs::an_editable_combo_box_takes_a_typed_value", "crates/ui-egui/tests/it/forms.rs::a_combo_box_without_edit_takes_no_typing", "crates/automation/tests/it/automation.rs::form_fill_takes_free_text_only_in_an_editable_combo_box", "crates/forms/src/tests.rs::a_list_box_with_duplicate_export_values_selects_the_option_i_names", "crates/forms/src/tests.rs::an_unusable_i_falls_back_to_the_first_option_with_the_value", "crates/forms/src/tests.rs::picking_one_of_two_options_with_the_same_export_value_round_trips", "crates/ui-egui/tests/it/forms.rs::a_list_box_click_picks_one_of_two_options_with_the_same_export_value", "crates/automation/tests/it/list_selection.rs::form_fields_reports_which_of_two_options_with_one_export_value_is_selected", "crates/forms/src/tests.rs::a_single_select_list_given_several_values_shows_the_first", "crates/forms/src/tests.rs::picking_in_a_single_select_list_given_several_values_writes_one", "crates/ui-egui/tests/it/forms.rs::a_single_select_list_box_given_two_values_shows_and_takes_one", "crates/automation/tests/it/list_selection.rs::form_fields_reports_one_value_for_a_single_select_list_given_two"] +notes = "combo and list boxes, multi-select lists; an editable combo box (Edit flag) has a text box over the field as well as its list, and takes typed text on the page and through form_fill; a combo box without Edit takes only its options. a list box whose options share an export value shows and picks the one /I names (ISO 32000-2 Table 234): /I is used when it is integers, ascending, in range and names the same export values as /V, otherwise /V decides and each value selects the first option exporting it; a click or arrow key picks the row by position and writes its export value to /V and its position to /I, and form_fields reports selected (1-based positions); a list box without MultiSelect given an array /V or /DV holds, shows and resets to the first value only (Table 233), leaving the file as it is until the field is filled, when /V becomes one string and /I its position. No type-ahead or spell check in the typed box; a combo box whose options share an export value still shows the first of them" [[feature]] id = "form.push-buttons" From 3f708f8eb8e4b525b43061403d80fe04b36994c3 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:03:45 +0800 Subject: [PATCH 2/5] M6: list box picks survive same-name twins, repeats, Shift-click and an array /V - A same-name twin gets the copied /I mapped to its own options by export value and text, so options listed in another order keep the option picked; one with no match there leaves the twin selecting from /V, never part of /I. - A position picked twice is written once in /V and /I, so they still match item for item. - Shift-click extends from the row clicked last, kept in the view: with rows sharing an export value the value can't say which came last. - A single-select list loaded from an array /V writes one value when its shown row is clicked again. (review) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/forms/src/lib.rs | 4 ++ crates/forms/src/read.rs | 2 + crates/forms/src/tests.rs | 49 ++++++++++++++++ crates/forms/src/values.rs | 85 ++++++++++++++++++++++------ crates/ui-egui/src/tools/forms_ui.rs | 14 ++++- crates/ui-egui/tests/it/forms.rs | 83 +++++++++++++++++++++++++++ 6 files changed, 219 insertions(+), 18 deletions(-) diff --git a/crates/forms/src/lib.rs b/crates/forms/src/lib.rs index d6cf3a2012..0be2912518 100644 --- a/crates/forms/src/lib.rs +++ b/crates/forms/src/lib.rs @@ -164,6 +164,10 @@ pub struct Field { /// [`selected_options`]). Two options can share an export value, so `value` alone can't say /// which of them is selected. Empty for other kinds. pub selected: Vec, + /// A list box without MultiSelect whose `/V` is an array (a value of one option is a string, + /// ISO 32000-2 Table 233): `value` holds its first entry only, and picking that option again + /// writes the value as one string rather than leaving the array. + pub single_value_is_array: bool, /// Default appearance string (`/DA`, inherited from the form if absent). pub da: String, /// Quadding: 0 left, 1 centred, 2 right. diff --git a/crates/forms/src/read.rs b/crates/forms/src/read.rs index 177e32fd1d..64394b18ab 100644 --- a/crates/forms/src/read.rs +++ b/crates/forms/src/read.rs @@ -614,6 +614,7 @@ fn walk( // value only, and shows and resets to it; the file keeps its entries until the field is filled. let mut default = values_of(doc, inh.dv.as_ref()); let single = kind == FieldKind::List && ff & flags::MULTI_SELECT == 0; + let single_value_is_array = single && inh.v.as_ref().is_some_and(|v| doc.resolve(v).as_array().is_some()); if single { value.truncate(1); default.truncate(1); @@ -635,6 +636,7 @@ fn walk( max_len: inh.max_len, options, selected, + single_value_is_array, da: inh.da.clone().unwrap_or_else(|| "/Helv 0 Tf 0 g".into()), quadding: inh.q.unwrap_or(0), tooltip: d.get(b"TU").and_then(|o| text_of(&doc.resolve(o))), diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index c76f934553..dbd2016a90 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -5709,3 +5709,52 @@ fn picking_in_a_single_select_list_given_several_values_writes_one() { let pair = field(&fields(&doc), "pair").clone(); assert_eq!((pair.value, pair.selected), (vec!["JP".to_string()], vec![1])); } + +/// A position picked twice is written once in both `/V` and `/I`, so they still match item for +/// item and the option picked (the second of two sharing an export value) comes back on reopen. +#[test] +fn a_position_picked_twice_is_written_once_in_v_and_i() { + let mut doc = duplicate_exports(1 << 21, ""); + set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![1, 1])).unwrap(); + let dup = field(&fields(&doc), "dup").clone(); + let d = doc.get(dup.obj).as_dict().cloned().unwrap(); + let v: Vec = match d.get(b"V") { + Some(Object::Array(a)) => a.iter().filter_map(|o| o.as_string().map(|s| s.to_text())).collect(), + Some(o) => o.as_string().map(|s| vec![s.to_text()]).unwrap_or_default(), + None => Vec::new(), + }; + let i: Vec = d.get(b"I").and_then(|i| i.as_array()).map(|a| a.iter().filter_map(Object::as_int).collect()).unwrap_or_default(); + assert_eq!((v, i), (vec!["1".to_string()], vec![1]), "one entry each"); + assert_eq!(dup.selected, [1], "One-B, not One-A"); +} + +/// A same-name twin listing the options sharing an export value in another order is given the +/// option picked, not the one at the same position: `/I` is mapped to the twin's own options. +#[test] +fn a_twin_with_options_in_another_order_gets_the_option_picked() { + 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 [6 0 R 7 0 R] >>".into(), + "<< /Fields [6 0 R 7 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >>".into(), + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".into(), + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (dup) /Ff 0 /Opt [[(1) (One-A)] [(1) (One-B)] [(2) (Two)]] /Rect [50 700 200 746] /P 3 0 R >>" + .into(), + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (dup) /Ff 0 /Opt [[(1) (One-B)] [(1) (One-A)] [(2) (Two)]] /Rect [50 600 200 646] /P 3 0 R >>" + .into(), + ]; + let mut doc = document(&objs); + set_value(&mut doc, "dup", &FieldValue::ChoiceIndices(vec![1])).unwrap(); + let all = fields(&doc); + let twin = all.iter().find(|f| f.obj == pdfcraft_cos::ObjRef::new(7, 0)).unwrap(); + let i: Vec = doc + .get(twin.obj) + .as_dict() + .and_then(|d| d.get(b"I").and_then(|i| i.as_array().cloned())) + .unwrap_or_default() + .iter() + .filter_map(Object::as_int) + .collect(); + assert_eq!(i, [0], "One-B is first in the twin's options"); + assert_eq!(twin.selected, [0]); +} diff --git a/crates/forms/src/values.rs b/crates/forms/src/values.rs index fc4263b345..6996549520 100644 --- a/crates/forms/src/values.rs +++ b/crates/forms/src/values.rs @@ -102,15 +102,21 @@ fn fill(doc: &mut Document, all: &[Field], named: &[usize], name: &str, value: & if !twins.is_empty() { let taken = fields(doc).into_iter().find(|x| x.obj == f.obj).map(|x| x.value).unwrap_or_default(); for t in twins { - share_value(doc, f.obj, t, &taken, scripts)?; + share_value(doc, (f.obj, &f.options), t, &taken, scripts)?; } } Ok(()) } -/// Give `twin` the value `source` holds: its `/V` (and a choice's `/I`), drawn in the twin's own -/// widgets. -fn share_value(doc: &mut Document, source: ObjRef, twin: &Field, value: &[String], scripts: &mut dyn Scripts) -> Result<(), FormError> { +/// Give `twin` the value `source` (a field and its options) holds: its `/V`, and a choice's `/I` +/// mapped to the twin's own options, drawn in the twin's own widgets. +fn share_value( + doc: &mut Document, + (source, source_options): (ObjRef, &[(String, String)]), + twin: &Field, + value: &[String], + scripts: &mut dyn Scripts, +) -> Result<(), FormError> { match twin.kind { FieldKind::CheckBox => { let state = twin.widgets.iter().find_map(|w| w.on_state.clone()).unwrap_or_else(|| "Yes".into()); @@ -122,22 +128,35 @@ fn share_value(doc: &mut Document, source: ObjRef, twin: &Field, value: &[String } FieldKind::Text | FieldKind::Combo | FieldKind::List => { let src = doc.get(source).as_dict().cloned().unwrap_or_default(); + // `/I` holds positions in the source's options: each goes to the twin's first unused option + // with the same export value and text, its options perhaps in another order. One with no + // match leaves the twin no `/I` (its `/V` picks), never part of one. + let positions = match twin.kind { + FieldKind::Combo | FieldKind::List => { + listed_indices(doc, &src, source_options.len()).and_then(|picked| twin_positions(&picked, source_options, &twin.options)) + } + _ => None, + }; doc.update_dict(twin.obj, |d| { - for key in [&b"V"[..], b"I"] { - match src.get(key) { - Some(v) => d.set(key.to_vec(), v.clone()), - None => { - d.remove(key); - } + match src.get(b"V") { + Some(v) => d.set(b"V".to_vec(), v.clone()), + None => { + d.remove(b"V"); + } + } + match &positions { + Some(p) => d.set(b"I".to_vec(), Object::Array(p.iter().map(|&i| Object::Int(i64::try_from(i).unwrap_or(i64::MAX))).collect())), + None => { + d.remove(b"I"); } } d.remove(b"RV"); })?; - // Drawn with the options the copied `/I` selects, not only the export values. + // Drawn with the options that `/I` selects, not only the export values. let mut shown = twin.clone(); shown.value = value.to_vec(); if matches!(twin.kind, FieldKind::Combo | FieldKind::List) { - shown.selected = selected_options(&twin.options, value, listed_indices(doc, &src, twin.options.len()).as_deref()); + shown.selected = selected_options(&twin.options, value, positions.as_deref()); } redraw(doc, &shown, value, scripts) } @@ -282,14 +301,19 @@ pub(crate) fn write_value(doc: &mut Document, f: &Field, value: &FieldValue, scr // which one is picked (ISO 32000-2 Table 234). let mut exports = Vec::new(); let mut picked: Vec = Vec::new(); + // Each option once in both `/V` and `/I`: a reader that finds them differ (a repeat in one) + // goes by the export values alone and may pick another option sharing one. + let mut seen = vec![false; f.options.len()]; match value { FieldValue::ChoiceIndices(positions) => { for &i in positions { let Some((e, _)) = f.options.get(i) else { return invalid(format!("{:?} has no option {} (it has {})", f.name, i.saturating_add(1), f.options.len())); }; - exports.push(e.clone()); - picked.push(i); + if first_pick(&mut seen, i) { + exports.push(e.clone()); + picked.push(i); + } } } _ => { @@ -303,8 +327,10 @@ pub(crate) fn write_value(doc: &mut Document, f: &Field, value: &FieldValue, scr for v in &vals { match f.options.iter().position(|(e, d)| e == v || d == v) { Some(i) => { - exports.push(f.options.get(i).map(|(e, _)| e.clone()).unwrap_or_else(|| v.clone())); - picked.push(i); + if first_pick(&mut seen, i) { + exports.push(f.options.get(i).map(|(e, _)| e.clone()).unwrap_or_else(|| v.clone())); + picked.push(i); + } } None if f.kind == FieldKind::Combo && f.has(flags::EDIT) => exports.push(v.clone()), None if f.options.is_empty() => exports.push(v.clone()), @@ -689,3 +715,30 @@ pub fn reset(doc: &mut Document, names: Option<&[String]>) -> Result bool { + match seen.get_mut(i) { + Some(taken) if !*taken => { + *taken = true; + true + } + _ => false, + } +} + +/// The twin's positions for the source's picked positions `picked`: each picked option goes to +/// the twin's first not yet taken option with the same export value and text. `None` when one +/// has no such option there. The positions come back in ascending order, as `/I` lists them. +fn twin_positions(picked: &[usize], source: &[(String, String)], twin: &[(String, String)]) -> Option> { + let mut free: std::collections::HashMap<(&str, &str), std::collections::VecDeque> = std::collections::HashMap::new(); + for (i, (e, d)) in twin.iter().enumerate() { + free.entry((e.as_str(), d.as_str())).or_default().push_back(i); + } + let mut out = picked + .iter() + .map(|&p| source.get(p).and_then(|(e, d)| free.get_mut(&(e.as_str(), d.as_str()))).and_then(|q| q.pop_front())) + .collect::>>()?; + out.sort_unstable(); + Some(out) +} diff --git a/crates/ui-egui/src/tools/forms_ui.rs b/crates/ui-egui/src/tools/forms_ui.rs index 1cf6a1b610..7da73bf032 100644 --- a/crates/ui-egui/src/tools/forms_ui.rs +++ b/crates/ui-egui/src/tools/forms_ui.rs @@ -73,6 +73,10 @@ pub struct FormView { /// The widget holding egui's keyboard focus for the focused check box or radio button, and /// whether it takes the arrows (radio buttons). The page keys still work around it. pub(crate) keys: Option<(egui::Id, bool)>, + /// The list box row clicked last, without Shift: (its field name, the row). A Shift-click picks + /// the rows from it to the one clicked; the field's value can't say which row came last when + /// rows share an export value, and `/I` lists them in ascending order. + pub(crate) list_anchor: Option<(String, usize)>, } /// A form message for the app to show, with document data kept separate so the visible @@ -277,14 +281,20 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b picked } else if multi && shift - && let Some(&anchor) = in_order.last() + && let Some(anchor) = + view.forms.list_anchor.as_ref().filter(|(name, _)| *name == f.name).map(|(_, row)| *row).or_else(|| in_order.last().copied()) { let (min, max) = (anchor.min(opt_idx), anchor.max(opt_idx)); (min..=max).collect() } else { vec![opt_idx] }; - if picked != in_order { + if !(multi && shift) { + view.forms.list_anchor = Some((f.name.clone(), opt_idx)); + } + // A single-select list holding an array is written as one value even when its shown + // row is clicked again. + if picked != in_order || f.single_value_is_array { view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::ChoiceIndices(picked) }); } } diff --git a/crates/ui-egui/tests/it/forms.rs b/crates/ui-egui/tests/it/forms.rs index f089b33729..db7fd1bd14 100644 --- a/crates/ui-egui/tests/it/forms.rs +++ b/crates/ui-egui/tests/it/forms.rs @@ -1668,3 +1668,86 @@ fn a_form_button_shows_and_hides_the_attachments_panel() { h.run_steps(2); assert_eq!(h.state().right, None); } + +/// Click at `p` with `modifiers` held. +fn click_with(h: &mut Harness<'static, PdfCraftApp>, p: egui::Pos2, modifiers: egui::Modifiers) { + h.event(egui::Event::ModifiersChanged(modifiers)); + h.hover_at(p); + h.run_steps(1); + for pressed in [true, false] { + h.event(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Primary, pressed, modifiers }); + h.run_steps(1); + } + h.event(egui::Event::ModifiersChanged(egui::Modifiers::NONE)); + h.run_steps(3); +} + +/// A list box opened at 100% zoom on its own page, and its rectangle on screen. +fn list_box(objects: &[&str], name: &str) -> (Harness<'static, PdfCraftApp>, egui::Rect) { + let bytes = pdf_from_objects(objects); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("list.pdf", None, bytes).unwrap(); + app.set_option("left", "closed").unwrap(); + app.set_option("zoom", "100").unwrap(); + app + }); + h.run_steps(4); + let r = { + let s = h.state(); + let doc = s.session.get(s.views[0].id).unwrap(); + let f = doc.form.iter().find(|f| f.name == name).unwrap(); + pdfcraft_ui_egui::forms_ui::field_screen_rect(&s.views[0], &doc.info, f, 0).expect("on screen") + }; + (h, r) +} + +/// A Shift-click picks the rows from the one clicked last: after Ctrl-clicking row 3 and then +/// row 1, Shift on row 4 picks rows 1 to 4, not 3 to 4. The rows share one export value, so the +/// value can't say which came last, and `/I` lists them in ascending order. +#[test] +fn a_shift_click_in_a_list_box_extends_from_the_row_clicked_last() { + let (mut h, r) = list_box( + &[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (days) /Ff 2097152 /Opt [[(x) (Mon)] [(x) (Tue)] [(x) (Wed)] [(x) (Thu)] [(x) (Fri)] [(x) (Sat)]] /Rect [50 220 200 320] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + ], + "days", + ); + let zoom = r.height() / 100.0; + let row = |k: f32| egui::pos2(r.center().x, r.min.y + (8.0 + 11.5 * k) * zoom); + click_with(&mut h, row(3.0), egui::Modifiers::COMMAND); + click_with(&mut h, row(1.0), egui::Modifiers::COMMAND); + assert_eq!(selected(&h, "days"), [1, 3]); + click_with(&mut h, row(4.0), egui::Modifiers::SHIFT); + assert_eq!(selected(&h, "days"), [1, 2, 3, 4], "from row 1, the one clicked last"); +} + +/// A single-select list box holding an array `/V` is written as one value when its shown row is +/// clicked again, not left as an array because the row was already selected. +#[test] +fn clicking_the_shown_row_of_a_single_select_list_holding_an_array_writes_one_value() { + let (mut h, r) = list_box( + &[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (pair) /Opt [[(TW) (Taiwan)] [(JP) (Japan)] [(US) (United States)]] /V [(TW) (JP)] /Rect [50 250 200 320] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + ], + "pair", + ); + let field = |h: &Harness<'static, PdfCraftApp>| { + let s = h.state(); + s.session.get(s.views[0].id).unwrap().form.iter().find(|f| f.name == "pair").unwrap().clone() + }; + assert!(field(&h).single_value_is_array, "loaded from an array"); + let zoom = r.height() / 70.0; + click_at(&mut h, egui::pos2(r.center().x, r.min.y + 8.0 * zoom)); + let f = field(&h); + assert_eq!((f.value, f.selected, f.single_value_is_array), (vec!["TW".to_string()], vec![0], false), "Taiwan, now one value"); +} From 9d7728e68ae6849bc341ca5b22c4dcc7e8058227 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 17:46:54 +0800 Subject: [PATCH 3/5] M6: a click in a same-name twin list box picks the row clicked The click queued the clicked list box's own option positions under the shared field name, but set_value reads ChoiceIndices in the first field of that name. A twin listing options that share an export value in another order therefore gave the first field the option at the same position, not the one clicked. The UI now maps the twin's picks to the first field's positions by export value and text (Field::positions_in, the same matching the engine uses to copy /I to twins), falling back to the export values when an option has no match there. (review) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/forms/src/lib.rs | 19 ++++++++++++++ crates/forms/src/values.rs | 14 +++------- crates/ui-egui/src/tools/forms_ui.rs | 13 +++++++++- crates/ui-egui/tests/it/forms.rs | 39 ++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 12 deletions(-) diff --git a/crates/forms/src/lib.rs b/crates/forms/src/lib.rs index 0be2912518..608e5231d7 100644 --- a/crates/forms/src/lib.rs +++ b/crates/forms/src/lib.rs @@ -238,6 +238,14 @@ impl Field { } } + /// Combo and list boxes: this field's option positions `picked` as positions in `other`'s + /// options, in the same order. Same-name twins are one field (ISO 32000-2 §12.7.3.1) but can + /// list their options in different orders, and a value given by position is read in the first + /// twin's options. `None` when an option picked has no match in `other`. + pub fn positions_in(&self, other: &Field, picked: &[usize]) -> Option> { + option_positions_in(picked, &self.options, &other.options) + } + /// Combo and list boxes: the selected options in the order the value names them (`/V` keeps /// the order they were picked in), so the last is the latest pick. pub fn picked_in_order(&self) -> Vec { @@ -357,3 +365,14 @@ fn name_obj(text: &str) -> Object { fn invalid(m: impl Into) -> Result { Err(FormError::Invalid(m.into())) } + +/// The positions in `target` of the options at positions `picked` in `source`, in the same order: +/// each goes to `target`'s first not yet taken option with the same export value and text, so two +/// options sharing both stay apart. `None` when one has no such option in `target`. +pub(crate) fn option_positions_in(picked: &[usize], source: &[(String, String)], target: &[(String, String)]) -> Option> { + let mut free: std::collections::HashMap<(&str, &str), std::collections::VecDeque> = std::collections::HashMap::new(); + for (i, (e, d)) in target.iter().enumerate() { + free.entry((e.as_str(), d.as_str())).or_default().push_back(i); + } + picked.iter().map(|&p| source.get(p).and_then(|(e, d)| free.get_mut(&(e.as_str(), d.as_str()))).and_then(|q| q.pop_front())).collect() +} diff --git a/crates/forms/src/values.rs b/crates/forms/src/values.rs index 6996549520..348b18b347 100644 --- a/crates/forms/src/values.rs +++ b/crates/forms/src/values.rs @@ -727,18 +727,10 @@ fn first_pick(seen: &mut [bool], i: usize) -> bool { } } -/// The twin's positions for the source's picked positions `picked`: each picked option goes to -/// the twin's first not yet taken option with the same export value and text. `None` when one -/// has no such option there. The positions come back in ascending order, as `/I` lists them. +/// The twin's positions for the source's picked positions `picked`, as +/// [`crate::option_positions_in`] maps them, in ascending order, as `/I` lists them. fn twin_positions(picked: &[usize], source: &[(String, String)], twin: &[(String, String)]) -> Option> { - let mut free: std::collections::HashMap<(&str, &str), std::collections::VecDeque> = std::collections::HashMap::new(); - for (i, (e, d)) in twin.iter().enumerate() { - free.entry((e.as_str(), d.as_str())).or_default().push_back(i); - } - let mut out = picked - .iter() - .map(|&p| source.get(p).and_then(|(e, d)| free.get_mut(&(e.as_str(), d.as_str()))).and_then(|q| q.pop_front())) - .collect::>>()?; + let mut out = crate::option_positions_in(picked, source, twin)?; out.sort_unstable(); Some(out) } diff --git a/crates/ui-egui/src/tools/forms_ui.rs b/crates/ui-egui/src/tools/forms_ui.rs index 7da73bf032..8219ee3e5b 100644 --- a/crates/ui-egui/src/tools/forms_ui.rs +++ b/crates/ui-egui/src/tools/forms_ui.rs @@ -295,7 +295,18 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b // A single-select list holding an array is written as one value even when its shown // row is clicked again. if picked != in_order || f.single_value_is_array { - view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::ChoiceIndices(picked) }); + // The edit names the field, and positions are read in the first field of that + // name: a same-name twin listing its options in another order gives its own + // picks as positions there. One option missing there goes by export value. + let first = form.iter().find(|x| x.name == f.name).filter(|x| x.obj != f.obj); + let value = match first { + None => FieldValue::ChoiceIndices(picked), + Some(first) => match f.positions_in(first, &picked) { + Some(there) => FieldValue::ChoiceIndices(there), + None => FieldValue::Choice(picked.iter().filter_map(|&i| f.options.get(i)).map(|(e, _)| e.clone()).collect()), + }, + }; + view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value }); } } } diff --git a/crates/ui-egui/tests/it/forms.rs b/crates/ui-egui/tests/it/forms.rs index db7fd1bd14..781f23150b 100644 --- a/crates/ui-egui/tests/it/forms.rs +++ b/crates/ui-egui/tests/it/forms.rs @@ -1352,6 +1352,45 @@ fn a_list_box_click_picks_one_of_two_options_with_the_same_export_value() { let _ = std::fs::remove_dir_all(&dir); } +/// Same-name list boxes are one field (ISO 32000-2 §12.7.3.1), and a twin can list the options +/// sharing an export value in another order: clicking One-B in the second gives both One-B, not +/// the option at the same position in the first. +#[test] +fn a_click_in_a_twin_list_box_with_options_in_another_order_picks_the_row_clicked() { + let bytes = pdf_from_objects(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R 6 0 R] /NeedAppearances true /DA (/Helv 10 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 6 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (dup) /Opt [[(1) (One-A)] [(1) (One-B)] [(2) (Two)]] /Rect [50 250 200 320] /P 3 0 R /F 4 >>", + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>", + "<< /Type /Annot /Subtype /Widget /FT /Ch /T (dup) /Opt [[(1) (One-B)] [(1) (One-A)] [(2) (Two)]] /Rect [50 100 200 170] /P 3 0 R /F 4 >>", + ]); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("twins.pdf", None, bytes).unwrap(); + app.set_option("left", "closed").unwrap(); + app.set_option("zoom", "100").unwrap(); + app + }); + h.run_steps(4); + let twins = |h: &Harness<'static, PdfCraftApp>| -> Vec> { + let s = h.state(); + s.session.get(s.views[0].id).unwrap().form.iter().filter(|f| f.name == "dup").map(|f| f.selected.clone()).collect() + }; + let r = { + let s = h.state(); + let doc = s.session.get(s.views[0].id).unwrap(); + let f = doc.form.iter().filter(|f| f.name == "dup").nth(1).unwrap(); + pdfcraft_ui_egui::forms_ui::field_screen_rect(&s.views[0], &doc.info, f, 0).expect("on screen") + }; + let zoom = r.height() / 70.0; + // Row 0 of the second list box: One-B. + click_at(&mut h, egui::pos2(r.center().x, r.min.y + 8.0 * zoom)); + assert_eq!(value(&h, "dup"), ["1"]); + assert_eq!(twins(&h), [vec![1], vec![0]], "One-B in both: second in the first, first in the twin"); +} + /// ISO 32000-2 Table 233: a list box without MultiSelect selects "at most one item". Given /// `/V [(TW) (JP)]` it shows Taiwan alone, and a click writes the one option clicked. #[test] From 48c6a85cee11b864c0e48c40a8818c0fc2139b8d Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 2 Nov 2026 10:47:28 +0800 Subject: [PATCH 4/5] M6: arrow keys in a same-name twin list box step through its own rows The focus now carries the focused field's object, so the overlay, the draft commit and the focus frame use the twin that has the focus rather than the first field with its name. The arrow keys pick by position through the same twin mapping a click uses (list_pick_edit), so an arrow in a twin listing its options in another order picks its own next row. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/ui-egui/src/editing.rs | 1 + crates/ui-egui/src/tools/forms_ui.rs | 68 +++++++++++++++++++--------- crates/ui-egui/tests/it/forms.rs | 8 ++++ 3 files changed, 55 insertions(+), 22 deletions(-) diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index 8def569f58..f18d1c64b2 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -937,6 +937,7 @@ mod revert_draft_tests { fn focus(text: &str) -> Focus { Focus { name: "name".into(), + obj: None, widget: 0, text: text.into(), picked: Vec::new(), diff --git a/crates/ui-egui/src/tools/forms_ui.rs b/crates/ui-egui/src/tools/forms_ui.rs index 8219ee3e5b..51a8270555 100644 --- a/crates/ui-egui/src/tools/forms_ui.rs +++ b/crates/ui-egui/src/tools/forms_ui.rs @@ -10,6 +10,7 @@ use std::collections::HashSet; use egui::{Color32, CornerRadius, Pos2, Rect, Stroke, vec2}; +use pdfcraft_cos::ObjRef; use pdfcraft_engine::{Edit, FieldValue, FormField, FormFieldKind, FormWidget, field_flags, form_appearance}; use pdfcraft_render::DocInfo; @@ -26,6 +27,10 @@ const FIELD_FILL: Color32 = Color32::from_rgb(0xFF, 0xFF, 0xF4); #[derive(Clone, Debug, PartialEq)] pub struct Focus { pub name: String, + /// The focused field's object. Same-name twins are one field (ISO 32000-2 §12.7.3.1) but come + /// as separate fields here, each with its own widgets and option order, and this says which + /// one has the focus. `None` goes by the name: the first field with it. + pub obj: Option, /// Which of the field's widgets (a field can appear on several pages). pub widget: usize, /// Text being typed (text fields). @@ -117,9 +122,38 @@ pub(crate) fn draft_text(f: &FormField) -> String { value } +impl Focus { + /// Whether this is the focus of widget `widget` of `f` (of that twin, for same-name fields). + fn is_on(&self, f: &FormField, widget: usize) -> bool { + self.name == f.name && self.widget == widget && self.obj.is_none_or(|o| o == f.obj) + } +} + +/// The field `focus` is on: the twin with its field object, or else the first field with its name. +pub(crate) fn focused_field<'a>(focus: &Focus, form: &'a [FormField]) -> Option<&'a FormField> { + focus.obj.and_then(|o| form.iter().find(|f| f.obj == o && f.name == focus.name)).or_else(|| form.iter().find(|f| f.name == focus.name)) +} + +/// The edit picking the options at `picked` (positions in `f`'s options) in the list box `f`. +/// The edit names the field, and positions are read in the first field of that name: a same-name +/// twin listing its options in another order gives its picks as positions there. One option +/// missing there goes by export value. +fn list_pick_edit(form: &[FormField], f: &FormField, picked: Vec) -> Edit { + let first = form.iter().find(|x| x.name == f.name).filter(|x| x.obj != f.obj); + let value = match first { + None => FieldValue::ChoiceIndices(picked), + Some(first) => match f.positions_in(first, &picked) { + Some(there) => FieldValue::ChoiceIndices(there), + None => FieldValue::Choice(picked.iter().filter_map(|&i| f.options.get(i)).map(|(e, _)| e.clone()).collect()), + }, + }; + Edit::SetFieldValue { name: f.name.clone(), value } +} + fn open_focus(f: &FormField, widget: usize) -> Focus { Focus { name: f.name.clone(), + obj: Some(f.obj), widget, text: draft_text(f), picked: f.value.clone(), @@ -206,7 +240,7 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b if f.kind == FormFieldKind::Text && usable && let Some(at) = ptr.pressed() - && view.forms.focus.as_ref().is_none_or(|x| x.name != f.name || x.widget != wi) + && view.forms.focus.as_ref().is_none_or(|x| !x.is_on(f, wi)) { commit(view, form); view.forms.focus = Some(Focus { press: Some(at), ..open_focus(f, wi) }); @@ -232,7 +266,7 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b FormFieldKind::CheckBox | FormFieldKind::Radio => { // The box takes the focus, as in Acrobat, so Tab goes on from it (#2223). The overlay // toggles it, after the edit for any text this commits. - if view.forms.focus.as_ref().is_none_or(|x| x.name != f.name || x.widget != wi) { + if view.forms.focus.as_ref().is_none_or(|x| !x.is_on(f, wi)) { commit(view, form); view.forms.focus = Some(open_focus(f, wi)); } @@ -244,12 +278,12 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b FormFieldKind::Text if ptr.pressed().is_none() => { // An editor opened by the press cannot be hit until a frame has laid it out: the // release lands on the page and would take its focus away. - if let Some(x) = view.forms.focus.as_mut().filter(|x| x.name == f.name && x.widget == wi) { + if let Some(x) = view.forms.focus.as_mut().filter(|x| x.is_on(f, wi)) { x.request_focus = true; } } FormFieldKind::Text | FormFieldKind::Combo => { - if view.forms.focus.as_ref().is_none_or(|x| x.name != f.name || x.widget != wi) { + if view.forms.focus.as_ref().is_none_or(|x| !x.is_on(f, wi)) { commit(view, form); view.forms.focus = Some(open_focus(f, wi)); } @@ -295,18 +329,7 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b // A single-select list holding an array is written as one value even when its shown // row is clicked again. if picked != in_order || f.single_value_is_array { - // The edit names the field, and positions are read in the first field of that - // name: a same-name twin listing its options in another order gives its own - // picks as positions there. One option missing there goes by export value. - let first = form.iter().find(|x| x.name == f.name).filter(|x| x.obj != f.obj); - let value = match first { - None => FieldValue::ChoiceIndices(picked), - Some(first) => match f.positions_in(first, &picked) { - Some(there) => FieldValue::ChoiceIndices(there), - None => FieldValue::Choice(picked.iter().filter_map(|&i| f.options.get(i)).map(|(e, _)| e.clone()).collect()), - }, - }; - view.pending_edit = Some(Edit::SetFieldValue { name: f.name.clone(), value }); + view.pending_edit = Some(list_pick_edit(form, f, picked)); } } } @@ -316,7 +339,7 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, form: &[FormField], allowed: b /// The edit the focused field's draft makes, if it differs from the field's value. pub(crate) fn draft_edit(focus: &Focus, form: &[FormField]) -> Option { - let f = form.iter().find(|f| f.name == focus.name)?; + let f = focused_field(focus, form)?; let value = match f.kind { FormFieldKind::Text if f.value.first().map(String::as_str).unwrap_or("") != focus.text => FieldValue::Text(focus.text.clone()), // Typed into an editable combo box: a shown text picks its option, anything else is the @@ -525,7 +548,7 @@ pub(crate) fn paint_page( for f in form { for (wi, w) in f.widgets.iter().enumerate().filter(|(_, w)| w.page == Some(page) && !w.hidden) { let r = widget_rect(xf, info, page, w.rect); - let focused = view.forms.focus.as_ref().is_some_and(|x| x.name == f.name && x.widget == wi); + let focused = view.forms.focus.as_ref().is_some_and(|x| x.is_on(f, wi)); if focused { painter.rect_stroke(r.expand(1.0), CornerRadius::same(2), Stroke::new(2.0, FOCUS_BLUE), egui::StrokeKind::Outside); } @@ -738,7 +761,7 @@ fn tab_to_next_field(view: &mut DocView, form: &[FormField], f: &FormField, widg .collect(); order.sort_by_key(|o| o.0); if let Some(pos) = - order.iter().position(|(_, x, wi)| x.name == f.name && *wi == widget).or_else(|| order.iter().position(|(_, x, _)| x.name == f.name)) + order.iter().position(|(_, x, wi)| x.obj == f.obj && *wi == widget).or_else(|| order.iter().position(|(_, x, _)| x.name == f.name)) { let k = if forward { (pos + 1) % order.len() } else { (pos + order.len() - 1) % order.len() }; let (_, target, wi) = order[k]; @@ -790,7 +813,7 @@ pub(crate) fn overlay( let toggle = std::mem::take(&mut view.forms.toggle); view.forms.keys = None; let focus = view.forms.focus.clone()?; - let Some(f) = form.iter().find(|f| f.name == focus.name) else { + let Some(f) = focused_field(&focus, form) else { view.forms.focus = None; return None; }; @@ -950,8 +973,9 @@ pub(crate) fn overlay( let current_idx = f.picked_in_order().last().copied().unwrap_or(0); let next_idx = if up { current_idx.saturating_sub(1) } else { current_idx.saturating_add(1).min(f.options.len().saturating_sub(1)) }; if next_idx != current_idx && next_idx < f.options.len() { - // By position, so the arrows step through options that share an export value. - edit = Some(Edit::SetFieldValue { name: f.name.clone(), value: FieldValue::ChoiceIndices(vec![next_idx]) }); + // By position, so the arrows step through options that share an export value, + // in the focused twin's own order. + edit = Some(list_pick_edit(form, f, vec![next_idx])); } } if clicked_outside() { diff --git a/crates/ui-egui/tests/it/forms.rs b/crates/ui-egui/tests/it/forms.rs index 781f23150b..91e06d9c0b 100644 --- a/crates/ui-egui/tests/it/forms.rs +++ b/crates/ui-egui/tests/it/forms.rs @@ -1389,6 +1389,14 @@ fn a_click_in_a_twin_list_box_with_options_in_another_order_picks_the_row_clicke click_at(&mut h, egui::pos2(r.center().x, r.min.y + 8.0 * zoom)); assert_eq!(value(&h, "dup"), ["1"]); assert_eq!(twins(&h), [vec![1], vec![0]], "One-B in both: second in the first, first in the twin"); + // The arrows step through the focused twin's own rows: ↓ from One-B there is One-A, which is + // first in the first field, not Two (the row after One-B in the first field). + h.key_press(egui::Key::ArrowDown); + h.run_steps(3); + assert_eq!(twins(&h), [vec![0], vec![1]], "One-A in both after ↓ in the twin"); + h.key_press(egui::Key::ArrowUp); + h.run_steps(3); + assert_eq!(twins(&h), [vec![1], vec![0]], "back to One-B after ↑ in the twin"); } /// ISO 32000-2 Table 233: a list box without MultiSelect selects "at most one item". Given From 8e07d67eb6a32a7891c57ce112ec10d10489cd5e Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Mon, 9 Nov 2026 11:47:28 +0800 Subject: [PATCH 5/5] M6: fill in the list selection fields in the engine's XFA test helper Main's new engine test builds a pdfcraft_forms::Field literal; give it the selected and single_value_is_array fields this branch adds. --- crates/engine/src/xfa.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/engine/src/xfa.rs b/crates/engine/src/xfa.rs index 4246e5b504..2bdbcf10cc 100644 --- a/crates/engine/src/xfa.rs +++ b/crates/engine/src/xfa.rs @@ -1051,6 +1051,8 @@ mod tests { flags: 0, max_len: None, options: Vec::new(), + selected: Vec::new(), + single_value_is_array: false, da: String::new(), quadding: 0, tooltip: None,