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"