fix(ui): a right-click where two pages touch is one page's; the guard reads split calls

Found in review:
- **A right-click where two pages touch.** With the page gaps hidden, a right-click on the row
  two pages share ran on both: the first page selected its comment, the second unselected it,
  and the second page's anchor for the menu stood. A right-click is now the first page's, as a
  press has been since the last fix. This was so on `main` too.
- **The guard** matched one line at a time, so a call rustfmt splits across lines
  (`view` / `.router` / `.owner(i)`) got past it. A continued line is now read with the one it
  continues. It also refuses a hit test imported under a name that isn't a hit test's
  (`use …::hit as under`), which the checks by name would miss.
- **Docs.** Links still take a right-click while the primary button is held, as before, and when
  Escape comes in a press's own frame every page is `Elsewhere`, not `Free`.

Tests: a stamp clicked where two pages touch lands on the first, a click pressed on one page and
released on the next places nothing, a right-click where two pages touch is the first page's,
Prepare keeps its selection on a click that began off the page, and the Note tool starts no note
on added text with the Edit panel open. Each was seen failing with its fix, or the rule it pins,
reverted. A planted split router read and a planted aliased hit test fail the guard.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
alehelandClaude Opus 5.5 committed 2026-11-02 20:39:41 +01:00
1 parent 2a4032b2fc
commit 6756ab0ed4
5 files changed
+149 -6

No files matched your search

