diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs index b74fc7e99..9a580c791 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -7,7 +7,7 @@ //! the open document. Scripts made with Automate › New Script or Record Actions are a separate, //! older feature with their own storage; macros neither read nor change them. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap, HashSet}; use gridcraft_model::{DefinedName, Macro, Workbook}; use serde_json::{Value as Json, json}; @@ -142,6 +142,7 @@ pub fn load(wb: &mut Workbook) -> Vec { // All of them go through the same limits, and a clashing name gets a suffix. let mut all = std::mem::take(&mut wb.macros); all.extend(found); + let mut taken = Taken::default(); for m in all { if wb.macros.len() >= gridcraft_model::MAX_MACROS { bad += 1; @@ -149,12 +150,15 @@ pub fn load(wb: &mut Workbook) -> Vec { } let mut m = m; let checked = m.check_limits().and_then(|()| { - m.name = unique_name(wb, m.name.trim()); + m.name = taken.unique(m.name.trim()); // The macro as it is kept, renamed, must still pass, or the next save would lose it. m.check_limits().and_then(|()| check_size(&m)) }); match checked { - Ok(()) => wb.macros.push(m), + Ok(()) => { + taken.insert(&m.name); + wb.macros.push(m); + } Err(_) => bad += 1, } } @@ -181,19 +185,66 @@ fn macro_item(n: &DefinedName) -> Option<(String, Option)> { /// `name`, or `name (2)`, `name (3)`… if a macro already has it. A long name is shortened to make /// room for the suffix, so the result stays within [`gridcraft_model::MAX_MACRO_NAME`] characters. pub fn unique_name(wb: &Workbook, name: &str) -> String { - let name = if name.is_empty() { "Macro" } else { name }; - if find(wb, name).is_none() { - return name.to_string(); + let mut taken = Taken::default(); + for m in &wb.macros { + taken.insert(&m.name); } - (2..) - .map(|n| { + taken.unique(name) +} + +/// Macro names in use, compared as [`find`] compares them, and the next suffix to try for each +/// shortened name and suffix width, so naming many clashing macros takes near-linear work. +#[derive(Default)] +struct Taken { + names: HashSet, + next: HashMap<(String, usize), u64>, + /// Name lookups made, for tests. + probes: usize, +} + +impl Taken { + fn key(name: &str) -> String { + name.trim().to_ascii_lowercase() + } + + fn insert(&mut self, name: &str) { + self.names.insert(Taken::key(name)); + } + + fn contains(&mut self, name: &str) -> bool { + self.probes = self.probes.saturating_add(1); + self.names.contains(&Taken::key(name)) + } + + /// The first of `name`, `name (2)`, `name (3)`… not in use. + fn unique(&mut self, name: &str) -> String { + let name = if name.is_empty() { "Macro" } else { name }; + if !self.contains(name) { + return name.to_string(); + } + let mut n: u64 = 2; + loop { let suffix = format!(" ({n})"); let keep = gridcraft_model::MAX_MACRO_NAME.saturating_sub(suffix.chars().count()); let base: String = name.chars().take(keep).collect(); - format!("{}{suffix}", base.trim_end()) - }) - .find(|n| find(wb, n).is_none()) - .unwrap_or_else(|| name.to_string()) + let base = base.trim_end(); + // Every candidate for this base and suffix width below `next` is in use already + // (names are only added), so the search goes on from there. + let key = (Taken::key(base), suffix.len()); + let next = self.next.get(&key).copied().unwrap_or(n); + if next > n { + n = next; + continue; + } + let Some(after) = n.checked_add(1) else { return name.to_string() }; + self.next.insert(key, after); + let candidate = format!("{base}{suffix}"); + if !self.contains(&candidate) { + return candidate; + } + n = after; + } + } } #[cfg(test)] diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs index 63f8cef76..4bd2ac1bc 100644 --- a/crates/engine/src/macros/tests.rs +++ b/crates/engine/src/macros/tests.rs @@ -317,3 +317,53 @@ fn a_macro_in_a_newer_format_is_dropped_not_rewritten() { assert!(back.names.is_empty(), "{:?}", names_of(&back)); check("book.xlsx", &back, &warnings); } + +#[test] +fn many_macros_with_one_name_all_survive_save_and_reopen() { + // MAX_MACROS macros with the same name, at the length limit, so the suffixes run from " (2)" + // to " (1000)" and the kept part of the name shrinks as they widen. + let long = "é".repeat(MAX_MACRO_NAME); + for file in ["same.json", "same.xlsx"] { + let mut wb = Workbook::new(); + wb.macros = (0..MAX_MACROS).map(|i| Macro { name: long.clone(), description: format!("{i}"), ..Macro::default() }).collect(); + let (first, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(&wb, file).unwrap()).unwrap(); + assert!(warnings.is_empty(), "{file}: {warnings:?}"); + assert_eq!(first.macros.len(), MAX_MACROS, "{file}"); + let names: std::collections::HashSet = first.macros.iter().map(|m| m.name.to_lowercase()).collect(); + assert_eq!(names.len(), MAX_MACROS, "{file}: every name is different"); + assert!(first.macros.iter().all(|m| m.name.chars().count() <= MAX_MACRO_NAME && m.check_limits().is_ok()), "{file}"); + assert_eq!(first.macros[0].name, long); + assert_eq!(first.macros[9].name, format!("{} (10)", "é".repeat(MAX_MACRO_NAME - 5))); + assert_eq!(first.macros[MAX_MACROS - 1].name, format!("{} (1000)", "é".repeat(MAX_MACRO_NAME - 7))); + assert!(first.macros.iter().enumerate().all(|(i, m)| m.description == format!("{i}")), "{file}: order kept"); + let (again, warnings) = crate::io::open_bytes(file, &crate::io::save_bytes(&first, file).unwrap()).unwrap(); + assert!(warnings.is_empty(), "{file}: {warnings:?}"); + assert_eq!(again.macros, first.macros, "{file}"); + } +} + +#[test] +fn naming_clashing_macros_takes_linear_work() { + // Each suffix is looked up about once, not once per earlier macro with the same name. + let mut taken = Taken::default(); + // Names a file may already use, in the way of the first suffixes. + for n in 2..=500 { + taken.insert(&format!("Same ({n})")); + } + let mut out = Vec::new(); + for _ in 0..MAX_MACROS { + let name = taken.unique("SAME"); + taken.insert(&name); + out.push(name); + } + assert_eq!(out[..3], ["SAME", "SAME (501)", "SAME (502)"]); + assert!(taken.probes <= 3 * MAX_MACROS, "{} lookups", taken.probes); + // The smallest free suffix is still the one used. + let mut taken = Taken::default(); + for name in ["A", "A (3)"] { + taken.insert(name); + } + assert_eq!(taken.unique("a"), "a (2)"); + taken.insert("a (2)"); + assert_eq!(taken.unique("a"), "a (4)"); +}