From 1ee332ae5555c5f582994778814f7c14fa3f9923 Mon Sep 17 00:00:00 2001 From: alehel Date: Wed, 4 Nov 2026 15:36:15 +0100 Subject: [PATCH] fix(ui): a picture's keys act only while its page is drawn Found in review. Delete, Backspace and crop's Enter and Escape moved to the app's keys, which run before anything is drawn and don't look at what is on screen. So with Edit text and a picture selected: - Delete in the page grid removed the hidden picture and kept the pages; - Delete with the pointer on the Pages panel did the same; - with the picture's page scrolled away or in single-page view on another page, Delete removed a picture the reader couldn't see, and Enter applied its crop. `main` read these keys in the picture's own page pass, which none of those cases reach. `picture_keys` runs there again, from `page_tool_input`, under Edit text's gate (so no dialog gets them), and before the group acts. Delete still works wherever on the pages the pointer is. Tests, each seen failing with the fix reverted: `delete_in_the_page_grid_deletes_the_page_and_not_the_hidden_picture`, `a_selected_pictures_keys_wait_for_its_page_to_be_on_screen`, `delete_with_the_pointer_on_the_pages_panel_deletes_a_page`. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/app/keys.rs | 2 - crates/ui-egui/src/canvas/page_input.rs | 3 + .../ui-egui/src/tools/edit_text_ui/image.rs | 29 ++++---- crates/ui-egui/tests/it/editing.rs | 66 +++++++++++++++++++ 4 files changed, 83 insertions(+), 17 deletions(-) diff --git a/crates/ui-egui/src/app/keys.rs b/crates/ui-egui/src/app/keys.rs index 2fc44da6f2..c014453806 100644 --- a/crates/ui-egui/src/app/keys.rs +++ b/crates/ui-egui/src/app/keys.rs @@ -66,8 +66,6 @@ impl PdfCraftApp { { object_ui::nudge_keys(ctx, &mut self.views[i], doc); object_ui::delete_keys(ctx, &mut self.views[i]); - let held = ctx.input(|input| input.pointer.any_down()); - edit_text_ui::picture_keys(ctx, &mut self.views[i], doc, held); } else if let Some(doc) = self.session.get(self.views[i].id).filter(|d| d.allows_annotation()) { comments::nudge_keys(ctx, &mut self.views[i], &doc.info); } diff --git a/crates/ui-egui/src/canvas/page_input.rs b/crates/ui-egui/src/canvas/page_input.rs index 3cf1ae8ec2..dbf0878bb3 100644 --- a/crates/ui-egui/src/canvas/page_input.rs +++ b/crates/ui-egui/src/canvas/page_input.rs @@ -193,6 +193,9 @@ pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut // pictures, each told what the press is to this page. Edit text ignores input while a dialog, // the palette, another window or another tab has it (`gate`), as before. let gate = !(tool == QuickTool::EditText && view.objects.input_blocked()); + if tool == QuickTool::EditText && can_modify && gate { + crate::edit_text_ui::picture_keys(ui.ctx(), view, i, &images, ptr.any_down); + } let on_objects = tool == QuickTool::EditText && can_modify && crate::object_ui::page_input(cx, press, view); let picks_up = matches!(tool, QuickTool::Select | QuickTool::AddText | QuickTool::EditText); let ccx = crate::content_ui::ContentCx { diff --git a/crates/ui-egui/src/tools/edit_text_ui/image.rs b/crates/ui-egui/src/tools/edit_text_ui/image.rs index 829a6d284b..f39c644487 100644 --- a/crates/ui-egui/src/tools/edit_text_ui/image.rs +++ b/crates/ui-egui/src/tools/edit_text_ui/image.rs @@ -467,24 +467,24 @@ fn apply_crop_mode(view: &mut DocView, image: &pdfcraft_engine::PageImage) { } } -/// The selected picture's keys, from the app's own (none reach it under a dialog): Delete or -/// Backspace removes it, wherever the pointer is, unless it is being dragged or cropped. 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, doc: &pdfcraft_engine::Document, held: bool) { +/// 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 +/// 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() { return; } - let Some(s) = view.image_selection.as_ref() else { return }; - let (page, index) = (s.page, s.index); + let Some(s) = view.image_selection.as_ref().filter(|s| s.page == page) else { return }; + let index = s.index; let key = |k: egui::Key| ctx.input(|i| i.key_pressed(k)); - // The page's pictures are read only when a key asks for one. - let image = || doc.page_images(page).into_iter().nth(index); + let image = images.get(index); if s.crop.is_some() { if key(egui::Key::Enter) { - if let Some(image) = image() { - apply_crop_mode(view, &image); + if let Some(image) = image { + apply_crop_mode(view, image); } } else if !held && key(egui::Key::Escape) @@ -492,9 +492,8 @@ pub(crate) fn picture_keys(ctx: &egui::Context, view: &mut DocView, doc: &pdfcra { s.crop = None; } - } else if !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, after this - // edit has applied. + } else if !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); i.consume_key(egui::Modifiers::NONE, egui::Key::Backspace); diff --git a/crates/ui-egui/tests/it/editing.rs b/crates/ui-egui/tests/it/editing.rs index 0ec0c6f781..06ff943bb5 100644 --- a/crates/ui-egui/tests/it/editing.rs +++ b/crates/ui-egui/tests/it/editing.rs @@ -7377,6 +7377,72 @@ fn a_right_click_on_an_open_paragraphs_frame_over_a_picture_selects_the_picture( assert!(h.query_by_label("Flip horizontal").is_some(), "its menu opens"); } +/// In the page grid, Delete deletes the page chosen there. The picture selected before the grid +/// opened is not on screen, and stays. +#[test] +fn delete_in_the_page_grid_deletes_the_page_and_not_the_hidden_picture() { + 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 pages = h.state().session.get(id).unwrap().info.pages.len(); + assert!(h.state_mut().execute("page.organize")); + h.run_steps(4); + h.get_by_label("Select page 2").click(); + h.run_steps(2); + h.key_press(egui::Key::Delete); + h.run_steps(4); + let doc = h.state().session.get(id).unwrap(); + assert_eq!(doc.info.pages.len(), pages - 1, "the page chosen in the grid is deleted"); + assert_eq!(doc.page_images(0).len(), pictures, "the picture stays"); +} + +/// With the picture's page not on screen, its keys do nothing: Delete removes no picture the reader +/// can't see, and Enter applies no crop. +#[test] +fn a_selected_pictures_keys_wait_for_its_page_to_be_on_screen() { + use pdfcraft_ui_egui::canvas::PageLayout; + for (crop, key) in [(false, egui::Key::Delete), (true, egui::Key::Enter)] { + let mut h = picture_pages(crop); + let id = h.state().views[0].id; + let before = h.state().session.get(id).unwrap().page_images(0); + if crop { + let keep = before[0].visible_rect.unwrap(); + let half = [keep[0], keep[1], (keep[0] + keep[2]) / 2.0, keep[3]]; + h.state_mut().views[0].image_selection.as_mut().unwrap().crop.as_mut().unwrap().keep = half; + } + h.state_mut().views[0].layout = PageLayout::Single; + h.state_mut().views[0].continuous = false; + h.state_mut().views[0].go_to_page(1); + h.run_steps(4); + assert!(h.state().views[0].page_screen_rect(0).is_none(), "page 1 is off the screen"); + h.key_press(key); + h.run_steps(4); + let after = h.state().session.get(id).unwrap().page_images(0); + assert_eq!(after.len(), before.len(), "the picture stays"); + assert_eq!(after[0].visible_rect, before[0].visible_rect, "and is not cropped"); + } +} + +/// With the pointer on the Pages panel, Delete is the panel's: a page goes, though a picture is +/// selected on the pages. +#[test] +fn delete_with_the_pointer_on_the_pages_panel_deletes_a_page() { + let mut h = picture_pages(false); + h.state_mut().set_option("panel", "pages").unwrap(); + h.run_steps(4); + let id = h.state().views[0].id; + let pages = h.state().session.get(id).unwrap().info.pages.len(); + assert!(h.state().views[0].image_selection.is_some(), "the picture is selected"); + assert!(h.state().views[0].page_screen_rect(0).is_some(), "on a page on screen"); + let viewport = h.state().views[0].viewport_rect(); + assert!(viewport.right() < 1300.0, "the panel lies right of the pages: {viewport:?}"); + h.hover_at(egui::pos2(viewport.right() + 60.0, viewport.center().y)); + h.run_steps(2); + h.key_press(egui::Key::Delete); + h.run_steps(4); + assert_eq!(h.state().session.get(id).unwrap().info.pages.len(), pages - 1, "a page goes"); +} + /// In crop mode, a right-click on another picture selects it for its menu, as without crop mode. It /// went to crop mode's whole-page hit, which unselected every picture. #[test]