fix(ui): a grouped double-click, held presses and blocked Add text, from review

CodeRabbit's findings on #6243, each checked against the code:

- A double-click on the objects selected together took its target from
  the hits at the release, so a release just off the paragraph pressed
  opened nothing. The router keeps what lay on top at the press with the
  objects aside (`under_objects`), as every owner acts where its press was.
- A drag first seen on another page left the picked picture picked: that
  page is told `Elsewhere`, and the picture's test no longer ran there.
- Delete with a button held removed the item, and the press held then named
  the next one. Delete waits for the button to come up, for pictures and
  added content.
- A page just off the screen is still run for what lies near it, so a
  picture's keys acted there unseen. They ask whether the page is on screen.
- Add text's drags end while its input is blocked, as Edit text's do.
- With the page gaps hidden, a Shift-click on the row two pages share
  selected on the first page and the second cleared it. A press another
  page holds leaves the selection alone.

Tests, each seen failing with its fix reverted:
`a_double_click_on_a_grouped_paragraph_released_just_off_it_opens_it`,
`a_drag_first_seen_on_the_next_page_lets_the_picked_picture_go`,
`delete_waits_for_the_button_to_come_up` (each half),
`a_selected_pictures_keys_wait_for_its_page_to_be_on_screen` (its
continuous case), `a_paragraph_drag_ends_when_the_window_loses_focus` (Add
text), `a_shift_click_where_two_pages_touch_keeps_what_it_selected`,
`a_press_counts_the_modifiers_of_its_own_event`.
`add_text_keeps_a_changed_paragraph_and_a_draft_when_it_moves_on` pins that a
click on the page applies the open paragraph before a new box starts, so
neither edit can take the other's place; seen failing with the draft's edit
dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
alehelandClaude Opus 5.5 committed 2026-11-05 08:51:44 +01:00
1 parent 723667f29d
commit fdc9f646f9
9 files changed
+260 -38

No files matched your search

