From a21e8db14fa24df3fc6136ccb2ec93d8aebbdb1d Mon Sep 17 00:00:00 2001 From: seanth Date: Sun, 11 Oct 2026 23:27:52 -0400 Subject: [PATCH] fix(compose): key parent-page compositions by absolute page index Parent-page text now uses the absolute page index as its cache key, so repeated page numbers and renumbering do not reuse another page's composition. Validation: targeted canvas and PDF regressions, plus all six cargo xtask ci gates on main after #514 (1,179 native tests passed, 3 ignored; 15 crates passed WASM compilation checks). Task: #523 --- crates/compose/src/cache.rs | 32 ++++++++++------ crates/engine/src/cmd/variables.rs | 59 +++++++++++++++++++++++++++++- crates/pdf/src/export.rs | 31 ++++++++-------- crates/render/src/fx.rs | 8 ++-- crates/render/src/lib.rs | 27 +++++++------- 5 files changed, 111 insertions(+), 46 deletions(-) diff --git a/crates/compose/src/cache.rs b/crates/compose/src/cache.rs index 6d6b1b9..8aabbf0 100644 --- a/crates/compose/src/cache.rs +++ b/crates/compose/src/cache.rs @@ -13,7 +13,8 @@ use designcraft_doc::{Document, Item, Story, StoryId, Styles, WrapMode}; use crate::vars::RunningIndex; use crate::{ComposeOptions, ComposedStory}; -type Key = (StoryId, Option); +/// A story, composed for one absolute page (parent-page items) or for its own frames (`None`). +type Key = (StoryId, Option); struct Entry { sig: Vec, @@ -55,11 +56,23 @@ impl Cache { Self::default() } - /// The composed story, from cache or freshly composed. - pub fn get(&self, doc: &Document, sid: StoryId, page_name: Option<&str>) -> Arc { + /// The composed story, from cache or freshly composed. `page` is the absolute page a + /// parent-page item is shown on (its page number markers and variables are that page's), or + /// `None` for a story's own frames. Pages are identified by index, not name: names repeat when + /// sections restart their numbering. + pub fn get(&self, doc: &Document, sid: StoryId, page: Option) -> Arc { let Some(story) = doc.stories.get(&sid) else { return Arc::new(ComposedStory { story: sid, ..Default::default() }) }; let (mut sig, keep_items) = signature(doc, story); - let key = (sid, page_name.map(str::to_string)); + let key = (sid, page); + // The key holds the page's index, which a renumbering doesn't change: its name (section + // start number, style, prefix) is part of the signature. + let page_name = page.map(|p| doc.page_name(p)); + if let Some(name) = &page_name { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + name.hash(&mut h); + sig.push(h.finish() as usize); + } let with_vars = crate::vars::has_vars(story); let running = with_vars && crate::vars::has_running(doc, story); if with_vars { @@ -125,23 +138,20 @@ impl Cache { return e.out.clone(); } } - // Parent-page items are composed per page; variables need that page's index. - let page = match page_name { - Some(n) if with_vars => (0..doc.page_count()).find(|&i| doc.page_name(i) == n), - _ => None, - }; + // Parent-page items are composed per page: markers show its name, variables need its index. + let page = page.filter(|_| with_vars); let running = running.then(|| self.running_index(doc)); let out = Arc::new(crate::compose_story( doc, sid, - &ComposeOptions { page_name: page_name.map(str::to_string), page, running, label: None, xrefs: with_xrefs.then(|| self.xref_index(doc)) }, + &ComposeOptions { page_name, page, running, label: None, xrefs: with_xrefs.then(|| self.xref_index(doc)) }, )); let mut g = self.map.lock().unwrap_or_else(|e| e.into_inner()); let stamp = g.1; if g.0.len() > 4096 { g.0.retain(|_, e| stamp - e.stamp < 64); } - let replaced = g.0.insert(key.clone(), Entry { sig, _keep: (story.clone(), doc.styles.clone(), keep_items), out: out.clone(), stamp }); + let replaced = g.0.insert(key, Entry { sig, _keep: (story.clone(), doc.styles.clone(), keep_items), out: out.clone(), stamp }); drop(g); if key.1.is_none() && let Some(old) = replaced diff --git a/crates/engine/src/cmd/variables.rs b/crates/engine/src/cmd/variables.rs index bbcfaf0..e7772d4 100644 --- a/crates/engine/src/cmd/variables.rs +++ b/crates/engine/src/cmd/variables.rs @@ -129,6 +129,63 @@ fn define(s: &mut Session, p: &Value) -> Result { mod tests { use super::*; + /// A parent-page page-number marker follows Numbering & Section Options: changing a section's + /// start number renames its pages without changing their index or the number of sections, and + /// the marker composed for a page shows the new name. + #[test] + fn parent_page_number_follows_section_renumbering() { + let mut s = Session::new(); + s.execute("file.new", &json!({"pages": 3, "facingPages": false})).unwrap(); + let marker = designcraft_doc::story::PAGE_NUMBER.to_string(); + let r = s + .execute("frame.create", &json!({"spread": {"kind": "parent", "index": 0}, "rect": [72, 30, 500, 60], "content": "text", "text": marker})) + .unwrap(); + let sid = s.doc().unwrap().doc.item(designcraft_doc::ItemId(r["id"].as_u64().unwrap())).unwrap().text_frame().unwrap().story; + assert_eq!(s.doc().unwrap().doc.story(sid).unwrap().text, marker); + let shown = |s: &Session, page: usize| -> usize { + let d = s.doc().unwrap().doc.clone(); + let cs = s.cache.get(&d, sid, Some(page)); + cs.frames.iter().flat_map(|f| f.lines.iter()).map(|l| l.glyphs.iter().filter(|g| g.visible && g.adv > 0.0).count()).sum() + }; + assert_eq!(shown(&s, 2), 1, "page 3"); + s.execute("layout.section", &json!({"page": 1, "startNumber": 100})).unwrap(); + assert_eq!(s.doc().unwrap().doc.page_name(2), "102"); + assert_eq!(shown(&s, 2), 3, "page 102 after renumbering"); + } + + /// Two sections that both number from 1 repeat page names ("1, 2, 1, 2"). A parent-page running + /// header is composed for the page it is shown on, not the first page with that name. + #[test] + fn running_header_on_parent_page_with_repeated_page_numbers() { + let mut s = Session::new(); + s.execute("file.new", &json!({"pages": 4, "facingPages": false})).unwrap(); + s.execute("layout.section", &json!({"page": 3, "startNumber": 1})).unwrap(); + let names: Vec = (0..4).map(|i| s.doc().unwrap().doc.page_name(i)).collect(); + assert_eq!(names, ["1", "2", "1", "2"]); + s.execute("style.paragraph.create", &json!({"name": "Heading"})).ok(); + // The only heading is on the third page (the second section's page 1). + let sp = s.doc().unwrap().doc.page_loc(2).unwrap().0; + let r = s.execute("frame.create", &json!({"spread": sp, "rect": [72, 200, 400, 260], "content": "text", "text": "Epsilon"})).unwrap(); + let sid = s.doc().unwrap().doc.item(designcraft_doc::ItemId(r["id"].as_u64().unwrap())).unwrap().text_frame().unwrap().story; + s.execute("text.select", &json!({"story": sid.0, "anchor": 0, "focus": 1})).unwrap(); + s.execute("style.paragraph.apply", &json!({"name": "Heading"})).unwrap(); + s.execute("variables.define", &json!({"name": "Chapter Head", "type": "runningHeader", "style": "Heading"})).unwrap(); + let r = s + .execute("frame.create", &json!({"spread": {"kind": "parent", "index": 0}, "rect": [72, 30, 500, 60], "content": "text", "text": ""})) + .unwrap(); + let sid = s.doc().unwrap().doc.item(designcraft_doc::ItemId(r["id"].as_u64().unwrap())).unwrap().text_frame().unwrap().story; + s.execute("text.select", &json!({"story": sid.0, "anchor": 0, "focus": 0})).unwrap(); + s.execute("variables.insert", &json!({"name": "Chapter Head"})).unwrap(); + let d = s.doc().unwrap().doc.clone(); + let shown = |page: usize| -> usize { + let cs = s.cache.get(&d, sid, Some(page)); + cs.frames.iter().flat_map(|f| f.lines.iter()).map(|l| l.glyphs.iter().filter(|g| g.visible && g.adv > 0.0).count()).sum() + }; + assert_eq!(shown(0), 0, "no heading on or before the first page"); + assert_eq!(shown(2), "Epsilon".len(), "the third page (named 1) shows its own heading"); + assert_eq!(shown(3), "Epsilon".len(), "the fourth page (named 2) carries it"); + } + #[test] fn running_header_on_parent_page_follows_headings() { let mut s = Session::new(); @@ -158,7 +215,7 @@ mod tests { assert!(matches!(d.find(fid).unwrap().spread, designcraft_doc::SpreadRef::Parent(0))); // Visible glyphs of the header composed for each page: "Alpha · 3" vs "Epsilon · 3". let shown = |page: usize| -> usize { - let cs = s.cache.get(&d, sid, Some(&d.page_name(page))); + let cs = s.cache.get(&d, sid, Some(page)); cs.frames.iter().flat_map(|f| f.lines.iter()).map(|l| l.glyphs.iter().filter(|g| g.visible && g.adv > 0.0).count()).sum() }; let (p1, p2, p3) = (shown(0), shown(1), shown(2)); diff --git a/crates/pdf/src/export.rs b/crates/pdf/src/export.rs index d92fc24..dcf8abb 100644 --- a/crates/pdf/src/export.rs +++ b/crates/pdf/src/export.rs @@ -630,7 +630,6 @@ impl Exporter<'_> { let Some((ppi, ppage)) = doc.parent_page_for(first + pi) else { continue }; let parent = &doc.parents[ppi]; let dx = page.x - parent.pages[ppage].x; - let page_name = doc.page_name(first + pi); for it in &parent.items { if it.layer != layer.id || page.overridden.contains(&it.id) { continue; @@ -639,7 +638,7 @@ impl Exporter<'_> { if parent.page_at_x(it.bounds().center().x) != Some(ppage) && parent.pages.len() > 1 { continue; } - self.top_item(s, it, Affine::translate((dx, 0.0)), Some(&page_name), true); + self.top_item(s, it, Affine::translate((dx, 0.0)), Some(first + pi), true); } } for it in &sp.items { @@ -679,9 +678,9 @@ impl Exporter<'_> { } /// A spread or parent item, inside its structure tag when tagging. - fn top_item(&mut self, s: &mut Surface, it: &Item, parent: Affine, page_name: Option<&str>, on_parent: bool) { + fn top_item(&mut self, s: &mut Surface, it: &Item, parent: Affine, page: Option, on_parent: bool) { if !self.opts.tagged || it.hidden || it.nonprinting { - return self.item(s, it, parent, page_name); + return self.item(s, it, parent, page); } let story = it.text_frame().map(|t| t.story); let figure = !it.alt_text.is_empty() || matches!(it.content, Content::Graphic(_)) || it.has_nested_items(); @@ -689,7 +688,7 @@ impl Exporter<'_> { // Page furniture and decoration. let kind = if on_parent { ArtifactType::Page } else { ArtifactType::Other }; s.start_tagged(ContentTag::Artifact(Artifact::new(kind, None))); - self.item(s, it, parent, page_name); + self.item(s, it, parent, page); s.end_tagged(); return; } @@ -700,12 +699,12 @@ impl Exporter<'_> { self.tags.push(TagEntry::Story(sid)); } self.tag_story = Some(sid); - self.item(s, it, parent, page_name); + self.item(s, it, parent, page); self.tag_story = None; return; } let id = s.start_tagged(ContentTag::Other); - self.item(s, it, parent, page_name); + self.item(s, it, parent, page); s.end_tagged(); let alt = (!it.alt_text.is_empty()).then(|| it.alt_text.clone()); let mut g = TagGroup::new(Tag::Figure(alt)); @@ -751,10 +750,10 @@ impl Exporter<'_> { tree } - fn item(&mut self, s: &mut Surface, it: &Item, parent: Affine, page_name: Option<&str>) { + fn item(&mut self, s: &mut Surface, it: &Item, parent: Affine, page: Option) { let gf = &it.effects.gradient_feather; if !gf.on || it.hidden || it.nonprinting { - return self.item_inner(s, it, parent, page_name); + return self.item_inner(s, it, parent, page); } // Gradient feather: a luminosity mask (white = opaque) over the object. let xf = parent * it.xf; @@ -804,11 +803,11 @@ impl Exporter<'_> { ms.finish(); let stream = sb.finish(); s.push_mask(krilla::mask::Mask::new(stream, krilla::mask::MaskType::Luminosity)); - self.item_inner(s, it, parent, page_name); + self.item_inner(s, it, parent, page); s.pop(); } - fn item_inner(&mut self, s: &mut Surface, it: &Item, parent: Affine, page_name: Option<&str>) { + fn item_inner(&mut self, s: &mut Surface, it: &Item, parent: Affine, page: Option) { if it.hidden || it.nonprinting { return; } @@ -843,7 +842,7 @@ impl Exporter<'_> { s.push_clip_path(c, &FillRule::NonZero); } for c in it.children() { - self.item(s, c, xf, page_name); + self.item(s, c, xf, page); } if clip.is_some() { s.pop(); @@ -863,7 +862,7 @@ impl Exporter<'_> { if !it.children().is_empty() { let pushes = Self::push_group(s, it.opacity, it.blend) + Self::push_isolation(s, it); for c in it.shown_children() { - self.item(s, c, xf, page_name); + self.item(s, c, xf, page); } for _ in 0..pushes { s.pop(); @@ -921,7 +920,7 @@ impl Exporter<'_> { } } Content::Text(tfr) => { - let cs = self.cache.get(doc, tfr.story, page_name); + let cs = self.cache.get(doc, tfr.story, page); if let (Some(ft), Some(pt)) = (cs.frame(it.id), &tfr.options.path) { // Type on a path: the glyphs as outlines along the path. for (si, gp) in designcraft_compose::path_glyphs(&cs, ft, &bp, pt) { @@ -966,11 +965,11 @@ impl Exporter<'_> { if let Content::Text(tfr) = &it.content && let Some(st) = doc.story(tfr.story).filter(|st| !st.objects.is_empty()) { - let cs = self.cache.get(doc, tfr.story, page_name); + let cs = self.cache.get(doc, tfr.story, page); if let Some(ft) = cs.frame(it.id) { for o in &ft.objects { if let Some(obj) = st.objects.get(o.index) { - self.item(s, &obj.item, xf * Affine::translate(o.origin.to_vec2()), page_name); + self.item(s, &obj.item, xf * Affine::translate(o.origin.to_vec2()), page); } } } diff --git a/crates/render/src/fx.rs b/crates/render/src/fx.rs index 77bb187..6b29516 100644 --- a/crates/render/src/fx.rs +++ b/crates/render/src/fx.rs @@ -66,7 +66,7 @@ fn shadow(sigma: f64, color: peniko::Color) -> Filter { impl Renderer { /// Draw an item with soft effects: below-effects, the (feathered) body, inner shadow. - pub(crate) fn draw_item_fx(&mut self, ctx: &mut RenderContext, f: &Frame, it: &Item, bp: &BezPath, xf: Affine, page_name: Option<&str>) { + pub(crate) fn draw_item_fx(&mut self, ctx: &mut RenderContext, f: &Frame, it: &Item, bp: &BezPath, xf: Affine, page: Option) { let doc = f.doc; let e = &it.effects; let sw = if it.stroke.is_none() { 0.0 } else { it.stroke.extent() }; @@ -95,7 +95,7 @@ impl Renderer { let moved = Frame { view: fr.view * Affine::translate(off), ..*fr }; c.set_transform(moved.view); c.push_layer(None, None, None, None, Some(filter)); - me.draw_body(c, &moved, it, bp, xf, page_name); + me.draw_body(c, &moved, it, bp, xf, page); if grow > 0.0 && solid { c.set_transform(moved.view * xf); c.set_paint(peniko::Color::BLACK); @@ -119,7 +119,7 @@ impl Renderer { self.with_filters(ctx, f, reach, w / 4.0, |me, c, fr| { c.set_transform(fr.view * xf); c.push_clip_layer(bp); - me.draw_body(c, fr, it, bp, xf, page_name); + me.draw_body(c, fr, it, bp, xf, page); // Keep the body where the blurred inset silhouette is. c.set_transform(Affine::IDENTITY); c.push_layer(None, Some(BlendMode::new(Mix::Normal, Compose::DestIn)), None, None, None); @@ -139,7 +139,7 @@ impl Renderer { c.pop_layer(); }); } else { - self.draw_body(ctx, f, it, bp, xf, page_name); + self.draw_body(ctx, f, it, bp, xf, page); } // Directional feather: one fading ramp per side, multiplied in. let df = &e.directional_feather; diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index b0f8df1..2cc5c54 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -216,7 +216,7 @@ pub struct Renderer { resources: Resources, glyphs: text::GlyphCache, /// Composed stories looked up during the current render. - stories: HashMap<(StoryId, Option), Arc>, + stories: HashMap<(StoryId, Option), Arc>, pub threads: u16, pub stats: FrameStats, } @@ -408,7 +408,6 @@ impl Renderer { let Some((ppi, ppage)) = doc.parent_page_for(first + pi) else { continue }; let parent = &doc.parents[ppi]; let dx = page.x - parent.pages[ppage].x; - let page_name = doc.page_name(first + pi); let pr = parent.pages[ppage].bounds(); for it in &parent.items { if it.layer != layer.id || page.overridden.contains(&it.id) { @@ -419,7 +418,7 @@ impl Renderer { continue; } let _ = pr; - self.draw_item(ctx, f, it, Affine::translate((dx, 0.0)), Some(&page_name)); + self.draw_item(ctx, f, it, Affine::translate((dx, 0.0)), Some(first + pi)); } } } @@ -431,7 +430,7 @@ impl Renderer { } } - fn draw_item(&mut self, ctx: &mut RenderContext, f: &Frame, it: &Item, parent: Affine, page_name: Option<&str>) { + fn draw_item(&mut self, ctx: &mut RenderContext, f: &Frame, it: &Item, parent: Affine, page: Option) { if it.hidden || f.opts.hidden.contains(&it.id) || f.opts.printing_only && it.nonprinting { return; } @@ -458,7 +457,7 @@ impl Renderer { ctx.set_transform(f.view * xf); ctx.push_clip_layer(&bp); for c in it.children() { - self.draw_item(ctx, f, c, xf, page_name); + self.draw_item(ctx, f, c, xf, page); } ctx.pop_layer(); if !it.stroke.is_none() { @@ -489,11 +488,11 @@ impl Renderer { ctx.fill_path(&c.path.to_bezpath()); ctx.pop_layer(); } - self.draw_item(ctx, f, c, xf, page_name); + self.draw_item(ctx, f, c, xf, page); } } else { for c in it.shown_children() { - self.draw_item(ctx, f, c, xf, page_name); + self.draw_item(ctx, f, c, xf, page); } } if layered { @@ -520,9 +519,9 @@ impl Renderer { ctx.push_layer(None, Some(blend_mode(it.blend)), Some(it.opacity), None, None); } if effects { - self.draw_item_fx(ctx, f, it, &bp, xf, page_name); + self.draw_item_fx(ctx, f, it, &bp, xf, page); } else { - self.draw_body(ctx, f, it, &bp, xf, page_name); + self.draw_body(ctx, f, it, &bp, xf, page); } if layered { ctx.pop_layer(); @@ -531,18 +530,18 @@ impl Renderer { /// The composed story for a frame (memoised for the current render: the cache lookup /// validates the whole thread each time). - fn story(&mut self, f: &Frame, sid: StoryId, page_name: Option<&str>) -> Arc { - let key = (sid, page_name.map(str::to_string)); + fn story(&mut self, f: &Frame, sid: StoryId, page: Option) -> Arc { + let key = (sid, page); if let Some(cs) = self.stories.get(&key) { return cs.clone(); } - let cs = f.cache.get(f.doc, sid, page_name); + let cs = f.cache.get(f.doc, sid, page); self.stories.insert(key, cs.clone()); cs } /// Fill, content and stroke of a leaf item (no opacity, blend or effects). - pub(crate) fn draw_body(&mut self, ctx: &mut RenderContext, f: &Frame, it: &Item, bp: &BezPath, xf: Affine, page_name: Option<&str>) { + pub(crate) fn draw_body(&mut self, ctx: &mut RenderContext, f: &Frame, it: &Item, bp: &BezPath, xf: Affine, page: Option) { let doc = f.doc; // Fill. if !it.fill.is_none() { @@ -575,7 +574,7 @@ impl Renderer { ctx.pop_layer(); } Content::Text(tf) => { - let cs = self.story(f, tf.story, page_name); + let cs = self.story(f, tf.story, page); if let Some(ft) = cs.frame(it.id) { match &tf.options.path { Some(pt) => self.draw_path_text(ctx, f, &cs, ft, xf, bp, pt),