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 a24b5ae4e8)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pq7HyG234xeYkJfr41E6iP

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).

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) => {

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;
             }

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();
             }

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 });
     }
 }

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();
         }

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;
     }

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;

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<String> {
+        let mut out: Vec<String> = 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<u8> {
     let mut objs = vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string()];