From a24b5ae4e8590be640028f501c9fe87cfdf679d2 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 and the arrow keys act 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) stays the only place that sets it. The panel's cards, deletion and undo, the context menus, 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"]). Review on #5517. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyCfwzyRXj4VfQAVVdv --- crates/ui-egui/src/app/control.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 | 22 ++++++++++---------- crates/ui-egui/src/tools/comments/menu.rs | 4 ++-- crates/ui-egui/src/tools/comments/state.rs | 12 +++++++++-- crates/ui-egui/src/tools/measure_ui.rs | 2 +- crates/ui-egui/tests/it/comments.rs | 23 +++++++++++++++++++++ 8 files changed, 53 insertions(+), 22 deletions(-) diff --git a/crates/ui-egui/src/app/control.rs b/crates/ui-egui/src/app/control.rs index 79bd2fa258..0abc85256b 100644 --- a/crates/ui-egui/src/app/control.rs +++ b/crates/ui-egui/src/app/control.rs @@ -355,7 +355,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/editing.rs b/crates/ui-egui/src/editing.rs index 8def569f58..bf85e3ddc2 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -55,12 +55,12 @@ fn select_after_edit( if matches!(a.shape, pdfcraft_engine::Shape::Ink { .. }) && quick_tool == crate::QuickTool::Comment(crate::comments::CommentTool::Ink) => { - 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. @@ -647,7 +647,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 58506efcda..aab4a891f7 100644 --- a/crates/ui-egui/src/panels/comments_panel.rs +++ b/crates/ui-egui/src/panels/comments_panel.rs @@ -497,7 +497,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))); } @@ -548,7 +548,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 969122105d..8fef68656b 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -34,7 +34,7 @@ pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, // On a group subordinate, its group (the primary), so the menu offers the comment's actions. match cx.click_hit(p) { Some(a) => cv.pick(cx, a), - None => cv.selected = None, + None => cv.select(None), } } let gestures = Gestures { ui, resp, cx, pointer, origin, page_rect, pressed_here, over_page }; @@ -275,7 +275,7 @@ fn select_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, view: &mu cv.reveal = true; consumed = true; } - None => cv.selected = None, + None => cv.select(None), } } if resp.double_clicked() @@ -353,7 +353,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 @@ -393,7 +393,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())); @@ -444,7 +444,7 @@ impl Gestures<'_> { } _ => { cv.gesture = Some(Gesture::Draw { page: cx.page, tool, points: vec![u] }); - cv.selected = None; + cv.select(None); } } } @@ -476,7 +476,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 @@ -513,7 +513,7 @@ impl Gestures<'_> { && let Some(p) = pointer { cv.attach_at = Some((cx.page, cx.to_user(p))); - cv.selected = None; + cv.select(None); } true } @@ -535,7 +535,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 } @@ -557,7 +557,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 @@ -587,7 +587,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 } @@ -613,7 +613,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 77f0a33870..361e26d2af 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 36a89c59ab..6f6b610685 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/crates/ui-egui/src/tools/comments/state.rs @@ -199,6 +199,14 @@ 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 what a press on this page's `a` selects ([`PageCx::selects`]), remembering `a` when /// it is a group subordinate. pub(crate) fn pick(&mut self, cx: &PageCx<'_>, a: &Annotation) { @@ -245,7 +253,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; @@ -257,7 +265,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 8173052ac7..b7cc4aab31 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"); } +/// #5517 review: 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"); +} + /// The Square's and the Circle's `/Rect` in [`GROUP_FIXTURE`] (or one built like it). fn group_rects(h: &Harness<'static, PdfCraftApp>) -> (Vec, Vec) { let all = comments(h);