From 368e07493cb9229be4721d556ff8a536fab35215 Mon Sep 17 00:00:00 2001 From: Faabyy <68284158+Faabyy@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:38:17 +0000 Subject: [PATCH] PPTX: a slide that can't be read is reported, not dropped in silence (#203) The importer skipped a slide whose part was missing or unreadable and only logged it, so `info` showed one slide fewer with exit code 0 and a save wrote the shorter deck over the original. `deckcraft_pptx::import_checked` returns the parts it left out (`import` is unchanged). The engine carries them as `Damage::slides` next to the damaged media of a `.deckcraft` file: `file.open` and `file.openBytes` return `lostSlides`, the app shows a message and opens a repaired copy (so Save asks for a new name), and `deckcraft-cli` warns on stderr. Co-Authored-By: Claude Code # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 1dc6972 # Last command done (1 command done): # pick d2e995c PPTX: a slide that can't be read is reported, not dropped in silence (#203) # No commands remaining. # You are currently rebasing. # # Changes to be committed: # modified: Cargo.lock # modified: ROADMAP.md # modified: apps/deckcraft-cli/Cargo.toml # modified: apps/deckcraft-cli/src/main.rs # new file: apps/deckcraft-cli/tests/damaged.rs # modified: crates/engine/src/cmd/file.rs # modified: crates/engine/src/tests.rs # modified: crates/pptx/src/lib.rs # modified: crates/pptx/src/read/mod.rs # modified: crates/pptx/tests/malformed.rs # modified: docs/file-format-parity.md # # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 22db926 # Last command done (1 command done): # pick 31b0d33 PPTX: a slide that can't be read is reported, not dropped in silence (#203) # No commands remaining. # You are currently rebasing. # # Changes to be committed: # modified: Cargo.lock # modified: ROADMAP.md # modified: apps/deckcraft-cli/Cargo.toml # modified: apps/deckcraft-cli/src/main.rs # new file: apps/deckcraft-cli/tests/damaged.rs # modified: crates/engine/src/cmd/file.rs # modified: crates/engine/src/tests.rs # modified: crates/pptx/src/lib.rs # modified: crates/pptx/src/read/mod.rs # modified: crates/pptx/tests/malformed.rs # modified: docs/file-format-parity.md # --- Cargo.lock | 1 + ROADMAP.md | 1 + apps/deckcraft-cli/Cargo.toml | 3 ++ apps/deckcraft-cli/src/main.rs | 18 ++++++- apps/deckcraft-cli/tests/damaged.rs | 53 ++++++++++++++++++++ crates/engine/src/cmd/file.rs | 77 ++++++++++++++++++++--------- crates/engine/src/tests.rs | 38 +++++++++++++- crates/pptx/src/lib.rs | 6 +++ crates/pptx/src/read/mod.rs | 10 +++- crates/pptx/tests/malformed.rs | 23 +++++++++ docs/file-format-parity.md | 3 +- 11 files changed, 206 insertions(+), 27 deletions(-) create mode 100644 apps/deckcraft-cli/tests/damaged.rs diff --git a/Cargo.lock b/Cargo.lock index e0715e25..66c79f0b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -941,6 +941,7 @@ dependencies = [ "deckcraft-engine", "deckcraft-mcp", "serde_json", + "zip", ] [[package]] diff --git a/ROADMAP.md b/ROADMAP.md index 9e430a0c..8c1d8367 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -133,6 +133,7 @@ app's JSON control channel (`deckcraft --control PORT`). |---|---| | 2026-10-15 | PPTX trigger sequences keep separate On Click steps, including paragraph builds; synthetic import/edit/save/reopen and headless show regressions | | 2026-10-15 | CLI: .txt and .md export includes table cell text, which it used to skip silently (#814) | +| 2026-10-15 | A `.pptx` slide that can't be read is reported, not dropped in silence: `lostSlides` from `file.open`, a message and a repaired copy in the app, a warning from `deckcraft-cli` (#203) | | 2026-10-15 | PDF export and rendering: `anchorCtr` centres the drawn text block — right-aligned text no longer ran past the box edge (#736) | | 2026-10-14 | connected MCP rejects unrelated, malformed, incomplete and oversized replies without replay; buffered unsolicited bytes force a fresh connection before sending (#718) | | 2026-10-14 | Empty bulleted and numbered paragraphs show gray markers while editing, including on first click and after Enter. Markers use the list style and stay out of slide shows and exports. | diff --git a/apps/deckcraft-cli/Cargo.toml b/apps/deckcraft-cli/Cargo.toml index 898a1fb3..6d2f572e 100644 --- a/apps/deckcraft-cli/Cargo.toml +++ b/apps/deckcraft-cli/Cargo.toml @@ -17,3 +17,6 @@ path = "src/main.rs" deckcraft-engine = { workspace = true } deckcraft-mcp = { workspace = true } serde_json = { workspace = true } + +[dev-dependencies] +zip = { workspace = true } diff --git a/apps/deckcraft-cli/src/main.rs b/apps/deckcraft-cli/src/main.rs index 5934973e..04338a9a 100644 --- a/apps/deckcraft-cli/src/main.rs +++ b/apps/deckcraft-cli/src/main.rs @@ -117,11 +117,27 @@ fn open(s: &mut Session, input: Option<&str>, sample: bool) -> Result<(), String return deckcraft_engine::sample::open_sample(s).map_err(|e| e.to_string()); } match input { - Some(p) => s.execute("file.open", &json!({"path": p})).map(|_| ()).map_err(|e| e.to_string()), + Some(p) => { + let opened = s.execute("file.open", &json!({"path": p})).map_err(|e| e.to_string())?; + warn_if_damaged(p, &opened); + Ok(()) + } None => s.execute("file.new", &json!({})).map(|_| ()).map_err(|e| e.to_string()), } } +/// Say on stderr what a file lost on opening, so a saved copy doesn't silently miss it (#203). +fn warn_if_damaged(path: &str, opened: &serde_json::Value) { + let count = |key: &str| opened.get(key).and_then(|v| v.as_array()).map_or(0, |a| a.len()); + let (slides, media) = (count("lostSlides"), count("damagedMedia")); + if slides > 0 { + eprintln!("deckcraft-cli: warning: {path}: {slides} slide{} couldn't be read and left out", if slides == 1 { "" } else { "s" }); + } + if media > 0 { + eprintln!("deckcraft-cli: warning: {path}: {media} picture or media file{} couldn't be read", if media == 1 { "" } else { "s" }); + } +} + /// Median and slowest of `runs` timings of `f`, in milliseconds. fn time_ms(runs: usize, mut f: impl FnMut() -> Result<(), String>) -> Result<(f64, f64), String> { let mut ms = Vec::with_capacity(runs); diff --git a/apps/deckcraft-cli/tests/damaged.rs b/apps/deckcraft-cli/tests/damaged.rs new file mode 100644 index 00000000..de85a66b --- /dev/null +++ b/apps/deckcraft-cli/tests/damaged.rs @@ -0,0 +1,53 @@ +//! Opening a `.pptx` that lost slides says so on stderr instead of saving a shorter deck in silence (#203). + +use std::io::{Cursor, Read, Write}; +use std::path::PathBuf; +use std::process::{Command, Output}; + +fn cli(args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_deckcraft-cli")).args(args).output().unwrap() +} + +fn temp_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("deckcraft-cli-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +/// `deck` with `ppt/slides/slide2.xml` cut off. +fn without_slide2(deck: &[u8]) -> Vec { + let mut zin = zip::ZipArchive::new(Cursor::new(deck)).unwrap(); + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let options = zip::write::SimpleFileOptions::default(); + for i in 0..zin.len() { + let mut f = zin.by_index(i).unwrap(); + let mut data = vec![]; + f.read_to_end(&mut data).unwrap(); + if f.name() == "ppt/slides/slide2.xml" { + data = b" Result { open_checked(name, bytes).map(|(p, _)| p) } -/// [`open_presentation`], plus the embedded media a `.deckcraft` file had lost or damaged. -pub fn open_checked(name: &str, bytes: &[u8]) -> Result<(Presentation, Vec)> { - if deckcraft_format::sniff(bytes) { - return deckcraft_format::load_checked(bytes).map_err(|e| EngineError::Other(e.to_string())); - } - open_other(name, bytes).map(|p| (p, vec![])) +/// What a file lost on opening. +#[derive(Debug, Default, Clone)] +pub struct Damage { + /// Pictures and media a `.deckcraft` file had lost or damaged. + pub media: Vec, + /// Slides a `.pptx` lists but that couldn't be read, so they are not in the presentation. + pub slides: Vec, } -fn open_other(name: &str, bytes: &[u8]) -> Result { +impl Damage { + pub fn is_empty(&self) -> bool { + self.media.is_empty() && self.slides.is_empty() + } +} + +/// [`open_presentation`], plus what the file had lost or damaged. +pub fn open_checked(name: &str, bytes: &[u8]) -> Result<(Presentation, Damage)> { + if deckcraft_format::sniff(bytes) { + let (p, media) = deckcraft_format::load_checked(bytes).map_err(|e| EngineError::Other(e.to_string()))?; + return Ok((p, Damage { media, slides: vec![] })); + } + open_other(name, bytes).map(|(p, slides)| (p, Damage { media: vec![], slides })) +} + +fn open_other(name: &str, bytes: &[u8]) -> Result<(Presentation, Vec)> { if deckcraft_pptx::sniff(bytes) { - let mut p = deckcraft_pptx::import(bytes).map_err(|e| EngineError::Other(e.to_string()))?; + let (mut p, lost_slides) = deckcraft_pptx::import_checked(bytes).map_err(|e| EngineError::Other(e.to_string()))?; deckcraft_format::repair(&mut p); - return Ok(p); + return Ok((p, lost_slides)); } if deckcraft_pptx::is_encrypted(bytes) { return Err(EngineError::Other(format!("{name} is password-protected. DeckCraft can't open password-protected presentations yet."))); @@ -157,7 +173,7 @@ fn open_other(name: &str, bytes: &[u8]) -> Result { if p.slides.is_empty() { p = defaults::new_presentation(None); } - return Ok(p); + return Ok((p, vec![])); } Err(EngineError::Other(format!("{name}: not a presentation DeckCraft can read"))) } @@ -180,8 +196,8 @@ pub struct LoadedFile { 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, - /// Pictures and media a `.deckcraft` file had lost or damaged. - pub damaged: Vec, + /// What the file had lost or damaged. + pub damaged: Damage, } /// Read and parse the presentation at `path`. @@ -202,7 +218,10 @@ fn adopt_loaded(s: &mut Session, f: LoadedFile) -> Result { let i = add_opened(s, &path, keep_path.then(|| path.clone()), doc); report_damage(s, &damaged)?; let st = s.doc()?; - Ok(json!({"document": i, "slides": st.doc.slides.len(), "missingFonts": st.missing_fonts, "damagedMedia": damaged})) + Ok(json!({ + "document": i, "slides": st.doc.slides.len(), "missingFonts": st.missing_fonts, + "damagedMedia": damaged.media, "lostSlides": damaged.slides, + })) } #[cfg(not(target_arch = "wasm32"))] @@ -236,23 +255,35 @@ fn open_bytes(s: &mut Session, p: &Value) -> Result { let (doc, damaged) = open_checked(name, &data)?; let i = add_opened(s, name, None, doc); report_damage(s, &damaged)?; - Ok(json!({"document": i, "missingFonts": s.doc()?.missing_fonts, "damagedMedia": damaged})) + Ok(json!({"document": i, "missingFonts": s.doc()?.missing_fonts, "damagedMedia": damaged.media, "lostSlides": damaged.slides})) } -/// Tell the user which pictures and media a damaged file lost, and mark the copy repaired. -fn report_damage(s: &mut Session, damaged: &[String]) -> Result<()> { +/// Tell the user which pictures, media and slides a damaged file lost, and mark the copy repaired. +fn report_damage(s: &mut Session, damaged: &Damage) -> Result<()> { if damaged.is_empty() { return Ok(()); } let st = s.doc_mut()?; st.title = format!("{} (Repaired)", st.title); - let n = damaged.len(); - let text = format!( - "{n} picture{} or media file{} in this presentation couldn't be read and {} empty. It was opened as a repaired copy; save it under a new name to keep the original file.", - if n == 1 { "" } else { "s" }, - if n == 1 { "" } else { "s" }, - if n == 1 { "is" } else { "are" }, - ); + let mut text = String::new(); + let n = damaged.media.len(); + if n > 0 { + text = format!( + "{n} picture{} or media file{} in this presentation couldn't be read and {} empty. ", + if n == 1 { "" } else { "s" }, + if n == 1 { "" } else { "s" }, + if n == 1 { "is" } else { "are" }, + ); + } + let n = damaged.slides.len(); + if n > 0 { + text.push_str(&format!( + "{n} slide{} in this file couldn't be read and {} left out. ", + if n == 1 { "" } else { "s" }, + if n == 1 { "was" } else { "were" }, + )); + } + text.push_str("It was opened as a repaired copy; save it under a new name to keep the original file."); s.ui_requests.push(crate::UiRequest::Message { text }); Ok(()) } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index a464fd8b..92a243b4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3537,7 +3537,8 @@ fn background_loads_are_adopted_through_a_command() { 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![] }); + let other = + s.stage_loaded(cmd::file::LoadedFile { path: "/elsewhere.pptx".into(), doc: Default::default(), native: true, damaged: Default::default() }); 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()); @@ -3623,3 +3624,38 @@ fn table_column_width_of_a_huge_column_is_ignored() { assert_eq!(table_of(&s, id).cols, vec![100.0, 100.0], "no column changes"); } } + +#[test] +fn opening_a_pptx_with_an_unreadable_slide_says_so() { + use std::io::{Cursor, Read, Write}; + let mut s = session(); + s.execute("slide.new", &json!({"layout": "blank"})).unwrap(); + assert_eq!(s.doc().unwrap().doc.slides.len(), 2); + let good = deckcraft_pptx::export(s.doc().unwrap().doc.as_ref()).unwrap(); + let mut zin = zip::ZipArchive::new(Cursor::new(good.clone())).unwrap(); + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let o = zip::write::SimpleFileOptions::default(); + for i in 0..zin.len() { + let mut f = zin.by_index(i).unwrap(); + let mut d = vec![]; + f.read_to_end(&mut d).unwrap(); + if f.name() == "ppt/slides/slide2.xml" { + d = b" Result { read::import(bytes) } +/// [`import`], plus the part names of slides the file lists but that couldn't be read and were +/// left out. +pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec), PptxError> { + read::import_checked(bytes) +} + /// Read the theme of a `.thmx` file, or of a `.pptx` / `.potx`'s first master, with its media. pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec), PptxError> { read::import_theme(bytes) diff --git a/crates/pptx/src/read/mod.rs b/crates/pptx/src/read/mod.rs index ac83dfc0..979cdb70 100644 --- a/crates/pptx/src/read/mod.rs +++ b/crates/pptx/src/read/mod.rs @@ -387,6 +387,13 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec< } pub fn import(bytes: &[u8]) -> Result { + import_checked(bytes).map(|(p, _)| p) +} + +/// [`import`], plus the parts of the slide list the file names but that couldn't be read (a +/// missing or truncated slide): those slides are left out, so a caller can say so (#203). +pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec), PptxError> { + let mut lost_slides: Vec = vec![]; let pkg = Package::open(bytes)?; let root_rels = pkg.rels(""); let pres_path = root_rels.first_of("officeDocument").map(|r| r.target.clone()).unwrap_or_else(|| "ppt/presentation.xml".into()); @@ -447,6 +454,7 @@ pub fn import(bytes: &[u8]) -> Result { for (file_id, path) in &slide_entries { if !docs.contains_key(&opc::norm(path)) { log::warn!("pptx: slide {path} is missing or unreadable; skipped"); + lost_slides.push(path.clone()); continue; } let id = SlideId(imp.alloc()); @@ -677,7 +685,7 @@ pub fn import(bytes: &[u8]) -> Result { p.media = std::mem::take(&mut imp.media); p.next_id = imp.next; p.fix_next_id(); - Ok(p) + Ok((p, lost_slides)) } /// Embedded font faces in one file. Past this, further typefaces are dropped. diff --git a/crates/pptx/tests/malformed.rs b/crates/pptx/tests/malformed.rs index 633d339e..774a5ee9 100644 --- a/crates/pptx/tests/malformed.rs +++ b/crates/pptx/tests/malformed.rs @@ -264,3 +264,26 @@ fn export_of_odd_models_is_valid() { let q = deckcraft_pptx::import(&deckcraft_pptx::export(&empty).expect("export")).expect("import"); assert!(!q.masters.is_empty()); } + +/// A slide the file lists but that can't be read is left out, and `import_checked` names it +/// where `import` would have said nothing (#203). +#[test] +fn unreadable_slide_is_reported() { + let mut p = defaults::new_presentation(None); + let mut second = p.slides.first().expect("a slide").as_ref().clone(); + second.id = deckcraft_model::SlideId(second.id.0 + 1000); + p.slides.push(std::sync::Arc::new(second)); + let bytes = deckcraft_pptx::export(&p).expect("export"); + + let (q, lost) = deckcraft_pptx::import_checked(&bytes).expect("import"); + assert_eq!((q.slides.len(), lost.len()), (2, 0), "{lost:?}"); + + let mut es = entries(&bytes); + let slide2 = es.iter_mut().find(|(n, _)| n == "ppt/slides/slide2.xml").expect("slide2"); + slide2.1 = b" **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** major (community PR wave 2026-10-14: seven owned PPTX corpus files, embedded fonts, EMF/WMF clip regions, threaded comments (#70), chart trendlines and workbooks (#335), SmartArt written as a diagram, tags and custom XML kept, picture export DPI; duplicated rows and headers folded; previous: trivial, lines in scaled groups, #233) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-15 · **Change:** major (community PR wave 2026-10-14: seven owned PPTX corpus files, embedded fonts, EMF/WMF clip regions, threaded comments (#70), chart trendlines and workbooks (#335), SmartArt written as a diagram, tags and custom XML kept, picture export DPI; duplicated rows and headers folded; previous: trivial, lines in scaled groups, #233) · **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** @@ -104,6 +104,7 @@ the round trip even though DeckCraft doesn't model it. | Date | Change | Summary | |---|---|---| | 2026-10-15 | trivial | PPTX interactive timing sequences keep later On Click steps and paragraph click builds (`trigger_steps.rs`); mixed starts and independent buttons survive import/edit/save/reopen | +| 2026-10-15 | minor | A `.pptx` slide that can't be read is reported instead of dropped in silence: `file.open` and `file.openBytes` return `lostSlides`, the app shows a message and opens a repaired copy (Save asks for a new name), and `deckcraft-cli` warns on stderr (#203) | | 2026-10-14 | minor | Community PR wave folded: one row each for `.pptx` (seven owned corpus files), outline, 3-D models, embedded fonts, text, effects, 3-D (extrusion sides draw now), SmartArt (written as a diagram) and VBA (tags and custom XML are kept) | | 2026-10-12 | minor | SmartArt import lays each shape's text out on its `dsp:txXfrm` text rectangle instead of the shape box | | 2026-10-13 | minor | EMF/WMF rectangle clip regions limit later drawing; EMF+ records remain |