From 7679f6b5562174cd1e9383ba84b2c2e4092f0706 Mon Sep 17 00:00:00 2001 From: RiceDee <345114586+RiceDee@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:02:30 +1100 Subject: [PATCH] fix(edit): keep column frames local and clear stale history selections --- crates/edit/src/tests.rs | 44 ++++++++++++++++++++++++ crates/edit/src/text/rewrite.rs | 2 +- crates/edit/src/text_regions/grouping.rs | 37 ++++++++++++-------- crates/ui-egui/src/editing.rs | 4 +++ crates/ui-egui/tests/it/content.rs | 11 ++++++ crates/ui-egui/tests/it/editing.rs | 11 ++++++ 6 files changed, 93 insertions(+), 16 deletions(-) diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 0859560620..96f71da36b 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -7259,6 +7259,50 @@ fn a_lone_row_of_two_styles_aligns_across_its_column() { assert!((title(&doc)[0] - centred[0]).abs() < 0.5, "centring again leaves it where it is: {:?} vs {centred:?}", title(&doc)); } +#[test] +fn a_titles_column_ignores_distant_full_width_headers_and_footers() { + for (title_x, column_prefix) in [(95, "The first"), (365, "The second")] { + for mixed in [false, true] { + for title_y in [710, 580] { + let title = if mixed { "(Big ) Tj 1 0 0 rg (Title) Tj" } else { "(Title) Tj" }; + let name = if mixed { "Big Title" } else { "Title" }; + let mut doc = text_page(&format!( + "BT /F1 10 Tf 12 TL 50 780 Td (A page-wide header extending well beyond either body column on this page and continuing all the way to the right margin.) Tj T* (A repeated page-wide heading across both columns and continuing to the right margin of the page.) Tj ET \ + BT /F1 18 Tf 1 0 0 1 {title_x} {title_y} Tm {title} ET \ + BT 0 g /F1 10 Tf 12 TL 50 660 Td (The first column runs to here.) Tj T* (A second row of it.) Tj ET \ + BT /F1 10 Tf 12 TL 320 660 Td (The second column runs to here.) Tj T* (Another row.) Tj ET \ + BT /F1 10 Tf 50 40 Td (A page-wide footer extending well beyond either body column on this page and continuing all the way to the right margin.) Tj ET" + )); + let rect = |doc: &Document, prefix: &str| { + let blocks = text::text_blocks(doc, 0).unwrap(); + blocks.iter().find(|b| b.text.starts_with(prefix)).unwrap_or_else(|| panic!("missing {prefix:?} in {blocks:?}")).rect + }; + let column = rect(&doc, column_prefix); + let header = rect(&doc, "A page-wide header"); + let footer = rect(&doc, "A page-wide footer"); + assert!(header[2] > column[2] + 20.0 && footer[2] > column[2] + 20.0); + for align in [added::Align::Center, added::Align::Center, added::Align::Right, added::Align::Left] { + let blocks = text::text_blocks(&doc, 0).unwrap(); + let (index, block) = blocks.iter().enumerate().find(|(_, b)| b.text == name).unwrap(); + let style = text::BlockStyle { aligns: vec![Some(align)], ..Default::default() }; + text::rewrite_block(&mut doc, 0, index, Some(&block.editor_text), &style).unwrap(); + doc = reopen(&doc); + let title = rect(&doc, name); + let error = match align { + added::Align::Center => ((title[0] + title[2]) - (column[0] + column[2])).abs() / 2.0, + added::Align::Right => (title[2] - column[2]).abs(), + _ => (title[0] - column[0]).abs(), + }; + assert!(error < 1.0, "{align:?}, mixed={mixed}, title_y={title_y}: {title:?} must use {column:?}"); + assert_eq!(rect(&doc, "A page-wide header"), header); + assert_eq!(rect(&doc, "A page-wide footer"), footer); + assert_eq!(rect(&doc, column_prefix), column); + } + } + } + } +} + /// How each glyph-showing operator on page 0 is painted, read the way a renderer reads the page: /// its text, `Tr`, stroke colour operators, line width (from `w` or an ExtGState's `/LW`) and /// the ExtGStates selected before it, following `q`/`Q`. diff --git a/crates/edit/src/text/rewrite.rs b/crates/edit/src/text/rewrite.rs index 77d3d8b2a3..5cb5f2a321 100644 --- a/crates/edit/src/text/rewrite.rs +++ b/crates/edit/src/text/rewrite.rs @@ -221,7 +221,7 @@ pub(crate) fn rewrite_block_from( // An alignment for a one-row paragraph is laid out across the column it sits in, as a // paragraph of that width moved to its left edge. let framed; - let style = match alignment_frame(&lines, &b, style) { + let style = match alignment_frame(&lines, &blocks, &b, style) { Some([left, right]) => { framed = BlockStyle { width: Some(right - left), offset: Some([left - b.rect[0], 0.0]), ..style.clone() }; &framed diff --git a/crates/edit/src/text_regions/grouping.rs b/crates/edit/src/text_regions/grouping.rs index fce75880d3..4f2e8371a3 100644 --- a/crates/edit/src/text_regions/grouping.rs +++ b/crates/edit/src/text_regions/grouping.rs @@ -28,12 +28,11 @@ fn frame_row_rect(row: &[usize], lines: &[TextLine]) -> Option<[f64; 4]> { /// The left edge the rest of the page's text sits on, ignoring the rows of `block`. A single row /// that does not share it is not merely an indented or full-width left-aligned line, so its own -/// centre is the only remaining explanation for where it sits. With `over`, only rows reaching -/// across that span count: the edge of the column under it, however many rows another column has. -fn neighbour_left(lines: &[TextLine], block: &TextBlock, over: Option<[f64; 2]>) -> Option { +/// centre is the only remaining explanation for where it sits. +fn neighbour_left(lines: &[TextLine], block: &TextBlock) -> Option { let mut edges: HashMap = HashMap::new(); for (i, line) in lines.iter().enumerate() { - if block.lines.contains(&i) || over.is_some_and(|[left, right]| line.rect[2] <= left || line.rect[0] >= right) { + if block.lines.contains(&i) { continue; } // Quarter-point buckets: equal edges found through different runs still agree. @@ -44,25 +43,33 @@ fn neighbour_left(lines: &[TextLine], block: &TextBlock, over: Option<[f64; 2]>) } /// Where an alignment picked for a one-row paragraph is measured, as `[left, right]`: the column of -/// text it sits in (the left edge the text under or over it uses, to the furthest right those rows -/// reach) when that column holds it, otherwise its own box. A lone row's own box is no frame +/// text it sits in (the nearest overlapping paragraph's left edge, to the furthest right +/// its rows reach) when that column holds it, otherwise its own box. A lone row's own box is no frame /// to align in: left-aligning a title centred on the page left it where it was, and since a single /// row grows to the page's margin, centring it again moved it right each time. `None` when no /// alignment is asked for, or when the paragraph is several rows, turned, or given a width or a /// move of its own. -pub(crate) fn alignment_frame(lines: &[TextLine], block: &TextBlock, style: &BlockStyle) -> Option<[f64; 2]> { +pub(crate) fn alignment_frame(lines: &[TextLine], blocks: &[TextBlock], block: &TextBlock, style: &BlockStyle) -> Option<[f64; 2]> { let asked = style.align.is_some() || style.aligns.iter().any(Option::is_some); if !asked || style.width.is_some() || style.offset.is_some() || block.rows.len() != 1 || block.rotation.abs() > 0.01 { return None; } let own = [block.rect[0], block.rect[2]]; let tolerance = block.size.max(1.0) * 0.5; - let Some(left) = neighbour_left(lines, block, Some(own)) else { return Some(own) }; - let right = lines + let gap = |other: &TextBlock| (other.rect[1] - block.rect[3]).max(block.rect[1] - other.rect[3]).max(0.0); + // Page-wide headers and footers sharing this margin aren't part of the local column. + let neighbour = blocks .iter() - .enumerate() - .filter(|(i, line)| !block.lines.contains(i) && (line.rect[0] - left).abs() < tolerance) - .map(|(_, line)| line.rect[2]) + .filter(|other| other.lines.iter().all(|i| !block.lines.contains(i)) && other.rect[2] > own[0] && other.rect[0] < own[1]) + .min_by(|a, b| gap(a).total_cmp(&gap(b))); + let Some(neighbour) = neighbour else { return Some(own) }; + let left = neighbour.rect[0]; + let right = neighbour + .lines + .iter() + .filter_map(|i| lines.get(*i)) + .filter(|line| (line.rect[0] - left).abs() < tolerance) + .map(|line| line.rect[2]) .fold(f64::NEG_INFINITY, f64::max); let holds = right.is_finite() && right - left > own[1] - own[0] && own[0] >= left - tolerance && own[1] <= right + tolerance; Some(if holds { [left, right] } else { own }) @@ -84,9 +91,9 @@ pub(crate) fn is_centred(block: &TextBlock, lines: &[TextLine], page_centre: f64 return left_span > size * 0.5 && block.rows.iter().all(|row| row_rect(row, lines).is_some_and(|r| ((r[0] + r[2]) / 2.0 - centre).abs() < size)); } - lefts.first().is_some_and(|left| { - (centre - page_centre).abs() < size && neighbour_left(lines, block, None).is_none_or(|edge| (*left - edge).abs() > size * 0.5) - }) + lefts + .first() + .is_some_and(|left| (centre - page_centre).abs() < size && neighbour_left(lines, block).is_none_or(|edge| (*left - edge).abs() > size * 0.5)) } pub(crate) fn group_blocks(lines: &[TextLine]) -> Vec { diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index fc8d637a70..17366df250 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -220,6 +220,7 @@ impl PdfCraftApp { ed.claim_typing(&ctx, doc); let Some(text) = typing_step(&ctx, crate::edit_text_ui::input_id(), &ed.text, true) else { return false }; let before = std::mem::replace(&mut ed.text, text); + ed.selection = None; ed.follow_typed_rows(&ctx, &before); return true; } @@ -227,6 +228,7 @@ impl PdfCraftApp { d.claim_typing(&ctx, doc); let Some(text) = typing_step(&ctx, crate::content_ui::input_id(), &d.text, true) else { return false }; let before = std::mem::replace(&mut d.text, text); + d.selection = None; crate::edit_text_ui::follow_typing(&ctx, crate::content_ui::input_id(), &before, &d.text, &mut [&mut d.style.aligns]); return true; } @@ -253,6 +255,7 @@ impl PdfCraftApp { 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() }; let before = std::mem::replace(&mut ed.text, text); + ed.selection = None; ed.follow_typed_rows(&ctx, &before); return true; } @@ -260,6 +263,7 @@ impl PdfCraftApp { 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() }; let before = std::mem::replace(&mut d.text, text); + d.selection = None; crate::edit_text_ui::follow_typing(&ctx, crate::content_ui::input_id(), &before, &d.text, &mut [&mut d.style.aligns]); return true; } diff --git a/crates/ui-egui/tests/it/content.rs b/crates/ui-egui/tests/it/content.rs index 69597591db..1dadc6adaa 100644 --- a/crates/ui-egui/tests/it/content.rs +++ b/crates/ui-egui/tests/it/content.rs @@ -1096,7 +1096,13 @@ fn undo_from_the_menu_takes_back_retyping_in_the_added_text_box() { h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::End); h.event(egui::Event::Text(" twice".into())); h.run_steps(2); + h.key_press_modifiers(egui::Modifiers::SHIFT, egui::Key::Home); + h.run_steps(2); + assert!(h.state().views[0].content.draft.as_ref().unwrap().selection.is_some()); + h.event(egui::Event::PointerMoved(egui::pos2(10.0, 10.0))); + h.ctx.memory_mut(|m| m.request_focus(egui::Id::unique("test-menu"))); assert!(h.state_mut().execute("edit.undo")); + assert_eq!(h.state().views[0].content.draft.as_ref().unwrap().selection, None, "menu history clears the old byte range before the next frame"); h.run_steps(3); assert_eq!(h.state().views[0].content.draft.as_ref().map(|d| d.text.as_str()), Some("Reviewed"), "Undo takes it back in the box"); let s = h.state(); @@ -1123,7 +1129,12 @@ fn redo_from_the_menu_is_enabled_for_the_added_text_boxs_own_typing() { h.run_steps(3); assert_eq!(h.state().views[0].content.draft.as_ref().map(|d| d.text.as_str()), Some("Reviewed"), "Undo takes it back in the box"); assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().can_redo(), None, "the document has nothing to redo"); + // A range retained while the unfocused editor still showed its previous text. + h.state_mut().views[0].content.draft.as_mut().unwrap().selection = Some(0..1); + h.event(egui::Event::PointerMoved(egui::pos2(10.0, 10.0))); + h.ctx.memory_mut(|m| m.request_focus(egui::Id::unique("test-menu"))); assert!(h.state_mut().execute("edit.redo"), "Redo is enabled for the box's own step"); + assert_eq!(h.state().views[0].content.draft.as_ref().unwrap().selection, None, "menu history clears the old byte range before the next frame"); h.run_steps(3); assert_eq!(h.state().views[0].content.draft.as_ref().map(|d| d.text.as_str()), Some("Reviewed twice"), "Redo puts it back"); } diff --git a/crates/ui-egui/tests/it/editing.rs b/crates/ui-egui/tests/it/editing.rs index 03154369ab..e69eb63d85 100644 --- a/crates/ui-egui/tests/it/editing.rs +++ b/crates/ui-egui/tests/it/editing.rs @@ -6742,7 +6742,13 @@ fn undo_from_the_menu_takes_back_typing_in_the_box() { h.key_press_modifiers(Modifiers::COMMAND, Key::End); h.event(egui::Event::Text(" more".into())); h.run_steps(2); + h.key_press_modifiers(Modifiers::SHIFT, Key::Home); + h.run_steps(2); + assert!(h.state().views[0].line_editor.as_ref().unwrap().selection.is_some()); + h.event(egui::Event::PointerMoved(egui::pos2(10.0, 10.0))); + h.ctx.memory_mut(|m| m.request_focus(egui::Id::unique("test-menu"))); assert!(h.state_mut().execute("edit.undo"), "Undo is enabled for the box's typing"); + assert_eq!(h.state().views[0].line_editor.as_ref().unwrap().selection, None, "menu history clears the old byte range before the next frame"); h.run_steps(3); let s = h.state(); let ed = s.views[0].line_editor.as_ref().expect("the box stays open"); @@ -6769,7 +6775,12 @@ fn redo_from_the_menu_is_enabled_for_the_boxs_own_typing() { h.run_steps(3); assert_eq!(h.state().views[0].line_editor.as_ref().unwrap().text, original, "the typing is taken back"); assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().can_redo(), None, "the document has nothing to redo"); + // A range retained while the unfocused editor still showed its previous text. + h.state_mut().views[0].line_editor.as_mut().unwrap().selection = Some(0..1); + h.event(egui::Event::PointerMoved(egui::pos2(10.0, 10.0))); + h.ctx.memory_mut(|m| m.request_focus(egui::Id::unique("test-menu"))); assert!(h.state_mut().execute("edit.redo"), "Redo is enabled for the box's own step"); + assert_eq!(h.state().views[0].line_editor.as_ref().unwrap().selection, None, "menu history clears the old byte range before the next frame"); h.run_steps(3); assert_eq!(h.state().views[0].line_editor.as_ref().expect("still open").text, typed, "Redo puts the typing back"); }