From 2f403698e0e2a8db6b404afe30292f3e3d15ce81 Mon Sep 17 00:00:00 2001 From: Hamza Date: Wed, 28 Oct 2026 18:53:45 +0500 Subject: [PATCH 1/3] An export says what text its pages had no room for (#1465) A page-based export leaves out the text its frames cannot fit, and said nothing about it: an imported 58-page form lost whole sentences -- a data-consent clause, a statutory rule, the last row of two tables -- with `warnings` empty while preflight reported 19 overset stories. The PDF and deck exports now report it, in the words preflight uses and with the same count, naming the page and the frame. A reflowable export carries the whole story, so it has nothing to report. Carries the review fix: a spread export puts the whole spread on the sheet, so choosing page 2 of a facing-pages document exports page 3 as well and page 3's overset is as absent from the file as page 2's would be. The selection is widened to the spreads it touches first. A page on another spread is still not reported, so the filter keeps doing what it is for. Rebased onto the post-split main: the PDF command is in export/pdf.rs, the deck one in export/formats.rs, and preflight.rs is unchanged by the split. The helper sits before export_pdf rather than between first_page_ref's doc comment and its function, which is where the pre-split patch had put it. --- crates/engine/src/cmd/export/formats.rs | 2 + crates/engine/src/cmd/export/pdf.rs | 123 +++++++++++++++++++++++- crates/engine/src/cmd/preflight.rs | 45 +++++++++ crates/engine/src/cmd/text.rs | 7 +- 4 files changed, 175 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/export/formats.rs b/crates/engine/src/cmd/export/formats.rs index 025bcdac4..dab0bb8d1 100644 --- a/crates/engine/src/cmd/export/formats.rs +++ b/crates/engine/src/cmd/export/formats.rs @@ -110,6 +110,8 @@ pub(super) fn export_pptx(s: &mut Session, p: &Value) -> Result { if rasterized > 0 { r.warnings.push(format!("{rasterized} object(s) went in as images")); } + // A slide has no room for the overset text either (#1465). + r.warnings.extend(crate::cmd::preflight::overset_warnings(s, opts.pages.as_deref())); match str_param(p, "path") { Some(path) => { #[cfg(not(target_arch = "wasm32"))] diff --git a/crates/engine/src/cmd/export/pdf.rs b/crates/engine/src/cmd/export/pdf.rs index 64011fa3c..12d5577ec 100644 --- a/crates/engine/src/cmd/export/pdf.rs +++ b/crates/engine/src/cmd/export/pdf.rs @@ -87,8 +87,27 @@ fn first_page_ref(pdf: &[u8]) -> Option { s[i..].split_whitespace().next()?.parse().ok() } +/// Every page sharing a spread with one of `pages` (0-based), for an export that puts whole +/// spreads on a sheet. The pages come back in document order. +fn with_their_spreads(s: &Session, pages: &[usize]) -> Vec { + let Some(st) = s.active() else { return pages.to_vec() }; + let d = &st.doc; + let spreads: std::collections::BTreeSet = pages.iter().filter_map(|p| d.page_loc(*p)).map(|(si, _)| si).collect(); + if spreads.is_empty() { + return pages.to_vec(); + } + (0..d.page_count()).filter(|p| d.page_loc(*p).is_some_and(|(si, _)| spreads.contains(&si))).collect() +} + pub(super) fn export_pdf(s: &mut Session, p: &Value) -> Result { - let r = pdf_export(s, p)?; + let mut r = pdf_export(s, p)?; + // Text the pages have no room for is not in the file: say so, as preflight does (#1465). + // A spread export puts the whole spread on the sheet, so choosing page 2 exports page 3 as + // well and page 3's overset is as absent from the file as page 2's would be. The selection + // is widened to the spreads it touches before the warnings are collected (noted in review). + let opts = options(p, s.doc()?.doc.page_count())?; + let warn_pages = opts.pages.as_deref().map(|pages| if opts.spreads { with_their_spreads(s, pages) } else { pages.to_vec() }); + r.warnings.extend(crate::cmd::preflight::overset_warnings(s, warn_pages.as_deref())); match str_param(p, "path") { Some(path) => { write_file(path, &r.bytes)?; @@ -461,3 +480,105 @@ mod page_list_tests { assert!(s.execute("file.exportPdf", &json!({"pages": []})).is_err()); } } + +#[cfg(test)] +mod overset_tests { + use serde_json::json; + + use crate::Session; + + /// A page-based export leaves out the text its frames have no room for, and said nothing + /// about it: an imported 58-page form lost whole sentences — a data-consent clause, a + /// statutory rule, the last row of two tables — with `warnings` empty while preflight + /// reported 19 overset stories (#1465). + #[test] + fn an_export_warns_about_the_text_the_pages_had_no_room_for() { + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 300, "height": 200, "pages": 2, "facingPages": false, "margins": 0})).unwrap(); + let text = "First line is visible.\nSecond paragraph does not fit: this sentence carries the legal notice and must not vanish from the PDF without a warning."; + let frame = s.execute("frame.create", &json!({"rect": [20, 20, 280, 60], "content": "text", "text": text})).unwrap(); + let (id, sid) = (frame["id"].as_u64().unwrap(), frame["story"].clone()); + + // What preflight says about the same frame, for the warning to agree with. + let issues = s.execute("preflight.run", &json!({})).unwrap(); + let overset = issues["issues"].as_array().unwrap().iter().find(|i| i["kind"] == "overset").expect("preflight reports it").clone(); + assert_eq!(overset["message"], "Overset text: 76 characters"); + + for command in ["file.exportPdf", "file.exportPptx"] { + let r = s.execute(command, &json!({})).unwrap(); + let warnings = r["warnings"].as_array().expect("warnings").clone(); + let about: Vec<&str> = warnings.iter().filter_map(|w| w.as_str()).filter(|w| w.contains("not in the export")).collect(); + assert_eq!(about.len(), 1, "{command}: {warnings:?}"); + let w = about[0]; + assert!(w.contains("page 1"), "{command}: the page it is on: {w}"); + assert!(w.contains("76 character"), "{command}: the count preflight gives: {w}"); + assert!(w.contains(&format!("frame {id}")), "{command}: the frame: {w}"); + } + + // The pages being exported are the ones reported on: page 2 has no overset frame. + let r = s.execute("file.exportPdf", &json!({"pages": [2]})).unwrap(); + assert_eq!(r["warnings"], json!([]), "{r}"); + + // A reflowable export carries the whole story, so it has nothing to report. + let r = s.execute("file.exportHtml", &json!({})).unwrap(); + assert_eq!(r["warnings"], json!([]), "{r}"); + assert!(r["text"].as_str().unwrap().contains("must not vanish"), "the HTML has the text the PDF left out"); + + // `overset` is the index where the text stops fitting; `oversetChars` is how much is left, + // which is the number preflight reports. The two were read as the same thing. + let story = s.execute("story.get", &json!({"story": sid})).unwrap(); + assert_eq!(story["overset"], json!(68), "{story}"); + assert_eq!(story["oversetChars"], json!(76), "{story}"); + + // A frame the text fits: nothing is reported anywhere. + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 300, "height": 200, "pages": 1, "facingPages": false, "margins": 0})).unwrap(); + let frame = s.execute("frame.create", &json!({"rect": [20, 20, 280, 180], "content": "text", "text": "Short."})).unwrap(); + for command in ["file.exportPdf", "file.exportPptx"] { + assert_eq!(s.execute(command, &json!({})).unwrap()["warnings"], json!([]), "{command}"); + } + let story = s.execute("story.get", &json!({"story": frame["story"].clone()})).unwrap(); + assert!(story["overset"].is_null() && story["oversetChars"].is_null(), "{story}"); + } + + /// A spread export puts the whole spread on the sheet, so choosing one page of it exports its + /// partner too -- and that partner's overset is just as absent from the file. Reporting only + /// the pages that were named left it unmentioned (noted in review of #1465). + #[test] + fn a_spread_export_warns_about_the_pages_it_takes_with_it() { + let mut s = Session::new(); + // Facing pages: 2 and 3 share a spread, 1 is the cover on its own. + s.execute("file.new", &json!({"width": 200, "height": 200, "pages": 4, "facingPages": true, "margins": 0})).unwrap(); + let text = "First line is visible.\nSecond paragraph does not fit: this sentence carries the legal notice and must not vanish from the PDF without a warning."; + // A frame on page 3, whose text does not fit it. `frame.create` takes a spread and + // spread coordinates, so page 3's own bounds say where to put it. + let (si, b) = { + let d = &s.active().expect("a document").doc; + let (si, pi) = d.page_loc(2).expect("page 3 is there"); + (si, d.spreads[si].pages[pi].bounds()) + }; + let rect = json!([b.x0 + 20.0, b.y0 + 20.0, b.x0 + 180.0, b.y0 + 60.0]); + let frame = s.execute("frame.create", &json!({"spread": si, "rect": rect, "content": "text", "text": text})).unwrap(); + let id = frame["id"].as_u64().unwrap(); + let about = |r: &serde_json::Value| -> Vec { + r["warnings"] + .as_array() + .map(|w| w.iter().filter_map(|w| w.as_str()).filter(|w| w.contains("not in the export")).map(str::to_string).collect()) + .unwrap_or_default() + }; + + // Page by page, choosing page 2 reports nothing: page 2 holds no overset frame. + let r = s.execute("file.exportPdf", &json!({"pages": [2]})).unwrap(); + assert!(about(&r).is_empty(), "page 2 alone has nothing to report: {r}"); + + // As spreads, choosing page 2 exports page 3 with it, so page 3's overset is reported. + let r = s.execute("file.exportPdf", &json!({"pages": [2], "spreads": true})).unwrap(); + let w = about(&r); + assert_eq!(w.len(), 1, "the page that came along is reported too: {r}"); + assert!(w[0].contains("page 3") && w[0].contains(&format!("frame {id}")), "{}", w[0]); + + // And a page on another spread entirely is still not reported. + let r = s.execute("file.exportPdf", &json!({"pages": [1], "spreads": true})).unwrap(); + assert!(about(&r).is_empty(), "the cover's spread holds no overset frame: {r}"); + } +} diff --git a/crates/engine/src/cmd/preflight.rs b/crates/engine/src/cmd/preflight.rs index 390dc7118..b3a22d6ad 100644 --- a/crates/engine/src/cmd/preflight.rs +++ b/crates/engine/src/cmd/preflight.rs @@ -113,6 +113,51 @@ fn overset_location(s: &Session, target: designcraft_doc::ItemId) -> Option<(des None } +/// Text that is in the document but not in a page-based export (PDF, PowerPoint): the overset +/// part of a story, and a story with no frame at all. The export used to write the file and +/// report nothing, so whole sentences left an imported document without a hint anywhere (#1465). +/// +/// One line per story, naming the page and the frame as the preflight issue does. `pages` are the +/// 0-based pages being exported; a story whose overset is on a page that is not being exported is +/// not reported, and one with no page of its own always is. A reflowable export (EPUB, HTML) or a +/// text export carries the whole story, so neither calls this. +pub fn overset_warnings(s: &Session, pages: Option<&[usize]>) -> Vec { + let Some(st) = s.active() else { return vec![] }; + let d = &st.doc; + let mut out = Vec::new(); + for story in d.stories.values() { + let cs = s.cache.get(d, story.id, None); + if !cs.is_overset() { + continue; + } + let n = story.text.get(cs.overset_at.unwrap_or(0)..).unwrap_or("").chars().count(); + if n == 0 { + continue; + } + if story.frames.is_empty() { + out.push(format!("a story with no text frame: {n} character(s) of it are not in the export")); + continue; + } + let last = story.frames.last().copied(); + let location = last.and_then(|f| overset_location(s, f)); + let page = location.and_then(|(_, page)| page); + if let (Some(page), Some(pages)) = (page, pages) + && !pages.contains(&page) + { + continue; + } + let item = location.map(|(item, _)| item).or(last); + out.push(match (page, item) { + (Some(page), Some(item)) => { + format!("page {}: {n} character(s) of the story in frame {} do not fit it and are not in the export", page + 1, item.0) + } + (None, Some(item)) => format!("the story in frame {}: {n} character(s) do not fit and are not in the export", item.0), + _ => format!("{n} character(s) of a story do not fit their frame and are not in the export"), + }); + } + out +} + pub fn check(s: &Session, min_ppi: f64) -> Vec { let Some(st) = s.active() else { return vec![] }; let d = &st.doc; diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 8e6038741..3e6dce9d5 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -191,12 +191,17 @@ pub fn specs() -> Vec { }) } ), - cmd!(query "story.get", "Get Story", [], None, "{story? | frame?} → text, frames, paragraphs, vertical, overset", has_doc, |s, p| { + cmd!(query "story.get", "Get Story", [], None, "{story? | frame?} → text, frames, paragraphs, vertical, lines, overset (the index in `text` where it stops fitting its frames, null when it all fits) and oversetChars (how many characters that leaves, as preflight reports them)", has_doc, |s, p| { let st = s.doc()?; let sid = story_of(s, p).ok_or_else(|| bad("story.get", "no story"))?; let mut v = st.doc.story_summary(sid).ok_or(designcraft_doc::DocError::NoStory(sid))?; let cs = s.cache.get(&st.doc, sid, None); + // `overset` is where the text stops fitting, not how much is left over: the two were + // read as the same number and disagreed with preflight's count (#1465). v["overset"] = json!(cs.overset_at); + v["oversetChars"] = json!(cs + .overset_at + .map(|at| st.doc.stories.get(&sid).and_then(|story| story.text.get(at..)).unwrap_or("").chars().count())); v["lines"] = json!(cs.line_count()); Ok(v) }), From 5a53a3807c08d156722ba8dce3f034d03b411181 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 29 Oct 2026 14:17:21 +0500 Subject: [PATCH 2/3] Check a parent page's frame on the pages that show it The warnings composed every story once with no page. For a frame on a parent page that is wrong twice over, and I measured both: exportPdf {"pages": [2]} ["the story in frame 9: 76 character(s) do not fit and are not in the export"] Page 2 used no parent at all, so nothing of that frame was in its export to be missing -- and the warning named no page, because a parent frame has no document page of its own. It now resolves the pages that draw the parent page and reports only those: {"pages": [1]} ["page 1: 76 character(s) ... frame 9 ..."] {"pages": [2]} [] The second half is page-dependent text. A parent frame 112 pt wide holding "Continued on page " fits while the number is one digit and does not once it is two, so composing it once could not tell: {"pages": [1]} [] {"pages": [9]} [] {"pages": [10]} ["page 10: 1 character(s) ..."] {} ["pages 10, 11, 12: 1 character(s) ..."] Each page that shows the parent is composed as that page, and the pages are grouped by how much they lose, so a story still gets one line. Also from review: `story.get` now says `overset` is a UTF-8 byte offset into `text`, not a character index. A test measures the difference on French text, where the offset runs ahead of the character count. --- crates/engine/src/cmd/export/pdf.rs | 70 +++++++++++++++++++++++++++++ crates/engine/src/cmd/preflight.rs | 63 +++++++++++++++++++++++++- crates/engine/src/cmd/text.rs | 2 +- 3 files changed, 132 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/export/pdf.rs b/crates/engine/src/cmd/export/pdf.rs index 12d5577ec..b5f16c110 100644 --- a/crates/engine/src/cmd/export/pdf.rs +++ b/crates/engine/src/cmd/export/pdf.rs @@ -541,6 +541,76 @@ mod overset_tests { assert!(story["overset"].is_null() && story["oversetChars"].is_null(), "{story}"); } + /// `overset` indexes `text` in UTF-8 bytes, not characters: a client that slices a JavaScript + /// string at it cuts the wrong place once the text is not ASCII (noted in review). + #[test] + fn overset_is_a_byte_offset_into_the_text() { + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 300, "height": 200, "pages": 1, "facingPages": false, "margins": 0})).unwrap(); + // Accented letters are two bytes each, so the byte offset runs ahead of the character index. + let text = "Réservé à l'évaluation détaillée des éléments.\nCette phrase dépasse le cadre et n'est pas dans le PDF exporté du tout."; + let frame = s.execute("frame.create", &json!({"rect": [20, 20, 280, 40], "content": "text", "text": text})).unwrap(); + let story = s.execute("story.get", &json!({"story": frame["story"].clone()})).unwrap(); + let at = story["overset"].as_u64().expect("it oversets") as usize; + assert!(text.is_char_boundary(at), "a byte offset that splits no character: {at}"); + assert!(at > text[..at].chars().count(), "ahead of the character index: {at} bytes, {} characters", text[..at].chars().count()); + // And `oversetChars` counts characters, which is why the two numbers differ. + assert_eq!(story["oversetChars"], json!(text[at..].chars().count()), "{story}"); + } + + /// A parent page's frame is composed once per page that shows it: its page number marker is + /// that page's, so the same frame fits on page 1 and oversets on page 10 -- and a parent page + /// no exported page uses puts nothing in the file to be missing. Composing it once with no + /// page reported neither: every selection got the same unpaged warning (noted in review). + #[test] + fn a_parent_frame_is_checked_on_the_pages_that_show_it() { + let about = |r: &serde_json::Value| -> Vec { + r["warnings"] + .as_array() + .map(|w| w.iter().filter_map(|w| w.as_str()).filter(|w| w.contains("not in the export")).map(str::to_string).collect()) + .unwrap_or_default() + }; + + // A parent page that only page 1 uses. + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 300, "height": 200, "pages": 2, "facingPages": false, "margins": 0})).unwrap(); + s.execute("layout.pages.applyParent", &json!({"pages": [1], "parent": null})).unwrap(); + let text = "First line is visible.\nSecond paragraph does not fit: this sentence carries the legal notice and must not vanish from the PDF without a warning."; + let frame = s + .execute("frame.create", &json!({"spread": {"kind": "parent", "index": 0}, "rect": [20, 20, 280, 60], "content": "text", "text": text})) + .unwrap(); + let id = frame["id"].as_u64().unwrap(); + + let w = about(&s.execute("file.exportPdf", &json!({"pages": [1]})).unwrap()); + assert_eq!(w.len(), 1, "page 1 shows the parent: {w:?}"); + assert!(w[0].contains("page 1") && w[0].contains(&format!("frame {id}")) && w[0].contains("76 character"), "{}", w[0]); + + // Page 2 has no parent, so nothing of that frame is in its export to be missing. + let r = s.execute("file.exportPdf", &json!({"pages": [2]})).unwrap(); + assert!(about(&r).is_empty(), "page 2 does not use the parent: {r}"); + + // Page-dependent text: the marker is one digit up to page 9 and two from page 10, and the + // wider line no longer fits. 112 pt is the width where that is the only difference. + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 300, "height": 200, "pages": 12, "facingPages": false, "margins": 0})).unwrap(); + let text = format!("Continued on page {}", designcraft_doc::story::PAGE_NUMBER); + s.execute("frame.create", &json!({"spread": {"kind": "parent", "index": 0}, "rect": [20, 20, 132, 36], "content": "text", "text": text})) + .unwrap(); + + for page in [1, 9] { + let r = s.execute("file.exportPdf", &json!({"pages": [page]})).unwrap(); + assert!(about(&r).is_empty(), "page {page} fits one digit: {r}"); + } + let w = about(&s.execute("file.exportPdf", &json!({"pages": [10]})).unwrap()); + assert_eq!(w.len(), 1, "two digits do not fit: {w:?}"); + assert!(w[0].contains("page 10"), "{}", w[0]); + + // The whole document: one line naming every page it happens on, not one line per page. + let w = about(&s.execute("file.exportPdf", &json!({})).unwrap()); + assert_eq!(w.len(), 1, "one line for the story: {w:?}"); + assert!(w[0].starts_with("pages 10, 11, 12:"), "{}", w[0]); + } + /// A spread export puts the whole spread on the sheet, so choosing one page of it exports its /// partner too -- and that partner's overset is just as absent from the file. Reporting only /// the pages that were named left it unmentioned (noted in review of #1465). diff --git a/crates/engine/src/cmd/preflight.rs b/crates/engine/src/cmd/preflight.rs index b3a22d6ad..bf98f9ab9 100644 --- a/crates/engine/src/cmd/preflight.rs +++ b/crates/engine/src/cmd/preflight.rs @@ -121,16 +121,76 @@ fn overset_location(s: &Session, target: designcraft_doc::ItemId) -> Option<(des /// 0-based pages being exported; a story whose overset is on a page that is not being exported is /// not reported, and one with no page of its own always is. A reflowable export (EPUB, HTML) or a /// text export carries the whole story, so neither calls this. +/// Pages a single warning names before "and N more". +const MAX_WARNED_PAGES: usize = 10; + +/// How the pages one warning covers are named: "page 3", "pages 3, 4", "pages 3, 4 and 6 more". +fn page_list(pages: &[usize]) -> String { + let shown: Vec = pages.iter().take(MAX_WARNED_PAGES).map(|p| (p + 1).to_string()).collect(); + let rest = pages.len() - shown.len(); + let list = shown.join(", "); + match (pages.len(), rest) { + (1, _) => format!("page {list}"), + (_, 0) => format!("pages {list}"), + _ => format!("pages {list} and {rest} more"), + } +} + +/// The parent spread and the page of it a frame sits on, or `None` for a frame on a document page. +fn parent_page_of(d: &Document, target: designcraft_doc::ItemId) -> Option<(usize, usize)> { + let loc = d.find(target)?; + let SpreadRef::Parent(pi) = loc.spread else { return None }; + let bounds = d.parent_xf(&loc).transform_rect_bbox(d.item(target)?.bounds()); + Some((pi, d.spread(loc.spread)?.page_at_x(bounds.center().x).unwrap_or(0))) +} + +/// Document pages that draw parent page `pp` of parent spread `pi`, in document order. +fn pages_showing(d: &Document, (pi, pp): (usize, usize)) -> Vec { + (0..d.page_count()) + .filter(|p| d.page_loc(*p).is_some_and(|(si, pgi)| d.shown_parent_pages(SpreadRef::Doc(si), pgi).contains(&(pi, pp)))) + .collect() +} + +/// Characters of `story` that `cs` had no room for. +fn overset_chars(story: &designcraft_doc::Story, cs: &ComposedStory) -> usize { + story.text.get(cs.overset_at.unwrap_or(0)..).unwrap_or("").chars().count() +} + pub fn overset_warnings(s: &Session, pages: Option<&[usize]>) -> Vec { let Some(st) = s.active() else { return vec![] }; let d = &st.doc; let mut out = Vec::new(); for story in d.stories.values() { + let last = story.frames.last().copied(); + // A story on a parent page is composed once per page that shows it: its page number + // markers and variables are that page's, so the same frame can fit on one page and + // overset on another, and a parent page no exported page uses puts nothing in the file + // to be missing. Composing it once with no page reported neither (noted in review). + if let Some(parent) = last.and_then(|f| parent_page_of(d, f)) { + let mut by_count: std::collections::BTreeMap> = std::collections::BTreeMap::new(); + for page in pages_showing(d, parent) { + if pages.is_some_and(|sel| !sel.contains(&page)) { + continue; + } + let cs = s.cache.get(d, story.id, Some(page)); + let n = overset_chars(story, &cs); + if cs.is_overset() && n > 0 { + by_count.entry(n).or_default().push(page); + } + } + let item = last.map_or(0, |f| f.0); + out.extend( + by_count.into_iter().map(|(n, ps)| { + format!("{}: {n} character(s) of the story in frame {item} do not fit it and are not in the export", page_list(&ps)) + }), + ); + continue; + } let cs = s.cache.get(d, story.id, None); if !cs.is_overset() { continue; } - let n = story.text.get(cs.overset_at.unwrap_or(0)..).unwrap_or("").chars().count(); + let n = overset_chars(story, &cs); if n == 0 { continue; } @@ -138,7 +198,6 @@ pub fn overset_warnings(s: &Session, pages: Option<&[usize]>) -> Vec { out.push(format!("a story with no text frame: {n} character(s) of it are not in the export")); continue; } - let last = story.frames.last().copied(); let location = last.and_then(|f| overset_location(s, f)); let page = location.and_then(|(_, page)| page); if let (Some(page), Some(pages)) = (page, pages) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 3e6dce9d5..01abf0e6e 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -191,7 +191,7 @@ pub fn specs() -> Vec { }) } ), - cmd!(query "story.get", "Get Story", [], None, "{story? | frame?} → text, frames, paragraphs, vertical, lines, overset (the index in `text` where it stops fitting its frames, null when it all fits) and oversetChars (how many characters that leaves, as preflight reports them)", has_doc, |s, p| { + cmd!(query "story.get", "Get Story", [], None, "{story? | frame?} → text, frames, paragraphs, vertical, lines, overset (the UTF-8 byte offset into `text` where it stops fitting its frames, not a character index, null when it all fits) and oversetChars (how many characters that leaves, as preflight reports them)", has_doc, |s, p| { let st = s.doc()?; let sid = story_of(s, p).ok_or_else(|| bad("story.get", "no story"))?; let mut v = st.doc.story_summary(sid).ok_or(designcraft_doc::DocError::NoStory(sid))?; From 2d5af1bd0d540995ad3f9892c2d71d7c1e23b847 Mon Sep 17 00:00:00 2001 From: Hamza Date: Mon, 2 Nov 2026 18:40:39 +0500 Subject: [PATCH 3/3] A frame that crosses into an exported page is checked on it A sheet draws its spread's items without asking which page each one sits on, so a frame straddling two pages is drawn, in part, on both. The warning chose the page from the frame's centre alone (`page_at_x(bounds.center().x)`), so a frame centred on a page the export leaves out had its warning dropped while the part on an exported page went into the file (noted in review). `pages_of` gives every page of the spread the frame's bounds overlap, and the story is kept when any of them is exported. The page named is one the export has: the centre page when that is exported, so an ordinary frame reads exactly as before, otherwise the first exported page the frame reaches. A frame wholly on the pasteboard overlaps no page's span and falls back to `page_of`, so the two cannot disagree about where such a frame is. `overset_location` returns the reached pages beside the centre page, which is what made it worth a named type. The test exports a frame centred on page 3 and reaching back into page 2, four ways: the whole document, page 3 alone, page 2 alone -- which used to report nothing -- and page 4, which the frame never reaches and which still reports nothing. --- crates/engine/src/cmd/export/pdf.rs | 44 +++++++++++++++++++ crates/engine/src/cmd/preflight.rs | 68 +++++++++++++++++++++++------ 2 files changed, 99 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/cmd/export/pdf.rs b/crates/engine/src/cmd/export/pdf.rs index b5f16c110..3466a18a3 100644 --- a/crates/engine/src/cmd/export/pdf.rs +++ b/crates/engine/src/cmd/export/pdf.rs @@ -541,6 +541,50 @@ mod overset_tests { assert!(story["overset"].is_null() && story["oversetChars"].is_null(), "{story}"); } + /// A sheet draws its spread's items without asking which page each one sits on, so a frame + /// straddling two pages is drawn, in part, on both. Choosing the page by the frame's centre + /// alone meant a frame centred on a page the export leaves out had its warning dropped while + /// the part on an exported page went into the file (#1493 review). + #[test] + fn an_export_warns_about_a_frame_that_only_crosses_into_one_of_its_pages() { + let mut s = Session::new(); + // Facing pages, so pages 2 and 3 are one spread and a frame can straddle their boundary. + s.execute("file.new", &json!({"width": 300, "height": 200, "pages": 4, "facingPages": true, "margins": 0})).unwrap(); + let text = "First line is visible.\nSecond paragraph does not fit: this sentence carries the legal notice and must not vanish from the PDF without a warning."; + // Spread 1 is pages 2-3; its page 2 runs x 0..300 and page 3 runs x 300..600. The frame + // is centred on page 3 and reaches back over the boundary into page 2. + let frame = json!({"spread": 1, "rect": [260, 20, 420, 60], "content": "text", "text": text}); + let id = s.execute("frame.create", &frame).unwrap()["id"].as_u64().unwrap(); + + let about = |s: &mut Session, pages: serde_json::Value| { + let params = if pages.is_null() { json!({}) } else { json!({"pages": pages}) }; + let r = s.execute("file.exportPdf", ¶ms).unwrap(); + let ws = r["warnings"].as_array().cloned().unwrap_or_default(); + ws.iter().filter_map(|w| w.as_str()).filter(|w| w.contains("not in the export")).map(str::to_owned).collect::>() + }; + + // The whole document: reported once, at the page the frame is centred on. + let all = about(&mut s, serde_json::Value::Null); + assert_eq!(all.len(), 1, "{all:?}"); + assert!(all[0].contains("page 3") && all[0].contains(&format!("frame {id}")), "{}", all[0]); + + // Exporting page 3 alone: the centre page, reported as before. + let three = about(&mut s, json!([3])); + assert_eq!(three.len(), 1, "{three:?}"); + assert!(three[0].contains("page 3"), "{}", three[0]); + + // Exporting page 2 alone: the frame's centre is on page 3, which is not in the export -- + // but part of the frame is drawn on page 2, so it is still reported, and the page named is + // one the export has. + let two = about(&mut s, json!([2])); + assert_eq!(two.len(), 1, "a frame crossing into page 2 is reported: {two:?}"); + assert!(two[0].contains("page 2"), "and the page it names is an exported one: {}", two[0]); + assert!(two[0].contains(&format!("frame {id}")), "{}", two[0]); + + // A page the frame does not reach at all: nothing to say. + assert_eq!(about(&mut s, json!([4])), Vec::::new()); + } + /// `overset` indexes `text` in UTF-8 bytes, not characters: a client that slices a JavaScript /// string at it cuts the wrong place once the text is not ASCII (noted in review). #[test] diff --git a/crates/engine/src/cmd/preflight.rs b/crates/engine/src/cmd/preflight.rs index bf98f9ab9..45fb8bc40 100644 --- a/crates/engine/src/cmd/preflight.rs +++ b/crates/engine/src/cmd/preflight.rs @@ -38,6 +38,25 @@ fn page_of(d: &Document, sr: SpreadRef, it: &Item, parent: Affine) -> Option Vec { + let SpreadRef::Doc(si) = sr else { return vec![] }; + let Some(sp) = d.spreads.get(si) else { return vec![] }; + let bounds = parent.transform_rect_bbox(it.bounds()); + let first = d.first_page_of_spread(si); + let touched: Vec = + sp.pages.iter().enumerate().filter(|(_, pg)| bounds.x1 > pg.x && bounds.x0 < pg.x + pg.width).map(|(i, _)| first + i).collect(); + // A frame wholly on the pasteboard touches no page's span; it still belongs to the page + // `page_at_x` nominates, so the two functions agree about where it is. + if touched.is_empty() { page_of(d, sr, it, parent).into_iter().collect() } else { touched } +} + /// Characters a missing-glyph report names before "and N more". const MISSING_GLYPHS_LISTED: usize = 20; @@ -74,11 +93,19 @@ fn missing_glyphs( } } -/// Locate an embedded frame at the outer page frame that actually renders it. -fn overset_location(s: &Session, target: designcraft_doc::ItemId) -> Option<(designcraft_doc::ItemId, Option)> { +/// Locate an embedded frame at the outer page frame that actually renders it: the frame, the page +/// its centre is on, and every page it reaches (see [`pages_of`]). +type Located = (designcraft_doc::ItemId, Option, Vec); + +fn overset_location(s: &Session, target: designcraft_doc::ItemId) -> Option { let d = &s.active()?.doc; + let at = |id: designcraft_doc::ItemId, loc: &designcraft_doc::ItemLoc| { + let it = d.item(id); + let xf = d.parent_xf(loc); + (id, it.and_then(|it| page_of(d, loc.spread, it, xf)), it.map(|it| pages_of(d, loc.spread, it, xf)).unwrap_or_default()) + }; if let Some(loc) = d.find(target) { - return Some((target, d.item(target).and_then(|it| page_of(d, loc.spread, it, d.parent_xf(&loc))))); + return Some(at(target, &loc)); } fn contains(root: &designcraft_compose::FrameText, target: designcraft_doc::ItemId) -> bool { let mut frames = vec![root]; @@ -107,7 +134,7 @@ fn overset_location(s: &Session, target: designcraft_doc::ItemId) -> Option<(des let frame = cs.frames.iter().find(|f| contains(f, current))?; current = frame.frame; if let Some(loc) = d.find(current) { - return Some((current, d.item(current).and_then(|it| page_of(d, loc.spread, it, d.parent_xf(&loc))))); + return Some(at(current, &loc)); } } None @@ -199,13 +226,28 @@ pub fn overset_warnings(s: &Session, pages: Option<&[usize]>) -> Vec { continue; } let location = last.and_then(|f| overset_location(s, f)); - let page = location.and_then(|(_, page)| page); - if let (Some(page), Some(pages)) = (page, pages) - && !pages.contains(&page) - { - continue; - } - let item = location.map(|(item, _)| item).or(last); + let centre = location.as_ref().and_then(|(_, page, _)| *page); + let touched: &[usize] = location.as_ref().map(|(_, _, ps)| ps.as_slice()).unwrap_or_default(); + // A sheet draws its spread's items without asking which page they are on, so a frame + // straddling two pages is drawn in part on both. Testing the centre page alone dropped + // the warning for a frame centred on a page the export leaves out while the part on an + // exported page went into the file (noted in review). + // + // The page named is one the export has: the centre page when that is exported, so an + // ordinary frame reads as before, otherwise the first exported page the frame reaches. + let page = match pages { + None => centre, + Some(sel) => { + let exported: Vec = touched.iter().copied().filter(|p| sel.contains(p)).collect(); + match centre.filter(|p| sel.contains(p)).or_else(|| exported.first().copied()) { + Some(p) => Some(p), + // Nothing of this frame is on an exported page. + None if centre.is_some() || !touched.is_empty() => continue, + None => None, + } + } + }; + let item = location.as_ref().map(|(item, _, _)| *item).or(last); out.push(match (page, item) { (Some(page), Some(item)) => { format!("page {}: {n} character(s) of the story in frame {} do not fit it and are not in the export", page + 1, item.0) @@ -242,8 +284,8 @@ pub fn check(s: &Session, min_ppi: f64) -> Vec { } else { format!("Overset text: {n} characters") }, - item: location.map(|(item, _)| item.0).or(last.map(|i| i.0)), - page: location.and_then(|(_, page)| page), + item: location.as_ref().map(|(item, _, _)| item.0).or(last.map(|i| i.0)), + page: location.as_ref().and_then(|(_, page, _)| *page), }); } }