From 492011348e818294c924f00de019bf1d96e28cf2 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 00:18:30 +0800 Subject: [PATCH] M5: comments in an /RT /Group are one comment, with the primary's attributes, ported onto main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single squashed port of the branch's four commits onto current main, which split annot's lib.rs (info.rs, modify.rs), render's inspect.rs (model.rs, annotations.rs, tests.rs), engine's lib.rs (comments.rs, edit/run.rs) and ui-egui's comments.rs (tools/comments/*). The behaviour and tests are the branch's (ISO 32000-2 §12.5.6.2): - a group subordinate is part of its primary's comment: listed, selected, exported and deleted with it, with the primary's attributes; - Replace Text writes the caret as the primary and the strikeout as its subordinate; earlier groups (strikeout primary, text on the caret) still list their text, through a map; - deleting a group refuses a locked member, and a subordinate whose primary is past the depth limit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/annot/src/info.rs | 44 ++++- crates/annot/src/lib.rs | 2 +- crates/annot/src/modify.rs | 64 +++++-- crates/annot/src/tests.rs | 138 +++++++++++++-- crates/automation/src/comments.rs | 24 ++- crates/automation/src/tools.rs | 2 +- crates/automation/tests/it/automation.rs | 175 +++++++++++++++++++- crates/engine/src/comments.rs | 19 ++- crates/engine/src/edit/run.rs | 4 +- crates/engine/src/tests.rs | 15 ++ crates/render/src/inspect.rs | 4 +- crates/render/src/inspect/annotations.rs | 5 +- crates/render/src/inspect/model.rs | 71 +++++++- crates/render/src/inspect/tests.rs | 128 ++++++++++++++ crates/render/src/lib.rs | 5 +- crates/ui-egui/src/panels/comments_panel.rs | 32 +++- crates/ui-egui/src/tools/comments/input.rs | 10 +- crates/ui-egui/src/tools/comments/paint.rs | 6 + crates/ui-egui/src/tools/comments/state.rs | 23 +++ crates/ui-egui/tests/it/comments.rs | 43 ++++- crates/xfdf/src/tests.rs | 12 +- parity/acrobat-features.toml | 8 +- 22 files changed, 768 insertions(+), 66 deletions(-) diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index b56e0de20d..0266cc0cd9 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -23,7 +23,8 @@ pub struct Summary { pub in_reply_to: Option, /// The page and `/Annots` position `/IRT` points at, by reference (`/NM` need not be unique). pub parent: Option<(usize, usize)>, - /// `/RT /Group`: grouped with the `/IRT` annotation (Replace Text's caret), not a reply to it. + /// `/RT /Group`: with an `/IRT` reference, a subordinate of the group whose primary that is + /// ([`is_group_subordinate`]), not a reply to it. pub grouped: bool, pub rect: [f32; 4], pub color: Option<[f32; 3]>, @@ -40,6 +41,15 @@ pub struct Summary { pub points: Vec<[f32; 2]>, } +impl Summary { + /// A subordinate of a group (`/RT /Group` with an `/IRT` to a listed comment, ISO 32000-2 + /// §12.5.6.2): part of its primary's comment, not a reply. As + /// `pdfcraft_render::Annotation::is_group_member`, which also drops a looping `/IRT`. + pub fn is_group_member(&self) -> bool { + self.grouped && self.parent.is_some() + } +} + pub(crate) fn text_value(doc: &Document, d: &Dict, key: &[u8]) -> Option { let o = doc.resolve(d.get(key)?); let s = match &*o { @@ -70,6 +80,38 @@ fn final_ref(doc: &Document, mut r: ObjRef) -> ObjRef { r } +/// Whether an annotation is a subordinate of a group rather than a reply: `/RT /Group` with an +/// `/IRT` reference (ISO 32000-2 §12.5.6.2, Table 172). The group's primary is the `/IRT` +/// annotation, and the subordinate's own text, author, colour, dates and pop-up are ignored. +/// `/RT` is `R` by default, and a value other than `Group` reads as `R` too. Keep in step with +/// `pdfcraft_render::inspect`. +pub fn is_group_subordinate(d: &Dict) -> bool { + d.name(b"RT") == Some(b"Group") && d.reference(b"IRT").is_some() +} + +/// How far a group subordinate's `/IRT` chain is followed to its primary. +pub(crate) const MAX_GROUP_DEPTH: usize = 64; + +/// The primary of the group `r` belongs to: up the `/IRT` chain while it leads to another +/// subordinate in `list` (one page's `/Annots`). `r` itself when it is not a subordinate, or +/// when its primary is not on the page; the chain stops at a loop. `None` when it is still +/// on a subordinate after [`MAX_GROUP_DEPTH`] annotations: the group's primary is unknown, as +/// `pdfcraft_render::inspect::group_primaries` reads it (keep the two in step). +pub(crate) fn group_primary(doc: &Document, list: &[Object], r: ObjRef) -> Option { + let mut seen = vec![r]; + let mut at = r; + for _ in 0..MAX_GROUP_DEPTH { + let obj = doc.get(at); + let Some(next) = obj.as_dict().filter(|d| is_group_subordinate(d)).and_then(|d| d.reference(b"IRT")) else { return Some(at) }; + if seen.contains(&next) || !list.iter().any(|e| e.as_ref() == Some(next)) { + return Some(at); + } + seen.push(next); + at = next; + } + None +} + /// Every comment (see [`is_comment_subtype`]), ordered by page and then top edge. pub fn summaries(doc: &Document) -> Vec { let mut out = Vec::new(); diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index 7bd0894c66..43b9d098e1 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -29,7 +29,7 @@ mod types; use create::num_array; pub use create::{add_annotation, callout_attach, orient_image_stamp}; pub use draw::{AppearanceCache, set_appearance, with_missing_appearances, with_missing_appearances_cached}; -pub use info::{Props, Summary, is_comment_subtype, props, set_info, summaries}; +pub use info::{Props, Summary, is_comment_subtype, is_group_subordinate, props, set_info, summaries}; #[cfg(test)] use lookup::{ANNOTS_ENTRIES_COPIED, PageWalk, annot_dict, annot_ref}; use lookup::{annots, page_ref}; diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index 2e3ae0b7d0..006bb14273 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -2,7 +2,7 @@ use super::create::{ add_annotation, base_dict, bounds, callout_attach, ending_draws, ending_pair, finite, grow, normalize, num_array, rect_for, rgb, }; use super::draw::{draw_appearance, redraw_or_restore, set_appearance}; -use super::info::{line_endings_of, text_value}; +use super::info::{MAX_GROUP_DEPTH, group_primary, is_group_subordinate, line_endings_of, text_value}; use super::lookup::{annot_dict, annot_ref, annotation_flags, annots, page_ref, set_annots}; use super::types::{ FLAG_HIDDEN, FLAG_LOCKED, FLAG_NO_ROTATE, FLAG_NO_ZOOM, FLAG_PRINT, LineEnding, Markup, Meta, NewAnnotation, ReviewState, Rgb, Shape, Style, @@ -70,33 +70,57 @@ pub(crate) fn touch(d: &mut Dict, meta: &Meta) { } /// Delete a comment together with its pop-up and its replies (and theirs), as Acrobat does. -/// Refused while it, or a reply that would go with it, is locked. +/// Refused while it, or a reply that would go with it, is locked. A comment in a group +/// (`/RT /Group`) takes the whole group with it: deleting a subordinate deletes its primary, and +/// with it every member (ISO 32000-2 §12.5.6.2). A subordinate whose primary is too far up its +/// `/IRT` chain to find is refused rather than deleted with only part of its group, and so is a +/// group with a locked member. pub fn delete_annotation(doc: &mut Document, page: usize, index: usize) -> Result<(), AnnotError> { - deletable(doc, page, index)?; + let index = deleted_index(doc, page, index)?; lift_annotation(doc, page, index) } /// Whether [`delete_annotation`] deletes the annotation at `(page, index)`, or why not, without /// changing anything: what a Delete command asks before it is offered. It is refused while the -/// annotation is locked, or a reply that would go with it is. +/// annotation is locked, or a reply (or group member) that would go with it is. pub fn deletable(doc: &Document, page: usize, index: usize) -> Result<(), AnnotError> { + deleted_index(doc, page, index).map(|_| ()) +} + +/// [`deletable`], returning the index [`delete_annotation`] deletes from: the group's primary for +/// a group subordinate, else `index`. +fn deleted_index(doc: &Document, page: usize, index: usize) -> Result { let list = annots(doc, page_ref(doc, page)?); - let target = list.get(index).ok_or(AnnotError::NoSuchAnnotation { page, index })?; + let mut target = list.get(index).ok_or(AnnotError::NoSuchAnnotation { page, index })?; + let mut index = index; if let Some(r) = target.as_ref() { unlocked(doc, r)?; + let Some(primary) = group_primary(doc, &list, r) else { + return Err(AnnotError::Invalid(format!( + "this comment can't be deleted: its group's primary is more than {MAX_GROUP_DEPTH} comments up its chain" + ))); + }; + if primary != r + && let Some((i, e)) = list.iter().enumerate().find(|(_, e)| e.as_ref() == Some(primary)) + { + unlocked(doc, primary)?; + (target, index) = (e, i); + } } let doomed = with_replies_and_popups(doc, &list, seed(target)); // A locked reply can't be deleted on its own, so it doesn't go with the comment or reply it - // answers either. Pop-ups follow their comment. + // answers either; nor does a locked group member keep its place while its group goes + // (ISO 32000-2 §12.5.3, Table 167: Locked). Pop-ups follow their comment. for (_, e) in list.iter().enumerate().filter(|(i, e)| *i != index && goes_with(e, &doomed)) { let obj = doc.resolve(e); if let Some(d) = obj.as_dict().filter(|d| d.name(b"Subtype") != Some(b"Popup".as_slice())) && annotation_flags(doc, d) & FLAG_LOCKED != 0 { - return Err(AnnotError::Invalid("a reply to it is locked".into())); + let what = if is_group_subordinate(d) { "a comment in its group is locked" } else { "a reply to it is locked" }; + return Err(AnnotError::Invalid(what.into())); } } - Ok(()) + Ok(index) } /// Take the annotation at `(page, index)` out of its page with its pop-up and replies, as @@ -709,11 +733,17 @@ fn pair_points(d: &Dict, key: &[u8]) -> 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. +/// struck text holding `replacement`, grouped (ISO 32000-2 §12.5.6.2) so they list, select and +/// delete as one. Returns the caret's index, the group's comment. +/// +/// The caret is the group's primary: it holds `replacement` in `/Contents` and has `/IT +/// /Replace`. The strikeout is its subordinate (`/IRT` to the caret, `/RT /Group`, `/IT +/// /StrikeOutTextEdit`) with no `/Contents`, since a reader ignores a subordinate's text. +/// Groups written before this the other way round (the strikeout primary, the text only on +/// the caret) still list their text: see `pdfcraft_render::fill_group_text`. /// /// 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 +/// to, so one id for both writes `inreplyto` equal to the strikeout'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. pub fn add_text_replacement(doc: &mut Document, replacement: TextReplacement<'_>) -> Result { @@ -731,7 +761,6 @@ pub fn add_text_replacement(doc: &mut Document, replacement: TextReplacement<'_> meta, )?; let (_, sr) = annot_ref(doc, page, s)?; - doc.update_dict(sr, |d| d.set(b"IT".to_vec(), Object::name("StrikeOutTextEdit")))?; // The caret sits on the baseline at the end of the last line, its point at the text. let xs = [last[0], last[2], last[4], last[6]]; let ys = [last[1], last[3], last[5], last[7]]; @@ -746,11 +775,16 @@ pub fn add_text_replacement(doc: &mut Document, replacement: TextReplacement<'_> )?; let (_, cr) = annot_ref(doc, page, c)?; doc.update_dict(cr, |d| { - d.set(b"IRT".to_vec(), Object::Ref(sr)); - d.set(b"RT".to_vec(), Object::name("Group")); + d.set(b"IT".to_vec(), Object::name("Replace")); d.set(b"Subj".to_vec(), PdfString::text("Replace Text")); })?; - Ok(s) + doc.update_dict(sr, |d| { + d.set(b"IT".to_vec(), Object::name("StrikeOutTextEdit")); + d.set(b"IRT".to_vec(), Object::Ref(cr)); + d.set(b"RT".to_vec(), Object::name("Group")); + d.remove(b"Contents"); + })?; + Ok(c) } /// The Eraser (Draw tools): remove the parts of drawing `(page, index)` within `radius` points diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index a3dc14e262..ff0daba407 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -891,22 +891,34 @@ fn replace_text_groups_a_strikeout_and_a_caret() { .unwrap(); let doc = reopen(&doc); let a = list(&doc, 0); - let strike = &a[i]; - assert_eq!((strike.name(b"Subtype"), strike.name(b"IT")), (Some(&b"StrikeOut"[..]), Some(&b"StrikeOutTextEdit"[..]))); - 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()); + // The caret is the group's primary (ISO 32000-2 §12.5.6.2): it holds the text, has no + // `/IRT`, and is what the call returns. + let caret = &a[i]; + assert_eq!((caret.name(b"Subtype"), caret.name(b"IT")), (Some(&b"Caret"[..]), Some(&b"Replace"[..]))); + assert_eq!(text(caret, b"Contents").as_str(), "new words"); + assert!(caret.get(b"IRT").is_none() && caret.get(b"RT").is_none()); + // The strikeout is its subordinate, with no text of its own. + let strike = a.iter().find(|d| d.name(b"Subtype") == Some(b"StrikeOut")).unwrap(); + assert_eq!((strike.name(b"IT"), strike.name(b"RT")), (Some(&b"StrikeOutTextEdit"[..]), Some(&b"Group"[..]))); + assert!(strike.get(b"Contents").is_none()); + assert!(is_group_subordinate(strike)); // #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. + // replies to, so sharing one id writes `inreplyto` equal to the subordinate'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 irt = strike.reference(b"IRT").unwrap(); + assert_eq!(text(doc.get(irt).as_dict().unwrap(), b"NM").as_str(), "r-caret", "the strikeout is grouped with the caret"); 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 summary = summaries(&doc); + assert_eq!(summary.iter().filter(|s| s.page == 0 && !s.is_group_member()).count(), 2, "the fixture's Square and the caret: {summary:?}"); + // Deleting either member deletes the group. + for member in [i, a.iter().position(|d| d.name(b"Subtype") == Some(b"StrikeOut")).unwrap()] { + let mut doc = doc.clone(); + delete_annotation(&mut doc, 0, member).unwrap(); + assert!(!list(&doc, 0).iter().any(|d| matches!(d.name(b"Subtype"), Some(b"Caret" | b"StrikeOut"))), "{member}"); + } 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"))); @@ -928,6 +940,110 @@ fn replace_text_groups_a_strikeout_and_a_caret() { ); } +/// ISO 32000-2 §12.5.6.2: `/RT /Group` makes an annotation a subordinate of the group its +/// `/IRT` annotation is primary of. Another `/RT` reads as a reply, and so does a bare `/IRT`. +/// Deleting a subordinate deletes the whole group, with the primary's replies. +#[test] +fn a_group_reads_as_one_and_deletes_as_one() { + let mut doc = fixture(); + let square = add_annotation(&mut doc, &new(0, Shape::Rectangle { rect: [100.0, 100.0, 200.0, 200.0] }), &meta("sq")).unwrap(); + let (_, sr) = annot_ref(&mut doc, 0, square).unwrap(); + let irt = |doc: &mut Document, id: &str, rt: Option<&str>| { + let i = add_annotation(doc, &new(0, Shape::Oval { rect: [300.0, 300.0, 350.0, 350.0] }), &meta(id)).unwrap(); + let (_, r) = annot_ref(doc, 0, i).unwrap(); + doc.update_dict(r, |d| { + d.set(b"IRT".to_vec(), Object::Ref(sr)); + if let Some(rt) = rt { + d.set(b"RT".to_vec(), Object::name(rt)); + } + }) + .unwrap(); + i + }; + let member = irt(&mut doc, "member", Some("Group")); + irt(&mut doc, "reply", None); + irt(&mut doc, "explicit", Some("R")); + irt(&mut doc, "odd", Some("Bogus")); + let doc = reopen(&doc); + let grouped: Vec<(String, bool)> = + summaries(&doc).into_iter().filter(|s| s.in_reply_to.is_some()).map(|s| (s.name.clone().unwrap_or_default(), s.is_group_member())).collect(); + assert_eq!( + grouped, + [("member".into(), true), ("reply".into(), false), ("explicit".into(), false), ("odd".into(), false)], + "only /RT /Group groups" + ); + let mut doc = doc; + delete_annotation(&mut doc, 0, member).unwrap(); + let left = list(&doc, 0); + assert_eq!(left.len(), 1, "the group, its primary and the primary's replies are gone; the unrelated Square stays"); + assert_eq!(left[0].name(b"Subtype"), Some(&b"Square"[..])); + assert!(left[0].get(b"NM").is_none()); +} + +/// A subordinate whose `/IRT` chain reaches its primary within the 64 annotations the listing +/// follows deletes the whole group; one whose chain runs past them is refused, and the file +/// keeps every member, rather than losing only the part of the group below the cut +/// (ISO 32000-2 §12.5.6.2). +#[test] +fn a_group_chain_past_the_depth_limit_is_not_deleted_in_part() { + for (subordinates, deletes) in [(63, true), (64, false), (65, false)] { + let mut doc = fixture(); + let top = add_annotation(&mut doc, &new(0, Shape::Rectangle { rect: [100.0, 100.0, 200.0, 200.0] }), &meta("top")).unwrap(); + let (_, mut up) = annot_ref(&mut doc, 0, top).unwrap(); + let mut last = top; + for k in 0..subordinates { + last = add_annotation(&mut doc, &new(0, Shape::Oval { rect: [300.0, 300.0, 350.0, 350.0] }), &meta(&format!("s{k}"))).unwrap(); + let (_, r) = annot_ref(&mut doc, 0, last).unwrap(); + doc.update_dict(r, |d| { + d.set(b"IRT".to_vec(), Object::Ref(up)); + d.set(b"RT".to_vec(), Object::name("Group")); + }) + .unwrap(); + up = r; + } + let before = list(&doc, 0).len(); + let result = delete_annotation(&mut doc, 0, last); + let left = list(&doc, 0).len(); + if deletes { + result.unwrap(); + assert_eq!(left, 1, "{subordinates}: the group is gone, the fixture's Square stays"); + } else { + assert!(matches!(result, Err(AnnotError::Invalid(_))), "{subordinates}: {result:?}"); + assert_eq!(left, before, "{subordinates}: nothing is deleted"); + } + } +} + +/// Deleting an unlocked subordinate takes every member of its group, so one locked member, +/// even one that is neither the subordinate nor the primary, refuses the whole deletion +/// (ISO 32000-2 §12.5.3, Table 167: Locked). +#[test] +fn a_locked_group_member_keeps_its_group() { + let mut doc = fixture(); + let top = add_annotation(&mut doc, &new(0, Shape::Rectangle { rect: [100.0, 100.0, 200.0, 200.0] }), &meta("top")).unwrap(); + let (_, tr) = annot_ref(&mut doc, 0, top).unwrap(); + let mut members = Vec::new(); + for id in ["a", "b"] { + let i = add_annotation(&mut doc, &new(0, Shape::Oval { rect: [300.0, 300.0, 350.0, 350.0] }), &meta(id)).unwrap(); + let (_, r) = annot_ref(&mut doc, 0, i).unwrap(); + doc.update_dict(r, |d| { + d.set(b"IRT".to_vec(), Object::Ref(tr)); + d.set(b"RT".to_vec(), Object::name("Group")); + }) + .unwrap(); + members.push(i); + } + set_locked(&mut doc, 0, members[1], true).unwrap(); + let before = list(&doc, 0).len(); + let result = delete_annotation(&mut doc, 0, members[0]); + assert!(matches!(result, Err(AnnotError::Invalid(_))), "{result:?}"); + assert_eq!(list(&doc, 0).len(), before, "nothing is deleted"); + + set_locked(&mut doc, 0, members[1], false).unwrap(); + delete_annotation(&mut doc, 0, members[0]).unwrap(); + assert_eq!(list(&doc, 0).len(), 1, "unlocked, the group goes; the fixture's Square stays"); +} + #[test] fn files_attach_as_comments() { let mut doc = fixture(); diff --git a/crates/automation/src/comments.rs b/crates/automation/src/comments.rs index f87fd0b58e..554325dd03 100644 --- a/crates/automation/src/comments.rs +++ b/crates/automation/src/comments.rs @@ -177,7 +177,23 @@ impl Automation { let doc = self.doc(a)?; let only = a.opt_int("page")?.map(|p| (p.max(1) - 1) as usize); let all = self.comments(a)?; + // A group (`/RT /Group`, ISO 32000-2 §12.5.6.2) lists as its primary, with the primary's + // attributes: the subordinates are neither comments nor replies, only its `group`. + let mut members: HashMap<(usize, usize), Vec<&Annotation>> = HashMap::new(); + let mut primary_of: HashMap<(usize, usize), (usize, usize)> = HashMap::new(); + for (m, p) in all.iter().zip(pdfcraft_render::group_primaries(&all)) { + if let Some(p) = p.and_then(|p| all.get(p)) { + members.entry((p.page, p.index)).or_default().push(m); + primary_of.insert((m.page, m.index), (p.page, p.index)); + } + } let view = |c: &Annotation| -> Value { + let group: Vec = members + .get(&(c.page, c.index)) + .into_iter() + .flatten() + .map(|m| json!({ "id": m.name, "page": m.page + 1, "index": m.index + 1, "type": m.subtype })) + .collect(); let mut v = json!({ "id": c.name, "page": c.page + 1, @@ -188,6 +204,7 @@ impl Automation { "modified": c.modified, "rect": crate::view_rect(&doc.info, c.page, c.shown_rect(doc.info.pages.get(c.page).map_or(0, |p| p.rotation))), "color": c.color.map(hex), + "group": group, }); // A line's ends, or the corners of connected lines, a polygon or a cloud, as // comment_edit's `points` takes them. @@ -204,13 +221,14 @@ impl Automation { // a reply to a reply keeps its `/IRT` on the direct parent, so following one level would // drop it. Parents are found by reference, as `/NM` need not be unique. A status or // checkmark belongs to the annotation its `/IRT` names (ISO 32000-2 §12.5.6.2), as in the - // Comments panel: one set on a reply is not the comment's. + // Comments panel: one set on a reply is not the comment's. A group subordinate is in no + // thread, and a reply to one (status included) is the group's, its primary's. let tops = pdfcraft_render::thread_tops(&all); let mut threads: HashMap<(usize, usize), Vec<&Annotation>> = HashMap::new(); - for r in &all { + for r in all.iter().filter(|r| !r.is_group_member()) { let Some(&top) = tops.get(&(r.page, r.index)) else { continue }; let key = match r.parent { - Some(parent) if r.state.is_some() => parent, + Some(parent) if r.state.is_some() => primary_of.get(&parent).copied().unwrap_or(parent), _ => top, }; threads.entry(key).or_default().push(r); diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index c6c200cece..1329e548d2 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -739,7 +739,7 @@ pub fn tools() -> Vec { .ro().cmd("measure.snap").with(schema(json!({"doc":doc(),"page":{"type":"integer","minimum":1},"at":point(),"tolerance":{"type":"number","minimum":0,"maximum":10000},"endpoints":{"type":"boolean"},"midpoints":{"type":"boolean"},"intersections":{"type":"boolean"},"paths":{"type":"boolean"}}), &["doc","page","at"])), t("measure_export", "Export measurements as CSV", "Atomically write saved measurement values, labels, authors and scale ratios as spreadsheet-safe CSV. Returns how many unsupported measurements were left out.") .cmd("measure.export").with(schema(json!({"doc":doc(),"out":path_arg()}), &["doc","out"])), - t("comment_list", "List comments", "Every comment (annotation other than links, form widgets and pop-ups) with its page, index, id, type, author, text, date, rectangle, colour, review status, checkmark and replies (each with its own status and checkmark).") + t("comment_list", "List comments", "Every comment (annotation other than links, form widgets and pop-ups) with its page, index, id, type, author, text, date, rectangle, colour, review status, checkmark and replies (each with its own status and checkmark). Annotations grouped with a comment (/RT /Group) are listed once, as that comment with its attributes, and named in its `group`.") .ro() .with(schema(json!({ "doc": doc(), "page": { "type": "integer", "minimum": 1, "description": "Only this page." } }), &["doc"])), t( diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 6a5765b911..34c837056b 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -2332,6 +2332,172 @@ trailer << /Root 1 0 R >> assert_eq!((&reply["marked"], &reply["status"]), (&json!(true), &json!("Accepted")), "{reply}"); } +// Contributor-original PDF: a Square, a Circle grouped with it (`/RT /Group`) that keeps its own +// author, text, colour, dates and subject, an ordinary reply, a reply to the Circle, a review +// status reply and a reply with a `/RT` name that is neither `R` nor `Group`. +const GROUP_FIXTURE: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R 6 0 R 7 0 R 8 0 R 9 0 R 10 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /Square /Rect [20 20 100 80] /NM (sq-primary) /T (Alice) /Contents (Primary) /C [1 0 0] >> endobj +6 0 obj << /Type /Annot /Subtype /Circle /Rect [180 120 260 180] /NM (circ-sub) /T (Bob) /Contents (Subordinate) /C [0 0 1] /IRT 5 0 R /RT /Group /M (D:20260101000000) /CreationDate (D:20260101000000) /Subj (Oval) >> endobj +7 0 obj << /Type /Annot /Subtype /Text /Rect [120 20 140 40] /NM (reply) /T (Carol) /Contents (Reply) /IRT 5 0 R >> endobj +8 0 obj << /Type /Annot /Subtype /Text /Rect [120 50 140 70] /NM (sub-reply) /T (Dan) /Contents (Reply to the subordinate) /IRT 6 0 R >> endobj +9 0 obj << /Type /Annot /Subtype /Text /Rect [120 80 140 100] /NM (status) /T (Eve) /IRT 5 0 R /StateModel /Review /State /Accepted >> endobj +10 0 obj << /Type /Annot /Subtype /Text /Rect [150 20 170 40] /NM (odd) /T (Fay) /Contents (Odd reply type) /IRT 5 0 R /RT /Bogus >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +/// ISO 32000-2 §12.5.6.2: an annotation with `/RT /Group` is a subordinate of the group whose +/// primary its `/IRT` names. It lists as one comment, with the primary's attributes; the +/// subordinate's own are ignored, and a reply to it joins the primary's thread. +#[test] +fn a_group_lists_as_its_primary() { + let dir = workdir("rt-group"); + std::fs::write(dir.join("group.pdf"), GROUP_FIXTURE).unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "group.pdf" }))["doc"].as_u64().unwrap(); + let check = |list: &Value, when: &str| { + assert_eq!(list["count"], 1, "{when}: one comment, the group's primary: {list}"); + let c = &list["comments"][0]; + assert_eq!((c["id"].as_str(), c["author"].as_str(), c["contents"].as_str()), (Some("sq-primary"), Some("Alice"), Some("Primary")), "{when}"); + assert_eq!(c["color"], "#FF0000", "{when}: the primary's colour"); + let group: Vec<&str> = c["group"].as_array().unwrap().iter().filter_map(|m| m["id"].as_str()).collect(); + assert_eq!(group, ["circ-sub"], "{when}: the subordinate is a member of the group"); + assert_eq!(c["group"][0]["type"], "Circle", "{when}"); + let mut replies: Vec<&str> = c["replies"].as_array().unwrap().iter().filter_map(|r| r["id"].as_str()).collect(); + replies.sort_unstable(); + assert_eq!(replies, ["odd", "reply", "sub-reply"], "{when}: an unknown /RT reads as a reply, a reply to the subordinate joins the thread"); + assert_eq!(c["status"], "Accepted", "{when}: a status reply is still a reply"); + let text = list.to_string(); + for leak in ["Bob", "Subordinate", "#0000FF"] { + assert!(!text.contains(leak), "{when}: the subordinate's own {leak:?} is ignored: {list}"); + } + }; + // As opened (the inspector), and after an edit (the engine's own list). + check(&ok(&mut a, "comment_list", json!({ "doc": doc })), "opened"); + ok(&mut a, "comment_add", json!({ "doc": doc, "page": 1, "type": "note", "at": [280, 20], "contents": "Other" })); + let after = ok(&mut a, "comment_list", json!({ "doc": doc })); + let primary = after["comments"].as_array().unwrap().iter().find(|c| c["id"] == "sq-primary").cloned().unwrap(); + check(&json!({ "count": 1, "comments": [primary] }), "edited"); + assert_eq!(after["count"], 2, "{after}"); + + // Deleting a member deletes the group (one undo step), as deleting the primary does. + ok(&mut a, "comment_delete", json!({ "doc": doc, "id": "circ-sub" })); + let left = ok(&mut a, "comment_list", json!({ "doc": doc })); + assert_eq!(left["count"], 1, "only the new note is left: {left}"); + assert!(!left.to_string().contains("sq-primary") && !left.to_string().contains("circ-sub"), "{left}"); + ok(&mut a, "edit_undo", json!({ "doc": doc })); + let back = ok(&mut a, "comment_list", json!({ "doc": doc })); + let primary = back["comments"].as_array().unwrap().iter().find(|c| c["id"] == "sq-primary").cloned().unwrap(); + check(&json!({ "count": 1, "comments": [primary] }), "undone"); +} + +// Contributor-original PDF: group subordinates whose `/IRT` chains loop (each other, and one +// itself), and a chain of two subordinates that leads to a primary. +const GROUP_LOOP_FIXTURE: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R 6 0 R 7 0 R 8 0 R 9 0 R 10 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /Circle /Rect [10 10 30 30] /NM (a) /Contents (Loop A) /IRT 6 0 R /RT /Group >> endobj +6 0 obj << /Type /Annot /Subtype /Circle /Rect [40 10 60 30] /NM (b) /Contents (Loop B) /IRT 5 0 R /RT /Group >> endobj +7 0 obj << /Type /Annot /Subtype /Circle /Rect [70 10 90 30] /NM (self) /Contents (Itself) /IRT 7 0 R /RT /Group >> endobj +8 0 obj << /Type /Annot /Subtype /Square /Rect [10 100 60 150] /NM (top) /Contents (Top) >> endobj +9 0 obj << /Type /Annot /Subtype /Circle /Rect [70 100 90 120] /NM (mid) /Contents (Middle) /IRT 8 0 R /RT /Group >> endobj +10 0 obj << /Type /Annot /Subtype /Circle /Rect [100 100 120 120] /NM (low) /Contents (Lowest) /IRT 9 0 R /RT /Group >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +/// Looping `/IRT` chains between group subordinates neither hang nor panic, and list as comments +/// of their own, as looping replies do; a subordinate of a subordinate joins the group of the +/// primary the chain leads to. +#[test] +fn looping_groups_end_and_chained_groups_reach_their_primary() { + let dir = workdir("rt-group-loop"); + std::fs::write(dir.join("loop.pdf"), GROUP_LOOP_FIXTURE).unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "loop.pdf" }))["doc"].as_u64().unwrap(); + let list = ok(&mut a, "comment_list", json!({ "doc": doc })); + let comments = list["comments"].as_array().unwrap(); + let mut ids: Vec<&str> = comments.iter().filter_map(|c| c["id"].as_str()).collect(); + ids.sort_unstable(); + assert_eq!(ids, ["a", "b", "self", "top"], "{list}"); + let top = comments.iter().find(|c| c["id"] == "top").unwrap(); + let group: Vec<&str> = top["group"].as_array().unwrap().iter().filter_map(|m| m["id"].as_str()).collect(); + assert_eq!(group, ["mid", "low"], "{list}"); + assert_eq!(top["replies"], json!([]), "{list}"); + // Deleting a looping member ends too, and takes its loop with it. + ok(&mut a, "comment_delete", json!({ "doc": doc, "id": "a" })); + ok(&mut a, "comment_delete", json!({ "doc": doc, "id": "self" })); + ok(&mut a, "comment_delete", json!({ "doc": doc, "id": "low" })); + let info = ok(&mut a, "doc_info", json!({ "doc": doc })); + assert_eq!(ok(&mut a, "comment_list", json!({ "doc": doc }))["count"], 0, "{info}"); +} + +// Contributor-original PDF: a Replace Text group as earlier PdfCraft builds wrote it, the +// strikeout (primary) with empty `/Contents` and the replacement only on the caret (subordinate). +const OLD_REPLACE_FIXTURE: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R 6 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /StrikeOut /Rect [20 100 120 120] /QuadPoints [20 120 120 120 20 100 120 100] /NM (strike) /T (Ada) /Contents () /IT /StrikeOutTextEdit >> endobj +6 0 obj << /Type /Annot /Subtype /Caret /Rect [115 95 125 105] /NM (caret) /T (Ada) /Contents (red) /IRT 5 0 R /RT /Group /Subj (Replace Text) >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +// Contributor-original PDF: a Replace Text group with the caret as the primary (holding the +// text, `/IT /Replace`) and the strikeout as its subordinate, each with its own author, colour +// and dates. +const REPLACE_FIXTURE: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R 6 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /Caret /Rect [115 95 125 105] /NM (caret) /T (Ada) /C [0 0 1] /Contents (busy) /IT /Replace /Subj (Replace Text) /M (D:20260101000000) >> endobj +6 0 obj << /Type /Annot /Subtype /StrikeOut /Rect [20 100 120 120] /QuadPoints [20 120 120 120 20 100 120 100] /NM (strike) /T (Grace) /C [1 0 0] /IRT 5 0 R /RT /Group /IT /StrikeOutTextEdit /Subj (Cross-Out) /M (D:20260101000000) >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +/// A Replace Text comment lists once, with its replacement text: one made now (the caret is the +/// primary and holds it), one with that orientation from another writer, and one saved by an +/// earlier PdfCraft (the strikeout primary with no text, the caret holding it), which is read +/// leniently and not rewritten. +#[test] +fn replace_text_lists_its_replacement() { + let dir = workdir("rt-group-replace"); + std::fs::write(dir.join("old.pdf"), OLD_REPLACE_FIXTURE).unwrap(); + std::fs::write(dir.join("other.pdf"), REPLACE_FIXTURE).unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap(); + ok(&mut a, "comment_add", json!({ "doc": doc, "page": 1, "type": "replace", "find": "Page 1", "contents": "red" })); + let list = ok(&mut a, "comment_list", json!({ "doc": doc })); + assert_eq!(list["count"], 1, "{list}"); + let c = &list["comments"][0]; + assert_eq!( + (c["type"].as_str(), c["contents"].as_str(), c["group"][0]["type"].as_str()), + (Some("Caret"), Some("red"), Some("StrikeOut")), + "{list}" + ); + ok(&mut a, "doc_save", json!({ "doc": doc, "path": "new.pdf" })); + let mut b = auto(&dir); + for (path, text, primary) in [("new.pdf", "red", "Caret"), ("other.pdf", "busy", "Caret"), ("old.pdf", "red", "StrikeOut")] { + let doc = ok(&mut b, "doc_open", json!({ "path": path }))["doc"].as_u64().unwrap(); + let list = ok(&mut b, "comment_list", json!({ "doc": doc })); + let c = &list["comments"][0]; + assert_eq!( + (list["count"].as_u64(), c["type"].as_str(), c["contents"].as_str()), + (Some(1), Some(primary), Some(text)), + "{path} as opened: {list}" + ); + assert!(!list.to_string().contains("Grace"), "{path}: the subordinate's author is ignored: {list}"); + // And from the engine's own list, after an edit. + ok(&mut b, "comment_add", json!({ "doc": doc, "page": 1, "type": "note", "at": [280, 20], "contents": "Other" })); + let list = ok(&mut b, "comment_list", json!({ "doc": doc })); + assert_eq!(list["count"], 2, "{path}: {list}"); + let c = list["comments"].as_array().unwrap().iter().find(|c| c["type"] == primary).cloned().unwrap(); + assert_eq!(c["contents"], text, "{path} after an edit: {list}"); + } +} + #[test] fn protecting_through_tools() { let dir = workdir("protect"); @@ -6173,8 +6339,13 @@ fn drawing_comments_through_tools() { // Replace Text: a strikeout over the found text and a grouped caret. ok(&mut a, "comment_add", json!({ "doc": doc, "page": 2, "type": "replace", "find": "page 2", "contents": "second page" })); let list = ok(&mut a, "comment_list", json!({ "doc": doc, "page": 2 })); - let strike = list["comments"].as_array().unwrap().iter().find(|c| c["type"] == "StrikeOut").unwrap().clone(); - assert_eq!(strike["replies"][0]["contents"], "second page", "the caret threads under the strikeout"); + let caret = list["comments"].as_array().unwrap().iter().find(|c| c["type"] == "Caret").unwrap().clone(); + // One comment, the caret (the group's primary, ISO 32000-2 §12.5.6.2), holding the new text; + // the strikeout is in its group, not a reply or a comment of its own. + assert_eq!(caret["contents"], "second page", "{list}"); + assert_eq!(caret["replies"], json!([]), "{list}"); + assert_eq!(caret["group"][0]["type"], "StrikeOut", "{list}"); + assert!(!list["comments"].as_array().unwrap().iter().any(|c| c["type"] == "StrikeOut"), "{list}"); assert!(matches!(a.call("comment_add", &json!({ "doc": doc, "page": 2, "type": "replace", "find": "page 2" })), Err(ToolError::InvalidArgs(_)))); // A file attached as a comment. std::fs::write(dir.join("notes.txt"), b"remember").unwrap(); diff --git a/crates/engine/src/comments.rs b/crates/engine/src/comments.rs index eee799ed2f..2f8fa07ef8 100644 --- a/crates/engine/src/comments.rs +++ b/crates/engine/src/comments.rs @@ -27,6 +27,7 @@ pub(crate) fn comment_list(doc: &pdfcraft_cos::Document) -> Vec &str { } } -/// [`comment_type_name`], telling callouts, clouds and typewriter text apart by `/IT`. +/// [`comment_type_name`], telling replaced text, callouts, clouds and typewriter text apart by `/IT`. pub fn comment_kind(a: &pdfcraft_render::Annotation) -> &str { match a.intent.as_deref() { + Some("StrikeOutTextEdit" | "Replace") => "Replace Text", Some("FreeTextCallout") => "Callout", Some("PolygonCloud") => "Cloud", Some("FreeTextTypeWriter") => "Typewriter", @@ -105,19 +107,26 @@ pub fn comment_summary(name: &str, all: &[pdfcraft_render::Annotation], sort: Su } // Write a comment's replies, indenting each level. A reply to a reply keeps its `/IRT` on the // direct parent, so following one level would leave the deeper replies out of the summary. - fn write_replies(out: &mut String, children: &Children<'_>, parent: &pdfcraft_render::Annotation, depth: usize) { + // A group subordinate (`/RT /Group`, ISO 32000-2 §12.5.6.2) is part of the comment, not a + // reply: it is not written, and replies to it are written at its level, as the comment's. + // `depth` bounds the walk; `level` is the indent. + fn write_replies(out: &mut String, children: &Children<'_>, parent: &pdfcraft_render::Annotation, depth: usize, level: usize) { use std::fmt::Write; if depth > 64 { return; } for r in children.get(&(parent.page, parent.index)).into_iter().flatten() { - let indent = " ".repeat(depth + 1); + if r.is_group_member() { + write_replies(out, children, r, depth + 1, level); + continue; + } + let indent = " ".repeat(level + 1); let _ = writeln!(out, "{indent}Author: {} Subject: Reply Date: {}", r.author.as_deref().unwrap_or(""), r.modified.as_deref().unwrap_or("")); for line in r.contents.as_deref().unwrap_or("").lines() { let _ = writeln!(out, "{indent}{line}"); } - write_replies(out, children, r, depth + 1); + write_replies(out, children, r, depth + 1, level + 1); } } let mut numbered: Vec<(usize, &pdfcraft_render::Annotation)> = Vec::new(); @@ -156,7 +165,7 @@ pub fn comment_summary(name: &str, all: &[pdfcraft_render::Annotation], sort: Su if let Some(c) = a.contents.as_deref().filter(|c| !c.is_empty()) { let _ = writeln!(out, "{c}"); } - write_replies(&mut out, &children, a, 0); + write_replies(&mut out, &children, a, 0, 0); out.push('\n'); } out diff --git a/crates/engine/src/edit/run.rs b/crates/engine/src/edit/run.rs index c6b1a7438b..bd2153e3bf 100644 --- a/crates/engine/src/edit/run.rs +++ b/crates/engine/src/edit/run.rs @@ -361,8 +361,8 @@ pub(crate) fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut E } Edit::DeleteAttachment { key } => pdfcraft_organize::delete_attachment(doc, key)?, Edit::ReplaceText { page, quads, text, author, strike, caret } => { - // One `/NM` each: the caret replies to the strikeout, and XFDF names that reply by - // the strikeout's `/NM` (#2666). + // One `/NM` each: the strikeout is grouped with the caret, and XFDF names that link by + // the caret's `/NM` (#2666). let (strike_meta, caret_meta) = (cx.meta(), cx.meta()); pdfcraft_annot::add_text_replacement( doc, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 833fa16b9f..a6ced87376 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4413,6 +4413,21 @@ fn comment_summary_includes_replies_to_replies() { assert!(text.contains(" NESTED_REPLY"), "the nested reply is indented under its parent: {text}"); } +/// ISO 32000-2 §12.5.6.2: a group subordinate (`/RT /Group`) is part of its primary's comment, +/// not a reply: the summary leaves it out and writes the replies to it as the comment's. +#[test] +fn comment_summary_leaves_group_members_out_and_keeps_their_replies() { + let (mut s, id) = session_with(1); + s.apply(id, rect_comment(0, [40.0, 40.0, 90.0, 90.0])).unwrap(); + s.apply(id, Edit::ReplyToAnnotation { page: 0, index: 0, text: "MEMBER_TEXT".into(), author: "Bob".into() }).unwrap(); + s.apply(id, Edit::ReplyToAnnotation { page: 0, index: 1, text: "REPLY_TO_MEMBER".into(), author: "Carol".into() }).unwrap(); + let mut all = s.get(id).unwrap().info.annotations.clone(); + all.iter_mut().find(|a| a.contents.as_deref() == Some("MEMBER_TEXT")).unwrap().grouped = true; + let text = comment_summary("doc.pdf", &all, SummarySort::Page); + assert!(!text.contains("MEMBER_TEXT") && !text.contains("Bob"), "{text}"); + assert!(text.lines().any(|l| l == " REPLY_TO_MEMBER"), "the reply is the comment's, one level in: {text}"); +} + #[test] fn reading_colors_set_and_invalidated() { let (mut s, id) = session_with(1); diff --git a/crates/render/src/inspect.rs b/crates/render/src/inspect.rs index 0cf287b3f1..554fbbdd8a 100644 --- a/crates/render/src/inspect.rs +++ b/crates/render/src/inspect.rs @@ -36,8 +36,8 @@ pub use core::{check_pages, inspect, pretty_date}; #[cfg(test)] use core::{inspect_structure, spend}; pub use model::{ - Annotation, Attachment, AttachmentSource, DestView, Field, FieldKind, FontInfo, Layer, LayerOp, Link, LinkTarget, OutlineItem, PageInfo, - PageTransition, RemoteDestination, TransitionStyle, Xfa, link_replies, thread_tops, + Annotation, Attachment, AttachmentSource, DestView, Field, FieldKind, FontInfo, Layer, LayerOp, Link, LinkTarget, MAX_GROUP_DEPTH, OutlineItem, + PageInfo, PageTransition, RemoteDestination, TransitionStyle, Xfa, fill_group_text, group_primaries, link_replies, thread_tops, }; #[cfg(test)] use navigation::{OUTLINE_DEPTH, OUTLINE_WINDOW}; diff --git a/crates/render/src/inspect/annotations.rs b/crates/render/src/inspect/annotations.rs index 603aa88110..2265d78e7f 100644 --- a/crates/render/src/inspect/annotations.rs +++ b/crates/render/src/inspect/annotations.rs @@ -1,5 +1,7 @@ use super::core::{limited_pdf_string, pretty_date, spend}; -use super::model::{Annotation, AttachmentSource, DestView, Field, FieldKind, LayerOp, Link, LinkTarget, RemoteDestination, link_replies}; +use super::model::{ + Annotation, AttachmentSource, DestView, Field, FieldKind, LayerOp, Link, LinkTarget, RemoteDestination, fill_group_text, link_replies, +}; use super::navigation::text_string; use super::{Dictionary, DocInfo, Entry, HashMap, HashSet, Inspector, Object, ObjectId}; @@ -190,6 +192,7 @@ impl<'a> Inspector<'a> { } info.annotations.sort_by(|a, b| a.page.cmp(&b.page).then(b.rect[3].total_cmp(&a.rect[3]))); link_replies(&mut info.annotations); + fill_group_text(&mut info.annotations); } fn link(&self, page: usize, index: usize, d: &Dictionary, text_budget: &mut usize) -> Option { diff --git a/crates/render/src/inspect/model.rs b/crates/render/src/inspect/model.rs index 490897b60f..3686af8438 100644 --- a/crates/render/src/inspect/model.rs +++ b/crates/render/src/inspect/model.rs @@ -315,8 +315,9 @@ pub struct Annotation { /// names the first comment on every page `fitz-A0`. `None` when `/IRT` leads to no listed /// comment ([`link_replies`]). pub parent: Option<(usize, usize)>, - /// `/RT /Group`: grouped with the annotation `/IRT` points at (Replace Text's caret with its - /// strikeout) rather than a reply to it. + /// `/RT /Group` (ISO 32000-2 §12.5.6.2, Table 172): with a `parent`, a subordinate of the + /// group whose primary `/IRT` points at, not a reply ([`Self::is_group_member`]). Its own + /// text, author, colour and dates are ignored; see [`group_primaries`]. pub grouped: bool, /// Rect in PDF user space: [x0, y0, x1, y1]. pub rect: [f32; 4], @@ -343,6 +344,65 @@ pub struct Annotation { pub points: Vec<[f32; 2]>, } +/// How far [`group_primaries`] follows `/IRT` from a group subordinate to its primary. +pub const MAX_GROUP_DEPTH: usize = 64; + +/// For each annotation of `all`, the position in `all` of the primary of the group it is a +/// subordinate of ([`Annotation::is_group_member`], ISO 32000-2 §12.5.6.2), or `None` when it is +/// not one. The group's attributes, its comment and its replies are the primary's. A subordinate +/// of a subordinate leads to the first non-subordinate up the `/IRT` chain. Expects links +/// [`link_replies`] settled: a subordinate whose chain loops or names nothing listed has lost its +/// `parent` there and is a comment of its own, as such a reply is. One still on a subordinate +/// after [`MAX_GROUP_DEPTH`] steps has no primary (`None`) and is listed under no comment. +pub fn group_primaries(all: &[Annotation]) -> Vec> { + let mut at: HashMap<(usize, usize), usize> = HashMap::new(); + for (i, a) in all.iter().enumerate() { + at.entry((a.page, a.index)).or_insert(i); + } + let primary = |start: usize| -> Option { + let mut i = start; + for _ in 0..MAX_GROUP_DEPTH { + let a = all.get(i)?; + if !a.is_group_member() { + return Some(i); + } + i = *at.get(&a.parent?)?; + } + None + }; + all.iter().enumerate().map(|(i, a)| if a.is_group_member() { primary(i) } else { None }).collect() +} + +/// Give a group's StrikeOut primary with no text of its own (`/Contents` missing or empty) the +/// text of its first Caret subordinate that has some. A reader ignores a subordinate's +/// `/Contents` (ISO 32000-2 §12.5.6.2), but Replace Text groups written by earlier PdfCraft builds +/// made the strikeout the primary, with no text, and kept the replacement only on the caret: +/// without this they would list as a comment with no text. A primary's own text always wins, no +/// other subordinate lends its text, and no other kind of primary borrows one. Only the listing +/// changes, not the file. +pub fn fill_group_text(all: &mut [Annotation]) { + if !all.iter().any(Annotation::is_group_member) { + return; + } + let primaries = group_primaries(all); + let lacks_text = |p: usize| all.get(p).is_some_and(|a| a.subtype == "StrikeOut" && a.contents.as_deref().is_none_or(str::is_empty)); + // Keyed by the primary's position: the first caret per primary wins, and each primary is + // written once, so the order of the map does not matter. + let mut texts: HashMap = HashMap::new(); + for (a, p) in all.iter().zip(&primaries) { + if let (Some(p), Some(text), "Caret") = (*p, a.contents.as_ref(), a.subtype.as_str()) + && lacks_text(p) + { + texts.entry(p).or_insert_with(|| text.clone()); + } + } + for (p, text) in texts { + if let Some(primary) = all.get_mut(p) { + primary.contents = Some(text); + } + } +} + impl Annotation { /// A checkmark reply (`/StateModel /Marked`): "Marked" or "Unmarked" rather than a review status. pub fn is_mark(&self) -> bool { @@ -361,6 +421,13 @@ impl Annotation { !self.is_reply() && self.state.is_none() } + /// A subordinate of a group (`/RT /Group` with an `/IRT` that leads to a listed comment, ISO + /// 32000-2 §12.5.6.2): part of its primary's comment ([`group_primaries`]), neither a comment + /// of its own nor one of its replies. Expects links [`link_replies`] settled. + pub fn is_group_member(&self) -> bool { + self.grouped && self.is_reply() + } + /// Whether this annotation's `/IRT` points at `other`. pub fn replies_to(&self, other: &Annotation) -> bool { self.parent == Some((other.page, other.index)) diff --git a/crates/render/src/inspect/tests.rs b/crates/render/src/inspect/tests.rs index a5bfec474d..b17c155662 100644 --- a/crates/render/src/inspect/tests.rs +++ b/crates/render/src/inspect/tests.rs @@ -1870,3 +1870,131 @@ trailer << /Root 1 0 R >> assert!((p.width - 3874.96).abs() < 0.1, "width expected ~3874.96, got {}", p.width); assert!((p.height - 5493.54).abs() < 0.1, "height expected ~5493.54, got {}", p.height); } + +mod group_tests { + use crate::inspect::{Annotation, MAX_GROUP_DEPTH, group_primaries, link_replies}; + + /// The annotation at `/Annots` position `index` of page 0, whose `/IRT` points at position + /// `parent`, with `/RT /Group` when `grouped`. + fn annot(index: usize, parent: Option, grouped: bool) -> Annotation { + Annotation { + page: 0, + subtype: "Circle".into(), + author: None, + contents: None, + modified: None, + name: Some(index.to_string()), + in_reply_to: parent.map(|p| p.to_string()), + parent: parent.map(|p| (0, p)), + grouped, + rect: [0.0; 4], + color: None, + index, + state: None, + quads: Vec::new(), + locked: false, + intent: None, + fill_sign: false, + no_rotate: false, + points: Vec::new(), + } + } + + /// `all` with its reply links settled, as both comment readers list it. + fn listed(mut all: Vec) -> Vec { + link_replies(&mut all); + all + } + + /// ISO 32000-2 §12.5.6.2: a subordinate's primary is the annotation its `/IRT` points at, + /// past any other subordinate; a reply is no subordinate. A loop, or an `/IRT` to nothing + /// listed, leaves a comment of its own (as `link_replies` leaves such a reply), and an + /// over-long chain ends. + #[test] + fn group_primaries_follow_irt_to_the_first_non_subordinate() { + let all = listed(vec![ + annot(0, None, false), // top + annot(1, Some(0), true), // mid + annot(2, Some(1), true), // low + annot(3, Some(0), false), // a reply + annot(4, Some(5), true), // a loop + annot(5, Some(4), true), // of two + annot(6, Some(6), true), // itself + annot(7, Some(99), true), // nothing listed + annot(8, Some(3), true), // grouped with the reply + ]); + assert_eq!(group_primaries(&all), [None, Some(0), Some(0), None, None, None, None, None, Some(3)]); + assert!(all.iter().skip(4).take(4).all(Annotation::is_comment), "{all:?}"); + // A chain longer than the cap has no primary; one within it does, whether the reply links + // are settled first or not. + for (len, found) in [(MAX_GROUP_DEPTH - 1, true), (MAX_GROUP_DEPTH + 1, false)] { + let mut chain = vec![annot(0, None, false)]; + chain.extend((1..=len).map(|i| annot(i, Some(i - 1), true))); + assert_eq!(group_primaries(&chain).last().copied().flatten().is_some(), found, "{len}"); + assert_eq!(group_primaries(&listed(chain)).last().copied().flatten().is_some(), found, "{len}, listed"); + } + } + + fn with(mut a: Annotation, subtype: &str, t: Option<&str>) -> Annotation { + a.subtype = subtype.into(); + a.contents = t.map(Into::into); + a + } + + /// A StrikeOut primary with no text lists its first Caret subordinate's (earlier Replace + /// Text groups kept the replacement on the caret only); a primary's own text always wins, a + /// subordinate of another type lends nothing, and a primary of another type borrows nothing + /// (ISO 32000-2 §12.5.6.2: the group's text is the primary's). + #[test] + fn a_primary_without_text_lists_its_carets() { + let mut all = listed(vec![ + with(annot(0, None, false), "StrikeOut", None), + with(annot(1, Some(0), true), "Caret", Some("red")), + with(annot(2, Some(0), true), "Caret", Some("second")), + with(annot(3, None, false), "Caret", Some("kept")), + with(annot(4, Some(3), true), "Caret", Some("ignored")), + with(annot(5, Some(0), false), "Text", Some("a reply")), + with(annot(6, None, false), "StrikeOut", None), + with(annot(7, Some(6), true), "Circle", Some("not lent")), + with(annot(8, None, false), "Square", None), + with(annot(9, Some(8), true), "Caret", Some("not borrowed")), + ]); + super::fill_group_text(&mut all); + let shown: Vec> = all.iter().map(|a| a.contents.as_deref()).collect(); + assert_eq!( + shown, + [ + Some("red"), + Some("red"), + Some("second"), + Some("kept"), + Some("ignored"), + Some("a reply"), + None, + Some("not lent"), + None, + Some("not borrowed"), + ] + ); + } + + /// Many groups each get their own caret's text (one map lookup per caret, not a scan of the + /// groups seen so far). + #[test] + fn many_groups_each_list_their_own_caret() { + const N: usize = 20_000; + let mut all = Vec::with_capacity(2 * N); + for i in 0..N { + all.push(with(annot(i, None, false), "StrikeOut", None)); + } + // Carets in reverse order, so a primary's caret is far from it in the list. + for (k, i) in (0..N).rev().enumerate() { + all.push(with(annot(N + k, Some(i), true), "Caret", Some(&format!("t{i}")))); + } + let mut all = listed(all); + super::fill_group_text(&mut all); + for (i, a) in all.iter().take(N).enumerate() { + assert_eq!(a.contents.as_deref(), Some(format!("t{i}").as_str()), "{i}"); + } + } +} diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 954823ef09..f623945a7f 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -17,8 +17,9 @@ pub mod text; pub mod transition; pub use inspect::{ - Annotation, Attachment, AttachmentSource, DestView, DocInfo, Field, FieldKind, FontInfo, Layer, LayerOp, Link, LinkTarget, OutlineItem, PageInfo, - PageTransition, RemoteDestination, TransitionStyle, Xfa, attachment_data, check_pages, inspect, link_replies, pretty_date, thread_tops, + Annotation, Attachment, AttachmentSource, DestView, DocInfo, Field, FieldKind, FontInfo, Layer, LayerOp, Link, LinkTarget, MAX_GROUP_DEPTH, + OutlineItem, PageInfo, PageTransition, RemoteDestination, TransitionStyle, Xfa, attachment_data, check_pages, fill_group_text, group_primaries, + inspect, link_replies, pretty_date, thread_tops, }; pub use pixels::Pixels; pub use raster::{ diff --git a/crates/ui-egui/src/panels/comments_panel.rs b/crates/ui-egui/src/panels/comments_panel.rs index c549ed8e3e..58506efcda 100644 --- a/crates/ui-egui/src/panels/comments_panel.rs +++ b/crates/ui-egui/src/panels/comments_panel.rs @@ -40,7 +40,9 @@ pub fn subtype_label(s: &str) -> &str { /// The comment type as the panel names it, telling callouts, clouds and replaced text apart by `/IT`. pub fn kind_label(a: &Annotation) -> &str { match a.intent.as_deref() { - Some("StrikeOutTextEdit") => "Replace text", + // Replace Text: the caret (`/IT /Replace`) is the group's primary; earlier builds made + // the strikeout primary. + Some("StrikeOutTextEdit" | "Replace") => "Replace text", Some("FreeTextCallout") => "Callout", Some("PolygonCloud") => "Cloud", Some("FreeTextTypeWriter") => "Typewriter", @@ -95,10 +97,10 @@ pub fn count(info: &DocInfo) -> usize { info.annotations.iter().filter(|a| a.is_comment()).count() } -/// The replies under the header's count, at any depth: not status changes, checkmarks or a -/// Replace Text caret (grouped with its strikeout, not a reply to it). +/// The replies under the header's count, at any depth: not status changes, checkmarks or group +/// subordinates (`/RT /Group`, such as Replace Text's strikeout: part of the comment, not a reply). pub fn reply_count(info: &DocInfo) -> usize { - info.annotations.iter().filter(|a| a.is_reply() && a.state.is_none() && !a.grouped).count() + info.annotations.iter().filter(|a| a.is_reply() && a.state.is_none() && !a.is_group_member()).count() } fn matches(a: &Annotation, q: &str) -> bool { @@ -181,11 +183,29 @@ pub(crate) fn show( // it. A status or checkmark stays with the annotation it was set on (its `/IRT` names it, // ISO 32000-2 §12.5.6.2): one set on a reply, at any depth, is that reply's and not the // comment's, so the card's badge and checkmark leave it out (comment_list reports it on the - // reply). Page badges count roots before filtering. + // reply). Page badges count roots before filtering. A group (`/RT /Group`, ISO 32000-2 + // §12.5.6.2) is one comment, its primary's: a subordinate is no reply, and a reply to one + // (a status too) is the group's. let tops = pdfcraft_render::thread_tops(&info.annotations); + let primary_of: HashMap<(usize, usize), (usize, usize)> = if info.annotations.iter().any(Annotation::is_group_member) { + let all = &info.annotations; + all.iter() + .zip(pdfcraft_render::group_primaries(all)) + .filter_map(|(a, p)| all.get(p?).map(|p| ((a.page, a.index), (p.page, p.index)))) + .collect() + } else { + HashMap::new() + }; for a in &info.annotations { + if a.is_group_member() { + continue; + } if let Some(parent) = a.parent { - let key = if a.state.is_none() { tops.get(&(a.page, a.index)).copied().unwrap_or(parent) } else { parent }; + let key = if a.state.is_none() { + tops.get(&(a.page, a.index)).copied().unwrap_or(parent) + } else { + primary_of.get(&parent).copied().unwrap_or(parent) + }; replies.entry(key).or_default().push(a); } else if a.is_comment() { let count = page_counts.entry(a.page).or_default(); diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index 5e74fdf56e..6e948bcf21 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -91,7 +91,13 @@ pub(crate) fn hit(cx: &PageCx<'_>, view: &DocView, p: Pos2) -> Option { return Some(Hit::new(Owner::Comment { index: a.index, part: CommentPart::Handle(x, y) }, Layer::Handle, icon)); } } - let a = cx.hit(p).filter(|a| bodies && pick(a))?; + // A group subordinate (`/RT /Group`) stands for its primary (ISO 32000-2 §12.5.6.2): a click + // there selects the group. Only a click: a drag still moves what is under the pointer. + let (a, member) = match cx.hit(p) { + Some(a) => (a, false), + None => (cx.click_hit(p)?, true), + }; + let a = Some(a).filter(|a| bodies && pick(a))?; // A click on a highlight over a link follows the link, so the link's hand and tip show there. if cx.tool == QuickTool::Select && a.subtype == "Highlight" && on_link(cx, p) { return None; @@ -102,7 +108,7 @@ pub(crate) fn hit(cx: &PageCx<'_>, view: &DocView, p: Pos2) -> Option { // page, over a form field; text markup lies on the text, under one. let layer = if a.fill_sign || !markup { Layer::Placed } else { Layer::Comment }; // Text markup does not move: a click selects it, a drag across it selects the text. - Some(Hit { declines_drag: markup, ..Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer, icon) }) + Some(Hit { declines_drag: markup || member, ..Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer, icon) }) } /// Whether the comments take the press on this page (or, with no button down, would take one diff --git a/crates/ui-egui/src/tools/comments/paint.rs b/crates/ui-egui/src/tools/comments/paint.rs index bfcd67982b..93941a3814 100644 --- a/crates/ui-egui/src/tools/comments/paint.rs +++ b/crates/ui-egui/src/tools/comments/paint.rs @@ -34,6 +34,12 @@ pub(crate) fn paint_page(ui: &egui::Ui, painter: &egui::Painter, cx: &PageCx<'_> && page == cx.page && let Some(a) = cx.get(index) { + // The rest of the group is selected with its primary. + for (m, _) in cx.group_members().into_iter().filter(|(_, p)| p.index == index) { + for r in cx.screen_rects(m) { + painter.rect_stroke(r.expand(2.0), CornerRadius::ZERO, Stroke::new(1.0, SELECT_BLUE), egui::StrokeKind::Middle); + } + } if is_markup(&a.subtype) && !a.quads.is_empty() { for r in cx.screen_rects(a) { painter.rect_stroke(r.expand(2.0), CornerRadius::ZERO, Stroke::new(1.0, SELECT_BLUE), egui::StrokeKind::Middle); diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index ab24352256..304c687b54 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -391,6 +391,29 @@ impl PageCx<'_> { self.comments().find(|a| a.index == index) } + /// The group subordinates (`/RT /Group`, ISO 32000-2 §12.5.6.2) on this page, each with the + /// comment on this page that is its group's primary. + pub(super) fn group_members(&self) -> Vec<(&Annotation, &Annotation)> { + let all = &self.info.annotations; + // Painted every frame a comment is selected: most documents have no group to look up. + if self.hidden || !all.iter().any(|a| a.is_group_member() && a.page == self.page) { + return Vec::new(); + } + let primaries = pdfcraft_render::group_primaries(all); + all.iter() + .zip(primaries) + .filter(|(a, _)| a.is_group_member() && a.page == self.page) + .filter_map(|(a, p)| Some((a, all.get(p?).filter(|p| p.page == self.page && p.is_comment())?))) + .collect() + } + + /// The comment a click at `p` selects: the topmost one under it or, on a group subordinate, + /// the group's primary, which stands for the group. + pub(super) fn click_hit(&self, p: Pos2) -> Option<&Annotation> { + self.hit(p) + .or_else(|| self.group_members().into_iter().rev().find(|(m, _)| self.screen_rects(m).any(|r| r.expand(3.0).contains(p))).map(|(_, p)| p)) + } + pub(crate) fn rect_to_user(&self, r: Rect) -> [f64; 4] { let (a, b) = (self.to_user(r.left_top()), self.to_user(r.right_bottom())); [a[0].min(b[0]), a[1].min(b[1]), a[0].max(b[0]), a[1].max(b[1])] diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 664f61733e..3e078b6d49 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -2205,7 +2205,9 @@ fn replacing_text_strikes_it_and_adds_a_caret() { let strike = c.iter().find(|a| a.subtype == "StrikeOut").expect("a strikeout"); let caret = c.iter().find(|a| a.subtype == "Caret").expect("a caret"); assert_eq!(caret.contents.as_deref(), Some("slow red")); - assert_eq!(caret.in_reply_to, strike.name, "grouped with the strikeout"); + // The caret is the group's primary, the strikeout its `/RT /Group` subordinate (ISO 32000-2 §12.5.6.2). + assert_eq!(strike.in_reply_to, caret.name, "the strikeout is grouped with the caret"); + assert!(strike.is_group_member() && !caret.grouped && !caret.is_reply()); assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().can_undo(), Some("Replace text")); assert_eq!(h.state().quick_tool, QuickTool::Select); // The strikeout is the Replace text tool's, not Strikethrough's (#3947): the panel names it @@ -2213,7 +2215,13 @@ fn replacing_text_strikes_it_and_adds_a_caret() { assert_eq!(pdfcraft_ui_egui::comments::tool_for(strike), Some(pdfcraft_ui_egui::comments::CommentTool::ReplaceText)); h.state_mut().set_option("panel", "comments").unwrap(); h.run_steps(3); + // One card, the caret's, showing the replacement text; sorted by type, its group is named + // Replace text (not Insert text, and no Strikethrough). + assert_eq!(h.query_all_by_label("slow red").count(), 1, "one comment with the replacement text"); + h.state_mut().views[0].comments.sort = pdfcraft_ui_egui::comments::SortBy::Type; + h.run_steps(3); assert!(h.query_by_role_and_label(egui::accesskit::Role::Label, "Replace text").is_some(), "the panel names the comment Replace text"); + assert!(h.query_by_role_and_label(egui::accesskit::Role::Label, "Insert text").is_none(), "not Insert text"); assert!(h.query_by_role_and_label(egui::accesskit::Role::Label, "Strikethrough").is_none(), "not Strikethrough"); } @@ -3022,3 +3030,36 @@ fn an_upright_comment_on_a_turned_page_is_drawn_where_it_goes_before_release() { h.drop_at(from + delta); h.run_steps(3); } + +// Contributor-original PDF: a Square, a Circle grouped with it (`/RT /Group`) that keeps its own +// author, text and colour, an ordinary reply and a reply to the Circle. +const GROUP_FIXTURE: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R 6 0 R 7 0 R 8 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /Square /Rect [20 20 100 80] /NM (sq-primary) /T (Alice) /Contents (Primary) /C [1 0 0] >> endobj +6 0 obj << /Type /Annot /Subtype /Circle /Rect [180 120 260 180] /NM (circ-sub) /T (Bob) /Contents (Subordinate) /C [0 0 1] /IRT 5 0 R /RT /Group >> endobj +7 0 obj << /Type /Annot /Subtype /Text /Rect [120 20 140 40] /NM (reply) /T (Carol) /Contents (Plain reply) /IRT 5 0 R >> endobj +8 0 obj << /Type /Annot /Subtype /Text /Rect [120 50 140 70] /NM (sub-reply) /T (Dan) /Contents (Reply to the subordinate) /IRT 6 0 R >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +/// ISO 32000-2 §12.5.6.2: a group is one comment, its primary's. The panel lists the Square +/// with both replies (the one to the Circle joins its thread) and not the Circle's own author +/// or text; clicking the Circle on the page selects the group, which is the Square's card. +#[test] +fn a_group_is_one_comment_in_the_panel_and_on_the_page() { + let mut h = harness_with_bytes(GROUP_FIXTURE.to_vec(), |app| app.set_option("panel", "comments").unwrap()); + h.get_by_label("Primary"); + h.get_by_label("Plain reply"); + h.get_by_label("Reply to the subordinate"); + assert_eq!(h.query_all_by_label("Subordinate").count(), 0, "the subordinate's own text is ignored"); + assert_eq!(h.query_all_by_label("Bob").count(), 0, "the subordinate's own author is ignored"); + let p = at(&h, 220.0, 150.0); + click(&mut h, p); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "clicking the Circle selects its group"); + h.state_mut().views[0].comments.selected = None; + let p = at(&h, 60.0, 50.0); + click(&mut h, p); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0))); +} diff --git a/crates/xfdf/src/tests.rs b/crates/xfdf/src/tests.rs index 0da17c1e67..c758635938 100644 --- a/crates/xfdf/src/tests.rs +++ b/crates/xfdf/src/tests.rs @@ -142,9 +142,10 @@ 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. +/// #2666: Replace Text writes a Caret and a StrikeOut grouped with it (`/IRT`, `/RT /Group`). XFDF +/// names the `/IRT` annotation by its `/NM`, so when both carried one id the strikeout'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(); @@ -169,8 +170,8 @@ fn a_replace_text_pair_survives_an_xfdf_round_trip() { 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("inreplyto=\"caret\""), "never a reply to itself"); + assert!(xfdf.contains("inreplyto=\"caret\""), "the strikeout is grouped with the caret: {xfdf}"); + assert!(!xfdf.contains("inreplyto=\"strike\""), "never a reply to itself"); let mut dst = blank(); let r = import(&mut dst, xfdf.as_bytes()).unwrap(); @@ -179,6 +180,7 @@ fn a_replace_text_pair_survives_an_xfdf_round_trip() { 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"); + assert!(found.iter().any(|s| s.subtype == "StrikeOut" && s.is_group_member()), "still grouped (/RT /Group) with the caret: {found:?}"); } #[test] diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 1404247cd7..86d86d0a22 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -3595,8 +3595,8 @@ milestone = "M3" status = "shipped" commands = ["comment.list"] tools = ["comment_list"] -evidence = ["crates/ui-egui/tests/it/ui.rs::opening_a_pdf_shows_comments_and_bookmarks", "crates/ui-egui/tests/it/comments.rs::the_panel_posts_comments_and_replies", "crates/ui-egui/tests/it/comments.rs::page_groups_collapse_and_expand", "crates/ui-egui/tests/it/comments.rs::replies_follow_irt_references_when_names_repeat_on_every_page", "crates/engine/src/tests.rs::replies_thread_by_reference_when_names_repeat_across_pages", "crates/automation/tests/it/automation.rs::replies_thread_by_reference_when_ids_repeat_across_pages"] -notes = "cards grouped by page with replies (threaded by the `/IRT` reference, so files whose `/NM` repeats per page, as PyMuPDF writes them, thread correctly; the header counts comments without replies, as Acrobat does), status badges, selection, reply box and an Add a comment box; page groups fold and unfold from their chevron or with Collapse/Expand all pages in the panel's \"…\" menu (a comment selected on a folded page opens its group). Groups by author, type or colour don't fold yet" +evidence = ["crates/ui-egui/tests/it/ui.rs::opening_a_pdf_shows_comments_and_bookmarks", "crates/ui-egui/tests/it/comments.rs::the_panel_posts_comments_and_replies", "crates/ui-egui/tests/it/comments.rs::page_groups_collapse_and_expand", "crates/ui-egui/tests/it/comments.rs::replies_follow_irt_references_when_names_repeat_on_every_page", "crates/engine/src/tests.rs::replies_thread_by_reference_when_names_repeat_across_pages", "crates/automation/tests/it/automation.rs::replies_thread_by_reference_when_ids_repeat_across_pages", "crates/ui-egui/tests/it/comments.rs::a_group_is_one_comment_in_the_panel_and_on_the_page", "crates/automation/tests/it/automation.rs::a_group_lists_as_its_primary", "crates/automation/tests/it/automation.rs::looping_groups_end_and_chained_groups_reach_their_primary", "crates/render/src/inspect.rs::group_primaries_follow_irt_to_the_first_non_subordinate"] +notes = "cards grouped by page with replies (threaded by the `/IRT` reference, so files whose `/NM` repeats per page, as PyMuPDF writes them, thread correctly; the header counts comments without replies, as Acrobat does), status badges, selection, reply box and an Add a comment box; page groups fold and unfold from their chevron or with Collapse/Expand all pages in the panel's \"…\" menu (a comment selected on a folded page opens its group). Groups by author, type or colour don't fold yet. Annotation groups (`/RT /Group`, ISO 32000-2 §12.5.6.2) list as one comment with the primary's author, text and colour (the subordinate's own are ignored), a reply to a subordinate joins the primary's thread, `comment_list` names the members in `group`, clicking a subordinate on the page selects the group and deleting any member deletes the group (one undo step). Missing: moving or restyling a member does not move or restyle the group, a subordinate has no hover tooltip, and a subordinate whose primary is on another page is selected on neither" [[feature]] id = "comment.appearance-generation" @@ -3679,8 +3679,8 @@ milestone = "M5" status = "shipped" commands = ["comment.replace"] tools = ["comment_add"] -evidence = ["crates/annot/src/tests.rs::replace_text_groups_a_strikeout_and_a_caret", "crates/ui-egui/tests/it/comments.rs::replacing_text_strikes_it_and_adds_a_caret", "crates/automation/tests/it/automation.rs::drawing_comments_through_tools"] -notes = "strikeout (/IT StrikeOutTextEdit) and a grouped caret (/RT /Group) with the replacement" +evidence = ["crates/annot/src/tests.rs::replace_text_groups_a_strikeout_and_a_caret", "crates/ui-egui/tests/it/comments.rs::replacing_text_strikes_it_and_adds_a_caret", "crates/automation/tests/it/automation.rs::drawing_comments_through_tools", "crates/automation/tests/it/automation.rs::replace_text_lists_its_replacement"] +notes = "a caret holding the replacement (/IT /Replace) is the group's primary and a strikeout (/IT /StrikeOutTextEdit) its /RT /Group subordinate (ISO 32000-2 §12.5.6.2), so the pair lists, selects and deletes as one comment with the replacement text. Groups saved by earlier builds the other way round (strikeout primary with no text) still list the caret's text, without rewriting the file. Missing: moving the caret does not move the strikeout" [[feature]] id = "comment.insert-text"