Merge PR #209 from justinjohn0306: PPTX: SmartArt and OLE objects we can't draw keep their parts on save (#203)

# Conflicts:
#	crates/pptx/src/read/shapes.rs
#	crates/pptx/tests/roundtrip.rs

crates/model/src/lib.rs

@@ -298,6 +298,10 @@ pub enum ShapeKind {
         xml: String,
         preview: Option<MediaId>,
         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)>,
     },
 }
 

crates/pptx/src/opc.rs

@@ -133,7 +133,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
     }
@@ -162,6 +162,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,

crates/pptx/src/read/mod.rs

@@ -726,19 +726,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<String> {
+    fn rec(e: &El, out: &mut Vec<String>, 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)> {

crates/pptx/src/read/shapes.rs

@@ -449,7 +449,22 @@ 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::<Option<Vec<_>>>()
+        .unwrap_or_default();
+    s.kind = ShapeKind::Opaque { xml: part.keep(e), preview, label, parts };
     s
 }
 
@@ -490,6 +505,34 @@ fn vml_index(pkg: &crate::opc::Package, vml: &str) -> std::collections::HashMap<
     out
 }
 
+/// 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<deckcraft_model::MediaId> {
+    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!("<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"yes\"?>\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<Shape> {
     let rel_ids = gd.child("relIds")?;

crates/pptx/src/write/shapes.rs

@@ -437,7 +437,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),
     }
 }
 
@@ -823,13 +823,32 @@ 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<deckcraft_model::MediaId>, 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<deckcraft_model::MediaId>,
+    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::<Option<std::collections::HashMap<_, _>>>()
     {
         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());
@@ -879,6 +898,52 @@ 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<deckcraft_model::MediaId>) {
+    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#"<p:pic><p:nvPicPr><p:cNvPr id="0" name=""/><p:cNvPicPr/><p:nvPr/></p:nvPicPr><p:blipFill><a:blip r:embed="{rel}"/><a:stretch><a:fillRect/></a:stretch></p:blipFill><p:spPr><a:xfrm><a:off x="{ox}" y="{oy}"/><a:ext cx="{cx}" cy="{cy}"/></a:xfrm><a:prstGeom prst="rect"><a:avLst/></a:prstGeom></p:spPr></p:pic>"#
+    );
+    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<String, String>, 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 {

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: "<not xml".into(), preview: None, label: "Thing".into() },
+        kind: ShapeKind::Opaque { xml: "<not xml".into(), preview: None, label: "Thing".into(), parts: vec![] },
         ..Default::default()
     });
     p.slides.push(Arc::new(s));

crates/pptx/tests/roundtrip.rs

@@ -1650,3 +1650,57 @@ fn radar_stock_and_combo_round_trip() {
         }
     }
 }
+
+/// #203: SmartArt we can't draw kept its XML but lost its diagram parts on save.
+#[test]
+fn smartart_without_a_drawing_keeps_its_parts() {
+    let mut p = defaults::new_presentation(None);
+    let data = p.add_media(
+        "data1.xml",
+        "application/vnd.openxmlformats-officedocument.drawingml.diagramData+xml",
+        br#"<dgm:dataModel xmlns:dgm="http://schemas.openxmlformats.org/drawingml/2006/diagram" xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main"><dgm:ptLst><dgm:pt modelId="1"><dgm:t><a:p><a:r><a:t>Manager</a:t></a:r></a:p></dgm:t></dgm:pt></dgm:ptLst><dgm:extLst><a:ext uri="http://schemas.microsoft.com/office/drawing/2008/diagram"><dsp:dataModelExt xmlns:dsp="http://schemas.microsoft.com/office/drawing/2008/diagram" relId="rId6"/></a:ext></dgm:extLst></dgm:dataModel>"#.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#"<dgm:{kind} xmlns:dgm="http://schemas.openxmlformats.org/drawingml/2006/diagram"/>"#).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#"<p:graphicFrame xmlns:p="http://schemas.openxmlformats.org/presentationml/2006/main" xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"><p:nvGraphicFramePr><p:cNvPr id="4" name="Diagram 3"/><p:cNvGraphicFramePr/><p:nvPr/></p:nvGraphicFramePr><p:xfrm><a:off x="0" y="0"/><a:ext cx="914400" cy="914400"/></p:xfrm><a:graphic><a:graphicData uri="http://schemas.openxmlformats.org/drawingml/2006/diagram"><dgm:relIds xmlns:dgm="http://schemas.openxmlformats.org/drawingml/2006/diagram" r:dm="rId2" r:lo="rId3" r:qs="rId4" r:cs="rId5"/></a:graphicData></a:graphic></p:graphicFrame>"#;
+    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}");
+}
+
+/// #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#"<p:graphicFrame xmlns:p="http://schemas.openxmlformats.org/presentationml/2006/main" xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"><p:nvGraphicFramePr><p:cNvPr id="4" name="Object 3"/><p:cNvGraphicFramePr/><p:nvPr/></p:nvGraphicFramePr><p:xfrm><a:off x="0" y="0"/><a:ext cx="914400" cy="914400"/></p:xfrm><a:graphic><a:graphicData uri="http://schemas.openxmlformats.org/presentationml/2006/ole"><p:oleObj spid="_x0000_s1026" name="Slide" r:id="rId4" progId="PowerPoint.Slide.8"><p:embed/></p:oleObj></a:graphicData></a:graphic></p:graphicFrame>"#;
+    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("<p:pic>") && !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:?}");
+}