From c462859f6f8de2e68d0fdaba569fe21773e9d2ad Mon Sep 17 00:00:00 2001 From: justinjohn0306 Date: Mon, 12 Oct 2026 17:09:09 +0530 Subject: [PATCH] PPTX: SmartArt and OLE objects we can't draw keep their parts on save (#203) An object DeckCraft doesn't model kept its XML, but the parts that XML points to (diagram data, layout, style and colours; an OLE embedding) were not kept, so a save wrote it as an empty rectangle or its preview. Opaque now carries those parts (relationship id, full relationship type, the part as a media item) and the writer re-adds them under the slide's new relationship ids. A part with relationships of its own, or an external link, still falls back to the preview with the existing warning. A diagram's data part loses its link to the cached drawing, which is a slide relationship id a save renumbers; only diagrams whose drawing is missing or empty reach this path. Co-Authored-By: Claude Opus 5.5 --- crates/model/src/lib.rs | 4 +++ crates/pptx/src/opc.rs | 4 ++- crates/pptx/src/read/mod.rs | 34 +++++++++++++++++-------- crates/pptx/src/read/shapes.rs | 45 ++++++++++++++++++++++++++++++++- crates/pptx/src/write/shapes.rs | 42 +++++++++++++++++++++++++++--- crates/pptx/tests/malformed.rs | 2 +- crates/pptx/tests/roundtrip.rs | 32 +++++++++++++++++++++++ 7 files changed, 146 insertions(+), 17 deletions(-) diff --git a/crates/model/src/lib.rs b/crates/model/src/lib.rs index 6940fa4b..4186187b 100644 --- a/crates/model/src/lib.rs +++ b/crates/model/src/lib.rs @@ -286,6 +286,10 @@ pub enum ShapeKind { xml: String, preview: Option, label: String, + /// The parts its XML points to (SmartArt data, an OLE embedding…), so a save writes them + /// back: (relationship id used in `xml`, relationship type, the part as a media item). + #[serde(default, skip_serializing_if = "Vec::is_empty")] + parts: Vec<(String, String, MediaId)>, }, } diff --git a/crates/pptx/src/opc.rs b/crates/pptx/src/opc.rs index 4ef21c3d..94dd3879 100644 --- a/crates/pptx/src/opc.rs +++ b/crates/pptx/src/opc.rs @@ -105,7 +105,7 @@ impl Package { let (Some(id), Some(ty), Some(target)) = (r.attr("Id"), r.attr("Type"), r.attr("Target")) else { continue }; let external = r.attr("TargetMode").is_some_and(|m| m.eq_ignore_ascii_case("external")); let target = if external { target.to_string() } else { resolve(&base, target) }; - out.items.push(Rel { id: id.to_string(), kind: ty.rsplit('/').next().unwrap_or(ty).to_string(), target, external }); + out.items.push(Rel { id: id.to_string(), kind: ty.rsplit('/').next().unwrap_or(ty).to_string(), ty: ty.to_string(), target, external }); } out } @@ -134,6 +134,8 @@ pub struct Rel { pub id: String, /// Last segment of the relationship type (`slide`, `image`, `media`…). pub kind: String, + /// The whole relationship type URI, to write it back as it was. + pub ty: String, /// Absolute part name for internal targets; the URL for external ones. pub target: String, pub external: bool, diff --git a/crates/pptx/src/read/mod.rs b/crates/pptx/src/read/mod.rs index b8c403b5..f004e4c4 100644 --- a/crates/pptx/src/read/mod.rs +++ b/crates/pptx/src/read/mod.rs @@ -400,19 +400,33 @@ fn sane_len(v: f64, dflt: f64) -> f64 { /// Does the element (or a descendant) reference a relationship? pub fn has_rel_refs(el: &El) -> bool { - fn rec(e: &El, depth: usize) -> bool { + !rel_refs(el).is_empty() +} + +/// Is this attribute a relationship id (`r:id`, `r:embed`, `r:dm`…)? +pub fn is_rel_attr(k: &str, v: &str) -> bool { + !k.starts_with("xmlns") + && k.contains(':') + && matches!(k.rsplit(':').next(), Some("id" | "embed" | "link" | "dm" | "lo" | "qs" | "cs" | "pict" | "href")) + && !v.is_empty() +} + +/// Every relationship id `el` and its descendants use, sorted and without repeats. +pub fn rel_refs(el: &El) -> Vec { + fn rec(e: &El, out: &mut Vec, depth: usize) { if depth > crate::xml::MAX_DEPTH { - return false; + return; + } + out.extend(e.attrs.iter().filter(|(k, v)| is_rel_attr(k, v)).map(|(_, v)| v.clone())); + for c in e.elements() { + rec(c, out, depth + 1); } - let own = e.attrs.iter().any(|(k, v)| { - !k.starts_with("xmlns") - && k.contains(':') - && matches!(k.rsplit(':').next(), Some("id" | "embed" | "link" | "dm" | "lo" | "qs" | "cs" | "pict" | "href")) - && !v.is_empty() - }); - own || e.elements().any(|c| rec(c, depth + 1)) } - rec(el, 0) + let mut out = vec![]; + rec(el, &mut out, 0); + out.sort(); + out.dedup(); + out } pub fn color_map(cm: &El) -> Vec<(String, String)> { diff --git a/crates/pptx/src/read/shapes.rs b/crates/pptx/src/read/shapes.rs index 2e10e362..3954cee8 100644 --- a/crates/pptx/src/read/shapes.rs +++ b/crates/pptx/src/read/shapes.rs @@ -356,10 +356,53 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S } else { uri.rsplit('/').next().unwrap_or("Object").to_string() }; - s.kind = ShapeKind::Opaque { xml: part.keep(e), preview, label }; + // Keep the parts it points to so a save writes it back whole (#203). A part with relationships + // of its own, or an external link, can't be carried: the save then falls back to the preview. + let parts = super::rel_refs(e) + .into_iter() + .map(|id| { + let rel = part.rels.get(&id).filter(|r| !r.external)?; + if !imp.pkg.rels(&rel.target).items.is_empty() { + return None; + } + let (kind, target) = (rel.ty.clone(), rel.target.clone()); + let media = if rel.kind == "diagramData" { diagram_data(imp, &target) } else { imp.media_for(&target) }; + Some((id, kind, media?)) + }) + .collect::>>() + .unwrap_or_default(); + s.kind = ShapeKind::Opaque { xml: part.keep(e), preview, label, parts }; s } +/// A diagram's data part, without the link to its cached drawing: that link is a slide +/// relationship id, which a save renumbers. Only diagrams whose drawing is missing or empty get +/// here, and those are drawn from their data. +fn diagram_data(imp: &mut Imp, target: &str) -> Option { + fn strip(e: &mut El, depth: usize) { + if depth > crate::xml::MAX_DEPTH { + return; + } + e.children.retain(|n| !matches!(n, crate::xml::Node::El(c) if c.is("ext") && c.find("dataModelExt").is_some())); + for c in e.elements_mut() { + strip(c, depth + 1); + } + } + let mut doc = imp.pkg.xml(target)?; + strip(&mut doc.root, 0); + let xml = format!("\n{}", doc.root.to_xml()); + let id = deckcraft_model::MediaId(imp.alloc()); + let content_type = imp.pkg.content_type(target)?; + imp.media.push(deckcraft_model::MediaItem { + id, + name: crate::opc::file_of(target).to_string(), + content_type, + data: std::sync::Arc::new(xml.into_bytes()), + link: None, + }); + Some(id) +} + /// SmartArt: read the pre-drawn shapes of its drawing part as a group. fn smartart(imp: &mut Imp, part: &Part, gd: &El, frame: &Shape, depth: usize) -> Option { let rel_ids = gd.child("relIds")?; diff --git a/crates/pptx/src/write/shapes.rs b/crates/pptx/src/write/shapes.rs index 9b7309c0..3df45c6c 100644 --- a/crates/pptx/src/write/shapes.rs +++ b/crates/pptx/src/write/shapes.rs @@ -380,7 +380,7 @@ pub fn shape(w: &mut W, x: &mut Exp, o: &mut Out, cx: &mut Ctx, s: &Shape, depth } w.close("p:grpSp"); } - ShapeKind::Opaque { xml, preview, label } => opaque(w, x, o, cx, s, xml, *preview, label), + ShapeKind::Opaque { xml, preview, label, parts } => opaque(w, x, o, cx, s, xml, *preview, label, parts), } } @@ -612,13 +612,31 @@ fn media(w: &mut W, x: &mut Exp, o: &mut Out, id: u32, s: &Shape, m: &deckcraft_ } #[allow(clippy::too_many_arguments)] -fn opaque(w: &mut W, x: &mut Exp, o: &mut Out, cx: &mut Ctx, s: &Shape, xml: &str, preview: Option, label: &str) { - // Kept XML that references no relationships can be written back as it was. +fn opaque( + w: &mut W, + x: &mut Exp, + o: &mut Out, + cx: &mut Ctx, + s: &Shape, + xml: &str, + preview: Option, + label: &str, + parts: &[(String, String, deckcraft_model::MediaId)], +) { + // Kept XML is written back as it was when every part it references was kept too, with those + // parts re-added under this slide's relationship ids. if let Ok(doc) = crate::xml::parse(xml.as_bytes()) - && !crate::read::has_rel_refs(&doc.root) && matches!(doc.root.local(), "graphicFrame" | "sp" | "grpSp" | "cxnSp") + && let Some(ids) = crate::read::rel_refs(&doc.root) + .iter() + .map(|r| { + let (_, kind, m) = parts.iter().find(|(id, ..)| id == r)?; + Some((r.clone(), x.media_rel(o, *m, kind)?)) + }) + .collect::>>() { let mut root = doc.root; + set_rel_ids(&mut root, &ids, 0); let id = cx.id_for(s); set_cnvpr_id(&mut root, id); w.raw(&root.to_xml()); @@ -668,6 +686,22 @@ fn shape_with_id(w: &mut W, x: &mut Exp, o: &mut Out, cx: &mut Ctx, s: &Shape, i } } +fn set_rel_ids(e: &mut crate::xml::El, ids: &std::collections::HashMap, depth: usize) { + if depth > crate::xml::MAX_DEPTH { + return; + } + for (k, v) in &mut e.attrs { + if crate::read::is_rel_attr(k, v) + && let Some(new) = ids.get(v.as_str()) + { + v.clone_from(new); + } + } + for c in e.elements_mut() { + set_rel_ids(c, ids, depth + 1); + } +} + fn set_cnvpr_id(el: &mut crate::xml::El, id: u32) { fn rec(e: &mut crate::xml::El, id: u32, depth: usize) -> bool { if depth > 8 { diff --git a/crates/pptx/tests/malformed.rs b/crates/pptx/tests/malformed.rs index e76e3201..633d339e 100644 --- a/crates/pptx/tests/malformed.rs +++ b/crates/pptx/tests/malformed.rs @@ -251,7 +251,7 @@ fn export_of_odd_models_is_valid() { } s.shapes.push(Shape { id: ShapeId(7), - kind: ShapeKind::Opaque { xml: "Manager"#.to_vec(), + ); + let mut parts = vec![("rId2".to_string(), "http://schemas.openxmlformats.org/officeDocument/2006/relationships/diagramData".to_string(), data)]; + for (i, kind) in ["Layout", "QuickStyle", "Colors"].into_iter().enumerate() { + let xml = format!(r#""#).into_bytes(); + let m = p.add_media(&format!("{kind}1.xml"), &format!("application/vnd.openxmlformats-officedocument.drawingml.diagram{kind}+xml"), xml); + parts.push((format!("rId{}", i + 3), format!("http://schemas.openxmlformats.org/officeDocument/2006/relationships/diagram{kind}"), m)); + } + let xml = r#""#; + let mut s = new_slide(&mut p, "SmartArt"); + let mut frame = shape(&mut p, "Diagram 3", 0.0, 0.0, 72.0, 72.0); + frame.kind = ShapeKind::Opaque { xml: xml.into(), preview: None, label: "SmartArt".into(), parts }; + s.shapes.push(frame); + p.slides.push(Arc::new(s)); + + // Twice: the second save starts from what the first one wrote. + let q = round(&round(&p)); + let ShapeKind::Opaque { parts, .. } = &by_name(&q.slides[1], "Diagram 3").kind else { panic!("not kept as an object") }; + assert_eq!(parts.len(), 4, "data, layout, style and colours all kept"); + let data = q.media(parts.iter().find(|(_, k, _)| k.ends_with("/diagramData")).expect("data").2).expect("media"); + let data = String::from_utf8_lossy(&data.data); + assert!(data.contains("Manager"), "{data}"); + assert!(!data.contains("dataModelExt"), "the link to a drawing that isn't written is dropped: {data}"); +}