diff --git a/crates/engine/src/cmd/sheet/tabs.rs b/crates/engine/src/cmd/sheet/tabs.rs index 2f87294bb..176c1d542 100644 --- a/crates/engine/src/cmd/sheet/tabs.rs +++ b/crates/engine/src/cmd/sheet/tabs.rs @@ -164,13 +164,15 @@ pub(super) fn move_sheet(s: &mut Session, p: &Json) -> Result { sh.tables.clear(); // table names must be unique sh.preserved = None; // kept file content (form controls, OLE objects…) stays with the original // Object ids are unique across the workbook (selection and the `{kind, id}` commands - // rely on it), so the copied objects get fresh ones. - let mut next = cx.wb.next_object_id(); - let mut fresh = || { - let id = next; - next = next.saturating_add(1); - id - }; + // rely on it), so the copied objects get fresh ones (free ones, even when a file used + // the largest id). + let count = sh.charts.len() + sh.images.len() + sh.shapes.len() + sh.slicers.len(); + let ids = cx.wb.free_object_ids(count); + if ids.len() < count { + return Err(bad("sheet.move", "no free object ids are left for the copied objects")); + } + let mut ids = ids.into_iter(); + let mut fresh = || ids.next().unwrap_or(u32::MAX); sh.charts.iter_mut().for_each(|o| o.id = fresh()); sh.images.iter_mut().for_each(|o| o.id = fresh()); sh.shapes.iter_mut().for_each(|o| o.id = fresh()); diff --git a/crates/engine/src/tests/objects.rs b/crates/engine/src/tests/objects.rs index c39b14e11..e590f4851 100644 --- a/crates/engine/src/tests/objects.rs +++ b/crates/engine/src/tests/objects.rs @@ -281,3 +281,23 @@ fn bubble_sizes_follow_edits_like_the_other_series_references() { s.execute("sheet.rename", json!({"sheet": 0, "name": "Data"})).unwrap(); assert_eq!(sizes(&s), "Data!$F$2:$F$4", "a renamed sheet is renamed in the bubble sizes"); } + +/// A file can carry an object with the largest id. New objects and a copied sheet's objects then +/// take free ids instead of the largest one again, so `{kind, id}` still names one object. +#[test] +fn object_ids_stay_unique_when_a_file_used_the_largest_id() { + let mut s = s(); + let first = id_of(&s.execute("insert.shape", json!({"kind": "rectangle", "at": "B2"})).unwrap(), "shape"); + // The file's shape carries the largest id (set on the model, as a loaded file would). + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().wb).sheet_mut(0).unwrap().shapes.iter_mut().find(|o| o.id == first).unwrap().id = u32::MAX; + let second = id_of(&s.execute("insert.shape", json!({"kind": "ellipse", "at": "D2"})).unwrap(), "shape"); + let third = id_of(&s.execute("insert.shape", json!({"kind": "triangle", "at": "F2"})).unwrap(), "shape"); + assert!(second != u32::MAX && third != u32::MAX && second != third, "new shapes take free ids, not the largest again: {second}, {third}"); + s.execute("sheet.move", json!({"sheet": 0, "to": 1, "copy": true})).unwrap(); + let wb = &s.doc().unwrap().wb; + let mut ids: Vec = wb.sheets.iter().flat_map(|sh| sh.shapes.iter().map(|o| o.id)).collect(); + assert_eq!(ids.len(), 6); + ids.sort_unstable(); + ids.dedup(); + assert_eq!(ids.len(), 6, "every shape id is distinct across both sheets"); +} diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index 486b7019d..c1a7f2913 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -463,26 +463,54 @@ impl Workbook { } None } + /// A free id for a new chart, picture, shape, table or slicer: one more than the largest in + /// use, or, when that is `u32::MAX` (a file can carry any id), the lowest free one. pub fn next_object_id(&self) -> u32 { - let mut m = 0; - for s in &self.sheets { - for c in &s.charts { - m = m.max(c.id); - } - for c in &s.images { - m = m.max(c.id); - } - for c in &s.shapes { - m = m.max(c.id); - } - for c in &s.tables { - m = m.max(c.id); - } - for c in &s.slicers { - m = m.max(c.id); - } + self.free_object_ids(1).first().copied().unwrap_or(u32::MAX) + } + + /// `count` distinct ids no object in the workbook uses, sorted: the ones above the largest id + /// in use while they last, then the lowest free ones. Fewer come back only when the id space + /// is exhausted. + pub fn free_object_ids(&self, count: usize) -> Vec { + let mut used: Vec = self + .sheets + .iter() + .flat_map(|s| { + s.charts + .iter() + .map(|o| o.id) + .chain(s.images.iter().map(|o| o.id)) + .chain(s.shapes.iter().map(|o| o.id)) + .chain(s.tables.iter().map(|o| o.id)) + .chain(s.slicers.iter().map(|o| o.id)) + }) + .collect(); + used.sort_unstable(); + used.dedup(); + let max = used.last().copied().unwrap_or(0); + let mut out = Vec::with_capacity(count.min(1 << 16)); + let mut next = max.checked_add(1); + while out.len() < count + && let Some(id) = next + { + out.push(id); + next = id.checked_add(1); } - m.saturating_add(1) + // Past the top: the gaps below the largest id, from 1 (0 is never an object's id). + let mut taken = used.iter().copied().peekable(); + let mut id = 1u32; + while out.len() < count && id < max { + while taken.next_if(|&t| t < id).is_some() {} + if taken.peek() == Some(&id) { + taken.next(); + } else { + out.push(id); + } + id += 1; + } + out.sort_unstable(); + out } } @@ -534,7 +562,7 @@ mod tests { } #[test] - fn object_ids_saturate_instead_of_overflowing() { + fn object_ids_never_overflow_or_repeat() { let mut wb = Workbook::new(); let image = |id| crate::Image { id, anchor: Default::default(), data: Default::default(), mime: String::new(), alt: String::new() }; assert_eq!(wb.next_object_id(), 1); @@ -545,6 +573,40 @@ mod tests { if let Some(sh) = wb.sheet_mut(0) { sh.images.push(image(u32::MAX)); } - assert_eq!(wb.next_object_id(), u32::MAX, "an id read from a file at the maximum saturates"); + // An id read from a file at the maximum: the lowest free id, not the maximum again. + assert_eq!(wb.next_object_id(), 1); + if let Some(sh) = wb.sheet_mut(0) { + sh.images.push(image(1)); + sh.images.push(image(3)); + } + assert_eq!(wb.next_object_id(), 2); + assert_eq!(wb.free_object_ids(4), vec![2, 4, 5, 6], "the gaps, skipping 1, 3 and 7"); + let mut wb = Workbook::new(); + if let Some(sh) = wb.sheet_mut(0) { + sh.images.push(image(u32::MAX - 1)); + } + assert_eq!(wb.free_object_ids(3), vec![1, 2, u32::MAX], "the top first, then from 1"); + } + + /// A file can use the largest id and the one below it: nothing is free above them, so the free + /// ids are the lowest unused ones from 1 (0 is never an object's id), and a large request is + /// served in time linear in the count and the ids in use. + #[test] + fn free_object_ids_come_from_below_when_nothing_is_free_above() { + let mut wb = Workbook::new(); + let image = |id| crate::Image { id, anchor: Default::default(), data: Default::default(), mime: String::new(), alt: String::new() }; + if let Some(sh) = wb.sheet_mut(0) { + sh.images.push(image(u32::MAX)); + sh.images.push(image(u32::MAX - 1)); + sh.images.push(image(0)); + } + assert_eq!(wb.free_object_ids(3), vec![1, 2, 3]); + assert_eq!(wb.next_object_id(), 1); + // The 100,000 lowest free ids: distinct, ascending, none of them in use. + let many = wb.free_object_ids(100_000); + assert_eq!(many.len(), 100_000); + assert!(many.windows(2).all(|w| w[0] < w[1]), "ascending without repeats"); + assert!(!many.contains(&0) && !many.contains(&(u32::MAX - 1)) && !many.contains(&u32::MAX)); + assert_eq!(wb.free_object_ids(0), Vec::::new()); } }