From 1192e5ef3076f7df1b239e2f4a14645ec53186b8 Mon Sep 17 00:00:00 2001 From: Stephan Schott-Verdugo Date: Sun, 1 Nov 2026 23:06:09 +0100 Subject: [PATCH] Detached placeholders follow each property's style precedence Review follow-up (CodeRabbit on #1658): - Text levels are resolved before the copy's style changes. A master placeholder's font reference reached the copy's text (body font in an accent colour for a title) although the original ignores it: text only takes the shape's or the layout placeholder's font reference. - Fill, line and effects come from where `resolve::fill`, `resolve::line` and `resolve::effects` take them: the first shape in the chain with an explicit value or a style. A layout's (or the slide's own) style no longer loses to an explicit fill, line or effect on the master. The copy gets one style whose references each come from the shape that supplied that property; unused references are neutral. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/tests.rs | 100 ++++++++++++++++++++++++++++++++++++ crates/model/src/resolve.rs | 71 ++++++++++++++++--------- 2 files changed, 147 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index eec43cc4a..26fb7b9cf 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -267,6 +267,106 @@ fn a_pasted_or_duplicated_placeholder_keeps_its_inherited_look() { assert!(s.doc().unwrap().doc.validate().is_empty()); } +/// The resolved fill, line and effects of shape `id` on the current slide, with their style colours. +fn resolved_box(s: &Session, id: ShapeId) -> String { + use deckcraft_model::resolve; + let st = s.doc().unwrap(); + let ctx = resolve::Ctx::for_slide(&st.doc, st.current_slide().unwrap()).unwrap(); + let sh = st.shape(id).unwrap(); + format!("{:?}\n{:?}\n{:?}", resolve::fill(&ctx, sh), resolve::line(&ctx, sh), resolve::effects(&ctx, sh)) +} + +/// Edit the title placeholders the current slide's title inherits from: its layout's and its +/// master's. Returns the slide title's id. +fn edit_title_parents( + s: &mut Session, + layout: impl FnOnce(&mut deckcraft_model::Shape), + master: impl FnOnce(&mut deckcraft_model::Shape), +) -> ShapeId { + let st = s.doc_mut().unwrap(); + let slide = st.current_slide().unwrap().clone(); + let title = slide.shapes.iter().find(|x| x.ph_type().is_some_and(|k| k.is_title())).unwrap().id; + let doc = std::sync::Arc::make_mut(&mut st.doc); + let m = std::sync::Arc::make_mut(&mut doc.masters[0]); + let lay = m.layouts.iter_mut().find(|l| l.id == slide.layout).unwrap(); + layout(lay.shapes.iter_mut().find(|x| x.ph_type().is_some_and(|k| k.is_title())).unwrap()); + master(m.shapes.iter_mut().find(|x| x.ph_type().is_some_and(|k| k.is_title())).unwrap()); + title +} + +/// A copied placeholder takes each of fill, line and effects from where the original does: a +/// style on the layout's placeholder (or on the slide's own) wins over an explicit value on the +/// master's, as `resolve::fill`, `resolve::line` and `resolve::effects` decide. +#[test] +fn a_pasted_placeholder_keeps_the_style_precedence_of_its_box() { + use deckcraft_color::SchemeSlot; + use deckcraft_model::{ColorRef, Effects, Fill, Line, ShapeStyle}; + for style_on_slide in [false, true] { + let mut s = session(); + s.execute("slide.new", &json!({})).unwrap(); + let style = ShapeStyle { + line_ref: (1, ColorRef::scheme(SchemeSlot::Accent2)), + fill_ref: (1, ColorRef::scheme(SchemeSlot::Accent1)), + effect_ref: (0, ColorRef::scheme(SchemeSlot::Accent3)), + font_ref: ("minor".into(), None), + }; + let layout_style = (!style_on_slide).then(|| style.clone()); + let title = edit_title_parents( + &mut s, + |l| l.style = layout_style, + |m| { + m.fill = Some(Fill::solid(ColorRef::scheme(SchemeSlot::Accent4))); + m.line = Some(Line::solid(ColorRef::scheme(SchemeSlot::Accent5), 20.0)); + m.effects = Some(Effects { soft_edge: Some(5.0), ..Default::default() }); + }, + ); + if style_on_slide { + s.edit(|doc, sel| { + let list = crate::shapes_mut(doc, sel).unwrap(); + list.iter_mut().find(|x| x.id == title).unwrap().style = Some(style.clone()); + Ok(()) + }) + .unwrap(); + } + s.execute("text.set", &json!({"id": title.0, "text": "Styled title"})).unwrap(); + let want = (resolved_box(&s, title), resolved_look(&s, title)); + s.execute("selection.select", &json!({"id": title.0})).unwrap(); + s.execute("edit.copy", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + let copy = s.doc().unwrap().selection.shapes[0]; + assert_eq!((resolved_box(&s, copy), resolved_look(&s, copy)), want, "style on the slide: {style_on_slide}"); + } +} + +/// The master's placeholder style never reaches the copy's text: the original's text ignores it +/// (only the shape's or the layout's font reference counts), so the copy must too. +#[test] +fn a_pasted_placeholder_ignores_the_masters_font_reference() { + use deckcraft_color::SchemeSlot; + use deckcraft_model::{ColorRef, ShapeStyle}; + let mut s = session(); + s.execute("slide.new", &json!({})).unwrap(); + let title = edit_title_parents( + &mut s, + |l| l.style = None, + |m| { + m.style = Some(ShapeStyle { + line_ref: (0, ColorRef::scheme(SchemeSlot::Accent2)), + fill_ref: (0, ColorRef::scheme(SchemeSlot::Accent1)), + effect_ref: (0, ColorRef::scheme(SchemeSlot::Accent3)), + font_ref: ("minor".into(), Some(ColorRef::scheme(SchemeSlot::Accent6))), + }) + }, + ); + s.execute("text.set", &json!({"id": title.0, "text": "Title"})).unwrap(); + let want = (resolved_box(&s, title), resolved_look(&s, title)); + assert!(want.1.contains("+mj-lt"), "the title draws in the heading font: {}", want.1); + s.execute("selection.select", &json!({"id": title.0})).unwrap(); + s.execute("edit.duplicate", &json!({})).unwrap(); + let copy = s.doc().unwrap().selection.shapes[0]; + assert_eq!((resolved_box(&s, copy), resolved_look(&s, copy)), want); +} + /// The same on every text placeholder of the sample deck (titles, bulleted bodies, subtitles). #[test] fn sample_placeholders_paste_with_their_look() { diff --git a/crates/model/src/resolve.rs b/crates/model/src/resolve.rs index 8cf87582d..5f1651de9 100644 --- a/crates/model/src/resolve.rs +++ b/crates/model/src/resolve.rs @@ -4,7 +4,7 @@ use deckcraft_color::{ColorScheme, Rgba, SchemeSlot}; use deckcraft_geom::Xfrm; -use crate::style::{ColorRef, Effects, Fill, Line}; +use crate::style::{ColorRef, Effects, Fill, Line, ShapeStyle}; use crate::text::{Anchor, AutoFit, BodyProps, LevelStyle, ListStyle, ParaProps, RunProps}; use crate::{Background, Layout, Master, PhType, Presentation, Shape, Slide}; @@ -229,30 +229,53 @@ pub fn detach_placeholder(ctx: &Ctx, shape: &mut Shape) { if shape.ph.is_none() { return; } - let (lp, mp) = parents(ctx, shape); - let parents = [lp, mp].into_iter().flatten(); - shape.xfrm = Some(xfrm(ctx, shape)); - if shape.fill.is_none() && shape.style.is_none() { - shape.fill = parents.clone().find_map(|s| s.fill.clone()); - } - if shape.style.is_none() { - shape.style = parents.clone().find_map(|s| s.style.clone()); - } - if shape.effects.is_none() { - shape.effects = parents.clone().find_map(|s| s.effects.clone()); - } - let mut line = shape.line.clone(); - for p in parents.filter_map(|s| s.line.as_ref()) { - line.get_or_insert_with(Line::default).inherit(p); - } - shape.line = line; - if shape.text.is_some() { - let body = body(ctx, shape); - let levels: Vec> = (0..9).map(|l| Some(level_style(ctx, shape, l))).collect(); - if let Some(t) = shape.text.as_mut() { - t.body = body; - t.list_style = ListStyle { levels }; + // The text first: `level_style` reads the shape's own style, which changes below. + let text = shape.text.as_ref().map(|_| (body(ctx, shape), (0..9).map(|l| Some(level_style(ctx, shape, l))).collect::>())); + let own = shape.clone(); + let (lp, mp) = parents(ctx, &own); + let chain = [Some(&own), lp, mp].into_iter().flatten(); + + // Each property comes from where `fill`, `line` and `effects` take it: the first shape in the + // chain with an explicit value or a style. Lines merge explicit values up to the first styled + // shape, whose style line fills the rest. + let fill_src = chain.clone().find(|s| s.fill.is_some() || s.style.is_some()); + let effects_src = chain.clone().find(|s| s.effects.is_some() || s.style.is_some()); + let mut line = None; + let mut line_src = None; + for s in chain.clone() { + if let Some(l) = &s.line { + line.get_or_insert_with(Line::default).inherit(l); } + if s.style.is_some() { + line_src = Some(s); + break; + } + } + // Text takes its font reference from the shape's style or its layout placeholder's (a table's + // from its own only), never the master's. + let table = matches!(own.kind, crate::ShapeKind::Table(_)); + let font_src = own.style.as_ref().or(lp.filter(|_| !table).and_then(|s| s.style.as_ref())); + + // One style for the copy, each reference taken from the shape that supplied that property. A + // property with an explicit value ignores its reference, except a gradient without stops, + // which takes them from its own shape's style; a neutral reference (index 0) adds nothing. + let style_of = |s: Option<&Shape>| s.and_then(|s| s.style.clone()); + let neutral = (0, ColorRef::scheme(SchemeSlot::Tx1)); + let style = chain.clone().any(|s| s.style.is_some()).then(|| ShapeStyle { + fill_ref: style_of(fill_src).map_or_else(|| neutral.clone(), |st| st.fill_ref), + line_ref: style_of(line_src).map_or_else(|| neutral.clone(), |st| st.line_ref), + effect_ref: style_of(effects_src).map_or_else(|| neutral.clone(), |st| st.effect_ref), + font_ref: font_src.map_or_else(|| ("none".into(), None), |st| st.font_ref.clone()), + }); + + shape.xfrm = Some(xfrm(ctx, &own)); + shape.fill = fill_src.and_then(|s| s.fill.clone()); + shape.effects = effects_src.and_then(|s| s.effects.clone()); + shape.line = line; + shape.style = style; + if let (Some(t), Some((body, levels))) = (shape.text.as_mut(), text) { + t.body = body; + t.list_style = ListStyle { levels }; } shape.ph = None; }