From 078b8bdcd2b1c87ada6e299a47e1d3dc37d145d7 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 5 Nov 2026 12:15:32 +0500 Subject: [PATCH 01/17] Data Merge: a field in a table cell goes into the cell and is filled (#2315) With the insertion point in a table cell, data.placeholder.add took the selection's offsets, which are the cell's, as offsets into the table's own story, so the marker landed outside the table. The fill only read each story's own text, so a field in a cell, inserted or converted from text, was left as its marker. A field inserted in a cell now goes into the cell's text as its marker, and the fill replaces the markers in the table cells and footnotes of the stories it fills. --- crates/engine/src/cmd/datamerge.rs | 14 +++++++ crates/engine/src/cmd/datamerge/fill.rs | 39 ++++++++++++++++++ crates/engine/src/cmd/datamerge/tests.rs | 52 ++++++++++++++++++++++++ 3 files changed, 105 insertions(+) diff --git a/crates/engine/src/cmd/datamerge.rs b/crates/engine/src/cmd/datamerge.rs index 4d3cd5dd6..e834127fd 100644 --- a/crates/engine/src/cmd/datamerge.rs +++ b/crates/engine/src/cmd/datamerge.rs @@ -482,6 +482,20 @@ fn add_placeholder(d: &mut Document, sel: &Selection, p: &Value, name: &str) -> if matches!(role, PlaceholderRole::Image | PlaceholderRole::Qr) { return Err(bad("data.placeholder.add", "image and qr placeholders bind to a frame")); } + // The insertion point is in a table cell: the field goes into the cell's text as its + // marker, which the merge fills as it fills a typed one. A cell's text is a story of its + // own that a placeholder can't name, and the selection's offsets are the cell's, so they + // used to put the marker in the table's story instead (#2315). + if !explicit_story && let Some(t) = sel.text.filter(|t| t.cell.is_some()) { + if role != PlaceholderRole::Text { + return Err(bad("data.placeholder.add", "a hyperlink field can't go in a table cell; insert it as text")); + } + let marker = format!("<<{name}>>"); + let st = d.text_story_mut(t.story, t.cell).ok_or_else(|| bad("data.placeholder.add", "no table cell there"))?; + let at = designcraft_doc::floor_char_boundary(&st.text, t.range().start.min(st.text.len())); + st.replace(at..at, &marker); + return Ok(json!({"cell": true, "marker": marker})); + } let (story, at, end) = text_target(p, sel).ok_or_else(|| bad("data.placeholder.add", "select text or a frame"))?; if d.story(story).is_none() { return Err(bad("data.placeholder.add", "no story")); diff --git a/crates/engine/src/cmd/datamerge/fill.rs b/crates/engine/src/cmd/datamerge/fill.rs index 825179ac9..288dbf44f 100644 --- a/crates/engine/src/cmd/datamerge/fill.rs +++ b/crates/engine/src/cmd/datamerge/fill.rs @@ -228,6 +228,7 @@ pub fn fill_row( let Pending { mut reps, links, parent, mut report } = pending; typed_reps(doc, fields, row, maps, &placeholders, &mut reps); apply_reps(doc, &reps); + fill_nested_markers(doc, fields, row, maps); for (anchor, dest, field) in links { let anchor = match anchor { PlaceholderAnchor::Text { story, start, end } => { @@ -331,6 +332,44 @@ fn typed_reps( } } +/// The `<>` markers in the table cells and footnotes of the stories being filled. A cell's +/// or a footnote's text is a story of its own inside its story, which no placeholder can name and +/// `typed_reps` doesn't read, so a field in a table was left as its marker (#2315). Only a table +/// or note that holds a marker is copied. +fn fill_nested_markers(doc: &mut designcraft_doc::Document, fields: &[DataField], row: &[String], maps: &Maps) { + let fill = |st: &mut Story| { + let found: Vec<(usize, usize, String)> = + find_markers(&st.text).into_iter().filter(|(_, _, name)| fields.iter().any(|f| f.name == *name)).collect(); + for (start, end, name) in found.into_iter().rev() { + st.replace(start..end, cell_of(fields, row, &name)); + } + }; + let has = |st: &Story| find_markers(&st.text).iter().any(|(_, _, name)| fields.iter().any(|f| f.name == *name)); + let mut mapped: Vec = maps.stories.values().copied().collect(); + mapped.sort(); + mapped.dedup(); + for sid in mapped { + let Some(st) = doc.story_mut(sid) else { continue }; + for t in st.tables.values_mut() { + if t.cells.iter().any(|c| nested_has(&c.text, &has)) { + for c in &mut Arc::make_mut(t).cells { + c.text.for_each_text_mut(&mut |x| fill(x)); + } + } + } + for n in &mut st.notes { + if nested_has(&n.text, &has) { + Arc::make_mut(n).text.for_each_text_mut(&mut |x| fill(x)); + } + } + } +} + +/// Whether `st` or a text inside it (its table cells and notes) satisfies `has`. +fn nested_has(st: &Story, has: &impl Fn(&Story) -> bool) -> bool { + has(st) || st.tables.values().any(|t| t.cells.iter().any(|c| nested_has(&c.text, has))) || st.notes.iter().any(|n| nested_has(&n.text, has)) +} + fn find_markers(text: &str) -> Vec<(usize, usize, String)> { let mut out = Vec::new(); let mut i = 0; diff --git a/crates/engine/src/cmd/datamerge/tests.rs b/crates/engine/src/cmd/datamerge/tests.rs index e2c76a90f..66eaea7de 100644 --- a/crates/engine/src/cmd/datamerge/tests.rs +++ b/crates/engine/src/cmd/datamerge/tests.rs @@ -1285,3 +1285,55 @@ fn merged_records_give_anchored_objects_their_own_ids() { doc.check().unwrap(); designcraft_format::load(&designcraft_format::save(doc).unwrap()).unwrap(); } + +/// The cell texts of the first table in the active document's first story that has one. +fn cell_texts(s: &Session) -> Vec { + let doc = &s.documents()[s.active_index().unwrap()].doc; + let t = doc.stories.values().find_map(|st| st.tables.values().next().cloned()).expect("a table"); + t.cells.iter().map(|c| c.text.text.clone()).collect() +} + +/// A field inserted with the insertion point in a table cell goes into that cell, and the +/// preview and the merge fill it. The cell's offsets used to put the marker in the table's own +/// story, outside the table, and the merge never read a cell's text (#2315). +#[test] +fn a_field_inserted_in_a_table_cell_goes_there_and_is_filled() { + let mut s = Session::new(); + new_doc(&mut s, 1); + select_csv(&mut s, "Name,City\nAda,London\nGrace,Arlington\n"); + let made = text_frame(&mut s, [36.0, 36.0, 400.0, 200.0], "Who\tWhere", None); + exec(&mut s, "text.select", json!({"story": made["story"], "anchor": 0, "focus": 9})); + exec(&mut s, "table.convertFromText", json!({"columnSeparator": "tab"})); + let story_text = |s: &Session| s.documents()[s.active_index().unwrap()].doc.stories.values().next().unwrap().text.clone(); + let before = story_text(&s); + exec(&mut s, "table.select", json!({"story": made["story"], "rows": [0, 0], "cols": [1, 1]})); + let r = exec(&mut s, "data.placeholder.add", json!({"field": "City"})); + assert_eq!(r["marker"], "<>", "{r}"); + assert_eq!(cell_texts(&s), ["Who", "<>Where"], "the marker is in the cell with the caret"); + assert_eq!(story_text(&s), before, "and not in the table's story"); + + exec(&mut s, "data.preview", json!({"record": 2})); + assert_eq!(cell_texts(&s), ["Who", "ArlingtonWhere"], "the preview fills the cell"); + exec(&mut s, "data.preview.stop", json!({})); + assert_eq!(cell_texts(&s), ["Who", "<>Where"], "stopping it puts the marker back"); + exec(&mut s, "data.merge", json!({})); + let merged = &s.documents()[s.active_index().unwrap()].doc; + let mut cities: Vec = + merged.stories.values().flat_map(|st| st.tables.values().map(|t| t.cells[1].text.text.clone()).collect::>()).collect(); + cities.sort(); + assert_eq!(cities, ["ArlingtonWhere", "LondonWhere"], "one table per record, filled"); +} + +/// A field typed (or inserted) in text that is then converted to a table is filled in its cell. +#[test] +fn a_field_converted_into_a_table_is_filled_in_its_cell() { + let mut s = Session::new(); + new_doc(&mut s, 1); + select_csv(&mut s, "Name,City\nAda,London\n"); + let made = text_frame(&mut s, [36.0, 36.0, 400.0, 200.0], "Name\t<>", None); + exec(&mut s, "text.select", json!({"story": made["story"], "anchor": 0, "focus": 13})); + exec(&mut s, "table.convertFromText", json!({"columnSeparator": "tab"})); + assert_eq!(cell_texts(&s), ["Name", "<>"]); + exec(&mut s, "data.preview", json!({"record": 1})); + assert_eq!(cell_texts(&s), ["Name", "Ada"], "the field is filled in its cell"); +} From 3eb83555820ebaf8df105e3284b273a619234b4e Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Thu, 5 Nov 2026 07:42:43 +0000 Subject: [PATCH 02/17] Apply review fixes - crates/engine/src/cmd/datamerge/fill.rs --- crates/engine/src/cmd/datamerge/fill.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/engine/src/cmd/datamerge/fill.rs b/crates/engine/src/cmd/datamerge/fill.rs index 288dbf44f..fdde21825 100644 --- a/crates/engine/src/cmd/datamerge/fill.rs +++ b/crates/engine/src/cmd/datamerge/fill.rs @@ -349,6 +349,11 @@ fn fill_nested_markers(doc: &mut designcraft_doc::Document, fields: &[DataField] mapped.sort(); mapped.dedup(); for sid in mapped { + if !doc.story(sid).is_some_and(|st| { + st.tables.values().any(|t| t.cells.iter().any(|c| nested_has(&c.text, &has))) || st.notes.iter().any(|n| nested_has(&n.text, &has)) + }) { + continue; + } let Some(st) = doc.story_mut(sid) else { continue }; for t in st.tables.values_mut() { if t.cells.iter().any(|c| nested_has(&c.text, &has)) { From d22331c60f3c7518fdb7d9c861805a0bdc240da0 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Thu, 5 Nov 2026 07:50:55 +0000 Subject: [PATCH 03/17] Apply review fixes - crates/engine/src/cmd/datamerge.rs --- crates/engine/src/cmd/datamerge.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/datamerge.rs b/crates/engine/src/cmd/datamerge.rs index e834127fd..28c3be75f 100644 --- a/crates/engine/src/cmd/datamerge.rs +++ b/crates/engine/src/cmd/datamerge.rs @@ -434,7 +434,7 @@ fn overset_count(doc: &Document) -> usize { .count() } -fn add_placeholder(d: &mut Document, sel: &Selection, p: &Value, name: &str) -> Result { +fn add_placeholder(d: &mut Document, sel: &mut Selection, p: &Value, name: &str) -> Result { let (source_id, field) = { let source = d.data_merge.sources.first().ok_or_else(|| bad("data.placeholder.add", "select a data source"))?; let field = @@ -490,10 +490,21 @@ fn add_placeholder(d: &mut Document, sel: &Selection, p: &Value, name: &str) -> if role != PlaceholderRole::Text { return Err(bad("data.placeholder.add", "a hyperlink field can't go in a table cell; insert it as text")); } + if name.contains(['<', '>', '\n']) { + return Err(bad("data.placeholder.add", "this field's name can't be used in a table cell")); + } let marker = format!("<<{name}>>"); - let st = d.text_story_mut(t.story, t.cell).ok_or_else(|| bad("data.placeholder.add", "no table cell there"))?; + let cell = t.cell.map(|c| { + d.story(t.story).and_then(|st| st.tables.get(&c.table)).map_or(c, |tb| { + let (row, col) = tb.owner(c.row, c.col); + designcraft_doc::CellAddr { row, col, ..c } + }) + }); + let st = d.text_story_mut(t.story, cell).ok_or_else(|| bad("data.placeholder.add", "no table cell there"))?; let at = designcraft_doc::floor_char_boundary(&st.text, t.range().start.min(st.text.len())); st.replace(at..at, &marker); + let caret = at + marker.len(); + *sel = Selection::text(designcraft_doc::TextSel { anchor: caret, focus: caret, cell, ..t }); return Ok(json!({"cell": true, "marker": marker})); } let (story, at, end) = text_target(p, sel).ok_or_else(|| bad("data.placeholder.add", "select text or a frame"))?; From 12106c76000f1a11858f89798853f29a734be39e Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 5 Nov 2026 12:54:12 +0500 Subject: [PATCH 04/17] Data Merge: a field in a cell leaves the caret after it, goes into a merged cell, refuses a name a marker can't hold; endnotes are filled; the walk is bounded From review: after inserting a field in a cell the cell selection stayed, so the next keystroke replaced the cell; the caret now follows the field. A covered position resolves to its merged cell. A field name with < > or a line break can't be read back from a marker, so it isn't inserted in a cell. Endnote texts are filled like footnotes, and nested texts are followed to MAX_NESTED_DEPTH (32). --- crates/engine/src/cmd/datamerge.rs | 47 ++++++---- crates/engine/src/cmd/datamerge/fill.rs | 80 ++++++++++------ crates/engine/src/cmd/datamerge/tests.rs | 113 +++++++++++++++++++++++ 3 files changed, 195 insertions(+), 45 deletions(-) diff --git a/crates/engine/src/cmd/datamerge.rs b/crates/engine/src/cmd/datamerge.rs index 28c3be75f..28855d8ea 100644 --- a/crates/engine/src/cmd/datamerge.rs +++ b/crates/engine/src/cmd/datamerge.rs @@ -482,29 +482,40 @@ fn add_placeholder(d: &mut Document, sel: &mut Selection, p: &Value, name: &str) if matches!(role, PlaceholderRole::Image | PlaceholderRole::Qr) { return Err(bad("data.placeholder.add", "image and qr placeholders bind to a frame")); } - // The insertion point is in a table cell: the field goes into the cell's text as its - // marker, which the merge fills as it fills a typed one. A cell's text is a story of its - // own that a placeholder can't name, and the selection's offsets are the cell's, so they - // used to put the marker in the table's story instead (#2315). - if !explicit_story && let Some(t) = sel.text.filter(|t| t.cell.is_some()) { + // The insertion point is in a table cell or a footnote: the field goes into that text as + // its marker, which the merge fills as it fills a typed one. Such a text is a story of its + // own that a placeholder can't name, and the selection's offsets are its own, so they used + // to put the marker in the table's story instead (#2315). + if !explicit_story + && let Some(t) = sel.text + && let Some(pressed) = t.cell + { if role != PlaceholderRole::Text { - return Err(bad("data.placeholder.add", "a hyperlink field can't go in a table cell; insert it as text")); + return Err(bad("data.placeholder.add", "a hyperlink field can't go in a table cell or a note; insert it as text")); } - if name.contains(['<', '>', '\n']) { - return Err(bad("data.placeholder.add", "this field's name can't be used in a table cell")); + // The merge finds a marker by its name, so a name that can't be read back out of one + // would stay a marker: say so rather than insert it. + if name.is_empty() || name.contains(['<', '>', '\n']) { + return Err(bad( + "data.placeholder.add", + format!("the field \"{name}\" can't be inserted in a table cell or a note: its name holds < > or a line break"), + )); } - let marker = format!("<<{name}>>"); - let cell = t.cell.map(|c| { - d.story(t.story).and_then(|st| st.tables.get(&c.table)).map_or(c, |tb| { - let (row, col) = tb.owner(c.row, c.col); - designcraft_doc::CellAddr { row, col, ..c } - }) + // A position a merged cell covers is the merged cell's: the field goes where it shows. + let cell = d.story(t.story).and_then(|st| st.tables.get(&pressed.table)).map_or(pressed, |tab| { + let (row, col) = tab.owner(pressed.row, pressed.col); + designcraft_doc::CellAddr { row, col, ..pressed } }); - let st = d.text_story_mut(t.story, cell).ok_or_else(|| bad("data.placeholder.add", "no table cell there"))?; - let at = designcraft_doc::floor_char_boundary(&st.text, t.range().start.min(st.text.len())); + let marker = format!("<<{name}>>"); + let st = d.text_story_mut(t.story, Some(cell)).ok_or_else(|| bad("data.placeholder.add", "no table cell there"))?; + let at = if cell == pressed { t.range().start.min(st.text.len()) } else { st.text.len() }; + let at = designcraft_doc::floor_char_boundary(&st.text, at); st.replace(at..at, &marker); - let caret = at + marker.len(); - *sel = Selection::text(designcraft_doc::TextSel { anchor: caret, focus: caret, cell, ..t }); + // The caret goes after the field, in that text, as after typing; a cell selection + // would have the next keystroke replace the whole cell, field and all. + let end = at + marker.len(); + sel.cells = None; + sel.text = Some(designcraft_doc::TextSel { anchor: end, focus: end, cell: Some(cell), ..t }); return Ok(json!({"cell": true, "marker": marker})); } let (story, at, end) = text_target(p, sel).ok_or_else(|| bad("data.placeholder.add", "select text or a frame"))?; diff --git a/crates/engine/src/cmd/datamerge/fill.rs b/crates/engine/src/cmd/datamerge/fill.rs index fdde21825..38da7eada 100644 --- a/crates/engine/src/cmd/datamerge/fill.rs +++ b/crates/engine/src/cmd/datamerge/fill.rs @@ -332,47 +332,73 @@ fn typed_reps( } } -/// The `<>` markers in the table cells and footnotes of the stories being filled. A cell's -/// or a footnote's text is a story of its own inside its story, which no placeholder can name and -/// `typed_reps` doesn't read, so a field in a table was left as its marker (#2315). Only a table -/// or note that holds a marker is copied. +/// How deep the merge follows texts inside texts (a table in a cell of a table, a note in a cell). +/// Documents don't come anywhere near it; a file that goes deeper is filled down to here instead of +/// exhausting the stack. +pub(super) const MAX_NESTED_DEPTH: usize = 32; + +/// The `<>` markers in the table cells, footnotes and endnotes of the stories being filled. +/// Those texts are stories inside their story, which no placeholder can name and `typed_reps` +/// doesn't read, so a field in a table was left as its marker (#2315). A story, table or note is +/// copied only when something inside it holds a marker of a known field. fn fill_nested_markers(doc: &mut designcraft_doc::Document, fields: &[DataField], row: &[String], maps: &Maps) { - let fill = |st: &mut Story| { - let found: Vec<(usize, usize, String)> = - find_markers(&st.text).into_iter().filter(|(_, _, name)| fields.iter().any(|f| f.name == *name)).collect(); - for (start, end, name) in found.into_iter().rev() { - st.replace(start..end, cell_of(fields, row, &name)); - } - }; - let has = |st: &Story| find_markers(&st.text).iter().any(|(_, _, name)| fields.iter().any(|f| f.name == *name)); let mut mapped: Vec = maps.stories.values().copied().collect(); mapped.sort(); mapped.dedup(); for sid in mapped { - if !doc.story(sid).is_some_and(|st| { - st.tables.values().any(|t| t.cells.iter().any(|c| nested_has(&c.text, &has))) || st.notes.iter().any(|n| nested_has(&n.text, &has)) - }) { + if !doc.story(sid).is_some_and(|st| inner_has(st, fields, 0)) { continue; } let Some(st) = doc.story_mut(sid) else { continue }; - for t in st.tables.values_mut() { - if t.cells.iter().any(|c| nested_has(&c.text, &has)) { - for c in &mut Arc::make_mut(t).cells { - c.text.for_each_text_mut(&mut |x| fill(x)); - } + fill_inner(st, fields, row, 0); + } +} + +/// Whether `st`'s own text holds a marker of a known field. +fn own_has(st: &Story, fields: &[DataField]) -> bool { + find_markers(&st.text).iter().any(|(_, _, name)| fields.iter().any(|f| f.name == *name)) +} + +/// Whether a text inside `st` (a table cell, footnote or endnote, or one inside those), not `st`'s +/// own text, holds a marker of a known field. +fn inner_has(st: &Story, fields: &[DataField], depth: usize) -> bool { + if depth >= MAX_NESTED_DEPTH { + return false; + } + let holds = |t: &Story| own_has(t, fields) || inner_has(t, fields, depth + 1); + st.tables.values().any(|t| t.cells.iter().any(|c| holds(&c.text))) || st.notes.iter().chain(&st.endnotes).any(|n| holds(&n.text)) +} + +/// Fill the known fields' markers in the texts inside `st`, not in its own text. +fn fill_inner(st: &mut Story, fields: &[DataField], row: &[String], depth: usize) { + if depth >= MAX_NESTED_DEPTH { + return; + } + let holds = |t: &Story| own_has(t, fields) || inner_has(t, fields, depth + 1); + for t in st.tables.values_mut() { + if t.cells.iter().any(|c| holds(&c.text)) { + for c in &mut Arc::make_mut(t).cells { + fill_own(&mut c.text, fields, row); + fill_inner(&mut c.text, fields, row, depth + 1); } } - for n in &mut st.notes { - if nested_has(&n.text, &has) { - Arc::make_mut(n).text.for_each_text_mut(&mut |x| fill(x)); - } + } + for n in st.notes.iter_mut().chain(st.endnotes.iter_mut()) { + if holds(&n.text) { + let n = Arc::make_mut(n); + fill_own(&mut n.text, fields, row); + fill_inner(&mut n.text, fields, row, depth + 1); } } } -/// Whether `st` or a text inside it (its table cells and notes) satisfies `has`. -fn nested_has(st: &Story, has: &impl Fn(&Story) -> bool) -> bool { - has(st) || st.tables.values().any(|t| t.cells.iter().any(|c| nested_has(&c.text, has))) || st.notes.iter().any(|n| nested_has(&n.text, has)) +/// Replace the markers of known fields in `st`'s own text with the record's values. +fn fill_own(st: &mut Story, fields: &[DataField], row: &[String]) { + let found: Vec<(usize, usize, String)> = + find_markers(&st.text).into_iter().filter(|(_, _, name)| fields.iter().any(|f| f.name == *name)).collect(); + for (start, end, name) in found.into_iter().rev() { + st.replace(start..end, cell_of(fields, row, &name)); + } } fn find_markers(text: &str) -> Vec<(usize, usize, String)> { diff --git a/crates/engine/src/cmd/datamerge/tests.rs b/crates/engine/src/cmd/datamerge/tests.rs index 66eaea7de..119e159d2 100644 --- a/crates/engine/src/cmd/datamerge/tests.rs +++ b/crates/engine/src/cmd/datamerge/tests.rs @@ -1337,3 +1337,116 @@ fn a_field_converted_into_a_table_is_filled_in_its_cell() { exec(&mut s, "data.preview", json!({"record": 1})); assert_eq!(cell_texts(&s), ["Name", "Ada"], "the field is filled in its cell"); } + +/// A table `Who⇥Where` with the caret in its second cell, and a source with `Name` and `City`. +fn table_with_caret_in_cell(s: &mut Session, csv: &str) -> Value { + new_doc(s, 1); + select_csv(s, csv); + let made = text_frame(s, [36.0, 36.0, 400.0, 200.0], "Who\tWhere", None); + exec(s, "text.select", json!({"story": made["story"], "anchor": 0, "focus": 9})); + exec(s, "table.convertFromText", json!({"columnSeparator": "tab"})); + exec(s, "table.select", json!({"story": made["story"], "rows": [0, 0], "cols": [1, 1]})); + made +} + +/// After a field goes into a cell the caret is after it, in that cell, and the cell selection is +/// gone: typing goes on after the field instead of replacing the cell, field and all. +#[test] +fn typing_after_a_field_inserted_in_a_cell_goes_on_after_it() { + let mut s = Session::new(); + table_with_caret_in_cell(&mut s, "Name,City\nAda,London\n"); + exec(&mut s, "data.placeholder.add", json!({"field": "City"})); + let sel = &s.documents()[s.active_index().unwrap()].selection; + assert!(sel.cells.is_none(), "no cell selection left"); + exec(&mut s, "text.insert", json!({"text": ", "})); + assert_eq!(cell_texts(&s), ["Who", "<>, Where"]); +} + +/// A position that a merged cell covers is the merged cell's: the field goes into the merged cell, +/// where it shows, not into the hidden one under it. +#[test] +fn a_field_inserted_in_a_merged_cell_goes_where_it_shows() { + let mut s = Session::new(); + let made = table_with_caret_in_cell(&mut s, "Name,City\nAda,London\n"); + exec(&mut s, "table.merge", json!({"story": made["story"], "rows": [0, 0], "cols": [0, 1]})); + exec(&mut s, "table.select", json!({"story": made["story"], "rows": [0, 0], "cols": [1, 1]})); + exec(&mut s, "data.placeholder.add", json!({"field": "City"})); + let cells = cell_texts(&s); + assert!(cells[0].contains("<>"), "in the merged cell: {cells:?}"); + assert!(!cells[1].contains("<>"), "not in the covered one: {cells:?}"); + exec(&mut s, "data.preview", json!({"record": 1})); + assert!(cell_texts(&s)[0].contains("London"), "and filled: {:?}", cell_texts(&s)); +} + +/// A field whose name a marker can't hold (it has `<` or `>`) can't be filled from a cell, so it +/// isn't inserted there; in body text it still becomes an anchored placeholder. +#[test] +fn a_field_named_with_angle_brackets_is_refused_in_a_cell() { + let mut s = Session::new(); + let made = table_with_caret_in_cell(&mut s, "Customer ,City\n7,London\n"); + let e = s.execute("data.placeholder.add", &json!({"field": "Customer "})).expect_err("refused").to_string(); + assert!(e.contains("can't be inserted in a table cell"), "{e}"); + assert_eq!(cell_texts(&s), ["Who", "Where"], "nothing inserted"); + let other = text_frame(&mut s, [36.0, 300.0, 400.0, 400.0], "No. ", None); + exec(&mut s, "text.select", json!({"story": other["story"], "anchor": 4, "focus": 4})); + assert!(exec(&mut s, "data.placeholder.add", json!({"field": "Customer "}))["id"].is_u64(), "body text still takes it"); + let _ = made; +} + +/// Endnote text is filled too: it is a text inside its story, like a footnote's. +#[test] +fn a_field_in_an_endnote_is_filled() { + let mut s = Session::new(); + new_doc(&mut s, 1); + select_csv(&mut s, "Name,City\nAda,London\n"); + let made = text_frame(&mut s, [36.0, 36.0, 400.0, 200.0], "See the note", None); + exec(&mut s, "text.select", json!({"story": made["story"], "anchor": 12, "focus": 12})); + exec(&mut s, "endnote.insert", json!({"text": "From <>"})); + let endnotes = |s: &Session| -> Vec { + s.documents()[s.active_index().unwrap()].doc.stories.values().flat_map(|st| st.endnotes.iter().map(|e| e.text.text.clone())).collect() + }; + assert_eq!(endnotes(&s), ["From <>"]); + exec(&mut s, "data.preview", json!({"record": 1})); + assert_eq!(endnotes(&s), ["From London"]); +} + +/// Tables nested in cells far deeper than any document goes are filled down to the merge's depth +/// limit, and the preview returns instead of exhausting the stack. +#[test] +fn deeply_nested_tables_are_filled_down_to_the_limit_without_overflowing() { + use designcraft_doc::{Cell, ParaFormat, Table}; + let mut s = Session::new(); + new_doc(&mut s, 1); + select_csv(&mut s, "Name,City\nAda,London\n"); + let made = text_frame(&mut s, [36.0, 36.0, 400.0, 200.0], "x", None); + let sid = StoryId(made["story"].as_u64().unwrap()); + // `depth` tables, each in the one cell of the next, each cell's own text `<>`. + let nest = |depth: usize| { + let mut inner: Option = None; + for i in 0..depth { + let mut t = Table::new(1000 + i as u64, 1, 1, 0, 0, 100.0); + t.cells[0] = Cell::with_text("<>", ParaFormat::default()); + if let Some(child) = inner.take() { + t.cells[0].text.insert_table(0, child); + } + inner = Some(t); + } + inner.unwrap() + }; + s.edit(|d, _| { + d.story_mut(sid).unwrap().insert_table(0, nest(500)); + Ok(()) + }) + .unwrap(); + exec(&mut s, "data.preview", json!({"record": 1})); + let doc = &s.documents()[s.active_index().unwrap()].doc; + let mut t = doc.story(sid).unwrap().tables.values().next().unwrap().clone(); + let mut filled = Vec::new(); + for _ in 0..40 { + filled.push(t.cells[0].text.text.contains("Ada")); + let Some(next) = t.cells[0].text.tables.values().next().cloned() else { break }; + t = next; + } + assert!(filled[..super::fill::MAX_NESTED_DEPTH].iter().all(|f| *f), "filled down to the limit: {filled:?}"); + assert!(!filled[super::fill::MAX_NESTED_DEPTH], "and not past it"); +} From 72877fae246ca6c206835488ebd22b1571f4fc81 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Thu, 5 Nov 2026 08:09:17 +0000 Subject: [PATCH 05/17] Apply review fixes - crates/engine/src/cmd/datamerge.rs --- crates/engine/src/cmd/datamerge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/datamerge.rs b/crates/engine/src/cmd/datamerge.rs index 28855d8ea..f8d6d12a3 100644 --- a/crates/engine/src/cmd/datamerge.rs +++ b/crates/engine/src/cmd/datamerge.rs @@ -60,7 +60,7 @@ pub fn specs() -> Vec { "Insert Field", [], None, - "{field, role?: text|image|qr|hyperlink, story?, at?, end?, item?} → {id}", + "{field, role?: text|image|qr|hyperlink, story?, at?, end?, item?} → {id}; with the insertion point in a table cell or a note → {cell: true, marker} (a typed marker, not an anchored placeholder)", has_doc, placeholder_add ), From b284052acc966a6d0e0b298805a04a7f4407b05e Mon Sep 17 00:00:00 2001 From: ".CooCoo" Date: Wed, 11 Nov 2026 17:30:21 -0800 Subject: [PATCH 06/17] Type: type.align without a value is an error (#2645) type.align with no `align` key applied Align Left, so a call that misspelled the key, or passed a bare string, changed the paragraphs to left alignment instead of failing. The command now returns an error, leaves the text unchanged and records no undo step. A named value still applies, and `align: null` still removes the override. --- crates/engine/src/cmd/text.rs | 5 +++-- crates/engine/src/cmd/text/format.rs | 31 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index bee08c167..4bcbd6cc4 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -238,8 +238,9 @@ pub fn specs() -> Vec { has_text_or_frames, |s, p| format_paras(s, p.get("attrs").unwrap_or(p)) ), - cmd!("type.align", "Align", [], None, "{align: left|center|right|leftJustified|…}", has_text_or_frames, |s, p| { - format_paras(s, &json!({"align": p.get("align").cloned().unwrap_or(json!("left"))})) + cmd!("type.align", "Align", [], None, "{align: left|center|right|leftJustified|…|null} (required)", has_text_or_frames, |s, p| { + let align = p.get("align").cloned().ok_or_else(|| bad("type.align", "`align` required"))?; + format_paras(s, &json!({ "align": align })) }), cmd!("type.alignLeft", "Align Left", [], Some("Cmd+Shift+L"), "{}", has_text_or_frames, |s, _| format_paras(s, &json!({"align": "left"}))), cmd!("type.alignCenter", "Align Center", [], Some("Cmd+Shift+C"), "{}", has_text_or_frames, |s, _| format_paras( diff --git a/crates/engine/src/cmd/text/format.rs b/crates/engine/src/cmd/text/format.rs index d915da91d..10bce4841 100644 --- a/crates/engine/src/cmd/text/format.rs +++ b/crates/engine/src/cmd/text/format.rs @@ -1074,6 +1074,37 @@ mod pick_up_format_tests { } } +#[cfg(test)] +mod align_param_tests { + use serde_json::json; + + use crate::Session; + + /// `type.align` with no `align` applied Align Left, so a script that misspelled the key + /// (`{"alignment": "center"}`) changed the paragraph to the opposite of what it asked for. + #[test] + fn type_align_without_a_value_is_refused_and_changes_nothing() { + let mut s = Session::new(); + s.execute("file.new", &json!({"pages": 1})).unwrap(); + let f = s.execute("frame.create", &json!({"rect": [20, 20, 400, 300], "content": "text", "text": "Hello world"})).unwrap(); + s.execute("text.select", &json!({"story": f["story"].clone(), "anchor": 0, "focus": 5})).unwrap(); + s.execute("type.align", &json!({"align": "right"})).unwrap(); + let align = |s: &mut Session| s.execute("type.selectionAttrs", &json!({})).unwrap()["para"]["align"].clone(); + assert_eq!(align(&mut s), json!("right")); + let steps = s.doc().unwrap().history.undo.len(); + for params in [json!({}), json!({"alignment": "center"}), json!("center"), json!({"align": "middle"})] { + assert!(s.execute("type.align", ¶ms).is_err(), "{params}"); + assert_eq!(align(&mut s), json!("right"), "{params}"); + assert_eq!(s.doc().unwrap().history.undo.len(), steps, "{params}: no undo step"); + } + // A named value still applies, and null removes the override. + s.execute("type.align", &json!({"align": "center"})).unwrap(); + assert_eq!(align(&mut s), json!("center")); + s.execute("type.align", &json!({"align": null})).unwrap(); + assert_eq!(align(&mut s), json!("left")); + } +} + #[cfg(test)] mod size_bounds_tests { use serde_json::json; From 6fff5328eb59a5fdff5d91df44034212cc07e670 Mon Sep 17 00:00:00 2001 From: ".CooCoo" Date: Wed, 11 Nov 2026 20:52:41 -0800 Subject: [PATCH 07/17] Tests: type.align refusal cases name the check that refuses each (#2662) Follows #2645. Its test said a misspelled key used to apply Align Left. Only empty params and a bare string did; the parameter check already refused a misspelled key, and the value parser refused an unknown value. Each case now asserts the refusal message it gets. --- crates/engine/src/cmd/text/format.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/text/format.rs b/crates/engine/src/cmd/text/format.rs index 10bce4841..64f3c17c7 100644 --- a/crates/engine/src/cmd/text/format.rs +++ b/crates/engine/src/cmd/text/format.rs @@ -1080,8 +1080,9 @@ mod align_param_tests { use crate::Session; - /// `type.align` with no `align` applied Align Left, so a script that misspelled the key - /// (`{"alignment": "center"}`) changed the paragraph to the opposite of what it asked for. + /// `type.align` with empty params (`{}`) or a bare value (`"center"`) applied Align Left. + /// A misspelled key and an unknown value were already refused, and each case checks which + /// check refuses it. #[test] fn type_align_without_a_value_is_refused_and_changes_nothing() { let mut s = Session::new(); @@ -1092,8 +1093,16 @@ mod align_param_tests { let align = |s: &mut Session| s.execute("type.selectionAttrs", &json!({})).unwrap()["para"]["align"].clone(); assert_eq!(align(&mut s), json!("right")); let steps = s.doc().unwrap().history.undo.len(); - for params in [json!({}), json!({"alignment": "center"}), json!("center"), json!({"align": "middle"})] { - assert!(s.execute("type.align", ¶ms).is_err(), "{params}"); + // `{}` and `"center"` reach the command without `align`. The parameter check refuses + // `{"alignment": …}` and the value parser refuses `"middle"`. + for (params, refusal) in [ + (json!({}), "`align` required"), + (json!("center"), "`align` required"), + (json!({"alignment": "center"}), "unknown parameter `alignment`"), + (json!({"align": "middle"}), "middle"), + ] { + let err = s.execute("type.align", ¶ms).unwrap_err().to_string(); + assert!(err.contains(refusal), "{params}: {err}"); assert_eq!(align(&mut s), json!("right"), "{params}"); assert_eq!(s.doc().unwrap().history.undo.len(), steps, "{params}: no undo step"); } From f87c24a1cda25c64ac49b575548e40fca9585065 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:18 -0500 Subject: [PATCH 08/17] PPTX export: bound the retained glyph-to-character maps * PPTX export: keep only the last four faces' glyph-to-character maps --- crates/pptx/src/fonts.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/crates/pptx/src/fonts.rs b/crates/pptx/src/fonts.rs index 27c976aaf..ed02ff297 100644 --- a/crates/pptx/src/fonts.rs +++ b/crates/pptx/src/fonts.rs @@ -15,6 +15,9 @@ pub struct RunFont { pub italic: bool, } +/// Faces whose glyph-to-character map is kept at once. +const MAX_REVERSE_MAPS: usize = 4; + #[derive(Default)] pub struct Fonts { names: HashMap, @@ -116,6 +119,11 @@ impl Fonts { /// The character a glyph shows (the lowest code point mapping to it). pub fn char_of(&mut self, face: &FontFace, gid: u32) -> Option { + // A face's reverse map can hold over a million entries; keeping one per face a bundle uses + // is unbounded, so only the last few stay (a rebuilt map answers the same). + if !self.reverse.contains_key(&face.id()) && self.reverse.len() >= MAX_REVERSE_MAPS { + self.reverse.clear(); + } let map = self.reverse.entry(face.id()).or_insert_with(|| { let mut m = HashMap::new(); for (c, g) in face.chars() { @@ -216,3 +224,26 @@ pub fn eot(face: &FontFace) -> Result, NoEmbed> { h.extend_from_slice(data); Ok(h) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn only_a_few_reverse_maps_are_kept_and_answers_do_not_change() { + let db = designcraft_fonts::FontDb::global(); + let mut fonts = Fonts::default(); + let mut last = None; + for style in ["Regular", "Bold", "Italic", "Bold Italic", "Semibold", "Light", "Black", "ExtraLight", "Medium", "SemiBold Italic"] { + let face = db.face("Source Serif 4", style); + let gid = face.glyph_for('A'); + assert_eq!(fonts.char_of(&face, gid), Some('A'), "{style}"); + assert!(fonts.reverse.len() <= MAX_REVERSE_MAPS); + last = Some((face, gid)); + } + // The map evicted earlier is rebuilt on demand with the same answer. + if let Some((face, gid)) = last { + assert_eq!(fonts.char_of(&face, gid), Some('A')); + } + } +} From 23465223bc655a4177282f8c3a33a55693751226 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:24 -0500 Subject: [PATCH 09/17] xtask: refuse an oversized corpus archive before extracting it * xtask: refuse a corpus archive that expands past the cap before extracting it * address review: a within-cap answer needs tar to exit successfully * Apply review fixes - xtask/src/main.rs --------- Co-authored-by: autocraft-bot[bot] <354215646+autocraft-bot[bot]@users.noreply.github.com> --- xtask/src/main.rs | 52 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 19f8557ad..0ce6b1974 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -273,6 +273,30 @@ fn dir_bytes(dir: &std::path::Path) -> u64 { total } +/// Whether the file bytes of the tar.gz at `tgz` add up to at most `max`, found by streaming them +/// through `tar -xzOf` and counting: nothing is written to disk, and a decompression bomb is cut +/// off (the child is killed) as soon as it passes `max`. +fn expands_within(tgz: &Path, max: u64) -> Result { + use std::io::Read; + let mut child = Command::new("tar") + .arg("-xzOf") + .arg(tgz) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .map_err(|e| format!("tar -xzOf: failed to spawn: {e}"))?; + let Some(out) = child.stdout.take() else { return Err("tar -xzOf: no stdout".into()) }; + let n = std::io::copy(&mut out.take(max.saturating_add(1)), &mut std::io::sink()).map_err(|e| format!("tar -xzOf: {e}"))?; + if n > max { + let _ = child.kill(); + let _ = child.wait(); + return Ok(false); + } + // Within the cap only counts when tar read the whole archive: an early exit proves nothing. + let status = child.wait().map_err(|e| format!("tar -xzOf: {e}"))?; + if status.success() { Ok(true) } else { Err(format!("tar -xzOf {} failed ({status}); archive refused", tgz.display())) } +} + fn cmd_corpus(download: bool) -> Result<(), String> { let corpus = root().join("corpus"); println!( @@ -309,6 +333,11 @@ Tests that use a corpus skip cleanly when it is absent. if !listing.status.success() || listing.stdout.iter().filter(|b| **b == b'\n').count() > PNGSUITE_MAX_ENTRIES { return refuse(format!("the PngSuite download isn't a tar.gz of at most {PNGSUITE_MAX_ENTRIES} entries")); } + match expands_within(&tgz, PNGSUITE_MAX_EXTRACTED) { + Ok(true) => {} + Ok(false) => return refuse(format!("the PngSuite archive expands to more than {PNGSUITE_MAX_EXTRACTED} bytes; nothing was extracted")), + Err(e) => return refuse(e), + } let mut tar = Command::new("tar"); tar.arg("-xzf").arg(&tgz).arg("-C").arg(&dest); if let Err(e) = run(tar, "tar -xzf PngSuite-2017jul19.tgz") { @@ -324,3 +353,26 @@ Tests that use a corpus skip cleanly when it is absent. println!("PngSuite: {n} PNG files in {}", dest.display()); if n == 0 { Err("no PNG files extracted".into()) } else { Ok(()) } } + +#[cfg(test)] +mod corpus_tests { + use super::*; + + #[test] + fn a_tarball_that_expands_past_the_cap_is_refused_without_extracting() { + let dir = std::env::temp_dir().join(format!("dc-xtask-corpus-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("big"), vec![0u8; 100_000]).unwrap(); + let tgz = dir.join("a.tgz"); + let made = Command::new("tar").arg("-czf").arg(&tgz).arg("-C").arg(&dir).arg("big").status(); + if made.is_ok_and(|s| s.success()) { + assert_eq!(expands_within(&tgz, 100_000), Ok(true)); + assert_eq!(expands_within(&tgz, 99_999), Ok(false)); + // A truncated archive makes tar fail: not a within-cap answer. + let bytes = std::fs::read(&tgz).unwrap(); + std::fs::write(&tgz, &bytes[..bytes.len() / 2]).unwrap(); + assert!(expands_within(&tgz, 1 << 20).is_err()); + } + let _ = std::fs::remove_dir_all(&dir); + } +} From eb0ea78d9ad8fbea22361c1812f55303fbabe550 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:29 -0500 Subject: [PATCH 10/17] Images: a WebP with an ICC chunk longer than the file is read without its profile * Images: a WebP whose ICC chunk declares more than the file holds is read without it --- crates/images/src/lib.rs | 23 +++++++++++++++++++++++ crates/images/src/tests.rs | 27 +++++++++++++++++++++++++++ crates/pdf/src/export/images.rs | 6 ++++++ 3 files changed, 56 insertions(+) diff --git a/crates/images/src/lib.rs b/crates/images/src/lib.rs index 83c540e6e..98c058aa6 100644 --- a/crates/images/src/lib.rs +++ b/crates/images/src/lib.rs @@ -433,9 +433,32 @@ pub fn decode_rgba(bytes: &[u8]) -> Option { to_display_rgba(px, Some(&icc), &DisplayColour::active()) } +/// False when `bytes` is a WebP whose `ICCP` chunk declares more bytes than the file holds or than +/// [`colour::MAX_ICC_BYTES`]. The decoder sizes its buffer from the declared length before it reads +/// anything, so such a file must not reach `icc_profile()`; its pixels decode without it. +pub fn webp_icc_is_sane(bytes: &[u8]) -> bool { + if bytes.get(0..4) != Some(b"RIFF") || bytes.get(8..12) != Some(b"WEBP") { + return true; + } + let mut pos = 12usize; + while let Some(head) = bytes.get(pos..pos.saturating_add(8)) { + let size = u32::from_le_bytes([head[4], head[5], head[6], head[7]]) as usize; + let body = pos + 8; + if &head[..4] == b"ICCP" { + return size <= colour::MAX_ICC_BYTES && body.saturating_add(size) <= bytes.len(); + } + // Chunks are padded to an even size. + pos = body.saturating_add(size).saturating_add(size & 1); + } + true +} + /// The ICC profile embedded in a PNG, JPEG, TIFF or WebP. fn embedded_icc(bytes: &[u8]) -> Option> { use image::ImageDecoder; + if !webp_icc_is_sane(bytes) { + return None; + } let mut d = image::ImageReader::new(std::io::Cursor::new(bytes)).with_guessed_format().ok()?.into_decoder().ok()?; d.icc_profile().ok().flatten() } diff --git a/crates/images/src/tests.rs b/crates/images/src/tests.rs index 3e5daf8ab..cd577f09a 100644 --- a/crates/images/src/tests.rs +++ b/crates/images/src/tests.rs @@ -534,3 +534,30 @@ fn gif_tone_and_cmyk_inks_check_the_rgba_budget() { assert!(too_large_to_decode(&tiff)); assert_eq!(decode_cmyk_tiff(&tiff), None); } + +fn webp_with(chunks: &[(&[u8; 4], u32, &[u8])]) -> Vec { + let mut v = b"RIFF\0\0\0\0WEBP".to_vec(); + for (id, declared, body) in chunks { + v.extend_from_slice(*id); + v.extend_from_slice(&declared.to_le_bytes()); + v.extend_from_slice(body); + if body.len() % 2 == 1 { + v.push(0); + } + } + v +} + +/// A WebP that declares an `ICCP` chunk larger than the file (or than the ICC cap) never reaches the +/// decoder's profile read, which would size a buffer from the declared length. +#[test] +fn a_webp_icc_chunk_that_declares_more_than_it_holds_is_refused_before_decoding() { + assert!(webp_icc_is_sane(&webp_with(&[(b"VP8X", 10, &[0; 10]), (b"ICCP", 4, &[1; 4])]))); + // An odd-sized chunk before it is padded to even. + assert!(webp_icc_is_sane(&webp_with(&[(b"EXIF", 3, &[0; 3]), (b"ICCP", 4, &[1; 4])]))); + assert!(!webp_icc_is_sane(&webp_with(&[(b"VP8X", 10, &[0; 10]), (b"ICCP", u32::MAX, &[1; 4])]))); + assert!(!webp_icc_is_sane(&webp_with(&[(b"ICCP", 100, &[1; 4])]))); + assert!(embedded_icc(&webp_with(&[(b"ICCP", u32::MAX, &[1; 4])])).is_none()); + assert!(webp_icc_is_sane(b"\x89PNG not a webp")); + assert!(webp_icc_is_sane(&webp_with(&[(b"VP8L", 6, &[0; 6])]))); +} diff --git a/crates/pdf/src/export/images.rs b/crates/pdf/src/export/images.rs index ec7c76fd6..b50c6b20a 100644 --- a/crates/pdf/src/export/images.rs +++ b/crates/pdf/src/export/images.rs @@ -47,6 +47,9 @@ pub(super) struct RasterMode { /// The ICC profile embedded in an image file, if it has one. pub(super) fn embedded_icc(data: &[u8]) -> Option> { use image::ImageDecoder; + if !designcraft_images::webp_icc_is_sane(data) { + return None; + } let mut dec = image::ImageReader::new(std::io::Cursor::new(data)).with_guessed_format().ok()?.into_decoder().ok()?; dec.icc_profile().ok().flatten() } @@ -89,6 +92,9 @@ pub(super) fn jpeg(data: krilla::Data, icc: impl FnOnce() -> Option>, mo /// its own. fn untagged_rgb(data: &[u8]) -> bool { use image::ImageDecoder; + if !designcraft_images::webp_icc_is_sane(data) { + return false; + } let Some(mut dec) = image::ImageReader::new(std::io::Cursor::new(data)).with_guessed_format().ok().and_then(|r| r.into_decoder().ok()) else { return false; }; From a41fc046f395696df98c2cce6524177c2cd31d9f Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:35 -0500 Subject: [PATCH 11/17] Data merge: a record range stops expanding once the limit is met * Data merge: a record range stops expanding once the limit or the record count is met --- crates/engine/src/cmd/datamerge/parse.rs | 15 ++++++++++++--- crates/engine/src/cmd/datamerge/tests.rs | 10 ++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/datamerge/parse.rs b/crates/engine/src/cmd/datamerge/parse.rs index dfc49e842..20b57e39a 100644 --- a/crates/engine/src/cmd/datamerge/parse.rs +++ b/crates/engine/src/cmd/datamerge/parse.rs @@ -159,7 +159,11 @@ pub fn select_records(count: usize, records: &str, one: u32, range: &str, limit: let i = record_index(one, count)?; idx.push(i); } - "range" => select_range(range, count, &mut idx, &mut seen)?, + "range" => { + // Past this many records nothing more can be kept, so later ranges are only validated. + let keep = limit.filter(|l| *l > 0).map_or(count, |l| count.min(l as usize)); + select_range(range, count, keep, &mut idx, &mut seen)? + } other => return Err(format!("unknown records value \"{other}\"")), } if let Some(limit) = limit { @@ -175,7 +179,9 @@ pub fn select_records(count: usize, records: &str, one: u32, range: &str, limit: } /// Expand a comma-separated list of record numbers and `a-b` ranges into `idx`, skipping repeats. -fn select_range(range: &str, count: usize, idx: &mut Vec, seen: &mut std::collections::HashSet) -> Result<(), String> { +/// Every token is still checked, but once `idx` holds `keep` records no more are expanded, so +/// repeated or overlapping ranges cost nothing past that point. +fn select_range(range: &str, count: usize, keep: usize, idx: &mut Vec, seen: &mut std::collections::HashSet) -> Result<(), String> { if range.trim().is_empty() { return Err("no records".into()); } @@ -194,8 +200,11 @@ fn select_range(range: &str, count: usize, idx: &mut Vec, seen: &mut std: if a > b { return Err(format!("range {a}-{b} is reversed")); } + // Both ends in range means every record between them is. + record_index(a, count)?; + record_index(b, count)?; let mut n = a; - while n <= b { + while n <= b && idx.len() < keep { let i = record_index(n, count)?; if seen.insert(i) { idx.push(i); diff --git a/crates/engine/src/cmd/datamerge/tests.rs b/crates/engine/src/cmd/datamerge/tests.rs index 4793aac17..bcad97bb5 100644 --- a/crates/engine/src/cmd/datamerge/tests.rs +++ b/crates/engine/src/cmd/datamerge/tests.rs @@ -1310,3 +1310,13 @@ fn merged_records_give_anchored_objects_their_own_ids() { doc.check().unwrap(); designcraft_format::load(&designcraft_format::save(doc).unwrap()).unwrap(); } + +#[test] +fn select_records_stops_expanding_once_the_limit_is_met_but_still_checks_every_range() { + // 4 billion records in a range would not finish if it were expanded; the limit stops it. + let idx = parse::select_records(u32::MAX as usize, "range", 1, "1-3,1-4294967295,2-4294967295", Some(5)).unwrap(); + assert_eq!(idx, vec![0, 1, 2, 3, 4]); + // A later token that is out of range is still an error. + assert!(parse::select_records(10, "range", 1, "1-10,5-11", Some(2)).is_err()); + assert!(parse::select_records(10, "range", 1, "1-2,0", Some(2)).is_err()); +} From 26d5c8941c4d8b7dd6e013ce02c4aef407f360f2 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:41 -0500 Subject: [PATCH 12/17] IDML: story export and object-style overlay in linear time, embedded image budget * idml: story runs and marker offsets found once, object-style overlay by position, embedded image budget * address review: saturating embedded-bytes counter * address review: IDML export past the embed limit succeeds and warns for each image written as a link * address review: an unlinked image is always embedded, only linked ones fall back to their link --- crates/engine/src/cmd/interchange.rs | 14 +++-- crates/idml/src/export.rs | 26 ++++++++- crates/idml/src/export/item.rs | 12 +++- crates/idml/src/export/story.rs | 76 +++++++++++++++++++++----- crates/idml/src/import/object_style.rs | 30 ++++++---- crates/idml/src/lib.rs | 2 +- crates/idml/src/tests/assets.rs | 39 +++++++++++++ 7 files changed, 165 insertions(+), 34 deletions(-) diff --git a/crates/engine/src/cmd/interchange.rs b/crates/engine/src/cmd/interchange.rs index 47b36b776..e84c28103 100644 --- a/crates/engine/src/cmd/interchange.rs +++ b/crates/engine/src/cmd/interchange.rs @@ -17,7 +17,7 @@ use crate::{DocState, EngineError, Result, Session}; pub fn specs() -> Vec { vec![ cmd!(query "file.exportIdml", "Export IDML…", [], None, - "{path?, embedImages?: true} — writes an IDML package to `path`, or returns {base64} without a path", + "{path?, embedImages?: true} — writes an IDML package to `path`, or returns {base64} without a path; `warnings` names each image written as a link because the embedded-image limit was reached", has_doc, export_idml), cmd!(noundo "file.openIdml", "Open IDML", [], None, "{path | base64, name?} — opens an IDML package, or an InDesign document or template (.indd, .indt: converted to IDML first; its conversion warnings join `warnings`), as a new document (linked images are read next to the file or from its Links/ folder; the fonts in a `Document Fonts` folder beside it load first) → {index, documentFonts, warnings: conversion warnings, package parts missing, font files skipped, GREP styles whose pattern can't compile}", @@ -29,15 +29,19 @@ fn export_idml(s: &mut Session, p: &Value) -> Result { let st = s.doc()?; let opts = designcraft_idml::ExportOptions { embed_images: p.get("embedImages").and_then(Value::as_bool).unwrap_or(true) }; let layer_doc = with_pdf_layer_variants(&st.doc, opts.embed_images)?; - let bytes = designcraft_idml::export_idml_with(layer_doc.as_ref().unwrap_or(&st.doc), &opts); - match str_param(p, "path") { + let (bytes, warnings) = designcraft_idml::export_idml_report(layer_doc.as_ref().unwrap_or(&st.doc), &opts); + let mut out = match str_param(p, "path") { Some(path) => { #[cfg(not(target_arch = "wasm32"))] crate::io::write_file(path, &bytes).map_err(EngineError::Other)?; - Ok(json!({"path": path, "bytes": bytes.len()})) + json!({"path": path, "bytes": bytes.len()}) } - None => Ok(json!({"base64": base64_encode(&bytes), "bytes": bytes.len()})), + None => json!({"base64": base64_encode(&bytes), "bytes": bytes.len()}), + }; + if !warnings.is_empty() { + out["warnings"] = json!(warnings); } + Ok(out) } /// Whether a placement holds an Object Layer Options choice an export can still apply: a graphic diff --git a/crates/idml/src/export.rs b/crates/idml/src/export.rs index 717bb5d6c..fccbc7c2f 100644 --- a/crates/idml/src/export.rs +++ b/crates/idml/src/export.rs @@ -65,9 +65,20 @@ pub fn export_idml(doc: &Document) -> Vec { /// Export with options, using the index sort-key conflict policy of [`export_idml`]. pub fn export_idml_with(doc: &Document, opts: &ExportOptions) -> Vec { + export_idml_report(doc, opts).0 +} + +/// [`export_idml_with`] and its warnings: an image written as a link only because the export's +/// embedded-image limit was reached is named in one, so the package is not silently incomplete. +pub fn export_idml_report(doc: &Document, opts: &ExportOptions) -> (Vec, Vec) { + export_with_limit(doc, opts, MAX_EMBEDDED_BYTES) +} + +pub(crate) fn export_with_limit(doc: &Document, opts: &ExportOptions, embed_limit: usize) -> (Vec, Vec) { let mut ex = Ex::new(doc, opts); + ex.embed_limit = embed_limit; let parts = ex.build(); - zip_parts(&parts) + (zip_parts(&parts), ex.warnings) } fn zip_parts(parts: &[(String, Vec)]) -> Vec { @@ -106,8 +117,18 @@ struct Ex<'a> { /// Index topic path → first serialized sort key; See / See also cross-references. index_topics: BTreeMap, String>, index_xrefs: Vec<(Vec, &'static str, String)>, + /// Bytes of image data written into `` so far (see [`MAX_EMBEDDED_BYTES`]). + embedded: usize, + embed_limit: usize, + /// Images written as links because the embed limit was reached (one line per placement). + warnings: Vec, } +/// Image bytes one export embeds, counting each placement (IDML holds a copy per placement): past +/// it a graphic that has a link of its own is written as that link without its contents (an +/// image with no link is always embedded: it has nowhere else to live). +const MAX_EMBEDDED_BYTES: usize = 512 * 1024 * 1024; + impl<'a> Ex<'a> { fn new(d: &'a Document, opts: &'a ExportOptions) -> Self { let mut ex = Ex { @@ -121,6 +142,9 @@ impl<'a> Ex<'a> { link_text_sources: Vec::new(), index_topics: BTreeMap::new(), index_xrefs: Vec::new(), + embedded: 0, + embed_limit: MAX_EMBEDDED_BYTES, + warnings: Vec::new(), }; ex.assign_swatch_ids(); for s in d.stories.values() { diff --git a/crates/idml/src/export/item.rs b/crates/idml/src/export/item.rs index 457172253..ca27a8ca0 100644 --- a/crates/idml/src/export/item.rs +++ b/crates/idml/src/export/item.rs @@ -268,9 +268,19 @@ impl<'a> Ex<'a> { el.set("FillColor", id.clone()); el.set("FillTint", pct(g.fill.tint as f64)); } - let embed = !asset.data.is_empty() && (self.opts.embed_images || asset.link.is_none()); + let wanted = !asset.data.is_empty() && (self.opts.embed_images || asset.link.is_none()); + // An image with no link of its own has nowhere else to live, so it is always embedded + // (dropping its data would lose it); only an image with a real link can fall back to it. + let embed = wanted && (asset.link.is_none() || self.embedded.saturating_add(asset.data.len()) <= self.embed_limit); + if wanted && !embed { + self.warnings.push(format!( + "image {} is written as a link, not embedded: the package already holds {} bytes of images (limit {})", + asset.name, self.embedded, self.embed_limit + )); + } let mut props = Vec::new(); if embed { + self.embedded = self.embedded.saturating_add(asset.data.len()); let mut c = El::new("Contents"); c.children.push(Node::CData(base64_encode(&asset.data))); props.push(c); diff --git a/crates/idml/src/export/story.rs b/crates/idml/src/export/story.rs index 437376190..7ca41e231 100644 --- a/crates/idml/src/export/story.rs +++ b/crates/idml/src/export/story.rs @@ -278,7 +278,14 @@ impl<'a> Ex<'a> { None } - fn apply_tracked_changes(&mut self, s: &Story, f: &CharFormat, csrs: &mut [El], para_br: bool, para_end: usize) { + fn apply_tracked_changes( + &mut self, + f: &CharFormat, + csrs: &mut [El], + para_br: bool, + runs: &[(std::ops::Range, &CharFormat)], + para_end: usize, + ) { // Tracked changes wrap the text they mark. if let Some(kind) = change_type(f.over.change) { for c in csrs.iter_mut().filter(|c| c.name == "CharacterStyleRange" && !c.children.is_empty()) { @@ -288,7 +295,10 @@ impl<'a> Ex<'a> { } } if para_br && let Some(c) = csrs.last_mut() { - let nl = s.runs().find(|(rr, _)| rr.contains(¶_end)).and_then(|(_, nf)| change_type(nf.over.change)); + let nl = runs + .get(runs.partition_point(|(rr, _)| rr.end <= para_end)) + .filter(|(rr, _)| rr.start <= para_end) + .and_then(|(_, nf)| change_type(nf.over.change)); match nl { None => c.push(El::new("Br")), Some(kind) => match c.children.last_mut() { @@ -307,17 +317,27 @@ impl<'a> Ex<'a> { pub(super) fn story_paras(&mut self, s: &Story) -> Vec { let ranges = s.para_ranges(); let n = ranges.len(); - ranges.into_iter().enumerate().map(|(pi, r)| self.story_para(s, pi, r, pi + 1 == n)).collect() + // The runs and the marker offsets, once: each paragraph (and each marker) searched them from the start. + let runs: Vec<(std::ops::Range, &CharFormat)> = s.runs().collect(); + let marks = Marks::of(s); + ranges.into_iter().enumerate().map(|(pi, r)| self.story_para(s, pi, r, pi + 1 == n, &runs, &marks)).collect() } - fn paragraph_runs(s: &Story, r: &std::ops::Range) -> Vec<(std::ops::Range, CharFormat)> { - // Character runs intersecting the paragraph. + fn paragraph_runs( + s: &Story, + r: &std::ops::Range, + runs: &[(std::ops::Range, &CharFormat)], + ) -> Vec<(std::ops::Range, CharFormat)> { + // Character runs intersecting the paragraph (they ascend: the first is a search away). let mut segs: Vec<(std::ops::Range, CharFormat)> = Vec::new(); - for (rr, f) in s.runs() { + for (rr, f) in &runs[runs.partition_point(|(rr, _)| rr.end <= r.start)..] { + if rr.start >= r.end { + break; + } let a = rr.start.max(r.start); let b = rr.end.min(r.end); if a < b { - segs.push((a..b, f.clone())); + segs.push((a..b, (*f).clone())); } } if segs.is_empty() { @@ -354,11 +374,19 @@ impl<'a> Ex<'a> { } } - fn story_para(&mut self, s: &Story, pi: usize, r: std::ops::Range, last: bool) -> El { + fn story_para( + &mut self, + s: &Story, + pi: usize, + r: std::ops::Range, + last: bool, + runs: &[(std::ops::Range, &CharFormat)], + marks: &Marks, + ) -> El { let pf = s.paras.get(pi).cloned().unwrap_or_default(); let mut psr = self.psr_el(&pf); let ends_with_break = s.text.get(r.clone()).and_then(|t| t.chars().next_back()).is_some_and(st::is_break_char); - let segs = Self::paragraph_runs(s, &r); + let segs = Self::paragraph_runs(s, &r, runs); // A hyperlink on a run of text is written as a `HyperlinkTextSource` around the // ranges it covers, so the runs are cut at its edges first (#1446). let (links, segs) = self.cut_runs_at_links(s, &r, segs); @@ -414,7 +442,7 @@ impl<'a> Ex<'a> { } else if ch == designcraft_doc::ANCHOR_MARK { flush_text(&mut cur, &mut pending); flush_content(&mut pending, &mut out); - let k = s.text[..seg_start + ci].matches(designcraft_doc::ANCHOR_MARK).count(); + let k = marks.anchor.partition_point(|&at| at < seg_start + ci); if let Some(a) = s.anchors.get(k) { out.push(Node::El( El::new("HyperlinkTextDestination") @@ -428,14 +456,14 @@ impl<'a> Ex<'a> { // An anchored object: the item inside the range with its anchoring settings. flush_text(&mut cur, &mut pending); flush_content(&mut pending, &mut out); - let k = s.text[..seg_start + ci].matches(designcraft_doc::OBJECT_MARK).count(); + let k = marks.object.partition_point(|&at| at < seg_start + ci); if let Some(el) = self.anchored_object(s, k) { out.push(Node::El(el)); } } else if ch == designcraft_doc::INDEX_MARK { flush_text(&mut cur, &mut pending); flush_content(&mut pending, &mut out); - let k = s.text[..seg_start + ci].matches(designcraft_doc::INDEX_MARK).count(); + let k = marks.index.partition_point(|&at| at < seg_start + ci); if let Some(el) = self.index_reference(s, k) { out.push(Node::El(el)); } @@ -448,7 +476,7 @@ impl<'a> Ex<'a> { c.children.append(&mut out); csrs.push(c); } - let k = s.text[..seg_start + ci].matches(designcraft_doc::XREF_MARK).count(); + let k = marks.xref.partition_point(|&at| at < seg_start + ci); if let Some(x) = s.xrefs.get(k) { let me = self.fresh(); let fmt = self.d.xref_formats.iter().position(|ff| ff.name == x.format).map_or("n".to_string(), xref_format_self); @@ -525,7 +553,7 @@ impl<'a> Ex<'a> { c.children.extend(out); csrs.push(c); } - self.apply_tracked_changes(s, &f, &mut csrs, para_br, r.end); + self.apply_tracked_changes(&f, &mut csrs, para_br, runs, r.end); let link = links.iter().find(|(_, l)| l.contains(&seg_start) || (l.start == seg_start && l.start == l.end)).map(|(i, _)| *i); for c in csrs { psr_csrs.push((link, c)); @@ -655,3 +683,23 @@ pub(super) fn with_list_tab(mut a: ParaAttrs, resolved: impl FnOnce() -> (design a.tabs = Some(tabs); a } + +/// Byte offsets of a story's anchor, object, index and cross-reference markers, in text order. +pub(super) struct Marks { + anchor: Vec, + object: Vec, + index: Vec, + xref: Vec, +} + +impl Marks { + fn of(s: &Story) -> Self { + let at = |c: char| s.text.match_indices(c).map(|(i, _)| i).collect::>(); + Marks { + anchor: at(designcraft_doc::ANCHOR_MARK), + object: at(designcraft_doc::OBJECT_MARK), + index: at(designcraft_doc::INDEX_MARK), + xref: at(designcraft_doc::XREF_MARK), + } + } +} diff --git a/crates/idml/src/import/object_style.rs b/crates/idml/src/import/object_style.rs index 343e67211..6af6f845e 100644 --- a/crates/idml/src/import/object_style.rs +++ b/crates/idml/src/import/object_style.rs @@ -242,9 +242,11 @@ fn overlay(base: &mut El, top: &El, depth: usize) { *base = top.clone(); return; } + // Where `base`'s Properties child is, once (a search per attribute otherwise). + let props = base.children.iter().position(|n| matches!(n, Node::El(p) if p.local() == "Properties")); for (k, v) in &top.attrs { base.set(k, v); - if let Some(Node::El(p)) = base.children.iter_mut().find(|n| matches!(n, Node::El(p) if p.local() == "Properties")) { + if let Some(Node::El(p)) = props.and_then(|at| base.children.get_mut(at)) { p.children.retain(|n| !matches!(n, Node::El(x) if x.local() == k)); } } @@ -259,22 +261,26 @@ fn overlay(base: &mut El, top: &El, depth: usize) { // The k-th child of a name merges with the k-th child of that name in `base` (repeated elements // such as gradient stops pair up in order). let mut seen: HashMap<&str, usize> = HashMap::new(); + // Positions in `base.children` of its elements, by name, in order: the k-th is a lookup, not a + // scan from the first child (many repeated names made the scans quadratic). + let mut at: HashMap> = HashMap::new(); + for (i, b) in base.children.iter().enumerate() { + if let Node::El(b) = b { + at.entry(b.local().to_string()).or_default().push(i); + } + } for n in &top.children { match n { Node::El(c) => { let k = seen.entry(c.local()).or_insert(0); - let slot = base - .children - .iter_mut() - .filter_map(|b| match b { - Node::El(b) if b.local() == c.local() => Some(b), - _ => None, - }) - .nth(*k); + let slot = at.get(c.local()).and_then(|v| v.get(*k)).copied(); *k += 1; - match slot { - Some(b) => overlay(b, c, depth + 1), - None => base.children.push(n.clone()), + match slot.and_then(|i| base.children.get_mut(i)) { + Some(Node::El(b)) => overlay(b, c, depth + 1), + _ => { + at.entry(c.local().to_string()).or_default().push(base.children.len()); + base.children.push(n.clone()); + } } } Node::Text(_) | Node::CData(_) if has_text => base.children.push(n.clone()), diff --git a/crates/idml/src/lib.rs b/crates/idml/src/lib.rs index 8f7b22ff9..0c024dfab 100644 --- a/crates/idml/src/lib.rs +++ b/crates/idml/src/lib.rs @@ -57,7 +57,7 @@ mod xml; #[cfg(test)] mod tests; -pub use export::{ExportOptions, export_idml, export_idml_with}; +pub use export::{ExportOptions, export_idml, export_idml_report, export_idml_with}; pub use import::{Imported, import_idml, import_idml_report, import_idml_report_with_previews, import_idml_with}; /// The IDML package mimetype (content of the first, stored `mimetype` entry). diff --git a/crates/idml/src/tests/assets.rs b/crates/idml/src/tests/assets.rs index 48875c256..265183540 100644 --- a/crates/idml/src/tests/assets.rs +++ b/crates/idml/src/tests/assets.rs @@ -393,3 +393,42 @@ fn the_swatch_applied_to_an_image_is_kept() { assert!(!images.iter().any(|e| e.contains("Swatch/None")), "{images:?}"); assert_eq!(fills(&import_idml_with(&out, &reader).unwrap()), want); } + +/// Past the embedded-image limit an export still succeeds: the images that no longer fit are +/// written as links, and each is named in a warning. +#[test] +fn export_past_the_embed_limit_warns_for_each_image_written_as_a_link() { + let bytes = vec![42u8; 300]; + let reader = |p: &str| p.ends_with("couple.jpg").then(|| bytes.clone()); + let frames = vec![ + frame(1, 0.0, 1.0, &image(1, PHOTO, None)), + frame(2, 100.0, 2.0, &image(2, PHOTO, None)), + frame(3, 200.0, 3.0, &image(3, PHOTO, None)), + frame(4, 300.0, 4.0, &image(4, "file:/elsewhere/copy.jpg", Some(&bytes))), + ]; + let d = import_idml_with(&package(&frames), &reader).unwrap(); + let opts = ExportOptions { embed_images: true }; + // Four placements of 300 bytes each; the limit holds two. + let (out, warnings) = crate::export::export_with_limit(&d, &opts, 700); + assert_eq!(warnings.len(), 2, "{warnings:?}"); + assert!(warnings.iter().all(|w| w.contains("written as a link")), "{warnings:?}"); + assert_eq!(import_idml_with(&out, &reader).unwrap().assets.len(), 2); + assert!(crate::export::export_with_limit(&d, &opts, 1200).1.is_empty()); +} + +/// An image with no link of its own is embedded even past the limit: it has no other home, and +/// dropping its data would lose it. +#[test] +fn an_unlinked_image_is_embedded_past_the_embed_limit() { + let bytes = vec![42u8; 300]; + let reader = |p: &str| p.ends_with("couple.jpg").then(|| bytes.clone()); + let frames = vec![frame(1, 0.0, 1.0, &image(1, PHOTO, None)), frame(2, 100.0, 2.0, &image(2, PHOTO, None))]; + let mut d = import_idml_with(&package(&frames), &reader).unwrap(); + for a in d.assets.values_mut() { + std::sync::Arc::make_mut(a).link = None; + } + let (out, warnings) = crate::export::export_with_limit(&d, &ExportOptions { embed_images: true }, 100); + assert!(warnings.is_empty(), "{warnings:?}"); + let back = import_idml_with(&out, &|_: &str| None).unwrap(); + assert!(!back.assets.is_empty() && back.assets.values().all(|a| a.data.as_slice() == bytes.as_slice())); +} From 665fe2a8e42a97c881d8d80dd2bf37d34f168b70 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:47 -0500 Subject: [PATCH 13/17] Format: a failed entry read counts against the archive budget * format: bytes a failed entry read inflated count against the archive budget * address review: mutate the payload bytes explicitly in the checksum test * address review: charge a failed read to the budget it belongs to (stored vs deflated) --- crates/format/src/lib.rs | 11 +++++++++-- crates/format/src/tests.rs | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 8f9d8856e..a03667209 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -224,10 +224,17 @@ fn read_entry( return Err(too_big()); } let mut v = Vec::new(); - if f.take(limit.saturating_add(1)).read_to_end(&mut v).is_err() { + let read = f.take(limit.saturating_add(1)).read_to_end(&mut v); + let n = v.len() as u64; + // What a failed read produced was inflated all the same: it counts against the archive's budget. + if read.is_err() { + if stored { + *stored_left = stored_left.saturating_sub(n); + } else { + *left = left.saturating_sub(n); + } return Ok(None); } - let n = v.len() as u64; if n > limit { return Err(too_big()); } diff --git a/crates/format/src/tests.rs b/crates/format/src/tests.rs index ca57c7770..0e6f613cd 100644 --- a/crates/format/src/tests.rs +++ b/crates/format/src/tests.rs @@ -3,6 +3,25 @@ use designcraft_doc::build::NewDocument; use designcraft_doc::geom::Rect; use designcraft_doc::{Asset, ParaFormat, SpreadRef}; +/// An entry that fails after inflating (a bad checksum) still uses up the archive's byte budget. +#[test] +fn a_failed_read_is_charged_to_the_budget() { + let mut z = zip::ZipWriter::new(Cursor::new(Vec::new())); + let stored = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); + z.start_file("a", stored).unwrap(); + z.write_all(&[7u8; 1000]).unwrap(); + let mut bytes = z.finish().unwrap().into_inner(); + // Flip a data byte: the entry reads in full, then fails its checksum. + // The payload is the only run of 1,000 consecutive 7s in the archive. + let start = bytes.windows(1000).position(|w| w.iter().all(|b| *b == 7)).expect("payload"); + bytes[start] = 8; + let mut archive = zip::ZipArchive::new(Cursor::new(bytes.as_slice())).unwrap(); + let (mut left, mut stored_left) = (5000, 5000); + assert_eq!(read_entry(&mut archive, "a", 4000, &mut left, &mut stored_left).unwrap(), None); + // A failed stored read is charged to the stored budget only. + assert_eq!((left, stored_left), (5000, 4000)); +} + fn png_asset(d: &mut Document, data: &[u8]) -> AssetId { let aid = AssetId(d.alloc()); let a = From 568f25fa9f2b2e50097bb1f76499f24355b18fa1 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:53 -0500 Subject: [PATCH 14/17] Fonts: cap decoded names, bound outline flattening, cache fallback coverage * fonts: cap decoded names, outline flatten tolerance by size, coverage cache for system fallback * Apply review fixes - crates/fonts/src/fontdb/system.rs - crates/fonts/src/fontdb/names.rs * Apply review fixes - crates/fonts/src/fontdb/system.rs * address review: cap flattened edges in has_serifs * address review: check alias blankness before capping --------- Co-authored-by: aiongg <67543387+aiongg@users.noreply.github.com> --- crates/fonts/src/fontdb/fallback.rs | 9 +++ crates/fonts/src/fontdb/names.rs | 40 +++++++++++++- crates/fonts/src/fontdb/outline.rs | 26 ++++++++- crates/fonts/src/fontdb/system.rs | 85 +++++++++++++++++++++++++++-- 4 files changed, 150 insertions(+), 10 deletions(-) diff --git a/crates/fonts/src/fontdb/fallback.rs b/crates/fonts/src/fontdb/fallback.rs index 73a2e0a25..7291cb343 100644 --- a/crates/fonts/src/fontdb/fallback.rs +++ b/crates/fonts/src/fontdb/fallback.rs @@ -10,8 +10,17 @@ pub(super) struct SysFallback { pub(super) misses: std::collections::HashSet, /// CJK chain families already looked up in the catalog (loaded, or not installed). pub(super) chain_tried: std::collections::HashSet<&'static str>, + /// The code points each installed file covers (as ranges), by path, valid for the file's + /// stamp: one character after another is looked up without reading every file again. + pub(super) coverage: std::collections::HashMap>)>, + /// Ranges held in `coverage` (it stops growing at [`MAX_COVERAGE_RANGES`]). + pub(super) coverage_ranges: usize, } +/// Most coverage ranges kept across all files (8 bytes each). +#[cfg(not(target_arch = "wasm32"))] +pub(super) const MAX_COVERAGE_RANGES: usize = 4 * 1024 * 1024; + #[cfg(all(test, target_os = "linux"))] pub(super) struct FallbackSnapshotHook { pub(super) reached: std::sync::mpsc::SyncSender<()>, diff --git a/crates/fonts/src/fontdb/names.rs b/crates/fonts/src/fontdb/names.rs index af5f14cc3..6c2610824 100644 --- a/crates/fonts/src/fontdb/names.rs +++ b/crates/fonts/src/fontdb/names.rs @@ -3,16 +3,42 @@ use super::face::FontFace; use super::system::CatalogEntry; use super::{Arc, MetadataProvider, StringId}; +/// Characters kept of one decoded name: a name record can be 64 KB, and a font has many of them, +/// kept for every face (a real family or style name is far shorter). +const MAX_NAME_CHARS: usize = 256; + +fn capped(name: impl std::fmt::Display) -> String { + use std::fmt::Write as _; + let mut out = String::new(); + // Stops decoding at the cap: the name is never built in full. + let mut room = MAX_NAME_CHARS; + struct Cap<'a>(&'a mut String, &'a mut usize); + impl std::fmt::Write for Cap<'_> { + fn write_str(&mut self, s: &str) -> std::fmt::Result { + for c in s.chars() { + if *self.1 == 0 { + return Err(std::fmt::Error); + } + *self.1 -= 1; + self.0.push(c); + } + Ok(()) + } + } + let _ = write!(Cap(&mut out, &mut room), "{name}"); + out +} + pub(super) fn aliases(font: &skrifa::FontRef<'_>, ids: &[StringId]) -> Vec { let mut names: Vec = - ids.iter().flat_map(|id| font.localized_strings(*id)).take(256).map(|name| name.to_string()).filter(|name| !name.trim().is_empty()).collect(); + ids.iter().flat_map(|id| font.localized_strings(*id)).take(256).filter(|name| !name.chars().all(char::is_whitespace)).map(capped).collect(); names.sort(); names.dedup(); names } pub(super) fn name(font: &skrifa::FontRef<'_>, ids: &[StringId]) -> Option { - ids.iter().find_map(|id| font.localized_strings(*id).english_or_first().map(|s| s.to_string()).filter(|s| !s.is_empty())) + ids.iter().find_map(|id| font.localized_strings(*id).english_or_first().map(capped).filter(|s| !s.is_empty())) } /// A face's family and style names (the default instance's style for a variable font). @@ -116,3 +142,13 @@ pub(super) fn style_italic(style: &str) -> bool { let s = norm(style); s.contains("italic") || s.contains("oblique") || s == "it" } + +#[cfg(test)] +mod tests { + #[test] + fn decoded_names_are_capped() { + assert_eq!(super::capped("Arial"), "Arial"); + let long = "é".repeat(10_000); + assert_eq!(super::capped(&long).chars().count(), super::MAX_NAME_CHARS); + } +} diff --git a/crates/fonts/src/fontdb/outline.rs b/crates/fonts/src/fontdb/outline.rs index 30e328be4..0b07f3b3f 100644 --- a/crates/fonts/src/fontdb/outline.rs +++ b/crates/fonts/src/fontdb/outline.rs @@ -11,28 +11,48 @@ pub(super) fn has_serifs(face: &FontFace) -> Option { if bbox.height().is_nan() || bbox.height() <= 1.0 { return None; } + // Flattened segments kept: an "l" has a few dozen; a hostile outline has no such limit of its own. + const MAX_EDGES: usize = 1 << 20; let mut edges = Vec::new(); + let mut over = false; let mut last = None; let mut start = None; - kurbo::flatten(&path, 1.0, |el| match el { + // A tolerance of one unit is fine for outlines of normal size; a transform that blows one up + // gets a tolerance in proportion, so the subdivision stays bounded (4096 units stay at 1). + let tol = (bbox.width().max(bbox.height()) / 4096.0).max(1.0); + if !tol.is_finite() { + return None; + } + kurbo::flatten(&path, tol, |el| match el { kurbo::PathEl::MoveTo(p) => { last = Some(p); start = Some(p); } kurbo::PathEl::LineTo(p) => { if let Some(a) = last { - edges.push((a, p)); + if edges.len() < MAX_EDGES { + edges.push((a, p)); + } else { + over = true; + } } last = Some(p); } kurbo::PathEl::ClosePath => { if let (Some(a), Some(b)) = (last, start) { - edges.push((a, b)); + if edges.len() < MAX_EDGES { + edges.push((a, b)); + } else { + over = true; + } } last = start; } _ => {} }); + if over { + return None; + } // The outline is y-down: the foot is near bbox.y1 (the baseline). let width_at = |y: f64| { let xs: Vec = edges diff --git a/crates/fonts/src/fontdb/system.rs b/crates/fonts/src/fontdb/system.rs index 2a367b894..84916807e 100644 --- a/crates/fonts/src/fontdb/system.rs +++ b/crates/fonts/src/fontdb/system.rs @@ -38,6 +38,60 @@ fn file_stamp(path: &std::path::Path) -> Option { std::fs::metadata(path).ok().map(|m| (m.len(), m.modified().ok())) } +/// The code points any face of a font file maps, as sorted inclusive ranges. +#[cfg(not(target_arch = "wasm32"))] +fn coverage_ranges(data: &[u8]) -> Vec<(u32, u32)> { + // One bit per code point: memory stays fixed however many faces (or named instances of one face) map however much. + let mut bits = vec![0u64; 0x110000 / 64]; + let mut last = None; + for (i, _, _, _) in enumerate_faces(data) { + if last.replace(i) == Some(i) { + continue; + } + if let Ok(f) = skrifa::FontRef::from_index(data, i) { + for (cp, _) in f.charmap().mappings() { + if let Some(w) = bits.get_mut((cp / 64) as usize) { + *w |= 1u64 << (cp % 64); + } + } + } + } + let mut out: Vec<(u32, u32)> = Vec::new(); + for (wi, &w) in bits.iter().enumerate() { + if w == 0 { + continue; + } + for b in 0..64u32 { + if (w >> b) & 1 == 0 { + continue; + } + let cp = wi as u32 * 64 + b; + match out.last_mut() { + Some(last) if last.1 + 1 == cp => last.1 = cp, + _ => out.push((cp, cp)), + } + } + } + out +} + +#[cfg(all(test, not(target_arch = "wasm32")))] +mod coverage_tests { + #[test] + fn coverage_ranges_match_the_charmap() { + let data = include_bytes!("../../../../assets/fonts/SourceSans3-Regular.ttf"); + let ranges = super::coverage_ranges(data); + let f = skrifa::FontRef::new(data).unwrap(); + use skrifa::MetadataProvider as _; + let in_ranges = |c: char| ranges.iter().any(|&(lo, hi)| (lo..=hi).contains(&u32::from(c))); + assert!(in_ranges('a') && !in_ranges('\u{10FFFE}')); + for c in ['a', 'Z', 'é', '\u{4E2D}', '\u{1F600}'] { + assert_eq!(in_ranges(c), f.charmap().map(c).is_some(), "{c:?}"); + } + assert!(ranges.windows(2).all(|w| w[0].1 + 1 < w[1].0), "sorted and merged"); + } +} + /// What [`FontDb::reload_fonts`] did: the faces cataloged by the rescan, and the installed font /// files that changed or were removed since they were read (their faces are read again, from the /// new file, the next time they are looked up). @@ -272,6 +326,8 @@ impl FontDb { sys.generation = sys.generation.wrapping_add(1); sys.misses.clear(); sys.chain_tried.clear(); + sys.coverage.clear(); + sys.coverage_ranges = 0; count } @@ -564,13 +620,32 @@ impl FontDb { } } for p in paths { - if std::fs::metadata(&p).map(|m| m.len() > 40 << 20).unwrap_or(true) { + let Some(stamp) = file_stamp(&p).filter(|s| s.0 <= 40 << 20) else { continue }; + let cached = + self.sys.lock().unwrap_or_else(PoisonError::into_inner).coverage.get(&p).filter(|(s, _)| *s == stamp).map(|(_, r)| r.clone()); + let mut data = None; + let ranges = match cached { + Some(r) => r, + None => { + let Some(d) = self.read_installed(&p) else { continue }; + let r = Arc::new(coverage_ranges(&d)); + let mut sys = self.sys.lock().unwrap_or_else(PoisonError::into_inner); + if sys.coverage_ranges + r.len() <= super::fallback::MAX_COVERAGE_RANGES { + sys.coverage_ranges += r.len(); + if let Some((_, old)) = sys.coverage.insert(p.clone(), (stamp, r.clone())) { + sys.coverage_ranges -= old.len(); + } + } + data = Some(d); + r + } + }; + let cp = u32::from(c); + if !ranges.iter().any(|&(lo, hi)| (lo..=hi).contains(&cp)) { continue; } - let Some(data) = self.read_installed(&p) else { continue }; - let hit = - enumerate_faces(&data).iter().any(|(i, _, _, _)| skrifa::FontRef::from_index(&data, *i).is_ok_and(|f| f.charmap().map(c).is_some())); - if hit && self.add_faces(data, FontSource::Installed(p)) > 0 && covered(self) { + let Some(data) = data.or_else(|| self.read_installed(&p)) else { continue }; + if self.add_faces(data, FontSource::Installed(p)) > 0 && covered(self) { return true; } } From b7174dc92dca0129ae82969bbf7cf4c5dbc0128e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:59 -0500 Subject: [PATCH 15/17] Engine: bounded DTD validation, compound-path hole orientation * dtd: bounded matching steps, element depth and problem count * address review: stop DTD matching when a step charge fails; refuse hole orientation past its budget * Apply review fixes - crates/engine/src/dtd.rs --------- Co-authored-by: autocraft-bot[bot] <354215646+autocraft-bot[bot]@users.noreply.github.com> --- crates/engine/src/cmd/paths.rs | 62 ++++++++++++++-- crates/engine/src/dtd.rs | 130 +++++++++++++++++++++++++++++---- 2 files changed, 174 insertions(+), 18 deletions(-) diff --git a/crates/engine/src/cmd/paths.rs b/crates/engine/src/cmd/paths.rs index 7800b3189..946da4215 100644 --- a/crates/engine/src/cmd/paths.rs +++ b/crates/engine/src/cmd/paths.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use designcraft_doc::{Content, Document, Fill, Item, ItemId, Selection, Shape, SpreadRef, Stroke}; -use designcraft_geom::{Affine, BezPath, PathData, SubPath}; +use designcraft_geom::{Affine, BezPath, PathData, Shape as _, SubPath}; use serde_json::{Value, json}; use super::{CommandSpec, bad, cmd, has_selection, point_param, targets}; @@ -75,20 +75,40 @@ fn spread_xf(d: &Document, id: ItemId) -> Option<(SpreadRef, Affine)> { Some((loc.spread, d.parent_xf(&loc) * d.item_at(&loc)?.xf)) } +/// Containment tests `orient_holes` makes at most (after the bounding-box filter). A compound path +/// of thousands of nested contours would otherwise take quadratic time; past this the command is +/// refused rather than leave later subpaths with the wrong direction. +const MAX_HOLE_TESTS: usize = 4_000_000; + /// Orient subpaths so those nested an odd number of times run against the others: with the /// non-zero rule they become holes. -fn orient_holes(path: &mut PathData) { +fn orient_holes(path: &mut PathData, command: &str) -> Result<()> { + orient_holes_within(path, command, MAX_HOLE_TESTS) +} + +fn orient_holes_within(path: &mut PathData, command: &str, max_tests: usize) -> Result<()> { let beziers: Vec = path.subpaths.iter().map(|s| PathData::single(s.clone()).to_bezpath()).collect(); + // A point outside a contour's box is outside the contour: most pairs are skipped without a winding count. + let boxes: Vec<_> = beziers.iter().map(BezPath::bounding_box).collect(); let n = path.subpaths.len(); + let mut tests = 0usize; for i in 0..n { let Some(p) = path.subpaths[i].anchors.first().map(|a| a.p) else { continue }; - let depth = (0..n).filter(|&j| j != i && designcraft_geom::hit::fill_contains(&beziers[j], designcraft_geom::FillRule::NonZero, p)).count(); + let mut depth = 0; + for j in (0..n).filter(|&j| j != i && boxes[j].inflate(1e-6, 1e-6).contains(p)) { + tests += 1; + if tests > max_tests { + return Err(bad(command, "too many nested contours to tell holes from fills")); + } + depth += usize::from(designcraft_geom::hit::fill_contains(&beziers[j], designcraft_geom::FillRule::NonZero, p)); + } let sp = &mut path.subpaths[i]; let cw = sp.area() > 0.0; if cw == (depth % 2 == 1) { sp.reverse(); } } + Ok(()) } fn make_compound(s: &mut Session, p: &Value) -> Result { @@ -115,7 +135,7 @@ fn make_compound(s: &mut Session, p: &Value) -> Result { } let it = d.item_mut(base_id).ok_or(designcraft_doc::DocError::NoItem(base_id))?; it.path = PathData::new(subpaths); - orient_holes(&mut it.path); + orient_holes(&mut it.path, "object.makeCompoundPath")?; it.shape = Shape::Path; *sel = Selection::items(vec![base_id]); Ok(json!({"id": base_id.0})) @@ -159,7 +179,7 @@ fn pathfinder(s: &mut Session, p: &Value) -> Result { } let it = d.item_mut(keep).ok_or(designcraft_doc::DocError::NoItem(keep))?; it.path = result.transformed(kxf.inverse()); - orient_holes(&mut it.path); + orient_holes(&mut it.path, "object.pathfinder")?; it.shape = Shape::Path; it.corners = Default::default(); *sel = Selection::items(vec![keep]); @@ -409,6 +429,38 @@ mod tests { s.doc().unwrap().doc.check().unwrap(); } + #[test] + fn orient_holes_alternates_nested_contours_and_skips_distant_ones() { + use designcraft_geom::{Rect, shapes::rectangle}; + let mut subs: Vec = Vec::new(); + for k in 0..3 { + let o = 10.0 * f64::from(k); + subs.extend(rectangle(Rect::new(o, o, 100.0 - o, 100.0 - o)).subpaths); + } + // Thousands of separate contours cost no winding counts (their boxes miss each other). + for k in 0..3000 { + let x = 200.0 + 20.0 * f64::from(k); + subs.extend(rectangle(Rect::new(x, 0.0, x + 10.0, 10.0)).subpaths); + } + let mut path = PathData::new(subs); + orient_holes(&mut path, "test").unwrap(); + let cw: Vec = path.subpaths.iter().map(|s| s.area() > 0.0).collect(); + assert_eq!(cw[..3], [true, false, true]); + assert!(cw[3..].iter().all(|c| *c)); + } + + #[test] + fn orient_holes_refuses_past_its_budget_instead_of_half_orienting() { + use designcraft_geom::{Rect, shapes::rectangle}; + let mut subs: Vec = Vec::new(); + for k in 0..4 { + let o = 10.0 * f64::from(k); + subs.extend(rectangle(Rect::new(o, o, 100.0 - o, 100.0 - o)).subpaths); + } + assert!(orient_holes_within(&mut PathData::new(subs.clone()), "test", 3).is_err()); + assert!(orient_holes_within(&mut PathData::new(subs), "test", 100).is_ok()); + } + #[test] fn pathfinder_subtracts_front_from_back() { let mut s = Session::new(); diff --git a/crates/engine/src/dtd.rs b/crates/engine/src/dtd.rs index 6b48ba3ab..7128ba84e 100644 --- a/crates/engine/src/dtd.rs +++ b/crates/engine/src/dtd.rs @@ -309,7 +309,39 @@ pub fn parse(src: &str) -> Result { /// What `ends` has worked out for one sequence of children: the end positions of a group (by the /// address of its node, which lives as long as the model) from a position. -type EndsMemo = HashMap<(usize, usize), Vec>; +struct EndsMemo { + map: HashMap<(usize, usize), Vec>, + /// Matching steps left (shared by a whole validation); at zero `ends` stops and `over` is set. + left: usize, + /// End positions held in `map`; past [`MAX_STORED_ENDS`] results are no longer kept. + stored: usize, + over: bool, +} + +/// Matching steps one validation may take across all its elements (a step is a position handled or +/// compared). Valid documents with thousands of children per element use a small fraction. +const MAX_MATCH_STEPS: usize = 8 * 1024 * 1024; +/// End positions the memo may hold at once (8 bytes each). +const MAX_STORED_ENDS: usize = 4 * 1024 * 1024; +/// Element nesting depth: `check` recurses and keeps each level's full path. +const MAX_XML_DEPTH: usize = 256; +/// Elements in the document being validated. +const MAX_XML_NODES: usize = 4 * 1024 * 1024; +/// Problems reported before the list stops. +const MAX_PROBLEMS: usize = 10_000; + +impl EndsMemo { + /// Spend `n` steps; false once the budget is gone. + fn spend(&mut self, n: usize) -> bool { + if self.over || n > self.left { + self.left = 0; + self.over = true; + return false; + } + self.left -= n; + true + } +} /// End positions after `cp` matches a prefix of `seq[i..]`. /// @@ -321,8 +353,16 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec { return if seq.get(i) == Some(&n.as_str()) { vec![i + 1] } else { vec![] }; } let key = (std::ptr::from_ref(cp) as usize, i); - if let Some(done) = memo.get(&key) { - return done.clone(); + if let Some(done) = memo.map.get(&key) { + // Charged before the clone, and a failed charge ends the match. + let len = done.len(); + if !memo.spend(len) { + return vec![]; + } + return memo.map.get(&key).cloned().unwrap_or_default(); + } + if !memo.spend(1) { + return vec![]; } let mut out: Vec = match cp { Cp::Name(_) => vec![], @@ -334,6 +374,9 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec { for p in at { next.extend(ends(c, seq, p, memo)); } + if !memo.spend(next.len()) { + return vec![]; + } next.sort_unstable(); next.dedup(); at = next; @@ -349,16 +392,21 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec { } Cp::Rep(c, r) => { let mut all = if *r == '+' { vec![] } else { vec![i] }; + // `all` as a set too: a list search per position made long repeats quadratic. + let mut have: std::collections::HashSet = all.iter().copied().collect(); let mut frontier = ends(c, seq, i, memo); if *r == '?' { all.extend(frontier); } else { while !frontier.is_empty() { - let new: Vec = frontier.iter().copied().filter(|p| !all.contains(p)).collect(); + if !memo.spend(frontier.len()) { + break; + } + let new: Vec = frontier.iter().copied().filter(|p| have.insert(*p)).collect(); all.extend(&new); let mut next = Vec::new(); for p in new.into_iter().filter(|&p| p > i) { - next.extend(ends(c, seq, p, memo).into_iter().filter(|p| !all.contains(p))); + next.extend(ends(c, seq, p, memo).into_iter().filter(|p| !have.contains(p))); } // Two paths to one position are one position: kept apart they multiply per round. next.sort_unstable(); @@ -371,7 +419,10 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec { }; out.sort_unstable(); out.dedup(); - memo.insert(key, out.clone()); + if !memo.over && memo.stored + out.len() <= MAX_STORED_ENDS { + memo.stored += out.len(); + memo.map.insert(key, out.clone()); + } out } @@ -396,6 +447,7 @@ fn read_tree(xml: &str) -> Result { use quick_xml::events::Event; let mut r = quick_xml::Reader::from_str(xml); let mut stack: Vec = Vec::new(); + let mut count = 0usize; let node = |e: &quick_xml::events::BytesStart| Node { name: String::from_utf8_lossy(e.name().as_ref()).to_string(), attrs: e.attributes().flatten().map(|a| String::from_utf8_lossy(a.key.as_ref()).to_string()).collect(), @@ -403,8 +455,20 @@ fn read_tree(xml: &str) -> Result { text: false, }; loop { - match r.read_event().map_err(|e| e.to_string())? { - Event::Start(e) => stack.push(node(&e)), + let event = r.read_event().map_err(|e| e.to_string())?; + if matches!(event, Event::Start(_) | Event::Empty(_)) { + count += 1; + if count > MAX_XML_NODES { + return Err(format!("more than {MAX_XML_NODES} elements")); + } + } + match event { + Event::Start(e) => { + if stack.len() >= MAX_XML_DEPTH { + return Err(format!("elements nested more than {MAX_XML_DEPTH} deep")); + } + stack.push(node(&e)); + } Event::Empty(e) => { let n = node(&e); match stack.last_mut() { @@ -437,8 +501,15 @@ fn read_tree(xml: &str) -> Result { } } -fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec) { - let mut problem = |m: String| out.push(Problem { path: path.to_string(), message: m }); +fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec, steps: &mut usize) { + if out.len() >= MAX_PROBLEMS { + return; + } + let mut problem = |m: String| { + if out.len() < MAX_PROBLEMS { + out.push(Problem { path: path.to_string(), message: m }); + } + }; match dtd.model(&n.name) { None => problem(format!("`{}` isn't declared in the DTD", n.name)), Some(Model::Any) => {} @@ -459,7 +530,12 @@ fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec) { problem(format!("`{}` can't contain text", n.name)); } let seq: Vec<&str> = n.children.iter().map(|c| c.name.as_str()).collect(); - if !ends(cp, &seq, 0, &mut EndsMemo::new()).contains(&seq.len()) { + let mut memo = EndsMemo { map: HashMap::new(), left: *steps, stored: 0, over: false }; + let matched = ends(cp, &seq, 0, &mut memo).contains(&seq.len()); + *steps = memo.left; + if memo.over { + problem(format!("`{}` has too many children to check against its content model", n.name)); + } else if !matched { let got = if seq.is_empty() { "nothing".to_string() } else { seq.join(", ") }; problem(format!("`{}` contains {got}; the DTD asks for {}", n.name, show(cp))); } @@ -474,7 +550,7 @@ fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec) { for c in &n.children { let k = seen.entry(c.name.as_str()).or_default(); *k += 1; - check(dtd, c, &format!("{path}/{}[{k}]", c.name), out); + check(dtd, c, &format!("{path}/{}[{k}]", c.name), out, steps); } } @@ -484,7 +560,10 @@ pub fn validate(dtd: &Dtd, xml: &str) -> Vec { Ok(root) => { let mut out = Vec::new(); let path = root.name.clone(); - check(dtd, &root, &path, &mut out); + check(dtd, &root, &path, &mut out, &mut { MAX_MATCH_STEPS }); + if out.len() >= MAX_PROBLEMS { + out.push(Problem { path: String::new(), message: "further problems are not listed".into() }); + } out } Err(e) => vec![Problem { path: String::new(), message: format!("not well-formed XML: {e}") }], @@ -743,4 +822,29 @@ mod tests { assert_eq!(problems.len(), 1, "{problems:?}"); assert!(problems[0].message.starts_with("`r` contains a, a, a"), "{problems:?}"); } + + #[test] + fn nested_repeats_over_many_children_stop_at_the_step_budget() { + let dtd = parse("").unwrap(); + assert_eq!(validate(&dtd, &format!("{}", "".repeat(1000))), vec![]); + let problems = validate(&dtd, &format!("{}", "".repeat(20_000))); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].message.contains("too many children"), "{problems:?}"); + } + + #[test] + fn xml_nesting_is_bounded() { + let dtd = parse("").unwrap(); + let deep = |n: usize| format!("{}{}", "".repeat(n), "".repeat(n)); + assert_eq!(validate(&dtd, &deep(MAX_XML_DEPTH)), vec![]); + let problems = validate(&dtd, &deep(MAX_XML_DEPTH + 1)); + assert!(problems[0].message.contains("nested more than"), "{problems:?}"); + } + + #[test] + fn problems_are_capped() { + let dtd = parse("").unwrap(); + let problems = validate(&dtd, &format!("{}", "".repeat(MAX_PROBLEMS + 50))); + assert!(problems.len() <= MAX_PROBLEMS + 1, "{}", problems.len()); + } } From ff0463e299c65a74c0191909cdadc6f16f8092be Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:28:07 -0500 Subject: [PATCH 16/17] Exports: leave out text the page doesn't show (inherited conditions, deletions, hidden layers) * fix(export): leave out text hidden by inherited conditions or deletion in every exporter * address review: skip hidden frames in page text and hidden article items; first visible paragraph as nav label; read table anchor's own format * address review: skip nonprinting frames and non-printable layers in fixed-layout page text * Apply review fixes - crates/epub/src/html.rs - crates/engine/src/cmd/export/formats.rs * Apply review fixes - crates/epub/src/html.rs - crates/engine/src/cmd/export/formats.rs * address review: fixed-layout page text skips frames in inactive object states * address review: place a threaded story at its first shown frame; skip it only when every frame is hidden --------- Co-authored-by: autocraft-bot[bot] <354215646+autocraft-bot[bot]@users.noreply.github.com> --- crates/doc/src/lib.rs | 46 +++++++++++++ crates/engine/src/cmd/export/formats.rs | 21 +++++- crates/engine/src/cmd/export/text_tests.rs | 19 +++++ crates/epub/src/chapters.rs | 5 +- crates/epub/src/html.rs | 51 ++++++++++++-- crates/epub/src/story_writer.rs | 32 +++++++-- crates/epub/src/tests.rs | 69 +++++++++++++++++++ crates/epub/tests/it/grouped_reading_order.rs | 5 +- crates/textimport/src/export.rs | 62 ++++++++++++----- crates/textimport/src/tagged.rs | 4 +- 10 files changed, 281 insertions(+), 33 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index d9ee6d3e9..4ebcd665b 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -251,6 +251,52 @@ impl Document { pub fn conditions_hide(&self, conds: &[String]) -> bool { !conds.is_empty() && conds.iter().all(|c| self.conditions.iter().any(|x| x.name == *c && !x.visible)) } + + /// Is text with character format `f`, in a paragraph whose base character attributes are + /// `base` (`Styles::resolve_para`), left off the page: hidden by its effective conditions + /// (the paragraph's and character style's included) or marked deleted? Exports leave it out + /// as composition does. + pub fn hides_run(&self, base: &CharProps, f: &CharFormat) -> bool { + let props = self.styles.resolve_char(base, f); + props.change == ChangeMark::Deleted || self.conditions_hide(&props.conditions) + } + + /// The byte ranges of `st` that show, in order: the runs [`Self::hides_run`] hides left out. + /// Paragraphs and runs are walked together, once; take it once per story. + pub fn visible_spans(&self, st: &Story) -> Vec> { + let mut out: Vec> = Vec::new(); + let runs: Vec<(std::ops::Range, &CharFormat)> = st.runs().collect(); + let mut k = 0; + for (pi, pr) in st.para_ranges().iter().enumerate() { + let base = st.paras.get(pi).map(|pf| self.styles.resolve_para(pf).1).unwrap_or_default(); + while runs.get(k).is_some_and(|(rr, _)| rr.end <= pr.start) { + k += 1; + } + for (rr, f) in runs.iter().skip(k).take_while(|(rr, _)| rr.start < pr.end) { + let (a, b) = (rr.start.max(pr.start), rr.end.min(pr.end)); + if a >= b || self.hides_run(&base, f) { + continue; + } + match out.last_mut() { + Some(last) if last.end == a => last.end = b, + _ => out.push(a..b), + } + } + } + out + } + + /// The text of `st` within `range` that lies in `spans` ([`Self::visible_spans`] of `st`). + pub fn visible_text(st: &Story, spans: &[std::ops::Range], range: std::ops::Range) -> String { + let first = spans.partition_point(|s| s.end <= range.start); + let mut out = String::new(); + for s in spans.iter().skip(first).take_while(|s| s.start < range.end) { + if let Some(t) = st.text.get(s.start.max(range.start)..s.end.min(range.end)) { + out.push_str(t); + } + } + out + } } /// Ink Manager settings for output, by swatch name. Output applies them through a diff --git a/crates/engine/src/cmd/export/formats.rs b/crates/engine/src/cmd/export/formats.rs index a25553d69..4474d8ae7 100644 --- a/crates/engine/src/cmd/export/formats.rs +++ b/crates/engine/src/cmd/export/formats.rs @@ -142,7 +142,7 @@ pub(super) fn export_text(s: &mut Session, p: &Value) -> Result { let text = match format { "rtf" => designcraft_textimport::export::rtf(doc, story), "tagged" => designcraft_textimport::tagged::tagged_text(doc, story), - _ => designcraft_textimport::export::plain_text(story), + _ => designcraft_textimport::export::plain_text(doc, story), }; match path { Some(path) => { @@ -159,13 +159,30 @@ fn page_text(d: &designcraft_doc::Document, cache: &designcraft_compose::Cache, let mut out = String::new(); for st in d.stories.values() { let cs = cache.get(d, st.id, None); + let spans = d.visible_spans(st); for ft in &cs.frames { if d.page_of_item(ft.frame) != Some(abs) { continue; } + // Pages render printing-only: a hidden or nonprinting frame or group around it, or a top-level item on a hidden or non-printable layer (members draw on its layer), isn't on the rendered page. + // Inside a multi-state object only the active state is drawn. + if d.find(ft.frame).is_some_and(|loc| { + (1..=loc.path.len()).any(|n| { + d.item_at(&designcraft_doc::ItemLoc { spread: loc.spread, path: loc.path[..n].to_vec() }).is_some_and(|it| { + it.hidden + || it.nonprinting + || n == 1 && d.layers.iter().any(|l| (!l.visible || !l.printable) && l.id == it.layer) + || n < loc.path.len() && !it.states.is_empty() && loc.path[n] != it.active_state + }) + }) + }) { + continue; + } for l in &ft.lines { let r = l.range.start.min(st.text.len())..l.range.end.min(st.text.len()); - let t: String = st.text[r].chars().filter(|c| !('\u{E000}'..='\u{F8FF}').contains(c)).collect(); + // The line's text as it shows: hidden conditional text and deletions left out. + let t: String = + designcraft_doc::Document::visible_text(st, &spans, r).chars().filter(|c| !('\u{E000}'..='\u{F8FF}').contains(c)).collect(); if !t.trim().is_empty() { out.push_str(t.trim_end_matches('\n')); out.push('\n'); diff --git a/crates/engine/src/cmd/export/text_tests.rs b/crates/engine/src/cmd/export/text_tests.rs index b1b791d3b..48e255ef7 100644 --- a/crates/engine/src/cmd/export/text_tests.rs +++ b/crates/engine/src/cmd/export/text_tests.rs @@ -92,6 +92,25 @@ fn epub_cover_and_fixed_layout() { assert!(p1.contains("width=612, height=792") && p1.contains("Page one words"), "{p1}"); } +/// Fixed-layout page text leaves out frames in a multi-state object's inactive states. +#[test] +fn fixed_epub_text_skips_inactive_states() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let a = s.execute("frame.create", &json!({"rect": [72, 72, 200, 100], "content": "text", "text": "Active words", "caret": false})).unwrap()["id"] + .as_u64() + .unwrap(); + let b = + s.execute("frame.create", &json!({"rect": [72, 300, 200, 100], "content": "text", "text": "Inactive words", "caret": false})).unwrap()["id"] + .as_u64() + .unwrap(); + s.execute("states.create", &json!({"ids": [a, b]})).unwrap(); + let r = s.execute("file.exportFixedEpub", &json!({})).unwrap(); + let mut z = unzip(r["base64"].as_str().unwrap()); + let p1 = read(&mut z, "OEBPS/page1.xhtml"); + assert!(p1.contains("Active words") && !p1.contains("Inactive words"), "{p1}"); +} + /// #775: the fallback follows the font's class, and embedFonts packages the stylesheet's fonts /// (and reports those it can't). #[test] diff --git a/crates/epub/src/chapters.rs b/crates/epub/src/chapters.rs index ede96f390..b9b6e6183 100644 --- a/crates/epub/src/chapters.rs +++ b/crates/epub/src/chapters.rs @@ -99,7 +99,8 @@ pub(crate) fn toc_entries(doc: &Document, order: &[Block]) -> Vec<(String, Strin continue; } let Some(st) = doc.story(*sid) else { continue }; - let first = label(st.text.split('\n').next().unwrap_or("")); + let spans = doc.visible_spans(st); + let first = st.para_ranges().into_iter().map(|r| label(&Document::visible_text(st, &spans, r))).find(|t| !t.is_empty()).unwrap_or_default(); if !first.is_empty() { firsts.push((format!("s{k}"), first, 1)); } @@ -107,7 +108,7 @@ pub(crate) fn toc_entries(doc: &Document, order: &[Block]) -> Vec<(String, Strin let Some(pf) = st.paras.get(pi) else { continue }; let (tag, _) = export_mapping(&doc.styles, &pf.style, false, &pc); let Some(level) = heading_level(tag).filter(|l| *l <= 3) else { continue }; - let text = label(st.text.get(r.clone()).unwrap_or("")); + let text = label(&Document::visible_text(st, &spans, r.clone())); if !text.is_empty() { headings.push((format!("s{k}-p{pi}"), text, level)); } diff --git a/crates/epub/src/html.rs b/crates/epub/src/html.rs index 6442d5045..233c2764e 100644 --- a/crates/epub/src/html.rs +++ b/crates/epub/src/html.rs @@ -14,11 +14,22 @@ pub enum Block { pub fn reading_order(doc: &Document) -> Vec { // Articles, when there are any to export, decide the order (and what's included). if doc.articles.iter().any(|a| a.export) { + let hidden_layers: Vec<_> = doc.layers.iter().filter(|l| !l.visible).map(|l| l.id).collect(); let mut out = Vec::new(); let mut seen = Vec::new(); for a in doc.articles.iter().filter(|a| a.export) { for id in &a.items { let Some(top) = doc.item(*id) else { continue }; + // A hidden item or group hides everything in it. The layer that counts is the + // top-level item's: pages draw a group's members on the group's layer. + if doc.find(*id).is_some_and(|loc| { + (1..=loc.path.len()).any(|n| { + doc.item_at(&designcraft_doc::ItemLoc { spread: loc.spread, path: loc.path[..n].to_vec() }) + .is_some_and(|g| g.hidden || n == 1 && hidden_layers.contains(&g.layer)) + }) + }) { + continue; + } let mut push = |it: &Item| match &it.content { Content::Text(tf) if !seen.contains(&tf.story) => { seen.push(tf.story); @@ -27,11 +38,21 @@ pub fn reading_order(doc: &Document) -> Vec { Content::Graphic(_) => out.push(Block::Image(it.id)), _ => {} }; - top.walk(&mut |i| push(i)); + // Hidden items (with everything in them) aren't on the page. + let mut stack = vec![top]; + while let Some(it) = stack.pop() { + if it.hidden { + continue; + } + push(it); + stack.extend(it.children().iter().rev().map(|c| &**c)); + } } } return out; } + let hidden_layers: Vec<_> = doc.layers.iter().filter(|l| !l.visible).map(|l| l.id).collect(); + let shown = shown_items(doc, &hidden_layers); let mut keyed: Vec<((usize, i64, i64), Block)> = Vec::new(); let mut seen_stories = Vec::new(); for (si, sp) in doc.spreads.iter().enumerate() { @@ -45,7 +66,8 @@ pub fn reading_order(doc: &Document) -> Vec { continue; }; let xf = *parent * it.xf; - if it.hidden { + // Hidden items, and top-level items on hidden layers (with everything in them), aren't on the page. + if it.hidden || stack.len() == 1 && hidden_layers.contains(&it.layer) { continue; } let children = it.children(); @@ -55,8 +77,13 @@ pub fn reading_order(doc: &Document) -> Vec { let block = match &it.content { Content::Text(tf) => { let Some(story) = doc.story(tf.story) else { continue }; - // A story is placed where its first frame is. - if story.frames.first() != Some(&it.id) || seen_stories.contains(&tf.story) || story.text.trim().is_empty() { + // A story is placed where its first frame on the page is: a story is left out + // only when every frame of it is hidden, and a hidden first frame does not hide + // the rest of the thread. + if story.frames.iter().find(|f| shown.contains(f)) != Some(&it.id) + || seen_stories.contains(&tf.story) + || story.text.trim().is_empty() + { continue; } seen_stories.push(tf.story); @@ -78,6 +105,22 @@ pub fn reading_order(doc: &Document) -> Vec { keyed.into_iter().map(|(_, b)| b).collect() } +/// The items the page shows: not hidden themselves, in a hidden group, or (top level) on a hidden layer. +fn shown_items(doc: &Document, hidden_layers: &[designcraft_doc::LayerId]) -> std::collections::HashSet { + let mut shown = std::collections::HashSet::new(); + for sp in &doc.spreads { + let mut stack: Vec<(&Item, bool)> = sp.items.iter().map(|i| (&**i, true)).collect(); + while let Some((it, top)) = stack.pop() { + if it.hidden || top && hidden_layers.contains(&it.layer) { + continue; + } + shown.insert(it.id); + stack.extend(it.children().iter().map(|c| (&**c, false))); + } + } + shown +} + /// Package path of an asset: `{dir}/{name}-{id}.{ext}`, the name from its original file name /// (lowercase ASCII, so the package stays readable) and the id keeping two assets of the same /// name apart. diff --git a/crates/epub/src/story_writer.rs b/crates/epub/src/story_writer.rs index d6fb05159..31ca17ebc 100644 --- a/crates/epub/src/story_writer.rs +++ b/crates/epub/src/story_writer.rs @@ -120,9 +120,15 @@ impl<'a, 'f> StoryWriter<'a, 'f> { let mut lead = lead; let mut list: Option<&str> = None; let mut pre: Option<(String, bool)> = None; + let runs: Vec<(std::ops::Range, &story::CharFormat)> = st.runs().collect(); for (pi, (r, pf)) in st.para_ranges().into_iter().zip(&st.paras).enumerate() { let Some(text) = st.text.get(r.clone()) else { continue }; - let table = pf.table.and_then(|id| st.tables.get(&id)).filter(|_| text.contains(story::TABLE_ANCHOR)); + let base = doc.styles.resolve_para(pf).1; + // A table whose anchor is hidden or deleted is off the page, so it stays out. + let table = pf + .table + .and_then(|id| st.tables.get(&id)) + .filter(|_| text.find(story::TABLE_ANCHOR).is_some_and(|at| !doc.hides_run(&base, st.char_format_of(r.start + at)))); let only_table = table.is_some() && text.chars().all(|c| c == story::TABLE_ANCHOR || c.is_whitespace()); if !only_table || !lead.is_empty() { // Export Tagging: the style's element and class. @@ -167,7 +173,7 @@ impl<'a, 'f> StoryWriter<'a, 'f> { lead = ""; let mut after = String::new(); if !only_table { - self.runs(st, r, depth, out, &mut after); + self.runs(st, &runs, r, &base, depth, out, &mut after); } if tag == "pre" { // Closed when the next paragraph isn't a line of the same block. @@ -224,17 +230,29 @@ impl<'a, 'f> StoryWriter<'a, 'f> { t } - /// The runs of paragraph `para` of `st`; footnotes go to `after`. - fn runs(&mut self, st: &Story, para: std::ops::Range, depth: usize, out: &mut String, after: &mut String) { + /// The runs of paragraph `para` of `st` (`runs`: the story's, collected once; `base`: the + /// paragraph's base character attributes); footnotes go to `after`. + #[allow(clippy::too_many_arguments)] + fn runs( + &mut self, + st: &Story, + runs: &[(std::ops::Range, &story::CharFormat)], + para: std::ops::Range, + base: &designcraft_doc::CharProps, + depth: usize, + out: &mut String, + after: &mut String, + ) { let doc = self.doc; - for (rr, f) in st.runs() { + let first = runs.partition_point(|(rr, _)| rr.end <= para.start); + for (rr, f) in runs.iter().skip(first).take_while(|(rr, _)| rr.start < para.end).map(|(rr, f)| (rr.clone(), *f)) { let a = rr.start.max(para.start); let b = rr.end.min(para.end); if a >= b { continue; } - // Hidden conditional text isn't exported. - if f.over.conditions.as_deref().is_some_and(|c| doc.conditions_hide(c)) || f.over.change == Some(designcraft_doc::ChangeMark::Deleted) { + // Hidden conditional text (the styles' conditions included) and deletions aren't exported. + if doc.hides_run(base, f) { continue; } let Some(raw) = st.text.get(a..b) else { continue }; diff --git a/crates/epub/src/tests.rs b/crates/epub/src/tests.rs index 62683f91a..62195aec0 100644 --- a/crates/epub/src/tests.rs +++ b/crates/epub/src/tests.rs @@ -1025,3 +1025,72 @@ fn hidden_group_children_are_not_exported() { assert!(html::reading_order(&grouped_text(true)).is_empty()); assert_eq!(html::reading_order(&grouped_text(false)).len(), 1); } + +/// Text hidden by a condition the paragraph style carries, and text marked deleted, are off the +/// page: neither the HTML body nor the navigation labels show them. A frame on a hidden layer +/// isn't exported at all. +#[test] +fn text_hidden_by_inherited_conditions_or_deletion_is_not_exported() { + use designcraft_doc::{ChangeMark, CharAttrs, Condition, ParagraphStyle}; + let mut d = Document::new(&NewDocument::default()); + d.conditions.push(Condition { name: "Secret".into(), color: [0, 0, 0], visible: false }); + d.styles_mut().paragraph.push(ParagraphStyle { + name: "Classified".into(), + based_on: None, + next_style: None, + para: Default::default(), + chars: CharAttrs { conditions: Some(vec!["Secret".into()]), ..Default::default() }, + shortcut: String::new(), + }); + let lid = d.default_layer(); + let (_, sid) = d + .add_text_frame( + SpreadRef::Doc(0), + Rect::new(36.0, 36.0, 300.0, 100.0), + lid, + "Public heading\nclassified line\nkept dropped", + ParaFormat::default(), + ) + .unwrap(); + let st = d.story_mut(sid).unwrap(); + let c = st.text.find("classified").unwrap(); + st.format_paras(c..c + 1, |p| p.style = "Classified".into()); + let g = st.text.find(" dropped").unwrap(); + st.format_chars(g..g + 8, |f| f.over = CharAttrs { change: Some(ChangeMark::Deleted), ..Default::default() }); + let hidden_layer = designcraft_doc::LayerId(d.alloc()); + d.layers.push(designcraft_doc::Layer { + id: hidden_layer, + name: "Hidden".into(), + color: [0, 0, 0], + visible: false, + locked: false, + printable: true, + show_guides: true, + suppress_wrap_when_hidden: false, + }); + d.add_text_frame(SpreadRef::Doc(0), Rect::new(36.0, 200.0, 300.0, 300.0), hidden_layer, "On a hidden layer", ParaFormat::default()).unwrap(); + let html = html::export_html(&d, &html::HtmlOptions::default()); + assert!(html.contains("Public heading") && html.contains("kept"), "{html}"); + for gone in ["classified line", "dropped", "On a hidden layer"] { + assert!(!html.contains(gone), "{gone} must not be exported: {html}"); + } + let labels: Vec = chapters::toc_entries(&d, &html::reading_order(&d)).into_iter().map(|(_, label, _)| label).collect(); + assert_eq!(labels, ["Public heading"]); +} + +/// A threaded story is left out only when every one of its frames is hidden: a hidden first +/// frame does not hide the frames after it. +#[test] +fn a_threaded_story_is_skipped_only_when_every_frame_is_hidden() { + let mut d = Document::new(&NewDocument::default()); + let lid = d.default_layer(); + let (a, sid) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(36.0, 36.0, 300.0, 100.0), lid, "first", ParaFormat::default()).unwrap(); + let (b, _) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(36.0, 200.0, 300.0, 300.0), lid, "second", ParaFormat::default()).unwrap(); + d.thread(a, b).unwrap(); + let story = |d: &Document| html::reading_order(d).iter().filter(|x| **x == html::Block::Story(sid)).count(); + assert_eq!(story(&d), 1); + d.item_mut(a).unwrap().hidden = true; + assert_eq!(story(&d), 1, "the visible second frame keeps the story"); + d.item_mut(b).unwrap().hidden = true; + assert_eq!(story(&d), 0); +} diff --git a/crates/epub/tests/it/grouped_reading_order.rs b/crates/epub/tests/it/grouped_reading_order.rs index c4689cf20..eb820883a 100644 --- a/crates/epub/tests/it/grouped_reading_order.rs +++ b/crates/epub/tests/it/grouped_reading_order.rs @@ -145,10 +145,13 @@ fn articles_keep_their_explicit_order_and_exclusions() { let (c, _) = text(&mut doc, 0, 36.0, 10.0, "Article third"); let (omitted, _) = text(&mut doc, 0, 36.0, 5.0, "Excluded story"); doc.articles.push(Article { name: "Unused".into(), items: vec![omitted], export: false }); - doc.articles.push(Article { name: "Chosen".into(), items: vec![grouped, c, grouped], export: true }); + let (hidden, _) = text(&mut doc, 0, 36.0, 1.0, "Hidden in article"); + doc.item_mut(hidden).unwrap().hidden = true; + doc.articles.push(Article { name: "Chosen".into(), items: vec![grouped, c, grouped, hidden], export: true }); assert_output_order(&doc, &["Article first", "Article second", "Article third"]); for output in outputs(&doc) { assert!(!output.contains("Excluded story")); + assert!(!output.contains("Hidden in article")); } } diff --git a/crates/textimport/src/export.rs b/crates/textimport/src/export.rs index 2493f7f98..8d38780d1 100644 --- a/crates/textimport/src/export.rs +++ b/crates/textimport/src/export.rs @@ -20,33 +20,35 @@ fn plain_char(c: char) -> Option<&'static str> { } } -fn table_text(story: &Story, id: u64, nl: &str) -> String { +/// Cells of table `id` as rows of tab-separated cell text (the cells' visible text, nested +/// tables and notes left out), `nl` between rows. +fn table_text(doc: &Document, story: &Story, id: u64, nl: &str) -> String { let Some(t) = story.tables.get(&id) else { return String::new() }; (0..t.nrows()) - .map(|r| (0..t.ncols()).map(|c| t.cell(r, c).map(|x| x.text.text.replace('\n', " ")).unwrap_or_default()).collect::>().join("\t")) + .map(|r| { + (0..t.ncols()) + .map(|c| t.cell(r, c).map(|x| plain_text(doc, &x.text).replace("\r\n", " ").replace('\n', " ")).unwrap_or_default()) + .collect::>() + .join("\t") + }) .collect::>() .join(nl) } -/// Text Only: the story's text, CRLF between paragraphs. -pub fn plain_text(story: &Story) -> String { - // Tracked deletions aren't part of the text. - let deleted: Vec> = - story.runs().filter(|(_, f)| f.over.change == Some(designcraft_doc::ChangeMark::Deleted)).map(|(r, _)| r).collect(); +/// Text Only: the story's text as it shows (hidden conditional text and tracked deletions left +/// out), CRLF between paragraphs. +pub fn plain_text(doc: &Document, story: &Story) -> String { let mut out = String::with_capacity(story.text.len() + 16); + let spans = doc.visible_spans(story); for (i, r) in story.para_ranges().into_iter().enumerate() { if i > 0 { out.push_str("\r\n"); } - let start = r.start; - for (ci, c) in story.text[r].char_indices() { - if deleted.iter().any(|d| d.contains(&(start + ci))) { - continue; - } + for c in Document::visible_text(story, &spans, r).chars() { match (c, plain_char(c)) { (TABLE_ANCHOR, _) => { if let Some(id) = story.paras.get(i).and_then(|p| p.table) { - out.push_str(&table_text(story, id, "\r\n")); + out.push_str(&table_text(doc, story, id, "\r\n")); } } (_, Some("")) => out.push(c), @@ -178,7 +180,7 @@ pub fn rtf(doc: &Document, story: &Story) -> String { match c { TABLE_ANCHOR => { if let Some(id) = table { - for ch in table_text(story, *id, "\u{2028}").chars() { + for ch in table_text(doc, story, *id, "\u{2028}").chars() { rtf_escape(&mut out, ch); } } @@ -214,14 +216,42 @@ mod tests { #[test] fn text_variable_markers_are_dropped() { let st = Story::with_text(StoryId(1), "Page \u{E100} of", Default::default()); - assert_eq!(plain_text(&st), "Page of"); + assert_eq!(plain_text(&Document::new(&NewDocument::default()), &st), "Page of"); let r = rtf(&Document::new(&NewDocument::default()), &st); assert!(!r.contains("\\u-7936") && r.contains("Page of"), "{r}"); } #[test] fn text_only() { - assert_eq!(plain_text(&story()), "Plain bold {x}\r\nCafé\ntwo"); + assert_eq!(plain_text(&Document::new(&NewDocument::default()), &story()), "Plain bold {x}\r\nCafé\ntwo"); + } + + /// Text hidden by a condition set on the paragraph style, and text marked deleted, don't + /// reach Text Only: not in the story and not in a table cell. + #[test] + fn text_only_leaves_out_hidden_and_deleted_text() { + let mut doc = Document::new(&NewDocument::default()); + doc.conditions.push(designcraft_doc::Condition { name: "Secret".into(), color: [0, 0, 0], visible: false }); + doc.styles_mut().paragraph.push(designcraft_doc::ParagraphStyle { + name: "Hidden".into(), + based_on: None, + next_style: None, + para: Default::default(), + chars: CharAttrs { conditions: Some(vec!["Secret".into()]), ..Default::default() }, + shortcut: String::new(), + }); + let mut st = Story::with_text(StoryId(1), "shown\nhidden by style\nkept gone", Default::default()); + let h = st.text.find("hidden").unwrap(); + st.format_paras(h..h + 1, |p| p.style = "Hidden".into()); + let g = st.text.find(" gone").unwrap(); + st.format_chars(g..g + 5, |f| f.over = CharAttrs { change: Some(designcraft_doc::ChangeMark::Deleted), ..Default::default() }); + assert_eq!(plain_text(&doc, &st), "shown\r\n\r\nkept"); + let mut outer = Story::with_text(StoryId(2), &TABLE_ANCHOR.to_string(), Default::default()); + let mut t = designcraft_doc::Table::new(7, 1, 1, 0, 0, 100.0); + t.cells[0].text = st.clone(); + outer.tables.insert(7, std::sync::Arc::new(t)); + outer.paras[0].table = Some(7); + assert_eq!(plain_text(&doc, &outer), "shown kept"); } #[test] diff --git a/crates/textimport/src/tagged.rs b/crates/textimport/src/tagged.rs index c3668614d..572c732c8 100644 --- a/crates/textimport/src/tagged.rs +++ b/crates/textimport/src/tagged.rs @@ -74,13 +74,15 @@ pub fn tagged_text(doc: &Document, story: &Story) -> String { out.push_str("\r\n"); } let pf = story.paras.get(i).cloned().unwrap_or_default(); + let base = doc.styles.resolve_para(&pf).1; let _ = write!(out, "", escape_value(&pf.style)); for (rr, f) in story.runs() { let (a, b) = (rr.start.max(r.start), rr.end.min(r.end)); if a >= b { continue; } - if doc.conditions_hide(f.over.conditions.as_deref().unwrap_or(&[])) || f.over.change == Some(designcraft_doc::ChangeMark::Deleted) { + // Hidden conditional text (the styles' conditions included) and deletions aren't exported. + if doc.hides_run(&base, f) { continue; } let mut open = Vec::new(); From 027bbae9cb3b157750e9213254f03e7192dc10c9 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:28:15 -0500 Subject: [PATCH 17/17] Engine: index style, tag, swatch and run lookups; bound polygon sides, Change All and spelling candidates * fix(engine): index lookups that were quadratic in a document's styles, tags, swatches and runs * address review: first swatch per name in book sync; bound spelling input before collecting * address review: keep the last source style per name in book sync; bound GREP replacement bytes while generating them * address review: write GREP replacements into a byte-bounded buffer; gate keep_last test off wasm * merge main into fix/sec-engine-linear-lookups * address review: make the suggestion length-cap test deterministic --- crates/compose/src/grep.rs | 58 ++++++++++++++++++++++++--- crates/engine/src/cmd/book.rs | 51 ++++++++++++++++++----- crates/engine/src/cmd/find.rs | 26 +++++++----- crates/engine/src/cmd/find/tests.rs | 18 +++++++++ crates/engine/src/cmd/paste_svg.rs | 51 ++++++++++++++++++----- crates/engine/src/cmd/select.rs | 3 +- crates/engine/src/cmd/spelling.rs | 20 +++++++++ crates/engine/src/cmd/table/styles.rs | 7 +++- crates/engine/src/cmd/text/edit.rs | 7 +++- crates/engine/src/cmd/xml.rs | 17 +++++--- crates/engine/src/dtd.rs | 14 ++++++- 11 files changed, 229 insertions(+), 43 deletions(-) diff --git a/crates/compose/src/grep.rs b/crates/compose/src/grep.rs index b387384f9..5a56920dc 100644 --- a/crates/compose/src/grep.rs +++ b/crates/compose/src/grep.rs @@ -432,15 +432,36 @@ impl Grep { Ok((out, more)) } - /// Matches in `text` and their replacements, `template` expanded per match (`$1`…; see - /// [`translate_replacement`]). Stops after the first match when `first_only`. - pub fn replacements(&self, text: &str, template: &str, first_only: bool) -> Result, String)>, GrepError> { + /// Matches in `text` that `keep` accepts and their replacements, `template` expanded per match + /// (`$1`…; see [`translate_replacement`]). Stops after the first kept match when `first_only`, + /// and with an error once the expanded replacements pass `max_bytes` in all (before any more + /// are made). + pub fn replacements( + &self, + text: &str, + template: &str, + first_only: bool, + max_bytes: usize, + keep: impl Fn(&Range) -> bool, + ) -> Result, String)>, GrepError> { let exp = fancy_regex::Expander::default(); let mut out = Vec::new(); + let mut bytes = 0usize; for caps in self.re.captures_iter(text) { let caps = caps.map_err(run_err)?; let Some(m) = caps.get(0) else { continue }; - out.push((m.range(), exp.expansion(template, &caps))); + if !keep(&m.range()) { + continue; + } + // Written into a buffer that refuses bytes past the budget, so an oversized + // expansion is rejected before it is allocated. + let mut w = BoundedBuf { buf: Vec::new(), left: max_bytes.saturating_sub(bytes) }; + if exp.write_expansion(&mut w, template, &caps).is_err() { + return Err(err(format!("the replacements would put more than {max_bytes} bytes in"))); + } + bytes += w.buf.len(); + let expanded = String::from_utf8(w.buf).map_err(|_| err("the replacement is not valid UTF-8".to_string()))?; + out.push((m.range(), expanded)); if first_only { break; } @@ -449,6 +470,27 @@ impl Grep { } } +/// A byte sink that fails a write that would pass its remaining budget (nothing is copied then). +struct BoundedBuf { + buf: Vec, + left: usize, +} + +impl std::io::Write for BoundedBuf { + fn write(&mut self, data: &[u8]) -> std::io::Result { + if data.len() > self.left { + return Err(std::io::ErrorKind::OutOfMemory.into()); + } + self.left -= data.len(); + self.buf.extend_from_slice(data); + Ok(data.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + fn run_err(e: fancy_regex::Error) -> GrepError { err(format!("GREP search stopped: {e}")) } @@ -520,7 +562,13 @@ mod tests { assert_eq!(g.find_all("axb A.B a.b", 1).unwrap().first(), Some(&(4..7))); let g = grep(r"(\d+)\h(\l+)"); let tpl = translate_replacement(r"$2~S$1x\r\$$"); - assert_eq!(g.replacements("5 kg", &tpl, false).unwrap(), [(0..4, "kg\u{A0}5x\n$$".to_string())]); + assert_eq!(g.replacements("5 kg", &tpl, false, usize::MAX, |_| true).unwrap(), [(0..4, "kg\u{A0}5x\n$$".to_string())]); + // The total expanded size is bounded while the replacements are made, over the matches kept. + let g = grep("a"); + assert!(g.replacements("aaaa", "xxxx", false, 15, |_| true).is_err()); + assert_eq!(g.replacements("aaaa", "xxxx", false, 16, |_| true).unwrap().len(), 4); + assert_eq!(g.replacements("aaaa", "xxxx", false, 8, |m| m.start < 2).unwrap().len(), 2); + assert_eq!(g.replacements("aaaa", "xxxx", true, 4, |m| m.start >= 2).unwrap(), [(2..3, "xxxx".to_string())]); } #[test] diff --git a/crates/engine/src/cmd/book.rs b/crates/engine/src/cmd/book.rs index a4217f825..29f95741d 100644 --- a/crates/engine/src/cmd/book.rs +++ b/crates/engine/src/cmd/book.rs @@ -1,6 +1,8 @@ //! Books (File › New / Open Book): an ordered list of documents with continuous page numbers, //! styles synchronised from a style source, and one PDF for the whole book. Saved as `.dcbook`. +use std::collections::{HashMap, HashSet}; + use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; @@ -177,6 +179,15 @@ fn paginate(s: &Session) -> Result { Ok(json!({"pages": next - 1, "chapters": chapters})) } +/// `list` with only the last entry of each name, in the order of those last entries. +#[cfg(not(target_arch = "wasm32"))] +fn keep_last(list: &[T], name: impl Fn(&T) -> &str) -> Vec<&T> { + let mut seen = HashSet::new(); + let mut out: Vec<&T> = list.iter().rev().filter(|x| seen.insert(name(x))).collect(); + out.reverse(); + out +} + #[cfg(not(target_arch = "wasm32"))] fn sync_styles(s: &Session) -> Result { let b = s.book.as_ref().ok_or_else(no_book)?; @@ -187,6 +198,11 @@ fn sync_styles(s: &Session) -> Result { let styles = src.styles.clone(); let swatches = src.swatches.clone(); drop(src); + // A name the source defines twice keeps its last definition, at its last position. + let paragraph = keep_last(&styles.paragraph, |x| x.name.as_str()); + let character = keep_last(&styles.character, |x| x.name.as_str()); + let (paragraph_names, character_names): (HashSet<&str>, HashSet<&str>) = + (paragraph.iter().map(|x| x.name.as_str()).collect(), character.iter().map(|x| x.name.as_str()).collect()); let mut n = 0; for (i, path) in paths.iter().enumerate() { if i == b.style_source { @@ -195,21 +211,27 @@ fn sync_styles(s: &Session) -> Result { writable_chapter(s, path, "book.syncStyles")?; let mut d = load(path)?; { + // The source's styles replace the chapter's of the same names and go last, in the + // source's order (one pass over each list, not one per style). let st = d.styles_mut(); - for ps in &styles.paragraph { - st.paragraph.retain(|x| x.name != ps.name); - st.paragraph.push(ps.clone()); - } - for cs in &styles.character { - st.character.retain(|x| x.name != cs.name); - st.character.push(cs.clone()); - } + st.paragraph.retain(|x| !paragraph_names.contains(x.name.as_str())); + st.paragraph.extend(paragraph.iter().map(|x| (*x).clone())); + st.character.retain(|x| !character_names.contains(x.name.as_str())); + st.character.extend(character.iter().map(|x| (*x).clone())); + } + // First index per name, as Document::swatch resolves colours. + let mut at: HashMap = HashMap::new(); + for (i, w) in d.swatches.iter().enumerate() { + at.entry(w.name.clone()).or_insert(i); } for sw in &swatches { - match d.swatches.iter_mut().find(|w| w.name == sw.name) { + match at.get(&sw.name).and_then(|i| d.swatches.get_mut(*i)) { Some(w) if !w.locked => *w = sw.clone(), Some(_) => {} - None => d.swatches.push(sw.clone()), + None => { + at.insert(sw.name.clone(), d.swatches.len()); + d.swatches.push(sw.clone()); + } } } batch.stage(path, &d)?; @@ -431,4 +453,13 @@ mod tests { assert_eq!(t.execute("book.open", &json!({"path": p("book.dcbook")})).unwrap()["documents"], 2); std::fs::remove_dir_all(&dir).ok(); } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn a_source_style_defined_twice_keeps_its_last_definition() { + let list = [("a", 1), ("b", 2), ("a", 3), ("c", 4)]; + let kept: Vec<_> = super::keep_last(&list, |x| x.0).into_iter().copied().collect(); + // As the one-style-at-a-time loop left them: each name once, at its last position. + assert_eq!(kept, [("b", 2), ("a", 3), ("c", 4)]); + } } diff --git a/crates/engine/src/cmd/find.rs b/crates/engine/src/cmd/find.rs index b9ab42550..63a1debb9 100644 --- a/crates/engine/src/cmd/find.rs +++ b/crates/engine/src/cmd/find.rs @@ -316,19 +316,27 @@ impl Search { ) -> Result, String)>> { let limit = if first_only { 1 } else { usize::MAX }; match (&self.re, self.spans(d, sid, cell, r)) { - (Some(re), None) if grep => re.replacements(text, rep, first_only).map_err(|e| bad("find.change", e)), - (Some(re), Some(spans)) if grep => Ok(re - .replacements(text, rep, false) - .map_err(|e| bad("find.change", e))? - .into_iter() - .filter(|(m, _)| within(&spans, m)) - .take(limit) - .collect()), - _ => Ok(self.find_all(d, sid, cell, r, text, limit)?.into_iter().map(|m| (m, rep.to_string())).collect()), + (Some(re), spans) if grep => re + .replacements(text, rep, first_only, MAX_CHANGE_BYTES, |m| spans.as_ref().is_none_or(|s| within(s, m))) + .map_err(|e| bad("find.change", e)), + _ => { + let found = self.find_all(d, sid, cell, r, text, limit)?; + // Every match gets its own copy of the change: bounded before any is made. + if found.len().saturating_mul(rep.len()) > MAX_CHANGE_BYTES { + return Err(bad( + "find.change", + format!("changing {} matches to {} bytes each would put more than {MAX_CHANGE_BYTES} bytes in", found.len(), rep.len()), + )); + } + Ok(found.into_iter().map(|m| (m, rep.to_string())).collect()) + } } } } +/// The most text one Change All may put into a story: the matches times the change's length. +const MAX_CHANGE_BYTES: usize = 64 << 20; + #[derive(Clone, Debug, PartialEq, Eq)] enum Domain { Document, diff --git a/crates/engine/src/cmd/find/tests.rs b/crates/engine/src/cmd/find/tests.rs index 348acd294..edea872ad 100644 --- a/crates/engine/src/cmd/find/tests.rs +++ b/crates/engine/src/cmd/find/tests.rs @@ -959,3 +959,21 @@ fn change_takes_any_selection_find_next_made() { assert_eq!(s.doc().unwrap().doc.story(sid).unwrap().text, if grep { "a" } else { "ab" }); } } + +/// Change All with a long change and many matches is refused before any copy of the change is +/// made, not after the memory for all of them has gone. +#[test] +fn change_all_is_bounded_by_what_it_would_put_in() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let text = "x ".repeat(2048); + s.execute("frame.create", &json!({"rect": [36, 36, 300, 300], "content": "text", "text": text})).unwrap(); + let huge = "y".repeat(40_000); + let e = s.execute("find.change", &json!({"find": "x", "change": huge})).unwrap_err().to_string(); + assert!(e.contains("would put more than"), "{e}"); + // GREP replacements are held to the same total while they are made. + let e = s.execute("find.change", &json!({"find": "x", "change": huge, "grep": true})).unwrap_err().to_string(); + assert!(e.contains("would put more than"), "{e}"); + let r = s.execute("find.change", &json!({"find": "x", "change": "yy"})).unwrap(); + assert_eq!(r["count"], 2048); +} diff --git a/crates/engine/src/cmd/paste_svg.rs b/crates/engine/src/cmd/paste_svg.rs index a5d738cef..fa3c1cffc 100644 --- a/crates/engine/src/cmd/paste_svg.rs +++ b/crates/engine/src/cmd/paste_svg.rs @@ -3,6 +3,8 @@ //! clipboard as SVG markup; each path becomes a DesignCraft path with its fill and stroke, groups //! stay groups, and text comes in as outlines. +use std::collections::{HashMap, HashSet}; + use designcraft_doc::{Content, Fill, Item, ItemId, Selection, Shape, Stroke, StrokeType}; use designcraft_geom::{Affine, BezPath, PathData, Point, Rect}; use designcraft_images::usvg; @@ -201,16 +203,46 @@ fn bounds(art: &[Art]) -> Option { .reduce(|a, b| a.union(b)) } +/// The document's RGB colour swatches by colour and its swatch names, looked up once per paste +/// (a pasted SVG can hold tens of thousands of colours). +struct PasteSwatches { + by_rgb: HashMap<[u32; 3], String>, + names: HashSet, +} + +impl PasteSwatches { + /// An RGB colour as a key: equal colours, equal keys (`+ 0.0` makes -0 and 0 one key). + fn key(r: f32, g: f32, b: f32) -> [u32; 3] { + [(r + 0.0).to_bits(), (g + 0.0).to_bits(), (b + 0.0).to_bits()] + } + + fn new(d: &designcraft_doc::Document) -> Self { + use designcraft_color::swatch::SwatchValue; + let mut by_rgb = HashMap::new(); + for w in &d.swatches { + if let SwatchValue::Color { color: designcraft_color::Color::Rgb { r, g, b }, .. } = &w.value { + // The first swatch of a colour is the one, as a search from the start finds it. + by_rgb.entry(Self::key(*r, *g, *b)).or_insert_with(|| w.name.clone()); + } + } + Self { by_rgb, names: d.swatches.iter().map(|w| w.name.clone()).collect() } + } +} + /// The swatch holding exactly `rgb`: an existing process colour, else a new unnamed one (hidden /// from the Swatches panel until Add to Swatches), as InDesign does for pasted colours. -fn swatch_for(d: &mut designcraft_doc::Document, rgb: [u8; 3]) -> String { +fn swatch_for(d: &mut designcraft_doc::Document, known: &mut PasteSwatches, rgb: [u8; 3]) -> String { use designcraft_color::swatch::{Swatch, SwatchValue}; let color = designcraft_color::Color::rgb8(rgb[0], rgb[1], rgb[2]); - if let Some(w) = d.swatches.iter().find(|w| matches!(&w.value, SwatchValue::Color { color: c, .. } if *c == color)) { - return w.name.clone(); + let designcraft_color::Color::Rgb { r, g, b } = color else { return String::new() }; + let key = PasteSwatches::key(r, g, b); + if let Some(name) = known.by_rgb.get(&key) { + return name.clone(); } let base = format!("R={} G={} B={}", rgb[0], rgb[1], rgb[2]); - let name = designcraft_doc::Styles::unique_name(|n| d.swatch(n).is_some(), &base); + let name = designcraft_doc::Styles::unique_name(|n| known.names.contains(n), &base); + known.names.insert(name.clone()); + known.by_rgb.insert(key, name.clone()); d.swatches.push(Swatch { name: name.clone(), value: SwatchValue::Color { color, color_type: designcraft_color::ColorType::Process }, @@ -221,7 +253,7 @@ fn swatch_for(d: &mut designcraft_doc::Document, rgb: [u8; 3]) -> String { name } -fn build(d: &mut designcraft_doc::Document, art: Vec, layer: designcraft_doc::LayerId, shift: Affine) -> Vec { +fn build(d: &mut designcraft_doc::Document, known: &mut PasteSwatches, art: Vec, layer: designcraft_doc::LayerId, shift: Affine) -> Vec { let mut out = Vec::new(); for a in art { match a { @@ -239,12 +271,12 @@ fn build(d: &mut designcraft_doc::Document, art: Vec, layer: designcraft_do let paint = fill.map(|f| f.opacity).or(stroke.as_ref().map(|(c, _)| c.opacity)).unwrap_or(1.0); let mut it = Item::new(ItemId(d.alloc()), layer, if line { Shape::GraphicLine } else { Shape::Path }, data); it.fill = match fill { - Some(f) => Fill::swatch(&swatch_for(d, f.rgb)), + Some(f) => Fill::swatch(&swatch_for(d, known, f.rgb)), None => Fill::none(), }; it.stroke = match stroke { Some((c, s)) => Stroke { - swatch: swatch_for(d, c.rgb), + swatch: swatch_for(d, known, c.rgb), weight: s.width, cap: s.cap, join: s.join, @@ -258,7 +290,7 @@ fn build(d: &mut designcraft_doc::Document, art: Vec, layer: designcraft_do out.push(it); } Art::Group(items, opacity) => { - let children = build(d, items, layer, shift); + let children = build(d, known, items, layer, shift); if children.is_empty() { continue; } @@ -312,7 +344,8 @@ fn paste_svg(s: &mut Session, p: &Value) -> Result { warnings.push(format!("{} embedded image(s) left out (place images with File › Place)", notes.images)); } s.edit(|d, sel| { - let items = build(d, art, layer, shift); + let mut known = PasteSwatches::new(d); + let items = build(d, &mut known, art, layer, shift); let mut ids = Vec::new(); for it in items { ids.push(it.id); diff --git a/crates/engine/src/cmd/select.rs b/crates/engine/src/cmd/select.rs index 0f77d5372..e351fe837 100644 --- a/crates/engine/src/cmd/select.rs +++ b/crates/engine/src/cmd/select.rs @@ -133,7 +133,8 @@ fn convert_shape(s: &mut Session, p: &Value) -> Result { const ID: &str = "object.convertShape"; let to = str_param(p, "to").ok_or_else(|| bad(ID, "`to` required"))?.to_string(); let ids = targets(s, p)?; - let sides = s.prefs.polygon_sides; + // The preference as object.polygon takes it: 3–100 sides, whatever prefs.set was given. + let sides = s.prefs.polygon_sides.clamp(3, 100); let inset = s.prefs.star_inset; s.edit(|d, _| { let mut changed = 0; diff --git a/crates/engine/src/cmd/spelling.rs b/crates/engine/src/cmd/spelling.rs index 4a43ab752..8ff9ac94d 100644 --- a/crates/engine/src/cmd/spelling.rs +++ b/crates/engine/src/cmd/spelling.rs @@ -161,6 +161,9 @@ const COMMON: &[&str] = &[ "yay", ]; +/// Longest word [`suggest`] makes candidates for. +pub const MAX_SUGGEST_CHARS: usize = 40; + /// Is `w` (lowercase) a known word, allowing common inflections of dictionary stems? pub fn known(dict: &Dictionary, user: &[String], w: &str) -> bool { // A typographer's apostrophe (don’t, we’re) is the word list's straight one. @@ -207,6 +210,11 @@ pub fn known(dict: &Dictionary, user: &[String], w: &str) -> bool { /// Up to 6 suggestions at edit distance 1, ranked: transpositions, replacements, deletions, insertions. pub fn suggest(dict: &Dictionary, w: &str) -> Vec { + // Each candidate is a copy of the word: 53 per position, quadratic in its length. No word of + // a dictionary is this long, so no edit of such a "word" is one either. Checked before collecting. + if w.chars().take(MAX_SUGGEST_CHARS + 1).count() > MAX_SUGGEST_CHARS { + return Vec::new(); + } let chars: Vec = w.chars().collect(); let mut out: Vec = Vec::new(); let mut push = |c: Vec| { @@ -325,6 +333,18 @@ mod tests { assert!(known(d, &["designcraft".into()], "designcraft")); } + /// A "word" longer than any in the dictionary gets no candidates: there would be more than + /// fifty copies of it per character. + #[test] + fn very_long_words_get_no_suggestions() { + // The dictionary holds the longest allowed word, so deleting one letter of the 41-letter + // input would be a hit if the length cap were not applied. + let d = Dictionary::from_entries(vec![("a".repeat(MAX_SUGGEST_CHARS), 0)]); + assert_eq!(suggest(&d, &"a".repeat(MAX_SUGGEST_CHARS)), Vec::::new()); + assert_eq!(suggest(&d, &"a".repeat(MAX_SUGGEST_CHARS - 1)), vec!["a".repeat(MAX_SUGGEST_CHARS)]); + assert!(suggest(&d, &"a".repeat(MAX_SUGGEST_CHARS + 1)).is_empty()); + } + #[test] fn user_dictionary_replaces_and_counts() { let mut s = Session::new(); diff --git a/crates/engine/src/cmd/table/styles.rs b/crates/engine/src/cmd/table/styles.rs index fa3717977..046904fef 100644 --- a/crates/engine/src/cmd/table/styles.rs +++ b/crates/engine/src/cmd/table/styles.rs @@ -133,6 +133,11 @@ fn reapply_cell_styles(d: &mut Document, names: &[String]) { /// Include descendants when editing an inherited style, with the resolver's depth limit. fn dependent_styles<'a>(name: &str, styles: impl Iterator)>) -> Vec { let styles: Vec<_> = styles.collect(); + // The first style of a name is the one, as the resolver's lookup finds it. + let mut based_on: std::collections::HashMap<&str, Option<&str>> = std::collections::HashMap::new(); + for (n, base) in &styles { + based_on.entry(n).or_insert(*base); + } styles .iter() .filter(|(candidate, _)| { @@ -142,7 +147,7 @@ fn dependent_styles<'a>(name: &str, styles: impl Iterator Result { let mut changes = Vec::new(); for seg in segs.iter().rev() { let old = st.slice(seg.clone()).to_string(); - let new: String = starts.iter().zip(&mapped).filter(|(b, _)| seg.contains(b)).map(|(_, m)| m.as_str()).collect(); + // The characters in this run: `starts` is sorted, so a search finds the slice. + let (from, to) = (starts.partition_point(|b| *b < seg.start), starts.partition_point(|b| *b < seg.end)); + let new: String = mapped.get(from..to).unwrap_or_default().iter().map(String::as_str).collect(); if new != old { delta += new.len() as isize - old.len() as isize; st.replace(seg.clone(), &new); // Per-character changes (back to front), so ranges inside the run keep their boundaries. - for (i, (b, m)) in starts.iter().zip(&mapped).enumerate().rev().filter(|(_, (b, _))| seg.contains(b)) { + for i in (from..to.min(mapped.len())).rev() { + let (b, m) = (&starts[i], &mapped[i]); let ce = starts.get(i + 1).copied().unwrap_or(r.end); if m.len() != ce - b { changes.push((*b..ce, m.len())); diff --git a/crates/engine/src/cmd/xml.rs b/crates/engine/src/cmd/xml.rs index 81b57b5b3..949d96436 100644 --- a/crates/engine/src/cmd/xml.rs +++ b/crates/engine/src/cmd/xml.rs @@ -60,10 +60,13 @@ fn plain(t: &str) -> String { } /// A paragraph's text with inline-tagged runs as elements. -fn para_xml(st: &designcraft_doc::Story, r: std::ops::Range) -> String { +/// `runs` are the story's runs in order (collected once per story): only those reaching the +/// paragraph are visited, found by binary search. +fn para_xml(st: &designcraft_doc::Story, runs: &[(std::ops::Range, &designcraft_doc::CharFormat)], r: std::ops::Range) -> String { let mut out = String::new(); let mut open: Option = None; - for (rr, f) in st.runs() { + let first = runs.partition_point(|(rr, _)| rr.end <= r.start); + for (rr, f) in runs.iter().skip(first).take_while(|(rr, _)| rr.start < r.end) { let (a, b) = (rr.start.max(r.start), rr.end.min(r.end)); if a >= b { continue; @@ -97,8 +100,10 @@ pub fn export_xml(d: &Document) -> String { let Some(st) = d.story(tf.story) else { continue }; let mapped = |style: &str| d.xml.style_map.iter().find(|(s, _)| s == style).map(|(_, t)| t.clone()); // Paragraph text with inline tags already escaped. - let paras: Vec<(Option, String)> = - st.para_ranges().into_iter().enumerate().map(|(i, r)| (mapped(&st.paras[i].style), para_xml(st, r))).collect(); + let paras: Vec<(Option, String)> = { + let runs: Vec<_> = st.runs().collect(); + st.para_ranges().into_iter().enumerate().map(|(i, r)| (mapped(&st.paras[i].style), para_xml(st, &runs, r))).collect() + }; if paras.iter().any(|(m, _)| m.is_some()) { out.push_str(&format!(" <{tag}>\n")); for (m, text) in paras { @@ -341,8 +346,10 @@ pub fn specs() -> Vec { let dtd = crate::dtd::parse(&text).map_err(|e| bad("xml.loadDtd", e))?; let names: Vec = dtd.element_names().map(str::to_string).collect(); s.edit(|d, _| { + // The tags there are, then those added: one set, not a search per element. + let mut known: std::collections::HashSet = d.xml.tags.iter().map(|t| t.name.clone()).collect(); for n in &names { - if valid_name(n) && !d.xml.tags.iter().any(|t| t.name == *n) { + if valid_name(n) && known.insert(n.clone()) { let color = COLORS[d.xml.tags.len() % COLORS.len()]; d.xml.tags.push(XmlTag { name: n.clone(), color }); } diff --git a/crates/engine/src/dtd.rs b/crates/engine/src/dtd.rs index 7128ba84e..c0d45d820 100644 --- a/crates/engine/src/dtd.rs +++ b/crates/engine/src/dtd.rs @@ -29,6 +29,8 @@ enum Model { #[derive(Clone, Debug, Default)] pub struct Dtd { elements: Vec<(String, Model)>, + /// Each declared name's first declaration (which wins), by name. + index: HashMap, /// Element → its `#REQUIRED` attributes. required: HashMap>, } @@ -47,7 +49,7 @@ impl Dtd { } fn model(&self, name: &str) -> Option<&Model> { - self.elements.iter().find(|(n, _)| n == name).map(|(_, m)| m) + self.elements.get(*self.index.get(name)?).map(|(_, m)| m) } } @@ -252,6 +254,7 @@ pub fn parse(src: &str) -> Result { let rest = rest.trim_start(); let name: String = rest.chars().take_while(|c| !c.is_whitespace()).collect(); let model = parse_model(&rest[name.len()..]).map_err(|e| format!(": {e}"))?; + dtd.index.entry(name.clone()).or_insert(dtd.elements.len()); dtd.elements.push((name, model)); } else if let Some(rest) = d.strip_prefix("ATTLIST") { // name (attr type default)*; only `#REQUIRED` matters here. @@ -609,6 +612,15 @@ mod tests { assert!(parse("just text").is_err()); } + /// An element declared twice keeps its first model (the index points there). + #[test] + fn the_first_declaration_of_an_element_wins() { + let dtd = parse("\n\n").unwrap(); + assert_eq!(dtd.element_names().collect::>(), ["a", "b", "a"]); + assert_eq!(validate(&dtd, ""), vec![]); + assert!(!validate(&dtd, "").is_empty()); + } + /// A few hundred bytes of nested parameter entities used to inflate multiplicatively across /// the eight wholly-string-replacing passes (#713); expansion is capped. #[test]