From aa326f37cdf7ae823cb2149321bf413a7b036ef3 Mon Sep 17 00:00:00 2001 From: RiceDee <345114586+RiceDee@users.noreply.github.com> Date: Sun, 25 Oct 2026 21:15:26 +1100 Subject: [PATCH] fix(ui): formatting restarts a text box's history; the menu claims the box first Three holes in the shared text box's history: - Formatting applied with the paragraph open left typing_from as it was, so when the format left the text unchanged the box kept its history, and Redo put typing undone before the format back on top of it. applied() now resets typing_from, so the next claim clears it. - Undo or Redo from the menu in the same frame as a tab switch ran before the box was drawn, on the history of the other tab's box: Undo put tab B's text into tab A's box, and Redo put A's undone typing into B's. undo_in_page_box and redo_in_page_box now make the box claim the text box first (claim_typing), as drawing it does. - A box new to the text box cleared only the last box's history and kept its caret and selection; it now starts from a default state. claim_typing on LineEditor and TextDraft is now the one way a box takes the text box, for drawing and for the menu. --- crates/ui-egui/src/content_ui.rs | 8 ++- crates/ui-egui/src/edit_text_ui.rs | 23 ++++-- crates/ui-egui/src/editing.rs | 6 ++ crates/ui-egui/tests/it/content.rs | 36 ++++++++++ crates/ui-egui/tests/it/editing.rs | 110 +++++++++++++++++++++++++++++ 5 files changed, 175 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/content_ui.rs b/crates/ui-egui/src/content_ui.rs index 04d626088d..77edee6876 100644 --- a/crates/ui-egui/src/content_ui.rs +++ b/crates/ui-egui/src/content_ui.rs @@ -75,6 +75,12 @@ pub struct TextDraft { } impl TextDraft { + /// Take the shared text box for this box ([`crate::edit_text_ui::forget_typing_before`]): its + /// own history, or none once the page has its text. + pub(crate) fn claim_typing(&mut self, ctx: &egui::Context, doc: pdfcraft_engine::DocId) { + crate::edit_text_ui::forget_typing_before(ctx, input_id(), doc, &mut self.typing_from, &self.style.text); + } + /// Typing the page doesn't have yet: Undo takes it back in the box before touching the document. pub(crate) fn has_unapplied_typing(&self) -> bool { self.text != self.style.text @@ -604,7 +610,7 @@ pub(crate) fn editor(ctx: &egui::Context, view: &mut DocView, info: &DocInfo, ad ui.fonts_mut(|f| f.layout_job(job)) }; let input = input_id(); - crate::edit_text_ui::forget_typing_before(ui.ctx(), input, doc_id, &mut t.typing_from, &t.style.text); + t.claim_typing(ui.ctx(), doc_id); ui.horizontal_top(|ui| { ui.spacing_mut().item_spacing.x = 4.0; let mut edit = egui::TextEdit::multiline(&mut t.text) diff --git a/crates/ui-egui/src/edit_text_ui.rs b/crates/ui-egui/src/edit_text_ui.rs index 4724cbb6e5..a0f73eaf43 100644 --- a/crates/ui-egui/src/edit_text_ui.rs +++ b/crates/ui-egui/src/edit_text_ui.rs @@ -277,7 +277,9 @@ pub(crate) fn input_id() -> egui::Id { /// own record of that; which document `doc` last used the history is kept beside it, since a box /// in another tab on the same text would otherwise take that document's typing with its Redo. /// When another document's box takes the text box, the box it leaves keeps its history (and -/// caret) aside, and gets it back on its return, so its typing can still be undone there. +/// caret) aside, and gets it back on its return, so its typing can still be undone there. Each +/// box claims the text box when it is drawn, and Undo and Redo from the menu claim it first too, +/// since they can run before the box is drawn again after a tab switch. pub(crate) fn forget_typing_before(ctx: &egui::Context, id: egui::Id, doc: pdfcraft_engine::DocId, from: &mut Option, text: &str) { let owner = id.with("typing-doc"); let last = ctx.data(|d| d.get_temp::(owner)); @@ -297,11 +299,9 @@ pub(crate) fn forget_typing_before(ctx: &egui::Context, id: egui::Id, doc: pdfcr } // Only the same box, still on the text it opened on, takes its history back. let back = ctx.data_mut(|d| d.remove_temp::(aside(doc))).filter(|_| from.as_deref() == Some(text)); - let state = back.unwrap_or_else(|| { - let mut state = egui::TextEdit::load_state(ctx, id).unwrap_or_default(); - state.clear_undoer(); - state - }); + // A box new to the text box starts with nothing of the last one's: no history, caret or + // selection. + let state = back.unwrap_or_default(); egui::TextEdit::store_state(ctx, id, state); ctx.data_mut(|d| d.insert_temp(owner, doc)); *from = Some(text.to_owned()); @@ -421,9 +421,18 @@ impl LineEditor { self.look0 = self.look.clone(); self.extras0 = self.extras; self.original = self.text.clone(); + // The page now has this text: the box's history restarts, even when the format left the + // text as it was, so a Redo from before can't put undone typing back on top of it. + self.typing_from = None; self.focus = true; } + /// Take the shared text box for this paragraph ([`forget_typing_before`]): its own history, + /// or none once the page has its text. + pub(crate) fn claim_typing(&mut self, ctx: &egui::Context, doc: pdfcraft_engine::DocId) { + forget_typing_before(ctx, input_id(), doc, &mut self.typing_from, &self.original); + } + /// Adopt the rewritten paragraph's current geometry before the next overlay frame. pub(crate) fn refresh_source(&mut self, block: &pdfcraft_engine::TextBlock, lines: Vec) { self.source_rect = block.rect.map(|v| v as f32); @@ -1724,7 +1733,7 @@ pub(crate) fn overlay(ctx: &egui::Context, view: &mut DocView, info: &DocInfo) - if snapshot.changes_with(text.as_str()) { galley } else { unseen(galley) } }; let input = input_id(); - forget_typing_before(ui.ctx(), input, view_id, &mut ed.typing_from, &ed.original); + ed.claim_typing(ui.ctx(), view_id); let output = egui::TextEdit::multiline(&mut ed.text) .id(input) .font(font.clone()) diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index 408474eaa9..a7ee6b3d54 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -231,12 +231,15 @@ impl PdfCraftApp { fn undo_in_page_box(&mut self) -> bool { let Some(ctx) = self.ctx.clone() else { return false }; let Some(view) = self.active.and_then(|i| self.views.get_mut(i)) else { return false }; + let doc = view.id; if let Some(ed) = view.line_editor.as_mut().filter(|ed| ed.has_unsaved_text()) { + ed.claim_typing(&ctx, doc); let Some(text) = typing_step(&ctx, crate::edit_text_ui::input_id(), &ed.text, true) else { return false }; ed.text = text; return true; } if let Some(d) = view.content.draft.as_mut().filter(|d| d.has_unapplied_typing()) { + d.claim_typing(&ctx, doc); let Some(text) = typing_step(&ctx, crate::content_ui::input_id(), &d.text, true) else { return false }; d.text = text; return true; @@ -259,12 +262,15 @@ impl PdfCraftApp { fn redo_in_page_box(&mut self) -> bool { let Some(ctx) = self.ctx.clone() else { return false }; let Some(view) = self.active.and_then(|i| self.views.get_mut(i)) else { return false }; + let doc = view.id; if let Some(ed) = view.line_editor.as_mut() { + ed.claim_typing(&ctx, doc); let Some(text) = typing_step(&ctx, crate::edit_text_ui::input_id(), &ed.text, false) else { return ed.has_unsaved_text() }; ed.text = text; return true; } if let Some(d) = view.content.draft.as_mut() { + d.claim_typing(&ctx, doc); let Some(text) = typing_step(&ctx, crate::content_ui::input_id(), &d.text, false) else { return d.has_unapplied_typing() }; d.text = text; return true; diff --git a/crates/ui-egui/tests/it/content.rs b/crates/ui-egui/tests/it/content.rs index 43771ed724..387cc1aed1 100644 --- a/crates/ui-egui/tests/it/content.rs +++ b/crates/ui-egui/tests/it/content.rs @@ -1058,6 +1058,42 @@ fn undo_after_a_round_trip_through_another_tab_takes_back_added_text_typing() { assert_eq!(draft(&h).as_deref(), Some("Reviewed"), "Undo takes the typing back in A's box"); } +/// Edit ▸ Undo chosen in the same frame as the switch back to tab A's added text box, before it +/// is drawn again: the shared text box still holds tab B's history, from a box on other text, so +/// A's box claims its own back before stepping. +#[test] +fn undo_in_the_frame_of_the_switch_back_takes_back_added_text_typing() { + let mut h = harness(); + h.state_mut().open_bytes("copy.pdf", None, include_bytes!("../data/form.pdf").to_vec()).unwrap(); + h.run_steps(4); + let rect = [40.0, 195.0, 240.0, 210.0]; + for (tab, text) in [(0, "Reviewed"), (1, "Approved")] { + h.state_mut().active = Some(tab); + let text = pdfcraft_engine::AddedText { rect, text: text.into(), ..Default::default() }; + assert!(h.state_mut().apply_edit(pdfcraft_engine::Edit::AddText { page: 0, text })); + } + let open_in = |h: &mut Harness<'static, PdfCraftApp>, tab: usize| { + h.state_mut().active = Some(tab); + h.run_steps(2); + assert!(h.state_mut().execute("edit.text")); + h.run_steps(3); + let r = h.state().views[tab].page_screen_rect(0).expect("on screen"); + let k = r.width() / 300.0; + click(h, r.min + egui::vec2(45.0 * k, 195.0 * k)); + assert_eq!(h.state().views[tab].content.draft.as_ref().and_then(|d| d.index), Some(0), "the editor opened on the text"); + }; + open_in(&mut h, 0); + h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::End); + h.event(egui::Event::Text(" twice".into())); + h.run_steps(2); + open_in(&mut h, 1); + // Back to A and Undo at once: no frame draws A's box in between. + h.state_mut().active = Some(0); + assert!(h.state_mut().execute("edit.undo")); + h.run_steps(3); + assert_eq!(h.state().views[0].content.draft.as_ref().map(|d| d.text.as_str()), Some("Reviewed"), "Undo takes the typing back in A's box"); +} + /// Redo with an added text box open and unchanged reaches the document's waiting Redo, even when /// the box's text ends in a space: closing the unchanged box writes nothing, so it can't clear /// that Redo first. diff --git a/crates/ui-egui/tests/it/editing.rs b/crates/ui-egui/tests/it/editing.rs index c1acc43fe7..da2a703f55 100644 --- a/crates/ui-egui/tests/it/editing.rs +++ b/crates/ui-egui/tests/it/editing.rs @@ -6688,6 +6688,116 @@ fn undo_after_a_round_trip_through_another_tab_takes_back_the_typing() { assert_eq!(s.session.get(s.views[0].id).unwrap().can_redo(), Some("Edit text"), "the document's Redo still waits"); } +/// Typing taken back with Ctrl+Z, then Bold applied with the paragraph still open: Ctrl+Shift+Z +/// must not bring the typing back on top of the bold, as the box's history is cleared once the +/// page has its text. The format left the text as it was, so the box kept its history and its +/// Redo put "Page 1 more" back (FredApps's review of abc378c0). +#[test] +fn redo_after_formatting_never_brings_back_typing_undone_before_it() { + let mut h = harness(1, |_| {}); + assert!(h.state_mut().execute("edit.edit_text")); + h.run_steps(2); + open_paragraph(&mut h); + let original = h.state().views[0].line_editor.as_ref().unwrap().text.clone(); + h.key_press_modifiers(Modifiers::COMMAND, Key::End); + h.event(egui::Event::Text(" more".into())); + h.run_steps(2); + h.key_press_modifiers(Modifiers::COMMAND, Key::Z); + h.run_steps(3); + assert_eq!(h.state().views[0].line_editor.as_ref().unwrap().text, original, "the typing is taken back"); + h.get_by_label("Bold").click(); + h.run_steps(4); + assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().can_undo(), Some("Edit text"), "the bold is applied"); + h.key_press_modifiers(Modifiers::COMMAND | Modifiers::SHIFT, Key::Z); + h.run_steps(3); + let text = h.state().views[0].line_editor.as_ref().map(|ed| ed.text.clone()); + assert_eq!(text.as_deref(), Some(original.as_str()), "the typing undone before the bold stays undone"); + // Nor through what the menu's Redo runs. + h.state_mut().redo(); + h.run_steps(3); + let s = h.state(); + let text = s.views[0].line_editor.as_ref().map(|ed| ed.text.clone()); + assert!(!text.as_deref().unwrap_or_default().contains("more"), "the menu's Redo brings it back: {text:?}"); + assert_eq!(s.session.get(s.views[0].id).unwrap().text_blocks(0)[0].base_font, "Helvetica-Bold", "the bold stays"); +} + +/// Edit ▸ Undo chosen in the same frame as the switch back to tab A, before A's box is drawn +/// again: the shared text box still holds tab B's history then. Stepping through it put B's +/// typing into A's box; the box now claims its own history back first (CodeRabbit on abc378c0). +#[test] +fn undo_in_the_frame_of_the_switch_back_takes_back_the_typing() { + let mut h = harness(1, |app| { + app.open_bytes("copy.pdf", None, fixture(1)).expect("the copy opens"); + }); + let open_in = |h: &mut Harness<'static, PdfCraftApp>, tab: usize| { + h.state_mut().active = Some(tab); + h.run_steps(2); + if h.state().views[tab].line_editor.is_none() { + assert!(h.state_mut().execute("edit.edit_text")); + h.run_steps(2); + } + let r = h.state().views[tab].page_screen_rect(0).expect("on screen"); + click_at(h, egui::pos2(r.left() + 40.0 / 200.0 * r.width(), r.top() + (300.0 - 158.0) / 300.0 * r.height())); + assert!(h.state().views[tab].line_editor.is_some(), "the paragraph opens for typing"); + }; + open_in(&mut h, 0); + let original = h.state().views[0].line_editor.as_ref().unwrap().text.clone(); + h.key_press_modifiers(Modifiers::COMMAND, Key::End); + h.event(egui::Event::Text(" more".into())); + h.run_steps(2); + // In B, a paragraph whose text differs from A's ("Page 1 B", applied with Bold), so B's + // history can't pass for A's. + open_in(&mut h, 1); + h.key_press_modifiers(Modifiers::COMMAND, Key::End); + h.event(egui::Event::Text(" B".into())); + h.run_steps(2); + h.get_by_label("Bold").click(); + h.run_steps(4); + assert_eq!(h.state().views[1].line_editor.as_ref().map(|ed| ed.text.as_str()), Some("Page 1 B"), "B's paragraph is open on its own text"); + // Back to A and Undo at once: no frame draws A's box in between. + h.state_mut().active = Some(0); + assert!(h.state_mut().execute("edit.undo"), "Undo is enabled"); + h.run_steps(3); + let ed = h.state().views[0].line_editor.clone().expect("the box stays open: Undo takes back the typing"); + assert_eq!(ed.text, original, "the typing is taken back in A's box"); +} + +/// Redo from the menu chosen in the same frame as a switch to tab B, whose paragraph is open on +/// the same text as A's: the shared text box still holds A's history then, with A's undone +/// typing to redo, so B's box claims its own history back before stepping. +#[test] +fn redo_in_the_frame_of_a_tab_switch_never_brings_another_tabs_typing() { + let mut h = harness(1, |app| { + app.open_bytes("copy.pdf", None, fixture(1)).expect("the copy opens"); + }); + let open_in = |h: &mut Harness<'static, PdfCraftApp>, tab: usize| { + h.state_mut().active = Some(tab); + h.run_steps(2); + assert!(h.state_mut().execute("edit.edit_text")); + h.run_steps(2); + let r = h.state().views[tab].page_screen_rect(0).expect("on screen"); + click_at(h, egui::pos2(r.left() + 40.0 / 200.0 * r.width(), r.top() + (300.0 - 158.0) / 300.0 * r.height())); + assert!(h.state().views[tab].line_editor.is_some(), "the paragraph opens for typing"); + }; + open_in(&mut h, 1); + open_in(&mut h, 0); + let original = h.state().views[0].line_editor.as_ref().unwrap().text.clone(); + h.key_press_modifiers(Modifiers::COMMAND, Key::End); + h.event(egui::Event::Text(" secret".into())); + h.run_steps(2); + assert!(h.state_mut().execute("edit.undo")); + h.run_steps(3); + assert_eq!(h.state().views[0].line_editor.as_ref().unwrap().text, original, "taken back in A"); + // To B and Redo at once: no frame draws B's box in between. + h.state_mut().active = Some(1); + h.state_mut().redo(); + h.run_steps(3); + let s = h.state(); + let b = s.views[1].line_editor.as_ref().map(|ed| ed.text.clone()); + assert!(!b.as_deref().unwrap_or_default().contains("secret"), "A's typing came back in B: {b:?}"); + assert!(!s.session.get(s.views[1].id).unwrap().text_blocks(0)[0].text.contains("secret"), "A's typing reached B's page"); +} + /// Two copies of a document, the same paragraph open in each. The paragraph boxes share one text /// box, and so one Undo history: a box opened on the same text as another tab's didn't clear it. /// Typing in one tab, taken back with Undo, then came back with Redo in the other tab.