From 9f5166f2979692e38e49188902f4795c4514669a Mon Sep 17 00:00:00 2001 From: Tiago Kiel <186439951+TiagoCK@users.noreply.github.com> Date: Sun, 8 Nov 2026 16:10:47 -0500 Subject: [PATCH] fix(edit): spaces a deleted word leaves don't redraw the OCR phrase (refs #3313) Backspacing over a word leaves the spaces either side of it, and the rule that sent spacing to the whole-phrase rewrite took that for spacing someone asked for, so the most ordinary deletion redrew the whole line again. Spaces beside a change are nothing to draw: the words around it stay where the scan has them, with the gap already between them. Only what has to be drawn goes to the whole rewrite: spacing inside the new words, between two words nothing else changed, or any whitespace other than spaces. Co-Authored-By: Claude Opus 5.5 --- crates/edit/src/text_regions/ocr_words.rs | 95 +++++++++++++++++++++-- crates/engine/tests/it/ocr_edit.rs | 38 +++++++-- 2 files changed, 119 insertions(+), 14 deletions(-) diff --git a/crates/edit/src/text_regions/ocr_words.rs b/crates/edit/src/text_regions/ocr_words.rs index c62ef85bf3..e3e32691ac 100644 --- a/crates/edit/src/text_regions/ocr_words.rs +++ b/crates/edit/src/text_regions/ocr_words.rs @@ -24,6 +24,8 @@ const MIN_SQUEEZE: f64 = 0.75; #[derive(Debug, PartialEq)] struct Change { runs: Range, + /// Which of the draft's words replace them. + words: Range, text: String, squeeze: Option, } @@ -47,13 +49,23 @@ fn changes(runs: &[usize], kept_as: &[Option], new: &[&str], redrawn: &mu continue 'plan; } if first_run < run { - out.push(Change { runs: first_run..run, text: new.get(first_new..at).unwrap_or_default().join(" "), squeeze: None }); + out.push(Change { + runs: first_run..run, + words: first_new..at, + text: new.get(first_new..at).unwrap_or_default().join(" "), + squeeze: None, + }); } first_run = run.saturating_add(1); first_new = at.saturating_add(*count); } if first_run < runs.len() { - out.push(Change { runs: first_run..runs.len(), text: new.get(first_new..).unwrap_or_default().join(" "), squeeze: None }); + out.push(Change { + runs: first_run..runs.len(), + words: first_new..new.len(), + text: new.get(first_new..).unwrap_or_default().join(" "), + squeeze: None, + }); } else if first_new < new.len() && let Some(last) = runs.len().checked_sub(1) { @@ -64,6 +76,40 @@ fn changes(runs: &[usize], kept_as: &[Option], new: &[&str], redrawn: &mu } } +/// Each word of `text` as a byte range. +fn word_ranges(text: &str) -> Vec> { + let mut out = Vec::new(); + let mut start = None; + for (at, c) in text.char_indices() { + match (c.is_whitespace(), start) { + (true, Some(from)) => { + out.push(from..at); + start = None; + } + (false, None) => start = Some(at), + _ => {} + } + } + if let Some(from) = start { + out.push(from..text.len()); + } + out +} + +/// Whether `plan` can stand for the spacing in `draft`, whose words are `words`. Spaces beside a +/// change are nothing to draw: Backspacing over a word leaves one either side of where it was, and +/// the words around it stay where the scan has them. Anything else that isn't one space has to be +/// drawn, inside the new words or between two words nothing else changed, which only the whole +/// rewrite can do. +fn spacing_fits(draft: &str, words: &[Range], plan: &[Change]) -> bool { + words.windows(2).enumerate().all(|(index, pair)| { + let [before, after] = pair else { return true }; + let gap = draft.get(before.end..after.start).unwrap_or(" "); + let boundary = index.saturating_add(1); + gap == " " || (gap.chars().all(|c| c == ' ') && plan.iter().any(|change| change.words.start == boundary || change.words.end == boundary)) + }) +} + /// The phrase's stretch `indices` of `lines`, as a paragraph of its own. It keeps the whole /// phrase's size, so what is redrawn is set alike wherever in the phrase it is. fn stretch(phrase: &TextBlock, lines: &[TextLine], indices: &[usize]) -> Result { @@ -235,9 +281,8 @@ pub(crate) fn rewrite_changed_words( let runs: Vec> = members.iter().filter_map(|i| lines.get(*i)).map(|line| line.text.split_whitespace().collect()).collect(); let old: Vec<&str> = runs.iter().flatten().copied().collect(); let new: Vec<&str> = draft.split_whitespace().collect(); - // Words the layer doesn't show apart (a run per letter, say) can't be kept apart, and spacing - // other than one space between words is the draft's to keep, which only the whole rewrite can. - if !phrase.editor_text.split_whitespace().eq(old.iter().copied()) || draft.trim() != new.join(" ") { + // Words the layer doesn't show apart (a run per letter, say) can't be kept apart. + if !phrase.editor_text.split_whitespace().eq(old.iter().copied()) { return None; } // A single word that changes has nothing to keep. With the same words, the plan is empty. @@ -251,6 +296,9 @@ pub(crate) fn rewrite_changed_words( let mut squeezes = BTreeMap::new(); for _ in 0..MAX_TRIES { let mut plan = changes(&counts, &kept_as, &new, &mut redrawn); + if !spacing_fits(draft, &word_ranges(draft), &plan) { + return None; + } for change in &mut plan { change.squeeze = squeezes.get(&change.runs.start).copied(); } @@ -308,19 +356,50 @@ mod tests { assert_eq!(plan(runs, "a x b c"), [(0..1, "a x b".to_owned())], "a word went in between its words"); } + #[test] + fn a_drafts_words_are_found_by_their_bytes() { + assert!(word_ranges("").is_empty() && word_ranges(" ").is_empty()); + assert_eq!(word_ranges("ab é c "), [0..2, 4..6, 7..8]); + } + + /// The plan for `draft` against the words a, b and c, and whether it stands for its spacing. + fn fits(draft: &str) -> bool { + let (old, new): (Vec<&str>, Vec<&str>) = (vec!["a", "b", "c"], draft.split_whitespace().collect()); + let plan = changes(&[1, 1, 1], &kept_words(&old, &new).unwrap(), &new, &mut BTreeSet::new()); + spacing_fits(draft, &word_ranges(draft), &plan) + } + + #[test] + fn spaces_beside_a_change_are_nothing_to_draw() { + assert!(fits("a b c")); + assert!(fits("a b c "), "after the words"); + assert!(fits(" a b c"), "before them"); + assert!(fits("a c"), "either side of a deleted word"); + assert!(fits(" b c"), "where a first word went"); + assert!(fits("a x c"), "either side of a replacement"); + } + + #[test] + fn spacing_that_has_to_be_drawn_doesnt_fit() { + assert!(!fits("a b c"), "between words nothing else changed"); + assert!(!fits("x y c"), "inside the new words"); + assert!(!fits("a\u{3000}x c"), "another kind of space, even beside a change"); + assert!(!fits("a\u{a0}x c")); + } + /// Word boxes 10 wide, with a gap of 4 between them. const BOXES: [[f64; 4]; 3] = [[0.0, 0.0, 10.0, 10.0], [14.0, 0.0, 24.0, 10.0], [28.0, 0.0, 38.0, 10.0]]; fn room_for(right: f64) -> (bool, BTreeMap, BTreeSet) { let (mut squeezes, mut redrawn) = (BTreeMap::new(), BTreeSet::new()); - let change = Change { runs: 0..1, text: "x".to_owned(), squeeze: None }; + let change = Change { runs: 0..1, words: 0..1, text: "x".to_owned(), squeeze: None }; (make_room(&change, right, &BOXES, &mut squeezes, &mut redrawn), squeezes, redrawn) } #[test] fn a_word_that_fits_is_left_alone() { assert_eq!(room_for(12.0), (false, BTreeMap::new(), BTreeSet::new()), "it ends in the gap's first half"); - let last = Change { runs: 2..3, text: "x".to_owned(), squeeze: None }; + let last = Change { runs: 2..3, words: 2..3, text: "x".to_owned(), squeeze: None }; assert!(!make_room(&last, 900.0, &BOXES, &mut BTreeMap::new(), &mut BTreeSet::new()), "the last word has the page to grow into"); } @@ -343,6 +422,6 @@ mod tests { let (counts, old, new) = ([1, 1, 1], ["a", "b", "c"], ["x", "b", "c"]); let mut redrawn = BTreeSet::from([1]); let planned = changes(&counts, &kept_words(&old, &new).unwrap(), &new, &mut redrawn); - assert_eq!(planned, [Change { runs: 0..2, text: "x b".to_owned(), squeeze: None }]); + assert_eq!(planned, [Change { runs: 0..2, words: 0..2, text: "x b".to_owned(), squeeze: None }]); } } diff --git a/crates/engine/tests/it/ocr_edit.rs b/crates/engine/tests/it/ocr_edit.rs index e425a9722d..e636088bf5 100644 --- a/crates/engine/tests/it/ocr_edit.rs +++ b/crates/engine/tests/it/ocr_edit.rs @@ -491,14 +491,40 @@ fn a_cover_stops_at_the_word_beside_it() { assert!(smith < 0.02, "Smith keeps its ink: {smith}"); } -/// Spacing the word plan can't hold goes to the rewrite that can: it isn't quietly dropped. +/// Deleting a word with Backspace leaves the spaces either side of it. That isn't spacing anyone +/// asked for: the words around it stay where the scan has them, with the gap already between them. #[test] -fn spacing_inside_a_draft_is_kept_by_redrawing_the_phrase() { +fn the_spaces_a_deleted_word_leaves_keep_the_scan() { + for (draft, scan, drawn) in [ + ("John Example", vec!["John", "Example"], vec![]), + (" John Example", vec!["John", "Example"], vec![]), + ("John Smyth Example", vec!["John", "Example"], vec!["Smyth"]), + ] { + let (mut s, id) = uneven_phrase(); + s.apply(id, Edit::EditTextBlock { page: 0, block: 0, text: draft.into(), style: Default::default() }).unwrap(); + let (kept, redrawn) = scan_and_drawn(&s, id); + assert_eq!( + (kept.iter().map(String::as_str).collect::>(), redrawn.iter().map(String::as_str).collect::>()), + (scan, drawn), + "{draft:?}" + ); + } +} + +/// Spacing that has to be drawn goes to the rewrite that can draw it: inside new words, or typed +/// between words nothing else changed. It isn't quietly dropped. +#[test] +fn spacing_that_has_to_be_drawn_redraws_the_phrase() { + for draft in ["Jon Smyth Example", "John Smith Example"] { + let (mut s, id) = uneven_phrase(); + s.apply(id, Edit::EditTextBlock { page: 0, block: 0, text: draft.into(), style: Default::default() }).unwrap(); + let (scan, _) = scan_and_drawn(&s, id); + assert!(scan.is_empty(), "{draft:?} keeps nothing as the scan: {scan:?}"); + } let (mut s, id) = uneven_phrase(); - s.apply(id, Edit::EditTextBlock { page: 0, block: 0, text: "John Smyth Example".into(), style: Default::default() }).unwrap(); - let (scan, drawn) = scan_and_drawn(&s, id); - assert!(scan.is_empty(), "{scan:?}"); - assert!(drawn.join(" ").contains("John Smyth"), "the typed spaces are there: {drawn:?}"); + s.apply(id, Edit::EditTextBlock { page: 0, block: 0, text: "Jon Smyth Example".into(), style: Default::default() }).unwrap(); + let (_, drawn) = scan_and_drawn(&s, id); + assert!(drawn.join("").contains("Jon Smy"), "the typed spaces are there: {drawn:?}"); } /// A layer whose words are already narrowed (50 Tz): squeezing a replacement narrows it further,