From 7d6af8a508a257df7b55e1a3e30fb3a1e56fab51 Mon Sep 17 00:00:00 2001 From: Hitesh Date: Thu, 22 Oct 2026 21:04:22 +0530 Subject: [PATCH 1/2] fix: Delete removes the selected paragraph or objects in Edit text A paragraph selected by its frame (or objects taken by an area selection) answered the arrow keys and Escape but not Delete, so the only way to remove text was one typed character at a time in the inline editor. Delete or Backspace now removes the whole selection as one undo step: paragraphs and images last-first in their inventories, added content after them, under the existing "Delete content" history label. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/lib.rs | 1 + crates/ui-egui/src/object_ui.rs | 73 ++++++++++++++++++++++++++++++ crates/ui-egui/tests/it/editing.rs | 29 ++++++++++++ parity/acrobat-features.toml | 4 +- 4 files changed, 105 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 64b22ff18..aea4b6f84 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -3023,6 +3023,7 @@ impl PdfCraftApp { && let Some(doc) = self.session.get(self.views[i].id).filter(|d| d.allows_modification()) { object_ui::nudge_keys(ctx, &mut self.views[i], doc); + object_ui::delete_keys(ctx, &mut self.views[i]); } // A selected field moves with the arrows too (Prepare a form, #2608), before the view // turns the page with them. diff --git a/crates/ui-egui/src/object_ui.rs b/crates/ui-egui/src/object_ui.rs index 029dc5bc9..7f81f76a2 100644 --- a/crates/ui-egui/src/object_ui.rs +++ b/crates/ui-egui/src/object_ui.rs @@ -384,6 +384,45 @@ pub(crate) fn nudge_keys(ctx: &egui::Context, view: &mut DocView, doc: &Document } } +/// Delete (or Backspace) removes the selected objects, as in Acrobat: a paragraph selected by its +/// frame, or everything an area selection took, goes at once instead of one typed character at a +/// time. +pub(crate) fn delete_keys(ctx: &egui::Context, view: &mut DocView) { + if ctx.egui_wants_keyboard_input() || view.pending_edit.is_some() || view.objects.gesture.is_some() { + return; + } + let Some(page) = view.objects.page.filter(|_| !view.objects.selected.is_empty()) else { return }; + use egui::{Key, Modifiers}; + if !ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Delete) || i.consume_key(Modifiers::NONE, Key::Backspace)) { + return; + } + view.pending_edit = delete_edit(page, view.objects.selected.iter().map(|o| o.target).collect()); + view.objects.clear(); +} + +/// The edit that deletes `targets` (indexes into one inventory of `page`) as one undo step. +/// Each deletion renumbers only what follows it in its own inventory, so paragraphs and images +/// go from the last to the first, and added content, which the source inventories leave out, +/// after them. +fn delete_edit(page: usize, mut targets: Vec) -> Option { + targets.sort_by_key(|t| (t.kind == ObjectKind::Added, std::cmp::Reverse(t.index))); + targets.dedup(); + let mut edits: Vec = targets + .into_iter() + .map(|t| match t.kind { + ObjectKind::Text => Edit::EditTextBlock { page, block: t.index, text: String::new(), style: pdfcraft_engine::BlockStyle::default() }, + ObjectKind::Image => Edit::EditPageImage { page, index: t.index, change: pdfcraft_engine::ImageEdit::Delete }, + ObjectKind::Added => Edit::DeleteContent { page, index: t.index }, + }) + .collect(); + if edits.len() > 1 { + // "Delete content" is an edit label already, so the history reads the same in every language. + Some(Edit::Batch { label: "Delete content".into(), edits }) + } else { + edits.pop() + } +} + pub(crate) fn paint(painter: &egui::Painter, xf: &PageXform, page: usize, info: &DocInfo, selection: &ObjectSelection) { if selection.page != Some(page) { return; @@ -418,6 +457,8 @@ pub(crate) fn move_error(error: &pdfcraft_engine::EditError) -> String { #[cfg(test)] mod tests { + use super::*; + #[test] fn selection_labels_and_refusals_are_translated_in_every_catalog() { for language in crate::i18n::LANGUAGES.iter().filter(|l| !l.source.is_empty()) { @@ -439,4 +480,36 @@ mod tests { assert_ne!(crate::i18n::trn(lang, 2, "{n} object selected", "{n} objects selected"), "2 objects selected", "{}", language.code); } } + + fn t(kind: ObjectKind, index: usize) -> ObjectTarget { + ObjectTarget { kind, index } + } + + /// Deleting renumbers what follows in the same inventory, so the batch takes each inventory + /// from its last item to its first, and added content (outside the source inventories) last. + #[test] + fn several_objects_delete_last_first_in_one_step() { + let targets = vec![t(ObjectKind::Added, 0), t(ObjectKind::Text, 1), t(ObjectKind::Image, 0), t(ObjectKind::Text, 3), t(ObjectKind::Added, 2)]; + let Some(Edit::Batch { label, edits }) = delete_edit(0, targets) else { panic!("several objects make one batch") }; + assert_eq!(label, "Delete content"); + let order: Vec<(char, usize)> = edits + .iter() + .map(|e| match e { + Edit::EditTextBlock { block, text, .. } => { + assert!(text.is_empty()); + ('t', *block) + } + Edit::EditPageImage { index, change: pdfcraft_engine::ImageEdit::Delete, .. } => ('i', *index), + Edit::DeleteContent { index, .. } => ('a', *index), + other => panic!("unexpected {}", other.label().wording), + }) + .collect(); + assert_eq!(order, [('t', 3), ('t', 1), ('i', 0), ('a', 2), ('a', 0)]); + } + + #[test] + fn one_object_deletes_with_its_own_edit_and_none_with_nothing() { + assert!(matches!(delete_edit(2, vec![t(ObjectKind::Image, 4)]), Some(Edit::EditPageImage { page: 2, index: 4, .. }))); + assert!(delete_edit(0, Vec::new()).is_none()); + } } diff --git a/crates/ui-egui/tests/it/editing.rs b/crates/ui-egui/tests/it/editing.rs index 71ba9ccda..38c4ddbdb 100644 --- a/crates/ui-egui/tests/it/editing.rs +++ b/crates/ui-egui/tests/it/editing.rs @@ -5980,3 +5980,32 @@ fn mode_tabs_keep_the_hand_over_a_paragraph_scrolled_under_them() { click_at(&mut h, convert); assert_eq!(h.state().mode, pdfcraft_ui_egui::Mode::Convert, "the tab takes the click, not the paragraph's editor"); } + +/// A paragraph selected by its frame goes with one Delete (or Backspace), as in Acrobat, instead +/// of being emptied a typed character at a time, and undo brings it back. +#[test] +fn a_paragraph_selected_by_its_frame_deletes_with_one_key() { + for key in [Key::Delete, Key::Backspace] { + let mut h = harness(2, |_| {}); + assert!(h.state_mut().execute("edit.edit_text")); + h.run_steps(2); + let id = h.state().views[0].id; + let blocks = |h: &Harness<'static, PdfCraftApp>| h.state().session.get(id).unwrap().text_blocks(0); + let r = h.state().views[0].page_screen_rect(0).expect("on screen"); + let k = r.width() / 200.0; + let screen = |x: f64, y: f64| egui::pos2(r.left() + x as f32 * k, r.top() + (300.0 - y as f32) * k); + let b = blocks(&h)[0].clone(); + click_at(&mut h, screen((b.rect[0] + b.rect[2]) / 2.0, (b.rect[1] + b.rect[3]) / 2.0)); + assert!(h.state().views[0].line_editor.is_some(), "a click on the text opens it for typing"); + click_at(&mut h, screen((b.rect[0] + b.rect[2]) / 2.0, b.rect[3]) - egui::vec2(0.0, 4.0)); + assert!(h.state().views[0].line_editor.is_none(), "the frame selects the box"); + press(&mut h, Modifiers::NONE, key); + assert!(blocks(&h).is_empty(), "{key:?} deletes the whole paragraph: {:?}", blocks(&h)); + assert!(h.state().views[0].line_editor.is_none(), "no empty editor is left behind"); + assert_eq!(h.state().session.get(id).unwrap().text_blocks(1)[0].text, "Page 2", "the next page keeps its text"); + assert_eq!(h.state().session.get(id).unwrap().info.pages.len(), 2, "Delete removed the paragraph, not a page"); + assert!(h.state_mut().execute("edit.undo")); + h.run_steps(2); + assert_eq!(blocks(&h).iter().map(|b| b.text.as_str()).collect::>(), ["Page 1"], "undo restores it"); + } +} diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 362c2aa77..db4cc2819 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -2202,8 +2202,8 @@ milestone = "M11" status = "partial" commands = ["edit.edit_text"] tools = ["object_list", "object_move"] -evidence = ["crates/ui-egui/tests/it/object_selection.rs::marquee_and_mixed_group_drag_move_once_and_undo_together", "crates/automation/tests/it/automation.rs::mixed_objects_move_through_tools_with_generation_and_one_undo", "crates/edit/src/tests.rs::group_moves_preserve_glyph_bytes_and_unselected_text_state", "crates/engine/src/tests.rs::mixed_object_moves_are_one_undo_step_and_survive_save_reopen"] -notes = "Same-page mixed selection (#844): Shift/Ctrl/Command-click toggles, empty-space marquee (additive with modifiers), drag previews and one atomic undo step for existing paragraphs, Image/Form XObjects and added text/images. Source glyph bytes, fonts and kerning retained; unselected text and shared Form resources stay put. Generation-scoped references with optional automation guard; count and geometry bounded; unsupported vertical/clipping text refused without partial changes. Still missing: arbitrary paths, inline images, cross-page movement, group resizing, arrange/align and a general object editor. See docs/editing-multiple-objects.md." +evidence = ["crates/ui-egui/tests/it/object_selection.rs::marquee_and_mixed_group_drag_move_once_and_undo_together", "crates/automation/tests/it/automation.rs::mixed_objects_move_through_tools_with_generation_and_one_undo", "crates/edit/src/tests.rs::group_moves_preserve_glyph_bytes_and_unselected_text_state", "crates/engine/src/tests.rs::mixed_object_moves_are_one_undo_step_and_survive_save_reopen", "crates/ui-egui/tests/it/editing.rs::a_paragraph_selected_by_its_frame_deletes_with_one_key"] +notes = "Same-page mixed selection (#844): Shift/Ctrl/Command-click toggles, empty-space marquee (additive with modifiers), drag previews and one atomic undo step for existing paragraphs, Image/Form XObjects and added text/images. Source glyph bytes, fonts and kerning retained; unselected text and shared Form resources stay put. Delete or Backspace removes the selection (a paragraph selected by its frame, or a group) as one undo step. Generation-scoped references with optional automation guard; count and geometry bounded; unsupported vertical/clipping text refused without partial changes. Still missing: arbitrary paths, inline images, cross-page movement, group resizing, arrange/align and a general object editor. See docs/editing-multiple-objects.md." [[feature]] id = "edit.object-properties" From caf189522fac9d239f4897e4884032a4f2c81f87 Mon Sep 17 00:00:00 2001 From: Hitesh Date: Thu, 22 Oct 2026 23:18:01 +0530 Subject: [PATCH 2/2] fix(edit): a refused delete keeps the object selection Review on #4111: Delete cleared the selection before the edit ran, so a delete the engine refuses (text used as a clipping path) left the document as it was but dropped what the user had selected. The selection now stays until the edit applies; `ObjectSelection::prepare` already clears it when the document changes and keeps it when an edit is refused. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/object_ui.rs | 3 ++- crates/ui-egui/tests/it/editing.rs | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/object_ui.rs b/crates/ui-egui/src/object_ui.rs index 7f81f76a2..04e37feee 100644 --- a/crates/ui-egui/src/object_ui.rs +++ b/crates/ui-egui/src/object_ui.rs @@ -396,8 +396,9 @@ pub(crate) fn delete_keys(ctx: &egui::Context, view: &mut DocView) { if !ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Delete) || i.consume_key(Modifiers::NONE, Key::Backspace)) { return; } + // The selection stays until the edit applies: `prepare` clears it when the document changes, + // and a refused delete (text used as a clipping path) leaves it selected. view.pending_edit = delete_edit(page, view.objects.selected.iter().map(|o| o.target).collect()); - view.objects.clear(); } /// The edit that deletes `targets` (indexes into one inventory of `page`) as one undo step. diff --git a/crates/ui-egui/tests/it/editing.rs b/crates/ui-egui/tests/it/editing.rs index 38c4ddbdb..4a35390c9 100644 --- a/crates/ui-egui/tests/it/editing.rs +++ b/crates/ui-egui/tests/it/editing.rs @@ -4219,6 +4219,30 @@ fn an_ordinary_paragraph_drag_refusal_never_rewrites_the_text() { assert!(h.state().toast.as_ref().unwrap().0.contains("Text used as a clipping path cannot be moved with this selection.")); } +/// A delete the engine refuses (text used as a clipping path) leaves the document as it was and +/// keeps the selection, so the user can still move or nudge what they had selected. +#[test] +fn a_refused_delete_keeps_the_object_selection() { + let content = "q BT 4 Tr /F1 12 Tf 60 320 Td (Clipping text) Tj ET Q"; + let mut h = paragraph_drag_harness(paragraph_drag_fixture(content, 0, 1), 0); + let id = h.state().views[0].id; + let (generation, bytes) = { + let doc = h.state().session.get(id).unwrap(); + (doc.edit_generation(), doc.bytes.clone()) + }; + // An area selection from empty page space around the paragraph. + let (center, _) = paragraph_drag_points(&h, "Clipping text", [0.0, 0.0]); + drag(&mut h, center - egui::vec2(60.0, 25.0), center + egui::vec2(60.0, 25.0)); + h.get_by_label("1 object selected"); + h.key_press(Key::Delete); + h.run_steps(4); + let doc = h.state().session.get(id).unwrap(); + assert_eq!((doc.edit_generation(), &doc.bytes), (generation, &bytes), "the refused delete changed nothing"); + assert!(doc.can_undo().is_none()); + assert!(h.state().toast.is_some(), "the refusal is reported"); + h.get_by_label("1 object selected"); +} + #[test] fn an_ordinary_paragraph_drag_cannot_reuse_an_index_after_an_edit() { let content = "BT /F1 12 Tf 60 120 Td (Discard) Tj ET BT /F1 12 Tf 60 320 Td (Moving alpha) Tj ET BT /F1 12 Tf 60 220 Td (Keep here) Tj ET"; @@ -6002,6 +6026,7 @@ fn a_paragraph_selected_by_its_frame_deletes_with_one_key() { press(&mut h, Modifiers::NONE, key); assert!(blocks(&h).is_empty(), "{key:?} deletes the whole paragraph: {:?}", blocks(&h)); assert!(h.state().views[0].line_editor.is_none(), "no empty editor is left behind"); + assert!(h.query_all_by_label_contains("object selected").next().is_none(), "the deleted paragraph is no longer selected"); assert_eq!(h.state().session.get(id).unwrap().text_blocks(1)[0].text, "Page 2", "the next page keeps its text"); assert_eq!(h.state().session.get(id).unwrap().info.pages.len(), 2, "Delete removed the paragraph, not a page"); assert!(h.state_mut().execute("edit.undo"));