Move selected comments together with arrow keys (#7165)

* Move selected comments together with arrow keys

* Test keyboard nudging of selected lines

* test(ui): cover locked and duplicate nudge selections
This commit is contained in:
FredApps authored and GitHub committed 2026-11-14 22:25:50 +01:00
1 parent eead454105
commit 54b91598a3
2 files changed
+97 -2

No files matched your search

+20 -2
View File
@@ -788,7 +788,7 @@ pub(crate) fn delete_selected(cv: &mut CommentView, info: &DocInfo) -> Option<Ed
}
}
/// The arrow keys move the selected comment a screen pixel; with ⌘/Ctrl or Shift, ten. Up is up
/// The arrow keys move selected comments a screen pixel; with ⌘/Ctrl or Shift, ten. Up is up
/// on screen whatever the zoom, the page's rotation and the view's; the keys are taken before
/// the view scrolls or turns the page with them. Each press is one undo step. Text markup stays
/// with its text, and a locked comment where it is.
@@ -838,7 +838,25 @@ pub(crate) fn nudge_keys(ctx: &egui::Context, keys: PageKeys, view: &mut DocView
let (a, b) = (user(xf.rect.center()), user(xf.rect.center() + step));
let (dx, dy) = (b[0] - a[0], b[1] - a[1]);
if dx.is_finite() && dy.is_finite() && (dx != 0.0 || dy != 0.0) {
view.pending_edit = Some(Edit::MoveAnnotation { page, index, dx, dy });
// Match a pointer drag: move the eligible selection on this page as one undo step.
// Text markup and locked comments cannot be moved and would reject a batch.
let mut indices = vec![index];
for &(other_page, other_index) in &view.comments.also {
if other_page == page
&& !indices.contains(&other_index)
&& info.annotations.iter().any(|a| a.page == page && a.index == other_index && !a.is_reply() && !on_text(a) && !a.locked)
{
indices.push(other_index);
}
}
view.pending_edit = Some(if indices.len() == 1 {
Edit::MoveAnnotation { page, index, dx, dy }
} else {
Edit::Batch {
label: tl!("Move comments").to_owned(),
edits: indices.into_iter().map(|index| Edit::MoveAnnotation { page, index, dx, dy }).collect(),
}
});
}
}
+77
View File
@@ -1127,6 +1127,58 @@ fn arrow_keys_nudge_the_selected_comment() {
assert_eq!(h.state().views[0].current, 1, "and the view goes to the next page");
}
#[test]
fn arrow_keys_nudge_all_selected_comments_as_one_undo_step() {
use egui::Key;
let mut h = harness(|app| app.set_option("quick", "square").unwrap());
drag_pt(&mut h, (30.0, 110.0), (80.0, 70.0));
drag_pt(&mut h, (130.0, 110.0), (180.0, 70.0));
h.state_mut().set_option("quick", "select").unwrap();
let before: Vec<_> = comments(&h).iter().map(|a| a.rect).collect();
assert_eq!(before.len(), 2);
h.state_mut().views[0].comments.selected = Some((0, 0));
h.state_mut().views[0].comments.also = vec![(0, 1)];
h.key_press(Key::ArrowRight);
h.run_steps(3);
let moved: Vec<_> = comments(&h).iter().map(|a| a.rect).collect();
for (old, new) in before.iter().zip(&moved) {
assert!(new[0] > old[0], "each selected comment moves: {before:?} -> {moved:?}");
}
h.state_mut().undo();
h.run_steps(2);
let undone: Vec<_> = comments(&h).iter().map(|a| a.rect).collect();
assert_eq!(undone, before, "one undo restores the whole selection");
}
#[test]
fn arrow_nudge_skips_locked_comments_and_duplicate_selection_entries() {
let mut h = harness(|app| app.set_option("quick", "square").unwrap());
for (a, b) in [((20.0, 20.0), (60.0, 60.0)), ((100.0, 20.0), (140.0, 60.0)), ((180.0, 20.0), (220.0, 60.0))] {
drag_pt(&mut h, a, b);
}
h.state_mut().set_option("quick", "select").unwrap();
assert!(h.state_mut().apply_edit(pdfcraft_engine::Edit::LockAnnotation { page: 0, index: 1, locked: true }));
h.run_steps(2);
let before = comments(&h);
h.state_mut().views[0].comments.selected = Some((0, 0));
h.state_mut().views[0].comments.also = vec![(0, 1), (0, 2), (0, 2), (0, 0)];
h.key_press(egui::Key::ArrowRight);
h.run_steps(3);
let after = comments(&h);
let left = |list: &[Annotation], i: usize| list.iter().find(|a| a.index == i).expect("comment still exists").rect[0];
let primary_delta = left(&after, 0) - left(&before, 0);
let secondary_delta = left(&after, 2) - left(&before, 2);
assert!(primary_delta > 0.0, "primary comment moves");
assert!((secondary_delta - primary_delta).abs() < 0.01, "duplicate entries must not move the second comment twice");
assert_eq!(left(&after, 1), left(&before, 1), "locked comment stays in place");
h.state_mut().undo();
h.run_steps(2);
let undone = comments(&h);
for i in 0..3 {
assert_eq!(left(&undone, i), left(&before, i), "one undo restores comment {i}");
}
}
/// #3632: with a comment selected, Alt+← / Alt+→ still go back and forward through the view
/// history instead of nudging it (egui's key patterns let an extra Alt through).
#[test]
@@ -5848,3 +5900,28 @@ fn properties_dialog_makes_the_text_box_font_size_default() {
assert_eq!(font_size_of_last(&fresh).font_size, Some(size), "{quick}: the default font size persisted");
}
}
#[test]
fn ctrl_selected_lines_nudge_together_and_undo() {
let mut h = harness(|app| app.set_option("quick", "line").unwrap());
for y in [40.0, 80.0, 120.0] {
drag_pt(&mut h, (40.0, y), (160.0, y));
}
assert_eq!(comments(&h).len(), 3);
h.state_mut().set_option("quick", "select").unwrap();
h.run_steps(2);
click_pt(&mut h, (100.0, 40.0));
click_pt_with(&mut h, (100.0, 80.0), egui::Modifiers::COMMAND);
click_pt_with(&mut h, (100.0, 120.0), egui::Modifiers::COMMAND);
assert_eq!(h.state().views[0].comments.selection().len(), 3);
let before: Vec<_> = comments(&h).iter().map(|a| a.rect).collect();
h.key_press(egui::Key::ArrowRight);
h.run_steps(3);
let moved: Vec<_> = comments(&h).iter().map(|a| a.rect).collect();
for (old, new) in before.iter().zip(&moved) {
assert!(new[0] > old[0], "each selected line moves: {before:?} -> {moved:?}");
}
h.state_mut().undo();
h.run_steps(2);
assert_eq!(comments(&h).iter().map(|a| a.rect).collect::<Vec<_>>(), before);
}