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 <noreply@anthropic.com>
This commit is contained in:
alehelandClaude Opus 5.5 committed 2026-10-28 12:33:14 +01:00
1 parent 72edd945e5
commit 28a31aebc0
13 files changed
+120 -34

No files matched your search

+2 -2
View File
@@ -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,
+21 -7
View File
@@ -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<Object> = list.into_iter().enumerate().filter(|(i, e)| *i != index && !goes_with(e, &doomed)).map(|(_, e)| e).collect();
+4
View File
@@ -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(&copy, 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<String> = list(&doc, 0).iter().map(|d| text(d, b"Contents")).collect();
+1 -1
View File
@@ -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() }))
})?
+1 -1
View File
@@ -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());
+18
View File
@@ -3245,6 +3245,24 @@ fn comment_text(s: &Session, id: DocId) -> Vec<String> {
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]
+13 -5
View File
@@ -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<Edit> {
fn reply_menu(ui: &egui::Ui, view: &mut DocView, r: &Annotation, info: &DocInfo, more: &egui::Response) -> Option<Edit> {
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();
}
+1 -1
View File
@@ -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);
+3 -2
View File
@@ -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();
+28 -13
View File
@@ -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<usize, usize> = 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<usize, usize> = 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();
@@ -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.
+7
View File
@@ -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();
+2 -2
View File
@@ -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"