From 4c67b8f73cc019aafa2754170689b82f0ae28952 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 14 Nov 2026 08:02:17 -0800 Subject: [PATCH] Graphs: swatch edits reach the kept black legend fill of graphs that draw it Only a pie with Legends in Wedges picks its legend labels' fill, white on dark wedges. Paint visits and swatch mappings now leave its default black legend fill out only for such a pie; other graphs draw the kept fill, so it is visited and mapped like the other label fills. A color mode change still leaves the default black legend fill as the default, so turning on Legends in Wedges later keeps the white labels on dark wedges. --- crates/doc/src/graph.rs | 8 ++++ crates/doc/src/swatches.rs | 56 ++++++++++++++++----------- crates/engine/src/cmd/graph/series.rs | 4 +- crates/engine/src/cmd/graph/tests.rs | 21 ++++++++++ 4 files changed, 64 insertions(+), 25 deletions(-) diff --git a/crates/doc/src/graph.rs b/crates/doc/src/graph.rs index b387eecb2a..2746390b86 100644 --- a/crates/doc/src/graph.rs +++ b/crates/doc/src/graph.rs @@ -548,6 +548,14 @@ impl GraphSpec { self.series_kinds.get(s).copied().flatten().filter(|k| k.combines_with(self.kind)).unwrap_or(self.kind) } + /// Does the graph pick the fill of its legend labels itself, white on a dark wedge and black on the others: a pie + /// with Legends in Wedges whose legend labels keep no fill of their own ([`LabelStyles::legend_fill_is_own`])? Only + /// then is a black legend fill it keeps the default, which edits that map the kept paints leave as it is; other + /// graphs draw the kept fill as it is. + pub fn picks_legend_fill(&self) -> bool { + self.kind == GraphKind::Pie && self.legend && self.pie_legend_in_wedges && !self.label_styles.legend_fill_is_own() + } + /// The data with blank cells as `None`, every row as long as the longest (at most [`MAX_GRAPH_CATEGORIES`] rows /// of [`MAX_GRAPH_SERIES`] cells). pub fn cells(&self) -> Vec>> { diff --git a/crates/doc/src/swatches.rs b/crates/doc/src/swatches.rs index cab2115fda..63951d8104 100644 --- a/crates/doc/src/swatches.rs +++ b/crates/doc/src/swatches.rs @@ -406,12 +406,13 @@ fn graph_paints_mut(g: &mut crate::graph::GraphSpec) -> impl Iterator Option { let g = n.extras().graph.as_deref()?; let l = &g.label_styles; - if l.legend.is_none() || l.legend_fill_is_own() { + if l.legend.is_none() || !g.picks_legend_fill() { return None; } let series: usize = g.series_paints.iter().map(|s| usize::from(s.fill.is_some()) + usize::from(s.stroke.is_some())).sum(); @@ -764,26 +765,35 @@ mod tests { #[test] fn the_default_black_legend_fill_is_left_as_it_is() { - let (mut d, a, _) = doc(); - let black = || crate::CharStyle { fill: Paint::solid(Color::BLACK), size: 11.0, ..Default::default() }; - let spec = crate::graph::GraphSpec { - series_paints: vec![crate::SeriesPaint { fill: Some(Paint::solid(Color::BLACK)), ..Default::default() }], - label_styles: crate::graph::LabelStyles { values: Some(black()), legend: Some(black()), ..Default::default() }, - ..Default::default() - }; - let blacks = |d: &Document| { - let mut seen = 0; - d.visit_paints(&mut |p| seen += usize::from(*p == Paint::solid(Color::BLACK))); - seen - }; - let before = blacks(&d); - d.node_mut(a).unwrap().extras_mut().graph = Some(Box::new(spec)); - assert_eq!(blacks(&d) - before, 2, "the series fill and the value labels' fill are visited"); - let mut link_black = |c: &mut Color, s: &mut Option, _: &mut f32| *c == Color::BLACK && s.replace("Black".into()).is_none(); - assert_eq!(d.map_solid_paints_in(&[a], &mut link_black), 2, "the series fill and the value labels' fill"); - let styles = d.node(a).unwrap().extras().graph.clone().unwrap().label_styles; - assert_eq!(styles.legend.unwrap().fill, Paint::solid(Color::BLACK), "not linked"); - assert_eq!(styles.values.unwrap().fill, linked(Color::BLACK, "Black")); + use crate::graph::GraphKind; + // Only a pie with Legends in Wedges picks its legend labels' fill; the other graphs draw the kept black. + for (kind, in_wedges, picks) in [(GraphKind::Pie, true, true), (GraphKind::Pie, false, false), (GraphKind::Column, true, false)] { + let (mut d, a, _) = doc(); + let black = || crate::CharStyle { fill: Paint::solid(Color::BLACK), size: 11.0, ..Default::default() }; + let spec = crate::graph::GraphSpec { + kind, + legend: true, + pie_legend_in_wedges: in_wedges, + series_paints: vec![crate::SeriesPaint { fill: Some(Paint::solid(Color::BLACK)), ..Default::default() }], + label_styles: crate::graph::LabelStyles { values: Some(black()), legend: Some(black()), ..Default::default() }, + ..Default::default() + }; + let blacks = |d: &Document| { + let mut seen = 0; + d.visit_paints(&mut |p| seen += usize::from(*p == Paint::solid(Color::BLACK))); + seen + }; + let before = blacks(&d); + d.node_mut(a).unwrap().extras_mut().graph = Some(Box::new(spec)); + let want = if picks { 2 } else { 3 }; + assert_eq!(blacks(&d) - before, want, "{kind:?}, in wedges {in_wedges}: the series, value labels' and legend fills visited"); + let mut link_black = |c: &mut Color, s: &mut Option, _: &mut f32| *c == Color::BLACK && s.replace("Black".into()).is_none(); + assert_eq!(d.map_solid_paints_in(&[a], &mut link_black), want, "{kind:?}, in wedges {in_wedges}"); + let styles = d.node(a).unwrap().extras().graph.clone().unwrap().label_styles; + let legend = if picks { Paint::solid(Color::BLACK) } else { linked(Color::BLACK, "Black") }; + assert_eq!(styles.legend.unwrap().fill, legend, "{kind:?}, in wedges {in_wedges}: linked unless the pie picks the fill"); + assert_eq!(styles.values.unwrap().fill, linked(Color::BLACK, "Black")); + } } #[test] diff --git a/crates/engine/src/cmd/graph/series.rs b/crates/engine/src/cmd/graph/series.rs index 21dc2ea167..5f6d602ad5 100644 --- a/crates/engine/src/cmd/graph/series.rs +++ b/crates/engine/src/cmd/graph/series.rs @@ -886,9 +886,9 @@ fn run_paint_change(was: &Node, now: &Node, fill: bool) -> Option<(CharStyle, Ve } /// Does graph `g` pick the fill of its legend labels itself, white on a dark wedge and black on the others: a pie with -/// Legends in Wedges whose legend labels keep no fill of their own ([`vectorcraft_doc::LabelStyles::legend_fill_is_own`])? +/// Legends in Wedges whose legend labels keep no fill of their own ([`GraphSpec::picks_legend_fill`])? pub(super) fn picks_legend_fill(g: &GraphSpec) -> bool { - g.kind == GraphKind::Pie && g.legend && g.pie_legend_in_wedges && !g.label_styles.legend_fill_is_own() + g.picks_legend_fill() } /// The graph labels among `roots` and inside them, for an edit that reaches graph labels without naming them. diff --git a/crates/engine/src/cmd/graph/tests.rs b/crates/engine/src/cmd/graph/tests.rs index b7ee6fe77e..7eda9804df 100644 --- a/crates/engine/src/cmd/graph/tests.rs +++ b/crates/engine/src/cmd/graph/tests.rs @@ -6409,6 +6409,27 @@ fn a_color_mode_change_keeps_the_default_wedge_legend_fill() { assert!(spec(&s, id).label_styles.legend_fill_set, "the converted red is still the labels' own"); } +/// The default black legend fill of a graph that keeps its legend beside the marks stays the default through a color +/// mode change, so turning on Legends in Wedges later still draws the labels on dark wedges white. +#[test] +fn a_color_mode_change_keeps_the_default_legend_fill_of_other_graphs() { + // A pie made here keeps its legend beside the pies. + for kind in ["column", "pie"] { + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 800, "height": 800})).unwrap(); + let id = make(&mut s, kind, ",a,b\nP1,1,2"); + s.execute("select.set", &json!({"ids": [id.0]})).unwrap(); + s.execute("text.setStyle", &json!({"size": 11})).unwrap(); + let black = Paint::solid(vectorcraft_color::Color::BLACK); + let before = spec(&s, id).label_styles; + assert_eq!(before.legend.as_ref().map(|st| st.fill.clone()), Some(black.clone()), "{kind}: {before:?}"); + s.execute("file.documentColorMode", &json!({"mode": "cmyk"})).unwrap(); + let after = spec(&s, id).label_styles; + assert_eq!(after.legend.as_ref().map(|st| st.fill.clone()), Some(black), "{kind}: {after:?}"); + assert!(!after.legend_fill_set && !after.legend_fill_is_own(), "{kind}: still the default: {after:?}"); + } +} + #[test] fn labels_are_told_apart_by_where_they_come_not_by_their_text() { let mut s = Session::new();