From a0254022cf1a0e2b73edf4555b313182ea2f6f31 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:47:29 +0800 Subject: [PATCH] M5: selecting from anywhere but a page press forgets the group member a press picked CommentView remembered the group member a press on the page picked, so Delete acts on the member clicked, as Acrobat desktop does. The memory outlived the selection it came from: after choosing the group's card in the Comments panel, a deletion, an undo or a selection made by a tool, Delete still took the member clicked earlier. Every selection now goes through CommentView::select(), which clears the remembered member; pick() (a press on the page: the Select tool's press and click, and the context-menu press) is the only place that sets it. The panel's cards and their Delete, Delete and Backspace, Escape, the context menus (Delete, Flatten comments), deletion and undo (annotation_deleted, history_stepped), the drawing tools, the measuring panel and the control channel use select(). Test: choosing_the_groups_card_makes_delete_act_on_the_card_not_the_member_clicked (ui-egui). With select() leaving the member in place it fails: the Circle clicked earlier goes instead of the Square the card shows (left: ["Square"], right: ["Circle"]). CodeRabbit review on #4358. (cherry picked from commit a24b5ae4e8590be640028f501c9fe87cfdf679d2) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyG234xeYkJfr41E6iP --- crates/ui-egui/src/app/control.rs | 2 +- crates/ui-egui/src/canvas.rs | 2 +- crates/ui-egui/src/editing.rs | 6 ++-- crates/ui-egui/src/panels/comments_panel.rs | 4 +-- crates/ui-egui/src/tools/comments/input.rs | 40 ++++++++++++--------- crates/ui-egui/src/tools/comments/menu.rs | 4 +-- crates/ui-egui/src/tools/comments/state.rs | 20 +++++++++-- crates/ui-egui/src/tools/measure_ui.rs | 2 +- crates/ui-egui/tests/it/comments.rs | 23 ++++++++++++ 9 files changed, 75 insertions(+), 28 deletions(-) diff --git a/crates/ui-egui/src/app/control.rs b/crates/ui-egui/src/app/control.rs index e4078efd13..5ab24328f7 100644 --- a/crates/ui-egui/src/app/control.rs +++ b/crates/ui-egui/src/app/control.rs @@ -383,7 +383,7 @@ impl PdfCraftApp { let (p, i) = value.split_once(':').ok_or("comment: PAGE:INDEX")?; let (p, i): (usize, usize) = (p.trim().parse().map_err(|_| "comment: PAGE:INDEX")?, i.trim().parse().map_err(|_| "comment: PAGE:INDEX")?); - v.comments.selected = Some((p.saturating_sub(1), i.saturating_sub(1))); + v.comments.select(Some((p.saturating_sub(1), i.saturating_sub(1)))); v.comments.reveal = true; } // `--combine-view list`: Combine files as thumbnails (grid) or as the table (list). diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index a220644421..b6c9a8aec0 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -3987,7 +3987,7 @@ fn apply_frame_requests(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui, Some(crate::comments::Composer { page, at, kind: crate::comments::ComposerKind::Note, text: String::new(), focus: true }); } Some(PageMenuAction::FlattenComments) => { - app.views[index].comments.selected = None; + app.views[index].comments.select(None); app.views[index].pending_edit = Some(pdfcraft_engine::Edit::Flatten { comments: true, fields: false }); } Some(PageMenuAction::AddBookmark) => { diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index b810e2abe5..7d73d004cf 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -58,12 +58,12 @@ fn select_after_edit( crate::QuickTool::Comment(crate::comments::CommentTool::Ink | crate::comments::CommentTool::FreehandHighlight) ) => { - view.comments.selected = None; + view.comments.select(None); } Edit::AddAnnotation(a) => { // Select the new comment (appended last among the page's comments). let newest = info.annotations.iter().filter(|x| x.page == a.page && !x.is_reply()).map(|x| x.index).max(); - view.comments.selected = newest.map(|n| (a.page, n)); + view.comments.select(newest.map(|n| (a.page, n))); view.comments.reveal = true; // A new highlight opens its note for typing straight away, as in Acrobat, // unless a note typed into another card is still unsaved. @@ -697,7 +697,7 @@ impl PdfCraftApp { .session .get(view.id) .and_then(|d| d.info.annotations.iter().filter(|a| a.page == page && !a.is_reply()).map(|a| a.index).max()); - view.comments.selected = newest.map(|index| (page, index)); + view.comments.select(newest.map(|index| (page, index))); view.comments.reveal = true; self.quick_tool = crate::QuickTool::Select; } diff --git a/crates/ui-egui/src/panels/comments_panel.rs b/crates/ui-egui/src/panels/comments_panel.rs index cae1afc4eb..cc3b9c2494 100644 --- a/crates/ui-egui/src/panels/comments_panel.rs +++ b/crates/ui-egui/src/panels/comments_panel.rs @@ -571,7 +571,7 @@ fn card(ui: &mut egui::Ui, view: &mut DocView, context: CardContext<'_>) -> Opti .on_hover_cursor(egui::CursorIcon::PointingHand) .on_hover_text(crate::i18n::fmt(tl!("{kind} — click to show it on the page"), &[("kind", tl!(kind_label(a)))])); if click.clicked() { - view.comments.selected = Some(key); + view.comments.select(Some(key)); view.comments.reply.clear(); *nav = Some(Nav::Flash(a.page, a.shown_rect(rotation))); } @@ -622,7 +622,7 @@ fn card(ui: &mut egui::Ui, view: &mut DocView, context: CardContext<'_>) -> Opti ui.separator(); // A locked reply can't be deleted, so it keeps the comment it answers too. if ui.add_enabled(doc.annotation_deletable(a.page, a.index), egui::Button::new(tl!("Delete"))).clicked() { - view.comments.selected = None; + view.comments.select(None); edit = Some(Edit::DeleteAnnotation { page: a.page, index: a.index }); ui.close(); } diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index 5d7c870154..305b537d82 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -32,8 +32,10 @@ pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, { cv.context_at = Some((cx.page, cx.to_user(p))); // On a group subordinate, its group (the primary), so the menu offers the comment's actions. - cv.selected = cx.click_hit(p).map(|a| (cx.page, a.index)); - cv.member = cx.picked(p, cv.selected); + match cx.click_hit(p) { + Some(a) => cv.pick(cx, p, a), + None => cv.select(None), + } } let gestures = Gestures { ui, resp, cx, pointer, origin, page_rect, pressed_here, over_page }; match cx.tool { @@ -209,7 +211,7 @@ fn select_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, view: &mu consumed = true; } CommentPart::Body if !is_markup(&a.subtype) => { - cv.selected = Some((cx.page, a.index)); + cv.pick(cx, o, a); cv.reveal = true; if cx.allowed { cv.gesture = Some(Gesture::Move { page: cx.page, index: a.index, from: o }); @@ -264,12 +266,14 @@ fn select_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, view: &mu // leaves comments unselected. match owner { Some((a, _)) => { - cv.selected = Some((cx.page, a.index)); - cv.member = pointer.and_then(|p| cx.picked(p, cv.selected)); + match pointer { + Some(p) => cv.pick(cx, p, a), + None => cv.select(Some((cx.page, a.index))), + } cv.reveal = true; consumed = true; } - None => cv.selected = None, + None => cv.select(None), } } if resp.double_clicked() @@ -344,7 +348,7 @@ impl Gestures<'_> { && let Some(o) = origin { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![cx.to_user(o)] }); - cv.selected = None; + cv.select(None); } if let Some(Gesture::Draw { page, tool, points }) = cv.gesture.as_mut() && *page == cx.page @@ -384,7 +388,7 @@ impl Gestures<'_> { && let Some(p) = pointer && let Some(shape) = drawn_shape(tool, &[cx.to_user(p)]) { - cv.selected = None; + cv.select(None); let now = ui.input(|i| i.time); cv.settle(now, cx.page, tool, &[cx.to_user(p)]); view.pending_edit = Some(new_comment(cx, tool, shape, String::new())); @@ -435,7 +439,7 @@ impl Gestures<'_> { } _ => { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![u] }); - cv.selected = None; + cv.select(None); } } } @@ -467,7 +471,7 @@ impl Gestures<'_> { && let Some(o) = origin { cv.gesture = Some(Gesture::Draw { page: cx.page, tool: CommentTool::Callout, points: vec![cx.to_user(o)] }); - cv.selected = None; + cv.select(None); } if let Some(Gesture::Draw { page, points, .. }) = cv.gesture.as_mut() && *page == cx.page @@ -504,7 +508,7 @@ impl Gestures<'_> { && let Some(p) = pointer { cv.attach_at = Some((cx.page, cx.to_user(p))); - cv.selected = None; + cv.select(None); } true } @@ -526,7 +530,7 @@ impl Gestures<'_> { let snapped = cx.info.pages.get(cx.page).zip(caret_text).and_then(|(page, t)| snapped_caret(page, t, vx, vy)); let (at, size) = snapped.unwrap_or_else(|| (cx.to_user(p), CARET_SIZE)); cv.composer = Some(Composer { page: cx.page, at, kind: ComposerKind::Caret { size }, text: String::new(), focus: true }); - cv.selected = None; + cv.select(None); } true } @@ -548,7 +552,7 @@ impl Gestures<'_> { && let Some(o) = origin { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![cx.to_user(o)] }); - cv.selected = None; + cv.select(None); } if let Some(Gesture::Draw { page, points, .. }) = cv.gesture.as_mut() && *page == cx.page @@ -578,7 +582,7 @@ impl Gestures<'_> { { let kind = if tool == CommentTool::Note { ComposerKind::Note } else { ComposerKind::TextBox { size: None } }; cv.composer = Some(Composer { page: cx.page, at: cx.to_user(p), kind, text: String::new(), focus: true }); - cv.selected = None; + cv.select(None); } true } @@ -595,7 +599,11 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool let cv = &mut view.comments; if ctx.input(|i| i.key_pressed(egui::Key::Escape)) { // One step back per press: cancel the gesture, then deselect, then drop the tool. - let cancelled = cv.gesture.take().is_some() || cv.selected.take().is_some(); + let cancelled = cv.gesture.take().is_some() || { + let deselected = cv.selected.is_some(); + cv.select(None); + deselected + }; if !cancelled && matches!(tool, QuickTool::Comment(_)) { *tool = QuickTool::Select; } @@ -604,7 +612,7 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, tool: &mut QuickTool && let Some((page, index)) = cv.target() && ctx.input(|i| i.key_pressed(egui::Key::Delete) || i.key_pressed(egui::Key::Backspace)) { - cv.selected = None; + cv.select(None); view.pending_edit = Some(Edit::DeleteAnnotation { page, index }); } } diff --git a/crates/ui-egui/src/tools/comments/menu.rs b/crates/ui-egui/src/tools/comments/menu.rs index af2b792f1a..708b31cad4 100644 --- a/crates/ui-egui/src/tools/comments/menu.rs +++ b/crates/ui-egui/src/tools/comments/menu.rs @@ -96,7 +96,7 @@ pub(crate) fn context_menu( // subordinate, Delete takes just it, as Acrobat's does. let (dpage, dindex) = view.comments.target().unwrap_or((page, index)); if ui.add_enabled(allowed && deletable(dpage, dindex), egui::Button::new(tl!("Delete"))).clicked() { - view.comments.selected = None; + view.comments.select(None); action = Some(CanvasAction::Edit(Box::new(Edit::DeleteAnnotation { page: dpage, index: dindex }))); ui.close(); } @@ -127,7 +127,7 @@ pub(crate) fn context_menu( if !info.annotations.is_empty() { ui.separator(); if ui.add_enabled(can_modify, egui::Button::new(tl!("Flatten comments"))).clicked() { - view.comments.selected = None; + view.comments.select(None); action = Some(CanvasAction::Edit(Box::new(Edit::Flatten { comments: true, fields: false }))); ui.close(); } diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index b987ab5ba0..149357dfbe 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -202,6 +202,22 @@ impl CommentView { } } + /// Select `key`, or nothing, from anywhere but a press on the page ([`Self::pick`]): the group + /// member a press picked goes, so Delete and the arrow keys act on what is selected now, not + /// on a member remembered from before (after the Comments panel, a deletion or undo). + pub(crate) fn select(&mut self, key: Option<(usize, usize)>) { + self.selected = key; + self.member = None; + } + + /// Select `a`, the comment a press at `p` on this page selects ([`PageCx::click_hit`]), + /// remembering the group subordinate under `p` when `a` is its group's primary + /// ([`PageCx::picked`]). The only place that sets [`Self::member`]. + pub(crate) fn pick(&mut self, cx: &PageCx<'_>, p: Pos2, a: &Annotation) { + self.selected = Some((cx.page, a.index)); + self.member = cx.picked(p, self.selected); + } + /// Takes back the last point of a polygon, cloud or connected lines being drawn; the first /// point's removal cancels the drawing. Returns whether there was such a drawing. pub fn undo_point(&mut self) -> bool { @@ -240,7 +256,7 @@ impl CommentView { /// were on, where it now is (deleting a reply keeps its comment selected), and close when it /// went too. pub(crate) fn annotation_deleted(&mut self, before: &[Annotation], after: &[Annotation], gone: (usize, usize)) { - self.selected = self.selected.and_then(|k| after_delete(before, after, gone, k)); + self.select(self.selected.and_then(|k| after_delete(before, after, gone, k))); self.editing = self.editing.take().and_then(|(p, i, text)| after_delete(before, after, gone, (p, i)).map(|(p, i)| (p, i, text))); if self.editing.is_none() { self.editor_focus = None; @@ -252,7 +268,7 @@ impl CommentView { /// editor close, its unsaved text with it, rather than Save writing that text over whatever /// took its place. pub(crate) fn history_stepped(&mut self) { - self.selected = None; + self.select(None); self.editing = None; self.editor_focus = None; } diff --git a/crates/ui-egui/src/tools/measure_ui.rs b/crates/ui-egui/src/tools/measure_ui.rs index 5008464ad9..60481f65da 100644 --- a/crates/ui-egui/src/tools/measure_ui.rs +++ b/crates/ui-egui/src/tools/measure_ui.rs @@ -562,7 +562,7 @@ fn panel_body(app: &mut PdfCraftApp, ui: &mut egui::Ui, _t: &Tokens) { } if let Some((page, annotation, reading, scale)) = select { app.views[index].current = page; - app.views[index].comments.selected = Some((page, annotation)); + app.views[index].comments.select(Some((page, annotation))); app.views[index].measure.reading = Some(reading); app.views[index].measure.scale = Some(scale); app.quick_tool = QuickTool::Select; diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 3b59436784..879c155829 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -3108,6 +3108,29 @@ fn delete_deletes_the_group_member_clicked() { assert_eq!(shapes(&h), [("Circle".to_string(), true)], "the Square goes, the Circle stays as a comment of its own"); } +/// CodeRabbit on #4358: the member a click on the page picked is forgotten once the selection is +/// made anywhere else. After clicking the Circle, choosing the group's card in the Comments panel +/// selects the comment the card shows, the Square, so Delete deletes the Square, not the Circle +/// the page click remembered. +#[test] +fn choosing_the_groups_card_makes_delete_act_on_the_card_not_the_member_clicked() { + let shapes = |h: &Harness<'static, PdfCraftApp>| -> Vec { + let mut out: Vec = comments(h).iter().filter(|a| a.subtype != "Text").map(|a| a.subtype.clone()).collect(); + out.sort(); + out + }; + let mut h = harness_with_bytes(GROUP_FIXTURE.to_vec(), |app| app.set_option("panel", "comments").unwrap()); + click_pt(&mut h, (220.0, 150.0)); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the Circle's click selects the group"); + // The selected card's header row, with its author. + h.get_by_label("Alice").click(); + h.run_steps(3); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the card selects the same group"); + h.key_press(egui::Key::Delete); + h.run_steps(3); + assert_eq!(shapes(&h), ["Circle".to_string()], "the card's comment, the Square, goes; the Circle stays"); +} + /// `n` pages with one comment each, "Card 1" to "Card n". fn one_comment_per_page(n: usize) -> Vec { let mut objs = vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string()];