diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index bc93b9f55..f54573595 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -22,6 +22,7 @@ mod inquiry; mod layer; mod layout; mod lisp; +mod merge; mod mleader_content; mod mleader_edit; mod modify; diff --git a/crates/engine/src/cmd/blocks/insert_file.rs b/crates/engine/src/cmd/blocks/insert_file.rs index a00dbc64c..7a0492fad 100644 --- a/crates/engine/src/cmd/blocks/insert_file.rs +++ b/crates/engine/src/cmd/blocks/insert_file.rs @@ -6,7 +6,7 @@ //! (names this drawing already has keep this drawing's definitions). The file is read through the //! host's io hooks like OPEN, so the engine stays I/O-agnostic. -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use std::sync::Arc; use cadcraft_doc::{Block, Drawing, Entity, EntityKind, Handle}; @@ -102,7 +102,7 @@ fn define_from(s: &mut Session, src: &Drawing, name: &str) -> Result { // Redefining keeps the existing definition's name (block names are case-insensitive). let name = d.blocks.keys().find(|k| k.eq_ignore_ascii_case(name)).cloned().unwrap_or_else(|| name.to_string()); merge_tables(&mut d, src); - let renames = block_names(&d, src, &name); + let renames = crate::cmd::merge::block_renames(&d, src, Some(&name)); for (from, to) in &renames { let Some(b) = src.blocks.get(from) else { continue }; let mut nb = (**b).clone(); @@ -136,13 +136,7 @@ fn define_from(s: &mut Session, src: &Drawing, name: &str) -> Result { /// Add the source's layers, linetypes and styles that this drawing doesn't have (a name that /// exists keeps this drawing's definition). fn merge_tables(d: &mut Drawing, src: &Drawing) { - fn merge(ours: &mut Vec, theirs: &[T], name: fn(&T) -> &str) { - for t in theirs { - if !ours.iter().any(|o| name(o).eq_ignore_ascii_case(name(t))) { - ours.push(t.clone()); - } - } - } + use crate::cmd::merge::merge_named as merge; merge(&mut d.layers, &src.layers, |l| &l.name); merge(&mut d.linetypes, &src.linetypes, |l| &l.name); merge(&mut d.text_styles, &src.text_styles, |t| &t.name); @@ -152,35 +146,6 @@ fn merge_tables(d: &mut Drawing, src: &Drawing) { merge(&mut d.mline_styles, &src.mline_styles, |t| &t.name); } -/// Which source blocks to copy, and under which name: new names keep theirs, generated -/// (anonymous `*…`) blocks that collide get a fresh generated name, and a named block this drawing -/// already has (or the block being defined) keeps this drawing's definition (not copied). -fn block_names(d: &Drawing, src: &Drawing, defining: &str) -> BTreeMap { - let mut out = BTreeMap::new(); - let mut taken: BTreeSet = d.blocks.keys().map(|k| k.to_ascii_uppercase()).collect(); - taken.insert(defining.to_ascii_uppercase()); - for (name, b) in &src.blocks { - let upper = name.to_ascii_uppercase(); - if upper.starts_with("*MODEL_SPACE") || upper.starts_with("*PAPER_SPACE") { - continue; - } - let to = if !taken.contains(&upper) { - name.clone() - } else if b.anonymous || name.starts_with('*') { - let prefix: String = name.chars().take_while(|c| !c.is_ascii_digit()).collect(); - match (1..=1_000_000).map(|i| format!("{prefix}{i}")).find(|n| !taken.contains(&n.to_ascii_uppercase())) { - Some(n) => n, - None => continue, - } - } else { - continue; - }; - taken.insert(to.to_ascii_uppercase()); - out.insert(name.clone(), to); - } - out -} - /// A copy of a source entity for this drawing: a fresh handle, block references renamed, and /// links to other source objects (associative dimension points) dropped. fn adopt(d: &mut Drawing, src: &Drawing, e: &Entity, renames: &BTreeMap) -> Entity { diff --git a/crates/engine/src/cmd/blocks/xref/load.rs b/crates/engine/src/cmd/blocks/xref/load.rs index b47aa00e0..981df918d 100644 --- a/crates/engine/src/cmd/blocks/xref/load.rs +++ b/crates/engine/src/cmd/blocks/xref/load.rs @@ -10,6 +10,7 @@ //! loaded further up (a circular reference) is not loaded again. Every failure leaves the xref's //! block empty with a status saying why; nothing here stops the host drawing from opening. +use std::collections::HashSet; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -339,14 +340,15 @@ fn load_in(d: &mut Drawing, name: &str, host: Option<&Path>, chosen: bool, cx: & let (status, detail) = match fetch(&path, host, chosen, cx, depth) { Ok((x, file)) => match import(d, &key, &x, cx) { Ok(()) => (XrefStatus::Loaded, file.to_string_lossy().into_owned()), - Err(e) => (XrefStatus::Unresolved, e), + Err(e) => { + // Whatever made it in before the failure goes again. A fetch that failed imported + // nothing, so it doesn't pay for a second scan of the block table. + clear(d, &key); + (XrefStatus::Unresolved, e) + } }, Err(e) => e, }; - if status != XrefStatus::Loaded { - // Whatever made it in before the failure goes again. - clear(d, &key); - } set_status(d, &key, status); (status, detail) } @@ -390,9 +392,15 @@ fn import(d: &mut Drawing, key: &str, x: &Drawing, cx: &mut Ctx) -> Result<(), S return Err(format!("more than {MAX_XREF_OBJECTS} objects")); } let dep = |sym: Sym, n: &str| (!shared(sym, n)).then(|| format!("{key}|{n}")); + // The host's names per table, looked up in a set built once: both drawings are hostile input, + // so a scan per imported record would be quadratic. + fn names(all: &[T], name: fn(&T) -> &str) -> HashSet { + all.iter().map(|t| name(t).to_ascii_uppercase()).collect() + } + let mut have = names(&d.layers, |l| &l.name); for l in &x.layers { if let Some(n) = dep(Sym::Layer, &l.name) - && d.layer(&n).is_none() + && have.insert(n.to_ascii_uppercase()) { let mut l = l.clone(); l.name = n; @@ -400,39 +408,44 @@ fn import(d: &mut Drawing, key: &str, x: &Drawing, cx: &mut Ctx) -> Result<(), S d.layers.push(l); } } + let mut have = names(&d.linetypes, |t| &t.name); for t in &x.linetypes { if let Some(n) = dep(Sym::Linetype, &t.name) - && d.linetype(&n).is_none() + && have.insert(n.to_ascii_uppercase()) { d.linetypes.push(cadcraft_doc::Linetype { name: n, ..t.clone() }); } } + let mut have = names(&d.text_styles, |t| &t.name); for t in &x.text_styles { if let Some(n) = dep(Sym::Style, &t.name) - && d.text_style(&n).is_none() + && have.insert(n.to_ascii_uppercase()) { d.text_styles.push(cadcraft_doc::TextStyle { name: n, ..t.clone() }); } } + let mut have = names(&d.dim_styles, |t| &t.name); for t in &x.dim_styles { if let Some(n) = dep(Sym::DimStyle, &t.name) - && d.dim_style(&n).is_none() + && have.insert(n.to_ascii_uppercase()) { let text_style = dep(Sym::Style, &t.text_style).unwrap_or_else(|| t.text_style.clone()); d.dim_styles.push(cadcraft_doc::DimStyle { name: n, text_style, ..t.clone() }); } } + let mut have = names(&d.mleader_styles, |t| &t.name); for t in &x.mleader_styles { if let Some(n) = dep(Sym::MLeaderStyle, &t.name) - && !d.mleader_styles.iter().any(|o| o.name.eq_ignore_ascii_case(&n)) + && have.insert(n.to_ascii_uppercase()) { let text_style = dep(Sym::Style, &t.text_style).unwrap_or_else(|| t.text_style.clone()); d.mleader_styles.push(cadcraft_doc::MLeaderStyle { name: n, text_style, ..t.clone() }); } } + let mut have = names(&d.table_styles, |t| &t.name); for t in &x.table_styles { if let Some(n) = dep(Sym::TableStyle, &t.name) - && !d.table_styles.iter().any(|o| o.name.eq_ignore_ascii_case(&n)) + && have.insert(n.to_ascii_uppercase()) { d.table_styles.push(cadcraft_doc::TableStyle { name: n, ..t.clone() }); } diff --git a/crates/engine/src/cmd/clipboard.rs b/crates/engine/src/cmd/clipboard.rs index 49a7e7f33..3ddf8cf5b 100644 --- a/crates/engine/src/cmd/clipboard.rs +++ b/crates/engine/src/cmd/clipboard.rs @@ -10,7 +10,7 @@ //! The clipboard can also travel as DXF text ([`system_text`], [`load_system_text`]), so a front //! end can put it on the system clipboard and paste it into another CADCraft window. -use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::collections::{BTreeMap, HashSet}; use std::sync::Arc; use cadcraft_doc::{Block, Drawing, Entity, EntityKind, HVal, Handle}; @@ -279,20 +279,14 @@ fn defs_for(src: &Drawing, ents: &[Entity]) -> Drawing { /// Add `defs`' table records and blocks that `d` doesn't have. Returns the blocks copied, from /// their name in `defs` to their name in `d`. fn merge(d: &mut Drawing, defs: &Drawing) -> BTreeMap { - fn add(ours: &mut Vec, theirs: &[T], name: fn(&T) -> &str) { - for t in theirs { - if !ours.iter().any(|o| name(o).eq_ignore_ascii_case(name(t))) { - ours.push(t.clone()); - } - } - } + use crate::cmd::merge::merge_named as add; add(&mut d.layers, &defs.layers, |l| &l.name); add(&mut d.linetypes, &defs.linetypes, |l| &l.name); add(&mut d.text_styles, &defs.text_styles, |t| &t.name); add(&mut d.dim_styles, &defs.dim_styles, |t| &t.name); add(&mut d.mleader_styles, &defs.mleader_styles, |t| &t.name); add(&mut d.table_styles, &defs.table_styles, |t| &t.name); - let renames = block_names(d, defs); + let renames = crate::cmd::merge::block_renames(d, defs, None); for (from, to) in &renames { let Some(b) = defs.blocks.get(from) else { continue }; let mut nb = (**b).clone(); @@ -308,34 +302,6 @@ fn merge(d: &mut Drawing, defs: &Drawing) -> BTreeMap { renames } -/// Which blocks of `defs` to copy, and under which name: a new name keeps its own, a generated -/// (anonymous `*…`) block whose name is taken gets a fresh generated name, and a named block `d` -/// already has keeps `d`'s definition (not copied). -fn block_names(d: &Drawing, defs: &Drawing) -> BTreeMap { - let mut out = BTreeMap::new(); - let mut taken: BTreeSet = d.blocks.keys().map(|k| up(k)).collect(); - for (name, b) in &defs.blocks { - let upper = up(name); - if upper.starts_with("*MODEL_SPACE") || upper.starts_with("*PAPER_SPACE") { - continue; - } - let to = if !taken.contains(&upper) { - name.clone() - } else if b.anonymous || name.starts_with('*') { - let prefix: String = name.chars().take_while(|c| !c.is_ascii_digit()).collect(); - match (1..=1_000_000).map(|i| format!("{prefix}{i}")).find(|n| !taken.contains(&up(n))) { - Some(n) => n, - None => continue, - } - } else { - continue; - }; - taken.insert(up(&to)); - out.insert(name.clone(), to); - } - out -} - /// A copy of a clipboard object for `d`: a fresh handle, block references renamed, and links to /// objects of the source drawing (associative dimension points) dropped. fn adopt(d: &mut Drawing, e: &Entity, renames: &BTreeMap) -> Entity { diff --git a/crates/engine/src/cmd/exchange.rs b/crates/engine/src/cmd/exchange.rs index 2a6a01f67..f0fbb9cef 100644 --- a/crates/engine/src/cmd/exchange.rs +++ b/crates/engine/src/cmd/exchange.rs @@ -7,7 +7,7 @@ //! stays I/O-agnostic; the JSON forms never open dialogs (the UI asks for files when these //! commands come from a menu, the tool bar or the command line). -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use std::sync::Arc; use cadcraft_color::Rgb; @@ -166,7 +166,7 @@ fn import_drawing(s: &mut Session, src: &Drawing, at: Vec2, scale: f64, file: &s let space = s.space(); let d = s.doc_mut()?; let (layers, styles) = merge_tables(d, src); - let renames = block_names(d, src); + let renames = crate::cmd::merge::block_renames(d, src, None); for (from, to) in &renames { let Some(b) = src.blocks.get(from) else { continue }; let mut nb = (**b).clone(); @@ -213,16 +213,7 @@ fn read_file(_path: &str) -> Result> { /// Add the source's layers, linetypes and styles that this drawing doesn't have (a name that /// exists keeps this drawing's definition). Returns how many layers and how many other records were added. fn merge_tables(d: &mut Drawing, src: &Drawing) -> (usize, usize) { - fn merge(ours: &mut Vec, theirs: &[T], name: fn(&T) -> &str) -> usize { - let mut n = 0; - for t in theirs { - if !ours.iter().any(|o| name(o).eq_ignore_ascii_case(name(t))) { - ours.push(t.clone()); - n += 1; - } - } - n - } + use crate::cmd::merge::merge_named as merge; let layers = merge(&mut d.layers, &src.layers, |l| &l.name); let others = merge(&mut d.linetypes, &src.linetypes, |l| &l.name) + merge(&mut d.text_styles, &src.text_styles, |t| &t.name) @@ -232,35 +223,6 @@ fn merge_tables(d: &mut Drawing, src: &Drawing) -> (usize, usize) { (layers, others) } -/// Which source blocks to copy, and under which name: new names keep theirs, generated -/// (anonymous `*…`) blocks that collide get a fresh generated name, and a named block this drawing -/// already has keeps this drawing's definition (not copied). -fn block_names(d: &Drawing, src: &Drawing) -> BTreeMap { - let mut out = BTreeMap::new(); - let mut taken: BTreeSet = d.blocks.keys().map(|k| k.to_ascii_uppercase()).collect(); - for (name, b) in &src.blocks { - let upper = name.to_ascii_uppercase(); - if upper.starts_with("*MODEL_SPACE") || upper.starts_with("*PAPER_SPACE") { - continue; - } - let anonymous = b.anonymous || name.starts_with('*'); - let to = if !taken.contains(&upper) { - name.clone() - } else if anonymous { - let prefix: String = name.chars().take_while(|c| !c.is_ascii_digit()).collect(); - match (1..=1_000_000).map(|i| format!("{prefix}{i}")).find(|n| !taken.contains(&n.to_ascii_uppercase())) { - Some(n) => n, - None => continue, - } - } else { - continue; - }; - taken.insert(to.to_ascii_uppercase()); - out.insert(name.clone(), to); - } - out -} - /// A copy of a source entity for this drawing: a fresh handle, block references renamed, and /// links to other source objects (associative dimension points) dropped. fn adopt(d: &mut Drawing, e: &Entity, renames: &BTreeMap) -> Entity { diff --git a/crates/engine/src/cmd/file/compare/export.rs b/crates/engine/src/cmd/file/compare/export.rs index f3801049d..667a19a36 100644 --- a/crates/engine/src/cmd/file/compare/export.rs +++ b/crates/engine/src/cmd/file/compare/export.rs @@ -20,7 +20,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use cadcraft_doc::color::{Color, Rgb}; -use cadcraft_doc::{Block, CellStyle, DimStyle, Drawing, Entity, EntityKind, EntityStore, Group, Handle, MAX_BLOCK_DEPTH, TableStyle}; +use cadcraft_doc::{Block, CellStyle, DimStyle, Drawing, Entity, EntityKind, EntityStore, Group, Handle, Layer, MAX_BLOCK_DEPTH, TableStyle}; use serde_json::{Value, json}; use super::super::super::{CommandSpec, bad}; @@ -197,6 +197,7 @@ fn snapshot(cur: &Drawing, c: &Comparison, folders: &Folders) -> Snapshot { d.skipped_records.clear(); d.handseed = d.handseed.max(other.handseed); + use crate::cmd::merge::merge_named as merge; merge(&mut d.layers, &other.layers, |t| &t.name); merge(&mut d.linetypes, &other.linetypes, |t| &t.name); merge(&mut d.text_styles, &other.text_styles, |t| &t.name); @@ -235,22 +236,17 @@ fn snapshot(cur: &Drawing, c: &Comparison, folders: &Folders) -> Snapshot { Snapshot { drawing: d, current, compared, counts: (here.len(), there.len()), truncated: diff.truncated, blocks_left_out, uncolored } } -/// Add the entries of `from` whose names `to` lacks (compared without letter case). -fn merge(to: &mut Vec, from: &[T], name: impl Fn(&T) -> &String) { - for t in from { - if !to.iter().any(|x| name(x).eq_ignore_ascii_case(name(t))) { - to.push(t.clone()); - } - } -} - /// Add copies of `objs` to model space with new handles, their block references renamed by /// `renames` and their files rebased on `folder`, each painted in its comparison color, and /// group them as `name` (or `name_2`… when that group name is taken). Returns the group's name. fn add_group(d: &mut Drawing, name: &str, objs: &[(&Entity, Color)], renames: &Renames, folder: Option<&Path>, tinted: &mut Tinted) -> String { let mut members = Vec::new(); + // The layers this drawing has, looked up once per object rather than scanned. + let mut layers: HashSet = d.layers.iter().map(|l| l.name.to_ascii_uppercase()).collect(); for (e, color) in objs { - d.ensure_layer(&e.common.layer); + if layers.insert(e.common.layer.to_ascii_uppercase()) { + d.layers.push(Layer::new(&e.common.layer)); + } let h = d.new_handle(); let mut copy = Entity { handle: h, ..(*e).clone() }; detach(&mut copy.kind); diff --git a/crates/engine/src/cmd/merge.rs b/crates/engine/src/cmd/merge.rs new file mode 100644 index 000000000..119f678b7 --- /dev/null +++ b/crates/engine/src/cmd/merge.rs @@ -0,0 +1,99 @@ +//! Bringing another drawing's symbol tables and blocks into this one (INSERT of a file, IMPORT, +//! paste, COMPARE export). +//! +//! Both drawings are hostile input, so the work is linear in their sizes: names are looked up in a +//! set built once per table, and a generated block name continues from the last one allocated for +//! its prefix instead of probing every taken number again (CWE-407). + +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; + +use cadcraft_doc::Drawing; + +/// Add the entries of `theirs` whose names `ours` lacks (compared without letter case; a name that +/// exists keeps `ours`' definition). Returns how many were added. +pub(crate) fn merge_named(ours: &mut Vec, theirs: &[T], name: fn(&T) -> &str) -> usize { + let mut have: HashSet = ours.iter().map(|o| name(o).to_ascii_uppercase()).collect(); + let mut added = 0; + for t in theirs { + if have.insert(name(t).to_ascii_uppercase()) { + ours.push(t.clone()); + added += 1; + } + } + added +} + +/// Which blocks of `src` to copy into `d`, and under which name: a new name keeps its own, a +/// generated (anonymous `*…`) block whose name is taken gets a fresh generated name, and a named +/// block `d` already has (or `defining`, the block being defined from `src`) keeps `d`'s +/// definition (not copied). +pub(crate) fn block_renames(d: &Drawing, src: &Drawing, defining: Option<&str>) -> BTreeMap { + let mut out = BTreeMap::new(); + let mut taken: BTreeSet = d.blocks.keys().map(|k| k.to_ascii_uppercase()).collect(); + taken.extend(defining.map(str::to_ascii_uppercase)); + // The next number to try per generated prefix: numbers below it are taken. + let mut next: HashMap = HashMap::new(); + for (name, b) in &src.blocks { + let upper = name.to_ascii_uppercase(); + if upper.starts_with("*MODEL_SPACE") || upper.starts_with("*PAPER_SPACE") { + continue; + } + let to = if !taken.contains(&upper) { + name.clone() + } else if b.anonymous || name.starts_with('*') { + let prefix: String = name.chars().take_while(|c| !c.is_ascii_digit()).collect(); + let cursor = next.entry(prefix.to_ascii_uppercase()).or_insert(1); + match (*cursor..=1_000_000).map(|i| (i, format!("{prefix}{i}"))).find(|(_, n)| !taken.contains(&n.to_ascii_uppercase())) { + Some((i, n)) => { + *cursor = i + 1; + n + } + None => continue, + } + } else { + continue; + }; + taken.insert(to.to_ascii_uppercase()); + out.insert(name.clone(), to); + } + out +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use cadcraft_doc::{Block, Layer}; + + use super::*; + + #[test] + fn merge_keeps_ours_and_adds_the_rest_without_regard_to_case() { + let mut ours = vec![Layer::new("Walls")]; + let theirs = vec![Layer::new("WALLS"), Layer::new("Doors"), Layer::new("doors")]; + assert_eq!(merge_named(&mut ours, &theirs, |l| &l.name), 1); + assert_eq!(ours.iter().map(|l| l.name.as_str()).collect::>(), ["Walls", "Doors"]); + } + + /// Many colliding anonymous blocks get consecutive names in one pass, not a rescan each. + #[test] + fn anonymous_collisions_take_consecutive_numbers() { + let mut d = Drawing::new_imperial(); + let mut src = Drawing::new_imperial(); + for i in 1..=5000 { + let mut b = Block::new(&format!("*U{i}")); + b.anonymous = true; + d.blocks.insert(b.name.clone(), Arc::new(b.clone())); + src.blocks.insert(b.name.clone(), Arc::new(b)); + } + let started = std::time::Instant::now(); + let renames = block_renames(&d, &src, None); + assert!(started.elapsed() < std::time::Duration::from_secs(2)); + assert_eq!(renames.len(), 5000); + assert_eq!(renames.get("*U1").map(String::as_str), Some("*U5001")); + let distinct: HashSet<&String> = renames.values().collect(); + assert_eq!(distinct.len(), 5000); + // Every new name is the next free number: nothing below 5001 is free, nothing above 10000 is needed. + assert!(renames.values().all(|n| n.strip_prefix("*U").and_then(|i| i.parse::().ok()).is_some_and(|i| (5001..=10000).contains(&i)))); + } +}