Merge pull request #7277 from dotCooCoo/fix/break-text-thread-lookup

Artwork Cleanup: Break Text and Remove Breaks read the text threads once

crates/engine/src/cmd/addons/cleanup/text.rs

@@ -206,11 +206,31 @@ struct Source {
     text: String,
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: how many times [`thread_index`] went through the document's threads on this thread.
+    static THREAD_SCANS: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
+}
+
+/// Each object the document's text threads list, with the index of the first thread that lists it.
+fn thread_index(d: &Document) -> std::collections::HashMap<NodeId, usize> {
+    #[cfg(test)]
+    THREAD_SCANS.with(|c| c.set(c.get() + 1));
+    let mut out = std::collections::HashMap::new();
+    for (i, thread) in d.text_threads.iter().enumerate() {
+        for id in thread {
+            out.entry(*id).or_insert(i);
+        }
+    }
+    out
+}
+
 impl Source {
-    /// Horizontal point or area type that isn't threaded and whose text all fits; None otherwise.
-    fn of(d: &Document, id: NodeId) -> Option<Self> {
+    /// Horizontal point or area type that isn't threaded (`threaded` false) and whose text all
+    /// fits; None otherwise.
+    fn of(d: &Document, id: NodeId, threaded: bool) -> Option<Self> {
         let t = text_of(d, id)?;
-        if t.vertical || matches!(t.kind, TextKind::OnPath { .. }) || d.text_threads.iter().any(|th| th.contains(&id)) {
+        if t.vertical || matches!(t.kind, TextKind::OnPath { .. }) || threaded {
             return None;
         }
         let lay = layout(t);
@@ -376,8 +396,10 @@ fn break_text(s: &mut Session, p: &Value) -> Result<Value> {
     };
     let d = &s.doc()?.doc;
     let (mut items, mut skipped, mut count) = (vec![], vec![], 0);
-    for id in texts_under(d, &checked_ids_or_selection(s, p, C)?)? {
-        let (Some(src), Some(node)) = (Source::of(d, id), d.node(id)) else {
+    let ids = texts_under(d, &checked_ids_or_selection(s, p, C)?)?;
+    let threads = thread_index(d);
+    for id in ids {
+        let (Some(src), Some(node)) = (Source::of(d, id, threads.contains_key(&id)), d.node(id)) else {
             skipped.push(id);
             continue;
         };
@@ -405,7 +427,8 @@ fn split_text(s: &mut Session, _: &Value) -> Result<Value> {
         return Err(EngineError::Other("Click in type with a Type tool to split it".into()));
     };
     let d = &s.doc()?.doc;
-    let (Some(src), Some(node)) = (Source::of(d, id), d.node(id).filter(|_| d.is_editable(id))) else {
+    let threaded = d.text_threads.iter().any(|th| th.contains(&id));
+    let (Some(src), Some(node)) = (Source::of(d, id, threaded), d.node(id).filter(|_| d.is_editable(id))) else {
         return Err(EngineError::Other("Select horizontal point or area type whose text fits".into()));
     };
     let text = &src.text;
@@ -821,13 +844,17 @@ fn remove_breaks(s: &mut Session, p: &Value) -> Result<Value> {
             let ids = texts_under(d, &checked_ids_or_selection(s, p, C)?)?;
             // A threaded story whose frames are all among them is read as one text, once.
             let named: std::collections::HashSet<NodeId> = ids.iter().copied().collect();
+            let threads = thread_index(d);
             let mut done = std::collections::HashSet::new();
             let mut out: Vec<(NodeId, Vec<(usize, bool)>)> = vec![];
             for &id in &ids {
                 if !done.insert(id) {
                     continue;
                 }
-                let story = crate::cmd::threads::story_of(d, id);
+                let story = match threads.get(&id).and_then(|&i| d.text_threads.get(i)) {
+                    Some(thread) => crate::cmd::threads::story_in(d, thread, id),
+                    None => vec![],
+                };
                 if !story.is_empty() && story.iter().all(|f| named.contains(f)) {
                     done.extend(story.iter().copied());
                     out.extend(story_cuts(d, &story, brk));

crates/engine/src/cmd/addons/cleanup/text/tests.rs

@@ -158,6 +158,62 @@ fn break_text_refuses_what_it_cannot_break() {
     assert!(matches!(r, Err(EngineError::BadParams { .. })), "{r:?}");
 }
 
+/// Two area frames threaded together, with `story` flowing through them → (first, second).
+fn threaded(s: &mut Session, story: &str) -> (NodeId, NodeId) {
+    let a = text(s, json!({"x": 10, "y": 300, "text": story, "size": 12, "area": {"width": 150, "height": 60}}));
+    let b = NodeId(s.execute("shape.rectangle", &json!({"x": 200, "y": 300, "width": 150, "height": 60})).unwrap()["id"].as_u64().unwrap());
+    select(s, &[a, b]);
+    s.execute("text.thread.create", &json!({})).unwrap();
+    (a, b)
+}
+
+fn thread_scans() -> usize {
+    super::THREAD_SCANS.with(|c| c.get())
+}
+
+/// Threaded frames in the selection stay as they are and are reported as skipped; the other type
+/// is broken. The document's threads are gone through once for the whole selection, not once
+/// per object.
+#[test]
+fn break_text_skips_threaded_frames_and_reads_the_threads_once() {
+    let mut s = session();
+    let (a, b) = threaded(&mut s, "Threaded story text");
+    let mut plain = vec![];
+    for i in 0..6 {
+        plain.push(text(&mut s, json!({"x": 20, "y": 30 + 30 * i, "text": "Two words", "size": 14})));
+    }
+    let was = glyphs(&s, &plain);
+    let frames = (obj(&s, a), obj(&s, b));
+    let before = doc_json(&s);
+    let steps = undo_steps(&s);
+    let mut all = vec![a, b];
+    all.extend(&plain);
+    select(&mut s, &all);
+    let scans = thread_scans();
+    let r = s.execute("addon.cleanup.breakText", &json!({"by": "words"})).unwrap();
+    assert_eq!(thread_scans() - scans, 1, "the threads are gone through once");
+    assert_eq!(ids(&json!({"ids": r["skipped"]})), [a, b]);
+    assert_eq!((obj(&s, a), obj(&s, b)), frames, "the threaded frames stay as they are");
+    let pieces = ids(&r);
+    assert_eq!(pieces.len(), 12);
+    assert_eq!(s.doc().unwrap().selection.objects.len(), 12, "the pieces are selected");
+    assert_same_places(&was, &glyphs(&s, &pieces));
+    assert_eq!(undo_steps(&s), steps + 1);
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert_eq!(doc_json(&s), before);
+}
+
+#[test]
+fn split_text_refuses_a_threaded_frame() {
+    let mut s = session();
+    let (a, _) = threaded(&mut s, "Threaded story text");
+    select(&mut s, &[a]);
+    edit_text(&mut s, a, 9, 9);
+    let before = doc_json(&s);
+    assert!(other(s.execute("addon.cleanup.splitText", &json!({}))).contains("point or area type"));
+    assert_eq!(doc_json(&s), before);
+}
+
 #[test]
 fn combine_puts_glyph_pieces_back_together_where_they_were() {
     let mut s = session();
@@ -300,6 +356,29 @@ fn remove_breaks_with_ids_null_in_the_characters_a_type_tool_selected() {
     assert_eq!(obj(&s, id).plain_text(), "a b\nc");
 }
 
+/// Remove Breaks reads a threaded story whose frames are all selected as one text and each other
+/// object on its own, going through the document's threads once for the whole selection.
+#[test]
+fn remove_breaks_reads_the_threads_once() {
+    let mut s = session();
+    let (a, b) = threaded(&mut s, "one\ntwo");
+    let mut plain = vec![];
+    for i in 0..6 {
+        plain.push(text(&mut s, json!({"x": 20, "y": 30 + 30 * i, "text": "a\nb", "size": 12})));
+    }
+    let mut all = vec![a, b];
+    all.extend(&plain);
+    select(&mut s, &all);
+    let scans = thread_scans();
+    let r = s.execute("addon.cleanup.removeBreaks", &json!({"kind": "paragraph"})).unwrap();
+    assert_eq!(thread_scans() - scans, 1, "the threads are gone through once");
+    assert_eq!(r["removed"], 7);
+    assert_eq!(obj(&s, a).plain_text(), "one two");
+    for id in plain {
+        assert_eq!(obj(&s, id).plain_text(), "a b");
+    }
+}
+
 #[test]
 fn split_at_the_cursor_and_around_a_selection() {
     for (anchor, caret, want) in [(6, 6, &["Split", "here please"][..]), (6, 10, &["Split", "here", "please"][..])] {

crates/engine/src/cmd/threads.rs

@@ -399,6 +399,11 @@ pub(crate) fn mark_continued(doc: &mut Document) {
 /// path; empty when `id` is in no thread or the thread has fewer than two such frames.
 pub(crate) fn story_of(d: &Document, id: NodeId) -> Vec<NodeId> {
     let Some(thread) = d.text_threads.iter().find(|t| t.contains(&id)) else { return vec![] };
+    story_in(d, thread, id)
+}
+
+/// [`story_of`] for `id` in `thread`, the first of the document's threads that lists it.
+pub(crate) fn story_in(d: &Document, thread: &[NodeId], id: NodeId) -> Vec<NodeId> {
     let frames: Vec<NodeId> = thread.iter().copied().filter(|f| text_of(d, *f).is_some()).collect();
     if frames.len() >= 2 && frames.contains(&id) { frames } else { vec![] }
 }