From c9956bf77f01d205e6954d385b4871cebc45eb76 Mon Sep 17 00:00:00 2001 From: Muhammad Saad Date: Fri, 16 Oct 2026 00:34:21 +0400 Subject: [PATCH] Replace Text: give the caret its own /NM (#2666) add_text_replacement built the StrikeOut and the Caret from one Meta, and add_annotation stamps /NM from meta.id, so the pair shared a single name. XFDF names a reply by the /NM of what it replies to, so the exporter wrote the caret's inreplyto as the caret's own name. Importing that file found a comment replying to itself, and the Replace Text comment did not arrive: the reporter's second document listed none. FDF was unaffected because it keeps the pair by object structure rather than by name. Take a second Meta for the caret. The ids stay supplied by the caller, as the rest of the crate does so edits remain deterministic, and the one production caller already has a generator and calls it twice. /IRT still points at the strikeout, so the group still moves, lists and deletes as one. Tests: the annotation test now asserts the two carry different names and that the caret's /IRT resolves to the strikeout's. A new XFDF test exports a Replace Text pair and imports it into another document, asserting both comments arrive with their thread intact; it fails if the two are given one id again. --- crates/annot/src/lib.rs | 8 +++++++- crates/annot/src/tests.rs | 12 ++++++++++-- crates/engine/src/lib.rs | 5 ++++- crates/xfdf/src/tests.rs | 28 +++++++++++++++++++++++++++- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index f4973aa02..b7f9d6269 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -2282,6 +2282,11 @@ fn line_endings_of(d: &Dict) -> Option> { /// Replace Text (Acrobat's proposal): strike out `quads` and add a caret at the end of the /// struck text holding `replacement`, grouped with the strikeout (`/IRT` + `/RT /Group`) so they /// move, list and delete as one. Returns the strikeout's index. +/// +/// The two annotations need their own `/NM`: XFDF names a reply by the `/NM` of what it replies +/// to, so one id for both writes `inreplyto` equal to the caret's own `name`, and a reader that +/// resolves names (Acrobat, and our own importer) loses the pair. `caret_meta` is a second id +/// from the caller, so the edit stays deterministic. #[allow(clippy::too_many_arguments)] pub fn add_text_replacement( doc: &mut Document, @@ -2292,6 +2297,7 @@ pub fn add_text_replacement( strike: &Style, caret: &Style, meta: &Meta, + caret_meta: &Meta, ) -> Result { let last = *quads.last().ok_or_else(|| AnnotError::Invalid("select the text to replace first".into()))?; let s = add_annotation( @@ -2317,7 +2323,7 @@ pub fn add_text_replacement( let c = add_annotation( doc, &NewAnnotation { page, shape: Shape::Caret { rect }, style: caret.clone(), contents: replacement.into(), author: author.into() }, - meta, + caret_meta, )?; let (_, cr) = annot_ref(doc, page, c)?; doc.update_dict(cr, |d| { diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index faed409ab..7c73b0508 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -784,7 +784,7 @@ fn replace_text_groups_a_strikeout_and_a_caret() { let quads = [[100.0, 720.0, 200.0, 720.0, 100.0, 706.0, 200.0, 706.0]]; let red = Style { color: [0.89, 0.13, 0.13], opacity: 1.0, width: 1.0, fill: None }; let blue = Style { color: [0.0, 0.47, 0.84], opacity: 1.0, width: 1.0, fill: None }; - let i = add_text_replacement(&mut doc, 0, &quads, "new words", "Ada", &red, &blue, &meta("r")).unwrap(); + let i = add_text_replacement(&mut doc, 0, &quads, "new words", "Ada", &red, &blue, &meta("r"), &meta("r-caret")).unwrap(); let doc = reopen(&doc); let a = list(&doc, 0); let strike = &a[i]; @@ -792,13 +792,21 @@ fn replace_text_groups_a_strikeout_and_a_caret() { let caret = a.iter().find(|d| d.name(b"Subtype") == Some(b"Caret")).unwrap(); assert_eq!((text(caret, b"Contents").as_str(), caret.name(b"RT")), ("new words", Some(&b"Group"[..]))); assert!(caret.reference(b"IRT").is_some()); + // #2666: the pair needs its own `/NM` each. XFDF names a reply by the `/NM` of what it + // replies to, so sharing one id writes `inreplyto` equal to the caret's own `name` and the + // pair does not survive an export and import. + assert_eq!(text(caret, b"NM").as_str(), "r-caret"); + assert_eq!(text(strike, b"NM").as_str(), "r"); + assert_ne!(text(caret, b"NM"), text(strike, b"NM"), "the caret must not carry the strikeout's name"); + let irt = caret.reference(b"IRT").unwrap(); + assert_eq!(text(doc.get(irt).as_dict().unwrap(), b"NM").as_str(), "r", "the caret replies to the strikeout"); let r = rect(caret); assert!(r[0] < 200.0 && r[2] > 200.0 && r[1] < 706.0, "at the end of the line, below its baseline: {r:?}"); // Deleting the strikeout takes the caret with it (the group). let mut doc = doc; delete_annotation(&mut doc, 0, i).unwrap(); assert!(!list(&doc, 0).iter().any(|d| d.name(b"Subtype") == Some(b"Caret"))); - assert!(add_text_replacement(&mut doc, 0, &[], "x", "Ada", &red, &blue, &meta("r")).is_err()); + assert!(add_text_replacement(&mut doc, 0, &[], "x", "Ada", &red, &blue, &meta("r"), &meta("r-caret")).is_err()); } #[test] diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 01f162ae7..461950d42 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -2368,7 +2368,10 @@ fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut EditCtx) -> pdfcraft_annot::erase_ink(doc, *page, *index, path, *radius, &cx.meta())?; } Edit::ReplaceText { page, quads, text, author, strike, caret } => { - pdfcraft_annot::add_text_replacement(doc, *page, quads, text, author, strike, caret, &cx.meta())?; + // One `/NM` each: the caret replies to the strikeout, and XFDF names that reply by + // the strikeout's `/NM` (#2666). + let (strike_meta, caret_meta) = (cx.meta(), cx.meta()); + pdfcraft_annot::add_text_replacement(doc, *page, quads, text, author, strike, caret, &strike_meta, &caret_meta)?; } Edit::MarkAnnotation { page, index, marked, author } => { pdfcraft_annot::set_marked(doc, *page, *index, *marked, author, &cx.meta())?; diff --git a/crates/xfdf/src/tests.rs b/crates/xfdf/src/tests.rs index d244df1ad..a33f1a101 100644 --- a/crates/xfdf/src/tests.rs +++ b/crates/xfdf/src/tests.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use pdfcraft_annot::{Markup, Meta, NewAnnotation, Shape, Style, add_annotation, add_reply, summaries}; +use pdfcraft_annot::{Markup, Meta, NewAnnotation, Shape, Style, add_annotation, add_reply, add_text_replacement, summaries}; use pdfcraft_cos::{SaveOptions, write_full, write_incremental}; use pdfcraft_forms::{NewField, add_field, fields, set_value}; @@ -142,6 +142,32 @@ fn values(doc: &Document) -> Vec<(String, Vec)> { fields(doc).into_iter().map(|f| (f.name, f.value)).collect() } +/// #2666: Replace Text writes a StrikeOut and a Caret that replies to it. XFDF names a reply by +/// the `/NM` of what it replies to, so when both carried one id the caret's `inreplyto` was its +/// own `name`: importing it found a comment replying to itself and the pair was lost. +#[test] +fn a_replace_text_pair_survives_an_xfdf_round_trip() { + let mut src = blank(); + let quads = [[100.0, 720.0, 200.0, 720.0, 100.0, 706.0, 200.0, 706.0]]; + let style = Style { color: [0.89, 0.13, 0.13], opacity: 1.0, width: 1.0, fill: None }; + let meta = |id: &str| Meta { date: Some("D:20261001120000Z".into()), id: id.into() }; + add_text_replacement(&mut src, 0, &quads, "new words", "Ada", &style, &style, &meta("strike"), &meta("caret")).unwrap(); + assert_eq!(summaries(&src).len(), 2, "a strikeout and a caret"); + + let xfdf = export_xfdf(&src, true, false, "a.pdf"); + assert!(xfdf.contains("name=\"strike\"") && xfdf.contains("name=\"caret\""), "each gets its own name: {xfdf}"); + assert!(xfdf.contains("inreplyto=\"strike\""), "the caret replies to the strikeout: {xfdf}"); + assert!(!xfdf.contains("name=\"caret\" ... inreplyto=\"caret\""), "never a reply to itself"); + + let mut dst = blank(); + let r = import(&mut dst, xfdf.as_bytes()).unwrap(); + assert_eq!(r.comments, 2, "both comments import: {r:?}"); + let found = summaries(&dst); + let kinds: Vec<&str> = found.iter().map(|s| s.subtype.as_str()).collect(); + assert!(kinds.contains(&"StrikeOut") && kinds.contains(&"Caret"), "{kinds:?}"); + assert_eq!(comment_view(&dst), comment_view(&src), "the pair and its thread survive"); +} + #[test] fn xfdf_round_trips_comments_and_values() { let src = filled();