pptx: read a notes or comments part for the first slide that names it

crates/engine/src/cmd/table.rs

@@ -285,8 +285,6 @@ fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> R
     };
     let r = edit_shapes(s, &json!({"id": id}), cmd, |sh| match &mut sh.kind {
         ShapeKind::Table(t) => {
-            // Edits walk the grid: bound it first (a loaded table need not be normalized yet).
-            t.normalize();
             f(t)?;
             t.normalize();
             let (w, h) = (t.width(), t.height());

crates/model/src/table.rs

@@ -77,9 +77,6 @@ impl Default for Table {
     }
 }
 
-/// Cells a table may hold after [`Table::normalize`] (PowerPoint's own limit is 75 x 75).
-pub const MAX_CELLS: usize = 100_000;
-
 impl Table {
     pub fn new(rows: usize, cols: usize, width: f64, row_height: f64) -> Self {
         let rows = rows.clamp(1, 1000);
@@ -167,12 +164,8 @@ impl Table {
         true
     }
     /// The merged areas: every uncovered cell spanning more than one cell, clipped to the grid.
-    /// Merges of a well-formed table do not overlap, so together they cover at most the grid; a
-    /// file's overlapping spans past that are dropped, so rebuilding them cannot revisit cells
-    /// without bound.
     fn merges(&self) -> Vec<Merge> {
         let (nr, nc) = (self.rows.len(), self.cols.len());
-        let mut area = nr.saturating_mul(nc);
         let mut out = vec![];
         for (r, row) in self.rows.iter().enumerate() {
             for (c, cell) in row.cells.iter().enumerate().take(nc) {
@@ -181,10 +174,7 @@ impl Table {
                 }
                 let rs = (cell.row_span.max(1) as usize).min(nr - r);
                 let gs = (cell.grid_span.max(1) as usize).min(nc - c);
-                if (rs > 1 || gs > 1)
-                    && let Some(left) = area.checked_sub(rs.saturating_mul(gs))
-                {
-                    area = left;
+                if rs > 1 || gs > 1 {
                     out.push(Merge { r, c, rs, gs });
                 }
             }
@@ -293,25 +283,21 @@ impl Table {
             return false;
         }
         // Spans come from the file and may reach past the table; only walk the cells that exist.
-        let end = r.saturating_add(rs).min(self.rows.len());
-        let gs = gs.min(self.cols.len().saturating_sub(c));
-        for row in self.rows.get_mut(r..end).unwrap_or_default() {
-            for x in row.cells.iter_mut().skip(c).take(gs) {
-                x.h_merge = false;
-                x.v_merge = false;
-                x.grid_span = 1;
-                x.row_span = 1;
+        for rr in r..r.saturating_add(rs).min(self.rows.len()) {
+            for cc in c..c.saturating_add(gs).min(self.cols.len()) {
+                if let Some(x) = self.cell_mut(rr, cc) {
+                    x.h_merge = false;
+                    x.v_merge = false;
+                    x.grid_span = 1;
+                    x.row_span = 1;
+                }
             }
         }
         true
     }
-    /// Make every row have exactly `cols.len()` cells (repair after loading). A grid over
-    /// [`MAX_CELLS`] loses its last rows (and columns) first, so a file naming many rows and many
-    /// columns but few cells cannot make this allocate their product.
+    /// Make every row have exactly `cols.len()` cells (repair after loading).
     pub fn normalize(&mut self) {
-        self.cols.truncate(MAX_CELLS);
         let n = self.cols.len();
-        self.rows.truncate((MAX_CELLS / n.max(1)).max(1));
         for r in &mut self.rows {
             r.cells.resize_with(n, || Cell::new(""));
             for c in &mut r.cells {
@@ -527,42 +513,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn normalize_caps_a_sparse_grid_at_max_cells() {
-        // 1,000 columns and 1,000 empty rows would be a million cells.
-        let mut t = Table { cols: vec![10.0; 1000], rows: vec![Row::default(); 1000], ..Default::default() };
-        t.normalize();
-        assert_eq!(t.cols.len(), 1000);
-        assert_eq!(t.rows.len(), MAX_CELLS / 1000);
-        assert!(t.rows.iter().all(|r| r.cells.len() == 1000));
-        // Columns alone past the cap keep one row of MAX_CELLS cells.
-        let mut t = Table { cols: vec![1.0; MAX_CELLS + 5], rows: vec![Row::default(); 3], ..Default::default() };
-        t.normalize();
-        assert_eq!((t.cols.len(), t.rows.len()), (MAX_CELLS, 1));
-        // A normal table is untouched.
-        let mut t = Table::new(4, 4, 400.0, 30.0);
-        t.normalize();
-        assert_eq!((t.n_rows(), t.n_cols()), (4, 4));
-    }
-
-    #[test]
-    fn overlapping_file_spans_rebuild_in_bounded_work() {
-        // Every cell an uncovered anchor spanning to the far corner: their areas add up to about
-        // n^4 / 4, but only what fits the grid is kept, so an edit's rebuild stays near n^2.
-        let n = 200;
-        let mut t = Table::new(n, n, 400.0, 30.0);
-        for row in &mut t.rows {
-            for c in &mut row.cells {
-                c.grid_span = u32::MAX;
-                c.row_span = u32::MAX;
-            }
-        }
-        assert_eq!(t.merges().len(), 1, "the first anchor already covers the whole grid");
-        t.insert_row(n);
-        assert_eq!(t.n_rows(), n + 1);
-        assert!(t.split(0, 0));
-    }
-
     fn merged() -> Table {
         // 4x4 with a 2x2 merge at (1,1) holding "M".
         let mut t = Table::new(4, 4, 400.0, 30.0);

crates/pptx/src/read.rs

@@ -663,42 +663,46 @@ fn read_slides(
     let pkg = imp.pkg;
     let fallback_layout = p.masters.first().and_then(|m| m.layouts.first()).map(|l| l.id).unwrap_or_default();
     let mut file_slide_ids: HashMap<u32, SlideId> = HashMap::new();
-    let mut notes_by_target: HashMap<String, TextBody> = HashMap::new();
+    // A notes or comments part belongs to one slide; one shared by many must not be read (and
+    // copied) once per slide that names it.
+    let mut owned_parts: HashSet<String> = HashSet::new();
     for (file_id, path, id) in slide_ids {
         let Some(doc) = docs.get(&opc::norm(path)) else { continue };
         let part = Part::new(pkg, path, doc);
         let layout = part.rels.first_of("slideLayout").and_then(|r| layout_by_part.get(&opc::norm(&r.target)).copied()).unwrap_or(fallback_layout);
         let mut slide = read_slide(imp, &part, doc, *id, layout);
-        if let Some(notes) = slide_notes(imp, &part, &mut notes_by_target) {
+        if let Some(notes) = slide_notes(imp, &part, &mut owned_parts) {
             slide.notes = notes;
         }
-        read_slide_comments(pkg, pres_part, &part, &mut slide);
+        read_slide_comments(pkg, pres_part, &part, &mut slide, &mut owned_parts);
         file_slide_ids.insert(*file_id, *id);
         p.slides.push(Arc::new(slide));
     }
     file_slide_ids
 }
 
-/// The slide's notes text, read once per notes part.
-fn slide_notes(imp: &mut Imp, part: &Part, cache: &mut HashMap<String, TextBody>) -> Option<TextBody> {
+/// The slide's notes text. A notes part is the notes of the first slide that names it.
+fn slide_notes(imp: &mut Imp, part: &Part, owned: &mut HashSet<String>) -> Option<TextBody> {
     let n = part.rels.first_of("notesSlide")?;
-    let key = opc::norm(&n.target);
-    if let Some(cached) = cache.get(&key) {
-        return Some(cached.clone());
+    if !owned.insert(opc::norm(&n.target)) {
+        log::warn!("pptx: notes part {} is named by more than one slide; kept on the first", n.target);
+        return None;
     }
     let pkg = imp.pkg;
     let nd = load(pkg, &n.target)?;
     let np = Part::new(pkg, &n.target, &nd);
-    let notes = read_notes(imp, &np, &nd.root);
-    cache.insert(key, notes.clone());
-    Some(notes)
+    Some(read_notes(imp, &np, &nd.root))
 }
 
 /// Comments. A modern part and the legacy part can describe the same threads (PowerPoint
 /// writes both): the modern one wins, its flattened legacy copy is skipped.
-fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide) {
+/// Each comments part is read once, for the first slide that names it.
+fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide, owned: &mut HashSet<String>) {
     let mut parts: Vec<(Doc, bool)> = vec![];
     for c in part.rels.all_of("comments") {
+        if !owned.insert(opc::norm(&c.target)) {
+            continue;
+        }
         if let Some(cd) = pkg.xml(&c.target) {
             // The content type names the modern part; a package that types it only through a
             // Default entry instead of an Override is still modern when its root is p188.

crates/pptx/tests/it/malformed.rs

@@ -287,3 +287,34 @@ fn unreadable_slide_is_reported() {
     assert_eq!(lost, vec!["ppt/slides/slide2.xml".to_string()]);
     assert_eq!(deckcraft_pptx::import(&broken).expect("import").slides.len(), 1);
 }
+
+/// Many slides naming one notes part: it is read for the first slide only, not copied onto every
+/// slide that names it.
+#[test]
+fn a_shared_notes_part_is_read_once() {
+    use std::sync::Arc;
+    let mut p = defaults::new_presentation(None);
+    let layout = p.slides[0].layout;
+    for _ in 0..3 {
+        let s = defaults::new_slide(&mut p, layout).expect("slide");
+        p.slides.push(Arc::new(s));
+    }
+    for (i, s) in p.slides.iter_mut().enumerate() {
+        Arc::make_mut(s).notes = deckcraft_model::text::TextBody::from_text(&format!("notes {i}"));
+    }
+    let mut es = entries(&deckcraft_pptx::export(&p).expect("export"));
+    for (n, d) in es.iter_mut() {
+        if n.starts_with("ppt/slides/_rels/") {
+            let x = String::from_utf8(d.clone()).expect("utf8");
+            let x = x
+                .replace("notesSlide2.xml", "notesSlide1.xml")
+                .replace("notesSlide3.xml", "notesSlide1.xml")
+                .replace("notesSlide4.xml", "notesSlide1.xml");
+            *d = x.into_bytes();
+        }
+    }
+    let q = deckcraft_pptx::import(&rezip(&es)).expect("import");
+    assert_eq!(q.slides.len(), 4);
+    assert_eq!(q.slides[0].notes.text(), "notes 0");
+    assert!(q.slides[1..].iter().all(|s| s.notes.text().is_empty()), "a shared notes part is not copied");
+}