From 8788f994ef42002c899c1623472912de08ba3bb9 Mon Sep 17 00:00:00 2001 From: Nikita Date: Tue, 13 Oct 2026 10:40:59 +0300 Subject: [PATCH 01/16] Charts: keep the embedded workbook when a chart is saved unchanged (#335) PowerPoint stores chart data in ppt/embeddings. Saving used to drop that part and the chart relationship, so the deck still rendered but the workbook was gone. An edit still omits the workbook rather than writing numbers that no longer match it. Co-authored-by: Cursor --- ROADMAP.md | 4 +- crates/model/src/chart.rs | 30 +++++++ crates/pptx/src/opc.rs | 42 +++++++++- crates/pptx/src/read/chart.rs | 56 +++++++++++++ crates/pptx/src/read/shapes.rs | 3 +- crates/pptx/src/write/chart.rs | 52 ++++++++++-- crates/pptx/src/write/mod.rs | 8 +- crates/pptx/tests/chart_embed.rs | 137 +++++++++++++++++++++++++++++++ docs/file-format-parity.md | 5 +- docs/gaps.md | 6 +- 10 files changed, 328 insertions(+), 15 deletions(-) create mode 100644 crates/pptx/tests/chart_embed.rs diff --git a/ROADMAP.md b/ROADMAP.md index 2dba0e27..9e52b416 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~20 points and ~120–210 h away -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-10 · **Change:** minor (hours to ~95% per audience added; ready 56% → 55%; stage stays alpha; previous: major, full re-measure against PowerPoint for Mac 16.113.4; restructured to the craftrules progress-docs standard) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-13 · **Last updated:** 2026-10-13 · **Change:** trivial (unchanged charts keep their embedded workbook, #335) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) DeckCraft aims at full PowerPoint parity, and to be better: faster, open (a documented zip+JSON format plus PPTX), scriptable by agents (CLI, JSON control channel, MCP), and available everywhere @@ -128,6 +128,7 @@ app's JSON control channel (`deckcraft --control PORT`). | Date | What landed | |---|---| +| 2026-10-13 | Saving a presentation keeps the embedded workbook behind an unchanged chart, and the chart part's relationships (#335). Editing the chart drops that workbook instead of writing numbers that no longer match it | | 2026-10-12 | Objects off the slide show and can be selected and moved around it in Normal view, as in PowerPoint; the show, exports and thumbnails still clip to the slide (#270) | | 2026-10-12 | Korean, Japanese and Chinese input: IME composition shown in place and underlined, one undo step per composition, candidate window at the caret (slide text, notes, fields); interface text (title, notes, outline, sections, comments) borrows installed fonts for scripts the interface fonts lack (#101) | | 2026-10-12 | Selection Pane like PowerPoint (#133): front-most first, groups fold, drag and drop restacks among siblings, Bring Forward / Send Backward, Show All / Hide All, eye, rename in place, keyboard; Lock (pane padlock, canvas menu) keeps objects from moving, sizing or rotating and round-trips PowerPoint's locking attributes | @@ -147,6 +148,7 @@ app's JSON control channel (`deckcraft --control PORT`). | Date | Change | Summary | |---|---|---| +| 2026-10-13 | trivial | Progress log: chart workbook round-trip (#335) | | 2026-10-12 | trivial | Progress log: File tab (Backstage, #76) | | 2026-10-12 | trivial | Progress log: lines in scaled groups (#233) | | 2026-10-10 | minor | Readiness-by-audience table with hours to ~95% for full, mainstream and essentials | diff --git a/crates/model/src/chart.rs b/crates/model/src/chart.rs index 5ba72fc4..ff55db4f 100644 --- a/crates/model/src/chart.rs +++ b/crates/model/src/chart.rs @@ -162,6 +162,35 @@ pub struct Chart { /// Original chart XML part kept for round-trip of unmodelled options. #[serde(skip_serializing_if = "Option::is_none")] pub raw: Option, + /// Package parts the original chart still points at (embedded workbook, styles, drawings). + /// The writer clears this before comparing modelled state. A native save does not store the bytes. + #[serde(skip)] + pub kept: ChartKept, +} + +/// One relationship of a chart part, with the target string as it was written. +#[derive(Clone, Debug, PartialEq)] +pub struct ChartRel { + pub id: String, + pub rel_type: String, + pub target: String, + pub external: bool, +} + +/// A part reached from a chart (the workbook, a drawing, an image it uses). +#[derive(Clone, Debug, PartialEq)] +pub struct ChartPart { + /// Absolute part name (`ppt/embeddings/Workbook.xlsx`). + pub name: String, + pub content_type: String, + pub bytes: Vec, +} + +/// Sidecar for an unchanged chart: relationships and the parts they reach. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct ChartKept { + pub rels: Vec, + pub parts: Vec, } impl Default for Chart { @@ -187,6 +216,7 @@ impl Default for Chart { y_min: None, y_max: None, raw: None, + kept: ChartKept::default(), } } } diff --git a/crates/pptx/src/opc.rs b/crates/pptx/src/opc.rs index 4ef21c3d..f6d3d3cc 100644 --- a/crates/pptx/src/opc.rs +++ b/crates/pptx/src/opc.rs @@ -104,8 +104,16 @@ impl Package { for r in doc.root.children_named("Relationship") { 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 }); + let target_raw = target.to_string(); + let target = if external { target_raw.clone() } else { resolve(&base, target) }; + out.items.push(Rel { + id: id.to_string(), + kind: ty.rsplit('/').next().unwrap_or(ty).to_string(), + ty: ty.to_string(), + target_raw, + target, + external, + }); } out } @@ -134,6 +142,10 @@ pub struct Rel { pub id: String, /// Last segment of the relationship type (`slide`, `image`, `media`…). pub kind: String, + /// Full relationship type URI. + pub ty: String, + /// `Target` as written in the rels part (relative path or URL). + pub target_raw: String, /// Absolute part name for internal targets; the URL for external ones. pub target: String, pub external: bool, @@ -163,6 +175,28 @@ pub fn norm(name: &str) -> String { name.trim_start_matches('/').replace('\\', "/").to_ascii_lowercase() } +/// A part a chart may carry along beside its own `chartN.xml` (workbook, style, drawing, picture). +/// Chart documents themselves are rewritten, and nothing outside these folders is copied: a relationship +/// must not be able to replace `ppt/slides/slide1.xml` on save. +pub fn sidecar_part(name: &str) -> bool { + let n = norm(name); + let segs: Vec<&str> = n.split('/').filter(|s| !s.is_empty()).collect(); + if segs.iter().any(|s| *s == "." || *s == "..") { + return false; + } + let (Some(root), Some(area)) = (segs.first().copied(), segs.get(1).copied()) else { return false }; + if root != "ppt" || !matches!(area, "embeddings" | "charts" | "drawings" | "media") { + return false; + } + let file = segs.last().copied().unwrap_or(""); + let stem = file.strip_prefix("chart").unwrap_or(""); + let num = stem.strip_suffix(".xml.rels").or_else(|| stem.strip_suffix(".xml")).unwrap_or(""); + if stem != file && !num.is_empty() && num.chars().all(|c| c.is_ascii_digit()) { + return false; + } + true +} + pub fn dir_of(part: &str) -> String { let p = part.trim_start_matches('/'); match p.rfind('/') { @@ -311,6 +345,10 @@ impl PackageOut { } self.files.push((rels_path(part), rels.to_xml(), true)); } + pub fn part_data(&self, name: &str) -> Option<&[u8]> { + let n = norm(name); + self.files.iter().find(|(f, _, _)| norm(f) == n).map(|(_, d, _)| d.as_slice()) + } /// Add a binary part; its extension gets a Default content type. pub fn binary(&mut self, name: &str, content_type: &str, data: Vec) { let ext = name.rsplit('.').next().unwrap_or("bin").to_ascii_lowercase(); diff --git a/crates/pptx/src/read/chart.rs b/crates/pptx/src/read/chart.rs index a9a960a3..2e7ceaa2 100644 --- a/crates/pptx/src/read/chart.rs +++ b/crates/pptx/src/read/chart.rs @@ -225,3 +225,59 @@ pub(crate) fn chart_from(root: &El) -> Option { } Some(c) } + +const MAX_KEPT_PARTS: usize = 256; +const MAX_KEPT_DEPTH: usize = 8; + +/// Relationships and parts a chart points at, so a later save can write them back. +pub(crate) fn capture_kept(pkg: &crate::opc::Package, chart_part: &str) -> deckcraft_model::chart::ChartKept { + use deckcraft_model::chart::{ChartKept, ChartRel}; + + let mut kept = ChartKept::default(); + let mut seen = std::collections::HashSet::new(); + seen.insert(crate::opc::norm(chart_part)); + for r in pkg.rels(chart_part).items { + kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: r.external }); + if !r.external { + copy_part(pkg, &r.target, &mut kept.parts, &mut seen, 0); + } + } + kept +} + +fn copy_part( + pkg: &crate::opc::Package, + part: &str, + out: &mut Vec, + seen: &mut std::collections::HashSet, + depth: usize, +) { + use deckcraft_model::chart::ChartPart; + + if depth > MAX_KEPT_DEPTH || out.len() >= MAX_KEPT_PARTS { + log::warn!("pptx: chart part closure stopped at {part}"); + return; + } + let key = crate::opc::norm(part); + if !seen.insert(key) { + return; + } + let Some(bytes) = pkg.get(part) else { + log::warn!("pptx: chart relationship target {part} is missing"); + return; + }; + let name = pkg.original_name(part).unwrap_or(part).trim_start_matches('/').to_string(); + let content_type = pkg.content_type(part).unwrap_or_default(); + out.push(ChartPart { name, content_type, bytes: bytes.to_vec() }); + let rels_name = crate::opc::rels_path(part); + if seen.insert(crate::opc::norm(&rels_name)) + && let Some(rels_bytes) = pkg.get(&rels_name) + { + let name = pkg.original_name(&rels_name).unwrap_or(&rels_name).trim_start_matches('/').to_string(); + out.push(ChartPart { name, content_type: "application/vnd.openxmlformats-package.relationships+xml".into(), bytes: rels_bytes.to_vec() }); + let targets: Vec = pkg.rels(part).items.into_iter().filter(|r| !r.external).map(|r| r.target).collect(); + for target in targets { + copy_part(pkg, &target, out, seen, depth + 1); + } + } +} diff --git a/crates/pptx/src/read/shapes.rs b/crates/pptx/src/read/shapes.rs index 51b2b863..7e5d48ba 100644 --- a/crates/pptx/src/read/shapes.rs +++ b/crates/pptx/src/read/shapes.rs @@ -368,8 +368,9 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S if let Some(c) = gd.child("chart") && let Some(rel) = part.rels.get(c.attr("r:id").unwrap_or("")) && let Some(bytes) = imp.pkg.get(&rel.target) - && let Some(chart) = super::read_chart_xml(bytes) + && let Some(mut chart) = super::read_chart_xml(bytes) { + chart.kept = super::chart::capture_kept(imp.pkg, &rel.target); s.kind = ShapeKind::Chart(Box::new(chart)); return s; } diff --git a/crates/pptx/src/write/chart.rs b/crates/pptx/src/write/chart.rs index d832c34f..5a075b01 100644 --- a/crates/pptx/src/write/chart.rs +++ b/crates/pptx/src/write/chart.rs @@ -1,4 +1,5 @@ -//! Chart parts: a `c:chartSpace` with cached categories and values (no embedded workbook). +//! Chart parts: a `c:chartSpace` with cached categories and values. +//! An unchanged chart also keeps the embedded workbook and the other parts it points at. use deckcraft_model::chart::{Chart, ChartType, Series}; use deckcraft_model::style::Fill; @@ -362,8 +363,8 @@ fn plot_group(w: &mut W, c: &Chart, kind: ChartType, list: &[(usize, &Series)]) w.close(tag); } -/// The chart part XML. -pub fn chart_xml(c: &Chart) -> Vec { +/// The chart part XML, and whether it is the original part (so its relationships still apply). +pub fn chart_xml(c: &Chart) -> (Vec, bool) { // Reuse the kept part when the chart is unchanged since it was read. if let Some(raw) = &c.raw && let Ok(doc) = crate::xml::parse(raw.as_bytes()) @@ -374,12 +375,17 @@ pub fn chart_xml(c: &Chart) -> Vec { if let Some(back) = crate::read::chart_from_el(&root) { let mut me = c.clone(); me.raw = None; + // The sidecar is not modelled chart state. Comparing it would throw away an unchanged chart. + me.kept = Default::default(); if back == me { let mut keep = doc.root; - keep.remove_all(&["externalData", "userShapes"]); + // Without the parts those elements point at, PowerPoint repairs the file. + if c.kept.rels.is_empty() { + keep.remove_all(&["externalData", "userShapes"]); + } let mut w = W::new(); w.raw(&keep.to_xml()); - return w.finish(); + return (w.finish(), true); } } } @@ -477,5 +483,39 @@ pub fn chart_xml(c: &Chart) -> Vec { sp_pr(&mut w, Some(f), None); } w.close("c:chartSpace"); - w.finish() + (w.finish(), false) +} + +/// Write the chart's own relationships and the parts they reach. +pub fn write_kept(pkg: &mut crate::opc::PackageOut, chart_name: &str, c: &Chart) { + if c.kept.rels.is_empty() && c.kept.parts.is_empty() { + return; + } + let mut rels = crate::opc::RelsOut::default(); + for r in &c.kept.rels { + rels.items.push((r.id.clone(), r.rel_type.clone(), r.target.clone(), r.external)); + } + pkg.rels(chart_name, &rels); + for p in &c.kept.parts { + match pkg.part_data(&p.name) { + Some(existing) if existing != p.bytes.as_slice() => { + log::warn!("pptx: chart part {} already written with different bytes; keeping the first", p.name); + continue; + } + Some(_) => continue, + None => {} + } + if !crate::opc::sidecar_part(&p.name) { + log::warn!("pptx: not writing chart part {}", p.name); + continue; + } + let owned; + let ct = if !p.content_type.is_empty() { + p.content_type.as_str() + } else { + owned = crate::mime_for(&p.name).to_string(); + owned.as_str() + }; + pkg.binary(&p.name, ct, p.bytes.clone()); + } } diff --git a/crates/pptx/src/write/mod.rs b/crates/pptx/src/write/mod.rs index 7fb6a713..dc9ccf50 100644 --- a/crates/pptx/src/write/mod.rs +++ b/crates/pptx/src/write/mod.rs @@ -161,7 +161,13 @@ impl Exp<'_> { pub fn chart_rel(&mut self, o: &mut Out, c: &deckcraft_model::Chart) -> String { self.n_chart += 1; let name = format!("ppt/charts/chart{}.xml", self.n_chart); - self.pkg.xml_part(&name, CT_CHART, chart::chart_xml(c)); + let (xml, preserved) = chart::chart_xml(c); + self.pkg.xml_part(&name, CT_CHART, xml); + if preserved { + chart::write_kept(&mut self.pkg, &name, c); + } else if !c.kept.parts.is_empty() { + log::warn!("pptx: chart data changed; the embedded workbook was not written back"); + } o.rels.add_new("chart", &opc::relative(&o.name, &name), false) } } diff --git a/crates/pptx/tests/chart_embed.rs b/crates/pptx/tests/chart_embed.rs new file mode 100644 index 00000000..47b2f709 --- /dev/null +++ b/crates/pptx/tests/chart_embed.rs @@ -0,0 +1,137 @@ +//! An unchanged chart keeps the embedded workbook its `c:externalData` points at (#335). + +use std::io::{Cursor, Read, Write}; +use std::sync::Arc; + +use deckcraft_geom::Xfrm; +use deckcraft_model::chart::{Chart, ChartType}; +use deckcraft_model::{LayoutType, Shape, ShapeId, ShapeKind, defaults}; + +const WORKBOOK: &[u8] = b"chart-workbook-bytes-v1"; +const EMBED: &str = "ppt/embeddings/Microsoft_Excel_Worksheet1.xlsx"; + +fn chart_pptx() -> Vec { + let mut p = defaults::new_presentation(None); + p.slides.clear(); + let layout = defaults::layout_of_kind(&p, LayoutType::Blank).unwrap_or_default(); + let mut slide = defaults::new_slide(&mut p, layout); + let mut shape = Shape { id: ShapeId(p.alloc_id()), name: "Pie".into(), xfrm: Some(Xfrm::new(40.0, 40.0, 400.0, 300.0)), ..Default::default() }; + shape.kind = ShapeKind::Chart(Box::new(Chart::sample(ChartType::Pie))); + slide.shapes.push(shape); + p.slides.push(Arc::new(slide)); + deckcraft_pptx::export(&p).expect("export") +} + +/// Add an embedded workbook and point the chart part at it, the way PowerPoint writes a chart. +fn with_workbook(bytes: &[u8]) -> Vec { + let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip"); + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default(); + let mut chart_part = None; + for i in 0..zip.len() { + let mut f = zip.by_index(i).expect("entry"); + let name = f.name().to_string(); + let mut data = vec![]; + f.read_to_end(&mut data).expect("read"); + if name.starts_with("ppt/charts/chart") && name.ends_with(".xml") && !name.contains("_rels") { + let xml = String::from_utf8(data).expect("chart xml"); + assert!(xml.contains(""), "{name}"); + data = xml + .replacen("", r#""#, 1) + .into_bytes(); + chart_part = Some(name.clone()); + } else if name == "[Content_Types].xml" { + let xml = String::from_utf8(data).expect("content types"); + data = xml + .replacen( + "", + r#""#, + 1, + ) + .into_bytes(); + } + out.start_file(&name, opts).expect("start"); + out.write_all(&data).expect("write"); + } + let chart_part = chart_part.expect("chart part"); + let rels = format!("ppt/charts/_rels/{}.rels", chart_part.rsplit('/').next().unwrap_or("chart1.xml")); + let rels_xml = br#" +"#; + out.start_file(rels, opts).expect("rels"); + out.write_all(rels_xml).expect("rels bytes"); + out.start_file(EMBED, opts).expect("embed"); + out.write_all(WORKBOOK).expect("workbook"); + out.finish().expect("finish").into_inner() +} + +fn parts(bytes: &[u8]) -> Vec<(String, Vec)> { + let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip"); + let mut out = vec![]; + for i in 0..zip.len() { + let mut f = zip.by_index(i).expect("entry"); + let name = f.name().to_string(); + let mut data = vec![]; + f.read_to_end(&mut data).expect("read"); + out.push((name, data)); + } + out +} + +fn chart_of(bytes: &[u8]) -> deckcraft_model::Chart { + let p = deckcraft_pptx::import(bytes).expect("import"); + p.slides + .iter() + .flat_map(|s| s.shapes.iter()) + .find_map(|s| match &s.kind { + ShapeKind::Chart(c) if s.name == "Pie" => Some((**c).clone()), + _ => None, + }) + .expect("pie chart") +} + +#[test] +fn an_unchanged_chart_keeps_its_embedded_workbook() { + let src = with_workbook(&chart_pptx()); + let chart = chart_of(&src); + assert_eq!(chart.kept.parts.iter().filter(|p| p.name == EMBED).count(), 1); + assert_eq!(chart.kept.parts.iter().find(|p| p.name == EMBED).map(|p| p.bytes.as_slice()), Some(WORKBOOK)); + + let saved = deckcraft_pptx::export(&deckcraft_pptx::import(&src).expect("import")).expect("export"); + let saved_parts = parts(&saved); + let workbook = saved_parts.iter().find(|(n, _)| n == EMBED).map(|(_, d)| d.as_slice()); + assert_eq!(workbook, Some(WORKBOOK), "the embedding part is dropped on save"); + let rels = saved_parts + .iter() + .find(|(n, _)| n.starts_with("ppt/charts/_rels/chart") && n.ends_with(".xml.rels")) + .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) + .expect("chart relationships"); + assert!(rels.contains("../embeddings/Microsoft_Excel_Worksheet1.xlsx"), "{rels}"); + let xml = saved_parts + .iter() + .find(|(n, _)| n.starts_with("ppt/charts/chart") && n.ends_with(".xml")) + .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) + .expect("chart xml"); + assert!(xml.contains("externalData"), "{xml}"); +} + +#[test] +fn an_edited_chart_does_not_write_a_stale_workbook() { + let src = with_workbook(&chart_pptx()); + let mut p = deckcraft_pptx::import(&src).expect("import"); + let slide = Arc::make_mut(p.slides.get_mut(0).expect("slide")); + let chart = slide.shapes.iter_mut().find_map(|s| match &mut s.kind { + ShapeKind::Chart(c) if s.name == "Pie" => Some(c), + _ => None, + }); + let chart = chart.expect("pie"); + let value = chart.series.get_mut(0).and_then(|s| s.values.get_mut(0)).expect("value"); + *value = Some(99.0); + let saved = parts(&deckcraft_pptx::export(&p).expect("export")); + assert!(saved.iter().all(|(n, _)| n != EMBED), "a stale workbook was written after the chart changed"); + let xml = saved + .iter() + .find(|(n, _)| n.starts_with("ppt/charts/chart") && n.ends_with(".xml")) + .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) + .expect("chart xml"); + assert!(!xml.contains("externalData"), "{xml}"); +} diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 1e888c7a..1eb86aff 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -1,6 +1,6 @@ # File-format parity with PowerPoint -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** trivial (Groups with a small legacy child coordinate space draw outlines, dashes, arrowheads and ink at their own width (#233); previously major: created; formats taken from PowerPoint 16.113.4's Info.plist, PPTX coverage measured from `crates/pptx`) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-13 · **Last updated:** 2026-10-13 · **Change:** trivial (an unchanged chart keeps its embedded workbook on save, #335) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) Every format PowerPoint reads or writes, DeckCraft's support, fidelity and how it is tested. Gaps are itemized in [gaps.md](gaps.md#file-formats). Dimension score: **~58% ready** @@ -72,7 +72,7 @@ the round trip even though DeckCraft doesn't model it. | Text warp (`prstTxWarp`) | yes | yes | All 40 presets rendered (`crates/geom/src/warp.rs`); adjust values (`avLst`) round-trip | | Pictures with crop, recolour, duotone, alpha, luminance | yes | yes | | | Tables with styles | yes | yes | | -| Charts (`c:chartSpace`) | yes | yes | Original XML kept in `Chart.raw` | +| Charts (`c:chartSpace`) | yes | yes | Original XML kept in `Chart.raw`. An unchanged chart also keeps its embedded workbook and the other parts its relationships point at (#335); editing the chart drops that workbook instead of writing a stale one | | Charts (`cx:` chartex: waterfall, funnel, treemap, sunburst, histogram, box) | no | no | | | SmartArt (`dgm`) | drawing part only, as a group | as shapes | Diagram data model lost on save | | Equations (OMML, `a14:m`) | no | no | PR #55 open | @@ -99,6 +99,7 @@ the round trip even though DeckCraft doesn't model it. | Date | Change | Summary | |---|---|---| +| 2026-10-13 | trivial | Unchanged charts keep the embedded workbook (`ppt/embeddings/`) and chart relationships (#335) | | 2026-10-12 | minor | EMF/WMF text and pattern brushes drawn; OLE (VML) and ActiveX previews read | | 2026-10-12 | minor | Text warps rendered; their adjust values read and written | | 2026-10-12 | trivial | Groups with a small legacy child coordinate space draw outlines, dashes, arrowheads and ink at their own width (#233) | diff --git a/docs/gaps.md b/docs/gaps.md index d14700b0..f7552f6d 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -1,6 +1,6 @@ # Where DeckCraft falls short of PowerPoint -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** minor (IME composition and CJK interface text landed, #101; previous: minor, core-workflow alpha gate added; stage stays alpha; before that: major, first itemized gap list; full re-measure against PowerPoint for Mac 16.113.4 and the open GitHub issues) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-13 · **Last updated:** 2026-10-13 · **Change:** trivial (unchanged chart workbooks round-trip, #335) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) Every known shortfall, one entry each, ranked by how much it stops someone who presents with PowerPoint from switching. This is the work list: pick from the top. Numbers are summarized in @@ -146,7 +146,8 @@ Detail in [file-format-parity.md](file-format-parity.md). ### Charts: data grid, elements and the newer chart types - **Evidence:** checklist rows "Edit chart data" (P0, partial), "Chart elements" (partial), "Other chart types" (partial); `chartex` parts (waterfall, funnel, treemap, sunburst, histogram, box & - whisker) aren't read. + whisker) aren't read. An unchanged chart's embedded workbook now survives save (#335); editing + the chart still drops it rather than writing a stale spreadsheet. - **Impact:** most business decks have charts; editing their data is routine. - **Estimate:** 12–20 h. Data and elements are **beta blockers** (6–10 h). @@ -255,6 +256,7 @@ Detail in [localization-parity.md](localization-parity.md). | Date | Change | Summary | |---|---|---| +| 2026-10-13 | trivial | Chart workbook round-trip (#335) noted under the charts gap; editing chart data is still open | | 2026-10-12 | minor | IME composition in slide text and notes, and CJK interface text with real glyphs (#101): the IME gap is now Korean on the web and table-cell underlines | | 2026-10-12 | minor | EMF/WMF text, pattern and hatch brushes drawn; legacy OLE and ActiveX previews shown; the SVG gap keeps only EMF clip regions | | 2026-10-12 | minor | Text warps render (all 40 presets, adjusts round-trip); the warp gap is now the gallery, command and handles | From 71c5b1712ad3ac8bb40b876d59bd1effab59173a Mon Sep 17 00:00:00 2001 From: Nikita Date: Tue, 13 Oct 2026 10:53:08 +0300 Subject: [PATCH 02/16] Charts: ignore relationships that point outside a chart's own parts (#335) A chart relationship must not be able to replace a slide or another package part when the deck is saved. Co-authored-by: Cursor --- crates/pptx/src/opc.rs | 13 ++++++++ crates/pptx/src/read/chart.rs | 51 ++++++++++++++++++++++++-------- crates/pptx/src/write/chart.rs | 10 ++----- crates/pptx/tests/chart_embed.rs | 10 ++++++- 4 files changed, 63 insertions(+), 21 deletions(-) diff --git a/crates/pptx/src/opc.rs b/crates/pptx/src/opc.rs index f6d3d3cc..2a44722f 100644 --- a/crates/pptx/src/opc.rs +++ b/crates/pptx/src/opc.rs @@ -402,4 +402,17 @@ mod tests { assert_eq!(relative("ppt/presentation.xml", "ppt/slides/slide1.xml"), "slides/slide1.xml"); assert_eq!(relative("ppt/slides/slide1.xml", "ppt/slides/slide2.xml"), "slide2.xml"); } + + #[test] + fn sidecar_parts_stay_inside_chart_folders() { + assert!(sidecar_part("ppt/embeddings/Microsoft_Excel_Worksheet1.xlsx")); + assert!(sidecar_part("ppt/charts/style1.xml")); + assert!(sidecar_part("ppt/drawings/chartDrawing1.xml")); + assert!(sidecar_part("ppt/media/image1.png")); + assert!(!sidecar_part("ppt/charts/chart1.xml")); + assert!(!sidecar_part("ppt/charts/_rels/chart1.xml.rels")); + assert!(!sidecar_part("ppt/slides/slide1.xml")); + assert!(!sidecar_part("ppt/embeddings/../../ppt/slides/slide1.xml")); + assert!(!sidecar_part("/etc/passwd")); + } } diff --git a/crates/pptx/src/read/chart.rs b/crates/pptx/src/read/chart.rs index 2e7ceaa2..9cf694f0 100644 --- a/crates/pptx/src/read/chart.rs +++ b/crates/pptx/src/read/chart.rs @@ -234,45 +234,71 @@ pub(crate) fn capture_kept(pkg: &crate::opc::Package, chart_part: &str) -> deckc use deckcraft_model::chart::{ChartKept, ChartRel}; let mut kept = ChartKept::default(); - let mut seen = std::collections::HashSet::new(); - seen.insert(crate::opc::norm(chart_part)); + // The chart document itself is rewritten; don't store a second copy of it. + let mut seen = std::collections::HashMap::from([(crate::opc::norm(chart_part), true)]); for r in pkg.rels(chart_part).items { - kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: r.external }); - if !r.external { - copy_part(pkg, &r.target, &mut kept.parts, &mut seen, 0); + if !safe_rel(&r.id, &r.ty, &r.target_raw) { + log::warn!("pptx: skipping chart relationship {}", r.id); + continue; + } + if r.external { + kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: true }); + continue; + } + if copy_part(pkg, &r.target, &mut kept.parts, &mut seen, 0) { + kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: false }); } } kept } +fn safe_rel(id: &str, ty: &str, target: &str) -> bool { + [id, ty, target].iter().all(|s| !s.is_empty() && s.len() <= 4096 && !s.chars().any(|c| c.is_control() || matches!(c, '<' | '>' | '"' | '\''))) +} + +/// `true` when `part` is stored (now or from an earlier relationship). fn copy_part( pkg: &crate::opc::Package, part: &str, out: &mut Vec, - seen: &mut std::collections::HashSet, + seen: &mut std::collections::HashMap, depth: usize, -) { +) -> bool { use deckcraft_model::chart::ChartPart; if depth > MAX_KEPT_DEPTH || out.len() >= MAX_KEPT_PARTS { log::warn!("pptx: chart part closure stopped at {part}"); - return; + return false; } let key = crate::opc::norm(part); - if !seen.insert(key) { - return; + if let Some(ok) = seen.get(&key) { + return *ok; + } + if !crate::opc::sidecar_part(part) { + log::warn!("pptx: chart relationship target {part} is not kept"); + seen.insert(key, false); + return false; } let Some(bytes) = pkg.get(part) else { log::warn!("pptx: chart relationship target {part} is missing"); - return; + seen.insert(key, false); + return false; }; let name = pkg.original_name(part).unwrap_or(part).trim_start_matches('/').to_string(); + if !crate::opc::sidecar_part(&name) { + log::warn!("pptx: chart part name {name} is not kept"); + seen.insert(key, false); + return false; + } let content_type = pkg.content_type(part).unwrap_or_default(); out.push(ChartPart { name, content_type, bytes: bytes.to_vec() }); + seen.insert(key, true); let rels_name = crate::opc::rels_path(part); - if seen.insert(crate::opc::norm(&rels_name)) + if crate::opc::sidecar_part(&rels_name) + && seen.get(&crate::opc::norm(&rels_name)).is_none() && let Some(rels_bytes) = pkg.get(&rels_name) { + seen.insert(crate::opc::norm(&rels_name), true); let name = pkg.original_name(&rels_name).unwrap_or(&rels_name).trim_start_matches('/').to_string(); out.push(ChartPart { name, content_type: "application/vnd.openxmlformats-package.relationships+xml".into(), bytes: rels_bytes.to_vec() }); let targets: Vec = pkg.rels(part).items.into_iter().filter(|r| !r.external).map(|r| r.target).collect(); @@ -280,4 +306,5 @@ fn copy_part( copy_part(pkg, &target, out, seen, depth + 1); } } + true } diff --git a/crates/pptx/src/write/chart.rs b/crates/pptx/src/write/chart.rs index 5a075b01..752f712f 100644 --- a/crates/pptx/src/write/chart.rs +++ b/crates/pptx/src/write/chart.rs @@ -509,13 +509,7 @@ pub fn write_kept(pkg: &mut crate::opc::PackageOut, chart_name: &str, c: &Chart) log::warn!("pptx: not writing chart part {}", p.name); continue; } - let owned; - let ct = if !p.content_type.is_empty() { - p.content_type.as_str() - } else { - owned = crate::mime_for(&p.name).to_string(); - owned.as_str() - }; - pkg.binary(&p.name, ct, p.bytes.clone()); + let ct = if !p.content_type.is_empty() { p.content_type.clone() } else { crate::mime_for(&p.name).to_string() }; + pkg.binary(&p.name, &ct, p.bytes.clone()); } } diff --git a/crates/pptx/tests/chart_embed.rs b/crates/pptx/tests/chart_embed.rs index 47b2f709..ed5a8def 100644 --- a/crates/pptx/tests/chart_embed.rs +++ b/crates/pptx/tests/chart_embed.rs @@ -56,7 +56,7 @@ fn with_workbook(bytes: &[u8]) -> Vec { let chart_part = chart_part.expect("chart part"); let rels = format!("ppt/charts/_rels/{}.rels", chart_part.rsplit('/').next().unwrap_or("chart1.xml")); let rels_xml = br#" -"#; +"#; out.start_file(rels, opts).expect("rels"); out.write_all(rels_xml).expect("rels bytes"); out.start_file(EMBED, opts).expect("embed"); @@ -106,6 +106,14 @@ fn an_unchanged_chart_keeps_its_embedded_workbook() { .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) .expect("chart relationships"); assert!(rels.contains("../embeddings/Microsoft_Excel_Worksheet1.xlsx"), "{rels}"); + assert!(!rels.contains("slide1.xml"), "a chart relationship must not replace the slide: {rels}"); + let slide = saved_parts + .iter() + .find(|(n, _)| n == "ppt/slides/slide1.xml") + .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) + .expect("slide"); + assert!(slide.contains("sld"), "{slide}"); + assert!(!slide.as_bytes().windows(WORKBOOK.len()).any(|w| w == WORKBOOK)); let xml = saved_parts .iter() .find(|(n, _)| n.starts_with("ppt/charts/chart") && n.ends_with(".xml")) From 5207844798e29375e343a0a12e8af2d0761f6370 Mon Sep 17 00:00:00 2001 From: Nikita Date: Tue, 13 Oct 2026 10:55:10 +0300 Subject: [PATCH 03/16] Credit Nikita Rakitin in the About window Co-authored-by: Cursor --- contributors/people.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index c10fb209..bac8a68c 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -41,3 +41,6 @@ sync_github_name = true [people.Silvertree2010] display_name = "Netter" sync_github_name = false + +[people.NikitkaRa] +real_name = "Nikita Rakitin" From c7a4930c4c5a648c6630f1338f7cc8fbc7d39062 Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Tue, 13 Oct 2026 21:08:47 +0300 Subject: [PATCH 04/16] UI: open presentations in the background with a progress window (#350) Opening from the file picker or the Backstage recent list read and parsed the file on the UI thread, so a large PPTX froze the window. The read now runs on a thread (engine: read_presentation_file / add_presentation_file), and a modal shows the file name, elapsed time and Cancel; Escape cancels too. A cancelled read is dropped when it finishes. Scripted opens (control, MCP, CLI) and the web build stay synchronous. Co-authored-by: Cursor --- crates/engine/src/cmd/file.rs | 29 +++- crates/ui-egui/src/backstage.rs | 6 +- crates/ui-egui/src/backstage_tests.rs | 9 ++ crates/ui-egui/src/lib.rs | 15 +- crates/ui-egui/src/opening.rs | 195 ++++++++++++++++++++++++++ 5 files changed, 245 insertions(+), 9 deletions(-) create mode 100644 crates/ui-egui/src/opening.rs diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index f6424f7c..970c5ef3 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -94,13 +94,36 @@ pub fn add_opened(s: &mut Session, name: &str, path: Option, p: Presenta s.add_document(st) } +/// A presentation read from disk, not yet added to a session: [`read_presentation_file`] needs no +/// session, so a host can run it on a background thread and add the result with +/// [`add_presentation_file`]. +#[derive(Debug)] +pub struct LoadedFile { + pub path: String, + pub doc: Presentation, + /// A native or PPTX file (saving goes back to it); an outline import has no file to save to. + pub native: bool, +} + +/// Read and parse the presentation at `path`. #[cfg(not(target_arch = "wasm32"))] -fn open(s: &mut Session, p: &Value) -> Result { - let path = str_param(p, "path").ok_or_else(|| bad("file.open", "missing `path`"))?; +pub fn read_presentation_file(path: &str) -> Result { let bytes = std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; let doc = open_presentation(path, &bytes)?; let native = deckcraft_format::sniff(&bytes) || deckcraft_pptx::sniff(&bytes); - let i = add_opened(s, path, native.then(|| path.to_string()), doc); + Ok(LoadedFile { path: path.to_string(), doc, native }) +} + +/// Add a presentation read by [`read_presentation_file`] as the active document. +pub fn add_presentation_file(s: &mut Session, f: LoadedFile) -> usize { + let LoadedFile { path, doc, native } = f; + add_opened(s, &path, native.then(|| path.clone()), doc) +} + +#[cfg(not(target_arch = "wasm32"))] +fn open(s: &mut Session, p: &Value) -> Result { + let path = str_param(p, "path").ok_or_else(|| bad("file.open", "missing `path`"))?; + let i = add_presentation_file(s, read_presentation_file(path)?); let st = s.doc()?; Ok(json!({"document": i, "slides": st.doc.slides.len(), "missingFonts": st.missing_fonts})) } diff --git a/crates/ui-egui/src/backstage.rs b/crates/ui-egui/src/backstage.rs index f1d0fde3..ca4e06bb 100644 --- a/crates/ui-egui/src/backstage.rs +++ b/crates/ui-egui/src/backstage.rs @@ -568,10 +568,8 @@ fn recent_list(app: &mut SlideApp, ui: &mut Ui, query: &str, limit: usize) { icons::paint(p, Rect::from_center_size(pos2(rect.left() + 20.0, rect.center().y), vec2(22.0, 22.0)), Icon::Slide, t.text, false); p.text(pos2(rect.left() + 44.0, rect.center().y - 8.0), Align2::LEFT_CENTER, &name, theme::font(13.0), t.text); p.text(pos2(rect.left() + 44.0, rect.center().y + 9.0), Align2::LEFT_CENTER, &folder, theme::font(11.0), t.text_dim); - if resp.on_hover_text(&path).clicked() - && let Err(e) = app.open_path(&path) - { - app.set_status(e); + if resp.on_hover_text(&path).clicked() { + app.open_path_in_background(&path); } } } diff --git a/crates/ui-egui/src/backstage_tests.rs b/crates/ui-egui/src/backstage_tests.rs index 115d782f..a0f07c87 100644 --- a/crates/ui-egui/src/backstage_tests.rs +++ b/crates/ui-egui/src/backstage_tests.rs @@ -217,6 +217,15 @@ fn open_lists_recent_files_and_opens_one() { h.run_steps(2); assert!(h.query_by_label("Other.pptx").is_none(), "the search box filters by name"); h.get_by_label("Quarterly review.pptx").click(); + h.run_steps(1); + // The file is read in the background (#350); its progress window shows meanwhile. + for _ in 0..300 { + if h.state().opening.is_none() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + h.run_steps(1); + } h.run_steps(3); assert_eq!(h.state().session.documents().len(), 2); assert_eq!(h.state().session.active().unwrap().path.as_deref(), Some(path.as_str())); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ec97e6e6..9334aced 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -15,6 +15,7 @@ pub mod fillui; pub mod icons; pub mod media; pub mod menus; +pub mod opening; pub mod panes; pub mod ribbon; pub mod selpane; @@ -188,6 +189,8 @@ pub struct SlideApp { /// Audio/video playback (editor control bar, slide show, `media.*` commands). pub media: media::MediaHost, pub status: Option<(String, f64)>, + /// A presentation being read in the background, with its progress window ([`opening`]). + pub opening: Option, /// A brief notice over the bottom of the window (missing fonts on opening a file). pub toast: Option, /// Documents (by uid) whose missing fonts have been mentioned. @@ -242,6 +245,7 @@ impl SlideApp { show: None, media: media::MediaHost::new(audio_out), status: None, + opening: None, toast: None, fonts_noted: Default::default(), perf: Perf { frame_ms: 0.0, render_ms: 0.0, fps: 0.0 }, @@ -564,7 +568,7 @@ impl SlideApp { fn pick_and_open(&mut self) { if let Some(pick) = self.services.pick_open.as_mut() { if let Some(path) = pick("open") { - let _ = self.open_path(&path); + self.open_path_in_background(&path); } } else if let Some(f) = self.services.open_async.as_mut() { f("open"); @@ -573,11 +577,16 @@ impl SlideApp { pub fn open_path(&mut self, path: &str) -> Result<(), String> { self.session.execute("file.open", &json!({"path": path})).map_err(|e| e.to_string())?; + self.note_recent(path); + Ok(()) + } + + /// A presentation was opened from `path`: first in the recent list, shown in Normal view. + pub(crate) fn note_recent(&mut self, path: &str) { self.ui.recent.retain(|p| p != path); self.ui.recent.insert(0, path.to_string()); self.ui.recent.truncate(12); self.ui.view = ViewMode::Normal; - Ok(()) } fn pick_and_insert(&mut self, purpose: &str, cmd: &str) { @@ -741,6 +750,7 @@ impl SlideApp { } self.last_time = now; self.drain_control(ctx); + self.poll_opening(); if self.fonts_ready { self.drain_inbox(); } @@ -935,6 +945,7 @@ impl SlideApp { let t0 = now_ms(); let t = theme::Tokens::get(&ctx); self.tick_media(&ctx); + self.opening_ui(&ctx); if self.show.is_some() { show::ui(self, ui); self.perf.frame_ms = now_ms() - t0; diff --git a/crates/ui-egui/src/opening.rs b/crates/ui-egui/src/opening.rs new file mode 100644 index 00000000..4c4dc45b --- /dev/null +++ b/crates/ui-egui/src/opening.rs @@ -0,0 +1,195 @@ +//! Opening a presentation in the background (#350): reading and parsing a large PPTX no longer +//! freezes the window. A modal shows the file being opened and the time so far, and Cancel (or +//! Escape) gives the window back at once; the abandoned read finishes on its thread and is dropped. + +use crate::SlideApp; +use deckcraft_engine::cmd::file::LoadedFile; +use std::sync::mpsc::{Receiver, TryRecvError}; + +/// A presentation being read on a background thread. +pub struct Opening { + pub path: String, + pub started_ms: f64, + rx: Receiver>, +} + +impl Opening { + /// The file name shown in the window. + pub fn name(&self) -> String { + std::path::Path::new(&self.path).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| self.path.clone()) + } +} + +impl SlideApp { + /// Open `path` without blocking the UI: the file is read on a thread and the progress window + /// shows until it arrives. Without threads (the web build) it opens at once. + pub fn open_path_in_background(&mut self, path: &str) { + if let Some(o) = &self.opening { + let name = o.name(); + self.set_status(format!("Still opening {name}")); + return; + } + #[cfg(target_arch = "wasm32")] + { + if let Err(e) = self.open_path(path) { + self.set_status(e); + } + } + #[cfg(not(target_arch = "wasm32"))] + { + let (tx, rx) = std::sync::mpsc::channel(); + let p = path.to_string(); + let spawned = std::thread::Builder::new().name("deckcraft-open".into()).spawn(move || { + let r = deckcraft_engine::cmd::file::read_presentation_file(&p).map_err(|e| e.to_string()); + let _ = tx.send(r); + }); + match spawned { + Ok(_) => self.opening = Some(Opening { path: path.to_string(), started_ms: crate::now_ms(), rx }), + Err(_) => { + if let Err(e) = self.open_path(path) { + self.set_status(e); + } + } + } + } + } + + /// Stop waiting for the file being opened; the presentation stays as it is. + pub fn cancel_opening(&mut self) { + if let Some(o) = self.opening.take() { + self.set_status(format!("Stopped opening {}", o.name())); + } + } + + /// Take the background read's result when it has arrived. + pub(crate) fn poll_opening(&mut self) { + let Some(o) = &self.opening else { return }; + let r = match o.rx.try_recv() { + Ok(r) => r, + Err(TryRecvError::Empty) => return, + Err(TryRecvError::Disconnected) => Err(format!("{}: reading the file stopped unexpectedly", o.path)), + }; + let Some(o) = self.opening.take() else { return }; + match r { + Ok(f) => { + deckcraft_engine::cmd::file::add_presentation_file(&mut self.session, f); + self.note_recent(&o.path); + } + Err(e) => self.set_status(e), + } + } + + /// The progress window over everything while a file opens. + pub(crate) fn opening_ui(&mut self, ctx: &egui::Context) { + let Some(o) = &self.opening else { return }; + let name = o.name(); + let secs = ((crate::now_ms() - o.started_ms) / 1000.0).max(0.0); + let mut cancel = false; + egui::Modal::new(egui::Id::new("deckcraft-opening")).show(ctx, |ui| { + ui.set_width(320.0); + ui.horizontal(|ui| { + ui.add(egui::Spinner::new().size(18.0)); + ui.label(format!("Opening {name}…")); + }); + ui.add_space(6.0); + ui.weak(format!("Reading the file: {secs:.0} s")); + ui.add_space(6.0); + ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { + cancel = ui.button("Cancel").clicked(); + }); + }); + if cancel || ctx.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Escape)) { + self.cancel_opening(); + } + ctx.request_repaint_after(std::time::Duration::from_millis(100)); + } +} + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used, clippy::expect_used)] + + use super::*; + use deckcraft_engine::Session; + + fn saved_deck(name: &str, slides: usize) -> String { + let mut s = Session::with_new(); + for _ in 1..slides { + s.execute("slide.new", &serde_json::json!({})).unwrap(); + } + let path = std::env::temp_dir().join(format!("deckcraft-opening-{}-{name}.deckcraft", std::process::id())); + let path = path.to_string_lossy().to_string(); + s.execute("file.saveAs", &serde_json::json!({"path": path})).unwrap(); + path + } + + fn wait(app: &mut SlideApp) { + for _ in 0..500 { + app.poll_opening(); + if app.opening.is_none() { + return; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + panic!("opening did not finish"); + } + + #[test] + fn opens_in_the_background_and_adds_the_document() { + let path = saved_deck("ok", 3); + let mut app = SlideApp::new(Session::new(), crate::Services::default()); + app.open_path_in_background(&path); + assert!(app.opening.is_some(), "the read runs on a thread"); + wait(&mut app); + assert_eq!(app.session.documents().len(), 1); + assert_eq!(app.session.active().unwrap().doc.slides.len(), 3); + assert_eq!(app.ui.recent.first(), Some(&path)); + std::fs::remove_file(&path).ok(); + } + + #[test] + fn cancel_drops_the_result() { + let path = saved_deck("cancel", 2); + let mut app = SlideApp::new(Session::with_new(), crate::Services::default()); + app.open_path_in_background(&path); + app.cancel_opening(); + assert!(app.opening.is_none()); + std::thread::sleep(std::time::Duration::from_millis(200)); + app.poll_opening(); + assert_eq!(app.session.documents().len(), 1, "nothing is added after Cancel"); + std::fs::remove_file(&path).ok(); + } + + #[test] + fn errors_show_in_the_status_bar_and_a_second_open_waits() { + let mut app = SlideApp::new(Session::new(), crate::Services::default()); + app.open_path_in_background("/nonexistent/deckcraft/missing.pptx"); + app.open_path_in_background("/nonexistent/deckcraft/other.pptx"); + assert!(app.status.as_ref().is_some_and(|(m, _)| m.starts_with("Still opening missing.pptx"))); + wait(&mut app); + assert!(app.session.documents().is_empty()); + assert!(app.status.as_ref().is_some_and(|(m, _)| m.contains("missing.pptx")), "{:?}", app.status); + } + + #[test] + fn the_progress_window_shows_and_cancel_closes_it() { + use egui_kittest::kittest::Queryable; + let mut app = SlideApp::new(Session::with_new(), crate::Services::default()); + // A read that never answers: the window stays until Cancel. + let (_tx, rx) = std::sync::mpsc::channel(); + app.opening = Some(Opening { path: "/decks/Big deck.pptx".into(), started_ms: crate::now_ms(), rx }); + let mut harness = egui_kittest::Harness::builder().with_size(egui::vec2(1000.0, 700.0)).build_ui_state( + |ui, app: &mut SlideApp| { + app.logic(ui.ctx()); + app.ui(ui); + }, + app, + ); + harness.run_steps(3); + harness.get_by_label("Opening Big deck.pptx…"); + harness.get_by_label("Cancel").click(); + harness.run_steps(2); + assert!(harness.state().opening.is_none()); + assert_eq!(harness.state().session.documents().len(), 1); + } +} From 36ec1dc0cac15fdbab8c90b60bff52b27f6f19a2 Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Tue, 13 Oct 2026 22:13:36 +0300 Subject: [PATCH 05/16] Format Shape: 3-D Format and 3-D Rotation controls shape.effects gains `threeD`: top and bottom bevel (preset, width, height), depth, contour, material, lighting, camera preset, X/Y/Z rotation, distance from ground, and reset. The settings are read from and written back to the shape's kept a:scene3d / a:sp3d XML (new cmd::three_d), so they render through the existing camera and bevel code and survive a PPTX save; extrusion and contour colours are kept as is. The `bevel` key used to set only Effects::bevel, which neither the renderer nor the PPTX writer reads, so a bevel set from the app vanished on save; it now goes through the same path. The Effects page of the Format Shape pane gets 3-D Format and 3-D Rotation sections. Co-authored-by: Cursor --- crates/engine/src/cmd/mod.rs | 1 + crates/engine/src/cmd/shape.rs | 17 +- crates/engine/src/cmd/three_d.rs | 304 +++++++++++++++++++++++++++++++ crates/engine/src/tests.rs | 50 +++++ crates/ui-egui/src/panes.rs | 75 ++++++++ docs/ui-parity.md | 2 +- 6 files changed, 445 insertions(+), 4 deletions(-) create mode 100644 crates/engine/src/cmd/three_d.rs diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index ce909b02..0e2f78a1 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -16,6 +16,7 @@ pub mod shape; pub mod slide; pub mod table; pub mod text; +pub mod three_d; pub mod transition; use deckcraft_geom::Xfrm; diff --git a/crates/engine/src/cmd/shape.rs b/crates/engine/src/cmd/shape.rs index b4c6aa4e..9fcc6c4b 100644 --- a/crates/engine/src/cmd/shape.rs +++ b/crates/engine/src/cmd/shape.rs @@ -60,7 +60,7 @@ pub fn specs() -> Vec { "Shape Effects", ["Shape Format", "Shape Styles"], None, - "{shadow?: none|outer|inner|perspective|{color, blur, dist, dir}, glow?: none|{color, radius}, softEdges?: pt, reflection?: none|tight|half|full, bevel?: none|circle|…, reset?, ids?}", + "{shadow?: none|outer|inner|perspective|{color, blur, dist, dir}, glow?: none|{color, radius}, softEdges?: pt, reflection?: none|tight|half|full, bevel?: none|circle|…, threeD?: {camera?: preset, rotX?, rotY?, rotZ?: deg, z?: pt, bevelTop?, bevelBottom?: none|circle|…, bevelTopW?, bevelTopH?, bevelBottomW?, bevelBottomH?, depth?, contour?: pt, material?, lighting?, reset?: true}, reset?, ids?}", has_selection, effects ), @@ -449,6 +449,7 @@ fn effects(s: &mut Session, p: &Value) -> Result { "full" => Some(Reflection { blur: 0.5, start_alpha: 0.5, end_alpha: 0.0, end_pos: 0.9, dist: 4.0 }), _ => Some(Reflection { blur: 0.5, start_alpha: 0.5, end_alpha: 0.0, end_pos: 0.35, dist: 0.0 }), }); + let three_d = p.get("threeD").filter(|v| v.is_object()); let bevel = str_param(p, "bevel").map(|b| if b == "none" { None } else { Some(b.to_string()) }); edit_shapes(s, p, "shape.effects", |sh| { if reset { @@ -481,8 +482,18 @@ fn effects(s: &mut Session, p: &Value) -> Result { if let Some(r) = &refl { e.reflection.clone_from(r); } - if let Some(b) = &bevel { - e.bevel.clone_from(b); + if bevel.is_some() || three_d.is_some() { + let mut s3 = e.raw3d.as_deref().map(super::three_d::Shape3d::parse).unwrap_or_default(); + if let Some(b) = &bevel { + s3.apply(&json!({"bevelTop": b.as_deref().unwrap_or("none")})); + } + if let Some(t) = three_d { + if t.get("reset").and_then(Value::as_bool) == Some(true) { + s3 = super::three_d::Shape3d::default(); + } + s3.apply(t); + } + super::three_d::store(e, &s3); } Ok(()) }) diff --git a/crates/engine/src/cmd/three_d.rs b/crates/engine/src/cmd/three_d.rs new file mode 100644 index 00000000..fe53450b --- /dev/null +++ b/crates/engine/src/cmd/three_d.rs @@ -0,0 +1,304 @@ +//! Format Shape ▸ 3-D Format and 3-D Rotation: a shape's `a:scene3d` / `a:sp3d`, kept as XML on +//! the model (`Effects::raw3d`), read into fields, changed, and written back. + +use serde_json::Value; + +/// Camera presets PowerPoint offers (3-D Rotation ▸ Presets). +pub const CAMERAS: &[&str] = &[ + "orthographicFront", + "isometricTopUp", + "isometricTopDown", + "isometricBottomUp", + "isometricBottomDown", + "isometricLeftUp", + "isometricLeftDown", + "isometricRightUp", + "isometricRightDown", + "obliqueTopLeft", + "obliqueTopRight", + "obliqueBottomLeft", + "obliqueBottomRight", + "perspectiveFront", + "perspectiveLeft", + "perspectiveRight", + "perspectiveAbove", + "perspectiveBelow", + "perspectiveAboveLeftFacing", + "perspectiveAboveRightFacing", + "perspectiveContrastingLeftFacing", + "perspectiveContrastingRightFacing", + "perspectiveHeroicLeftFacing", + "perspectiveHeroicRightFacing", + "perspectiveHeroicExtremeLeftFacing", + "perspectiveHeroicExtremeRightFacing", + "perspectiveRelaxed", + "perspectiveRelaxedModerately", +]; + +/// Bevel presets (`ST_BevelPresetType`). +pub const BEVELS: &[&str] = + &["circle", "relaxedInset", "cross", "coolSlant", "angle", "softRound", "convex", "slope", "divot", "riblet", "hardEdge", "artDeco"]; + +/// Materials (`ST_PresetMaterialType`). +pub const MATERIALS: &[&str] = + &["warmMatte", "matte", "plastic", "metal", "dkEdge", "softEdge", "flat", "legacyWireframe", "powder", "translucentPowder", "clear", "softmetal"]; + +/// Light rigs (`ST_LightRigType`) PowerPoint offers. +pub const LIGHTS: &[&str] = &[ + "threePt", + "balanced", + "soft", + "harsh", + "flood", + "contrasting", + "morning", + "sunrise", + "sunset", + "chilly", + "freezing", + "flat", + "twoPt", + "glow", + "brightRoom", +]; + +/// One bevel: preset, width and height in points. +#[derive(Clone, Debug, PartialEq)] +pub struct Bevel { + pub preset: String, + pub w: f64, + pub h: f64, +} + +/// The 3-D settings of a shape. +#[derive(Clone, Debug, PartialEq)] +pub struct Shape3d { + pub camera: String, + /// Camera rotation (X, Y, Z in degrees, as PowerPoint shows them), when set. + pub rot: Option<(f64, f64, f64)>, + pub light: String, + pub light_dir: String, + pub top: Option, + pub bottom: Option, + /// Depth (extrusion), contour width and distance from ground, in points. + pub depth: f64, + pub contour: f64, + pub z: f64, + pub material: String, + /// `a:extrusionClr` / `a:contourClr` XML, kept as is. + pub colors: String, +} + +impl Default for Shape3d { + fn default() -> Self { + Shape3d { + camera: "orthographicFront".into(), + rot: None, + light: "threePt".into(), + light_dir: "t".into(), + top: None, + bottom: None, + depth: 0.0, + contour: 0.0, + z: 0.0, + material: "warmMatte".into(), + colors: String::new(), + } + } +} + +/// The start tags of `raw` as (local name, tag text), in order. +fn tags(raw: &str) -> Vec<(&str, &str)> { + let mut out = Vec::new(); + let mut rest = raw; + while let Some(i) = rest.find('<') { + let Some(t) = rest.get(i + 1..) else { break }; + let end = t.find('>').unwrap_or(t.len()); + let tag = t.get(..end).unwrap_or(""); + rest = t.get(end..).unwrap_or(""); + if tag.starts_with('/') || tag.starts_with('?') || tag.starts_with('!') { + continue; + } + let name = tag.split(|c: char| c.is_whitespace() || c == '/').next().unwrap_or(""); + let local = name.rsplit(':').next().unwrap_or(name); + out.push((local, tag)); + } + out +} + +fn attr<'a>(tag: &'a str, name: &str) -> Option<&'a str> { + let key = format!(" {name}=\""); + let s = tag.find(&key)? + key.len(); + let rest = tag.get(s..)?; + rest.get(..rest.find('"')?) +} + +fn num(tag: &str, name: &str) -> Option { + attr(tag, name).and_then(|v| v.parse::().ok()).filter(|v| v.is_finite()) +} + +const EMU: f64 = 12700.0; +const ANGLE: f64 = 60000.0; + +/// An `a:`-prefixed element `local` (with its children) cut from `raw`, if there is one. +fn element(raw: &str, local: &str) -> Option { + let open = format!(""); + let i = raw.find(&open)?; + let j = raw.get(i..)?.find(&close)? + i + close.len(); + raw.get(i..j).map(str::to_string) +} + +impl Shape3d { + /// Reads kept 3-D XML; unknown parts take their defaults. + pub fn parse(raw: &str) -> Shape3d { + let mut s = Shape3d::default(); + let mut in_camera = false; + for (name, tag) in tags(raw) { + match name { + "camera" => { + s.camera = attr(tag, "prst").unwrap_or("orthographicFront").to_string(); + in_camera = !tag.ends_with('/'); + } + "rot" if in_camera => { + let d = |n| num(tag, n).unwrap_or(0.0) / ANGLE; + s.rot = Some((d("lon"), d("lat"), d("rev"))); + in_camera = false; + } + "lightRig" => { + in_camera = false; + s.light = attr(tag, "rig").unwrap_or("threePt").to_string(); + s.light_dir = attr(tag, "dir").unwrap_or("t").to_string(); + } + "sp3d" => { + in_camera = false; + s.depth = num(tag, "extrusionH").unwrap_or(0.0) / EMU; + s.contour = num(tag, "contourW").unwrap_or(0.0) / EMU; + s.z = num(tag, "z").unwrap_or(0.0) / EMU; + s.material = attr(tag, "prstMaterial").unwrap_or("warmMatte").to_string(); + } + "bevelT" | "bevelB" => { + let b = Bevel { + preset: attr(tag, "prst").unwrap_or("circle").to_string(), + w: num(tag, "w").unwrap_or(76200.0) / EMU, + h: num(tag, "h").unwrap_or(76200.0) / EMU, + }; + if name == "bevelT" { + s.top = Some(b); + } else { + s.bottom = Some(b); + } + } + _ => {} + } + } + s.colors = ["extrusionClr", "contourClr"].iter().filter_map(|n| element(raw, n)).collect(); + s + } + + /// Whether this is a flat shape with no 3-D at all. + pub fn is_flat(&self) -> bool { + self.camera == "orthographicFront" + && self.rot.is_none_or(|(x, y, z)| x == 0.0 && y == 0.0 && z == 0.0) + && self.top.is_none() + && self.bottom.is_none() + && self.depth == 0.0 + && self.contour == 0.0 + && self.z == 0.0 + } + + /// The `a:scene3d` and `a:sp3d` XML, or `None` for a flat shape. + pub fn to_xml(&self) -> Option { + if self.is_flat() { + return None; + } + let e = |pt: f64| (pt * EMU).round() as i64; + let mut x = format!(" { + let a = |d: f64| (d.rem_euclid(360.0) * ANGLE).round() as i64 % 21_600_000; + x += &format!(">", a(ry), a(rx), a(rz)); + } + None => x += "/>", + } + x += &format!(" 0.0 { + x += &format!(" extrusionH=\"{}\"", e(self.depth)); + } + if self.contour > 0.0 { + x += &format!(" contourW=\"{}\"", e(self.contour)); + } + if self.material != "warmMatte" { + x += &format!(" prstMaterial=\"{}\"", self.material); + } + x += ">"; + for (tag, b) in [("bevelT", &self.top), ("bevelB", &self.bottom)] { + if let Some(b) = b { + x += &format!("", e(b.w), e(b.h), b.preset); + } + } + x += &self.colors; + x += ""; + Some(x) + } + + /// Applies `{camera?, rotX?, rotY?, rotZ?, z?, bevelTop?, bevelTopW?, bevelTopH?, bevelBottom?, + /// bevelBottomW?, bevelBottomH?, depth?, contour?, material?, lighting?, lightAngle?}`. + pub fn apply(&mut self, p: &Value) { + let f = |k: &str| p.get(k).and_then(Value::as_f64).filter(|v| v.is_finite()); + let pick = |k: &str, list: &[&str]| p.get(k).and_then(Value::as_str).filter(|v| list.contains(v)).map(str::to_string); + if let Some(c) = pick("camera", CAMERAS) { + self.camera = c; + // A preset sets its own angles. + self.rot = None; + } + let (rx, ry, rz) = (f("rotX"), f("rotY"), f("rotZ")); + if rx.is_some() || ry.is_some() || rz.is_some() { + let (ox, oy, oz) = self.rot.unwrap_or((0.0, 0.0, 0.0)); + let d = |v: Option, o: f64| v.map_or(o, |v| v.rem_euclid(360.0)); + self.rot = Some((d(rx, ox), d(ry, oy), d(rz, oz))); + } + if let Some(v) = f("z") { + self.z = v.clamp(-1584.0, 1584.0); + } + for (key, slot) in [("bevelTop", &mut self.top), ("bevelBottom", &mut self.bottom)] { + match p.get(key).and_then(Value::as_str) { + Some("none") => *slot = None, + Some(b) if BEVELS.contains(&b) => { + let old = slot.take().unwrap_or(Bevel { preset: String::new(), w: 6.0, h: 6.0 }); + *slot = Some(Bevel { preset: b.to_string(), ..old }); + } + _ => {} + } + if let Some(b) = slot.as_mut() { + if let Some(w) = f(&format!("{key}W")) { + b.w = w.clamp(0.0, 1584.0); + } + if let Some(h) = f(&format!("{key}H")) { + b.h = h.clamp(0.0, 1584.0); + } + } + } + if let Some(v) = f("depth") { + self.depth = v.clamp(0.0, 1584.0); + } + if let Some(v) = f("contour") { + self.contour = v.clamp(0.0, 1584.0); + } + if let Some(m) = pick("material", MATERIALS) { + self.material = m; + } + if let Some(l) = pick("lighting", LIGHTS) { + self.light = l; + } + } +} + +/// Sets a shape's 3-D settings on its effects, keeping `bevel` (the top bevel's preset) in step. +pub fn store(e: &mut deckcraft_model::Effects, s: &Shape3d) { + e.raw3d = s.to_xml(); + e.bevel = if e.raw3d.is_some() { s.top.as_ref().map(|b| b.preset.clone()) } else { None }; +} diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index e83f72ef..ceb5dd1a 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -835,3 +835,53 @@ fn slide_ranges_parse_like_powerpoints_print_box() { let opts = cmd::file::pdf_options(&json!({"slides": [0, 7, 1]}), 2).unwrap(); assert_eq!(opts.slides, Some(vec![0, 1]), "indices past the end are dropped"); } + +#[test] +fn shape_three_d_format_and_rotation_round_trip() { + use crate::cmd::three_d::Shape3d; + let mut s = Session::with_new(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + s.execute("shape.insert", &json!({"preset": "rect", "rect": [100, 100, 200, 100]})).unwrap(); + let fx = |s: &Session| s.doc().unwrap().selected_shapes()[0].effects.clone().unwrap_or_default(); + // The old `bevel` key now writes real 3-D XML, so it is drawn and saved. + s.execute("shape.effects", &json!({"bevel": "angle"})).unwrap(); + let e = fx(&s); + assert_eq!(e.bevel.as_deref(), Some("angle")); + assert!(e.raw3d.as_deref().is_some_and(|r| r.contains("bevelT") && r.contains("prst=\"angle\""))); + s.execute( + "shape.effects", + &json!({"threeD": {"bevelTopW": 9, "bevelBottom": "circle", "depth": 20, "contour": 2, "material": "metal", + "lighting": "harsh", "camera": "isometricTopUp", "rotX": 30, "rotY": -20, "z": 5, "bogus": 1}}), + ) + .unwrap(); + // Unknown names and non-finite numbers are ignored. + s.execute("shape.effects", &json!({"threeD": {"material": "gold", "depth": f64::NAN, "camera": 7}})).unwrap(); + let want = fx(&s).raw3d.map(|r| Shape3d::parse(&r)).unwrap(); + assert_eq!(want.top.as_ref().map(|b| (b.preset.as_str(), b.w, b.h)), Some(("angle", 9.0, 6.0))); + assert_eq!(want.bottom.as_ref().map(|b| b.preset.as_str()), Some("circle")); + assert_eq!((want.depth, want.contour, want.z), (20.0, 2.0, 5.0)); + assert_eq!((want.material.as_str(), want.light.as_str(), want.camera.as_str()), ("metal", "harsh", "isometricTopUp")); + assert_eq!(want.rot, Some((30.0, 340.0, 0.0))); + // PPTX save and reopen keeps every setting. + let bytes = cmd::file::save_bytes(&s.doc().unwrap().doc, "pptx").unwrap(); + s.execute("file.openBytes", &json!({"name": "d.pptx", "data": cmd::base64_encode(&bytes)})).unwrap(); + let back = s.doc().unwrap().doc.slides.iter().flat_map(|sl| sl.shapes.iter()).find_map(|sh| sh.effects.as_ref()?.raw3d.clone()); + assert_eq!(Shape3d::parse(&back.unwrap()), want); +} + +#[test] +fn three_d_reset_and_flat_shapes_drop_the_xml() { + let mut s = Session::with_new(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + s.execute("shape.insert", &json!({"preset": "rect", "rect": [100, 100, 200, 100]})).unwrap(); + let fx = |s: &Session| s.doc().unwrap().selected_shapes()[0].effects.clone().unwrap_or_default(); + s.execute("shape.effects", &json!({"threeD": {"rotZ": 15, "depth": 4}})).unwrap(); + assert!(fx(&s).raw3d.is_some()); + s.execute("shape.effects", &json!({"threeD": {"reset": true}})).unwrap(); + assert_eq!((fx(&s).raw3d, fx(&s).bevel), (None, None)); + // Keeps colours it doesn't model. + let raw = ""; + let mut s3 = cmd::three_d::Shape3d::parse(raw); + s3.apply(&json!({"depth": 3})); + assert!(s3.to_xml().unwrap().contains("")); +} diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index 2b3ada3a..63c5e48b 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -150,6 +150,8 @@ fn format_shape(app: &mut SlideApp, ui: &mut Ui) { if ui.add(egui::Slider::new(&mut s, 0.0..=50.0).text("size (pt)")).drag_stopped() { run(app, "shape.effects", json!({"softEdges": s})); } + let raw3d = sh.effects.as_ref().and_then(|e| e.raw3d.clone()); + three_d_options(app, ui, sh.id.0, raw3d.as_deref()); if ui.button("Reset Effects").clicked() { run(app, "shape.effects", json!({"reset": true})); } @@ -339,6 +341,79 @@ fn format_shape(app: &mut SlideApp, ui: &mut Ui) { } } +/// "perspectiveHeroicLeftFacing" → "Perspective heroic left facing". +fn preset_label(name: &str) -> String { + let mut out = String::new(); + for (i, c) in name.chars().enumerate() { + if i == 0 { + out.extend(c.to_uppercase()); + } else if c.is_uppercase() { + out.push(' '); + out.extend(c.to_lowercase()); + } else { + out.push(c); + } + } + out +} + +/// A drop-down of `options`; picking one runs `shape.effects` with `{threeD: {key: option}}`. +fn three_d_combo(app: &mut SlideApp, ui: &mut Ui, id: u32, label: &str, key: &str, cur: &str, options: &[&str]) { + ui.horizontal(|ui| { + ui.add_sized(vec2(90.0, 20.0), egui::Label::new(label)); + egui::ComboBox::from_id_salt(("3d", id, key)).selected_text(preset_label(cur)).width(170.0).show_ui(ui, |ui| { + for o in options { + if ui.selectable_label(*o == cur, preset_label(o)).clicked() { + run(app, "shape.effects", json!({"threeD": {key: o}})); + } + } + }); + }); +} + +/// A number field running `shape.effects` with `{threeD: {key: value}}` when it changes. +fn three_d_drag(app: &mut SlideApp, ui: &mut Ui, label: &str, value: f64, suffix: &str, key: &str) { + ui.horizontal(|ui| { + ui.add_sized(vec2(90.0, 20.0), egui::Label::new(label)); + let mut v = value; + let r = ui.add(egui::DragValue::new(&mut v).suffix(suffix).max_decimals(1)); + if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged()) { + run(app, "shape.effects", json!({"threeD": {key: v}})); + } + }); +} + +/// Format Shape ▸ Effects ▸ 3-D Format and 3-D Rotation. +fn three_d_options(app: &mut SlideApp, ui: &mut Ui, id: u32, raw: Option<&str>) { + use deckcraft_engine::cmd::three_d::{BEVELS, CAMERAS, LIGHTS, MATERIALS, Shape3d}; + let s3 = raw.map(Shape3d::parse).unwrap_or_default(); + let with_none: Vec<&str> = std::iter::once("none").chain(BEVELS.iter().copied()).collect(); + ui.add_space(6.0); + ui.label(egui::RichText::new("3-D Format").font(theme::bold(13.0))); + for (label, key, b) in [("Top bevel", "bevelTop", &s3.top), ("Bottom bevel", "bevelBottom", &s3.bottom)] { + three_d_combo(app, ui, id, label, key, b.as_ref().map_or("none", |b| b.preset.as_str()), &with_none); + if let Some(b) = b { + three_d_drag(app, ui, " Width", b.w, " pt", &format!("{key}W")); + three_d_drag(app, ui, " Height", b.h, " pt", &format!("{key}H")); + } + } + three_d_drag(app, ui, "Depth", s3.depth, " pt", "depth"); + three_d_drag(app, ui, "Contour", s3.contour, " pt", "contour"); + three_d_combo(app, ui, id, "Material", "material", &s3.material, MATERIALS); + three_d_combo(app, ui, id, "Lighting", "lighting", &s3.light, LIGHTS); + ui.add_space(6.0); + ui.label(egui::RichText::new("3-D Rotation").font(theme::bold(13.0))); + three_d_combo(app, ui, id, "Presets", "camera", &s3.camera, CAMERAS); + let (rx, ry, rz) = s3.rot.unwrap_or((0.0, 0.0, 0.0)); + three_d_drag(app, ui, "X Rotation", rx, "°", "rotX"); + three_d_drag(app, ui, "Y Rotation", ry, "°", "rotY"); + three_d_drag(app, ui, "Z Rotation", rz, "°", "rotZ"); + three_d_drag(app, ui, "Distance from ground", s3.z, " pt", "z"); + if ui.add_enabled(raw.is_some(), egui::Button::new("Reset 3-D")).clicked() { + run(app, "shape.effects", json!({"threeD": {"reset": true}})); + } +} + fn background(app: &mut SlideApp, ui: &mut Ui) { let sc = scheme(app); let bg = app.session.active().and_then(|d| d.current_slide().and_then(|s| s.background.clone())); diff --git a/docs/ui-parity.md b/docs/ui-parity.md index c403f05e..709f6771 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -58,7 +58,7 @@ menu colours taken from the theme tokens (menu `chrome`, page `card`). | Pane / dialog | PowerPoint | DeckCraft | |---|---|---| -| Format Shape / Picture / Text pane (Fill & Line, Effects, Size & Properties, Picture, Text Box) | full | partial: fill, gradient editor, line, some effects, size; no 3-D, thin picture/texture options | +| Format Shape / Picture / Text pane (Fill & Line, Effects, Size & Properties, Picture, Text Box) | full | partial: fill, gradient editor, line, some effects, 3-D format and rotation (bevels, depth, contour, material, lighting, camera presets, X/Y/Z rotation), size; thin picture/texture options | | Format Background pane | yes | yes | | Selection Pane | front-most first, groups fold, drag to restack, Bring Forward / Send Backward arrows, Show All / Hide All, eye, padlock (Lock), rename in place | same: drag restacks among siblings (a child stays in its group), Ctrl+click toggles, Shift+click selects a run, arrows / Home / End / F2 / Esc in the list, right-click menu | | Animation Pane | list + advanced timeline | list, reorder, timing; no timeline | From 7432c61665ca72f002b5b4b9db844ba6c8ebf46c Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Tue, 13 Oct 2026 22:14:46 +0300 Subject: [PATCH 06/16] Slides: reuse slides from another presentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Home › New Slide gains Reuse Slides… and Reuse Slides (Keep Source Formatting)…, backed by a new slide.reuse command that inserts the slides of a .pptx or .deckcraft file (all, a list, or a range) after the current slide in one undo step. By default each slide moves to the destination layout with the same name or kind, so it takes this deck's theme; keepFormatting brings the source master and its layouts along. Pictures, media and sounds are copied under fresh media ids, so ids that clash between the two decks no longer mix up pictures, and shape ids are renumbered with animations, triggers and connector ends following them. Co-authored-by: Cursor --- crates/engine/src/cmd/mod.rs | 2 + crates/engine/src/cmd/reuse.rs | 226 +++++++++++++++++++++++++++++++++ crates/pptx/tests/roundtrip.rs | 63 +++++++++ crates/ui-egui/src/lib.rs | 16 +++ crates/ui-egui/src/ribbon.rs | 8 ++ docs/gaps.md | 2 +- docs/parity-checklist.md | 2 +- 7 files changed, 317 insertions(+), 2 deletions(-) create mode 100644 crates/engine/src/cmd/reuse.rs diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index ce909b02..7cd60555 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -10,6 +10,7 @@ pub mod insert; pub mod inspect; pub mod media; pub mod merge; +pub mod reuse; pub mod review; pub mod selection; pub mod shape; @@ -128,6 +129,7 @@ pub fn command_specs() -> &'static [CommandSpec] { v.extend(file::specs()); v.extend(edit::specs()); v.extend(slide::specs()); + v.extend(reuse::specs()); v.extend(insert::specs()); v.extend(shape::specs()); v.extend(arrange::specs()); diff --git a/crates/engine/src/cmd/reuse.rs b/crates/engine/src/cmd/reuse.rs new file mode 100644 index 00000000..d5e31b11 --- /dev/null +++ b/crates/engine/src/cmd/reuse.rs @@ -0,0 +1,226 @@ +//! Home ▸ New Slide ▸ Reuse Slides: insert slides from another presentation. +//! +//! The slides take the destination's look by default (each one moves to the destination layout +//! with the same name, else the same kind); with `keepFormatting` the source master and its +//! layouts come along. Pictures, media and sounds are copied under fresh ids, and shape ids are +//! renumbered with animations, triggers and connector ends following them. + +use std::collections::HashMap; +use std::sync::Arc; + +use deckcraft_model::{LayoutId, MasterId, MediaId, Presentation, ShapeId, ShapeKind, Slide, SlideId}; +use serde_json::{Value, json}; + +use super::*; +use crate::{Result, Session}; + +pub fn specs() -> Vec { + vec![cmd!( + "slide.reuse", + "Reuse Slides…", + ["Home", "Slides", "New Slide"], + None, + "{path? | data: base64, name?; slides?: [index] (0-based, default all); keepFormatting?: bool (default false: use the destination theme); at?: index (default after the current slide)}", + has_doc, + reuse + )] +} + +/// Keys under which the model stores a [`MediaId`] (pictures, media, posters, previews, sounds). +const MEDIA_KEYS: [&str; 4] = ["media", "poster", "preview", "sound"]; + +/// Rewrite every media id in `v` through `map`. +fn remap_media(v: &mut Value, map: &HashMap, depth: usize) { + if depth > 256 { + return; + } + match v { + Value::Object(o) => { + for (k, x) in o.iter_mut() { + if MEDIA_KEYS.contains(&k.as_str()) + && let Some(n) = x.as_u64().and_then(|n| u32::try_from(n).ok()) + { + if let Some(to) = map.get(&n) { + *x = json!(to); + } + continue; + } + remap_media(x, map, depth + 1); + } + } + Value::Array(a) => a.iter_mut().for_each(|x| remap_media(x, map, depth + 1)), + _ => {} + } +} + +/// `value` with its media ids rewritten, via its JSON form. +fn with_media(value: &T, map: &HashMap) -> Result { + let mut v = serde_json::to_value(value).map_err(|e| EngineError::Other(e.to_string()))?; + remap_media(&mut v, map, 0); + serde_json::from_value(v).map_err(|e| EngineError::Other(e.to_string())) +} + +fn shape_ids(shapes: &[deckcraft_model::Shape]) -> Vec { + let mut v = vec![]; + deckcraft_model::walk(shapes, &mut |sh, _| v.push(sh.id)); + v +} + +/// Point connector ends at the renumbered shapes. +fn remap_connectors(shapes: &mut [deckcraft_model::Shape], map: &HashMap, depth: usize) { + if depth > 64 { + return; + } + for sh in shapes { + if let ShapeKind::Connector { start, end } = &mut sh.kind { + for c in [start, end] { + if let Some((id, _)) = c.as_mut() { + *id = map.get(id).copied().unwrap_or(*id); + } + } + } + if let Some(ch) = sh.children_mut() { + remap_connectors(ch, map, depth + 1); + } + } +} + +/// Fresh shape ids for `shapes`, and the old → new map. +fn renumber(doc: &mut Presentation, shapes: &mut [deckcraft_model::Shape]) -> HashMap { + let old = shape_ids(shapes); + super::edit::reid(doc, shapes); + let map: HashMap = old.into_iter().zip(shape_ids(shapes)).collect(); + remap_connectors(shapes, &map, 0); + map +} + +/// The destination layout a reused slide moves to: same name, else same kind, else the first. +fn matching_layout(doc: &Presentation, name: &str, kind: deckcraft_model::LayoutType) -> LayoutId { + let all = || doc.masters.iter().flat_map(|m| m.layouts.iter()); + all() + .find(|l| !name.is_empty() && l.name.eq_ignore_ascii_case(name)) + .or_else(|| all().find(|l| l.kind == kind)) + .or_else(|| all().next()) + .map(|l| l.id) + .unwrap_or_default() +} + +fn reuse(s: &mut Session, p: &Value) -> Result { + let (name, bytes) = super::insert::media_bytes(p, "slide.reuse")?; + let src = super::file::open_presentation(&name, &bytes)?; + let n = src.slides.len(); + let picked: Vec = match p.get("slides") { + Some(Value::Array(a)) => { + let mut v = vec![]; + for x in a { + let i = x.as_u64().map(|i| i as usize).filter(|i| *i < n); + let i = i.ok_or_else(|| bad("slide.reuse", format!("{name} has {n} slides; {x} isn't one of them (indices count from 0)")))?; + if !v.contains(&i) { + v.push(i); + } + } + v + } + Some(Value::String(t)) if !t.trim().is_empty() => super::file::parse_slide_range(t, n)?, + _ => (0..n).collect(), + }; + if picked.is_empty() { + return Err(bad("slide.reuse", format!("{name} has no slides to reuse"))); + } + let keep = bool_or(p, "keepFormatting", false); + let at = usize_param(p, "at"); + s.edit(|doc, sel| { + let at = at.unwrap_or(sel.slide + 1).min(doc.slides.len()); + // Media: what the slides use is copied under fresh ids. + let mut media_map: HashMap = HashMap::new(); + // Keep Source Formatting: bring each source master used, once, with fresh ids. + let mut layout_map: HashMap = HashMap::new(); + if keep { + let mut masters_done: Vec = vec![]; + for &i in &picked { + let Some((m, _)) = src.master_for(&src.slides[i]) else { continue }; + if masters_done.contains(&m.id) { + continue; + } + masters_done.push(m.id); + let used = serde_json::to_value(m).map_err(|e| EngineError::Other(e.to_string()))?; + copy_media(doc, &src, &used, &mut media_map); + let mut m: deckcraft_model::Master = with_media(m, &media_map)?; + m.id = MasterId(doc.alloc_id()); + renumber(doc, &mut m.shapes); + for l in &mut m.layouts { + let new = LayoutId(doc.alloc_id()); + layout_map.insert(l.id, new); + l.id = new; + renumber(doc, &mut l.shapes); + } + doc.masters.push(Arc::new(m)); + } + } + let mut new_ids = vec![]; + for (k, &i) in picked.iter().enumerate() { + let from = &src.slides[i]; + let used = serde_json::to_value(from.as_ref()).map_err(|e| EngineError::Other(e.to_string()))?; + copy_media(doc, &src, &used, &mut media_map); + let mut sl: Slide = with_media(from.as_ref(), &media_map)?; + sl.id = SlideId(doc.alloc_id()); + let map = renumber(doc, &mut sl.shapes); + for a in &mut sl.animations { + a.shape = map.get(&a.shape).copied().unwrap_or(a.shape); + a.trigger = a.trigger.map(|t| map.get(&t).copied().unwrap_or(t)); + } + sl.layout = match layout_map.get(&sl.layout) { + Some(l) => *l, + None => { + let (lname, kind) = + src.layout(from.layout).map(|(_, l)| (l.name.clone(), l.kind)).unwrap_or_else(|| (String::new(), Default::default())); + matching_layout(doc, &lname, kind) + } + }; + new_ids.push(sl.id); + doc.slides.insert((at + k).min(doc.slides.len()), Arc::new(sl)); + } + sel.slide = (at + new_ids.len()).saturating_sub(1).min(doc.slides.len().saturating_sub(1)); + sel.slides = new_ids.clone(); + sel.shapes.clear(); + sel.text = None; + Ok(json!({"slides": new_ids.len(), "ids": new_ids, "from": name, "keepFormatting": keep})) + }) +} + +/// Copy the source media `used` refers to into `doc` under fresh ids, once each. +fn copy_media(doc: &mut Presentation, src: &Presentation, used: &Value, map: &mut HashMap) { + let mut ids = vec![]; + collect_media(used, &mut ids, 0); + for id in ids { + if map.contains_key(&id) { + continue; + } + if let Some(m) = src.media(MediaId(id)) { + let new = doc.add_media(&m.name, &m.content_type, m.data.as_ref().clone()); + map.insert(id, new.0); + } + } +} + +/// The media ids `v` refers to. +fn collect_media(v: &Value, out: &mut Vec, depth: usize) { + if depth > 256 { + return; + } + match v { + Value::Object(o) => { + for (k, x) in o { + if MEDIA_KEYS.contains(&k.as_str()) + && let Some(n) = x.as_u64().and_then(|n| u32::try_from(n).ok()) + { + out.push(n); + } else { + collect_media(x, out, depth + 1); + } + } + } + Value::Array(a) => a.iter().for_each(|x| collect_media(x, out, depth + 1)), + _ => {} + } +} diff --git a/crates/pptx/tests/roundtrip.rs b/crates/pptx/tests/roundtrip.rs index db6d0beb..ef7f1ab9 100644 --- a/crates/pptx/tests/roundtrip.rs +++ b/crates/pptx/tests/roundtrip.rs @@ -991,3 +991,66 @@ fn transition_defaults_read_like_powerpoint() { assert_eq!((t.kind.as_str(), t.option.as_str(), t.duration_ms), (*kind, *option, 2000), "{el}"); } } + +/// Home › New Slide › Reuse Slides: slides from another .pptx land after the current slide with +/// their pictures (fresh media ids, same bytes) and animations on the renumbered shapes. By +/// default they take a destination layout; Keep Source Formatting brings the source master. +#[test] +fn reuse_slides_from_another_presentation() { + use deckcraft_engine::Session; + use serde_json::json; + let b64 = deckcraft_engine::cmd::base64_encode; + // Source: three slides, a picture with an animation on the second. + let mut src = Session::with_new(); + src.execute("slide.new", &json!({})).unwrap(); + let pic = png(8, 8, [10, 200, 30]); + src.execute("insert.picture", &json!({"data": b64(&pic), "name": "green.png"})).unwrap(); + src.execute("animation.add", &json!({"effect": "fade"})).unwrap(); + src.execute("slide.new", &json!({})).unwrap(); + let src_bytes = deckcraft_pptx::export(&src.doc().unwrap().doc).unwrap(); + + let mut s = Session::with_new(); + // Media ids in the two decks overlap; the destination's own picture must stay its own. + let red = png(4, 4, [200, 0, 0]); + s.execute("insert.picture", &json!({"data": b64(&red), "name": "red.png"})).unwrap(); + let r = s.execute("slide.reuse", &json!({"data": b64(&src_bytes), "name": "src.pptx", "slides": [1]})).unwrap(); + assert_eq!(r["slides"], 1, "{r}"); + let doc = s.doc().unwrap().doc.clone(); + assert_eq!(doc.slides.len(), 2); + assert_eq!(s.doc().unwrap().selection.slide, 1, "the reused slide is selected"); + let pictures = |sl: &deckcraft_model::Slide| { + let mut v = vec![]; + deckcraft_model::walk(&sl.shapes, &mut |sh, _| { + if let deckcraft_model::ShapeKind::Picture { fill } = &sh.kind { + v.push(fill.media); + } + }); + v + }; + let mine = pictures(&doc.slides[0]); + let reused = pictures(&doc.slides[1]); + assert_eq!(doc.media(mine[0]).map(|m| m.data.as_slice() == red.as_slice()), Some(true), "own picture untouched"); + assert_eq!(doc.media(reused[0]).map(|m| m.data.as_slice() == pic.as_slice()), Some(true), "reused picture copied"); + let anim = &doc.slides[1].animations[0]; + assert!(doc.slides[1].shapes.iter().any(|sh| sh.id == anim.shape), "animation follows its shape"); + assert!(doc.layout(doc.slides[1].layout).is_some(), "uses a destination layout"); + assert_eq!(doc.masters.len(), 1); + // The result saves and reopens. + let back = deckcraft_pptx::import(&deckcraft_pptx::export(&doc).unwrap()).unwrap(); + assert_eq!(back.slides.len(), 2); + + // Keep Source Formatting: every slide, and the source master comes along. + let r = s.execute("slide.reuse", &json!({"data": b64(&src_bytes), "name": "src.pptx", "keepFormatting": true, "at": 0})).unwrap(); + assert_eq!(r["slides"], 3); + let doc = s.doc().unwrap().doc.clone(); + assert_eq!(doc.slides.len(), 5); + assert_eq!(doc.masters.len(), 2); + for sl in &doc.slides[..3] { + assert_eq!(doc.layout(sl.layout).map(|(m, _)| m.id), Some(doc.masters[1].id), "source master"); + } + // One undo removes the whole reuse. + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc.slides.len(), 2); + assert!(s.execute("slide.reuse", &json!({"data": b64(&src_bytes), "name": "src.pptx", "slides": [7]})).is_err()); + assert!(s.execute("slide.reuse", &json!({"data": b64(&[0u8, 159, 146, 150]), "name": "x.bin"})).is_err()); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ec97e6e6..4d3b314e 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -164,6 +164,7 @@ pub const UI_COMMANDS: &[(&str, &str, Option<&str>, &str)] = &[ ("app.openDialog", "Open…", None, "{purpose?}"), ("app.insertPictureDialog", "Picture from File…", None, "{}"), ("app.insertAudioDialog", "Audio from File…", None, "{}"), + ("app.reuseSlidesDialog", "Reuse Slides…", None, "{keepFormatting?: bool}"), ("app.insertVideoDialog", "Video from File…", None, "{}"), ("app.saveAsDialog", "Save As…", None, "{}"), ("app.exportDialog", "Export…", None, "{}"), @@ -538,6 +539,7 @@ impl SlideApp { "app.openDialog" => self.pick_and_open(), "app.insertPictureDialog" => self.pick_and_insert("picture", "insert.picture"), "app.insertAudioDialog" => self.pick_and_insert("audio", "insert.audio"), + "app.reuseSlidesDialog" => self.pick_and_reuse(p.get("keepFormatting").and_then(Value::as_bool).unwrap_or(false)), "app.insertVideoDialog" => self.pick_and_insert("video", "insert.video"), "app.saveAsDialog" => self.save_as_dialog("deckcraft"), "app.exportDialog" => self.dialog = Some(dialogs::Dialog::new("export")), @@ -580,6 +582,20 @@ impl SlideApp { Ok(()) } + /// Home › New Slide › Reuse Slides…: pick a presentation and insert all its slides after the + /// current one (desktop only: the browser picker opens what it picks as a document). + fn pick_and_reuse(&mut self, keep: bool) { + let Some(pick) = self.services.pick_open.as_mut() else { + self.set_status("Reuse Slides needs the desktop app".to_string()); + return; + }; + let Some(path) = pick("open") else { return }; + match self.session.execute("slide.reuse", &json!({"path": path, "keepFormatting": keep})) { + Ok(r) => self.set_status(format!("Inserted {} slides from {path}", r["slides"])), + Err(e) => self.set_status(e.to_string()), + } + } + fn pick_and_insert(&mut self, purpose: &str, cmd: &str) { if let Some(pick) = self.services.pick_open.as_mut() { if let Some(path) = pick(purpose) { diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 428eff0b..0e4a0e0d 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -380,6 +380,14 @@ fn slides_group(app: &mut SlideApp, ui: &mut Ui) { if ui.button("Slides from Outline…").clicked() { app.dialog = Some(crate::dialogs::Dialog::new("outline")); } + let desktop = app.services.pick_open.is_some(); + for (label, keep) in [("Reuse Slides…", false), ("Reuse Slides (Keep Source Formatting)…", true)] { + let r = ui.add_enabled(desktop, egui::Button::new(label)); + if r.clicked() { + run(app, "app.reuseSlidesDialog", json!({"keepFormatting": keep})); + } + r.on_disabled_hover_text("Choosing a presentation to reuse needs the desktop app"); + } }); } rows(ui, |ui| { diff --git a/docs/gaps.md b/docs/gaps.md index a2894b42..ecc3d27e 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -184,7 +184,7 @@ Detail in [animation-parity.md](animation-parity.md). 10–16 h; the pane is a * - **Estimate:** 3–6 h to review, land and round-trip. ### Smaller missing rows -Zoom (summary/section/slide, 6–10 h), reuse slides (2–4 h), laser pointer (1–2 h), transition +Zoom (summary/section/slide, 6–10 h), reuse slides pane with thumbnails (1–2 h; `slide.reuse` inserts from a file), laser pointer (1–2 h), transition sounds (1–2 h), artistic effects (6–10 h), remove background (6–10 h; needs a model or a classical matte), compress pictures (2–3 h), insert screenshot (2–3 h), record audio/screen and record slide show (8–12 h), compare presentations (6–10 h), password protection (4–6 h, ECMA-376 agile diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 2a3d450c..2237d6db 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -68,7 +68,7 @@ Open P0 items: Contextual tabs (Shape Format, Picture Format, Table Design/Layou | Files | Export video (MP4) / animated GIF | P2 | M | | Files | Print (slides, handouts, notes, outline) | P1 | M | | Files | Password protection | P2 | M | -| Files | Reuse slides from another file | P1 | M | +| Files | Reuse slides from another file | P1 | P | | Files | Drag and drop files onto the window | P1 | D | | Files | OpenDocument .odp open/save | P2 | M | | Slides | New slide by layout | P0 | D | From 5eef9fb55acf3312dc87973d3a2aec74103cecaf Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Tue, 13 Oct 2026 22:21:57 +0300 Subject: [PATCH 07/16] Pictures: compress pictures to a target resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picture Format › Adjust gains Compress Pictures, with PowerPoint's options: HD 330, Print 220, Web 150 or E-mail 96 ppi, apply to the selected pictures or all, and delete cropped areas. The new picture.compress command downsamples each PNG, JPEG, BMP, TIFF or WebP picture to the pixels its shown size needs at that resolution (JPEG stays JPEG at quality 85, the rest become PNG), cuts away cropped areas when asked, and only replaces a picture when the result is smaller or a crop was removed. Originals nothing else uses are dropped so the saved file shrinks; one shared with a picture that wasn't compressed stays. One undo step restores everything. Co-authored-by: Cursor --- crates/engine/src/cmd/compress.rs | 253 ++++++++++++++++++++++++++++++ crates/engine/src/cmd/mod.rs | 2 + crates/pptx/tests/roundtrip.rs | 45 ++++++ crates/ui-egui/src/ribbon.rs | 41 +++++ docs/gaps.md | 2 +- docs/parity-checklist.md | 2 +- 6 files changed, 343 insertions(+), 2 deletions(-) create mode 100644 crates/engine/src/cmd/compress.rs diff --git a/crates/engine/src/cmd/compress.rs b/crates/engine/src/cmd/compress.rs new file mode 100644 index 00000000..01da57eb --- /dev/null +++ b/crates/engine/src/cmd/compress.rs @@ -0,0 +1,253 @@ +//! Picture Format ▸ Adjust ▸ Compress Pictures: downsample pictures to a target resolution for +//! the size they are shown at, and optionally delete their cropped-away areas. + +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; + +use deckcraft_model::style::PictureFill; +use deckcraft_model::{MediaId, Presentation, Shape, ShapeId, ShapeKind}; +use serde_json::{Value, json}; + +use super::*; +use crate::{Result, Session}; + +pub fn specs() -> Vec { + vec![cmd!( + "picture.compress", + "Compress Pictures", + ["Picture Format", "Adjust"], + None, + "{ppi?: 330|220|150|96 (default 220), deleteCropped?: bool (default true), all?: bool (every picture in the presentation; default the selected pictures), ids?} → {pictures, before, after}", + has_doc, + compress + )] +} + +/// A picture compressed for one shape: new bytes, content type and the crop it now needs. +struct Packed { + bytes: Vec, + content_type: &'static str, + crop: [f64; 4], +} + +/// Whether `crop` cuts something away (and only cuts: negative values pad, and are kept). +fn real_crop(c: &[f64; 4]) -> bool { + c.iter().all(|v| (0.0..1.0).contains(v)) && c.iter().any(|v| *v > 0.0) && c[0] + c[2] < 1.0 && c[1] + c[3] < 1.0 +} + +/// Compress `data` for a picture shown `w_pt` × `h_pt` with `crop`, at `ppi`. `None` when the +/// format isn't a raster we re-encode (SVG, EMF, GIF…) or nothing would get smaller. +fn pack(data: &[u8], crop: [f64; 4], w_pt: f64, h_pt: f64, ppi: f64, delete_cropped: bool) -> Option { + use image::ImageFormat as F; + let format = image::guess_format(data).ok()?; + if !matches!(format, F::Png | F::Jpeg | F::Bmp | F::Tiff | F::WebP) { + return None; + } + let mut img = image::load_from_memory_with_format(data, format).ok()?; + let (iw, ih) = (img.width() as f64, img.height() as f64); + if iw < 1.0 || ih < 1.0 || iw * ih > 400_000_000.0 { + return None; + } + let cut = delete_cropped && real_crop(&crop); + let mut new_crop = crop; + // The share of the image that is visible across and down. + let (mut fx, mut fy) = ((1.0 - crop[0] - crop[2]).max(0.01), (1.0 - crop[1] - crop[3]).max(0.01)); + if cut { + let x = (crop[0] * iw).floor() as u32; + let y = (crop[1] * ih).floor() as u32; + let w = ((fx * iw).round() as u32).clamp(1, img.width() - x.min(img.width() - 1)); + let h = ((fy * ih).round() as u32).clamp(1, img.height() - y.min(img.height() - 1)); + img = img.crop_imm(x, y, w, h); + new_crop = [0.0; 4]; + (fx, fy) = (1.0, 1.0); + } else if !crop.iter().all(|v| v.is_finite()) { + return None; + } + // Pixels needed for the whole (possibly still cropped) image at `ppi`. + let need_w = (w_pt.abs() / 72.0 * ppi / fx).ceil().max(1.0); + let need_h = (h_pt.abs() / 72.0 * ppi / fy).ceil().max(1.0); + let k = (need_w / img.width() as f64).max(need_h / img.height() as f64); + let resized = k < 1.0; + if resized { + let w = ((img.width() as f64 * k).round() as u32).max(1); + let h = ((img.height() as f64 * k).round() as u32).max(1); + img = img.resize_exact(w, h, image::imageops::FilterType::Lanczos3); + } + if !resized && !cut { + return None; + } + let alpha = img.color().has_alpha() && img.to_rgba8().pixels().any(|p| p.0[3] < 255); + let mut out = std::io::Cursor::new(Vec::new()); + let content_type = if format == F::Jpeg && !alpha { + let rgb = img.to_rgb8(); + image::codecs::jpeg::JpegEncoder::new_with_quality(&mut out, 85).encode_image(&rgb).ok()?; + "image/jpeg" + } else { + img.write_to(&mut out, F::Png).ok()?; + "image/png" + }; + let bytes = out.into_inner(); + // Never make a picture bigger just to resample it; deleting a crop is worth it regardless. + if bytes.len() >= data.len() && !cut { + return None; + } + Some(Packed { bytes, content_type, crop: new_crop }) +} + +/// Picture shapes in `shapes` (groups included) that `want` accepts. +fn pictures_mut<'a>(shapes: &'a mut [Shape], want: &dyn Fn(ShapeId) -> bool, out: &mut Vec<&'a mut Shape>, depth: usize) { + if depth > 64 { + return; + } + for sh in shapes { + if matches!(sh.kind, ShapeKind::Picture { .. }) && want(sh.id) { + out.push(sh); + } else if let Some(ch) = sh.children_mut() { + pictures_mut(ch, want, out, depth + 1); + } + } +} + +/// Media ids anything in the presentation still refers to (everything but the media list). +fn referenced_media(doc: &Presentation) -> HashSet { + fn scan(v: &Value, out: &mut HashSet, depth: usize) { + if depth > 256 { + return; + } + match v { + Value::Object(o) => { + for (k, x) in o { + match (k.as_str(), x.as_u64()) { + ("media" | "poster" | "preview" | "sound", Some(n)) => { + out.insert(n as u32); + } + _ => scan(x, out, depth + 1), + } + } + } + Value::Array(a) => a.iter().for_each(|x| scan(x, out, depth + 1)), + _ => {} + } + } + let mut out = HashSet::new(); + let mut shell = doc.clone(); + shell.media.clear(); + if let Ok(v) = serde_json::to_value(&shell) { + scan(&v, &mut out, 0); + } + // Embedded fonts are (name, id) pairs. + out.extend(doc.embedded_fonts.iter().map(|(_, m)| m.0)); + out +} + +fn compress(s: &mut Session, p: &Value) -> Result { + let ppi = f64_param(p, "ppi").unwrap_or(220.0); + if !ppi.is_finite() || !(36.0..=1200.0).contains(&ppi) { + return Err(bad("picture.compress", "`ppi` must be between 36 and 1200 (PowerPoint offers 330, 220, 150 and 96)")); + } + let delete_cropped = bool_or(p, "deleteCropped", true); + let all = bool_or(p, "all", false); + let ids: HashSet = if all { HashSet::new() } else { targets(s, p)?.into_iter().collect() }; + if !all && ids.is_empty() { + return Err(bad("picture.compress", "select a picture, or pass all: true")); + } + let current = s.doc()?.selection.slide; + s.edit(|doc, _| { + let want = |id: ShapeId| all || ids.contains(&id); + let mut before = 0usize; + let mut after = 0usize; + let mut count = 0usize; + let mut changed: Vec = vec![]; + // The same picture at the same size and crop compresses once. + let mut cache: HashMap<(u32, [u64; 4], u64, u64), Option<(MediaId, [f64; 4], usize)>> = HashMap::new(); + let range: Vec = if all { (0..doc.slides.len()).collect() } else { vec![current] }; + for i in range { + let Some(slide) = doc.slides.get(i).cloned() else { continue }; + let mut slide = slide.as_ref().clone(); + let mut pics = vec![]; + pictures_mut(&mut slide.shapes, &want, &mut pics, 0); + let mut touched = false; + for sh in pics { + let (w, h) = sh.xfrm.map(|x| (x.w, x.h)).unwrap_or((0.0, 0.0)); + let ShapeKind::Picture { fill } = &mut sh.kind else { continue }; + let PictureFill { media, crop, .. } = fill.clone(); + let key = (media.0, crop.map(f64::to_bits), w.to_bits(), h.to_bits()); + let hit = match cache.get(&key) { + Some(v) => *v, + None => { + let packed = doc.media(media).filter(|m| !m.data.is_empty()).and_then(|m| { + let old = m.data.len(); + pack(&m.data, crop, w, h, ppi, delete_cropped).map(|pk| (pk, old, m.name.clone())) + }); + let v = packed.map(|(pk, old, name)| { + before += old; + after += pk.bytes.len(); + let stem = std::path::Path::new(&name).file_stem().map(|x| x.to_string_lossy().to_string()).unwrap_or(name); + let ext = if pk.content_type == "image/jpeg" { "jpg" } else { "png" }; + let id = doc.add_media(&format!("{stem}.{ext}"), pk.content_type, pk.bytes); + (id, pk.crop, old) + }); + cache.insert(key, v); + v + } + }; + if let Some((id, new_crop, _)) = hit { + changed.push(media); + fill.media = id; + fill.crop = new_crop; + count += 1; + touched = true; + } + } + if touched && let Some(sl) = doc.slides.get_mut(i) { + *sl = Arc::new(slide); + } + } + // Drop the originals nothing uses any more, so the saved file actually shrinks. + if !changed.is_empty() { + let used = referenced_media(doc); + doc.media.retain(|m| !changed.contains(&m.id) || used.contains(&m.id.0)); + } + Ok(json!({"pictures": count, "before": before, "after": after, "ppi": ppi})) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn png(w: u32, h: u32) -> Vec { + let img = image::RgbImage::from_fn(w, h, |x, y| image::Rgb([(x * 7 % 256) as u8, (y * 5 % 256) as u8, ((x ^ y) % 256) as u8])); + let mut out = std::io::Cursor::new(Vec::new()); + image::DynamicImage::ImageRgb8(img).write_to(&mut out, image::ImageFormat::Png).unwrap(); + out.into_inner() + } + + #[test] + fn pack_downsamples_to_the_shown_size() { + // 2000 px shown at 1 inch: 150 ppi needs 150 px. + let pk = pack(&png(2000, 1000), [0.0; 4], 72.0, 36.0, 150.0, true).unwrap(); + let img = image::load_from_memory(&pk.bytes).unwrap(); + assert_eq!((img.width(), img.height()), (150, 75)); + assert_eq!(pk.content_type, "image/png"); + // Already small enough: nothing to do. + assert!(pack(&png(100, 50), [0.0; 4], 72.0, 36.0, 150.0, true).is_none()); + // Not a raster we re-encode. + assert!(pack(b"", [0.0; 4], 72.0, 72.0, 150.0, true).is_none()); + } + + #[test] + fn pack_deletes_cropped_areas_or_keeps_them() { + let src = png(400, 400); + // Left half cropped away, shown 2 in wide at 100 ppi → 200 px of the visible half. + let pk = pack(&src, [0.5, 0.0, 0.0, 0.0], 144.0, 288.0, 100.0, true).unwrap(); + assert_eq!(pk.crop, [0.0; 4]); + let img = image::load_from_memory(&pk.bytes).unwrap(); + assert_eq!((img.width(), img.height()), (200, 400)); + // Keeping the crop: the whole image stays, scaled so the visible half has its pixels. + let pk = pack(&src, [0.5, 0.0, 0.0, 0.0], 72.0, 72.0, 100.0, false).unwrap(); + assert_eq!(pk.crop, [0.5, 0.0, 0.0, 0.0]); + let img = image::load_from_memory(&pk.bytes).unwrap(); + assert_eq!(img.width(), 200); + } +} diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index ce909b02..b26aac01 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -2,6 +2,7 @@ pub mod animation; pub mod arrange; +pub mod compress; pub mod design; pub mod edit; pub mod file; @@ -142,6 +143,7 @@ pub fn command_specs() -> &'static [CommandSpec] { v.extend(review::specs()); v.extend(inspect::specs()); v.extend(media::specs()); + v.extend(compress::specs()); v }) } diff --git a/crates/pptx/tests/roundtrip.rs b/crates/pptx/tests/roundtrip.rs index db6d0beb..d4c8bdfa 100644 --- a/crates/pptx/tests/roundtrip.rs +++ b/crates/pptx/tests/roundtrip.rs @@ -991,3 +991,48 @@ fn transition_defaults_read_like_powerpoint() { assert_eq!((t.kind.as_str(), t.option.as_str(), t.duration_ms), (*kind, *option, 2000), "{el}"); } } + +/// Picture Format › Compress Pictures: a big picture shown small is downsampled, its original +/// goes when nothing else uses it, the saved file shrinks, a copy that wasn't compressed keeps +/// the original, and undo brings everything back. +#[test] +fn compress_pictures_shrinks_the_file_and_keeps_shared_originals() { + use deckcraft_engine::Session; + use serde_json::json; + let b64 = deckcraft_engine::cmd::base64_encode; + // A noisy 1600 × 1600 picture (doesn't compress away on its own). + let big = { + let img = image::RgbImage::from_fn(1600, 1600, |x, y| image::Rgb([(x * 31 % 251) as u8, (y * 17 % 253) as u8, ((x * y) % 255) as u8])); + let mut out = std::io::Cursor::new(Vec::new()); + image::DynamicImage::ImageRgb8(img).write_to(&mut out, image::ImageFormat::Png).unwrap(); + out.into_inner() + }; + let mut s = Session::with_new(); + s.execute("insert.picture", &json!({"data": b64(&big), "name": "noise.png"})).unwrap(); + s.execute("shape.resize", &json!({"w": 144, "h": 144})).unwrap(); + let size_before = deckcraft_pptx::export(&s.doc().unwrap().doc).unwrap().len(); + let r = s.execute("picture.compress", &json!({"ppi": 150})).unwrap(); + assert_eq!(r["pictures"], 1, "{r}"); + let doc = s.doc().unwrap().doc.clone(); + assert_eq!(doc.media.len(), 1, "the original is gone"); + let img = image::load_from_memory(&doc.media[0].data).unwrap(); + assert_eq!((img.width(), img.height()), (300, 300), "2 in at 150 ppi"); + let size_after = deckcraft_pptx::export(&doc).unwrap().len(); + assert!(size_after * 4 < size_before, "{size_before} → {size_after}"); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc.media[0].data.len(), big.len(), "undo restores the original"); + + // A duplicate that isn't compressed still needs the original. + s.execute("edit.duplicate", &json!({})).unwrap(); + let copy = s.doc().unwrap().selection.shapes.clone(); + let first: Vec<_> = s.doc().unwrap().doc.slides[0].shapes.iter().map(|sh| sh.id).filter(|id| !copy.contains(id)).collect(); + s.execute("picture.compress", &json!({"ppi": 150, "ids": [first.last().unwrap().0]})).unwrap(); + let doc = s.doc().unwrap().doc.clone(); + assert_eq!(doc.media.len(), 2); + assert!(doc.media.iter().any(|m| m.data.len() == big.len()), "the copy keeps the original"); + // All pictures: the copy gets compressed too, and the original goes. + s.execute("picture.compress", &json!({"ppi": 150, "all": true})).unwrap(); + let doc = s.doc().unwrap().doc.clone(); + assert!(doc.media.iter().all(|m| m.data.len() < big.len() / 4)); + assert!(s.execute("picture.compress", &json!({"ppi": 5, "all": true})).is_err()); +} diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 428eff0b..2adfa6d8 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -2087,6 +2087,10 @@ fn picture_format(app: &mut SlideApp, ui: &mut Ui) { rows(ui, |ui| { small(app, ui, Icon::ChangePicture, "Change Picture", "app.insertPictureDialog", json!({})); small(app, ui, Icon::ResetPicture, "Reset Picture", "picture.reset", json!({})); + let (_, r) = small_button_ex(ui, Icon::CompressPictures, "Compress Pictures", enabled(app, "picture.compress"), false, true); + if let Some(r) = r { + egui::Popup::menu(&r).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).width(260.0).show(|ui| compress_menu(app, ui)); + } }); }); group(ui, |ui| { @@ -2420,3 +2424,40 @@ fn paint_freeform_icon(p: &egui::Painter, r: Rect, name: &str, ink: Color32) { }; p.add(egui::Shape::line(pts, egui::Stroke::new(1.3, ink))); } + +/// Compress Pictures options, as in PowerPoint's dialog: resolution, apply to this picture or +/// all, delete cropped areas. The choices last for the session. +fn compress_menu(app: &mut SlideApp, ui: &mut Ui) { + let id = egui::Id::new("compress_pictures_opts"); + let (mut ppi, mut only_selected, mut delete_cropped): (u32, bool, bool) = ui.data_mut(|d| d.get_temp(id).unwrap_or((220, true, true))); + ui.label(egui::RichText::new("Compression options").strong()); + ui.checkbox(&mut only_selected, "Apply only to this picture"); + ui.checkbox(&mut delete_cropped, "Delete cropped areas of pictures"); + ui.separator(); + ui.label(egui::RichText::new("Resolution").strong()); + for (v, label) in [ + (330, "HD (330 ppi): good quality for high-definition displays"), + (220, "Print (220 ppi): excellent quality on most printers and screens"), + (150, "Web (150 ppi): good for web pages and projectors"), + (96, "E-mail (96 ppi): minimise document size for sharing"), + ] { + ui.radio_value(&mut ppi, v, label); + } + ui.data_mut(|d| d.insert_temp(id, (ppi, only_selected, delete_cropped))); + ui.separator(); + if ui.button("Compress").clicked() { + let p = json!({"ppi": ppi, "deleteCropped": delete_cropped, "all": !only_selected}); + match app.session.execute("picture.compress", &p) { + Ok(r) => { + let kb = |v: &Value| v.as_u64().unwrap_or(0) / 1024; + let msg = match r["pictures"].as_u64().unwrap_or(0) { + 0 => "Pictures are already at or below that resolution".to_string(), + n => format!("Compressed {n} picture(s): {} KB → {} KB", kb(&r["before"]), kb(&r["after"])), + }; + app.set_status(msg); + } + Err(e) => app.set_status(e.to_string()), + } + ui.close(); + } +} diff --git a/docs/gaps.md b/docs/gaps.md index a2894b42..e4b4b274 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -186,7 +186,7 @@ Detail in [animation-parity.md](animation-parity.md). 10–16 h; the pane is a * ### Smaller missing rows Zoom (summary/section/slide, 6–10 h), reuse slides (2–4 h), laser pointer (1–2 h), transition sounds (1–2 h), artistic effects (6–10 h), remove background (6–10 h; needs a model or a classical -matte), compress pictures (2–3 h), insert screenshot (2–3 h), record audio/screen and record slide +matte), insert screenshot (2–3 h), record audio/screen and record slide show (8–12 h), compare presentations (6–10 h), password protection (4–6 h, ECMA-376 agile encryption), ink to shape/text (6–10 h), icons and 3-D models (8–14 h, needs an open icon set). diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 2a3d450c..8595c0f1 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -186,7 +186,7 @@ Open P0 items: Contextual tabs (Shape Format, Picture Format, Table Design/Layou | Pictures | Remove background | P2 | M | | Pictures | Picture styles and borders | P1 | P | | Pictures | Change / reset picture | P1 | D | -| Pictures | Compress pictures | P2 | M | +| Pictures | Compress pictures | P2 | D | | Pictures | Paste image from clipboard (fails on Windows and Linux, #53) | P0 | P | | Pictures | Insert screenshot | P2 | M | | Media | Insert audio (any common format) | P0 | D | From f22ff9eb6238e6758b3e6820e4610279527e3037 Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Wed, 14 Oct 2026 05:20:38 +0300 Subject: [PATCH 08/16] Format after merging main --- crates/pptx/tests/chart_embed.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/pptx/tests/chart_embed.rs b/crates/pptx/tests/chart_embed.rs index ed5a8def..ddf56731 100644 --- a/crates/pptx/tests/chart_embed.rs +++ b/crates/pptx/tests/chart_embed.rs @@ -107,11 +107,7 @@ fn an_unchanged_chart_keeps_its_embedded_workbook() { .expect("chart relationships"); assert!(rels.contains("../embeddings/Microsoft_Excel_Worksheet1.xlsx"), "{rels}"); assert!(!rels.contains("slide1.xml"), "a chart relationship must not replace the slide: {rels}"); - let slide = saved_parts - .iter() - .find(|(n, _)| n == "ppt/slides/slide1.xml") - .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) - .expect("slide"); + let slide = saved_parts.iter().find(|(n, _)| n == "ppt/slides/slide1.xml").map(|(_, d)| String::from_utf8_lossy(d).into_owned()).expect("slide"); assert!(slide.contains("sld"), "{slide}"); assert!(!slide.as_bytes().windows(WORKBOOK.len()).any(|w| w == WORKBOOK)); let xml = saved_parts From f403c65c943610e5290c73a56fc3d9a581c13d29 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:30:16 -0400 Subject: [PATCH 09/16] Fix up #355: kept chart parts get fresh names; chartex keeps its workbook A chart's kept parts were written under the names the source file gave them, so a relationship to ppt/media/image1.png (or any name the writer generates) produced a second zip entry with that name on save. Kept parts are now written as ppt//keptN. with a content-type override each, the chart's and the kept parts' own relationships are rewritten to the new names (relationships to parts that were not kept are dropped), and the package writer skips any second entry under a name it already wrote. externalData/userShapes are dropped only when their relationship is not written back. Office 2016 chartex charts (added on main since the PR) capture and write back their kept parts the same way; the plain-chart fallback carries none. Co-Authored-By: Claude Opus 5.5 --- crates/model/src/chart.rs | 4 + crates/pptx/src/opc.rs | 61 ++++++---- crates/pptx/src/read/chart.rs | 4 +- crates/pptx/src/write/chart.rs | 135 +++++++++++++++++----- crates/pptx/src/write/mod.rs | 12 +- crates/pptx/src/write/shapes.rs | 4 + crates/pptx/src/xml.rs | 13 --- crates/pptx/tests/chart_embed.rs | 191 ++++++++++++++++++++++++++++++- docs/file-format-parity.md | 8 +- docs/gaps.md | 5 +- 10 files changed, 360 insertions(+), 77 deletions(-) diff --git a/crates/model/src/chart.rs b/crates/model/src/chart.rs index fbca876f..36dd9708 100644 --- a/crates/model/src/chart.rs +++ b/crates/model/src/chart.rs @@ -404,6 +404,10 @@ pub struct ChartRel { pub id: String, pub rel_type: String, pub target: String, + /// Absolute name of the part an internal relationship reaches (matches a `ChartPart::name` + /// up to case); empty for external ones. The writer renames kept parts and uses this to + /// point the relationship at the new name. + pub part: String, pub external: bool, } diff --git a/crates/pptx/src/opc.rs b/crates/pptx/src/opc.rs index 0e7c5e64..0e7f86fe 100644 --- a/crates/pptx/src/opc.rs +++ b/crates/pptx/src/opc.rs @@ -127,23 +127,7 @@ impl Package { pub fn rels(&self, part: &str) -> Rels { let rp = rels_path(part); let Some(doc) = self.xml(&rp) else { return Rels::default() }; - let base = dir_of(part); - let mut out = Rels::default(); - for r in doc.root.children_named("Relationship") { - 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_raw = target.to_string(); - let target = if external { target_raw.clone() } else { resolve(&base, target) }; - out.items.push(Rel { - id: id.to_string(), - kind: ty.rsplit('/').next().unwrap_or(ty).to_string(), - ty: ty.to_string(), - target_raw, - target, - external, - }); - } - out + rels_of(&doc, part) } /// Content types: (extension defaults, part overrides). pub fn content_type(&self, part: &str) -> Option { @@ -165,6 +149,38 @@ impl Package { } } +/// The relationships in a parsed rels part, with internal targets resolved against `part`'s folder. +fn rels_of(doc: &xml::Doc, part: &str) -> Rels { + let base = dir_of(part); + let mut out = Rels::default(); + for r in doc.root.children_named("Relationship") { + 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_raw = target.to_string(); + let target = if external { target_raw.clone() } else { resolve(&base, target) }; + out.items.push(Rel { + id: id.to_string(), + kind: ty.rsplit('/').next().unwrap_or(ty).to_string(), + ty: ty.to_string(), + target_raw, + target, + external, + }); + } + out +} + +/// The relationships of `part` from the bytes of its rels part (kept parts carried by a chart). +pub fn rels_from_bytes(bytes: &[u8], part: &str) -> Rels { + match xml::parse(bytes) { + Ok(doc) => rels_of(&doc, part), + Err(e) => { + log::warn!("pptx: relationships of {part}: {e}"); + Rels::default() + } + } +} + #[derive(Clone, Debug, Default)] pub struct Rel { pub id: String, @@ -377,10 +393,6 @@ impl PackageOut { pub fn raw(&mut self, name: &str, data: Vec) { self.files.push((name.to_string(), data, true)); } - pub fn part_data(&self, name: &str) -> Option<&[u8]> { - let n = norm(name); - self.files.iter().find(|(f, _, _)| norm(f) == n).map(|(_, d, _)| d.as_slice()) - } /// Add a binary part; its extension gets a Default content type. pub fn binary(&mut self, name: &str, content_type: &str, data: Vec) { let ext = name.rsplit('.').next().unwrap_or("bin").to_ascii_lowercase(); @@ -407,7 +419,14 @@ impl PackageOut { let err = |e: &dyn std::fmt::Display| PptxError::Write(e.to_string()); zw.start_file("[Content_Types].xml", deflate).map_err(|e| err(&e))?; zw.write_all(&ct.finish()).map_err(|e| err(&e))?; + let mut written = std::collections::HashSet::new(); for (name, data, compress) in &self.files { + // Zip entry names must be unique: a second part under a name already written is + // dropped rather than producing a package readers reject or resolve differently. + if !written.insert(norm(name)) { + log::warn!("pptx: part {name} written twice; keeping the first"); + continue; + } let large = data.len() as u64 >= u32::MAX as u64 / 2; let opts = if *compress { deflate } else { stored }.large_file(large); zw.start_file(name.as_str(), opts).map_err(|e| err(&e))?; diff --git a/crates/pptx/src/read/chart.rs b/crates/pptx/src/read/chart.rs index bda93537..9fef9984 100644 --- a/crates/pptx/src/read/chart.rs +++ b/crates/pptx/src/read/chart.rs @@ -421,11 +421,11 @@ pub(crate) fn capture_kept(pkg: &crate::opc::Package, chart_part: &str) -> deckc continue; } if r.external { - kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: true }); + kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, part: String::new(), external: true }); continue; } if copy_part(pkg, &r.target, &mut kept.parts, &mut seen, 0) { - kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: false }); + kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, part: r.target, external: false }); } } kept diff --git a/crates/pptx/src/write/chart.rs b/crates/pptx/src/write/chart.rs index 38b16099..88917450 100644 --- a/crates/pptx/src/write/chart.rs +++ b/crates/pptx/src/write/chart.rs @@ -512,10 +512,15 @@ fn plot_group(w: &mut W, c: &Chart, kind: ChartType, list: &[(usize, &Series)]) } /// The `chartEx` part of an Office 2016 chart (waterfall, funnel…): the kept part when the chart -/// is unchanged since it was read (without its workbook link), else one written from the model. -/// `None` for other chart types. +/// is unchanged since it was read, else one written from the model. `None` for other chart types. +#[cfg(test)] pub fn chartex_xml(c: &Chart) -> Option> { - kept_chartex(c).or_else(|| c.kind.is_chartex().then(|| build_chartex(c))) + chartex_part(c).map(|(x, _)| x) +} + +/// [`chartex_xml`], and whether it is the original part (so its kept relationships still apply). +pub fn chartex_part(c: &Chart) -> Option<(Vec, bool)> { + kept_chartex(c).map(|x| (x, true)).or_else(|| c.kind.is_chartex().then(|| (build_chartex(c), false))) } fn kept_chartex(c: &Chart) -> Option> { @@ -525,11 +530,14 @@ fn kept_chartex(c: &Chart) -> Option> { crate::read::resolve_mc(&mut root, 0); let mut back = crate::read::chartex_from(&root)?; back.raw = c.raw.clone(); + // The sidecar is not modelled chart state. + back.kept = c.kept.clone(); if &back != c { return None; } let mut keep = doc.root; - keep.remove_all(&["externalData"]); + // The workbook link stays only when its part is written back. + drop_unkept_refs(&mut keep, c); let mut w = W::new(); w.raw(&keep.to_xml()); Some(w.finish()) @@ -553,9 +561,7 @@ pub fn chart_xml(c: &Chart) -> (Vec, bool) { if back == me { let mut keep = doc.root; // Without the parts those elements point at, PowerPoint repairs the file. - if c.kept.rels.is_empty() { - keep.remove_all(&["externalData", "userShapes"]); - } + drop_unkept_refs(&mut keep, c); let mut w = W::new(); w.raw(&keep.to_xml()); return (w.finish(), true); @@ -660,31 +666,106 @@ pub fn chart_xml(c: &Chart) -> (Vec, bool) { (w.finish(), false) } -/// Write the chart's own relationships and the parts they reach. -pub fn write_kept(pkg: &mut crate::opc::PackageOut, chart_name: &str, c: &Chart) { - if c.kept.rels.is_empty() && c.kept.parts.is_empty() { +/// The relationship ids of an unchanged chart that [`write_kept`] writes back: external ones, and +/// internal ones whose part was kept. +fn kept_rel_ids(c: &Chart) -> std::collections::HashSet<&str> { + let parts: std::collections::HashSet = c.kept.parts.iter().filter(|p| kept_payload(&p.name)).map(|p| crate::opc::norm(&p.name)).collect(); + c.kept.rels.iter().filter(|r| r.external || parts.contains(&crate::opc::norm(&r.part))).map(|r| r.id.as_str()).collect() +} + +/// A kept part that is written as a part of its own (rels parts are rewritten instead). +fn kept_payload(name: &str) -> bool { + !name.to_ascii_lowercase().ends_with(".rels") && crate::opc::sidecar_part(name) +} + +/// Drop `c:externalData` / `c:userShapes` (and the `cx:` ones) that point at a relationship that +/// is not written back: without the part PowerPoint repairs the file. +fn drop_unkept_refs(root: &mut crate::xml::El, c: &Chart) { + let ids = kept_rel_ids(c); + fn rec(e: &mut crate::xml::El, ids: &std::collections::HashSet<&str>, depth: usize) { + if depth > crate::xml::MAX_DEPTH { + return; + } + e.children.retain(|n| match n { + crate::xml::Node::El(x) if x.is("externalData") || x.is("userShapes") => x.attr("r:id").is_some_and(|id| ids.contains(id)), + _ => true, + }); + for ch in e.elements_mut() { + rec(ch, ids, depth + 1); + } + } + rec(root, &ids, 0); +} + +/// A fresh name for a kept part: `ppt//keptN.`. The writer generates no `kept…` +/// names and `n` is unique within the package, so a kept part never lands on (or duplicates) a +/// part the writer makes, whatever the source file called it. +fn kept_name(original: &str, n: u32) -> String { + let norm = crate::opc::norm(original); + let area = norm.split('/').nth(1).unwrap_or("embeddings"); + let area = if matches!(area, "embeddings" | "charts" | "drawings" | "media") { area } else { "embeddings" }; + let file = crate::opc::file_of(&norm); + let ext = file + .rsplit_once('.') + .map(|(_, e)| e) + .filter(|e| !e.is_empty() && e.len() <= 8 && e.chars().all(|c| c.is_ascii_alphanumeric())) + .unwrap_or("bin"); + format!("ppt/{area}/kept{n}.{ext}") +} + +/// Write an unchanged chart's own relationships and the parts they reach. Each kept part is +/// written under a fresh name ([`kept_name`]) and the relationships between kept parts are +/// rewritten to those names; a relationship to anything that was not kept is dropped. +/// `n_kept` numbers kept parts across the package. +pub fn write_kept(pkg: &mut crate::opc::PackageOut, chart_name: &str, c: &Chart, n_kept: &mut u32) { + use std::collections::HashMap; + + use crate::opc::{RelsOut, norm, relative, rels_path}; + + if c.kept.rels.is_empty() { return; } - let mut rels = crate::opc::RelsOut::default(); - for r in &c.kept.rels { - rels.items.push((r.id.clone(), r.rel_type.clone(), r.target.clone(), r.external)); - } - pkg.rels(chart_name, &rels); - for p in &c.kept.parts { - match pkg.part_data(&p.name) { - Some(existing) if existing != p.bytes.as_slice() => { - log::warn!("pptx: chart part {} already written with different bytes; keeping the first", p.name); - continue; - } - Some(_) => continue, - None => {} - } - if !crate::opc::sidecar_part(&p.name) { - log::warn!("pptx: not writing chart part {}", p.name); + // Original part name (normalized) → the name written. + let mut names: HashMap = HashMap::new(); + for p in c.kept.parts.iter().filter(|p| kept_payload(&p.name)) { + let key = norm(&p.name); + if names.contains_key(&key) { continue; } + *n_kept += 1; + let new = kept_name(&p.name, *n_kept); let ct = if !p.content_type.is_empty() { p.content_type.clone() } else { crate::mime_for(&p.name).to_string() }; - pkg.binary(&p.name, &ct, p.bytes.clone()); + // An override per part: a kept part never sets the package-wide default for its extension. + pkg.xml_part(&new, &ct, p.bytes.clone()); + names.insert(key, new); + } + let mut rels = RelsOut::default(); + for r in &c.kept.rels { + if r.external { + rels.items.push((r.id.clone(), r.rel_type.clone(), r.target.clone(), true)); + } else if let Some(new) = names.get(&norm(&r.part)) { + rels.items.push((r.id.clone(), r.rel_type.clone(), relative(chart_name, new), false)); + } else { + log::warn!("pptx: chart relationship {} has no kept part; dropped", r.id); + } + } + pkg.rels(chart_name, &rels); + // The kept parts' own relationships, pointed at the new names. + for p in c.kept.parts.iter().filter(|p| kept_payload(&p.name)) { + let Some(new_source) = names.get(&norm(&p.name)) else { continue }; + let rp = norm(&rels_path(&p.name)); + let Some(rels_part) = c.kept.parts.iter().find(|q| norm(&q.name) == rp) else { continue }; + let mut out = RelsOut::default(); + for r in crate::opc::rels_from_bytes(&rels_part.bytes, &p.name).items { + if r.external { + out.items.push((r.id, r.ty, r.target_raw, true)); + } else if let Some(new) = names.get(&norm(&r.target)) { + out.items.push((r.id, r.ty, relative(new_source, new), false)); + } else { + log::warn!("pptx: relationship {} of kept chart part {} has no kept target; dropped", r.id, p.name); + } + } + pkg.rels(new_source, &out); } } diff --git a/crates/pptx/src/write/mod.rs b/crates/pptx/src/write/mod.rs index 6e895a3d..dd222d3b 100644 --- a/crates/pptx/src/write/mod.rs +++ b/crates/pptx/src/write/mod.rs @@ -59,6 +59,8 @@ pub struct Exp<'a> { n_diagram: u32, n_tags: u32, n_chartex: u32, + /// Parts kept from an unchanged chart (`ppt/…/keptN.ext`). + n_kept: u32, pub slide_parts: HashMap, pub table_styles: Vec, placeholder: Option, @@ -195,10 +197,15 @@ impl Exp<'_> { } /// An unchanged Office 2016 chart's `chartEx` part and its relationship id. pub fn chartex_rel(&mut self, o: &mut Out, c: &deckcraft_model::Chart) -> Option { - let xml = chart::chartex_xml(c)?; + let (xml, preserved) = chart::chartex_part(c)?; self.n_chartex += 1; let name = format!("ppt/charts/chartEx{}.xml", self.n_chartex); self.pkg.xml_part(&name, CT_CHARTEX, xml); + if preserved { + chart::write_kept(&mut self.pkg, &name, c, &mut self.n_kept); + } else if !c.kept.parts.is_empty() { + log::warn!("pptx: chart data changed; the embedded workbook was not written back"); + } Some(o.rels.add_new(RT_CHARTEX, &opc::relative(&o.name, &name), false)) } @@ -208,7 +215,7 @@ impl Exp<'_> { let (xml, preserved) = chart::chart_xml(c); self.pkg.xml_part(&name, CT_CHART, xml); if preserved { - chart::write_kept(&mut self.pkg, &name, c); + chart::write_kept(&mut self.pkg, &name, c, &mut self.n_kept); } else if !c.kept.parts.is_empty() { log::warn!("pptx: chart data changed; the embedded workbook was not written back"); } @@ -528,6 +535,7 @@ pub fn export(p: &Presentation, main_ct: &str) -> Result, PptxError> { n_diagram: 0, n_tags: 0, n_chartex: 0, + n_kept: 0, slide_parts: HashMap::new(), table_styles: vec![], placeholder: None, diff --git a/crates/pptx/src/write/shapes.rs b/crates/pptx/src/write/shapes.rs index 770ee775..aff238b7 100644 --- a/crates/pptx/src/write/shapes.rs +++ b/crates/pptx/src/write/shapes.rs @@ -401,6 +401,10 @@ pub fn shape(w: &mut W, x: &mut Exp, o: &mut Out, cx: &mut Ctx, s: &Shape, depth } let mut plain = (**c).clone(); plain.raw = plain.raw.filter(|r| !r.contains(super::chart::NS_CX)); + if ex.is_some() { + // The kept workbook belongs to the `chartEx` part; the fallback has none. + plain.kept = Default::default(); + } let rid = x.chart_rel(o, &plain); frame_open(w, x, o, id, s); w.open("a:graphicData", A::new().a("uri", "http://schemas.openxmlformats.org/drawingml/2006/chart")); diff --git a/crates/pptx/src/xml.rs b/crates/pptx/src/xml.rs index eceb0708..9484863a 100644 --- a/crates/pptx/src/xml.rs +++ b/crates/pptx/src/xml.rs @@ -171,19 +171,6 @@ impl El { write_el(self, &mut s, 0); s } - /// Remove descendant elements with any of these local names (bounded). - pub fn remove_all(&mut self, locals: &[&str]) { - fn rec(e: &mut El, locals: &[&str], depth: usize) { - if depth > MAX_DEPTH { - return; - } - e.children.retain(|c| !matches!(c, Node::El(x) if locals.contains(&x.local()))); - for c in e.elements_mut() { - rec(c, locals, depth + 1); - } - } - rec(self, locals, 0) - } } fn write_el(e: &El, s: &mut String, depth: usize) { diff --git a/crates/pptx/tests/chart_embed.rs b/crates/pptx/tests/chart_embed.rs index ddf56731..d80d4d6d 100644 --- a/crates/pptx/tests/chart_embed.rs +++ b/crates/pptx/tests/chart_embed.rs @@ -98,14 +98,15 @@ fn an_unchanged_chart_keeps_its_embedded_workbook() { let saved = deckcraft_pptx::export(&deckcraft_pptx::import(&src).expect("import")).expect("export"); let saved_parts = parts(&saved); - let workbook = saved_parts.iter().find(|(n, _)| n == EMBED).map(|(_, d)| d.as_slice()); + // Kept parts are written under fresh names, so they can't land on a part the writer makes. + let workbook = saved_parts.iter().find(|(n, _)| n == "ppt/embeddings/kept1.xlsx").map(|(_, d)| d.as_slice()); assert_eq!(workbook, Some(WORKBOOK), "the embedding part is dropped on save"); let rels = saved_parts .iter() .find(|(n, _)| n.starts_with("ppt/charts/_rels/chart") && n.ends_with(".xml.rels")) .map(|(_, d)| String::from_utf8_lossy(d).into_owned()) .expect("chart relationships"); - assert!(rels.contains("../embeddings/Microsoft_Excel_Worksheet1.xlsx"), "{rels}"); + assert!(rels.contains(r#"Id="rId1""#) && rels.contains("../embeddings/kept1.xlsx"), "{rels}"); assert!(!rels.contains("slide1.xml"), "a chart relationship must not replace the slide: {rels}"); let slide = saved_parts.iter().find(|(n, _)| n == "ppt/slides/slide1.xml").map(|(_, d)| String::from_utf8_lossy(d).into_owned()).expect("slide"); assert!(slide.contains("sld"), "{slide}"); @@ -131,7 +132,7 @@ fn an_edited_chart_does_not_write_a_stale_workbook() { let value = chart.series.get_mut(0).and_then(|s| s.values.get_mut(0)).expect("value"); *value = Some(99.0); let saved = parts(&deckcraft_pptx::export(&p).expect("export")); - assert!(saved.iter().all(|(n, _)| n != EMBED), "a stale workbook was written after the chart changed"); + assert!(saved.iter().all(|(n, _)| !n.starts_with("ppt/embeddings/")), "a stale workbook was written after the chart changed"); let xml = saved .iter() .find(|(n, _)| n.starts_with("ppt/charts/chart") && n.ends_with(".xml")) @@ -139,3 +140,187 @@ fn an_edited_chart_does_not_write_a_stale_workbook() { .expect("chart xml"); assert!(!xml.contains("externalData"), "{xml}"); } + +fn png(rgb: [u8; 3]) -> Vec { + let img = image::RgbImage::from_pixel(4, 4, image::Rgb(rgb)); + let mut out = Cursor::new(vec![]); + image::DynamicImage::ImageRgb8(img).write_to(&mut out, image::ImageFormat::Png).expect("png"); + out.into_inner() +} + +/// Rewrite `src`: `edit` may change any entry (name, bytes → new bytes), then `extra` entries are added. +fn rewrite(src: &[u8], edit: impl Fn(&str, Vec) -> Vec, extra: &[(&str, &[u8])]) -> Vec { + let mut zip = zip::ZipArchive::new(Cursor::new(src)).expect("zip"); + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default(); + for i in 0..zip.len() { + let mut f = zip.by_index(i).expect("entry"); + let name = f.name().to_string(); + let mut data = vec![]; + f.read_to_end(&mut data).expect("read"); + let data = edit(&name, data); + out.start_file(&name, opts).expect("start"); + out.write_all(&data).expect("write"); + } + for (name, data) in extra { + out.start_file(*name, opts).expect("start"); + out.write_all(data).expect("write"); + } + out.finish().expect("finish").into_inner() +} + +fn unique_names(parts: &[(String, Vec)]) { + let mut seen = std::collections::HashSet::new(); + for (n, _) in parts { + assert!(seen.insert(n.to_ascii_lowercase()), "duplicate zip entry {n}"); + } +} + +fn text_of(parts: &[(String, Vec)], name: &str) -> String { + parts.iter().find(|(n, _)| n == name).map(|(_, d)| String::from_utf8_lossy(d).into_owned()).unwrap_or_else(|| panic!("no {name}")) +} + +const RT: &str = "http://schemas.openxmlformats.org/officeDocument/2006/relationships"; + +/// A chart whose relationships reach parts named like the ones the writer generates +/// (`ppt/media/image1.png`, `ppt/drawings/…`) must not collide with them on save: no duplicate +/// zip entries, and the slide's own picture keeps its bytes. +#[test] +fn kept_chart_parts_never_collide_with_written_parts() { + let picture = png([10, 200, 30]); + let mut p = defaults::new_presentation(None); + p.slides.clear(); + let img = p.add_media("image1.png", "image/png", picture.clone()); + let layout = defaults::layout_of_kind(&p, LayoutType::Blank).unwrap_or_default(); + let mut slide = defaults::new_slide(&mut p, layout); + // The chart comes first, so its kept parts are written before the picture. + let mut chart = Shape { id: ShapeId(p.alloc_id()), name: "Pie".into(), xfrm: Some(Xfrm::new(40.0, 40.0, 400.0, 300.0)), ..Default::default() }; + chart.kind = ShapeKind::Chart(Box::new(Chart::sample(ChartType::Pie))); + slide.shapes.push(chart); + let mut pic = Shape { id: ShapeId(p.alloc_id()), name: "Pic".into(), xfrm: Some(Xfrm::new(500.0, 40.0, 80.0, 80.0)), ..Default::default() }; + pic.kind = ShapeKind::Picture { fill: deckcraft_model::style::PictureFill { media: img, ..Default::default() } }; + slide.shapes.push(pic); + p.slides.push(Arc::new(slide)); + let base = deckcraft_pptx::export(&p).expect("export"); + assert!(parts(&base).iter().any(|(n, d)| n == "ppt/media/image1.png" && *d == picture)); + + // The chart's own relationships reach the slide picture's part and a drawing whose + // relationships reach it too (and, hostile, the slide). + let chart_rels = format!( + r#""# + ); + let drawing_rels = format!( + r#""# + ); + let drawing = br#""#; + let src = rewrite( + &base, + |name, data| { + if name.starts_with("ppt/charts/chart") && name.ends_with(".xml") { + String::from_utf8(data) + .expect("chart") + .replacen("", r#""#, 1) + .into_bytes() + } else { + data + } + }, + &[ + ("ppt/charts/_rels/chart1.xml.rels", chart_rels.as_bytes()), + ("ppt/drawings/drawing1.xml", drawing), + ("ppt/drawings/_rels/drawing1.xml.rels", drawing_rels.as_bytes()), + (EMBED, WORKBOOK), + ], + ); + + let saved = parts(&deckcraft_pptx::export(&deckcraft_pptx::import(&src).expect("import")).expect("export")); + unique_names(&saved); + let picture_parts: Vec<&String> = saved.iter().filter(|(_, d)| *d == picture).map(|(n, _)| n).collect(); + assert!(picture_parts.iter().any(|n| *n == "ppt/media/image1.png"), "{picture_parts:?}"); + let slide_rels = text_of(&saved, "ppt/slides/_rels/slide1.xml.rels"); + assert!(slide_rels.contains("../media/image1.png"), "{slide_rels}"); + + // The chart's relationships point at the renamed parts, which hold the kept bytes. + let rels = text_of(&saved, "ppt/charts/_rels/chart1.xml.rels"); + assert!(!rels.contains("image1.png") && !rels.contains("drawing1.xml") && !rels.contains("Worksheet1"), "{rels}"); + let target = |rels: &str, id: &str| -> String { + let at = rels.find(&format!(r#"Id="{id}""#)).unwrap_or_else(|| panic!("{id} in {rels}")); + let rest = &rels[at..]; + let t = rest.find(r#"Target=""#).expect("target") + 8; + rest[t..].split('"').next().unwrap_or("").to_string() + }; + let resolve = |from_dir: &str, t: &str| format!("ppt/{}", t.trim_start_matches("../").trim_start_matches(from_dir)); + let img_part = resolve("", &target(&rels, "rId3")); + assert!(img_part.starts_with("ppt/media/kept"), "{img_part}"); + assert!(saved.iter().any(|(n, d)| *n == img_part && *d == picture), "{img_part}"); + let drawing_part = resolve("", &target(&rels, "rId2")); + assert!(saved.iter().any(|(n, d)| *n == drawing_part && d.as_slice() == drawing.as_slice()), "{drawing_part}"); + let wb = resolve("", &target(&rels, "rId1")); + assert!(saved.iter().any(|(n, d)| *n == wb && d.as_slice() == WORKBOOK), "{wb}"); + + // The drawing's own relationships follow the rename; the one to the slide is dropped. + let file = drawing_part.rsplit('/').next().unwrap_or(""); + let drels = text_of(&saved, &format!("ppt/drawings/_rels/{file}.rels")); + assert_eq!(resolve("", &target(&drels, "rId1")), img_part, "{drels}"); + assert!(!drels.contains("slide"), "{drels}"); + + // And the result reads back with the same kept parts. + let again = deckcraft_pptx::export(&deckcraft_pptx::import(&saved_zip(&saved)).expect("reimport")).expect("export again"); + unique_names(&parts(&again)); +} + +fn saved_zip(parts: &[(String, Vec)]) -> Vec { + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default(); + for (n, d) in parts { + out.start_file(n.as_str(), opts).expect("start"); + out.write_all(d).expect("write"); + } + out.finish().expect("finish").into_inner() +} + +/// An unchanged Office 2016 chart keeps its embedded workbook too; its plain-chart fallback +/// carries none. +#[test] +fn an_unchanged_chartex_keeps_its_embedded_workbook() { + let mut p = defaults::new_presentation(None); + p.slides.clear(); + let layout = defaults::layout_of_kind(&p, LayoutType::Blank).unwrap_or_default(); + let mut slide = defaults::new_slide(&mut p, layout); + let mut shape = Shape { id: ShapeId(p.alloc_id()), name: "Pie".into(), xfrm: Some(Xfrm::new(40.0, 40.0, 400.0, 300.0)), ..Default::default() }; + shape.kind = ShapeKind::Chart(Box::new(Chart::sample(ChartType::Waterfall))); + slide.shapes.push(shape); + p.slides.push(Arc::new(slide)); + let base = deckcraft_pptx::export(&p).expect("export"); + assert!(parts(&base).iter().any(|(n, _)| n == "ppt/charts/chartEx1.xml")); + let rels = format!( + r#""# + ); + let src = rewrite( + &base, + |name, data| { + if name == "ppt/charts/chartEx1.xml" { + String::from_utf8(data) + .expect("chartex") + .replacen("", r#""#, 1) + .into_bytes() + } else { + data + } + }, + &[("ppt/charts/_rels/chartEx1.xml.rels", rels.as_bytes()), (EMBED, WORKBOOK)], + ); + let chart = chart_of(&src); + assert_eq!(chart.kind, ChartType::Waterfall); + assert!(chart.kept.parts.iter().any(|p| p.bytes == WORKBOOK)); + + let saved = parts(&deckcraft_pptx::export(&deckcraft_pptx::import(&src).expect("import")).expect("export")); + unique_names(&saved); + let xml = text_of(&saved, "ppt/charts/chartEx1.xml"); + assert!(xml.contains("externalData"), "{xml}"); + let rels = text_of(&saved, "ppt/charts/_rels/chartEx1.xml.rels"); + assert!(rels.contains("../embeddings/kept1.xlsx"), "{rels}"); + assert!(saved.iter().any(|(n, d)| n == "ppt/embeddings/kept1.xlsx" && d.as_slice() == WORKBOOK)); + assert!(saved.iter().all(|(n, _)| n != "ppt/charts/_rels/chart1.xml.rels"), "the fallback chart has no workbook"); + assert!(!text_of(&saved, "ppt/charts/chart1.xml").contains("externalData")); +} diff --git a/docs/file-format-parity.md b/docs/file-format-parity.md index 1e6e103d..163a3dfa 100644 --- a/docs/file-format-parity.md +++ b/docs/file-format-parity.md @@ -95,16 +95,12 @@ the round trip even though DeckCraft doesn't model it. | Text warp (`prstTxWarp`) | yes | yes | All 40 presets rendered (`crates/geom/src/warp.rs`); adjust values (`avLst`) round-trip | | Pictures with crop, recolour, duotone, alpha, luminance | yes | yes | | | Tables with styles | yes | yes | Merges and both diagonal borders round-trip | -| Charts (`c:chartSpace`) | yes | yes | Original XML kept in `Chart.raw`. Trendlines (type, order or period, forecast, intercept, equation and R²) are read, written and drawn. A combo chart's secondary value axis is read and is not written back when the chart is rebuilt | -| Charts (`cx:` chartex: waterfall, funnel, treemap, sunburst, histogram, Pareto, box) | yes | yes | Unchanged parts kept; edited and new ones written from the model with a plain-chart fallback. Treemap and sunburst hierarchy levels kept; region maps kept, drawn as tiles (no map geometry) | +| Charts (`c:chartSpace`) | yes | yes | Original XML kept in `Chart.raw`. Trendlines (type, order or period, forecast, intercept, equation and R²) are read, written and drawn. A combo chart's secondary value axis is read and is not written back when the chart is rebuilt. An unchanged chart also keeps its embedded workbook and the other parts its relationships point at, under fresh `keptN` names so they never replace a part DeckCraft writes (#335); editing the chart drops that workbook instead of writing a stale one | +| Charts (`cx:` chartex: waterfall, funnel, treemap, sunburst, histogram, Pareto, box) | yes | yes | Unchanged parts kept, with their embedded workbook (#335); edited and new ones written from the model with a plain-chart fallback. Treemap and sunburst hierarchy levels kept; region maps kept, drawn as tiles (no map geometry) | | SmartArt (`dgm`) | drawing part only, as a group; text laid out on its `dsp:txXfrm` rectangle | as shapes | Diagram data model lost on save | | SmartArt (`dgm`) | drawing part only, as a group | as shapes | Diagram data model lost on save. A diagram with no relationship parts keeps its XML; the drawing and data parts are still dropped (#203) | | SmartArt (`dgm`) | drawing part only, as a group | as shapes | Diagram data model lost on save (DeckCraft keeps its own outline in `.deckcraft` only) | | SmartArt (`dgm`) | the drawing part as a group, and the outline from the data model (Text Pane) | as a diagram: data model from the outline; layout, quick style and colour parts (the file's own kept until the layout is switched); the group as the cached drawing | DeckCraft's layout definitions are simple; a diagram without a cached drawing isn't drawn | -| Tables with styles | yes | yes | | -| Charts (`c:chartSpace`) | yes | yes | Original XML kept in `Chart.raw`. An unchanged chart also keeps its embedded workbook and the other parts its relationships point at (#335); editing the chart drops that workbook instead of writing a stale one | -| Charts (`cx:` chartex: waterfall, funnel, treemap, sunburst, histogram, box) | no | no | | -| SmartArt (`dgm`) | drawing part only, as a group | as shapes | Diagram data model lost on save | | Equations (OMML, `a14:m`) | no | no | PR #55 open | | Audio / video (`p14:media`, trim, fades, bookmarks) | yes | partly | Playback options lost on save (#74) | | Ink (`contentPart`) | yes | as a group of freeform lines | PowerPoint sees shapes, not ink | diff --git a/docs/gaps.md b/docs/gaps.md index 0d161449..e7454e85 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -244,9 +244,8 @@ Detail in [file-format-parity.md](file-format-parity.md). moving average, with forecast and the equation and R² label. A moving average is drawn with neither label. A combo chart's secondary value axis is read (`y2_*`, `Series.secondary_axis`) and is not written back when the chart is rebuilt. - chart types" (partial); `chartex` parts (waterfall, funnel, treemap, sunburst, histogram, box & - whisker) aren't read. An unchanged chart's embedded workbook now survives save (#335); editing - the chart still drops it rather than writing a stale spreadsheet. + An unchanged chart's embedded workbook now survives save (#335); editing the chart still + drops it rather than writing a stale spreadsheet. - **Impact:** most business decks have charts; editing their data is routine. - **Estimate:** 12–20 h. Data and elements are **beta blockers** (6–10 h). From 5ef510e57bb307ef34ce866f98bd8902d17bf2b4 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:35:50 -0400 Subject: [PATCH 10/16] Fix up #509: copy media by a typed walk of every reference slide.reuse indexed src.slides[i] and found media by scanning JSON keys (media/poster/preview/sound), which missed artistic-effect originals and would miss any reference stored under another key. New engine module media_refs: one typed walker over the model (written once, expanded for & and &mut) that visits every MediaId a presentation, master, layout, slide or shape can hold: pictures and their artistic originals, picture fills in shapes, lines, runs, table cells and styles, chart series and points, backgrounds and theme format schemes; media clips and posters, 3-D models, OLE/SmartArt previews and node pictures, picture bullets, hyperlink, transition and animation sounds, embedded fonts. slide.reuse now lists and remaps media through it, uses get() for slides, and keeps a linked media item's link when copying it. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/reuse.rs | 167 ++++++++------ crates/engine/src/lib.rs | 1 + crates/engine/src/media_refs.rs | 390 ++++++++++++++++++++++++++++++++ docs/gaps.md | 5 +- 4 files changed, 494 insertions(+), 69 deletions(-) create mode 100644 crates/engine/src/media_refs.rs diff --git a/crates/engine/src/cmd/reuse.rs b/crates/engine/src/cmd/reuse.rs index d5e31b11..1025b01e 100644 --- a/crates/engine/src/cmd/reuse.rs +++ b/crates/engine/src/cmd/reuse.rs @@ -12,7 +12,7 @@ use deckcraft_model::{LayoutId, MasterId, MediaId, Presentation, ShapeId, ShapeK use serde_json::{Value, json}; use super::*; -use crate::{Result, Session}; +use crate::{Result, Session, media_refs}; pub fn specs() -> Vec { vec![cmd!( @@ -26,40 +26,15 @@ pub fn specs() -> Vec { )] } -/// Keys under which the model stores a [`MediaId`] (pictures, media, posters, previews, sounds). -const MEDIA_KEYS: [&str; 4] = ["media", "poster", "preview", "sound"]; - -/// Rewrite every media id in `v` through `map`. -fn remap_media(v: &mut Value, map: &HashMap, depth: usize) { - if depth > 256 { - return; - } - match v { - Value::Object(o) => { - for (k, x) in o.iter_mut() { - if MEDIA_KEYS.contains(&k.as_str()) - && let Some(n) = x.as_u64().and_then(|n| u32::try_from(n).ok()) - { - if let Some(to) = map.get(&n) { - *x = json!(to); - } - continue; - } - remap_media(x, map, depth + 1); - } +/// Point every media id under `x` at its copy in the destination (`map`: source → destination). +fn remap(map: &HashMap) -> impl FnMut(&mut MediaId) + '_ { + move |id: &mut MediaId| { + if let Some(to) = map.get(id) { + *id = *to; } - Value::Array(a) => a.iter_mut().for_each(|x| remap_media(x, map, depth + 1)), - _ => {} } } -/// `value` with its media ids rewritten, via its JSON form. -fn with_media(value: &T, map: &HashMap) -> Result { - let mut v = serde_json::to_value(value).map_err(|e| EngineError::Other(e.to_string()))?; - remap_media(&mut v, map, 0); - serde_json::from_value(v).map_err(|e| EngineError::Other(e.to_string())) -} - fn shape_ids(shapes: &[deckcraft_model::Shape]) -> Vec { let mut v = vec![]; deckcraft_model::walk(shapes, &mut |sh, _| v.push(sh.id)); @@ -132,20 +107,22 @@ fn reuse(s: &mut Session, p: &Value) -> Result { s.edit(|doc, sel| { let at = at.unwrap_or(sel.slide + 1).min(doc.slides.len()); // Media: what the slides use is copied under fresh ids. - let mut media_map: HashMap = HashMap::new(); + let mut media_map: HashMap = HashMap::new(); // Keep Source Formatting: bring each source master used, once, with fresh ids. let mut layout_map: HashMap = HashMap::new(); if keep { let mut masters_done: Vec = vec![]; for &i in &picked { - let Some((m, _)) = src.master_for(&src.slides[i]) else { continue }; + let Some((m, _)) = src.slides.get(i).and_then(|sl| src.master_for(sl)) else { continue }; if masters_done.contains(&m.id) { continue; } masters_done.push(m.id); - let used = serde_json::to_value(m).map_err(|e| EngineError::Other(e.to_string()))?; + let mut used = vec![]; + media_refs::visit::master(m, &mut |id: &MediaId| used.push(*id)); copy_media(doc, &src, &used, &mut media_map); - let mut m: deckcraft_model::Master = with_media(m, &media_map)?; + let mut m = m.clone(); + media_refs::visit_mut::master(&mut m, &mut remap(&media_map)); m.id = MasterId(doc.alloc_id()); renumber(doc, &mut m.shapes); for l in &mut m.layouts { @@ -159,10 +136,10 @@ fn reuse(s: &mut Session, p: &Value) -> Result { } let mut new_ids = vec![]; for (k, &i) in picked.iter().enumerate() { - let from = &src.slides[i]; - let used = serde_json::to_value(from.as_ref()).map_err(|e| EngineError::Other(e.to_string()))?; - copy_media(doc, &src, &used, &mut media_map); - let mut sl: Slide = with_media(from.as_ref(), &media_map)?; + let Some(from) = src.slides.get(i) else { continue }; + copy_media(doc, &src, &media_refs::slide_media(from), &mut media_map); + let mut sl: Slide = from.as_ref().clone(); + media_refs::visit_mut::slide(&mut sl, &mut remap(&media_map)); sl.id = SlideId(doc.alloc_id()); let map = renumber(doc, &mut sl.shapes); for a in &mut sl.animations { @@ -188,39 +165,97 @@ fn reuse(s: &mut Session, p: &Value) -> Result { }) } -/// Copy the source media `used` refers to into `doc` under fresh ids, once each. -fn copy_media(doc: &mut Presentation, src: &Presentation, used: &Value, map: &mut HashMap) { - let mut ids = vec![]; - collect_media(used, &mut ids, 0); - for id in ids { +/// Copy the source media `used` into `doc` under fresh ids, once each (identical bytes already in +/// `doc` are shared; a linked item keeps its link). +fn copy_media(doc: &mut Presentation, src: &Presentation, used: &[MediaId], map: &mut HashMap) { + for &id in used { if map.contains_key(&id) { continue; } - if let Some(m) = src.media(MediaId(id)) { - let new = doc.add_media(&m.name, &m.content_type, m.data.as_ref().clone()); - map.insert(id, new.0); - } + let Some(m) = src.media(id) else { continue }; + let new = if m.link.is_some() { + let mut item = m.clone(); + item.id = MediaId(doc.alloc_id()); + let new = item.id; + doc.media.push(item); + new + } else { + doc.add_media(&m.name, &m.content_type, m.data.as_ref().clone()) + }; + map.insert(id, new); } } -/// The media ids `v` refers to. -fn collect_media(v: &Value, out: &mut Vec, depth: usize) { - if depth > 256 { - return; - } - match v { - Value::Object(o) => { - for (k, x) in o { - if MEDIA_KEYS.contains(&k.as_str()) - && let Some(n) = x.as_u64().and_then(|n| u32::try_from(n).ok()) - { - out.push(n); - } else { - collect_media(x, out, depth + 1); - } - } +#[cfg(test)] +mod tests { + use deckcraft_model::style::{Artistic, Fill, PictureFill}; + use deckcraft_model::text::{Action, Bullet, Hyperlink, TextBody}; + use deckcraft_model::{Background, MediaClip, Shape, ShapeKind, defaults}; + use serde_json::json; + + use super::*; + + /// Every kind of media a slide can hold comes along, each pointing at its own bytes: the + /// artistic-effect original, a background picture, poster, sounds and picture bullets too. + #[test] + fn reused_slides_bring_every_media_reference() { + let mut src = defaults::new_presentation(None); + let blobs: Vec> = (0..8u8).map(|i| vec![0xD0, i, i, i]).collect(); + let ids: Vec = + blobs.iter().enumerate().map(|(i, b)| src.add_media(&format!("m{i}.bin"), "application/octet-stream", b.clone())).collect(); + let id = |i: usize| ids.get(i).copied().unwrap_or_default(); + let mut text = TextBody::from_text("bullet"); + if let Some(p) = text.paragraphs.first_mut() { + p.props.bullet = Some(Bullet::Picture { media: id(6) }); } - Value::Array(a) => a.iter().for_each(|x| collect_media(x, out, depth + 1)), - _ => {} + let slide = Arc::make_mut(src.slides.first_mut().expect("slide")); + slide.shapes = vec![ + Shape { + id: ShapeId(9001), + kind: ShapeKind::Picture { + fill: PictureFill { + media: id(0), + artistic: Some(Artistic { effect: "artisticBlur".into(), original: id(1) }), + ..Default::default() + }, + }, + ..Default::default() + }, + Shape { + id: ShapeId(9002), + kind: ShapeKind::Media(MediaClip { media: id(2), poster: Some(id(3)), ..Default::default() }), + ..Default::default() + }, + Shape { + id: ShapeId(9003), + click: Some(Hyperlink { + action: Action::NextSlide, + tooltip: String::new(), + highlight_click: false, + sound: Some(id(5)), + stop_sound: false, + }), + text: Some(text), + ..Default::default() + }, + ]; + slide.background = Some(Background::Fill { fill: Fill::Picture(PictureFill { media: id(4), ..Default::default() }) }); + slide.transition = Some(deckcraft_model::anim::Transition { sound: Some(id(7)), ..Default::default() }); + let bytes = deckcraft_format::save(&src).expect("save"); + + let mut s = Session::with_new(); + let own = vec![1u8, 2, 3]; + let own_id = s.edit(|doc, _| Ok(doc.add_media("own.bin", "application/octet-stream", own.clone()))).expect("own media"); + s.execute("slide.reuse", &json!({"data": super::super::base64_encode(&bytes), "name": "src.deckcraft", "slides": [0]})).expect("reuse"); + let doc = &s.doc().expect("doc").doc; + assert_eq!(doc.media(own_id).map(|m| m.data.as_slice() == own.as_slice()), Some(true), "own media untouched"); + let reused = doc.slides.get(1).expect("reused slide"); + let got: Vec> = + media_refs::slide_media(reused).iter().map(|m| doc.media(*m).map(|m| m.data.as_ref().clone()).unwrap_or_default()).collect(); + let mut got = got; + got.sort(); + let mut want = blobs.clone(); + want.sort(); + assert_eq!(got, want, "every reference resolves to its own copied bytes"); } } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index b6c0bbe7..60eebaf8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -17,6 +17,7 @@ pub mod icons; pub mod inkshape; pub mod keys; pub mod links; +pub mod media_refs; pub mod recovery; pub mod sample; pub mod sounds; diff --git a/crates/engine/src/media_refs.rs b/crates/engine/src/media_refs.rs new file mode 100644 index 00000000..33bc6a37 --- /dev/null +++ b/crates/engine/src/media_refs.rs @@ -0,0 +1,390 @@ +//! Every [`MediaId`] the model can reference, found by type rather than by scanning JSON keys. +//! +//! One walker, written once ([`walkers!`]) and expanded twice: [`visit`] reads ids, [`visit_mut`] +//! rewrites them in place. It covers pictures (and their artistic-effect originals), picture fills +//! wherever a fill can sit (shapes, lines, text runs, table cells and styles, chart series and +//! points, backgrounds, theme format schemes), media clips and their posters, 3-D models, OLE and +//! SmartArt previews, SmartArt node pictures, picture bullets, hyperlink, transition and +//! animation sounds, and embedded font faces. Adding a `MediaId` field to the model means adding +//! it here; the shape kinds are matched exhaustively so a new kind can't be missed. + +use std::collections::HashSet; + +use deckcraft_model::{MediaId, Presentation, Shape, Slide}; + +macro_rules! walkers { + ($($m:tt)?) => { + use deckcraft_model::chart::Chart; + use deckcraft_model::style::{Fill, Line, PictureFill}; + use deckcraft_model::table::{Table, TableBorder, TablePartStyle, TableStyleDef}; + use deckcraft_model::text::{Hyperlink, ListStyle, ParaProps, RunProps, TextBody, Bullet}; + use deckcraft_model::theme::Theme; + use deckcraft_model::{Background, EmbeddedFont, Layout, Master, MediaId, Shape, ShapeKind, Slide}; + + /// Deepest group nesting walked (the model's other walkers stop at 64 too). + const MAX_DEPTH: usize = 64; + + pub type F<'a> = dyn FnMut(&$($m)? MediaId) + 'a; + + pub fn picture(p: &$($m)? PictureFill, f: &mut F) { + f(&$($m)? p.media); + if let Some(a) = &$($m)? p.artistic { + f(&$($m)? a.original); + } + } + + pub fn fill(x: &$($m)? Fill, f: &mut F) { + if let Fill::Picture(p) = x { + picture(p, f); + } + } + + fn opt_fill(x: &$($m)? Option, f: &mut F) { + if let Some(x) = x { + fill(x, f); + } + } + + pub fn line(l: &$($m)? Line, f: &mut F) { + opt_fill(&$($m)? l.fill, f); + } + + fn opt_line(l: &$($m)? Option, f: &mut F) { + if let Some(l) = l { + line(l, f); + } + } + + fn link(h: &$($m)? Option, f: &mut F) { + if let Some(Hyperlink { sound: Some(id), .. }) = h { + f(id); + } + } + + fn run(r: &$($m)? RunProps, f: &mut F) { + opt_fill(&$($m)? r.fill, f); + opt_line(&$($m)? r.outline, f); + link(&$($m)? r.link, f); + link(&$($m)? r.hover, f); + } + + fn para(p: &$($m)? ParaProps, f: &mut F) { + if let Some(Bullet::Picture { media }) = &$($m)? p.bullet { + f(media); + } + } + + pub fn list(l: &$($m)? ListStyle, f: &mut F) { + for lv in (&$($m)? l.levels).into_iter().flatten() { + para(&$($m)? lv.para, f); + run(&$($m)? lv.run, f); + } + } + + pub fn text(t: &$($m)? TextBody, f: &mut F) { + list(&$($m)? t.list_style, f); + for p in &$($m)? t.paragraphs { + para(&$($m)? p.props, f); + run(&$($m)? p.def_props, f); + run(&$($m)? p.end_props, f); + for r in &$($m)? p.runs { + run(&$($m)? r.props, f); + } + } + } + + fn table(t: &$($m)? Table, f: &mut F) { + opt_fill(&$($m)? t.background, f); + for row in &$($m)? t.rows { + for c in &$($m)? row.cells { + text(&$($m)? c.text, f); + opt_fill(&$($m)? c.fill, f); + for b in &$($m)? c.borders { + opt_line(b, f); + } + opt_line(&$($m)? c.diag_down, f); + opt_line(&$($m)? c.diag_up, f); + } + } + } + + fn table_part(s: &$($m)? TablePartStyle, f: &mut F) { + opt_fill(&$($m)? s.fill, f); + for b in &$($m)? s.borders { + if let Some(TableBorder::Line { line: l }) = b { + line(l, f); + } + } + } + + pub fn table_style(t: &$($m)? TableStyleDef, f: &mut F) { + for (_, s) in &$($m)? t.parts { + table_part(s, f); + } + if let Some(s) = &$($m)? t.background { + table_part(s, f); + } + } + + fn chart(c: &$($m)? Chart, f: &mut F) { + for s in &$($m)? c.series { + opt_fill(&$($m)? s.fill, f); + opt_line(&$($m)? s.line, f); + for (_, x) in &$($m)? s.point_fills { + fill(x, f); + } + } + opt_fill(&$($m)? c.plot_fill, f); + opt_fill(&$($m)? c.chart_fill, f); + } + + fn shape_at(s: &$($m)? Shape, f: &mut F, depth: usize) { + if depth > MAX_DEPTH { + return; + } + match &$($m)? s.kind { + ShapeKind::Shape | ShapeKind::Connector { .. } | ShapeKind::Ink { .. } => {} + ShapeKind::Picture { fill } => picture(fill, f), + ShapeKind::Group { children, .. } => { + for c in children { + shape_at(c, f, depth + 1); + } + } + ShapeKind::Table(t) => table(t, f), + ShapeKind::Chart(c) => chart(c, f), + ShapeKind::Media(m) => { + f(&$($m)? m.media); + if let Some(p) = &$($m)? m.poster { + f(p); + } + } + ShapeKind::Model3d { media, poster } => { + f(media); + if let Some(p) = poster { + f(p); + } + } + ShapeKind::Opaque { preview, .. } => { + if let Some(p) = preview { + f(p); + } + } + } + opt_fill(&$($m)? s.fill, f); + opt_line(&$($m)? s.line, f); + if let Some(t) = &$($m)? s.text { + text(t, f); + } + link(&$($m)? s.click, f); + link(&$($m)? s.hover, f); + if let Some(d) = &$($m)? s.diagram { + for n in &$($m)? d.nodes { + if let Some(p) = &$($m)? n.picture { + f(p); + } + } + } + } + + /// A shape and, for a group, everything in it. + pub fn shape(s: &$($m)? Shape, f: &mut F) { + shape_at(s, f, 0); + } + + pub fn shapes(v: &$($m)? [Shape], f: &mut F) { + for s in v { + shape_at(s, f, 0); + } + } + + fn background(b: &$($m)? Option, f: &mut F) { + if let Some(Background::Fill { fill: x }) = b { + fill(x, f); + } + } + + /// A slide: shapes, background, transition and animation sounds, notes. + pub fn slide(s: &$($m)? Slide, f: &mut F) { + shapes(&$($m)? s.shapes, f); + background(&$($m)? s.background, f); + if let Some(t) = &$($m)? s.transition + && let Some(id) = &$($m)? t.sound + { + f(id); + } + for a in &$($m)? s.animations { + if let Some(id) = &$($m)? a.sound { + f(id); + } + } + text(&$($m)? s.notes, f); + } + + pub fn layout(l: &$($m)? Layout, f: &mut F) { + shapes(&$($m)? l.shapes, f); + background(&$($m)? l.background, f); + } + + pub fn theme(t: &$($m)? Theme, f: &mut F) { + for x in &$($m)? t.format.fills { + fill(x, f); + } + for x in &$($m)? t.format.bg_fills { + fill(x, f); + } + for l in &$($m)? t.format.lines { + line(l, f); + } + } + + /// A master with its theme, text styles and layouts. + pub fn master(m: &$($m)? Master, f: &mut F) { + theme(&$($m)? m.theme, f); + shapes(&$($m)? m.shapes, f); + background(&$($m)? m.background, f); + list(&$($m)? m.title_style, f); + list(&$($m)? m.body_style, f); + list(&$($m)? m.other_style, f); + for l in &$($m)? m.layouts { + layout(l, f); + } + } + + pub fn font(e: &$($m)? EmbeddedFont, f: &mut F) { + for id in [&$($m)? e.regular, &$($m)? e.bold, &$($m)? e.italic, &$($m)? e.bold_italic].into_iter().flatten() { + f(id); + } + } + }; +} + +/// Read every media id under a part of the model. +pub mod visit { + walkers!(); +} + +/// Rewrite every media id under a part of the model in place. +pub mod visit_mut { + walkers!(mut); +} + +/// Every media reference in a presentation: slides, masters (with their layouts and themes), +/// the notes and handout masters, default text style, table styles and embedded fonts. +pub fn visit_presentation(p: &Presentation, f: &mut visit::F) { + for s in &p.slides { + visit::slide(s, f); + } + for m in p.masters.iter().chain(p.notes_master.iter()).chain(p.handout_master.iter()) { + visit::master(m, f); + } + visit::list(&p.default_text_style, f); + for t in &p.table_styles { + visit::table_style(t, f); + } + for e in &p.embedded_fonts { + visit::font(e, f); + } +} + +/// The media ids a presentation references, each once, in the order first met. +pub fn presentation_media(p: &Presentation) -> Vec { + collect(|f| visit_presentation(p, f)) +} + +/// The media ids a slide references (shapes, background, sounds, notes), each once. +pub fn slide_media(s: &Slide) -> Vec { + collect(|f| visit::slide(s, f)) +} + +/// The media ids shapes reference (groups included), each once. +pub fn shapes_media(v: &[Shape]) -> Vec { + collect(|f| visit::shapes(v, f)) +} + +fn collect(walk: impl FnOnce(&mut visit::F)) -> Vec { + let mut seen = HashSet::new(); + let mut out = vec![]; + walk(&mut |id: &MediaId| { + if seen.insert(*id) { + out.push(*id); + } + }); + out +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use deckcraft_model::style::{Artistic, Fill, PictureFill}; + use deckcraft_model::text::{Action, Bullet, Hyperlink}; + use deckcraft_model::{Background, MediaClip, ShapeKind, defaults}; + + use super::*; + + fn pf(media: u32) -> PictureFill { + PictureFill { media: MediaId(media), ..Default::default() } + } + + /// One reference of each kind, every one a different id. + fn deck() -> Presentation { + let mut p = defaults::new_presentation(None); + let mut s = (*p.slides.first().cloned().unwrap_or_default()).clone(); + let pic = Shape { + kind: ShapeKind::Picture { + fill: PictureFill { artistic: Some(Artistic { effect: "artisticBlur".into(), original: MediaId(9002) }), ..pf(9001) }, + }, + ..Default::default() + }; + let clip = Shape { + kind: ShapeKind::Media(MediaClip { media: MediaId(9003), poster: Some(MediaId(9004)), ..Default::default() }), + ..Default::default() + }; + let mut text = deckcraft_model::text::TextBody::from_text("x"); + if let Some(para) = text.paragraphs.first_mut() { + para.props.bullet = Some(Bullet::Picture { media: MediaId(9007) }); + } + let filled = Shape { + fill: Some(Fill::Picture(pf(9005))), + click: Some(Hyperlink { + action: Action::NextSlide, + tooltip: String::new(), + highlight_click: false, + sound: Some(MediaId(9006)), + stop_sound: false, + }), + text: Some(text), + ..Default::default() + }; + let group = Shape { kind: ShapeKind::Group { children: vec![pic, clip], child: Default::default() }, ..Default::default() }; + s.shapes = vec![group, filled]; + s.background = Some(Background::Fill { fill: Fill::Picture(pf(9008)) }); + p.slides = vec![Arc::new(s)]; + if let Some(m) = p.masters.first_mut() { + Arc::make_mut(m).theme.format.bg_fills.push(Fill::Picture(pf(9009))); + } + p.embedded_fonts.push(deckcraft_model::EmbeddedFont { regular: Some(MediaId(9010)), ..Default::default() }); + p + } + + #[test] + fn every_kind_of_reference_is_found() { + let p = deck(); + let mut got = presentation_media(&p); + got.sort(); + assert_eq!(got, (9001..=9010).map(MediaId).collect::>()); + let mut slide = p.slides.first().map(|s| slide_media(s)).unwrap_or_default(); + slide.sort(); + assert_eq!(slide, (9001..=9008).map(MediaId).collect::>()); + } + + #[test] + fn visit_mut_rewrites_every_reference() { + let p = deck(); + let mut s = (**p.slides.first().expect("slide")).clone(); + visit_mut::slide(&mut s, &mut |id: &mut MediaId| id.0 += 100); + let mut got = slide_media(&s); + got.sort(); + assert_eq!(got, (9101..=9108).map(MediaId).collect::>()); + } +} diff --git a/docs/gaps.md b/docs/gaps.md index 89485553..5723c8ba 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -349,11 +349,10 @@ selected effect, and most of the effect gallery. Detail in - **Estimate:** 3–6 h to review, land and round-trip. ### Smaller missing rows -Zoom (summary/section/slide, 6–10 h), reuse slides (2–4 h), laser pointer (1–2 h), transition +Zoom (summary/section/slide, 6–10 h), reuse slides pane with thumbnails (1–2 h; `slide.reuse` inserts from a file), laser pointer (1–2 h), transition sounds (1–2 h), artistic effects (6–10 h), compress pictures (2–3 h), insert screenshot (2–3 h), record audio/screen and record slide sounds (1–2 h), artistic effects (3–6 h for the other 15 of 22; 7 approximated and round-tripped), remove background (6–10 h; needs a model or a classical -Zoom (summary/section/slide, 6–10 h), reuse slides (2–4 h), transition -Zoom (summary/section/slide, 6–10 h), reuse slides pane with thumbnails (1–2 h; `slide.reuse` inserts from a file), laser pointer (1–2 h), transition +Zoom (summary/section/slide, 6–10 h), reuse slides pane with thumbnails (1–2 h), transition sounds (1–2 h), artistic effects (6–10 h), remove background (6–10 h; needs a model or a classical matte), compress pictures (2–3 h), insert screenshot (2–3 h), record audio/screen and record slide show (8–12 h), compare presentations (6–10 h), password protection (4–6 h, ECMA-376 agile From 6050c04ecdb09281ec234d131f533f7a6cb52c34 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:39:15 -0400 Subject: [PATCH 11/16] Fix up #513: keep every referenced media; cap picture decoding picture.compress decided which originals were still in use by scanning JSON keys, which missed artistic-effect originals (and the embedded-font shape it assumed no longer exists), so compressing a picture whose bytes were also another picture's artistic original deleted media still in use. It now uses the engine's typed media walker (media_refs::presentation_media). Decoding is capped: the header's size is read first and pictures over 64 MP are left alone, and the decoder runs with image::Limits (512 MiB allocation), instead of decoding up to 400 MP before checking. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/compress.rs | 140 ++++++++++++++++++++++-------- 1 file changed, 104 insertions(+), 36 deletions(-) diff --git a/crates/engine/src/cmd/compress.rs b/crates/engine/src/cmd/compress.rs index 37dfe2ea..efa7ebfb 100644 --- a/crates/engine/src/cmd/compress.rs +++ b/crates/engine/src/cmd/compress.rs @@ -5,7 +5,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; use deckcraft_model::style::PictureFill; -use deckcraft_model::{MediaId, Presentation, Shape, ShapeId, ShapeKind}; +use deckcraft_model::{MediaId, Shape, ShapeId, ShapeKind}; use serde_json::{Value, json}; use super::*; @@ -35,6 +35,28 @@ fn real_crop(c: &[f64; 4]) -> bool { c.iter().all(|v| (0.0..1.0).contains(v)) && c.iter().any(|v| *v > 0.0) && c[0] + c[2] < 1.0 && c[1] + c[3] < 1.0 } +/// Largest picture decoded for compression, in pixels (a 61 MP camera frame fits); bigger ones +/// are left as they are. +const MAX_PIXELS: u64 = 64_000_000; +/// Most memory the decoder may allocate for one picture. +const MAX_DECODE_BYTES: u64 = 512 << 20; + +/// Decode `data` within [`MAX_PIXELS`] and [`MAX_DECODE_BYTES`]: the size is read from the header +/// first, so a picture claiming to be huge is never decoded. +fn decode(data: &[u8], format: image::ImageFormat) -> Option { + let (w, h) = image::ImageReader::with_format(std::io::Cursor::new(data), format).into_dimensions().ok()?; + if w == 0 || h == 0 || u64::from(w) * u64::from(h) > MAX_PIXELS { + return None; + } + let mut reader = image::ImageReader::with_format(std::io::Cursor::new(data), format); + let mut limits = image::Limits::default(); + limits.max_image_width = Some(w); + limits.max_image_height = Some(h); + limits.max_alloc = Some(MAX_DECODE_BYTES); + reader.limits(limits); + reader.decode().ok() +} + /// Compress `data` for a picture shown `w_pt` × `h_pt` with `crop`, at `ppi`. `None` when the /// format isn't a raster we re-encode (SVG, EMF, GIF…) or nothing would get smaller. fn pack(data: &[u8], crop: [f64; 4], w_pt: f64, h_pt: f64, ppi: f64, delete_cropped: bool) -> Option { @@ -43,9 +65,9 @@ fn pack(data: &[u8], crop: [f64; 4], w_pt: f64, h_pt: f64, ppi: f64, delete_crop if !matches!(format, F::Png | F::Jpeg | F::Bmp | F::Tiff | F::WebP) { return None; } - let mut img = image::load_from_memory_with_format(data, format).ok()?; + let mut img = decode(data, format)?; let (iw, ih) = (img.width() as f64, img.height() as f64); - if iw < 1.0 || ih < 1.0 || iw * ih > 400_000_000.0 { + if iw < 1.0 || ih < 1.0 { return None; } let cut = delete_cropped && real_crop(&crop); @@ -108,37 +130,6 @@ fn pictures_mut<'a>(shapes: &'a mut [Shape], want: &dyn Fn(ShapeId) -> bool, out } } -/// Media ids anything in the presentation still refers to (everything but the media list). -fn referenced_media(doc: &Presentation) -> HashSet { - fn scan(v: &Value, out: &mut HashSet, depth: usize) { - if depth > 256 { - return; - } - match v { - Value::Object(o) => { - for (k, x) in o { - match (k.as_str(), x.as_u64()) { - ("media" | "poster" | "preview" | "sound", Some(n)) => { - out.insert(n as u32); - } - _ => scan(x, out, depth + 1), - } - } - } - Value::Array(a) => a.iter().for_each(|x| scan(x, out, depth + 1)), - _ => {} - } - } - let mut out = HashSet::new(); - let mut shell = doc.clone(); - shell.media.clear(); - if let Ok(v) = serde_json::to_value(&shell) { - scan(&v, &mut out, 0); - } - out.extend(doc.embedded_fonts.iter().flat_map(|e| [e.regular, e.bold, e.italic, e.bold_italic]).flatten().map(|m| m.0)); - out -} - fn compress(s: &mut Session, p: &Value) -> Result { let ppi = f64_param(p, "ppi").unwrap_or(220.0); if !ppi.is_finite() || !(36.0..=1200.0).contains(&ppi) { @@ -204,8 +195,9 @@ fn compress(s: &mut Session, p: &Value) -> Result { } // Drop the originals nothing uses any more, so the saved file actually shrinks. if !changed.is_empty() { - let used = referenced_media(doc); - doc.media.retain(|m| !changed.contains(&m.id) || used.contains(&m.id.0)); + // Typed walk of every reference (artistic originals, fills, posters, sounds, fonts…). + let used: HashSet = crate::media_refs::presentation_media(doc).into_iter().collect(); + doc.media.retain(|m| !changed.contains(&m.id) || used.contains(&m.id)); } Ok(json!({"pictures": count, "before": before, "after": after, "ppi": ppi})) }) @@ -249,4 +241,80 @@ mod tests { let img = image::load_from_memory(&pk.bytes).unwrap(); assert_eq!(img.width(), 200); } + + /// A PNG whose header claims `w` × `h` (no pixel data): enough for the size check. + fn png_header(w: u32, h: u32) -> Vec { + fn crc(bytes: &[u8]) -> u32 { + let mut c = 0xFFFF_FFFFu32; + for b in bytes { + c ^= u32::from(*b); + for _ in 0..8 { + c = if c & 1 == 1 { 0xEDB8_8320 ^ (c >> 1) } else { c >> 1 }; + } + } + !c + } + fn chunk(out: &mut Vec, kind: &[u8], data: &[u8]) { + let body: Vec = kind.iter().chain(data).copied().collect(); + out.extend((data.len() as u32).to_be_bytes()); + out.extend(&body); + out.extend(crc(&body).to_be_bytes()); + } + let mut ihdr = w.to_be_bytes().to_vec(); + ihdr.extend(h.to_be_bytes()); + ihdr.extend([8, 2, 0, 0, 0]); + let mut out = b"\x89PNG\r\n\x1a\n".to_vec(); + chunk(&mut out, b"IHDR", &ihdr); + chunk(&mut out, b"IDAT", &[]); + chunk(&mut out, b"IEND", &[]); + out + } + + #[test] + fn huge_pictures_are_not_decoded() { + // 72 MP: over the cap, though within what the decoder would allocate by default. + let data = png_header(9_000, 8_000); + assert_eq!( + image::ImageReader::with_format(std::io::Cursor::new(&data), image::ImageFormat::Png).into_dimensions().ok(), + Some((9_000, 8_000)) + ); + assert!(decode(&data, image::ImageFormat::Png).is_none()); + assert!(pack(&data, [0.0; 4], 72.0, 72.0, 96.0, true).is_none()); + } + + /// Compressing a picture whose media is also another picture's artistic-effect original must + /// keep that media: the effect needs it to be changed or removed. + #[test] + fn media_still_used_as_an_artistic_original_is_kept() { + use deckcraft_model::style::Artistic; + let mut s = Session::with_new(); + let big = png(1200, 600); + let processed = png(1100, 600); + s.edit(|doc, _| { + let original = doc.add_media("original.png", "image/png", big.clone()); + let shown = doc.add_media("processed.png", "image/png", processed.clone()); + let slide = Arc::make_mut(doc.slides.first_mut().ok_or_else(|| bad("test", "no slide"))?); + let xfrm = Some(deckcraft_geom::Xfrm::new(0.0, 0.0, 72.0, 36.0)); + let with_effect = + PictureFill { media: shown, artistic: Some(Artistic { effect: "artisticBlur".into(), original }), ..Default::default() }; + slide.shapes.push(Shape { id: ShapeId(70_001), xfrm, kind: ShapeKind::Picture { fill: with_effect }, ..Default::default() }); + slide.shapes.push(Shape { + id: ShapeId(70_002), + xfrm, + kind: ShapeKind::Picture { fill: PictureFill { media: original, ..Default::default() } }, + ..Default::default() + }); + Ok(()) + }) + .unwrap(); + let r = s.execute("picture.compress", &json!({"all": true, "ppi": 96})).unwrap(); + assert_eq!(r["pictures"], 2, "{r}"); + let doc = &s.doc().unwrap().doc; + // Every reference left in the deck resolves to media that is still there. + for id in crate::media_refs::presentation_media(doc) { + assert!(doc.media(id).is_some(), "{id:?} was deleted while still referenced"); + } + assert!(doc.media.iter().any(|m| m.data.as_slice() == big.as_slice()), "the artistic original was deleted"); + assert!(!doc.media.iter().any(|m| m.data.as_slice() == processed.as_slice()), "the replaced processed picture is dropped"); + } } From b54ec601d836a4de2af9a783453d48c21ab4428f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:42:23 -0400 Subject: [PATCH 12/16] Fix up #508: read 3-D settings with the XML parser; keep only known presets Shape3d::parse scanned the kept a:scene3d/a:sp3d XML as strings and copied the camera, light rig, direction, material and bevel names from the file into the XML it writes back unescaped. It now parses the fragment with the importer's XML reader (exported from deckcraft-pptx as parse_xml) and keeps a value only when it is in the format's lists (ST_PresetCameraType, ST_LightRigType, ST_LightRigDirection, ST_PresetMaterialType, ST_BevelPresetType), else the default; lengths are clamped and the extrusion/contour colours are re-serialized (escaped) by the XML writer. The Format Shape pane rows in ui-parity.md that earlier merges had duplicated are folded into one. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/three_d.rs | 280 ++++++++++++++++++++++--------- crates/pptx/src/lib.rs | 4 + docs/ui-parity.md | 6 +- 3 files changed, 209 insertions(+), 81 deletions(-) diff --git a/crates/engine/src/cmd/three_d.rs b/crates/engine/src/cmd/three_d.rs index fe53450b..881a39d1 100644 --- a/crates/engine/src/cmd/three_d.rs +++ b/crates/engine/src/cmd/three_d.rs @@ -62,6 +62,126 @@ pub const LIGHTS: &[&str] = &[ "brightRoom", ]; +/// Every camera preset the file format defines (`ST_PresetCameraType`); a file's camera is kept +/// only when it is one of these. +pub const ALL_CAMERAS: &[&str] = &[ + "legacyObliqueTopLeft", + "legacyObliqueTop", + "legacyObliqueTopRight", + "legacyObliqueLeft", + "legacyObliqueFront", + "legacyObliqueRight", + "legacyObliqueBottomLeft", + "legacyObliqueBottom", + "legacyObliqueBottomRight", + "legacyPerspectiveTopLeft", + "legacyPerspectiveTop", + "legacyPerspectiveTopRight", + "legacyPerspectiveLeft", + "legacyPerspectiveFront", + "legacyPerspectiveRight", + "legacyPerspectiveBottomLeft", + "legacyPerspectiveBottom", + "legacyPerspectiveBottomRight", + "orthographicFront", + "isometricTopUp", + "isometricTopDown", + "isometricBottomUp", + "isometricBottomDown", + "isometricLeftUp", + "isometricLeftDown", + "isometricRightUp", + "isometricRightDown", + "isometricOffAxis1Left", + "isometricOffAxis1Right", + "isometricOffAxis1Top", + "isometricOffAxis2Left", + "isometricOffAxis2Right", + "isometricOffAxis2Top", + "isometricOffAxis3Left", + "isometricOffAxis3Right", + "isometricOffAxis3Bottom", + "isometricOffAxis4Left", + "isometricOffAxis4Right", + "isometricOffAxis4Bottom", + "obliqueTopLeft", + "obliqueTop", + "obliqueTopRight", + "obliqueLeft", + "obliqueRight", + "obliqueBottomLeft", + "obliqueBottom", + "obliqueBottomRight", + "perspectiveFront", + "perspectiveLeft", + "perspectiveRight", + "perspectiveAbove", + "perspectiveBelow", + "perspectiveAboveLeftFacing", + "perspectiveAboveRightFacing", + "perspectiveContrastingLeftFacing", + "perspectiveContrastingRightFacing", + "perspectiveHeroicLeftFacing", + "perspectiveHeroicRightFacing", + "perspectiveHeroicExtremeLeftFacing", + "perspectiveHeroicExtremeRightFacing", + "perspectiveRelaxed", + "perspectiveRelaxedModerately", +]; + +/// Every light rig the file format defines (`ST_LightRigType`). +pub const ALL_LIGHTS: &[&str] = &[ + "legacyFlat1", + "legacyFlat2", + "legacyFlat3", + "legacyFlat4", + "legacyNormal1", + "legacyNormal2", + "legacyNormal3", + "legacyNormal4", + "legacyHarsh1", + "legacyHarsh2", + "legacyHarsh3", + "legacyHarsh4", + "threePt", + "balanced", + "soft", + "harsh", + "flood", + "contrasting", + "morning", + "sunrise", + "sunset", + "chilly", + "freezing", + "flat", + "twoPt", + "glow", + "brightRoom", +]; + +/// Light rig directions (`ST_LightRigDirection`). +pub const LIGHT_DIRS: &[&str] = &["tl", "t", "tr", "l", "r", "bl", "b", "br"]; + +/// Every material the file format defines (`ST_PresetMaterialType`). +pub const ALL_MATERIALS: &[&str] = &[ + "legacyMatte", + "legacyPlastic", + "legacyMetal", + "legacyWireframe", + "matte", + "plastic", + "metal", + "warmMatte", + "translucentPowder", + "powder", + "dkEdge", + "softEdge", + "clear", + "flat", + "softmetal", +]; + /// One bevel: preset, width and height in points. #[derive(Clone, Debug, PartialEq)] pub struct Bevel { @@ -107,92 +227,54 @@ impl Default for Shape3d { } } -/// The start tags of `raw` as (local name, tag text), in order. -fn tags(raw: &str) -> Vec<(&str, &str)> { - let mut out = Vec::new(); - let mut rest = raw; - while let Some(i) = rest.find('<') { - let Some(t) = rest.get(i + 1..) else { break }; - let end = t.find('>').unwrap_or(t.len()); - let tag = t.get(..end).unwrap_or(""); - rest = t.get(end..).unwrap_or(""); - if tag.starts_with('/') || tag.starts_with('?') || tag.starts_with('!') { - continue; - } - let name = tag.split(|c: char| c.is_whitespace() || c == '/').next().unwrap_or(""); - let local = name.rsplit(':').next().unwrap_or(name); - out.push((local, tag)); - } - out -} - -fn attr<'a>(tag: &'a str, name: &str) -> Option<&'a str> { - let key = format!(" {name}=\""); - let s = tag.find(&key)? + key.len(); - let rest = tag.get(s..)?; - rest.get(..rest.find('"')?) -} - -fn num(tag: &str, name: &str) -> Option { - attr(tag, name).and_then(|v| v.parse::().ok()).filter(|v| v.is_finite()) -} - const EMU: f64 = 12700.0; const ANGLE: f64 = 60000.0; +/// Largest length kept, in points (PowerPoint's own limit). +const MAX_PT: f64 = 1584.0; -/// An `a:`-prefixed element `local` (with its children) cut from `raw`, if there is one. -fn element(raw: &str, local: &str) -> Option { - let open = format!(""); - let i = raw.find(&open)?; - let j = raw.get(i..)?.find(&close)? + i + close.len(); - raw.get(i..j).map(str::to_string) +/// `value` when it is one of `known`, else `default`. Values from a file are written back into +/// XML as they are, so nothing outside these lists may pass. +fn known(value: Option<&str>, known: &[&str], default: &str) -> String { + value.filter(|v| known.contains(v)).unwrap_or(default).to_string() +} + +fn num(e: &deckcraft_pptx::XmlEl, name: &str) -> Option { + e.attr(name).and_then(|v| v.trim().parse::().ok()).filter(|v| v.is_finite()) } impl Shape3d { - /// Reads kept 3-D XML; unknown parts take their defaults. + /// Reads kept 3-D XML (`a:scene3d` and `a:sp3d`); unknown or invalid parts take their defaults. pub fn parse(raw: &str) -> Shape3d { let mut s = Shape3d::default(); - let mut in_camera = false; - for (name, tag) in tags(raw) { - match name { - "camera" => { - s.camera = attr(tag, "prst").unwrap_or("orthographicFront").to_string(); - in_camera = !tag.ends_with('/'); - } - "rot" if in_camera => { - let d = |n| num(tag, n).unwrap_or(0.0) / ANGLE; - s.rot = Some((d("lon"), d("lat"), d("rev"))); - in_camera = false; - } - "lightRig" => { - in_camera = false; - s.light = attr(tag, "rig").unwrap_or("threePt").to_string(); - s.light_dir = attr(tag, "dir").unwrap_or("t").to_string(); - } - "sp3d" => { - in_camera = false; - s.depth = num(tag, "extrusionH").unwrap_or(0.0) / EMU; - s.contour = num(tag, "contourW").unwrap_or(0.0) / EMU; - s.z = num(tag, "z").unwrap_or(0.0) / EMU; - s.material = attr(tag, "prstMaterial").unwrap_or("warmMatte").to_string(); - } - "bevelT" | "bevelB" => { - let b = Bevel { - preset: attr(tag, "prst").unwrap_or("circle").to_string(), - w: num(tag, "w").unwrap_or(76200.0) / EMU, - h: num(tag, "h").unwrap_or(76200.0) / EMU, - }; - if name == "bevelT" { - s.top = Some(b); - } else { - s.bottom = Some(b); - } - } - _ => {} + let Ok(doc) = deckcraft_pptx::parse_xml(format!("{raw}").as_bytes()) else { return s }; + let root = &doc.root; + if let Some(cam) = root.path(&["scene3d", "camera"]) { + s.camera = known(cam.attr("prst"), ALL_CAMERAS, "orthographicFront"); + if let Some(rot) = cam.child("rot") { + let d = |n| num(rot, n).unwrap_or(0.0) / ANGLE; + s.rot = Some((d("lon"), d("lat"), d("rev"))); } } - s.colors = ["extrusionClr", "contourClr"].iter().filter_map(|n| element(raw, n)).collect(); + if let Some(l) = root.path(&["scene3d", "lightRig"]) { + s.light = known(l.attr("rig"), ALL_LIGHTS, "threePt"); + s.light_dir = known(l.attr("dir"), LIGHT_DIRS, "t"); + } + if let Some(sp) = root.child("sp3d") { + let pt = |n, lo: f64| (num(sp, n).unwrap_or(0.0) / EMU).clamp(lo, MAX_PT); + s.depth = pt("extrusionH", 0.0); + s.contour = pt("contourW", 0.0); + s.z = pt("z", -MAX_PT); + s.material = known(sp.attr("prstMaterial"), ALL_MATERIALS, "warmMatte"); + for (name, slot) in [("bevelT", &mut s.top), ("bevelB", &mut s.bottom)] { + if let Some(b) = sp.child(name) { + let len = |n| (num(b, n).unwrap_or(76200.0) / EMU).clamp(0.0, MAX_PT); + *slot = Some(Bevel { preset: known(b.attr("prst"), BEVELS, "circle"), w: len("w"), h: len("h") }); + } + } + // Colours are re-serialized by the XML writer (escaped); only `a:`-prefixed ones, + // since they are written back inside our own `a:sp3d`. + s.colors = sp.elements().filter(|c| (c.is("extrusionClr") || c.is("contourClr")) && c.prefix() == "a").map(|c| c.to_xml()).collect(); + } s } @@ -302,3 +384,49 @@ pub fn store(e: &mut deckcraft_model::Effects, s: &Shape3d) { e.raw3d = s.to_xml(); e.bevel = if e.raw3d.is_some() { s.top.as_ref().map(|b| b.preset.clone()) } else { None }; } + +#[cfg(test)] +mod tests { + use super::*; + + const NS: &str = r#"xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main""#; + + /// Preset names from a file are written back into XML; anything outside the format's lists + /// (here, values that would close the tag and inject markup once unescaped) falls back to the + /// default instead. + #[test] + fn unknown_presets_from_a_file_are_not_written_back() { + let evil = "x"/><a:evil/>"; + let raw = format!( + r#""# + ); + let s = Shape3d::parse(&raw); + assert_eq!( + (s.camera.as_str(), s.light.as_str(), s.light_dir.as_str(), s.material.as_str()), + ("orthographicFront", "threePt", "t", "warmMatte") + ); + assert_eq!(s.top.as_ref().map(|b| b.preset.as_str()), Some("circle")); + assert_eq!(s.depth, 1.0); + let xml = s.to_xml().expect("not flat"); + assert!(!xml.contains("{xml}").as_bytes()).expect("well-formed"); + assert_eq!(doc.root.path(&["sp3d", "extrusionClr", "srgbClr"]).and_then(|c| c.attr("val")), Some(r#"x"/>"#)); + } + + #[test] + fn known_presets_round_trip() { + let raw = format!( + r#""# + ); + let s = Shape3d::parse(&raw); + assert_eq!(s.camera, "legacyObliqueTopLeft"); + assert_eq!(s.rot, Some((10.0, 20.0, 0.0))); + assert_eq!((s.light.as_str(), s.light_dir.as_str(), s.material.as_str()), ("legacyHarsh2", "tl", "legacyMetal")); + assert_eq!((s.z, s.contour), (-1.0, 1.0)); + assert_eq!(s.bottom, Some(Bevel { preset: "artDeco".into(), w: 3.0, h: 1.0 })); + assert_eq!(Shape3d::parse(&s.to_xml().expect("3-D")), s); + // Not XML at all: defaults. + assert_eq!(Shape3d::parse(" Date: Wed, 14 Oct 2026 15:47:24 -0400 Subject: [PATCH 13/16] Fix up #488: background opens enter the session through file.adopt The progress window's result was added with add_presentation_file, straight into the session: not journaled and outside the command panic guard that file.open runs under. The host now hands the loaded file to Session::stage_loaded and runs the new internal command file.adopt {path, token}, which shares file.open's tail (repaired-copy handling, damaged-media report) and so is journaled and guarded. Without the staged copy (a replayed journal, a stale token) file.adopt reads path like file.open. Reading still happens on the background thread. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/file.rs | 42 +++++++++++++++++++++++++++-------- crates/engine/src/lib.rs | 22 ++++++++++++++++++ crates/engine/src/tests.rs | 32 ++++++++++++++++++++++++++ crates/ui-egui/src/opening.rs | 14 ++++++++---- 4 files changed, 97 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index aa0b4719..3a24f70a 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -19,6 +19,15 @@ pub fn specs() -> Vec { ), cmd!(query "file.templates", "Starter Templates", [], None, "{} → [{name, summary, theme, slides}]", always, templates), cmd!(noundo "file.open", "Open…", ["File"], Some("Cmd+O"), "{path}", always, open), + cmd!( + noundo "file.adopt", + "Open", + [], + None, + "{path, token?} (a presentation the host read in the background and staged with Session::stage_loaded; without one, reads path like file.open)", + always, + adopt + ), cmd!(noundo "file.openBytes", "Open Data", [], None, "{name, data: base64}", always, open_bytes), cmd!(noundo "file.save", "Save", ["File"], Some("Cmd+S"), "{path?, format?: deckcraft|pptx}", has_doc, save), cmd!(noundo "file.saveAs", "Save As…", ["File"], Some("Cmd+Shift+S"), "{path, format?}", has_doc, save_as), @@ -163,8 +172,8 @@ pub fn add_opened(s: &mut Session, name: &str, path: Option, p: Presenta } /// A presentation read from disk, not yet added to a session: [`read_presentation_file`] needs no -/// session, so a host can run it on a background thread and add the result with -/// [`add_presentation_file`]. +/// session, so a host can run it on a background thread, hand the result to +/// [`Session::stage_loaded`] and add it with `file.adopt`. #[derive(Debug)] pub struct LoadedFile { pub path: String, @@ -184,22 +193,37 @@ pub fn read_presentation_file(path: &str) -> Result { Ok(LoadedFile { path: path.to_string(), doc, native, damaged }) } -/// Add a presentation read by [`read_presentation_file`] as the active document. A damaged file -/// opens as an untitled repaired copy, so Save can't overwrite the original with the empty media. -pub fn add_presentation_file(s: &mut Session, f: LoadedFile) -> Result<(usize, Vec)> { +/// Add a loaded presentation as the active document: the shared end of `file.open` and +/// `file.adopt`. A damaged file opens as an untitled repaired copy, so Save can't overwrite the +/// original with the empty media. +fn adopt_loaded(s: &mut Session, f: LoadedFile) -> Result { let LoadedFile { path, doc, native, damaged } = f; let keep_path = native && damaged.is_empty(); let i = add_opened(s, &path, keep_path.then(|| path.clone()), doc); report_damage(s, &damaged)?; - Ok((i, damaged)) + let st = s.doc()?; + Ok(json!({"document": i, "slides": st.doc.slides.len(), "missingFonts": st.missing_fonts, "damagedMedia": damaged})) } #[cfg(not(target_arch = "wasm32"))] fn open(s: &mut Session, p: &Value) -> Result { let path = str_param(p, "path").ok_or_else(|| bad("file.open", "missing `path`"))?; - let (i, damaged) = add_presentation_file(s, read_presentation_file(path)?)?; - let st = s.doc()?; - Ok(json!({"document": i, "slides": st.doc.slides.len(), "missingFonts": st.missing_fonts, "damagedMedia": damaged})) + adopt_loaded(s, read_presentation_file(path)?) +} + +/// `file.adopt {path, token?}`: add the presentation a host read in the background and staged +/// with [`Session::stage_loaded`]. Without that staged file (a replayed journal, a stale token) +/// it reads `path` like `file.open`, so the journal entry stays replayable. +fn adopt(s: &mut Session, p: &Value) -> Result { + let path = str_param(p, "path").ok_or_else(|| bad("file.adopt", "missing `path`"))?; + let staged = p.get("token").and_then(Value::as_u64).and_then(|t| s.take_loaded(t)).filter(|f| f.path == path); + match staged { + Some(f) => adopt_loaded(s, f), + #[cfg(not(target_arch = "wasm32"))] + None => adopt_loaded(s, read_presentation_file(path)?), + #[cfg(target_arch = "wasm32")] + None => Err(bad("file.adopt", format!("{path} was not loaded"))), + } } #[cfg(target_arch = "wasm32")] fn open(_s: &mut Session, _p: &Value) -> Result { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 60eebaf8..29fae722 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -470,6 +470,9 @@ pub struct Session { pub mac_keys: bool, /// Command panics caught by the guard (the document was rolled back); soak tests require 0. pub panics_caught: u64, + /// Presentations a host read off the UI thread, waiting for `file.adopt` (by token). + loaded: Vec<(u64, cmd::file::LoadedFile)>, + next_loaded: u64, } impl Default for Session { @@ -499,8 +502,27 @@ impl Session { gif_paused: Default::default(), mac_keys: cfg!(target_os = "macos"), panics_caught: 0, + loaded: vec![], + next_loaded: 0, } } + /// Hand over a presentation read on another thread ([`cmd::file::read_presentation_file`]); + /// `file.adopt` with the returned token adds it as a document, through the command path like + /// `file.open` (journaled, panic-guarded). Only the latest few wait; older ones are dropped. + pub fn stage_loaded(&mut self, f: cmd::file::LoadedFile) -> u64 { + const KEEP: usize = 4; + self.next_loaded = self.next_loaded.wrapping_add(1); + if self.loaded.len() >= KEEP { + self.loaded.remove(0); + } + self.loaded.push((self.next_loaded, f)); + self.next_loaded + } + /// The staged presentation for `token`, removed from the session. + pub(crate) fn take_loaded(&mut self, token: u64) -> Option { + let i = self.loaded.iter().position(|(t, _)| *t == token)?; + Some(self.loaded.remove(i).1) + } /// A session with one new presentation open. pub fn with_new() -> Self { let mut s = Self::new(); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index a4359ee8..e55127e0 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2739,3 +2739,35 @@ fn three_d_reset_and_flat_shapes_drop_the_xml() { s3.apply(&json!({"depth": 3})); assert!(s3.to_xml().unwrap().contains("")); } + +/// A presentation read off the UI thread enters the session through `file.adopt`: journaled like +/// `file.open`, and replayable from the journal without the staged copy. +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn background_loads_are_adopted_through_a_command() { + let mut src = Session::with_new(); + src.execute("slide.new", &json!({})).unwrap(); + let path = std::env::temp_dir().join(format!("deckcraft-adopt-{}.deckcraft", std::process::id())); + let path = path.to_string_lossy().to_string(); + src.execute("file.saveAs", &json!({"path": path})).unwrap(); + + let mut s = Session::new(); + let loaded = cmd::file::read_presentation_file(&path).unwrap(); + let token = s.stage_loaded(loaded); + let r = s.execute("file.adopt", &json!({"path": path, "token": token})).unwrap(); + assert_eq!(r["slides"], 2, "{r}"); + assert_eq!(s.documents().len(), 1); + assert_eq!(s.doc().unwrap().path.as_deref(), Some(path.as_str())); + let (id, params) = s.journal.last().cloned().unwrap(); + assert_eq!(id, "file.adopt"); + // The token was used up; replaying the entry reads the file again. + let mut replay = Session::new(); + replay.execute(&id, ¶ms).unwrap(); + assert_eq!(replay.doc().unwrap().doc.slides.len(), 2); + // A token staged for another path is not taken for this one. + let other = s.stage_loaded(cmd::file::LoadedFile { path: "/elsewhere.pptx".into(), doc: Default::default(), native: true, damaged: vec![] }); + s.execute("file.adopt", &json!({"path": path, "token": other})).unwrap(); + assert_eq!(s.doc().unwrap().doc.slides.len(), 2); + assert!(s.execute("file.adopt", &json!({})).is_err()); + std::fs::remove_file(&path).ok(); +} diff --git a/crates/ui-egui/src/opening.rs b/crates/ui-egui/src/opening.rs index d48734ec..debeb03c 100644 --- a/crates/ui-egui/src/opening.rs +++ b/crates/ui-egui/src/opening.rs @@ -71,10 +71,14 @@ impl SlideApp { }; let Some(o) = self.opening.take() else { return }; match r { - Ok(f) => match deckcraft_engine::cmd::file::add_presentation_file(&mut self.session, f) { - Ok(_) => self.note_recent(&o.path), - Err(e) => self.set_status(e.to_string()), - }, + Ok(f) => { + // Into the session through the command path, like file.open: journaled and guarded. + let token = self.session.stage_loaded(f); + match self.session.execute("file.adopt", &serde_json::json!({"path": o.path, "token": token})) { + Ok(_) => self.note_recent(&o.path), + Err(e) => self.set_status(e.to_string()), + } + } Err(e) => self.set_status(e), } } @@ -144,6 +148,8 @@ mod tests { assert_eq!(app.session.documents().len(), 1); assert_eq!(app.session.active().unwrap().doc.slides.len(), 3); assert_eq!(app.ui.recent.first(), Some(&path)); + // It went in through the command path (journaled like file.open). + assert_eq!(app.session.journal.last().map(|(id, _)| id.as_str()), Some("file.adopt")); std::fs::remove_file(&path).ok(); } From 76978b1634e5a8670461e84050a7dfc49c86b2e7 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:50:07 -0400 Subject: [PATCH 14/16] Fix up merged PRs: a background open that fails says why on the start screen Main (#346) shows why a file couldn't be opened as a notice over the start screen; #488's background open put the error in the status bar instead, which broke a_file_that_cant_be_opened_says_why. The background open now reports through the same notice, and the test waits for the background read. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/backstage_tests.rs | 9 +++++++++ crates/ui-egui/src/opening.rs | 13 +++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/backstage_tests.rs b/crates/ui-egui/src/backstage_tests.rs index e0f4e6d2..9b3906a2 100644 --- a/crates/ui-egui/src/backstage_tests.rs +++ b/crates/ui-egui/src/backstage_tests.rs @@ -444,6 +444,15 @@ fn a_file_that_cant_be_opened_says_why() { h.run_steps(2); h.get_by_label("Browse").click(); h.run_steps(3); + // The file is read in the background (#350): wait for it. + for _ in 0..500 { + if h.state().opening.is_none() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + h.run_steps(1); + } + h.run_steps(2); assert!(h.state().session.documents().is_empty()); assert!(h.query_by_label_contains("not a presentation DeckCraft can read").is_some(), "the error is on screen"); let _ = std::fs::remove_dir_all(&dir); diff --git a/crates/ui-egui/src/opening.rs b/crates/ui-egui/src/opening.rs index debeb03c..37d20a4e 100644 --- a/crates/ui-egui/src/opening.rs +++ b/crates/ui-egui/src/opening.rs @@ -32,7 +32,7 @@ impl SlideApp { #[cfg(target_arch = "wasm32")] { if let Err(e) = self.open_path(path) { - self.set_status(e); + self.notify(e); } } #[cfg(not(target_arch = "wasm32"))] @@ -47,7 +47,7 @@ impl SlideApp { Ok(_) => self.opening = Some(Opening { path: path.to_string(), started_ms: crate::now_ms(), rx }), Err(_) => { if let Err(e) = self.open_path(path) { - self.set_status(e); + self.notify(e); } } } @@ -76,10 +76,11 @@ impl SlideApp { let token = self.session.stage_loaded(f); match self.session.execute("file.adopt", &serde_json::json!({"path": o.path, "token": token})) { Ok(_) => self.note_recent(&o.path), - Err(e) => self.set_status(e.to_string()), + Err(e) => self.notify(e.to_string()), } } - Err(e) => self.set_status(e), + // Over the start screen too, like a failed file.open (#346). + Err(e) => self.notify(e), } } @@ -167,14 +168,14 @@ mod tests { } #[test] - fn errors_show_in_the_status_bar_and_a_second_open_waits() { + fn errors_are_shown_and_a_second_open_waits() { let mut app = SlideApp::new(Session::new(), crate::Services::default()); app.open_path_in_background("/nonexistent/deckcraft/missing.pptx"); app.open_path_in_background("/nonexistent/deckcraft/other.pptx"); assert!(app.status.as_ref().is_some_and(|(m, _)| m.starts_with("Still opening missing.pptx"))); wait(&mut app); assert!(app.session.documents().is_empty()); - assert!(app.status.as_ref().is_some_and(|(m, _)| m.contains("missing.pptx")), "{:?}", app.status); + assert!(app.toast.as_ref().is_some_and(|t| t.text.contains("missing.pptx")), "the reason is shown"); } #[test] From 10eb4866d966b872c54fe6dc2a4dced3b66d89ff Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:53:57 -0400 Subject: [PATCH 15/16] Fix up merged PRs: the media walker counts #209's kept object parts SmartArt and OLE objects now keep their parts as media items (ShapeKind::Opaque::parts). media_refs visits them, so slide.reuse copies them and picture.compress never deletes them; the Opaque arm names every field so a new one can't be missed. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/media_refs.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/media_refs.rs b/crates/engine/src/media_refs.rs index 33bc6a37..a4e17453 100644 --- a/crates/engine/src/media_refs.rs +++ b/crates/engine/src/media_refs.rs @@ -4,7 +4,7 @@ //! rewrites them in place. It covers pictures (and their artistic-effect originals), picture fills //! wherever a fill can sit (shapes, lines, text runs, table cells and styles, chart series and //! points, backgrounds, theme format schemes), media clips and their posters, 3-D models, OLE and -//! SmartArt previews, SmartArt node pictures, picture bullets, hyperlink, transition and +//! SmartArt previews and kept parts, SmartArt node pictures, picture bullets, hyperlink, transition and //! animation sounds, and embedded font faces. Adding a `MediaId` field to the model means adding //! it here; the shape kinds are matched exhaustively so a new kind can't be missed. @@ -164,10 +164,15 @@ macro_rules! walkers { f(p); } } - ShapeKind::Opaque { preview, .. } => { + // Every field named, so a new one can't slip past. + ShapeKind::Opaque { xml: _, preview, label: _, parts } => { if let Some(p) = preview { f(p); } + // SmartArt data, OLE embeddings… kept as media items (#209). + for (_, _, id) in parts { + f(id); + } } } opt_fill(&$($m)? s.fill, f); @@ -387,4 +392,19 @@ mod tests { got.sort(); assert_eq!(got, (9101..=9108).map(MediaId).collect::>()); } + + /// SmartArt and OLE objects keep their parts as media items (#209); those count as used. + #[test] + fn kept_parts_of_unmodelled_objects_are_references() { + let opaque = Shape { + kind: ShapeKind::Opaque { + xml: String::new(), + preview: Some(MediaId(9101)), + label: "object".into(), + parts: vec![("rId2".into(), "oleObject".into(), MediaId(9102))], + }, + ..Default::default() + }; + assert_eq!(shapes_media(std::slice::from_ref(&opaque)), vec![MediaId(9101), MediaId(9102)]); + } } From 5006f4cc842fcaf78225396c5b45db91035d99b7 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:56:55 -0400 Subject: [PATCH 16/16] Fix up merged PRs: slide.reuse's params doc uses commas The landing branch checks that every params doc parses into its keys; #509's used `;` between keys, which that parser rejects. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/reuse.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/reuse.rs b/crates/engine/src/cmd/reuse.rs index 1025b01e..bcdc5f7c 100644 --- a/crates/engine/src/cmd/reuse.rs +++ b/crates/engine/src/cmd/reuse.rs @@ -20,7 +20,7 @@ pub fn specs() -> Vec { "Reuse Slides…", ["Home", "Slides", "New Slide"], None, - "{path? | data: base64, name?; slides?: [index] (0-based, default all); keepFormatting?: bool (default false: use the destination theme); at?: index (default after the current slide)}", + "{path? | data: base64, name?, slides?: [index] (0-based, default all), keepFormatting?: bool (default false: use the destination theme), at?: index (default after the current slide)}", has_doc, reuse )]