From 28a31aebc025bf728f8487b16d214f699dca5e8a Mon Sep 17 00:00:00 2001 From: alehel Date: Wed, 28 Oct 2026 12:33:14 +0100 Subject: [PATCH] fix(comments): previews lift a stamp whatever its replies' locks; Delete greys out over a locked reply The locked-reply check in delete_annotation also reached stamp_preview and signature_image, which only take the annotation off a copy to draw the page beneath it: an unlocked stamp with a locked reply lost its drag preview. They now use lift_annotation, which checks no locks. The Comments panel's Delete (comment and reply menus) and the page's context menu are greyed out while a reply below is locked (comments::locked_below), rather than refusing after the click. Co-Authored-By: Claude Opus 5.5 --- crates/annot/src/lib.rs | 4 +- crates/annot/src/modify.rs | 28 ++++++++++---- crates/annot/src/tests.rs | 4 ++ crates/engine/src/signature_image.rs | 2 +- crates/engine/src/stamp_preview.rs | 2 +- crates/engine/src/tests.rs | 18 +++++++++ crates/ui-egui/src/panels/comments_panel.rs | 18 ++++++--- crates/ui-egui/src/tools/comments.rs | 2 +- crates/ui-egui/src/tools/comments/menu.rs | 5 ++- crates/ui-egui/src/tools/comments/state.rs | 41 ++++++++++++++------- crates/ui-egui/src/tools/comments/tests.rs | 19 ++++++++++ crates/ui-egui/tests/it/comments.rs | 7 ++++ parity/acrobat-features.toml | 4 +- 13 files changed, 120 insertions(+), 34 deletions(-) diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index e9e0359ce0..ae7e5a8727 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -36,8 +36,8 @@ use lookup::{annots, page_ref}; pub use lookup::{has_visible_fill_sign, is_fill_sign, isolate_annotation, page_refs, picture_rotation, set_annots, signature_image}; pub(crate) use modify::n; pub use modify::{ - add_reply, add_text_replacement, delete_annotation, erase_ink, fillable, goes_with, 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, 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, }; 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 92807babd0..8204e65107 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -70,11 +70,23 @@ 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. pub fn delete_annotation(doc: &mut Document, page: usize, index: usize) -> Result<(), AnnotError> { + remove_with_replies(doc, page, index, true) +} + +/// Take the annotation at `(page, index)` out of its page with its pop-up and replies, as +/// [`delete_annotation`] does but whatever their locks: for drawing the page beneath it on a +/// copy of the document (a stamp or signature being dragged), which changes nothing. +pub fn lift_annotation(doc: &mut Document, page: usize, index: usize) -> Result<(), AnnotError> { + remove_with_replies(doc, page, index, false) +} + +fn remove_with_replies(doc: &mut Document, page: usize, index: usize, check_locks: bool) -> Result<(), AnnotError> { let p = page_ref(doc, page)?; let list = annots(doc, p); let target = list.get(index).cloned().ok_or(AnnotError::NoSuchAnnotation { page, index })?; - if let Some(r) = target.as_ref() { + if check_locks && let Some(r) = target.as_ref() { unlocked(doc, r)?; } // An inline comment has no reference to collect; the pop-up it names still goes. @@ -82,12 +94,14 @@ pub fn delete_annotation(doc: &mut Document, page: usize, index: usize) -> Resul let doomed = with_replies_and_popups(doc, &list, seed); // 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. - 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())); + if check_locks { + 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 kept: Vec = list.into_iter().enumerate().filter(|(i, e)| *i != index && !goes_with(e, &doomed)).map(|(_, e)| e).collect(); diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index d1e2f97e89..f9992a949e 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -242,6 +242,10 @@ fn a_locked_reply_keeps_what_it_answers() { assert_eq!(delete_annotation(&mut doc, 0, reply), Err(AnnotError::Invalid("a reply to it is locked".into()))); assert_eq!(delete_annotation(&mut doc, 0, note), Err(AnnotError::Invalid("a reply to it is locked".into()))); assert_eq!(list(&doc, 0).len(), before, "nothing went"); + // Lifted on a copy, to draw the page beneath, the locks don't count. + let mut copy = doc.clone(); + lift_annotation(&mut copy, 0, note).unwrap(); + assert!(list(©, 0).len() < before, "the thread is lifted off the copy"); set_locked(&mut doc, 0, answer, false).unwrap(); delete_annotation(&mut doc, 0, reply).unwrap(); let left: Vec = list(&doc, 0).iter().map(|d| text(d, b"Contents")).collect(); diff --git a/crates/engine/src/signature_image.rs b/crates/engine/src/signature_image.rs index daff674c19..ae8842ddfb 100644 --- a/crates/engine/src/signature_image.rs +++ b/crates/engine/src/signature_image.rs @@ -61,7 +61,7 @@ impl Document { pdfcraft_annot::props(&editor.cos, page, index).map(|p| p.opacity).filter(|v| v.is_finite()).unwrap_or(1.0).clamp(0.0, 1.0) as f32; let turn = pdfcraft_annot::picture_rotation(&editor.cos, page, index).map_err(|e| e.to_string())?; let mut cos = editor.cos.clone(); - pdfcraft_annot::delete_annotation(&mut cos, page, index).map_err(|e| e.to_string())?; + pdfcraft_annot::lift_annotation(&mut cos, page, index).map_err(|e| e.to_string())?; let bytes = Arc::new(pdfcraft_cos::write_incremental(&cos, &pdfcraft_cos::SaveOptions::default()).map_err(|e| e.to_string())?); Ok(Some(ImageSignaturePreview { image, opacity, turn, page, bytes, config: self.config.clone() })) })? diff --git a/crates/engine/src/stamp_preview.rs b/crates/engine/src/stamp_preview.rs index 6e5758a7ce..d52496cdcd 100644 --- a/crates/engine/src/stamp_preview.rs +++ b/crates/engine/src/stamp_preview.rs @@ -73,7 +73,7 @@ impl Document { return Ok(None); } let mut background = editor.cos.clone(); - pdfcraft_annot::delete_annotation(&mut background, page, index).map_err(|e| e.to_string())?; + pdfcraft_annot::lift_annotation(&mut background, page, index).map_err(|e| e.to_string())?; // The page beneath as it is displayed: comments without an appearance get theirs. let background = pdfcraft_annot::with_missing_appearances(&background).unwrap_or(background); let write = |cos: &pdfcraft_cos::Document| write_incremental(cos, &SaveOptions::default()).map(Arc::new).map_err(|e| e.to_string()); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 14a61b2633..31b426992c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3245,6 +3245,24 @@ fn comment_text(s: &Session, id: DocId) -> Vec { t } +/// A stamp whose reply is locked can still be dragged, so its layers are still prepared: the page +/// beneath is drawn on a copy, where locks don't count. Deleting the stamp is what they refuse. +#[test] +fn a_stamp_with_a_locked_reply_still_has_its_drag_layers() { + let (mut s, id) = session_with(1); + s.apply(id, fill_item(Shape::Mark { rect: [30.0, 60.0, 130.0, 66.0], mark: FillMark::Line }, "")).unwrap(); + s.apply(id, Edit::ReplyToAnnotation { page: 0, index: 0, text: "Approved".into(), author: "Bob".into() }).unwrap(); + let reply = s.get(id).unwrap().info.annotations.iter().find(|a| a.parent == Some((0, 0))).expect("the reply").index; + s.apply(id, Edit::LockAnnotation { page: 0, index: reply, locked: true }).unwrap(); + let doc = s.get(id).unwrap(); + let background = doc.stamp_preview(0, 0).unwrap().expect("the line is a stamp").render_background(1.0).unwrap(); + let px = |p: &pdfcraft_render::RenderedPage, x: u32, y: u32| -> [u8; 4] { + p.rgba[(((300 - y) * p.width + x) * 4) as usize..][..4].try_into().unwrap() + }; + assert!((30..=130).all(|x| px(&background, x, 63) == [255, 255, 255, 255]), "the page beneath has no line"); + assert!(s.apply(id, Edit::DeleteAnnotation { page: 0, index: 0 }).is_err(), "the locked reply keeps the stamp"); +} + /// A dragged stamp's layers: the page without it (its text and other comments kept) and the /// stamp alone on a transparent page. Preparing them changes nothing in the document. #[test] diff --git a/crates/ui-egui/src/panels/comments_panel.rs b/crates/ui-egui/src/panels/comments_panel.rs index 5a1b736f63..9d49a68c0b 100644 --- a/crates/ui-egui/src/panels/comments_panel.rs +++ b/crates/ui-egui/src/panels/comments_panel.rs @@ -405,7 +405,7 @@ fn card(ui: &mut egui::Ui, view: &mut DocView, context: CardContext<'_>) -> Opti if selected && allowed && !r.grouped { ui.with_layout(Layout::right_to_left(Align::Center), |ui| { let more = icons::button(ui, "ellipsis", 20.0, false, tl!("Options for this reply")); - if let Some(e) = reply_menu(ui, view, r, &more) { + if let Some(e) = reply_menu(ui, view, r, info, &more) { edit = Some(e); } }); @@ -524,7 +524,11 @@ fn card(ui: &mut egui::Ui, view: &mut DocView, context: CardContext<'_>) -> Opti ui.close(); } ui.separator(); - if ui.add_enabled(!a.locked, egui::Button::new(tl!("Delete"))).clicked() { + // A locked reply can't be deleted, so it keeps the comment it answers too. + if ui + .add_enabled(!a.locked && !crate::comments::locked_below(&info.annotations, (a.page, a.index)), egui::Button::new(tl!("Delete"))) + .clicked() + { view.comments.selected = None; edit = Some(Edit::DeleteAnnotation { page: a.page, index: a.index }); ui.close(); @@ -562,7 +566,7 @@ fn inline_editor(ui: &mut egui::Ui, t: &Tokens, view: &mut DocView, key: (usize, /// the reply alone, with what hangs off it (its own replies, status changes and pop-up), as /// deleting a comment takes its thread; the comment and its other replies stay. Returns the /// edit to apply. -fn reply_menu(ui: &egui::Ui, view: &mut DocView, r: &Annotation, more: &egui::Response) -> Option { +fn reply_menu(ui: &egui::Ui, view: &mut DocView, r: &Annotation, info: &DocInfo, more: &egui::Response) -> Option { let mut edit = None; let menu_id = ui.scope_id().with(("reply-menu", r.page, r.index)); if more.clicked() { @@ -583,8 +587,12 @@ fn reply_menu(ui: &egui::Ui, view: &mut DocView, r: &Annotation, more: &egui::Re ui.close(); } ui.separator(); - // The card stays selected: the app finds its comment again after the delete. - if ui.add_enabled(!r.locked, egui::Button::new(tl!("Delete"))).clicked() { + // The card stays selected: the app finds its comment again after the delete. A locked + // reply to this one keeps it. + if ui + .add_enabled(!r.locked && !crate::comments::locked_below(&info.annotations, (r.page, r.index)), egui::Button::new(tl!("Delete"))) + .clicked() + { edit = Some(Edit::DeleteAnnotation { page: r.page, index: r.index }); ui.close(); } diff --git a/crates/ui-egui/src/tools/comments.rs b/crates/ui-egui/src/tools/comments.rs index 6f3c074427..d86a0d58d2 100644 --- a/crates/ui-egui/src/tools/comments.rs +++ b/crates/ui-egui/src/tools/comments.rs @@ -47,7 +47,7 @@ use paint::{changing_points, drawn_shape, edited_in_view}; use state::after_delete; use state::login_name; pub use state::{CommentView, Composer, ComposerKind, Gesture, SortBy}; -pub(crate) use state::{EditorFocus, PageCx}; +pub(crate) use state::{EditorFocus, PageCx, locked_below}; /// Acrobat's selection blue for annotation frames and handles (11-visual-spec §5). const SELECT_BLUE: Color32 = Color32::from_rgb(0x14, 0x73, 0xE6); diff --git a/crates/ui-egui/src/tools/comments/menu.rs b/crates/ui-egui/src/tools/comments/menu.rs index d3de66a4e7..0ce50e5077 100644 --- a/crates/ui-egui/src/tools/comments/menu.rs +++ b/crates/ui-egui/src/tools/comments/menu.rs @@ -1,5 +1,5 @@ use super::paint::edit_anchor; -use super::state::{Composer, ComposerKind}; +use super::state::{Composer, ComposerKind, locked_below}; use super::{ Annotation, Color32, CommentPrefs, CommentTool, DocInfo, DocView, Edit, Rect, ReviewState, Rgb, SELECT_BLUE, SWATCHES, Sense, Stroke, color32, icons, theme, tool_for, vec2, @@ -89,7 +89,8 @@ pub(crate) fn context_menu( ui.close(); } ui.separator(); - if ui.add_enabled(allowed && !a.locked, egui::Button::new(tl!("Delete"))).clicked() { + // A locked reply can't be deleted, so it keeps the comment it answers too. + if ui.add_enabled(allowed && !a.locked && !locked_below(&info.annotations, (page, index)), egui::Button::new(tl!("Delete"))).clicked() { view.comments.selected = None; action = Some(CanvasAction::Edit(Box::new(Edit::DeleteAnnotation { page, index }))); ui.close(); diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index c405422481..c9ca91fb01 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -245,19 +245,7 @@ pub(super) fn after_delete(before: &[Annotation], after: &[Annotation], gone: (u return Some(keep); } let (old, new) = (on_page(before, gone.0), on_page(after, gone.0)); - // Only replies on the page go with it: `/IRT` is followed through the page's `/Annots`. - let parents: HashMap = old.iter().filter_map(|a| a.parent.filter(|p| p.0 == gone.0).map(|p| (a.index, p.1))).collect(); - let goes = |mut i: usize| { - // `link_replies` settles every chain within 64 steps. - for _ in 0..=64 { - if i == gone.1 { - return true; - } - let Some(&up) = parents.get(&i) else { return false }; - i = up; - } - false - }; + let goes = in_thread(&old, gone); let stays: Vec<&Annotation> = old.iter().copied().filter(|a| !goes(a.index)).collect(); if stays.len() != new.len() { return None; @@ -268,6 +256,33 @@ pub(super) fn after_delete(before: &[Annotation], after: &[Annotation], gone: (u same.then_some((now.page, now.index)) } +/// Whether the annotation at an index on `top`'s page is `top` or a reply to it at any depth: +/// what deleting `top` takes along. `page` is that page's listed annotations. Only replies on the +/// page go with it (`pdfcraft_annot::delete_annotation` follows `/IRT` through the page's +/// `/Annots`). +fn in_thread(page: &[&Annotation], top: (usize, usize)) -> impl Fn(usize) -> bool { + let parents: HashMap = page.iter().filter_map(|a| a.parent.filter(|p| p.0 == top.0).map(|p| (a.index, p.1))).collect(); + move |mut i: usize| { + // `link_replies` settles every chain within 64 steps. + for _ in 0..=64 { + if i == top.1 { + return true; + } + let Some(&up) = parents.get(&i) else { return false }; + i = up; + } + false + } +} + +/// Whether a reply to the annotation at `top`, at any depth, is locked: deleting it is refused +/// then, as the reply can't be deleted. +pub(crate) fn locked_below(annotations: &[Annotation], top: (usize, usize)) -> bool { + let page = on_page(annotations, top.0); + let goes = in_thread(&page, top); + page.iter().any(|a| a.index != top.1 && a.locked && goes(a.index)) +} + /// The annotations listed on `page`, in `/Annots` order. fn on_page(list: &[Annotation], page: usize) -> Vec<&Annotation> { let mut v: Vec<&Annotation> = list.iter().filter(|a| a.page == page).collect(); diff --git a/crates/ui-egui/src/tools/comments/tests.rs b/crates/ui-egui/src/tools/comments/tests.rs index bf000539a5..36644bb445 100644 --- a/crates/ui-egui/src/tools/comments/tests.rs +++ b/crates/ui-egui/src/tools/comments/tests.rs @@ -90,6 +90,25 @@ fn listed(page: usize, index: usize, contents: &str, parent: Option<(usize, usiz } } +/// A locked reply at any depth keeps what it answers, up its own chain only: its comment, the +/// reply above it, but not a sibling reply or a comment on another page. +#[test] +fn a_locked_reply_keeps_the_replies_and_comment_above_it() { + let lock = |a: Annotation| Annotation { locked: true, ..a }; + let list = vec![ + listed(0, 0, "comment", None), + listed(0, 1, "A", Some((0, 0))), + lock(listed(0, 2, "to A, locked", Some((0, 1)))), + listed(0, 3, "B", Some((0, 0))), + listed(1, 0, "other page", None), + ]; + assert!(locked_below(&list, (0, 0)), "the comment"); + assert!(locked_below(&list, (0, 1)), "the reply above the locked one"); + assert!(!locked_below(&list, (0, 2)), "the locked reply itself: its own lock says so"); + assert!(!locked_below(&list, (0, 3)), "a sibling reply"); + assert!(!locked_below(&list, (1, 0)), "another page"); +} + /// What stays selected or open in the editor after a delete is found where it moved to: the /// reply's own replies go with it (pop-ups, unlisted, are gone too), and a page that lost /// something else, or whose annotation in that place reads differently, keeps nothing. diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index f8c26b59b8..2fcc2299d1 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -1411,6 +1411,13 @@ fn one_reply_is_deleted_from_its_menu_and_the_thread_stays() { h.run_steps(3); assert_eq!(texts(&h), ["The comment", "Right reply, reworded", "Accepted set by Ben"]); + // The locked reply keeps its comment too: deleting the comment would take it along. + h.get_by_label("More").click(); + h.run_steps(2); + assert!(h.get_by_label("Delete").accesskit_node().is_disabled(), "the comment of a locked reply isn't deleted"); + h.key_press(egui::Key::Escape); + h.run_steps(2); + h.state_mut().undo(); h.run_steps(2); h.state_mut().undo(); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 760fa0a338..c4670a584b 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -4003,8 +4003,8 @@ milestone = "M5" status = "shipped" commands = ["comment.list"] tools = ["comment_delete"] -evidence = ["crates/ui-egui/tests/it/comments.rs::comments_are_selected_moved_and_deleted_with_the_select_tool", "crates/annot/src/tests.rs::delete_takes_popup_and_reply_chain", "crates/ui-egui/tests/it/comments.rs::one_reply_is_deleted_from_its_menu_and_the_thread_stays", "crates/ui-egui/src/tools/comments/tests.rs::a_kept_comment_is_found_where_a_delete_moved_it", "crates/automation/tests/it/automation.rs::deleting_a_reply_keeps_its_comment_and_the_other_replies", "crates/annot/src/tests.rs::a_locked_reply_keeps_what_it_answers"] -notes = "deletes the pop-up and the reply thread. One reply is deleted from its own \"…\" in the Comments panel (comment_delete with the reply's id, or page and index), with its pop-up and the replies to it: the comment and its other replies stay, one undo step, and the card stays selected. A locked reply can't be deleted, nor the comment or reply it answers while it is locked." +evidence = ["crates/ui-egui/tests/it/comments.rs::comments_are_selected_moved_and_deleted_with_the_select_tool", "crates/annot/src/tests.rs::delete_takes_popup_and_reply_chain", "crates/ui-egui/tests/it/comments.rs::one_reply_is_deleted_from_its_menu_and_the_thread_stays", "crates/ui-egui/src/tools/comments/tests.rs::a_kept_comment_is_found_where_a_delete_moved_it", "crates/automation/tests/it/automation.rs::deleting_a_reply_keeps_its_comment_and_the_other_replies", "crates/annot/src/tests.rs::a_locked_reply_keeps_what_it_answers", "crates/ui-egui/src/tools/comments/tests.rs::a_locked_reply_keeps_the_replies_and_comment_above_it", "crates/engine/src/tests.rs::a_stamp_with_a_locked_reply_still_has_its_drag_layers"] +notes = "deletes the pop-up and the reply thread. One reply is deleted from its own \"…\" in the Comments panel (comment_delete with the reply's id, or page and index), with its pop-up and the replies to it: the comment and its other replies stay, one undo step, and the card stays selected. A locked reply can't be deleted, nor the comment or reply it answers while it is locked (their Delete is greyed out; a stamp or signature with a locked reply still drags with its live preview)." [[feature]] id = "comment.keyboard-nudge"