From eba00a0deca4f7bba04ec656efe79b4e63aeaec6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 16:20:34 -0500 Subject: [PATCH] Opening documents: index lookups in Document::check and the format-1 style upgrade (#2218) * fix(doc): index items, threads and styles when opening a document * Apply review fixes - crates/doc/src/tests.rs * address review: borrow style names in pin_para_default --------- Co-authored-by: aiongg <67543387+aiongg@users.noreply.github.com> Co-authored-by: autocraft-bot[bot] <354215646+autocraft-bot[bot]@users.noreply.github.com> --- crates/doc/src/lib.rs | 24 +++++++++++++++++++----- crates/doc/src/styles.rs | 19 +++++++++++++++++-- crates/doc/src/tests.rs | 19 +++++++++++++++++++ crates/format/src/lib.rs | 9 ++++++--- crates/format/src/tests.rs | 17 +++++++++++++++++ 5 files changed, 78 insertions(+), 10 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 8d1f99456..2be595e3d 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1136,6 +1136,11 @@ impl Document { } } let mut ids = std::collections::HashSet::new(); + // Every item's story (text frames), by id, and every (story, frame) of the threads: built + // once, so checking each reference both ways is a lookup, not a search of the document. + let mut frame_story: std::collections::HashMap> = std::collections::HashMap::new(); + let threads: std::collections::HashSet<(StoryId, ItemId)> = + self.stories.iter().flat_map(|(sid, st)| st.frames.iter().map(move |f| (*sid, *f))).collect(); for sp in self.spreads.iter().chain(self.parents.iter()) { for it in &sp.items { let mut dup = None; @@ -1143,20 +1148,30 @@ impl Document { if !ids.insert(i.id.0) { dup = Some(i.id); } + frame_story.insert(i.id, i.text_frame().map(|t| t.story)); }); if let Some(d) = dup { return Err(DocError::Invalid(format!("duplicate item id {d}"))); } } } + let embedded = self.anchored_items(); + // An anchored item's story, by id (the first of an id, as a search would find it). + let mut anchored: std::collections::HashMap> = std::collections::HashMap::new(); + for (_, item) in &embedded { + anchored.entry(item.id).or_insert_with(|| item.text_frame().map(|t| t.story)); + } for (sid, st) in &self.stories { st.check().map_err(DocError::Invalid)?; if st.id != *sid { return Err(DocError::Invalid(format!("story key {sid} != id {}", st.id))); } for f in &st.frames { - let item = self.item(*f).or_else(|| self.anchored_item(*f)).ok_or(DocError::NoItem(*f))?; - match item.text_frame().map(|t| t.story) { + let story = match frame_story.get(f) { + Some(story) => *story, + None => anchored.get(f).copied().ok_or(DocError::NoItem(*f))?, + }; + match story { Some(s) if s == *sid => {} other => return Err(DocError::Invalid(format!("frame {f} in story {sid} points at {other:?}"))), } @@ -1168,7 +1183,7 @@ impl Document { let mut bad = None; it.walk(&mut |i| { if let Some(t) = i.text_frame() - && !self.stories.get(&t.story).is_some_and(|s| s.frames.contains(&i.id)) + && !threads.contains(&(t.story, i.id)) { bad = Some(i.id); } @@ -1178,14 +1193,13 @@ impl Document { } } } - let embedded = self.anchored_items(); let mut edges: std::collections::HashMap> = std::collections::HashMap::new(); for (owner, item) in embedded { if !ids.insert(item.id.0) { return Err(DocError::Invalid(format!("duplicate item id {}", item.id))); } if let Some(tf) = item.text_frame() { - if !self.stories.get(&tf.story).is_some_and(|s| s.frames.contains(&item.id)) { + if !threads.contains(&(tf.story, item.id)) { return Err(DocError::Invalid(format!("embedded text frame {} not in its story's thread", item.id))); } edges.entry(owner).or_default().push(tf.story); diff --git a/crates/doc/src/styles.rs b/crates/doc/src/styles.rs index 7605cc447..8aa5c8132 100644 --- a/crates/doc/src/styles.rs +++ b/crates/doc/src/styles.rs @@ -775,14 +775,29 @@ impl Styles { /// The paragraph style chain from root to `name` (cycle-safe): resolution starts at its first. pub fn para_chain(&self, name: &str) -> Vec<&ParagraphStyle> { + Self::para_chain_by(name, |n| self.para(n)) + } + + /// The paragraph styles by name, the first of each name (as [`Self::para`] finds it), for + /// callers that look up every style: one map instead of a search per style. + pub fn para_index(&self) -> std::collections::HashMap<&str, &ParagraphStyle> { + let mut index = std::collections::HashMap::new(); + for s in &self.paragraph { + index.entry(s.name.as_str()).or_insert(s); + } + index + } + + /// [`Self::para_chain`] with styles found through `find` (a [`Self::para_index`]). + pub fn para_chain_by<'a>(name: &str, find: impl Fn(&str) -> Option<&'a ParagraphStyle>) -> Vec<&'a ParagraphStyle> { let mut chain = Vec::new(); - let mut cur = self.para(name); + let mut cur = find(name); while let Some(s) = cur { if chain.len() >= MAX_DEPTH || chain.iter().any(|c: &&ParagraphStyle| c.name == s.name) { break; } chain.push(s); - cur = s.based_on.as_deref().and_then(|b| self.para(b)); + cur = s.based_on.as_deref().and_then(&find); } chain.reverse(); chain diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index 08a525251..dd6805f41 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -370,6 +370,25 @@ fn placed_pdf_without_a_crop_choice_loads_with_the_crop_box() { assert_eq!(Asset { pdf_box: Some([0.5, 0.5, 0.5, 0.9]), ..back }.shown_box(), None); } +/// Checking a document looks each frame up once: 20,000 text frames check in well under a +/// second (each lookup searched every item before). +#[test] +fn check_is_linear_in_the_frames() { + let mut d = Document::new(&NewDocument::default()); + let lid = d.default_layer(); + for i in 0..20_000 { + let y = f64::from(i % 100); + d.add_text_frame(SpreadRef::Doc(0), Rect::new(0.0, y, 10.0, y + 1.0), lid, "x", ParaFormat::default()).unwrap(); + } + let t = std::time::Instant::now(); + d.check().unwrap(); + assert!(t.elapsed() < std::time::Duration::from_secs(10), "{:?}", t.elapsed()); + // A frame its story names but the document doesn't have is still refused. + let sid = *d.stories.keys().next().unwrap(); + Arc::make_mut(d.stories.get_mut(&sid).unwrap()).frames.push(ItemId(u64::MAX)); + assert!(matches!(d.check(), Err(DocError::NoItem(ItemId(u64::MAX))))); +} + #[test] fn adjust_text_ranges_tracks_and_prunes_hyperlinks_and_placeholders() { let mut d = doc(); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 09bdfe334..8f9d8856e 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -312,12 +312,14 @@ fn upgrade(doc: &mut Document, from: u32) { /// is missing. fn pin_para_default(doc: &mut Document, field: fn(&mut ParaAttrs) -> &mut Option, old: T) { let unset = |a: &ParaAttrs| field(&mut a.clone()).is_none(); + // Every style is looked up by name: through one index, not a search of all styles each time. + let index = doc.styles.para_index(); let roots: std::collections::HashSet = doc .styles .paragraph .iter() .filter_map(|s| { - let chain = doc.styles.para_chain(&s.name); + let chain = designcraft_doc::Styles::para_chain_by(&s.name, |n| index.get(n).copied()); if chain.iter().all(|c| unset(&c.para)) { chain.first().map(|c| c.name.clone()) } else { None } }) .collect(); @@ -326,9 +328,10 @@ fn pin_para_default(doc: &mut Document, field: fn(&mut ParaAttrs) -> & *field(&mut s.para) = Some(old.clone()); } } - let styles = doc.styles.clone(); + // Borrowed names: one entry per style already in memory, so no copy and no extra bound needed. + let names: std::collections::HashSet<&str> = doc.styles.paragraph.iter().map(|s| s.name.as_str()).collect(); let mut pin = |st: &mut Story| { - for p in st.paras.iter_mut().filter(|p| styles.para(&p.style).is_none()) { + for p in st.paras.iter_mut().filter(|p| !names.contains(p.style.as_str())) { let v = field(&mut p.para); if v.is_none() { *v = Some(old.clone()); diff --git a/crates/format/src/tests.rs b/crates/format/src/tests.rs index a95bee160..ca57c7770 100644 --- a/crates/format/src/tests.rs +++ b/crates/format/src/tests.rs @@ -355,6 +355,23 @@ fn format_1_keep_lines_without_mode_stays_all_lines() { assert!(!designcraft_doc::ParaProps::default().keep_all_lines); } +/// A format-1 file with many styles upgrades in time linear in them (each style's chain was +/// found by searching every style, twice over). +#[test] +fn format_1_upgrade_of_many_styles_is_linear() { + let mut d = Document::new(&NewDocument::default()); + let st = d.styles_mut(); + for i in 0..20_000 { + st.paragraph.push(para_style(&format!("S{i}"), (i % 2 == 1).then(|| format!("S{}", i - 1)).as_deref(), None)); + } + let bytes = save_v1(&d); + let t = std::time::Instant::now(); + let back = load(&bytes).unwrap(); + assert!(t.elapsed() < std::time::Duration::from_secs(10), "{:?}", t.elapsed()); + assert_eq!(back.styles.para("S0").unwrap().para.keep_all_lines, Some(true)); + assert_eq!(back.styles.para("S1").unwrap().para.keep_all_lines, None, "based on S0, which is pinned"); +} + /// Format 1 wrote paragraph rules, text frame options and numbered lists in full, so their /// old defaults (black rules, auto-size from the top centre, lists continuing across stories) /// load as saved. New documents take InDesign's.