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 01/10] 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" From 6085c9110eda60a17589f8693070469459399469 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 00:18:41 +0800 Subject: [PATCH 02/10] M5: a Replace Text group makes Replace Text defaults; an empty caret lends no text Review on #4358: - Make Current Properties Default on a Replace Text group selected the caret, the group's primary, and updated the Insert Text tool. A caret whose group holds a grouped /IT /StrikeOutTextEdit strikeout now updates the Replace Text tool; a caret of its own still updates Insert Text. Test: a_replace_text_group_makes_replace_text_defaults. - fill_group_text let an empty caret take the primary's slot, so a later caret with the replacement text was not listed. Only a caret with text lends it. Test: an_empty_caret_does_not_hide_the_next_carets_text. Both tests fail with their fix reverted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/render/src/inspect/model.rs | 5 ++-- crates/render/src/inspect/tests.rs | 13 +++++++++ crates/ui-egui/src/dialogs/comment_props.rs | 4 +-- crates/ui-egui/src/tools/comments.rs | 13 +++++++-- crates/ui-egui/src/tools/comments/tests.rs | 32 +++++++++++++++++++++ 5 files changed, 61 insertions(+), 6 deletions(-) diff --git a/crates/render/src/inspect/model.rs b/crates/render/src/inspect/model.rs index 3686af8438..8c9d75eb45 100644 --- a/crates/render/src/inspect/model.rs +++ b/crates/render/src/inspect/model.rs @@ -386,11 +386,12 @@ pub fn fill_group_text(all: &mut [Annotation]) { } 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. + // Keyed by the primary's position: the first caret with text per primary wins (an empty one + // lends nothing), 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()) + && !text.is_empty() && lacks_text(p) { texts.entry(p).or_insert_with(|| text.clone()); diff --git a/crates/render/src/inspect/tests.rs b/crates/render/src/inspect/tests.rs index b17c155662..afde323a7c 100644 --- a/crates/render/src/inspect/tests.rs +++ b/crates/render/src/inspect/tests.rs @@ -1978,6 +1978,19 @@ mod group_tests { ); } + /// Review on #4358: an empty caret ahead of one with text lends nothing, so the primary lists + /// the replacement rather than nothing. + #[test] + fn an_empty_caret_does_not_hide_the_next_carets_text() { + let mut all = listed(vec![ + with(annot(0, None, false), "StrikeOut", None), + with(annot(1, Some(0), true), "Caret", Some("")), + with(annot(2, Some(0), true), "Caret", Some("new words")), + ]); + super::fill_group_text(&mut all); + assert_eq!(all[0].contents.as_deref(), Some("new words")); + } + /// Many groups each get their own caret's text (one map lookup per caret, not a scan of the /// groups seen so far). #[test] diff --git a/crates/ui-egui/src/dialogs/comment_props.rs b/crates/ui-egui/src/dialogs/comment_props.rs index 4702e63f9e..a4a9892ac0 100644 --- a/crates/ui-egui/src/dialogs/comment_props.rs +++ b/crates/ui-egui/src/dialogs/comment_props.rs @@ -99,7 +99,7 @@ impl PdfCraftApp { let Some((_, id)) = self.active_ids() else { return }; let Some(doc) = self.session.get(id) else { return }; let Some(a) = doc.info.annotations.iter().find(|a| a.page == page && a.index == index && !a.is_reply()) else { return }; - let Some(tool) = crate::comments::tool_for_default(a, p.endings.as_deref()) else { return }; + let Some(tool) = crate::comments::tool_for_default(&doc.info.annotations, a, p.endings.as_deref()) else { return }; let mut style = self.comment_prefs.style(tool); if let Some(c) = p.color { style.color = c; @@ -180,7 +180,7 @@ pub(crate) fn body(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens) -> (boo let (_, id) = app.active_ids()?; let doc = app.session.get(id)?; let a = doc.info.annotations.iter().find(|a| a.page == d.page && a.index == d.index && !a.is_reply())?; - crate::comments::tool_for_default(a, d.edited.endings.as_deref()) + crate::comments::tool_for_default(&doc.info.annotations, a, d.edited.endings.as_deref()) }); let Some(d) = app.comment_props.as_mut() else { return (false, true) }; let kind = tl!(crate::comments_panel::subtype_label(&d.edited.subtype)).to_string(); diff --git a/crates/ui-egui/src/tools/comments.rs b/crates/ui-egui/src/tools/comments.rs index e9956e5f95..482dd85113 100644 --- a/crates/ui-egui/src/tools/comments.rs +++ b/crates/ui-egui/src/tools/comments.rs @@ -357,14 +357,23 @@ pub(crate) fn made_by(a: &Annotation, tool: CommentTool) -> bool { /// The tool whose defaults "Make Current Properties Default" should update. /// /// Arrows are stored as Line annotations. An arrow ending belongs to the Arrow tool, so its -/// defaults stay separate from a plain line. -pub(crate) fn tool_for_default(a: &Annotation, endings: Option<&[LineEnding]>) -> Option { +/// defaults stay separate from a plain line. A Replace Text group is selected by its caret (the +/// group's primary, ISO 32000-2 §12.5.6.2), so a caret whose group holds a Replace Text strikeout +/// (`/IT /StrikeOutTextEdit`) belongs to the Replace Text tool; `all` is the document's listing. +pub(crate) fn tool_for_default(all: &[Annotation], a: &Annotation, endings: Option<&[LineEnding]>) -> Option { match tool_for(a) { Some(CommentTool::Line) if endings.is_some_and(|ends| ends.iter().copied().any(arrow_ending)) => Some(CommentTool::Arrow), + Some(CommentTool::Caret) if replaces_text(all, a) => Some(CommentTool::ReplaceText), other => other, } } +/// Whether the caret `a` is the primary of a Replace Text group: a grouped strikeout with +/// `/IT /StrikeOutTextEdit` is one of its subordinates. +fn replaces_text(all: &[Annotation], a: &Annotation) -> bool { + all.iter().any(|s| s.is_group_member() && s.subtype == "StrikeOut" && s.intent.as_deref() == Some("StrikeOutTextEdit") && s.replies_to(a)) +} + fn arrow_ending(ending: LineEnding) -> bool { matches!(ending, LineEnding::OpenArrow | LineEnding::ClosedArrow | LineEnding::ROpenArrow | LineEnding::RClosedArrow) } diff --git a/crates/ui-egui/src/tools/comments/tests.rs b/crates/ui-egui/src/tools/comments/tests.rs index 91e796e43a..62cadb7298 100644 --- a/crates/ui-egui/src/tools/comments/tests.rs +++ b/crates/ui-egui/src/tools/comments/tests.rs @@ -482,3 +482,35 @@ fn text_boxes_fit_their_text() { assert_eq!(long[2], 300.0); assert!(long[3] - long[1] > 50.0); } + +/// Review on #4358: a Replace Text group is selected by its caret, the group's primary (ISO +/// 32000-2 §12.5.6.2), and Make Current Properties Default on it updates the Replace Text tool. A +/// caret of its own (Insert Text) still updates the Insert Text tool. +#[test] +fn a_replace_text_group_makes_replace_text_defaults() { + let annot = |index: usize, subtype: &str, intent: Option<&str>, parent: Option<(usize, usize)>| Annotation { + page: 0, + subtype: subtype.into(), + author: None, + contents: None, + modified: None, + name: None, + in_reply_to: None, + parent, + grouped: parent.is_some(), + rect: [10.0, 10.0, 20.0, 20.0], + color: None, + index, + state: None, + quads: Vec::new(), + locked: false, + intent: intent.map(Into::into), + fill_sign: false, + no_rotate: false, + points: Vec::new(), + }; + let all = + vec![annot(0, "Caret", Some("Replace"), None), annot(1, "StrikeOut", Some("StrikeOutTextEdit"), Some((0, 0))), annot(2, "Caret", None, None)]; + assert_eq!(tool_for_default(&all, &all[0], None), Some(CommentTool::ReplaceText)); + assert_eq!(tool_for_default(&all, &all[2], None), Some(CommentTool::Caret)); +} From f43a8af538a52efb86b9bbe532a08270e19a5e61 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 08:51:24 +0800 Subject: [PATCH 03/10] M5: a right-click on a group member selects its group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #4358: the secondary-click path still selected with cx.hit, which misses a group subordinate lying outside its primary's rectangle and cleared the selection, so the menu offered page actions instead of the comment's. It now uses the group-aware cx.click_hit, as the click does (ISO 32000-2 §12.5.6.2). Double-click editing goes through the same group-aware hit as the click since the rebase onto main, where the press owner comes from the module's hit test; the test covers both. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/ui-egui/src/tools/comments/input.rs | 3 ++- crates/ui-egui/tests/it/comments.rs | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index 6e948bcf21..ff1d42a309 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -31,7 +31,8 @@ pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, && let Some(p) = pointer.filter(|p| page_rect.contains(*p)) { cv.context_at = Some((cx.page, cx.to_user(p))); - cv.selected = cx.hit(p).map(|a| (cx.page, a.index)); + // On a group subordinate, its group (the primary), so the menu offers the comment's actions. + cv.selected = cx.click_hit(p).map(|a| (cx.page, a.index)); } let gestures = Gestures { ui, resp, cx, pointer, origin, page_rect, pressed_here, over_page }; match cx.tool { diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 3e078b6d49..dc64694db6 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -3062,4 +3062,20 @@ fn a_group_is_one_comment_in_the_panel_and_on_the_page() { let p = at(&h, 60.0, 50.0); click(&mut h, p); assert_eq!(h.state().views[0].comments.selected, Some((0, 0))); + // #4358 review: a right-click on the Circle selects the group too, so the menu is the + // comment's, and a double-click there edits the group's text, which is the Square's. + h.state_mut().views[0].comments.selected = None; + let p = at(&h, 220.0, 150.0); + h.hover_at(p); + h.run_steps(1); + h.event(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Secondary, pressed: true, modifiers: Default::default() }); + h.event(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Secondary, pressed: false, modifiers: Default::default() }); + h.run_steps(3); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "a right-click on the Circle selects its group"); + h.key_press(egui::Key::Escape); + h.run_steps(2); + h.state_mut().views[0].comments.selected = None; + crate::support::input::double_click(&mut h, p); + let composer = h.state().views[0].comments.composer.as_ref().map(|c| c.text.clone()); + assert_eq!(composer.as_deref(), Some("Primary"), "a double-click on the Circle edits its group's text"); } From 87a1778c826a35dd5c4ffc64b6c014a866f211ca Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 10:30:33 +0800 Subject: [PATCH 04/10] M5: parity evidence names the group test where main moved it The render inspect tests moved to crates/render/src/inspect/tests.rs on main; `cargo xtask parity` failed on the old path. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- parity/acrobat-features.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 86d86d0a22..e4f2151558 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -3595,7 +3595,7 @@ 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", "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"] +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/tests.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]] From 665b361d5cc00191c23aba76a655fa26058d348e Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 17:04:57 +0800 Subject: [PATCH 05/10] M5: a group is one page's, as Acrobat reads it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subordinate (/RT /Group) whose /IRT primary is on another page is now a comment of its own, listed with its own author and text, rather than part of that group. ISO 32000-2 §12.5.6.2 doesn't limit a group to a page, but Acrobat (desktop and iPad) and PDF-XChange all keep such a subordinate when the primary is deleted, and Acrobat desktop lists it on its own; tested on a synthetic two-page file. pdfcraft_render::link_replies and pdfcraft_annot::summaries both apply the rule. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/annot/src/info.rs | 8 ++++- crates/automation/tests/it/automation.rs | 40 ++++++++++++++++++++++++ crates/render/src/inspect/model.rs | 12 +++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index 0266cc0cd9..da3316a755 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -165,6 +165,12 @@ pub fn summaries(doc: &Document) -> Vec { let irt = d.get(b"IRT").map(|o| o.as_ref().map_or_else(|| doc.resolve(o), |r| doc.get(final_ref(doc, r)))); let in_reply_to = irt.and_then(|o| o.as_dict().cloned()).and_then(|p| text_value(doc, &p, b"NM")); let parent = d.get(b"IRT").and_then(Object::as_ref).and_then(|r| slots.get(&final_ref(doc, r)).copied()); + // A group is one page's, as Acrobat reads it: a subordinate whose primary is on + // another page is a comment of its own (keep in step with + // `pdfcraft_render::link_replies`). + let grouped = d.name(b"RT") == Some(b"Group"); + let cross_page = grouped && parent.is_some_and(|(p, _)| p != page); + let (parent, in_reply_to) = if cross_page { (None, None) } else { (parent, in_reply_to) }; out.push(Summary { page, index, @@ -177,7 +183,7 @@ pub fn summaries(doc: &Document) -> Vec { name: text_value(doc, d, b"NM"), in_reply_to, parent, - grouped: d.name(b"RT") == Some(b"Group"), + grouped, rect, color, state: text_value(doc, d, b"State"), diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 34c837056b..686f91d811 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -10361,3 +10361,43 @@ fn simple_rule_scan_drawn(content: &[u8]) -> Vec { } write_full(&doc, &SaveOptions::default()).unwrap() } + +// Contributor-original PDF: a Square on page 1 and, on page 2, a Circle with `/IRT` to it and +// `/RT /Group`. +const SPREAD_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 4 0 R] /Count 2 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R] >> endobj +4 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [6 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-other-page) /T (Bob) /Contents (Member on page 2) /C [0 0 1] /IRT 5 0 R /RT /Group >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +/// A group is one page's, as Acrobat reads it: a subordinate whose primary is on another page +/// lists as a comment of its own, with its own author and text, not as part of the group or a +/// reply, and deleting the primary leaves it. ISO 32000-2 §12.5.6.2 doesn't limit a group to a +/// page; Acrobat (desktop and iPad) and PDF-XChange all delete the primary without such a +/// subordinate, and Acrobat desktop lists it on its own. +#[test] +fn a_group_member_on_another_page_is_a_comment_of_its_own() { + let dir = workdir("rt-group-spread"); + std::fs::write(dir.join("group.pdf"), SPREAD_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"], 2, "{when}: the Square and the Circle, each a comment: {list}"); + let circle = list["comments"].as_array().unwrap().iter().find(|c| c["id"] == "circ-other-page").cloned().unwrap(); + assert_eq!((circle["author"].as_str(), circle["contents"].as_str()), (Some("Bob"), Some("Member on page 2")), "{when}"); + let square = list["comments"].as_array().unwrap().iter().find(|c| c["id"] == "sq-primary").cloned().unwrap(); + assert!(square["group"].as_array().unwrap().is_empty() && square["replies"].as_array().unwrap().is_empty(), "{when}: {square}"); + }; + 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 kept: Vec = after["comments"].as_array().unwrap().iter().filter(|c| c["contents"] != "Other").cloned().collect(); + check(&json!({ "count": kept.len(), "comments": kept }), "edited"); + ok(&mut a, "comment_delete", json!({ "doc": doc, "id": "sq-primary" })); + let left = ok(&mut a, "comment_list", json!({ "doc": doc })); + assert!(left.to_string().contains("circ-other-page"), "deleting the primary leaves the Circle on page 2: {left}"); +} diff --git a/crates/render/src/inspect/model.rs b/crates/render/src/inspect/model.rs index 8c9d75eb45..056c9a7009 100644 --- a/crates/render/src/inspect/model.rs +++ b/crates/render/src/inspect/model.rs @@ -517,6 +517,18 @@ pub fn link_replies(annotations: &mut [Annotation]) { a.parent = None; } } + // A group is one page's (Acrobat reads it so): a subordinate whose primary is on another page + // is a comment of its own, with its own author and text, rather than part of that group or a + // reply to it. ISO 32000-2 §12.5.6.2 doesn't limit a group to a page, but Acrobat (desktop and + // iPad) and PDF-XChange all delete the primary without such a subordinate, and Acrobat desktop + // lists it on its own. Keep in step with + // `pdfcraft_annot::summaries`. + for a in annotations.iter_mut() { + if a.grouped && a.parent.is_some_and(|(page, _)| page != a.page) { + a.parent = None; + a.in_reply_to = None; + } + } } /// The comment each reply's thread starts from, by the reply's page and `/Annots` position: a From bcbc122de58678b00c51054979dc5e0cd62d926b Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 20:29:12 +0800 Subject: [PATCH 06/10] M5: the group parity note says what deleting does across readers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting any member deletes its group on the page, as ISO 32000-2 §12.5.6.2 has it and PDF-XChange and Foxit do; a subordinate whose primary is on another page is a comment of its own and stays. The note's Missing list no longer says such a subordinate can't be selected, and the cross-page test is its evidence. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- parity/acrobat-features.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index e4f2151558..c4f8250727 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", "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/tests.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" +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::a_group_member_on_another_page_is_a_comment_of_its_own", "crates/automation/tests/it/automation.rs::looping_groups_end_and_chained_groups_reach_their_primary", "crates/render/src/inspect/tests.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 its group on the page (one undo step), as ISO 32000-2 §12.5.6.2 has it and PDF-XChange and Foxit do (Acrobat desktop deletes only the member clicked). A group is one page's: a subordinate whose primary is on another page is a comment of its own, listed, selected and deleted on its own, and it stays when the primary is deleted, as Acrobat (desktop and iPad) and PDF-XChange read it. Missing: moving or restyling a member does not move or restyle the group, and a subordinate has no hover tooltip" [[feature]] id = "comment.appearance-generation" From 8845066ab7e7f9c97fa228b2db4d511430728ecf Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 19:59:47 +0800 Subject: [PATCH 07/10] M5: Delete deletes the group member clicked, as Acrobat does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acrobat desktop deletes only the member of a group (/RT /Group) that was clicked: deleting the primary keeps its subordinates, on its page or another, and deleting a subordinate keeps the rest of its group. ISO 32000-2 §12.5.6.2 lists deletion among the operations that act on the entire group; this follows Acrobat. A subordinate left without its primary is a comment of its own. Replace Text stays one comment: its strikeout and caret go together from either, in both orientations (the caret as primary, and the strikeout as primary, as earlier PdfCraft builds wrote it), as Acrobat deletes the pair. On the page, a click on a subordinate still selects its group, and Delete (the key or the menu) acts on the member clicked. The group-primary lookup that deletion used is gone with it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/annot/src/info.rs | 23 ------- crates/annot/src/modify.rs | 78 ++++++++++++++-------- crates/annot/src/tests.rs | 65 +++++++++--------- crates/automation/src/tools.rs | 2 +- crates/automation/tests/it/automation.rs | 33 +++++++-- crates/redact/src/lib.rs | 2 +- crates/ui-egui/src/tools/comments/input.rs | 4 +- crates/ui-egui/src/tools/comments/menu.rs | 8 ++- crates/ui-egui/src/tools/comments/state.rs | 29 +++++++- crates/ui-egui/tests/it/comments.rs | 28 ++++++++ parity/acrobat-features.toml | 2 +- 11 files changed, 173 insertions(+), 101 deletions(-) diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index da3316a755..7f58ec299a 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -89,29 +89,6 @@ 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/modify.rs b/crates/annot/src/modify.rs index 006bb14273..ace7938cd4 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::{MAX_GROUP_DEPTH, group_primary, is_group_subordinate, line_endings_of, text_value}; +use super::info::{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,11 +70,11 @@ 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. 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. +/// Refused while it, or a reply that would go with it, is locked. A member of a group (`/RT +/// /Group`), its primary included, goes alone, as Acrobat deletes the member clicked and keeps the +/// rest of its group, although ISO 32000-2 §12.5.6.2 lists deletion among the operations that act +/// on the entire group; a subordinate left without its primary is a comment of its own. Replace +/// Text's strikeout and caret are one comment and go together from either ([`own_deletion`]). pub fn delete_annotation(doc: &mut Document, page: usize, index: usize) -> Result<(), AnnotError> { let index = deleted_index(doc, page, index)?; lift_annotation(doc, page, index) @@ -82,41 +82,42 @@ pub fn delete_annotation(doc: &mut Document, page: usize, index: usize) -> Resul /// 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 (or group member) that would go with it is. +/// annotation is locked, or a reply 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`. +/// [`deletable`], returning the index [`delete_annotation`] deletes from: the caret for Replace +/// Text's strikeout, else `index`. fn deleted_index(doc: &Document, page: usize, index: usize) -> Result { let list = annots(doc, page_ref(doc, page)?); 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); - } + } + // Replace Text is one comment: its strikeout goes with its caret, the group's primary, and + // the caret with it, as Acrobat deletes the pair from either. + let caret = doc + .resolve(target) + .as_dict() + .filter(|d| is_group_subordinate(d) && !own_deletion(doc, d)) + .and_then(|d| d.reference(b"IRT")) + .and_then(|irt| list.iter().enumerate().find(|(_, e)| e.as_ref() == Some(irt))); + if let Some((i, e)) = caret { + (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; 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)) { + // answers either (ISO 32000-2 §12.5.3, Table 167: Locked); nor does the other half of Replace + // Text. Pop-ups follow their comment. + for (i, e) in list.iter().enumerate().filter(|(i, e)| (*i != index || caret.is_some()) && goes_with(doc, 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 { - let what = if is_group_subordinate(d) { "a comment in its group is locked" } else { "a reply to it is locked" }; + let grouped = is_group_subordinate(d) || caret.is_some_and(|(c, _)| c == i); + let what = if grouped { "a comment in its group is locked" } else { "a reply to it is locked" }; return Err(AnnotError::Invalid(what.into())); } } @@ -131,7 +132,7 @@ pub fn lift_annotation(doc: &mut Document, page: usize, index: usize) -> Result< let list = annots(doc, p); let target = list.get(index).ok_or(AnnotError::NoSuchAnnotation { page, index })?; let doomed = with_replies_and_popups(doc, &list, seed(target)); - let kept: Vec = list.iter().enumerate().filter(|(i, e)| *i != index && !goes_with(e, &doomed)).map(|(_, e)| e.clone()).collect(); + let kept: Vec = list.iter().enumerate().filter(|(i, e)| *i != index && !goes_with(doc, e, &doomed)).map(|(_, e)| e.clone()).collect(); Ok(set_annots(doc, p, kept)?) } @@ -145,16 +146,35 @@ fn seed(target: &Object) -> Option { /// one of them, or it is written inline (a file can, though the standard asks for references) and /// replies to (`/IRT`) or pops up for (`/Parent`) one of them. An inline entry has no reference /// for the walk to collect, so it is matched here. -pub fn goes_with(entry: &Object, going: &HashSet) -> bool { +pub fn goes_with(doc: &Document, entry: &Object, going: &HashSet) -> bool { match entry.as_ref() { Some(r) => going.contains(&r), - None => entry.as_dict().is_some_and(|d| [d.reference(b"IRT"), d.reference(b"Parent")].into_iter().flatten().any(|r| going.contains(&r))), + None => entry.as_dict().is_some_and(|d| hangs_off(doc, d).into_iter().flatten().any(|r| going.contains(&r))), } } +/// What the annotation `d` goes with when it is deleted: what it replies to (`/IRT`) and what it +/// pops up for (`/Parent`). A group subordinate (`/RT /Group`) is deleted on its own +/// ([`own_deletion`]), so its `/IRT` doesn't count. +fn hangs_off(doc: &Document, d: &Dict) -> [Option; 2] { + [d.reference(b"IRT").filter(|_| !own_deletion(doc, d)), d.reference(b"Parent")] +} + +/// Whether the annotation `d` stays when its group's primary is deleted: a group subordinate +/// (`/RT /Group`) does, as Acrobat deletes only the member clicked, unless it is half of Replace +/// Text, which Acrobat deletes as one comment: the strikeout (`/IT /StrikeOutTextEdit`) under its +/// caret, or a caret under such a strikeout, as earlier PdfCraft builds wrote the pair. +fn own_deletion(doc: &Document, d: &Dict) -> bool { + let edit = |d: &Dict| d.name(b"IT") == Some(b"StrikeOutTextEdit".as_slice()); + let under_edit = + || d.name(b"Subtype") == Some(b"Caret".as_slice()) && d.reference(b"IRT").is_some_and(|r| doc.get(r).as_dict().is_some_and(edit)); + is_group_subordinate(d) && !edit(d) && !under_edit() +} + /// `doomed` and what goes with it, as Acrobat deletes a comment: the entries of `list` (one or /// more pages' `/Annots`) that reply to (`/IRT`) or pop up for (`/Parent`) anything going, their -/// own replies, and the pop-ups going annotations name (`/Popup`). An inline reply has no +/// own replies, and the pop-ups going annotations name (`/Popup`). A group subordinate stays +/// ([`own_deletion`]). An inline reply has no /// reference to collect, but the pop-up it names is. Each annotation is visited once, so a long or /// cyclic chain costs no more than its length. Filter a list with [`goes_with`], which also /// matches inline entries. @@ -166,7 +186,7 @@ pub fn with_replies_and_popups(doc: &Document, list: &[Object], doomed: impl Int // What goes with the annotation this entry hangs off: the entry, or for an inline one the // pop-up it names. let Some(goes) = e.as_ref().or_else(|| d.reference(b"Popup")) else { continue }; - for on in [d.reference(b"IRT"), d.reference(b"Parent")].into_iter().flatten() { + for on in hangs_off(doc, d).into_iter().flatten() { hanging.entry(on).or_default().push(goes); } } diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index ff0daba407..11e32c1791 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -913,7 +913,8 @@ fn replace_text_groups_a_strikeout_and_a_caret() { assert!(r[0] < 200.0 && r[2] > 200.0 && r[1] < 706.0, "at the end of the line, below its baseline: {r:?}"); 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. + // Replace Text is one comment: deleting either the caret or the strikeout deletes both, as + // Acrobat does. 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(); @@ -942,9 +943,11 @@ 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. +/// Deleting a member deletes just it, as Acrobat does, although §12.5.6.2 lists deletion among the +/// operations that act on the entire group: the subordinate goes alone, and the primary goes with +/// its replies and leaves its subordinate, which reads as a comment of its own. #[test] -fn a_group_reads_as_one_and_deletes_as_one() { +fn a_group_reads_as_one_and_deletes_by_member() { 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(); @@ -972,21 +975,23 @@ fn a_group_reads_as_one_and_deletes_as_one() { [("member".into(), true), ("reply".into(), false), ("explicit".into(), false), ("odd".into(), false)], "only /RT /Group groups" ); + let mut alone = doc.clone(); + delete_annotation(&mut alone, 0, member).unwrap(); + assert_eq!(list(&alone, 0).len(), list(&doc, 0).len() - 1, "the subordinate goes alone"); 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()); + delete_annotation(&mut doc, 0, square).unwrap(); + let left: Vec> = list(&doc, 0).iter().map(|d| d.get(b"NM").map(|_| text(d, b"NM"))).collect(); + assert_eq!(left, [None, Some("member".into())], "the primary and its replies are gone; the fixture's Square and the subordinate stay"); + let summary = summaries(&doc); + let member = summary.iter().find(|s| s.name.as_deref() == Some("member")).unwrap(); + assert!(member.parent.is_none() && !member.is_group_member(), "left without its primary, it is a comment of its own: {member:?}"); } -/// 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). +/// A subordinate deep in a chain of subordinates, within the 64 annotations the listing follows +/// up a chain or past them, is deleted alone: the chain above it stays. #[test] -fn a_group_chain_past_the_depth_limit_is_not_deleted_in_part() { - for (subordinates, deletes) in [(63, true), (64, false), (65, false)] { +fn a_subordinate_deep_in_a_chain_is_deleted_alone() { + for subordinates in [63, 64, 65] { 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(); @@ -1002,23 +1007,15 @@ fn a_group_chain_past_the_depth_limit_is_not_deleted_in_part() { 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"); - } + delete_annotation(&mut doc, 0, last).unwrap(); + assert_eq!(list(&doc, 0).len(), before - 1, "{subordinates}: the subordinate is gone, the rest of its group stays"); } } -/// 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). +/// A locked member doesn't hold back the deletion of another, the primary included: each goes +/// alone (ISO 32000-2 §12.5.3, Table 167: Locked). #[test] -fn a_locked_group_member_keeps_its_group() { +fn a_locked_group_member_holds_back_no_other_deletion() { 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(); @@ -1035,13 +1032,13 @@ fn a_locked_group_member_keeps_its_group() { } 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"); + let mut alone = doc.clone(); + delete_annotation(&mut alone, 0, members[0]).unwrap(); + assert_eq!(list(&alone, 0).len(), before - 1, "the unlocked subordinate goes alone"); + delete_annotation(&mut doc, 0, top).unwrap(); + assert_eq!(list(&doc, 0).len(), before - 1, "the primary goes alone too"); + let locked = list(&doc, 0).iter().filter(|d| d.get(b"F").and_then(|f| f.as_int()).is_some_and(|f| f & 128 != 0)).count(); + assert_eq!(locked, 1, "the locked member stays"); } #[test] diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 1329e548d2..a9ce3b3033 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). Annotations grouped with a comment (/RT /Group) are listed once, as that comment with its attributes, and named in its `group`.") + 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`. comment_delete deletes just the member named, as Acrobat does (Replace Text's strikeout and caret go together).") .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 686f91d811..11043e7b33 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -2382,11 +2382,17 @@ fn a_group_lists_as_its_primary() { 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. + // Deleting a member deletes just it, as Acrobat does: the subordinate, or the primary, which + // leaves the subordinate a comment of its own. One undo step each. 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}"); + assert_eq!(left["count"], 2, "the primary and the new note are left: {left}"); + assert!(left.to_string().contains("sq-primary") && !left.to_string().contains("circ-sub"), "{left}"); + ok(&mut a, "edit_undo", json!({ "doc": doc })); + ok(&mut a, "comment_delete", json!({ "doc": doc, "id": "sq-primary" })); + let left = ok(&mut a, "comment_list", json!({ "doc": doc })); + let ids: Vec<&str> = left["comments"].as_array().unwrap().iter().filter_map(|c| c["id"].as_str()).collect(); + assert!(ids.contains(&"circ-sub") && !ids.contains(&"sq-primary") && ids.len() == 2, "the new note and the Circle, on its own: {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(); @@ -2426,12 +2432,18 @@ fn looping_groups_end_and_chained_groups_reach_their_primary() { 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. + // Deleting a looping member ends too and takes just it, as any group member goes alone; so + // does the lowest subordinate of the chain. 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}"); + let list = ok(&mut a, "comment_list", json!({ "doc": doc })); + let mut ids: Vec<&str> = list["comments"].as_array().unwrap().iter().filter_map(|c| c["id"].as_str()).collect(); + ids.sort_unstable(); + assert_eq!(ids, ["b", "top"], "{list}"); + let top = list["comments"].as_array().unwrap().iter().find(|c| c["id"] == "top").cloned().unwrap_or_default(); + let group: Vec<&str> = top["group"].as_array().into_iter().flatten().filter_map(|m| m["id"].as_str()).collect(); + assert_eq!(group, ["mid"], "{list}"); } // Contributor-original PDF: a Replace Text group as earlier PdfCraft builds wrote it, the @@ -2495,6 +2507,15 @@ fn replace_text_lists_its_replacement() { 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}"); + // Replace Text is one comment: deleting either half deletes both, as Acrobat does, in + // either orientation. + for id in [c["id"].clone(), c["group"][0]["id"].clone()] { + ok(&mut b, "comment_delete", json!({ "doc": doc, "id": id })); + let left = ok(&mut b, "comment_list", json!({ "doc": doc })); + assert_eq!(left["count"], 1, "{path}: deleting {id} leaves only the note: {left}"); + assert!(!left.to_string().contains("StrikeOut") && !left.to_string().contains("Caret"), "{path}: {left}"); + ok(&mut b, "edit_undo", json!({ "doc": doc })); + } } } diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index de9dafe7db..f71400daef 100644 --- a/crates/redact/src/lib.rs +++ b/crates/redact/src/lib.rs @@ -752,7 +752,7 @@ pub fn clear_marks(doc: &mut Document, pages: Option<&[usize]>) -> Result = annots.iter().filter(|a| !pdfcraft_annot::goes_with(a, &doomed)).cloned().collect(); + let kept: Vec = annots.iter().filter(|a| !pdfcraft_annot::goes_with(doc, a, &doomed)).cloned().collect(); // As deleting a comment does: a page whose marks were all it had loses `/Annots` again, // and an indirect array stays the one object the page points to. if kept.len() != annots.len() { diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index ff1d42a309..7bb03b5eb8 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -33,6 +33,7 @@ pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, cv.context_at = Some((cx.page, cx.to_user(p))); // On a group subordinate, its group (the primary), so the menu offers the comment's actions. cv.selected = cx.click_hit(p).map(|a| (cx.page, a.index)); + cv.member = cx.picked(p, cv.selected); } let gestures = Gestures { ui, resp, cx, pointer, origin, page_rect, pressed_here, over_page }; match cx.tool { @@ -264,6 +265,7 @@ fn select_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, view: &mu match owner { Some((a, _)) => { cv.selected = Some((cx.page, a.index)); + cv.member = pointer.and_then(|p| cx.picked(p, cv.selected)); cv.reveal = true; consumed = true; } @@ -599,7 +601,7 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool } } if allowed - && let Some((page, index)) = cv.selected + && let Some((page, index)) = cv.target() && ctx.input(|i| i.key_pressed(egui::Key::Delete) || i.key_pressed(egui::Key::Backspace)) { cv.selected = None; diff --git a/crates/ui-egui/src/tools/comments/menu.rs b/crates/ui-egui/src/tools/comments/menu.rs index 52ea8d707d..77f0a33870 100644 --- a/crates/ui-egui/src/tools/comments/menu.rs +++ b/crates/ui-egui/src/tools/comments/menu.rs @@ -92,10 +92,12 @@ pub(crate) fn context_menu( ui.close(); } ui.separator(); - // The engine's own check: a locked comment, or one with a locked reply, stays. - if ui.add_enabled(allowed && deletable(page, index), egui::Button::new(tl!("Delete"))).clicked() { + // The engine's own check: a locked comment, or one with a locked reply, stays. On a group + // subordinate, Delete takes just it, as Acrobat's does. + let (dpage, dindex) = view.comments.target().unwrap_or((page, index)); + if ui.add_enabled(allowed && deletable(dpage, dindex), egui::Button::new(tl!("Delete"))).clicked() { view.comments.selected = None; - action = Some(CanvasAction::Edit(Box::new(Edit::DeleteAnnotation { page, index }))); + action = Some(CanvasAction::Edit(Box::new(Edit::DeleteAnnotation { page: dpage, index: dindex }))); ui.close(); } if ui.add_enabled(allowed, egui::Button::new(tl!("Properties…"))).clicked() { diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index 304c687b54..5c15154291 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -126,6 +126,10 @@ impl CommentView { pub struct CommentView { /// The selected comment: (page, index in `/Annots`). pub selected: Option<(usize, usize)>, + /// The group subordinate a click on the page picked, with the selection (its group's primary) + /// it was picked under: Delete acts on it while that group stays selected, as Acrobat deletes + /// the member clicked ([`Self::target`]). + pub(crate) member: Option<((usize, usize), (usize, usize))>, pub gesture: Option, /// Drawings just released: shown from their points until the page raster that includes the /// committed annotation arrives, so the stroke never vanishes in between (#1858). @@ -186,6 +190,15 @@ pub(crate) enum EditorFocus { } impl CommentView { + /// What Delete acts on: the group member clicked, while its group is still the selection, + /// else the selected comment. + pub(crate) fn target(&self) -> Option<(usize, usize)> { + match self.member { + Some((member, group)) if Some(group) == self.selected => Some(member), + _ => self.selected, + } + } + /// Takes back the last point of a polygon, cloud or connected lines being drawn; the first /// point's removal cancels the drawing. Returns whether there was such a drawing. pub fn undo_point(&mut self) -> bool { @@ -410,8 +423,20 @@ impl PageCx<'_> { /// 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)) + self.hit(p).or_else(|| self.member_at(p).map(|(_, p)| p)) + } + + /// The group subordinate under `p`, where no comment is, with its group's primary. + fn member_at(&self, p: Pos2) -> Option<(&Annotation, &Annotation)> { + self.group_members().into_iter().rev().find(|(m, _)| self.screen_rects(m).any(|r| r.expand(3.0).contains(p))) + } + + /// The group member a click at `p` picked when it selects `selected`, its group's primary + /// ([`CommentView::member`]): the subordinate under the pointer. + pub(super) fn picked(&self, p: Pos2, selected: Option<(usize, usize)>) -> Option<((usize, usize), (usize, usize))> { + let (m, primary) = self.hit(p).is_none().then(|| self.member_at(p)).flatten()?; + let group = (primary.page, primary.index); + (Some(group) == selected).then_some(((self.page, m.index), group)) } pub(crate) fn rect_to_user(&self, r: Rect) -> [f64; 4] { diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index dc64694db6..88f357fb5e 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -3079,3 +3079,31 @@ fn a_group_is_one_comment_in_the_panel_and_on_the_page() { let composer = h.state().views[0].comments.composer.as_ref().map(|c| c.text.clone()); assert_eq!(composer.as_deref(), Some("Primary"), "a double-click on the Circle edits its group's text"); } + +/// As in Acrobat desktop, Delete deletes the group member clicked and keeps the rest of its +/// group, although ISO 32000-2 §12.5.6.2 lists deletion among the operations that act on the +/// entire group. The Circle goes with its reply and the Square stays; the Square goes with its +/// reply and the Circle stays, a comment of its own. +#[test] +fn delete_deletes_the_group_member_clicked() { + let shapes = |h: &Harness<'static, PdfCraftApp>| -> Vec<(String, bool)> { + let mut out: Vec<(String, bool)> = comments(h).iter().filter(|a| a.subtype != "Text").map(|a| (a.subtype.clone(), a.is_comment())).collect(); + out.sort(); + out + }; + let mut h = harness_with_bytes(GROUP_FIXTURE.to_vec(), |_| {}); + click_pt(&mut h, (220.0, 150.0)); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the group is selected"); + h.key_press(egui::Key::Delete); + h.run_steps(3); + assert_eq!(shapes(&h), [("Square".to_string(), true)], "the Circle goes, the Square stays"); + let texts: Vec = comments(&h).iter().filter_map(|a| a.contents.clone()).collect(); + assert!(!texts.iter().any(|c| c == "Reply to the subordinate") && texts.iter().any(|c| c == "Plain reply"), "{texts:?}"); + h.state_mut().undo(); + h.run_steps(3); + + click_pt(&mut h, (60.0, 50.0)); + h.key_press(egui::Key::Delete); + h.run_steps(3); + assert_eq!(shapes(&h), [("Circle".to_string(), true)], "the Square goes, the Circle stays as a comment of its own"); +} diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index c4f8250727..3c0d3e6649 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -3596,7 +3596,7 @@ 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", "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::a_group_member_on_another_page_is_a_comment_of_its_own", "crates/automation/tests/it/automation.rs::looping_groups_end_and_chained_groups_reach_their_primary", "crates/render/src/inspect/tests.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 its group on the page (one undo step), as ISO 32000-2 §12.5.6.2 has it and PDF-XChange and Foxit do (Acrobat desktop deletes only the member clicked). A group is one page's: a subordinate whose primary is on another page is a comment of its own, listed, selected and deleted on its own, and it stays when the primary is deleted, as Acrobat (desktop and iPad) and PDF-XChange read it. Missing: moving or restyling a member does not move or restyle the group, and a subordinate has no hover tooltip" +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 Delete deletes the member clicked alone, the primary included, as Acrobat desktop does (one undo step; a subordinate left without its primary is a comment of its own; Replace Text's strikeout and caret delete together). ISO 32000-2 §12.5.6.2 has deletion act on the whole group, as PDF-XChange and Foxit do on one page; Acrobat's is followed as the more practical. A group is one page's: a subordinate whose primary is on another page is a comment of its own, listed, selected, moved and deleted on its own, as Acrobat (desktop and iPad), PDF-XChange and Foxit read it. Missing: restyling a member does not restyle its group as Acrobat does, a subordinate can't be dragged, resized or reshaped on its own, and it has no hover tooltip" [[feature]] id = "comment.appearance-generation" From 3602d9f2d9ff68cd54d6e0171ee3d4ba527054c9 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sun, 1 Nov 2026 22:09:33 +0800 Subject: [PATCH 08/10] M5: a group member outlives its deleted primary without /IRT; /IRT walks tested past their bounds Deleting a group's primary (delete_annotation, lift_annotation) or clearing a redaction mark with a group member (redact::clear_marks) now removes /IRT from each subordinate that stays and keeps its /RT /Group, as Acrobat desktop does. The kept member no longer refers to the deleted annotation, so a full save leaves the deleted one's content out. It is part of the same edit: one undo restores the group, /IRT included. lift_annotation's copy-only uses (the page beneath a dragged stamp or signature) only render the copy, so the change there is harmless. Each /IRT walk this PR relies on gets a test past its bound and a loop: group_primaries on an unsettled loop (render), comment_summary under a 100-deep chain of subordinates and a self-listed loop (engine), and deletion over a 200-deep thread and reply and group loops (annot). Review on #4358. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/annot/src/lib.rs | 4 +- crates/annot/src/modify.rs | 37 +++++++++- crates/annot/src/tests.rs | 112 +++++++++++++++++++++++++++++ crates/engine/src/tests.rs | 64 +++++++++++++++++ crates/redact/src/lib.rs | 8 ++- crates/redact/src/tests.rs | 28 ++++++++ crates/render/src/inspect/tests.rs | 9 +++ parity/acrobat-features.toml | 4 +- 8 files changed, 258 insertions(+), 8 deletions(-) diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index 43b9d098e1..51810738c3 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -38,8 +38,8 @@ pub use lookup::{ }; pub(crate) use modify::n; pub use modify::{ - add_reply, add_text_replacement, deletable, delete_annotation, erase_ink, fillable, goes_with, lift_annotation, move_annotation, set_contents, - set_fill, set_font_size, set_locked, set_marked, set_points, set_rect, set_review_state, set_style, with_replies_and_popups, + add_reply, add_text_replacement, deletable, delete_annotation, erase_ink, fillable, goes_with, lift_annotation, move_annotation, release_members, + set_contents, set_fill, set_font_size, set_locked, set_marked, set_points, set_rect, set_review_state, set_style, with_replies_and_popups, }; pub use types::{ AttachIcon, FillMark, LineEnding, Markup, Meta, NOTE_SIZE, NewAnnotation, NoteIcon, OverlayFont, OverlayLook, ReviewState, Rgb, Shape, diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index ace7938cd4..d6b365f2c2 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -73,8 +73,9 @@ pub(crate) fn touch(d: &mut Dict, meta: &Meta) { /// Refused while it, or a reply that would go with it, is locked. A member of a group (`/RT /// /Group`), its primary included, goes alone, as Acrobat deletes the member clicked and keeps the /// rest of its group, although ISO 32000-2 §12.5.6.2 lists deletion among the operations that act -/// on the entire group; a subordinate left without its primary is a comment of its own. Replace -/// Text's strikeout and caret are one comment and go together from either ([`own_deletion`]). +/// on the entire group; a subordinate left without its primary is a comment of its own and +/// loses its `/IRT` ([`release_members`]). Replace Text's strikeout and caret are one comment and +/// go together from either ([`own_deletion`]). pub fn delete_annotation(doc: &mut Document, page: usize, index: usize) -> Result<(), AnnotError> { let index = deleted_index(doc, page, index)?; lift_annotation(doc, page, index) @@ -133,9 +134,41 @@ pub fn lift_annotation(doc: &mut Document, page: usize, index: usize) -> Result< let target = list.get(index).ok_or(AnnotError::NoSuchAnnotation { page, index })?; let doomed = with_replies_and_popups(doc, &list, seed(target)); let kept: Vec = list.iter().enumerate().filter(|(i, e)| *i != index && !goes_with(doc, e, &doomed)).map(|(_, e)| e.clone()).collect(); + let kept = release_members(doc, kept, &doomed)?; Ok(set_annots(doc, p, kept)?) } +/// The `/Annots` entries `kept` after deleting `going`, with every group subordinate that stays +/// ([`own_deletion`]) but whose `/IRT` names an annotation going made a comment of its own: its +/// `/IRT` is removed and its `/RT /Group` kept, as Acrobat does when a group's primary is deleted. +/// A deleted primary then stays reachable from nothing, so a full save leaves its text out. An +/// inline entry is rewritten in the list; a referenced one is updated in place. Each entry is +/// read once, following no chain. +pub fn release_members(doc: &mut Document, kept: Vec, going: &HashSet) -> Result, pdfcraft_cos::CosError> { + let orphaned = |d: &Dict| is_group_subordinate(d) && d.reference(b"IRT").is_some_and(|r| going.contains(&r)); + let mut out = Vec::with_capacity(kept.len()); + for e in kept { + match e.as_ref() { + Some(r) => { + if doc.get(r).as_dict().is_some_and(orphaned) { + doc.update_dict(r, |d| { + d.remove(b"IRT"); + })?; + } + out.push(e); + } + None => match e { + Object::Dict(mut d) if orphaned(&d) => { + d.remove(b"IRT"); + out.push(Object::Dict(d)); + } + other => out.push(other), + }, + } + } + Ok(out) +} + /// What deleting the `/Annots` entry `target` starts from: its reference, or for an inline /// comment (which has none to collect) the pop-up it names, which still goes. fn seed(target: &Object) -> Option { diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index 11e32c1791..4f825b1a76 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -987,6 +987,118 @@ fn a_group_reads_as_one_and_deletes_by_member() { assert!(member.parent.is_none() && !member.is_group_member(), "left without its primary, it is a comment of its own: {member:?}"); } +/// A primary with one subordinate and one reply, all on page 0 of a reopened file. Returns the +/// primary's and the subordinate's `/Annots` positions and references. +fn group_of_two(text: &str) -> (Document, (usize, ObjRef), (usize, ObjRef)) { + let mut doc = fixture(); + let primary = add_annotation(&mut doc, &new(0, Shape::Rectangle { rect: [100.0, 100.0, 200.0, 200.0] }), &meta("primary")).unwrap(); + let (_, pr) = annot_ref(&mut doc, 0, primary).unwrap(); + doc.update_dict(pr, |d| d.set(b"Contents".to_vec(), pdfcraft_cos::PdfString::text(text))).unwrap(); + let member = add_annotation(&mut doc, &new(0, Shape::Oval { rect: [300.0, 300.0, 350.0, 350.0] }), &meta("member")).unwrap(); + let (_, mr) = annot_ref(&mut doc, 0, member).unwrap(); + doc.update_dict(mr, |d| { + d.set(b"IRT".to_vec(), Object::Ref(pr)); + d.set(b"RT".to_vec(), Object::name("Group")); + }) + .unwrap(); + add_reply(&mut doc, 0, primary, "a reply", "Ada", &meta("reply")).unwrap(); + (reopen(&doc), (primary, pr), (member, mr)) +} + +/// Deleting a group's primary leaves its subordinate a comment of its own, as Acrobat does: the +/// subordinate loses its `/IRT` and keeps `/RT /Group`, so nothing refers to the deleted primary +/// and a full save leaves its text out. +#[test] +fn deleting_a_primary_releases_its_subordinate() { + let (mut doc, (primary, _), (_, mr)) = group_of_two("primary secret text"); + delete_annotation(&mut doc, 0, primary).unwrap(); + let member = doc.get(mr).as_dict().cloned().unwrap(); + assert!(member.get(b"IRT").is_none(), "the subordinate no longer names the deleted primary: {member:?}"); + assert_eq!(member.name(b"RT"), Some(&b"Group"[..]), "its /RT stays, as Acrobat leaves it"); + let s = summaries(&doc); + let listed = s.iter().find(|s| s.name.as_deref() == Some("member")).unwrap(); + assert!(listed.parent.is_none() && listed.in_reply_to.is_none() && !listed.is_group_member(), "a comment of its own: {listed:?}"); + assert!(!s.iter().any(|s| s.name.as_deref() == Some("reply")), "the primary's reply went with it"); + let full = pdfcraft_cos::write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); + assert!(!full.windows(19).any(|w| w == b"primary secret text"), "the deleted primary's text is not in a full save"); +} + +/// Deleting a subordinate leaves its primary as it was. +#[test] +fn deleting_a_subordinate_leaves_its_primary_untouched() { + let (mut doc, (_, pr), (member, _)) = group_of_two("primary text"); + let before = doc.get(pr).as_dict().cloned(); + delete_annotation(&mut doc, 0, member).unwrap(); + assert_eq!(doc.get(pr).as_dict().cloned(), before); + assert!(summaries(&doc).iter().any(|s| s.name.as_deref() == Some("reply")), "the primary keeps its reply"); +} + +/// An inline subordinate (`/Annots` may hold a dictionary, though the standard asks for a +/// reference) whose primary is deleted loses its `/IRT` in the page's list. +#[test] +fn an_inline_subordinate_is_released_too() { + let mut doc = fixture(); + let primary = add_annotation(&mut doc, &new(0, Shape::Rectangle { rect: [100.0, 100.0, 200.0, 200.0] }), &meta("primary")).unwrap(); + let (_, pr) = annot_ref(&mut doc, 0, primary).unwrap(); + let p = page_refs(&doc).unwrap()[0]; + let mut entries = annots(&doc, p); + let mut inline = Dict::new(); + inline.set(b"Type".to_vec(), Object::name("Annot")); + inline.set(b"Subtype".to_vec(), Object::name("Square")); + inline.set(b"Rect".to_vec(), Object::Array([300, 300, 350, 350].map(Object::Int).to_vec())); + inline.set(b"IRT".to_vec(), Object::Ref(pr)); + inline.set(b"RT".to_vec(), Object::name("Group")); + entries.push(Object::Dict(inline)); + set_annots(&mut doc, p, entries).unwrap(); + delete_annotation(&mut doc, 0, primary).unwrap(); + let left = list(&doc, 0); + let inline = left.iter().find(|d| d.name(b"RT") == Some(b"Group")).expect("the inline subordinate stays"); + assert!(inline.get(b"IRT").is_none()); +} + +/// Deleting follows `/IRT` replies by visiting each annotation once, with no step limit to cut a +/// thread short: a thread of 200 replies (past the 64 steps the listings follow) goes whole, and +/// replies or group members in a loop (`/IRT` back to each other) end the walk. A loop of group +/// members deletes only the one picked; the other is released. +#[test] +fn deleting_follows_a_long_or_looping_irt_chain_once_each() { + 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 on = top; + for k in 0..200 { + on = add_reply(&mut doc, 0, on, "reply", "Ada", &meta(&format!("r{k}"))).unwrap(); + } + let before = list(&doc, 0).len(); + delete_annotation(&mut doc, 0, top).unwrap(); + assert_eq!(list(&doc, 0).len(), before - 201, "the comment and its 200-deep thread go together"); + for rt in [None, Some("Group")] { + let mut doc = fixture(); + let a = add_annotation(&mut doc, &new(0, Shape::Rectangle { rect: [100.0, 100.0, 200.0, 200.0] }), &meta("a")).unwrap(); + let b = add_annotation(&mut doc, &new(0, Shape::Oval { rect: [300.0, 300.0, 350.0, 350.0] }), &meta("b")).unwrap(); + let (_, ar) = annot_ref(&mut doc, 0, a).unwrap(); + let (_, br) = annot_ref(&mut doc, 0, b).unwrap(); + for (r, to) in [(ar, br), (br, ar)] { + doc.update_dict(r, |d| { + d.set(b"IRT".to_vec(), Object::Ref(to)); + if let Some(rt) = rt { + d.set(b"RT".to_vec(), Object::name(rt)); + } + }) + .unwrap(); + } + let before = list(&doc, 0).len(); + delete_annotation(&mut doc, 0, a).unwrap(); + let left = list(&doc, 0); + match rt { + None => assert_eq!(left.len(), before - 2, "a reply loop goes whole"), + Some(_) => { + assert_eq!(left.len(), before - 1, "a group member goes alone"); + assert!(doc.get(br).as_dict().is_some_and(|d| d.get(b"IRT").is_none()), "the other member is released"); + } + } + } +} + /// A subordinate deep in a chain of subordinates, within the 64 annotations the listing follows /// up a chain or past them, is deleted alone: the chain above it stays. #[test] diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index a6ced87376..4dad75adf5 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4428,6 +4428,70 @@ fn comment_summary_leaves_group_members_out_and_keeps_their_replies() { assert!(text.lines().any(|l| l == " REPLY_TO_MEMBER"), "the reply is the comment's, one level in: {text}"); } +/// The summary follows `/IRT` down from a comment at most 64 steps, group subordinates included: +/// a reply under a longer chain of subordinates is left out, one under a short chain is written, +/// and a subordinate listed at its own primary's position (a loop, from a malformed listing) +/// ends the walk. +#[test] +fn comment_summary_follows_a_group_chain_a_bounded_number_of_steps() { + let (mut s, id) = session_with(1); + s.apply(id, rect_comment(0, [40.0, 40.0, 90.0, 90.0])).unwrap(); + let top = s.get(id).unwrap().info.annotations.iter().find(|a| a.is_comment()).unwrap().clone(); + let at = |a: &pdfcraft_render::Annotation| (a.page, a.index); + let under = |index: usize, parent: (usize, usize), grouped: bool, text: &str| pdfcraft_render::Annotation { + index, + parent: Some(parent), + grouped, + contents: Some(text.into()), + ..top.clone() + }; + for (members, written) in [(10, true), (100, false)] { + let mut all = vec![top.clone()]; + let mut up = at(&top); + for k in 0..members { + let m = under(1000 + k, up, true, "MEMBER"); + up = at(&m); + all.push(m); + } + all.push(under(5000, up, false, "DEEP_REPLY")); + let text = comment_summary("doc.pdf", &all, SummarySort::Page); + assert_eq!(text.contains("DEEP_REPLY"), written, "{members}: {text}"); + assert!(!text.contains("MEMBER"), "{members}: {text}"); + } + let looped = vec![top.clone(), under(top.index, at(&top), true, "LOOP")]; + let text = comment_summary("doc.pdf", &looped, SummarySort::Page); + assert!(!text.contains("LOOP"), "{text}"); +} + +/// Deleting a group's primary removes the subordinate's `/IRT` in the same edit, as Acrobat does: +/// the subordinate lists as a comment of its own and keeps `/RT /Group`. One undo puts the group +/// back, `/IRT` included. +#[test] +fn deleting_a_group_primary_releases_its_member_in_one_undo_step() { + let bytes = pdf_with_annots(&[ + b"<< /Type /Annot /Subtype /Square /Rect [10 10 50 50] /NM (primary) /Contents (PRIMARY_TEXT) >>", + b"<< /Type /Annot /Subtype /Circle /Rect [60 60 90 90] /NM (member) /IRT 4 0 R /RT /Group >>", + ]); + let mut s = Session::new().with_clock(|| 1_700_000_000); + let id = s.open("group.pdf", None, Arc::new(bytes), None).unwrap(); + let names = |s: &Session| -> Vec<(Option, bool, bool)> { + s.get(id).unwrap().info.annotations.iter().map(|a| (a.name.clone(), a.is_comment(), a.is_group_member())).collect() + }; + // Listed by top edge: the Circle is the higher one. + let grouped = vec![(Some("member".into()), false, true), (Some("primary".into()), true, false)]; + assert_eq!(names(&s), grouped); + s.apply(id, Edit::DeleteAnnotation { page: 0, index: 0 }).unwrap(); + assert_eq!(names(&s), [(Some("member".into()), true, false)], "a comment of its own"); + let member = |s: &Session| { + let cos = &s.get(id).unwrap().editor.as_ref().unwrap().cos; + cos.get(pdfcraft_cos::ObjRef::new(5, 0)).as_dict().cloned().unwrap() + }; + assert!(member(&s).get(b"IRT").is_none() && member(&s).name(b"RT") == Some(b"Group".as_slice()), "{:?}", member(&s)); + s.undo(id).unwrap(); + assert_eq!(names(&s), grouped, "one undo puts the group back"); + assert!(member(&s).reference(b"IRT").is_some()); +} + #[test] fn reading_colors_set_and_invalidated() { let (mut s, id) = session_with(1); diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index f71400daef..634d4bd7ca 100644 --- a/crates/redact/src/lib.rs +++ b/crates/redact/src/lib.rs @@ -753,9 +753,13 @@ pub fn clear_marks(doc: &mut Document, pages: Option<&[usize]>) -> Result = annots.iter().filter(|a| !pdfcraft_annot::goes_with(doc, a, &doomed)).cloned().collect(); + // A group subordinate of a mark stays, as when a comment's group primary is deleted, and + // loses its `/IRT`: through it the mark would stay in the file. + let kept = pdfcraft_annot::release_members(doc, kept, &doomed)?; // As deleting a comment does: a page whose marks were all it had loses `/Annots` again, - // and an indirect array stays the one object the page points to. - if kept.len() != annots.len() { + // and an indirect array stays the one object the page points to. A page whose inline + // member lost its `/IRT` is written too. + if kept != annots { pdfcraft_annot::set_annots(doc, p.obj, kept)?; } } diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index 125f00fc5c..2e703769c2 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -1393,6 +1393,34 @@ fn clearing_marks_leaves_the_annotation_list_as_it_was() { assert_eq!((doc.get(page), doc.get(list)), before, "the page points at its array, which lists the link alone"); } +/// A group subordinate (`/RT /Group`) of a mark stays when the marks are cleared, as one stays +/// when its primary comment is deleted, and loses its `/IRT`: nothing then refers to the mark, +/// so a full save leaves the mark's text out. +#[test] +fn clearing_marks_releases_a_group_member_of_a_mark() { + let mut doc = one_page(b"BT /F1 10 Tf 10 100 Td (AB) Tj ET", "", vec![]); + mark(&mut doc, 0, &[[0.0, 0.0, 50.0, 50.0]], ""); + let m = marks(&doc)[0].obj; + doc.update_dict(m, |d| d.set(b"Contents".to_vec(), pdfcraft_cos::PdfString::text("MARK_SECRET_TEXT"))).unwrap(); + let mut member = Dict::new(); + member.set(b"Type".to_vec(), Object::name("Annot")); + member.set(b"Subtype".to_vec(), Object::name("Square")); + member.set(b"Rect".to_vec(), Object::Array([60, 60, 90, 90].map(Object::Int).to_vec())); + member.set(b"IRT".to_vec(), Object::Ref(m)); + member.set(b"RT".to_vec(), Object::name("Group")); + let member = doc.add(Object::Dict(member)); + let page = pdfcraft_model::pages(&doc)[0].obj; + let mut listed = annots_of(&doc, doc.get(page).as_dict().unwrap()); + listed.push(Object::Ref(member)); + doc.update_dict(page, |d| d.set(b"Annots".to_vec(), Object::Array(listed))).unwrap(); + assert_eq!(clear_marks(&mut doc, None), Ok(1)); + assert_eq!(annots_of(&doc, doc.get(page).as_dict().unwrap()), [Object::Ref(member)], "the member stays"); + let d = doc.get(member).as_dict().cloned().unwrap(); + assert!(d.get(b"IRT").is_none() && d.name(b"RT") == Some(b"Group".as_slice()), "{d:?}"); + let full = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); + assert!(!full.windows(16).any(|w| w == b"MARK_SECRET_TEXT"), "the cleared mark is not in a full save"); +} + #[test] fn unreadable_content_fails_closed() { let mut doc = one_page(b"", "", vec![]); diff --git a/crates/render/src/inspect/tests.rs b/crates/render/src/inspect/tests.rs index afde323a7c..21288a5edb 100644 --- a/crates/render/src/inspect/tests.rs +++ b/crates/render/src/inspect/tests.rs @@ -1935,6 +1935,15 @@ mod group_tests { } } + /// `group_primaries` is public and may be handed links `link_replies` has not settled: a loop + /// of subordinates (each other's `/IRT`, or its own) still ends, within `MAX_GROUP_DEPTH` + /// steps, with no primary. + #[test] + fn group_primaries_end_on_an_unsettled_loop() { + let all = vec![annot(0, Some(1), true), annot(1, Some(0), true), annot(2, Some(2), true), annot(3, None, false)]; + assert_eq!(group_primaries(&all), [None, None, None, None]); + } + fn with(mut a: Annotation, subtype: &str, t: Option<&str>) -> Annotation { a.subtype = subtype.into(); a.contents = t.map(Into::into); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 3c0d3e6649..5552304a86 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", "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::a_group_member_on_another_page_is_a_comment_of_its_own", "crates/automation/tests/it/automation.rs::looping_groups_end_and_chained_groups_reach_their_primary", "crates/render/src/inspect/tests.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 Delete deletes the member clicked alone, the primary included, as Acrobat desktop does (one undo step; a subordinate left without its primary is a comment of its own; Replace Text's strikeout and caret delete together). ISO 32000-2 §12.5.6.2 has deletion act on the whole group, as PDF-XChange and Foxit do on one page; Acrobat's is followed as the more practical. A group is one page's: a subordinate whose primary is on another page is a comment of its own, listed, selected, moved and deleted on its own, as Acrobat (desktop and iPad), PDF-XChange and Foxit read it. Missing: restyling a member does not restyle its group as Acrobat does, a subordinate can't be dragged, resized or reshaped on its own, and it has no hover tooltip" +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::a_group_member_on_another_page_is_a_comment_of_its_own", "crates/automation/tests/it/automation.rs::looping_groups_end_and_chained_groups_reach_their_primary", "crates/render/src/inspect/tests.rs::group_primaries_follow_irt_to_the_first_non_subordinate", "crates/annot/src/tests.rs::deleting_a_primary_releases_its_subordinate", "crates/engine/src/tests.rs::deleting_a_group_primary_releases_its_member_in_one_undo_step"] +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 Delete deletes the member clicked alone, the primary included, as Acrobat desktop does (one undo step; a subordinate left without its primary is a comment of its own and loses its `/IRT`, keeping `/RT /Group` as Acrobat does, so a full save leaves the deleted primary out; clearing redaction marks does the same for a mark's group member; Replace Text's strikeout and caret delete together). ISO 32000-2 §12.5.6.2 has deletion act on the whole group, as PDF-XChange and Foxit do on one page; Acrobat's is followed as the more practical. A group is one page's: a subordinate whose primary is on another page is a comment of its own, listed, selected, moved and deleted on its own, as Acrobat (desktop and iPad), PDF-XChange and Foxit read it. Missing: restyling a member does not restyle its group as Acrobat does, a subordinate can't be dragged, resized or reshaped on its own, and it has no hover tooltip" [[feature]] id = "comment.appearance-generation" From a0254022cf1a0e2b73edf4555b313182ea2f6f31 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:47:29 +0800 Subject: [PATCH 09/10] M5: selecting from anywhere but a page press forgets the group member a press picked CommentView remembered the group member a press on the page picked, so Delete acts on the member clicked, as Acrobat desktop does. The memory outlived the selection it came from: after choosing the group's card in the Comments panel, a deletion, an undo or a selection made by a tool, Delete still took the member clicked earlier. Every selection now goes through CommentView::select(), which clears the remembered member; pick() (a press on the page: the Select tool's press and click, and the context-menu press) is the only place that sets it. The panel's cards and their Delete, Delete and Backspace, Escape, the context menus (Delete, Flatten comments), deletion and undo (annotation_deleted, history_stepped), the drawing tools, the measuring panel and the control channel use select(). Test: choosing_the_groups_card_makes_delete_act_on_the_card_not_the_member_clicked (ui-egui). With select() leaving the member in place it fails: the Circle clicked earlier goes instead of the Square the card shows (left: ["Square"], right: ["Circle"]). CodeRabbit review on #4358. (cherry picked from commit a24b5ae4e8590be640028f501c9fe87cfdf679d2) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyG234xeYkJfr41E6iP --- crates/ui-egui/src/app/control.rs | 2 +- crates/ui-egui/src/canvas.rs | 2 +- crates/ui-egui/src/editing.rs | 6 ++-- crates/ui-egui/src/panels/comments_panel.rs | 4 +-- crates/ui-egui/src/tools/comments/input.rs | 40 ++++++++++++--------- crates/ui-egui/src/tools/comments/menu.rs | 4 +-- crates/ui-egui/src/tools/comments/state.rs | 20 +++++++++-- crates/ui-egui/src/tools/measure_ui.rs | 2 +- crates/ui-egui/tests/it/comments.rs | 23 ++++++++++++ 9 files changed, 75 insertions(+), 28 deletions(-) diff --git a/crates/ui-egui/src/app/control.rs b/crates/ui-egui/src/app/control.rs index e4078efd13..5ab24328f7 100644 --- a/crates/ui-egui/src/app/control.rs +++ b/crates/ui-egui/src/app/control.rs @@ -383,7 +383,7 @@ impl PdfCraftApp { let (p, i) = value.split_once(':').ok_or("comment: PAGE:INDEX")?; let (p, i): (usize, usize) = (p.trim().parse().map_err(|_| "comment: PAGE:INDEX")?, i.trim().parse().map_err(|_| "comment: PAGE:INDEX")?); - v.comments.selected = Some((p.saturating_sub(1), i.saturating_sub(1))); + v.comments.select(Some((p.saturating_sub(1), i.saturating_sub(1)))); v.comments.reveal = true; } // `--combine-view list`: Combine files as thumbnails (grid) or as the table (list). diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index a220644421..b6c9a8aec0 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -3987,7 +3987,7 @@ fn apply_frame_requests(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui, Some(crate::comments::Composer { page, at, kind: crate::comments::ComposerKind::Note, text: String::new(), focus: true }); } Some(PageMenuAction::FlattenComments) => { - app.views[index].comments.selected = None; + app.views[index].comments.select(None); app.views[index].pending_edit = Some(pdfcraft_engine::Edit::Flatten { comments: true, fields: false }); } Some(PageMenuAction::AddBookmark) => { diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index b810e2abe5..7d73d004cf 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -58,12 +58,12 @@ fn select_after_edit( crate::QuickTool::Comment(crate::comments::CommentTool::Ink | crate::comments::CommentTool::FreehandHighlight) ) => { - view.comments.selected = None; + view.comments.select(None); } Edit::AddAnnotation(a) => { // Select the new comment (appended last among the page's comments). let newest = info.annotations.iter().filter(|x| x.page == a.page && !x.is_reply()).map(|x| x.index).max(); - view.comments.selected = newest.map(|n| (a.page, n)); + view.comments.select(newest.map(|n| (a.page, n))); view.comments.reveal = true; // A new highlight opens its note for typing straight away, as in Acrobat, // unless a note typed into another card is still unsaved. @@ -697,7 +697,7 @@ impl PdfCraftApp { .session .get(view.id) .and_then(|d| d.info.annotations.iter().filter(|a| a.page == page && !a.is_reply()).map(|a| a.index).max()); - view.comments.selected = newest.map(|index| (page, index)); + view.comments.select(newest.map(|index| (page, index))); view.comments.reveal = true; self.quick_tool = crate::QuickTool::Select; } diff --git a/crates/ui-egui/src/panels/comments_panel.rs b/crates/ui-egui/src/panels/comments_panel.rs index cae1afc4eb..cc3b9c2494 100644 --- a/crates/ui-egui/src/panels/comments_panel.rs +++ b/crates/ui-egui/src/panels/comments_panel.rs @@ -571,7 +571,7 @@ fn card(ui: &mut egui::Ui, view: &mut DocView, context: CardContext<'_>) -> Opti .on_hover_cursor(egui::CursorIcon::PointingHand) .on_hover_text(crate::i18n::fmt(tl!("{kind} — click to show it on the page"), &[("kind", tl!(kind_label(a)))])); if click.clicked() { - view.comments.selected = Some(key); + view.comments.select(Some(key)); view.comments.reply.clear(); *nav = Some(Nav::Flash(a.page, a.shown_rect(rotation))); } @@ -622,7 +622,7 @@ fn card(ui: &mut egui::Ui, view: &mut DocView, context: CardContext<'_>) -> Opti ui.separator(); // A locked reply can't be deleted, so it keeps the comment it answers too. if ui.add_enabled(doc.annotation_deletable(a.page, a.index), egui::Button::new(tl!("Delete"))).clicked() { - view.comments.selected = None; + view.comments.select(None); edit = Some(Edit::DeleteAnnotation { page: a.page, index: a.index }); ui.close(); } diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index 5d7c870154..305b537d82 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -32,8 +32,10 @@ pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, { cv.context_at = Some((cx.page, cx.to_user(p))); // On a group subordinate, its group (the primary), so the menu offers the comment's actions. - cv.selected = cx.click_hit(p).map(|a| (cx.page, a.index)); - cv.member = cx.picked(p, cv.selected); + match cx.click_hit(p) { + Some(a) => cv.pick(cx, p, a), + None => cv.select(None), + } } let gestures = Gestures { ui, resp, cx, pointer, origin, page_rect, pressed_here, over_page }; match cx.tool { @@ -209,7 +211,7 @@ fn select_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, view: &mu consumed = true; } CommentPart::Body if !is_markup(&a.subtype) => { - cv.selected = Some((cx.page, a.index)); + cv.pick(cx, o, a); cv.reveal = true; if cx.allowed { cv.gesture = Some(Gesture::Move { page: cx.page, index: a.index, from: o }); @@ -264,12 +266,14 @@ fn select_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, view: &mu // leaves comments unselected. match owner { Some((a, _)) => { - cv.selected = Some((cx.page, a.index)); - cv.member = pointer.and_then(|p| cx.picked(p, cv.selected)); + match pointer { + Some(p) => cv.pick(cx, p, a), + None => cv.select(Some((cx.page, a.index))), + } cv.reveal = true; consumed = true; } - None => cv.selected = None, + None => cv.select(None), } } if resp.double_clicked() @@ -344,7 +348,7 @@ impl Gestures<'_> { && let Some(o) = origin { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![cx.to_user(o)] }); - cv.selected = None; + cv.select(None); } if let Some(Gesture::Draw { page, tool, points }) = cv.gesture.as_mut() && *page == cx.page @@ -384,7 +388,7 @@ impl Gestures<'_> { && let Some(p) = pointer && let Some(shape) = drawn_shape(tool, &[cx.to_user(p)]) { - cv.selected = None; + cv.select(None); let now = ui.input(|i| i.time); cv.settle(now, cx.page, tool, &[cx.to_user(p)]); view.pending_edit = Some(new_comment(cx, tool, shape, String::new())); @@ -435,7 +439,7 @@ impl Gestures<'_> { } _ => { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![u] }); - cv.selected = None; + cv.select(None); } } } @@ -467,7 +471,7 @@ impl Gestures<'_> { && let Some(o) = origin { cv.gesture = Some(Gesture::Draw { page: cx.page, tool: CommentTool::Callout, points: vec![cx.to_user(o)] }); - cv.selected = None; + cv.select(None); } if let Some(Gesture::Draw { page, points, .. }) = cv.gesture.as_mut() && *page == cx.page @@ -504,7 +508,7 @@ impl Gestures<'_> { && let Some(p) = pointer { cv.attach_at = Some((cx.page, cx.to_user(p))); - cv.selected = None; + cv.select(None); } true } @@ -526,7 +530,7 @@ impl Gestures<'_> { let snapped = cx.info.pages.get(cx.page).zip(caret_text).and_then(|(page, t)| snapped_caret(page, t, vx, vy)); let (at, size) = snapped.unwrap_or_else(|| (cx.to_user(p), CARET_SIZE)); cv.composer = Some(Composer { page: cx.page, at, kind: ComposerKind::Caret { size }, text: String::new(), focus: true }); - cv.selected = None; + cv.select(None); } true } @@ -548,7 +552,7 @@ impl Gestures<'_> { && let Some(o) = origin { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![cx.to_user(o)] }); - cv.selected = None; + cv.select(None); } if let Some(Gesture::Draw { page, points, .. }) = cv.gesture.as_mut() && *page == cx.page @@ -578,7 +582,7 @@ impl Gestures<'_> { { let kind = if tool == CommentTool::Note { ComposerKind::Note } else { ComposerKind::TextBox { size: None } }; cv.composer = Some(Composer { page: cx.page, at: cx.to_user(p), kind, text: String::new(), focus: true }); - cv.selected = None; + cv.select(None); } true } @@ -595,7 +599,11 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool let cv = &mut view.comments; if ctx.input(|i| i.key_pressed(egui::Key::Escape)) { // One step back per press: cancel the gesture, then deselect, then drop the tool. - let cancelled = cv.gesture.take().is_some() || cv.selected.take().is_some(); + let cancelled = cv.gesture.take().is_some() || { + let deselected = cv.selected.is_some(); + cv.select(None); + deselected + }; if !cancelled && matches!(tool, QuickTool::Comment(_)) { *tool = QuickTool::Select; } @@ -604,7 +612,7 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool && let Some((page, index)) = cv.target() && ctx.input(|i| i.key_pressed(egui::Key::Delete) || i.key_pressed(egui::Key::Backspace)) { - cv.selected = None; + cv.select(None); view.pending_edit = Some(Edit::DeleteAnnotation { page, index }); } } diff --git a/crates/ui-egui/src/tools/comments/menu.rs b/crates/ui-egui/src/tools/comments/menu.rs index af2b792f1a..708b31cad4 100644 --- a/crates/ui-egui/src/tools/comments/menu.rs +++ b/crates/ui-egui/src/tools/comments/menu.rs @@ -96,7 +96,7 @@ pub(crate) fn context_menu( // subordinate, Delete takes just it, as Acrobat's does. let (dpage, dindex) = view.comments.target().unwrap_or((page, index)); if ui.add_enabled(allowed && deletable(dpage, dindex), egui::Button::new(tl!("Delete"))).clicked() { - view.comments.selected = None; + view.comments.select(None); action = Some(CanvasAction::Edit(Box::new(Edit::DeleteAnnotation { page: dpage, index: dindex }))); ui.close(); } @@ -127,7 +127,7 @@ pub(crate) fn context_menu( if !info.annotations.is_empty() { ui.separator(); if ui.add_enabled(can_modify, egui::Button::new(tl!("Flatten comments"))).clicked() { - view.comments.selected = None; + view.comments.select(None); action = Some(CanvasAction::Edit(Box::new(Edit::Flatten { comments: true, fields: false }))); ui.close(); } diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index b987ab5ba0..149357dfbe 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -202,6 +202,22 @@ impl CommentView { } } + /// Select `key`, or nothing, from anywhere but a press on the page ([`Self::pick`]): the group + /// member a press picked goes, so Delete and the arrow keys act on what is selected now, not + /// on a member remembered from before (after the Comments panel, a deletion or undo). + pub(crate) fn select(&mut self, key: Option<(usize, usize)>) { + self.selected = key; + self.member = None; + } + + /// Select `a`, the comment a press at `p` on this page selects ([`PageCx::click_hit`]), + /// remembering the group subordinate under `p` when `a` is its group's primary + /// ([`PageCx::picked`]). The only place that sets [`Self::member`]. + pub(crate) fn pick(&mut self, cx: &PageCx<'_>, p: Pos2, a: &Annotation) { + self.selected = Some((cx.page, a.index)); + self.member = cx.picked(p, self.selected); + } + /// Takes back the last point of a polygon, cloud or connected lines being drawn; the first /// point's removal cancels the drawing. Returns whether there was such a drawing. pub fn undo_point(&mut self) -> bool { @@ -240,7 +256,7 @@ impl CommentView { /// were on, where it now is (deleting a reply keeps its comment selected), and close when it /// went too. pub(crate) fn annotation_deleted(&mut self, before: &[Annotation], after: &[Annotation], gone: (usize, usize)) { - self.selected = self.selected.and_then(|k| after_delete(before, after, gone, k)); + self.select(self.selected.and_then(|k| after_delete(before, after, gone, k))); self.editing = self.editing.take().and_then(|(p, i, text)| after_delete(before, after, gone, (p, i)).map(|(p, i)| (p, i, text))); if self.editing.is_none() { self.editor_focus = None; @@ -252,7 +268,7 @@ impl CommentView { /// editor close, its unsaved text with it, rather than Save writing that text over whatever /// took its place. pub(crate) fn history_stepped(&mut self) { - self.selected = None; + self.select(None); self.editing = None; self.editor_focus = None; } diff --git a/crates/ui-egui/src/tools/measure_ui.rs b/crates/ui-egui/src/tools/measure_ui.rs index 5008464ad9..60481f65da 100644 --- a/crates/ui-egui/src/tools/measure_ui.rs +++ b/crates/ui-egui/src/tools/measure_ui.rs @@ -562,7 +562,7 @@ fn panel_body(app: &mut PdfCraftApp, ui: &mut egui::Ui, _t: &Tokens) { } if let Some((page, annotation, reading, scale)) = select { app.views[index].current = page; - app.views[index].comments.selected = Some((page, annotation)); + app.views[index].comments.select(Some((page, annotation))); app.views[index].measure.reading = Some(reading); app.views[index].measure.scale = Some(scale); app.quick_tool = QuickTool::Select; diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 3b59436784..879c155829 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -3108,6 +3108,29 @@ fn delete_deletes_the_group_member_clicked() { assert_eq!(shapes(&h), [("Circle".to_string(), true)], "the Square goes, the Circle stays as a comment of its own"); } +/// CodeRabbit on #4358: the member a click on the page picked is forgotten once the selection is +/// made anywhere else. After clicking the Circle, choosing the group's card in the Comments panel +/// selects the comment the card shows, the Square, so Delete deletes the Square, not the Circle +/// the page click remembered. +#[test] +fn choosing_the_groups_card_makes_delete_act_on_the_card_not_the_member_clicked() { + let shapes = |h: &Harness<'static, PdfCraftApp>| -> Vec { + let mut out: Vec = comments(h).iter().filter(|a| a.subtype != "Text").map(|a| a.subtype.clone()).collect(); + out.sort(); + out + }; + let mut h = harness_with_bytes(GROUP_FIXTURE.to_vec(), |app| app.set_option("panel", "comments").unwrap()); + click_pt(&mut h, (220.0, 150.0)); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the Circle's click selects the group"); + // The selected card's header row, with its author. + h.get_by_label("Alice").click(); + h.run_steps(3); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the card selects the same group"); + h.key_press(egui::Key::Delete); + h.run_steps(3); + assert_eq!(shapes(&h), ["Circle".to_string()], "the card's comment, the Square, goes; the Circle stays"); +} + /// `n` pages with one comment each, "Card 1" to "Card n". fn one_comment_per_page(n: usize) -> Vec { let mut objs = vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string()]; From 4d89082ab54633979193e0f8132a2cbadd089369 Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Thu, 5 Nov 2026 04:22:50 +0800 Subject: [PATCH 10/10] M5: group selection goes through the page router's one look at the point (#4358) main now checks that only listed functions ask which comment lies under a point. The group lookup is `click_target` (a subordinate stands for its primary, ISO 32000-2 12.5.6.2), listed with `picked`; a right-click acts on the comment the router already found instead of looking again. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyDBXygTf7jcqdsjcfn --- crates/ui-egui/src/canvas/page_pointer/tests.rs | 9 +++++++-- crates/ui-egui/src/tools/comments/input.rs | 7 ++++--- crates/ui-egui/src/tools/comments/state.rs | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/canvas/page_pointer/tests.rs b/crates/ui-egui/src/canvas/page_pointer/tests.rs index 3f5d1e105f..634faf0cbc 100644 --- a/crates/ui-egui/src/canvas/page_pointer/tests.rs +++ b/crates/ui-egui/src/canvas/page_pointer/tests.rs @@ -559,9 +559,14 @@ const PAGE_HITS_ASKERS: [(&str, &str, &str); 5] = [ const COMMENT_POINT_TESTS: [&str; 2] = [".comment_at(", "PageCx::comment_at("]; /// The functions that ask which comment lies under a point: (file, function, why). -const COMMENT_POINT_TESTERS: [(&str, &str, &str); 3] = [ +const COMMENT_POINT_TESTERS: [(&str, &str, &str); 4] = [ ("tools/comments/input.rs", "hit", "the comments' hit test"), - ("tools/comments/input.rs", "context_hit", "the comments' hit test for a right-click"), + ( + "tools/comments/state.rs", + "click_target", + "the comments' hit tests' lookup: a group subordinate (`/RT /Group`, ISO 32000-2 §12.5.6.2) stands for its primary", + ), + ("tools/comments/state.rs", "picked", "which group subordinate a click selected its primary through, when the click is acted on"), ("tools/comments/paint.rs", "paint_page", "a redaction mark shows its fill under the pointer with every tool, as comments' tips show"), ]; diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index d0a1b3fdc1..5012c821a7 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -103,7 +103,7 @@ pub(crate) fn hit(cx: &PageCx<'_>, view: &DocView, p: Pos2, on_link: bool) -> Op // there selects the group. Only a click: a drag still moves what is under the pointer. let (a, member) = match cx.comment_at(p) { Some(a) => (a, false), - None => (cx.click_hit(p)?, true), + None => (cx.click_target(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. @@ -131,7 +131,7 @@ pub(crate) fn context_hit(cx: &PageCx<'_>, p: Pos2) -> Option { if cx.hidden || !cx.xf.rect.contains(p) { return None; } - let a = cx.click_hit(p)?; + let a = cx.click_target(p)?; Some(Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer(a), egui::CursorIcon::Default)) } @@ -146,7 +146,8 @@ pub(crate) fn anchor(cx: &PageCx<'_>, p: Pos2, view: &mut DocView) { /// anything else selects none. pub(crate) fn context(cx: &PageCx<'_>, p: Pos2, index: Option, view: &mut DocView) { match index { - Some(i) => match cx.click_hit(p).filter(|a| a.index == i) { + // `index` is what the right-click is on ([`context_hit`]): no second look at the point. + Some(i) => match cx.get(i) { Some(a) => view.comments.pick(cx, p, a), None => view.comments.select(Some((cx.page, i))), }, diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index 4d5f63e792..33177590ba 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -208,7 +208,7 @@ impl CommentView { self.member = None; } - /// Select `a`, the comment a press at `p` on this page selects ([`PageCx::click_hit`]), + /// Select `a`, the comment a press at `p` on this page selects ([`PageCx::click_target`]), /// remembering the group subordinate under `p` when `a` is its group's primary /// ([`PageCx::picked`]). The only place that sets [`Self::member`]. pub(crate) fn pick(&mut self, cx: &PageCx<'_>, p: Pos2, a: &Annotation) { @@ -439,7 +439,7 @@ impl PageCx<'_> { /// 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> { + pub(super) fn click_target(&self, p: Pos2) -> Option<&Annotation> { self.comment_at(p).or_else(|| self.member_at(p).map(|(_, p)| p)) }