From c462859f6f8de2e68d0fdaba569fe21773e9d2ad Mon Sep 17 00:00:00 2001 From: justinjohn0306 Date: Mon, 12 Oct 2026 17:09:09 +0530 Subject: [PATCH 1/2] 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}"); +} From f58f4f425f428eb334382886cdf528be8237a8de Mon Sep 17 00:00:00 2001 From: justinjohn0306 Date: Mon, 12 Oct 2026 17:17:55 +0530 Subject: [PATCH 2/2] PPTX: legacy OLE objects are saved in PowerPoint's fallback form (#203) A legacy OLE object finds its preview in the slide's VML drawing through spid; DeckCraft doesn't write that drawing, and PowerPoint refused to open a file whose oleObj has a spid but no VML shape. The reader now takes the preview picture from the VML drawing (so DeckCraft shows it too), and the writer drops spid and adds that preview as a p:pic, the form PowerPoint itself writes as the fallback. Checked in PowerPoint: the issue's 45545_Comment.pptx saved by DeckCraft opens with its three embedded objects and draws slide 8's table. Co-Authored-By: Claude Opus 5.5 --- crates/pptx/src/read/shapes.rs | 15 ++++++++++++++- crates/pptx/src/write/shapes.rs | 31 +++++++++++++++++++++++++++++++ crates/pptx/tests/roundtrip.rs | 22 ++++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/crates/pptx/src/read/shapes.rs b/crates/pptx/src/read/shapes.rs index 3954cee8..45f819e3 100644 --- a/crates/pptx/src/read/shapes.rs +++ b/crates/pptx/src/read/shapes.rs @@ -346,7 +346,8 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S let mut pics = vec![]; e.find_all("blip", &mut pics); pics.into_iter().find_map(|b| dml::media_ref(imp, part, b)) - }; + } + .or_else(|| vml_preview(imp, part, gd?.find("oleObj")?.attr("spid")?)); let label = if let Some(o) = gd.and_then(|g| g.find("oleObj")) { o.attr("progId").or(o.attr("name")).unwrap_or("Object").to_string() } else if uri.ends_with("/diagram") { @@ -375,6 +376,18 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S s } +/// A legacy OLE object's preview: the picture of its shape (`spid`) in the slide's VML drawing. +fn vml_preview(imp: &mut Imp, part: &Part, spid: &str) -> Option { + let found = part.rels.all_of("vmlDrawing").find_map(|r| { + let doc = imp.pkg.xml(&r.target)?; + let mut shapes = vec![]; + doc.root.find_all("shape", &mut shapes); + let relid = shapes.into_iter().find(|s| s.attr("id") == Some(spid))?.find("imagedata")?.attr("o:relid")?.to_string(); + Some(imp.pkg.rels(&r.target).get(&relid).filter(|i| !i.external)?.target.clone()) + })?; + imp.media_for(&found) +} + /// 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. diff --git a/crates/pptx/src/write/shapes.rs b/crates/pptx/src/write/shapes.rs index 3df45c6c..09c3af2c 100644 --- a/crates/pptx/src/write/shapes.rs +++ b/crates/pptx/src/write/shapes.rs @@ -637,6 +637,7 @@ fn opaque( { let mut root = doc.root; set_rel_ids(&mut root, &ids, 0); + ole_fallback(&mut root, x, o, preview); let id = cx.id_for(s); set_cnvpr_id(&mut root, id); w.raw(&root.to_xml()); @@ -686,6 +687,36 @@ fn shape_with_id(w: &mut W, x: &mut Exp, o: &mut Out, cx: &mut Ctx, s: &Shape, i } } +/// A legacy OLE object finds its preview in the slide's VML drawing through `spid`, and PowerPoint +/// won't open the slide without it. We don't write that drawing, so write PowerPoint's own +/// fallback form instead: no `spid`, and the preview as a `p:pic`. +fn ole_fallback(root: &mut crate::xml::El, x: &mut Exp, o: &mut Out, preview: Option) { + let xfrm = root.child("xfrm").map(|t| { + let at = |e: &str, a: &str| t.child(e).and_then(|c| c.attr(a)).unwrap_or("0").to_string(); + (at("off", "x"), at("off", "y"), at("ext", "cx"), at("ext", "cy")) + }); + let Some(ole) = root + .elements_mut() + .find(|e| e.is("graphic")) + .and_then(|g| g.elements_mut().find(|e| e.is("graphicData"))) + .and_then(|d| d.elements_mut().find(|e| e.is("oleObj"))) + else { + return; + }; + if ole.child("pic").is_some() || ole.attr("spid").is_none() { + return; + } + ole.attrs.retain(|(k, _)| k != "spid"); + let rel = preview.and_then(|m| x.media_rel(o, m, "image")).unwrap_or_else(|| x.placeholder_rel(o)); + let (ox, oy, cx, cy) = xfrm.unwrap_or_default(); + let pic = format!( + r#""# + ); + if let Ok(d) = crate::xml::parse(pic.as_bytes()) { + ole.children.push(crate::xml::Node::El(d.root)); + } +} + fn set_rel_ids(e: &mut crate::xml::El, ids: &std::collections::HashMap, depth: usize) { if depth > crate::xml::MAX_DEPTH { return; diff --git a/crates/pptx/tests/roundtrip.rs b/crates/pptx/tests/roundtrip.rs index ddd62e17..7e4745fb 100644 --- a/crates/pptx/tests/roundtrip.rs +++ b/crates/pptx/tests/roundtrip.rs @@ -762,3 +762,25 @@ fn smartart_without_a_drawing_keeps_its_parts() { assert!(data.contains("Manager"), "{data}"); assert!(!data.contains("dataModelExt"), "the link to a drawing that isn't written is dropped: {data}"); } + +/// #203: a legacy OLE object (preview in the VML drawing we don't write) is saved in PowerPoint's +/// fallback form, without `spid` and with a `p:pic` preview, or PowerPoint won't open the file. +#[test] +fn legacy_ole_object_is_written_in_the_fallback_form() { + let mut p = defaults::new_presentation(None); + let bin = p.add_media("oleObject1.bin", "application/vnd.openxmlformats-officedocument.oleObject", b"\xD0\xCF\x11\xE0not-a-real-ole".to_vec()); + let prev = p.add_media("image1.png", "image/png", png(4, 3, [10, 20, 30])); + let xml = r#""#; + let parts = vec![("rId4".to_string(), "http://schemas.openxmlformats.org/officeDocument/2006/relationships/oleObject".to_string(), bin)]; + let mut s = new_slide(&mut p, "OLE"); + let mut frame = shape(&mut p, "Object 3", 0.0, 0.0, 72.0, 72.0); + frame.kind = ShapeKind::Opaque { xml: xml.into(), preview: Some(prev), label: "Slide".into(), parts }; + s.shapes.push(frame); + p.slides.push(Arc::new(s)); + + let q = round(&round(&p)); + let ShapeKind::Opaque { xml, parts, preview, .. } = &by_name(&q.slides[1], "Object 3").kind else { panic!("not kept as an object") }; + assert!(xml.contains("") && !xml.contains("spid"), "{xml}"); + assert!(preview.is_some(), "the preview picture is kept"); + assert!(parts.iter().any(|(_, k, _)| k.ends_with("/oleObject")), "the embedding is kept: {parts:?}"); +}