+2 -2
View File
@@ -595,9 +595,9 @@ impl eframe::App for PdfCraftApp {
view.objects.set_input_blocked(shut);
if shut {
view.auto_scroll.cancel();
// Edit text ignores input while it is blocked: its drags end rather than wait, to
// Edit text and Add text's drags end while their input is blocked rather than wait, to
// land wherever the pointer is once it opens again.
if self.quick_tool == QuickTool::EditText {
if matches!(self.quick_tool, QuickTool::EditText | QuickTool::AddText) {
view.cancel_edit_text_gestures();
}
}
+9 -7
View File
@@ -129,13 +129,12 @@ pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut
let wants_under = editing || resp.clicked() || resp.secondary_clicked() || matches!(tool, QuickTool::Fill(_)) && !ptr.any_down;
let under = pointer.filter(|p| wants_under && xf.rect.contains(*p)).map(|p| hits.at(view, p, None)).unwrap_or_default();
let hovered = top(under.iter().cloned(), tool);
// A double-click on the objects selected together is the object's under it: the double-click
// leaves the group (`object_ui`), and added text opens for typing, a picture or a paragraph is
// the one taken (the user's ruling).
// A double-click on the objects selected together is the object's its press was on: the
// double-click leaves the group (`object_ui`), and added text opens for typing, a picture or a
// paragraph is the one taken (the user's ruling).
let press = match press {
PagePress::Mine(Owner::Objects { part: crate::page_router::ObjectsPart::Selection }) if resp.double_clicked() => {
top(under.iter().filter(|h| !matches!(h.owner, Owner::Objects { .. })).cloned(), tool)
.map_or(PagePress::Free, |h| PagePress::Mine(h.owner))
view.router.under_objects().map_or(PagePress::Free, PagePress::Mine)
}
press => press,
};
@@ -825,8 +824,11 @@ pub(super) fn page_overlays(ui: &mut egui::Ui, view: &mut DocView, frame: &mut P
let tool_cx = PageToolContext { ui, resp, xf: &xf, ptr, page: i, doc, info };
let pcx = comments::PageCx { page: i, xf: &xf, ptr, info, tool, prefs, allowed, hidden: comments_hidden, redact_mark };
// A press nobody holds picks the picture under it: a link's click follows the link only (ruling
// 5), though its drag or double-click still lets the picked picture go.
if tool == QuickTool::Select && matches!(input.press, PagePress::Free | PagePress::Mine(Owner::Link { .. })) && !input.consumed {
// 5), though its drag or double-click still lets the picked picture go, as does a drag the page
// it is over first sees, whichever page it began on.
let pick_sees =
matches!(input.press, PagePress::Free | PagePress::Mine(Owner::Link { .. })) || input.press == PagePress::Elsewhere && ptr.drag_started;
if tool == QuickTool::Select && pick_sees && !input.consumed {
crate::image_pick::page_input(resp, &xf, i, info, doc, ptr, input.press == PagePress::Free, view);
}
crate::image_pick::drop_stale(doc, view);
+11 -1
View File
@@ -175,6 +175,9 @@ impl PagePress {
struct Capture {
pub owner: Owner,
pub declines_drag: bool,
/// What lay on top at the press with the objects selected together aside: a double-click on
/// them reaches it.
under_objects: Option<Owner>,
}
/// The press the pages hold, from its frame until the button is up.
@@ -202,7 +205,9 @@ impl PageRouter {
if self.began != Some(None) {
return;
}
self.capture = top(hits, tool).map(|h| Capture { owner: h.owner, declines_drag: h.declines_drag });
let hits: Vec<Hit> = hits.into_iter().collect();
let under_objects = top(hits.iter().filter(|h| !matches!(h.owner, Owner::Objects { .. })).cloned(), tool).map(|h| h.owner);
self.capture = top(hits, tool).map(|h| Capture { owner: h.owner, declines_drag: h.declines_drag, under_objects });
self.began = Some(Some(page));
}
@@ -216,6 +221,11 @@ impl PageRouter {
}
}
/// What lay on top at the press with the objects selected together aside.
pub(in crate::canvas) fn under_objects(&self) -> Option<Owner> {
self.capture.as_ref().and_then(|c| c.under_objects)
}
/// Whether the press is held by someone who keeps its drag: then the page's text is not
/// selected by it.
pub(in crate::canvas) fn keeps_drag(&self) -> bool {
+8 -1
View File
@@ -110,6 +110,8 @@ pub struct ContentView {
grab: Option<(usize, Grab, Pos2)>,
/// Where the press nobody holds began, on which page: Add text's click starts its new box there.
pressed_at: Option<(usize, Pos2)>,
/// A button is down on the pages this frame (for [`keys`], which runs after them).
held: bool,
undrawable: UndrawableMemo,
layout: Option<(AddedText, f32, f64, [f64; 4])>,
}
@@ -404,6 +406,7 @@ pub(crate) struct ContentCx<'a> {
pub(crate) fn page_input(cx: PageToolContext<'_>, ccx: ContentCx<'_>, view: &mut DocView) -> bool {
let PageToolContext { resp, xf, ptr, page, info, .. } = cx;
let ContentCx { added, press, gate, picks_up, adding_text, edit_on_click, style } = ccx;
view.content.held = ptr.any_down;
if !gate {
return false;
}
@@ -634,7 +637,11 @@ pub(crate) fn keys(ctx: &egui::Context, view: &mut DocView, image_controller_act
return;
}
let (del, esc) = ctx.input(|i| (i.key_pressed(egui::Key::Delete) || i.key_pressed(egui::Key::Backspace), i.key_pressed(egui::Key::Escape)));
if del && let Some((page, index)) = view.content.selected.take() {
// Not while a button is down: the press held would then name another item.
if del
&& !view.content.held
&& let Some((page, index)) = view.content.selected.take()
{
view.pending_edit = Some(Edit::DeleteContent { page, index });
} else if esc {
view.content.selected = None;
@@ -467,14 +467,15 @@ fn apply_crop_mode(view: &mut DocView, image: &pdfcraft_engine::PageImage) {
}
}
/// The keys of the picture selected on `page`, a page being drawn, so the keys never reach a picture
/// off the screen, and the page grid and the Pages panel have had theirs. Delete or Backspace removes
/// it, wherever on the pages the pointer is, unless it is being dragged or cropped. In crop mode
/// The keys of the picture selected on `page`, a page being drawn, so the page grid and the Pages
/// panel have had theirs; nothing while the page is off the screen, as pages near it are drawn too.
/// Delete or Backspace removes it, wherever on the pages the pointer is, unless it is being cropped or
/// a button is down: the press held would then name another picture. In crop mode
/// Enter applies the crop and Escape leaves it, though not while a button is down (`held`): Escape
/// then cancels the press the pages hold, if any (`canvas::cancel_on_escape`), and crop mode stays.
/// Nothing while a text field has the keyboard.
pub(crate) fn picture_keys(ctx: &egui::Context, view: &mut DocView, page: usize, images: &[pdfcraft_engine::PageImage], held: bool) {
if ctx.egui_wants_keyboard_input() {
if ctx.egui_wants_keyboard_input() || view.page_screen_rect(page).is_none() {
return;
}
let Some(s) = view.image_selection.as_ref().filter(|s| s.page == page) else { return };
@@ -492,7 +493,7 @@ pub(crate) fn picture_keys(ctx: &egui::Context, view: &mut DocView, page: usize,
{
s.crop = None;
}
} else if !s.dragging() && !s.turning && (key(egui::Key::Delete) || key(egui::Key::Backspace)) && image.is_some() {
} else if !held && !s.dragging() && !s.turning && (key(egui::Key::Delete) || key(egui::Key::Backspace)) && image.is_some() {
// The key is the picture's: added content's own Delete runs later this frame.
ctx.input_mut(|i| {
i.consume_key(egui::Modifiers::NONE, egui::Key::Delete);
+3 -1
View File
@@ -340,7 +340,9 @@ fn input(s: &mut ObjectSelection, cx: PageToolContext<'_>, press: PagePress, vie
let Some(p) = pointer.filter(|p| xf.rect.contains(*p) && ui.clip_rect().contains(*p) && resp.rect.contains(*p)) else {
return s.page == Some(page) && !s.selected.is_empty();
};
if !pressed {
// Nor a press another page holds, on the row of pixels pages share with the gaps hidden: it may
// have just selected something there.
if !pressed || press == PagePress::Elsewhere {
return s.page == Some(page) && !s.selected.is_empty();
}
let Ok(objects) = s.inventory(doc, page) else { return false };
+153 -21
View File
@@ -7485,6 +7485,7 @@ fn add_text_moves_no_paragraph_while_it_holds_a_draft_it_cant_draw() {
h.event(egui::Event::Text("Hello 世界".into()));
h.run_steps(2);
assert_eq!(h.state().views[0].content.draft.as_ref().map(|d| d.text.as_str()), Some("Hello 世界"), "the draft holds the text");
let rect = h.state().views[0].content.draft.as_ref().map(|d| d.rect).unwrap();
let on_text = point_on(&h, (block.rect[0] + block.rect[2]) / 2.0, (block.rect[1] + block.rect[3]) / 2.0);
h.hover_at(on_text);
h.run_steps(1);
@@ -7497,6 +7498,8 @@ fn add_text_moves_no_paragraph_while_it_holds_a_draft_it_cant_draw() {
h.drop_at(on_text + egui::vec2(40.0, 0.0));
h.run_steps(3);
assert_eq!(h.state().session.get(id).unwrap().text_blocks(0)[0].rect, block.rect, "the paragraph stays");
let draft = h.state().views[0].content.draft.as_ref().map(|d| (d.text.clone(), d.rect));
assert_eq!(draft, Some(("Hello 世界".to_string(), rect)), "and the draft stays where it is typed");
}
/// With Edit text, a right-click on a comment over a picture opens the comment's menu, and the
@@ -7554,6 +7557,48 @@ fn a_crop_grip_takes_a_press_within_its_reach() {
assert_ne!(keep(&h), before, "the grip takes the drag");
}
/// With Add text, a click on another paragraph keeps both what was typed in the open paragraph and
/// the text being added: neither edit takes the other's place.
#[test]
fn add_text_keeps_a_changed_paragraph_and_a_draft_when_it_moves_on() {
let mut h = picture_pages(false);
h.state_mut().views[0].image_selection = None;
assert!(h.state_mut().execute("edit.text"));
h.run_steps(3);
let id = h.state().views[0].id;
let blocks = h.state().session.get(id).unwrap().text_blocks(0);
assert!(blocks.len() >= 2, "two paragraphs: {blocks:?}");
let centre = |r: [f64; 4]| ((r[0] + r[2]) / 2.0, (r[1] + r[3]) / 2.0);
let (ax, ay) = centre(blocks[0].rect);
let (bx, by) = centre(blocks[1].rect);
let a = page_at(&h, 0, ax as f32, ay as f32);
let b = page_at(&h, 0, bx as f32, by as f32);
slipping_click(&mut h, a, a);
assert!(h.state().views[0].line_editor.is_some(), "the first paragraph is open");
h.event(egui::Event::Text("X".into()));
h.run_steps(3);
// A click on the page's empty part, below the picture, starts a box.
let blank = page_at(&h, 0, 180.0, 20.0);
slipping_click(&mut h, blank, blank);
assert!(h.state().views[0].line_editor.is_none(), "that click applied the paragraph");
h.event(egui::Event::Text("Added words".into()));
h.run_steps(3);
slipping_click(&mut h, b, b);
h.run_steps(4);
let doc = h.state().session.get(id).unwrap();
let texts: Vec<String> = doc.text_blocks(0).iter().map(|b| b.text.clone()).collect();
let added: Vec<String> = doc
.added()
.iter()
.filter_map(|a| match &a.content {
pdfcraft_engine::AddedContent::Text(t) => Some(t.text.clone()),
_ => None,
})
.collect();
assert!(added.iter().any(|t| t == "Added words"), "the added text is kept: {added:?}");
assert!(texts.iter().any(|t| t.contains('X')), "the paragraph's typing is kept: {texts:?}");
}
/// In the page grid, Delete deletes the page chosen there. The picture selected before the grid
/// opened is not on screen, and stays.
#[test]
@@ -7598,6 +7643,91 @@ fn a_selected_pictures_keys_wait_for_its_page_to_be_on_screen() {
assert_eq!(after.len(), before.len(), "the picture stays");
assert_eq!(after[0].visible_rect, before[0].visible_rect, "and is not cropped");
}
// A page just off the screen, in a continuous view: it is still run for what lies near it.
let mut h = picture_pages(false);
let id = h.state().views[0].id;
let pictures = h.state().session.get(id).unwrap().page_images(0).len();
h.set_size(egui::vec2(1400.0, 400.0));
h.state_mut().views[0].go_to_page(1);
h.run_steps(6);
// Going to a page leaves a margin above it: a little further, past page 1's foot.
let centre = h.state().views[0].viewport_rect().center();
h.hover_at(centre);
h.run_steps(1);
h.event(egui::Event::MouseWheel {
unit: egui::MouseWheelUnit::Point,
delta: egui::vec2(0.0, -60.0),
phase: egui::TouchPhase::Move,
modifiers: egui::Modifiers::NONE,
source: egui::MouseWheelSource::Trackpad,
});
h.run_steps(6);
assert!(h.state().views[0].continuous, "a continuous view");
assert!(h.state().views[0].page_screen_rect(0).is_none(), "page 1 is off the screen");
h.key_press(egui::Key::Delete);
h.run_steps(4);
assert_eq!(h.state().session.get(id).unwrap().page_images(0).len(), pictures, "the picture stays");
}
/// Delete waits for the button to come up: the press held would then name another item, whose drag or
/// click would follow.
#[test]
fn delete_waits_for_the_button_to_come_up() {
let hold_and_delete = |h: &mut Harness<'static, PdfCraftApp>, on: egui::Pos2| {
h.hover_at(on);
h.run_steps(1);
h.drag_at(on);
h.run_steps(1);
h.key_press(egui::Key::Delete);
h.run_steps(2);
h.drop_at(on);
h.run_steps(3);
};
let mut h = picture_pages(false);
let id = h.state().views[0].id;
let pictures = h.state().session.get(id).unwrap().page_images(0).len();
let on_picture = page_at(&h, 0, 100.0, 180.0);
hold_and_delete(&mut h, on_picture);
assert_eq!(h.state().session.get(id).unwrap().page_images(0).len(), pictures, "the picture stays");
let mut h = edit_page(pdfcraft_ui_egui::QuickTool::Select);
assert!(h.state_mut().apply_edit(pdfcraft_engine::Edit::AddText {
page: 0,
text: pdfcraft_engine::AddedText { rect: [40.0, 250.0, 140.0, 265.0], text: "Added".into(), ..Default::default() },
}));
assert!(h.state_mut().execute("edit.text"));
h.run_steps(3);
h.state_mut().quick_tool = pdfcraft_ui_egui::QuickTool::Select;
h.run_steps(3);
let on_added = point_on(&h, 90.0, 258.0);
slipping_click(&mut h, on_added, on_added);
assert_eq!(h.state().views[0].content.selected, Some((0, 0)), "the added text is selected");
hold_and_delete(&mut h, on_added);
let id = h.state().views[0].id;
assert_eq!(h.state().session.get(id).unwrap().added().len(), 1, "the added text stays");
}
/// A drag the page it is over first sees lets the picked picture go, whichever page it began on.
#[test]
fn a_drag_first_seen_on_the_next_page_lets_the_picked_picture_go() {
let mut h = picture_pages(false);
h.state_mut().views[0].image_selection = None;
h.state_mut().quick_tool = pdfcraft_ui_egui::QuickTool::Select;
h.run_steps(3);
let on_picture = page_at(&h, 0, 100.0, 180.0);
slipping_click(&mut h, on_picture, on_picture);
assert!(h.state().views[0].picked_image.is_some(), "the picture is picked");
let press = page_at(&h, 0, 180.0, 20.0);
let release = page_at(&h, 1, 180.0, 280.0);
h.hover_at(press);
h.run_steps(1);
h.drag_at(press);
h.run_steps(1);
h.hover_at(release);
h.run_steps(1);
h.drop_at(release);
h.run_steps(4);
assert!(h.state().views[0].picked_image.is_none(), "the drag lets it go");
}
/// With the pointer on the Pages panel, Delete is the panel's: a page goes, though a picture is
@@ -7880,29 +8010,31 @@ fn an_add_text_click_pressed_beside_a_paragraph_starts_a_box_there() {
/// waited, and moved the paragraph to wherever the pointer then was.
#[test]
fn a_paragraph_drag_ends_when_the_window_loses_focus() {
let mut h = edit_page(pdfcraft_ui_egui::QuickTool::EditText);
let id = h.state().views[0].id;
let before = h.state().session.get(id).unwrap().text_blocks(0)[0].rect;
let from = point_on(&h, (before[0] + before[2]) / 2.0, (before[1] + before[3]) / 2.0);
h.hover_at(from);
h.run_steps(1);
h.drag_at(from);
h.run_steps(1);
for k in 1..=4 {
h.hover_at(from + egui::vec2(0.0, 10.0 * k as f32));
for tool in [pdfcraft_ui_egui::QuickTool::EditText, pdfcraft_ui_egui::QuickTool::AddText] {
let mut h = edit_page(tool);
let id = h.state().views[0].id;
let before = h.state().session.get(id).unwrap().text_blocks(0)[0].rect;
let from = point_on(&h, (before[0] + before[2]) / 2.0, (before[1] + before[3]) / 2.0);
h.hover_at(from);
h.run_steps(1);
h.drag_at(from);
h.run_steps(1);
for k in 1..=4 {
h.hover_at(from + egui::vec2(0.0, 10.0 * k as f32));
h.run_steps(1);
}
assert!(h.state().views[0].block_drag.is_some(), "the paragraph is being moved");
h.input_mut().focused = false;
h.run_steps(2);
h.drop_at(from + egui::vec2(0.0, 40.0));
h.run_steps(2);
h.input_mut().focused = true;
h.hover_at(from + egui::vec2(0.0, 80.0));
h.run_steps(4);
assert!(h.state().views[0].block_drag.is_none(), "the drag is over");
assert_eq!(h.state().session.get(id).unwrap().text_blocks(0)[0].rect, before, "nothing moved");
assert!(!dirty(&h));
}
assert!(h.state().views[0].block_drag.is_some(), "the paragraph is being moved");
h.input_mut().focused = false;
h.run_steps(2);
h.drop_at(from + egui::vec2(0.0, 40.0));
h.run_steps(2);
h.input_mut().focused = true;
h.hover_at(from + egui::vec2(0.0, 80.0));
h.run_steps(4);
assert!(h.state().views[0].block_drag.is_none(), "the drag is over");
assert_eq!(h.state().session.get(id).unwrap().text_blocks(0)[0].rect, before, "nothing moved");
assert!(!dirty(&h));
}
/// The rotate knob of a picture that fills its page lies past the page's edge, and a drag there
@@ -452,6 +452,47 @@ fn a_double_click_on_a_grouped_object_reaches_it() {
assert!(h.state().views[0].content.draft.as_ref().is_some_and(|d| d.index == Some(0)), "the added note opens for typing");
}
/// A double-click on a paragraph selected with others opens the one its press was on, though the
/// button came up just off it.
#[test]
fn a_double_click_on_a_grouped_paragraph_released_just_off_it_opens_it() {
let mut h = harness();
marquee(&mut h);
let (inside, outside) = {
let app = h.state();
let view = &app.views[0];
let xf = view.page_xform(0).unwrap();
let doc = app.session.get(view.id).unwrap();
let block = doc.text_blocks(0).into_iter().find(|b| b.text.contains("Group headline")).expect("the headline");
let frame = xf.user_rect(&doc.info, 0, block.rect.map(|v| v as f32));
(egui::pos2(frame.center().x, frame.top() + 1.0), egui::pos2(frame.center().x, frame.top() - 4.0))
};
let button = |pos, pressed| egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed, modifiers: Modifiers::NONE };
h.hover_at(inside);
h.run_steps(1);
crate::support::input::one_frame(&mut h, [button(inside, true), button(inside, false)]);
crate::support::input::one_frame(&mut h, [button(inside, true), egui::Event::PointerMoved(outside), button(outside, false)]);
h.run_steps(2);
assert!(h.state().views[0].line_editor.is_some(), "the paragraph pressed is open for typing");
}
/// A press counts its own modifiers, whatever the frame's are: Shift in the press event adds to the
/// selection, though the frame's modifiers show none.
#[test]
fn a_press_counts_the_modifiers_of_its_own_event() {
let mut h = harness();
let p = at(&h, 55.0, 70.0);
click(&mut h, p, Modifiers::NONE);
assert!(h.state().views[0].line_editor.is_some());
let p = at(&h, 60.0, 145.0);
let button = |pressed| egui::Event::PointerButton { pos: p, button: egui::PointerButton::Primary, pressed, modifiers: Modifiers::SHIFT };
h.hover_at(p);
h.run_steps(2);
crate::support::input::one_frame(&mut h, [button(true), button(false)]);
h.run_steps(4);
h.get_by_label("2 objects selected");
}
/// A double-click on a paragraph selected with others opens it for typing (the user's ruling).
#[test]
fn a_double_click_on_a_grouped_paragraph_opens_it() {
+27
View File
@@ -1934,6 +1934,33 @@ fn a_right_click_with_the_button_held_opens_the_selected_comments_menu() {
h.run_steps(2);
}
/// With the page gaps hidden, a Shift-click on the row two pages share is the first page's: what it
/// selects there stays selected, and the second page leaves it alone.
#[test]
fn a_shift_click_where_two_pages_touch_keeps_what_it_selected() {
let mut h = open(bottom_line_pages_pdf(), "100", false);
h.state_mut().views[0].show_page_gaps = false;
assert!(h.state_mut().execute("edit.edit_text"));
h.run_steps(4);
let (one, two) = {
let v = &h.state().views[0];
(v.page_screen_rect(0).expect("page 1 on screen"), v.page_screen_rect(1).expect("page 2 on screen"))
};
// "Bottom line" runs from x 40, a few points above page 1's foot.
let seam = egui::pos2(one.min.x + 70.0, one.max.y);
assert!(one.contains(seam) && two.contains(seam), "the pages touch: {one:?} {two:?}");
h.hover_at(seam);
h.run_steps(2);
for pressed in [true, false] {
h.event_modifiers(
egui::Event::PointerButton { pos: seam, button: egui::PointerButton::Primary, pressed, modifiers: egui::Modifiers::SHIFT },
egui::Modifiers::SHIFT,
);
}
h.run_steps(4);
assert!(h.query_by_label_contains("1 object").is_some(), "the line is selected");
}
/// With Prepare open on a document that can't be changed, a right-click on a field selects nothing
/// and opens the page's menu, whose field entries would only be refused.
#[test]