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