+15 -1
View File
@@ -1207,6 +1207,19 @@ struct PageFrame<'a> {
current_height: f32,
pressed_page: Option<usize>,
calc_hover: Option<crate::calc_order_ui::Focus>,
/// A page took this frame's right-click.
right_clicked: bool,
}
impl PageFrame<'_> {
/// Where this frame's right-click is, on the page at `rect`, for the first page it is on: pages
/// that touch (the gaps hidden) can share a row of pixels, and a right-click there is one
/// page's, as a press is.
fn right_click_on(&mut self, rect: Rect) -> Option<Pos2> {
let p = self.pointer.filter(|p| !self.right_clicked && self.resp.secondary_clicked() && rect.contains(*p))?;
self.right_clicked = true;
Some(p)
}
}
/// Visible heights (points) closer than this are equal: one page size can meet the viewport a
@@ -1502,7 +1515,7 @@ fn draw_page(ui: &mut egui::Ui, view: &mut DocView, frame: &mut PageFrame<'_>, i
// A cancelled press takes nothing until the button is up, though the page's menu still opens
// where a right-click is.
let input = if view.press_cancelled {
if let Some(p) = pointer.filter(|p| frame.resp.secondary_clicked() && xf.rect.contains(*p)) {
if let Some(p) = frame.right_click_on(xf.rect) {
comments::anchor(&pcx, p, view);
}
PageInput { consumed: true, area_tool: false, hover: None, released_over: false }
@@ -1683,6 +1696,7 @@ fn draw_pages(
current_height: f32::INFINITY,
pressed_page: None,
calc_hover: None,
right_clicked: false,
};
for &i in visible_pages {
draw_page(ui, view, &mut frame, i);
+3 -2
View File
@@ -109,7 +109,7 @@ pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut
// goes down, so the button is what tells. A touch's long press, which egui reports as a
// right-click with the finger still down, opens its menu.
let press_held = ptr.primary_down && !resp.long_touched();
let context = pointer.filter(|p| resp.secondary_clicked() && xf.rect.contains(*p)).and_then(|p| right_click(&hits, &under, view, p, press_held));
let context = frame.right_click_on(xf.rect).and_then(|p| right_click(&hits, &under, view, p, press_held));
// A mark already placed takes its own press (comments, below), and with no button down the
// Fill & Sign tool's preview hides over one, where a press would pick it up.
let over_mark =
@@ -332,7 +332,8 @@ fn tool_input(
/// ([`PageHits::context_at`]), which the menu then acts on. A comment there is selected, and none
/// elsewhere; a Prepare field is selected; links take theirs from what this returns.
/// `under`: the hits at `p` before the tools act. `press_held`: the primary button is held, and the
/// selections its press acts on are left alone until its release.
/// comment and Prepare selections its press acts on are left alone until its release; links still
/// take theirs, as before.
fn right_click(hits: &PageHits<'_>, under: &[crate::page_router::Hit], view: &mut DocView, p: Pos2, press_held: bool) -> Option<Owner> {
let context = hits.context_at(view, p, under);
// Delete page, Paste and Copy Image act where the menu opened, whoever takes the right-click.
@@ -293,7 +293,8 @@ fn outside_test_modules(source: &str) -> Vec<(usize, &str)> {
}
/// Each name `find` finds in a line of the page modules' code (their tests left out), as (file,
/// the top-level function or `impl` method it is in, line, name).
/// the top-level function or `impl` method it is in, line, name). A line rustfmt continued (a
/// method chain split as `view` / `.router` / `.owner(i)`) is read with the one it continues.
fn page_module_matches(find: impl Fn(&str) -> Vec<String>) -> Vec<(&'static str, Option<String>, usize, String)> {
let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
let mut found = Vec::new();
@@ -301,6 +302,7 @@ fn page_module_matches(find: impl Fn(&str) -> Vec<String>) -> Vec<(&'static str,
let source = std::fs::read_to_string(src.join(file)).expect("page module").replace("\r\n", "\n");
// The top-level function, or method of an `impl`, that each line is in.
let (mut function, mut in_impl) = (None, false);
let mut lines: Vec<(usize, Option<&str>, String)> = Vec::new();
for (n, line) in outside_test_modules(&source) {
if line.starts_with("impl") {
(function, in_impl) = (None, true);
@@ -312,7 +314,13 @@ fn page_module_matches(find: impl Fn(&str) -> Vec<String>) -> Vec<(&'static str,
if line.trim_start().starts_with("//") {
continue;
}
for name in find(line) {
match lines.last_mut() {
Some((_, _, text)) if line.trim_start().starts_with('.') => text.push_str(line.trim_start()),
_ => lines.push((n, function, line.to_owned())),
}
}
for (n, function, text) in lines {
for name in find(&text) {
found.push((file, function.map(str::to_owned), n, name));
}
}
@@ -320,6 +328,41 @@ fn page_module_matches(find: impl Fn(&str) -> Vec<String>) -> Vec<(&'static str,
found
}
/// Each hit test a page module imports under a name that is not a hit test's (`use …::hit as
/// under;`), which the checks by name would not see.
fn hit_test_aliases() -> Vec<String> {
let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
let mut found = Vec::new();
for file in PAGE_MODULES {
let source = std::fs::read_to_string(src.join(file)).expect("page module");
// Each `use` statement, from its first line to its `;`, wherever it is.
let mut statement: Option<String> = None;
for line in source.lines().map(str::trim) {
if statement.is_none() && ["use ", "pub use ", "pub(crate) use ", "pub(super) use "].iter().any(|start| line.starts_with(start)) {
statement = Some(String::new());
}
let Some(text) = statement.as_mut() else { continue };
text.push_str(line);
text.push(' ');
if !line.ends_with(';') {
continue;
}
for part in text.split([',', '{', '}', ';']) {
let Some((path, alias)) = part.split_once(" as ") else { continue };
let (name, alias) = (path.trim().rsplit("::").next().unwrap_or_default(), alias.trim());
if is_hit_test(name) && !is_hit_test(alias) {
found.push(format!(
"src/{file}: `{name} as {alias}` imports a hit test under another name, which the checks by name miss: \
name it `hit` or `…_hit`."
));
}
}
statement = None;
}
}
found
}
/// Each use of one of `names` in the page modules' code (their tests left out).
fn page_module_uses(names: &[&str]) -> Vec<(&'static str, Option<String>, usize, String)> {
page_module_matches(|line| names.iter().filter(|name| uses(line, name)).map(|name| (*name).to_owned()).collect())
@@ -615,6 +658,7 @@ fn page_modules_act_on_a_press_only_through_the_router() {
src/canvas/page_pointer/tests.rs, with why.",
"calls no hit test any more: remove it from HIT_TESTERS.",
));
problems.extend(hit_test_aliases());
// An entry no page module calls any more no longer guards anything.
let called = page_module_uses(&PAGE_HIT_CALLS);
problems.extend(
+2 -1
View File
@@ -118,7 +118,8 @@ pub(crate) enum PagePress {
/// No button is down, or the press began on this page and nobody's hit won it: the active
/// tool's. Also every page's while a press the page view does not hold is down (one that began
/// over a panel), and after the press was dropped ([`PageRouter::cancel`]) until the button is
/// up.
/// up (`Elsewhere` instead when Escape came in the press's own frame; page input skips a
/// cancelled press either way).
Free,
/// The press began on another page, or off the pages (between them, or beside one).
Elsewhere,
+83
View File
@@ -1607,3 +1607,86 @@ fn a_right_click_with_edit_text_on_added_text_unselects_the_comment() {
right_click(&mut h, on_added);
assert_eq!(h.state().views[0].comments.selected, None);
}
/// Two pages with their gaps hidden, after `tool` set the tool up, and a point on the row of pixels
/// where they touch, which lies on both.
fn touching_pages(tool: impl FnOnce(&mut PdfCraftApp)) -> (Harness<'static, PdfCraftApp>, egui::Pos2) {
let mut h = open(two_pages_pdf(), "100", false);
h.state_mut().views[0].show_page_gaps = false;
tool(h.state_mut());
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"))
};
let seam = egui::pos2(one.min.x + 100.0, one.max.y);
assert!(one.contains(seam) && two.contains(seam), "the pages touch: {one:?} {two:?}");
(h, seam)
}
/// The pages the document's stamps are on.
fn stamp_pages(h: &Harness<'_, PdfCraftApp>) -> Vec<usize> {
let s = h.state();
s.session.get(s.views[0].id).unwrap().info.annotations.iter().filter(|a| a.subtype == "Stamp").map(|a| a.page).collect()
}
/// Where pages touch (the gaps hidden), a press on the row they share is the first page's: a stamp
/// clicked there lands on page 1. The second page used to take the press over.
#[test]
fn a_stamp_clicked_where_two_pages_touch_lands_on_the_first() {
let (mut h, seam) = touching_pages(|app| app.quick_tool = QuickTool::Stamp(StampKind::DynApproved));
mouse_click(&mut h, seam);
assert_eq!(stamp_pages(&h), vec![0]);
}
/// A click pressed on one page and released on the next (the gaps hidden, within egui's click
/// distance) places nothing: the press is the first page's, and the pointer is off it. `main` placed
/// the stamp on the second page.
#[test]
fn a_stamp_click_pressed_on_one_page_and_released_on_the_next_places_nothing() {
let (mut h, seam) = touching_pages(|app| app.quick_tool = QuickTool::Stamp(StampKind::DynApproved));
slipping_click(&mut h, seam - egui::vec2(0.0, 2.0), seam + egui::vec2(0.0, 2.0));
assert_eq!(stamp_pages(&h), Vec::<usize>::new());
}
/// Where pages touch (the gaps hidden), a right-click on the row they share is the first page's:
/// the menu is anchored there. Both pages used to take it, and the second's anchor stood.
#[test]
fn a_right_click_where_two_pages_touch_is_the_first_pages() {
let (mut h, seam) = touching_pages(|_| {});
right_click(&mut h, seam);
assert_eq!(h.state().views[0].comments.context_at.map(|(page, _)| page), Some(0));
}
/// With Prepare open, a click that began off the page (in the gutter beside it) is no page's: the
/// selected field stays selected, as the Link tool's and comments' selections do.
#[test]
fn with_prepare_a_click_that_began_off_the_page_keeps_the_selected_field() {
let mut h = open(comment_and_checkbox_pdf(), "150", false);
assert!(h.state_mut().execute("form.prepare"));
h.run_steps(4);
let on_box = at(&h, 110.0, 210.0);
mouse_click(&mut h, on_box);
assert!(h.state().views[0].prepare.selected.is_some(), "the field is selected");
let left = page(&h).min.x;
slipping_click(&mut h, egui::pos2(left - 2.0, on_box.y), egui::pos2(left + 3.0, on_box.y));
assert!(h.state().views[0].prepare.selected.is_some(), "a click that began off the page leaves it selected");
}
/// With the Edit panel open and the Note tool, a click on added text is the added text's: no note is
/// started under it, as before.
#[test]
fn a_note_tool_click_on_added_text_starts_no_note() {
let mut h = open(comment_on_text_pdf(), "150", false);
assert!(h.state_mut().apply_edit(pdfcraft_engine::Edit::AddText {
page: 0,
text: pdfcraft_engine::AddedText { rect: [40.0, 195.0, 140.0, 210.0], text: "Added".into(), ..Default::default() },
}));
assert!(h.state_mut().execute("edit.text"));
h.run_steps(3);
h.state_mut().quick_tool = QuickTool::Comment(CommentTool::Note);
h.run_steps(3);
let on_added = at(&h, 90.0, 202.0);
mouse_click(&mut h, on_added);
assert!(h.state().views[0].comments.composer.is_none(), "no note is started");
}