From 466e1971d6be902e59c44c0edd4755feb0712ec2 Mon Sep 17 00:00:00 2001 From: jonathanwxh-cell Date: Tue, 10 Nov 2026 11:01:09 +0800 Subject: [PATCH] fix(annot,ui-egui): guard comment edits against non-comments and stale state (#3916) (#4673) --- crates/annot/src/info.rs | 5 +- crates/annot/src/lookup.rs | 17 +- crates/annot/src/modify.rs | 4 + crates/annot/src/tests.rs | 22 +++ crates/ui-egui/src/canvas.rs | 5 +- crates/ui-egui/src/editing.rs | 81 ++++++++-- crates/ui-egui/src/editing/tests.rs | 34 ++++ crates/ui-egui/src/tools/comments/composer.rs | 4 +- crates/ui-egui/src/tools/comments/input.rs | 16 +- crates/ui-egui/src/tools/comments/menu.rs | 2 +- crates/ui-egui/src/tools/comments/state.rs | 120 +++++++++++++-- crates/ui-egui/src/tools/comments/tests.rs | 145 +++++++++++++++++- 12 files changed, 401 insertions(+), 54 deletions(-) diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index 99c3264006..e91bb94e0a 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -123,7 +123,7 @@ pub fn is_comment_subtype(subtype: &str) -> bool { /// Where a reference to a reference (`5 0 obj 6 0 R endobj`) ends, as `pdfcraft_render::inspect` /// follows it, within a few steps (a cycle stops there). -fn final_ref(doc: &Document, mut r: ObjRef) -> ObjRef { +pub(crate) fn final_ref(doc: &Document, mut r: ObjRef) -> ObjRef { for _ in 0..8 { match &*doc.get(r) { Object::Ref(next) => r = *next, @@ -340,6 +340,9 @@ pub fn props(doc: &Document, page: usize, index: usize) -> Option { let direct = crate::draw::with_direct_values(doc, raw); let d = direct.as_ref(); let subtype = String::from_utf8_lossy(d.name(b"Subtype")?).into_owned(); + if !is_comment_subtype(&subtype) { + return None; + } // A text box reports the colour and size it is drawn in (`/DA`, or `/DS` where it sets one). let style = (subtype == "FreeText").then(|| text_style::text_style(d, None)); let color = match &style { diff --git a/crates/annot/src/lookup.rs b/crates/annot/src/lookup.rs index 9af83b0542..f8681f899b 100644 --- a/crates/annot/src/lookup.rs +++ b/crates/annot/src/lookup.rs @@ -1,4 +1,4 @@ -use super::info::text_value; +use super::info::{is_comment_subtype, text_value}; use super::types::{FLAG_HIDDEN, FLAG_NO_ROTATE, FLAG_NO_VIEW, Shape}; use super::{AnnotError, Dict, Document, ObjRef, Object, appearance}; @@ -106,16 +106,23 @@ pub fn set_annots(doc: &mut Document, page: ObjRef, list: Vec) -> Result Ok(()) } -/// The annotation at `(page, index)` as an indirect object (inline dictionaries are promoted). +/// Whether the annotation dictionary `d` has a comment `/Subtype` ([`is_comment_subtype`]). +pub(crate) fn is_comment_dict(d: &Dict) -> bool { + d.name(b"Subtype").is_some_and(|s| is_comment_subtype(&String::from_utf8_lossy(s))) +} + +/// The comment annotation at `(page, index)` as an indirect object (inline dictionaries are promoted). pub(crate) fn annot_ref(doc: &mut Document, page: usize, index: usize) -> Result<(ObjRef, ObjRef), AnnotError> { let p = page_ref(doc, page)?; let mut list = annots(doc, p); let entry = list.get(index).cloned().ok_or(AnnotError::NoSuchAnnotation { page, index })?; let r = match entry { - Object::Ref(r) if doc.get(r).as_dict().is_some() => r, - Object::Dict(d) => { + Object::Ref(r) if doc.get(r).as_dict().is_some_and(is_comment_dict) => r, + Object::Dict(d) if is_comment_dict(&d) => { let r = doc.add(Object::Dict(d)); - list[index] = Object::Ref(r); + if let Some(slot) = list.get_mut(index) { + *slot = Object::Ref(r); + } set_annots(doc, p, list)?; r } diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index eb77e3f973..c478c98a16 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -133,6 +133,10 @@ fn deleted_index(doc: &Document, page: usize, index: usize) -> Result bool { v.content.draft.as_ref().is_some_and(|d| d.index.is_none() && !d.text.trim().is_empty()) } +/// Clear an unfinished page-anchored drawing or composer when a page edit shifts or removes `page`. +fn clear_shifted_page_comment_state(view: &mut crate::DocView, shifted_from: usize) { + if matches!(view.comments.gesture, Some(crate::comments::Gesture::Draw { page, .. }) if page >= shifted_from) { + view.comments.gesture = None; + } + if view.comments.composer.as_ref().is_some_and(|c| !matches!(c.kind, crate::comments::ComposerKind::Edit(_)) && c.page >= shifted_from) { + view.comments.composer = None; + } +} + /// Keeps the pages (or comment) the user acted on selected, where they now are. `annots_before` /// is the page's annotations before a delete or a paste of comments (empty for other edits). fn select_after_edit( @@ -39,17 +49,27 @@ fn select_after_edit( ) { match edit { Edit::MovePages { pages, to } => { + clear_shifted_page_comment_state(view, 0); let n = pages.iter().collect::>().len(); let to = (*to).min(info.pages.len() - n); view.select_pages(&(to..to + n).collect::>()); } Edit::DuplicatePages { pages, at: Some(at) } => { + clear_shifted_page_comment_state(view, *at); let n = pages.iter().collect::>().len(); let at = (*at).min(info.pages.len().saturating_sub(n)); view.select_pages(&(at..at + n).collect::>()); } - Edit::InsertBlankPage { at, .. } => view.select_pages(&[(*at).min(info.pages.len() - 1)]), - Edit::DeletePages { .. } => view.select_pages(&[]), + Edit::InsertBlankPage { at, .. } => { + clear_shifted_page_comment_state(view, *at); + view.select_pages(&[(*at).min(info.pages.len() - 1)]); + } + Edit::DeletePages { pages } => { + if let Some(&first) = pages.iter().min() { + clear_shifted_page_comment_state(view, first); + } + view.select_pages(&[]); + } // A stroke drawn with the pen stays unselected, so the selection box and // author popup don't sit over the next stroke (#429). Edit::AddAnnotation(a) @@ -76,7 +96,6 @@ fn select_after_edit( view.comments.open_editor(a.page, n, String::new()); } } - Edit::DeleteAnnotation { page, index } => view.comments.annotation_deleted(annots_before, &info.annotations, (*page, *index)), // A group moved together stays selected (its positions don't change), so it // can be moved or copied again; anything else in a batch clears the selection. Edit::Batch { edits, .. } if edits.iter().all(|e| matches!(e, Edit::MoveAnnotation { .. })) => {} @@ -207,25 +226,15 @@ impl PdfCraftApp { .map(|before| form_edit_pages(&before, &doc.form)); let one_page = comment_page(&edit).or_else(|| content_page(&edit)); if signatures_before.is_some_and(|before| crate::canvas::signature_states_changed(&before, &doc.signatures)) { - // A signature shows its state on its own page, which an edit elsewhere can - // change (a certification forbidding it, say): every page redraws. An edit that - // moves no page (a form edit, or one to a page's comments or content) keeps the - // values typed, in it and before it, showing until their pages redraw. - let waiting = (form_edit.is_some() || one_page.is_some()).then(|| std::mem::take(&mut view.forms.unbaked)); - view.document_changed(info); - if let Some(waiting) = waiting { - view.forms.unbaked = waiting; - } - if let Some((_, unbaked)) = form_edit { - view.forms.unbaked.extend(unbaked); - } + signatures_changed_after_edit(view, &edit, info, &annots_before, form_edit.map(|(_, u)| u), one_page); } else if let Some(page) = one_page { - one_page_changed(view, &edit, page); + one_page_changed(view, &edit, page, &annots_before, info); } else if let Some((changed_pages, unbaked)) = form_edit { view.forms.unbaked.extend(unbaked); for page in changed_pages { view.page_changed(page); } + view.comments.document_changed(info); } else { view.document_changed(info); } @@ -1109,13 +1118,51 @@ fn content_page(edit: &Edit) -> Option { } } +/// A signature shows its state on its own page, which an edit elsewhere can change (a +/// certification forbidding it, say): every page redraws. An edit that moves no page (a form edit, +/// or one to a page's comments or content) keeps the values typed, in it and before it, showing +/// until their pages redraw. +fn signatures_changed_after_edit( + view: &mut crate::DocView, + edit: &Edit, + info: &pdfcraft_render::DocInfo, + annots_before: &[pdfcraft_render::Annotation], + form_unbaked: Option>, + one_page: Option, +) { + let waiting = (form_unbaked.is_some() || one_page.is_some()).then(|| std::mem::take(&mut view.forms.unbaked)); + if let Edit::DeleteAnnotation { page, index } = edit { + // Remap surviving comments before generic slot pruning loses their state. + view.comments.annotation_deleted(annots_before, info, (*page, *index)); + } + view.document_changed(info); + if let Some(waiting) = waiting { + view.forms.unbaked = waiting; + } + if let Some(unbaked) = form_unbaked { + view.forms.unbaked.extend(unbaked); + } +} + /// A comment or content edit changes one page, `page`: keep every other raster. Opening, closing /// or moving a pop-up changes none: the window is drawn over the page, and a /Popup annotation /// paints nothing in the raster (ISO 32000-2 §12.5.6.14). -fn one_page_changed(view: &mut crate::DocView, edit: &Edit, page: usize) { +fn one_page_changed( + view: &mut crate::DocView, + edit: &Edit, + page: usize, + annots_before: &[pdfcraft_render::Annotation], + info: &pdfcraft_render::DocInfo, +) { if !matches!(edit, Edit::SetAnnotationPopup { .. }) { view.page_changed(page); } + if let Edit::DeleteAnnotation { page, index } = edit { + // Remap surviving comments before generic slot pruning loses their state. + view.comments.annotation_deleted(annots_before, info, (*page, *index)); + } else { + view.comments.document_changed(info); + } } fn comment_page(edit: &Edit) -> Option { diff --git a/crates/ui-egui/src/editing/tests.rs b/crates/ui-egui/src/editing/tests.rs index b53ac49305..1f205ae759 100644 --- a/crates/ui-egui/src/editing/tests.rs +++ b/crates/ui-egui/src/editing/tests.rs @@ -375,6 +375,40 @@ fn save_commits_the_open_text_box() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn moving_pages_cancels_an_in_progress_drawing() { + let objs = [ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >>", + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 200 300] >>", + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 200 300] >>", + ]; + let mut pdf = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(pdf.len()); + pdf.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = pdf.len(); + pdf.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + pdf.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + pdf.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + + let mut app = crate::PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("a.pdf", None, pdf).unwrap(); + let drawing = + || Some(crate::comments::Gesture::Draw { page: 0, tool: crate::comments::CommentTool::Ink, points: vec![[10.0, 10.0], [30.0, 20.0]] }); + app.views[0].comments.gesture = drawing(); + assert!(app.apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: 90 })); + assert!(app.views[0].comments.gesture.is_some(), "rotating the page keeps the drawing on it"); + app.views[0].comments.gesture = drawing(); + assert!(app.apply_edit(pdfcraft_engine::Edit::MovePages { pages: vec![1], to: 0 })); + assert!(app.views[0].comments.gesture.is_none(), "the drawing's page index would now be a different page"); +} + #[test] fn save_asks_where_to_keep_an_email_attachment_and_leaves_the_temporary_copy_alone() { let dir = staging_dir("mail-temp"); diff --git a/crates/ui-egui/src/tools/comments/composer.rs b/crates/ui-egui/src/tools/comments/composer.rs index 842a5dfc84..4dad8d4c26 100644 --- a/crates/ui-egui/src/tools/comments/composer.rs +++ b/crates/ui-egui/src/tools/comments/composer.rs @@ -116,7 +116,9 @@ pub(crate) fn composer(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, )); let text = if typewriter_edit { c.text } else { c.text.trim_end().to_string() }; match c.kind { - ComposerKind::Edit(index) => Some(Edit::SetAnnotationContents { page, index, text }), + ComposerKind::Edit(index) => { + info.annotations.iter().any(|a| a.page == page && a.index == index).then_some(Edit::SetAnnotationContents { page, index, text }) + } ComposerKind::Note => { view.comments.tool_done = true; Some(new_comment(&cx, CommentTool::Note, Shape::Note { at: c.at, icon: NoteIcon::Comment }, text)) diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index cfaa4967ce..6e27e89537 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -740,7 +740,7 @@ impl Gestures<'_> { } /// Delete / Escape handling for comments (only while no text field has focus). -pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool, allowed: bool) { +pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, tool: &mut QuickTool, allowed: bool) { if ctx.egui_wants_keyboard_input() { return; } @@ -756,21 +756,19 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool *tool = QuickTool::Select; } } - if allowed - && cv.selected.is_some() - && ctx.input(|i| i.key_pressed(egui::Key::Delete) || i.key_pressed(egui::Key::Backspace)) - && let Some(edit) = delete_selected(cv) - { - view.pending_edit = Some(edit); + if allowed && cv.selected.is_some() && ctx.input(|i| i.key_pressed(egui::Key::Delete) || i.key_pressed(egui::Key::Backspace)) { + view.pending_edit = delete_selected(cv, info); } } /// The edit that deletes every selected comment (one undo step for several), and clears the /// selection. Later positions go first, so each deletion leaves the earlier ones where they are. -pub(crate) fn delete_selected(cv: &mut CommentView) -> Option { +pub(crate) fn delete_selected(cv: &mut CommentView, info: &DocInfo) -> Option { // A group's member picked on the page, with its group the selection alone, goes by itself, as // Acrobat deletes the member clicked ([`CommentView::target`]). - let mut keys = if cv.also.is_empty() { cv.target().into_iter().collect() } else { cv.selection() }; + let raw = if cv.also.is_empty() { cv.target().into_iter().collect() } else { cv.selection() }; + let mut keys: Vec<(usize, usize)> = + raw.into_iter().filter(|(page, index)| info.annotations.iter().any(|a| a.page == *page && a.index == *index && !a.locked)).collect(); cv.clear_selection(); keys.sort_unstable_by(|a, b| b.cmp(a)); keys.dedup(); diff --git a/crates/ui-egui/src/tools/comments/menu.rs b/crates/ui-egui/src/tools/comments/menu.rs index 4ee49921e4..d8da11fcc3 100644 --- a/crates/ui-egui/src/tools/comments/menu.rs +++ b/crates/ui-egui/src/tools/comments/menu.rs @@ -148,7 +148,7 @@ pub(crate) fn context_menu( let (dpage, dindex) = view.comments.target().unwrap_or((page, index)); let deletes = deletable(dpage, dindex) && view.comments.also.iter().all(|&(p, i)| deletable(p, i)); if ui.add_enabled(allowed && deletes, egui::Button::new(tl!(if several > 0 { "Delete comments" } else { "Delete" }))).clicked() { - action = delete_selected(&mut view.comments).map(|e| CanvasAction::Edit(Box::new(e))); + action = delete_selected(&mut view.comments, info).map(|e| CanvasAction::Edit(Box::new(e))); 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 62d6e9db36..acd869aa9c 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -207,6 +207,17 @@ pub struct CommentView { pub tool_done: bool, /// What the inline editor (`editing`) does with the keyboard the next time it is drawn. pub(crate) editor_focus: Option, + /// Snapshot of `(page, index)` comment slot identity from the last `DocInfo`, used to detect + /// when an edit removes or shifts `/Annots` entries under an active selection or editor. + pub(crate) slots: std::collections::HashMap<(usize, usize), CommentSlot>, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct CommentSlot { + subtype: String, + name: Option, + page_ord: usize, + page_count: usize, } /// A one-off change to the keyboard focus of the Comments panel's inline editor. The editor @@ -221,6 +232,78 @@ pub(crate) enum EditorFocus { } impl CommentView { + /// Initial commenting state for `info`, recording its current annotation slots. + pub(crate) fn from_info(info: &DocInfo) -> Self { + Self { slots: Self::snapshot_slots(info), ..Default::default() } + } + + /// Captures `(page, index)` comment slot identity for every comment in `info`. + fn snapshot_slots(info: &DocInfo) -> std::collections::HashMap<(usize, usize), CommentSlot> { + let mut by_page = std::collections::HashMap::>::new(); + for a in &info.annotations { + by_page.entry(a.page).or_default().push(a.index); + } + for indices in by_page.values_mut() { + indices.sort_unstable(); + indices.dedup(); + } + let mut slots = std::collections::HashMap::with_capacity(info.annotations.len()); + for a in &info.annotations { + let indices = by_page.get(&a.page).map(Vec::as_slice).unwrap_or_default(); + let page_ord = indices.binary_search(&a.index).unwrap_or(0); + let page_count = indices.len(); + slots.insert((a.page, a.index), CommentSlot { subtype: a.subtype.clone(), name: a.name.clone(), page_ord, page_count }); + } + slots + } + + /// Prunes comment state whose `(page, index)` slot was removed or shifted by a document change. + pub(crate) fn document_changed(&mut self, info: &DocInfo) { + let next = Self::snapshot_slots(info); + let page_count = info.pages.len(); + let same_slot = |page: usize, index: usize| -> bool { + let Some(cur) = next.get(&(page, index)) else { + return false; + }; + if self.slots.is_empty() { + return true; + } + self.slots.get(&(page, index)).is_some_and(|old| { + cur.subtype == old.subtype && cur.name == old.name && cur.page_ord == old.page_ord && cur.page_count >= old.page_count + }) + }; + if self.selected.is_some_and(|(p, i)| !same_slot(p, i)) { + self.selected = None; + } + self.also.retain(|&(p, i)| same_slot(p, i)); + if self.selected.is_none() { + self.selected = self.also.pop(); + } + if self.member.is_some_and(|((mp, mi), (gp, gi))| Some((gp, gi)) != self.selected || !same_slot(mp, mi)) { + self.member = None; + } + if self.editing.as_ref().is_some_and(|(p, i, _)| !same_slot(*p, *i)) { + self.editing = None; + self.editor_focus = None; + } + if self.composer.as_ref().is_some_and(|c| match c.kind { + ComposerKind::Edit(i) => !same_slot(c.page, i), + _ => c.page >= page_count, + }) { + self.composer = None; + } + if self.gesture.as_ref().is_some_and(|g| match g { + Gesture::Draw { page, .. } => *page >= page_count, + Gesture::Move { page, index, .. } | Gesture::Resize { page, index, .. } | Gesture::Reshape { page, index, .. } => { + !same_slot(*page, *index) + } + Gesture::Place { .. } => false, + }) { + self.gesture = None; + } + self.slots = next; + } + /// Every selected comment, the one the handles belong to first. pub fn selection(&self) -> Vec<(usize, usize)> { let mut all: Vec<(usize, usize)> = self.selected.into_iter().collect(); @@ -333,24 +416,33 @@ impl CommentView { } } - /// The annotation at `gone` was just deleted; `before` and `after` are the document's + /// The annotation at `gone` was just deleted; `before` and `info` hold the document's /// annotations until then and now. The selection and the inline editor stay on what they /// 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.also = std::mem::take(&mut self.also).into_iter().filter_map(|k| after_delete(before, after, gone, k)).collect(); - self.member = None; - 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; - } - // What is typed in a pop-up, and a window being dragged or resized, follow theirs too. + pub(crate) fn annotation_deleted(&mut self, before: &[Annotation], info: &DocInfo, gone: (usize, usize)) { + let after = &info.annotations; let moved = |k: (usize, usize)| after_delete(before, after, gone, k); - self.popup_text = self.popup_text.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text))); - self.popup_reply = self.popup_reply.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text))); - self.popup_drag = self.popup_drag.and_then(|(p, i, at)| moved((p, i)).map(|(p, i)| (p, i, at))); - self.popup_resize = self.popup_resize.and_then(|(p, i, size)| moved((p, i)).map(|(p, i)| (p, i, size))); + let selected = self.selected.and_then(moved); + let also: Vec<(usize, usize)> = std::mem::take(&mut self.also).into_iter().filter_map(moved).collect(); + let editing = self.editing.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text))); + let focus = self.editor_focus.take(); + // What is typed in a pop-up, and a window being dragged or resized, follow theirs too. + let popup_text = self.popup_text.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text))); + let popup_reply = self.popup_reply.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text))); + let popup_drag = self.popup_drag.and_then(|(p, i, at)| moved((p, i)).map(|(p, i)| (p, i, at))); + let popup_resize = self.popup_resize.and_then(|(p, i, size)| moved((p, i)).map(|(p, i)| (p, i, size))); + // Other transient state still needs conservative pruning; refresh the slot snapshot too. + self.document_changed(info); + self.selected = selected; + self.also = also; + self.member = None; + self.editor_focus = editing.as_ref().and(focus); + self.editing = editing; + self.popup_text = popup_text; + self.popup_reply = popup_reply; + self.popup_drag = popup_drag; + self.popup_resize = popup_resize; } /// Undo or Redo just ran. Either may have put annotations back or taken them away, on any diff --git a/crates/ui-egui/src/tools/comments/tests.rs b/crates/ui-egui/src/tools/comments/tests.rs index 6c3e4ea163..63fa8cc312 100644 --- a/crates/ui-egui/src/tools/comments/tests.rs +++ b/crates/ui-egui/src/tools/comments/tests.rs @@ -165,11 +165,17 @@ fn a_kept_comment_is_found_where_a_delete_moved_it() { let changed = vec![listed(0, 0, "something else", None), listed(0, 1, "B", Some((0, 0)))]; assert_eq!(after_delete(&before, &changed, gone, (0, 2)), None); - let mut view = CommentView { selected: Some((0, 2)), editing: Some((0, 3, "typed".into())), ..Default::default() }; - view.annotation_deleted(&before, &after, gone); + let info = DocInfo { annotations: before.clone(), ..Default::default() }; + let mut view = CommentView::from_info(&info); + view.selected = Some((0, 2)); + view.editing = Some((0, 3, "typed".into())); + view.annotation_deleted(&before, &DocInfo { annotations: after.clone(), ..Default::default() }, gone); + view.document_changed(&DocInfo { annotations: after.clone(), ..Default::default() }); assert_eq!((view.selected, view.editing), (Some((0, 0)), Some((0, 1, "typed".into()))), "both follow"); - let mut view = CommentView { editing: Some((0, 4, "typed".into())), editor_focus: Some(EditorFocus::Take), ..Default::default() }; - view.annotation_deleted(&before, &after, gone); + let mut view = CommentView::from_info(&info); + view.editing = Some((0, 4, "typed".into())); + view.editor_focus = Some(EditorFocus::Take); + view.annotation_deleted(&before, &DocInfo { annotations: after.clone(), ..Default::default() }, gone); assert_eq!((view.editing, view.editor_focus), (None, None), "the editor closes with the reply it was open on"); } @@ -554,6 +560,137 @@ fn text_boxes_fit_their_text() { assert!(long[3] - long[1] > 50.0); } +#[test] +fn stale_comment_state_is_cleared_and_delete_never_hits_non_comment_slots() { + let annot = |index: usize, name: &str| Annotation { + page: 0, + index, + subtype: "Text".into(), + rect: [10.0, 10.0, 30.0, 30.0], + color: None, + author: None, + contents: Some(name.into()), + modified: None, + in_reply_to: None, + parent: None, + grouped: false, + name: Some(name.into()), + state: None, + quads: Vec::new(), + locked: false, + intent: None, + fill_sign: false, + no_rotate: false, + points: Vec::new(), + text_box: None, + popup: None, + }; + // Page 0 has a Widget at index 0 (not in `info.annotations`), and Text comments at 1 and 2. + let info = DocInfo { + pages: vec![PageInfo { width: 300.0, height: 400.0, label: String::new(), crop: [0.0, 0.0, 300.0, 400.0], rotation: 0 }], + annotations: vec![annot(1, "c1"), annot(2, "c2")], + ..Default::default() + }; + let mut view = DocView::new(pdfcraft_engine::DocId(1), &info, crate::canvas::ViewDefaults::default()); + view.comments.selected = Some((0, 2)); + view.comments.open_editor(0, 2, "draft".into()); + view.comments.composer = Some(Composer { page: 0, at: [10.0, 10.0], kind: ComposerKind::Edit(2), text: "body".into(), focus: false }); + view.comments.gesture = Some(Gesture::Move { page: 0, index: 2, from: pos2(0.0, 0.0) }); + + // Moving/editing in place or appending a comment at index 3 preserves (0, 2) selection/editing/gesture. + let mut appended = info.clone(); + appended.annotations.push(annot(3, "c3")); + view.document_changed(&appended); + assert_eq!(view.comments.selected, Some((0, 2))); + assert!(view.comments.editing.is_some()); + assert!(view.comments.composer.is_some()); + assert!(view.comments.gesture.is_some()); + + // Deleting comment 1 shifts comment 2 down to index 1: stale state at (0, 2) and shifted (0, 1) must clear. + let mut shifted = info.clone(); + shifted.annotations = vec![annot(1, "c2")]; + view.comments.selected = Some((0, 1)); + view.comments.open_editor(0, 1, "for c1, not c2".into()); + view.comments.composer = Some(Composer { page: 0, at: [10.0, 10.0], kind: ComposerKind::Edit(1), text: "for c1".into(), focus: false }); + view.comments.gesture = Some(Gesture::Move { page: 0, index: 1, from: pos2(0.0, 0.0) }); + view.document_changed(&shifted); + assert_eq!(view.comments.selected, None); + assert_eq!(view.comments.editing, None); + assert_eq!(view.comments.composer, None); + assert_eq!(view.comments.gesture, None); + + // Pressing Delete when `selected` holds a widget index (0, 0) or stale index (0, 2) emits no edit. + let ctx = egui::Context::default(); + for bad_index in [0, 2] { + view.comments.selected = Some((0, bad_index)); + let mut input = egui::RawInput::default(); + input.events.push(egui::Event::Key { + key: egui::Key::Delete, + physical_key: None, + pressed: true, + repeat: false, + modifiers: egui::Modifiers::NONE, + }); + let mut out = ctx.run_ui(input, |ui| { + let mut tool = QuickTool::Select; + keys(ui.ctx(), &mut view, &shifted, &mut tool, true); + }); + out.textures_delta.clear(); + assert_eq!(view.comments.selected, None); + assert_eq!(view.pending_edit, None, "index {bad_index} must not emit DeleteAnnotation"); + } +} + +/// An unnamed comment has no object name to tell it from the one that slid into its index. +/// Deleting the earlier one still drops selection, editing, the composer and a gesture that +/// pointed at that index. +#[test] +fn an_unnamed_shift_clears_state_for_the_old_index() { + let annot = |index: usize| Annotation { + page: 0, + index, + subtype: "Text".into(), + rect: [10.0, 10.0, 30.0, 30.0], + color: None, + author: None, + contents: None, + modified: None, + in_reply_to: None, + parent: None, + grouped: false, + name: None, + state: None, + quads: Vec::new(), + locked: false, + intent: None, + fill_sign: false, + no_rotate: false, + points: Vec::new(), + text_box: None, + popup: None, + }; + let info = DocInfo { + pages: vec![ + PageInfo { width: 300.0, height: 400.0, label: String::new(), crop: [0.0, 0.0, 300.0, 400.0], rotation: 0 }, + PageInfo { width: 300.0, height: 400.0, label: String::new(), crop: [0.0, 0.0, 300.0, 400.0], rotation: 0 }, + ], + annotations: vec![annot(1), annot(2)], + ..Default::default() + }; + let mut view = DocView::new(pdfcraft_engine::DocId(1), &info, crate::canvas::ViewDefaults::default()); + view.comments.selected = Some((0, 1)); + view.comments.open_editor(0, 1, "the first note".into()); + view.comments.composer = Some(Composer { page: 0, at: [10.0, 10.0], kind: ComposerKind::Edit(1), text: "the first note".into(), focus: false }); + view.comments.gesture = Some(Gesture::Move { page: 0, index: 1, from: pos2(0.0, 0.0) }); + let mut shifted = info.clone(); + shifted.annotations = vec![annot(1)]; + view.document_changed(&shifted); + assert_eq!(view.comments.selected, None); + assert_eq!(view.comments.editing, None); + assert_eq!(view.comments.composer, None); + assert_eq!(view.comments.gesture, None); +} + /// 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.