From 0405fb73bfc5b912883820244debafb82154d386 Mon Sep 17 00:00:00 2001 From: Sazid Al Bayazid Date: Sun, 18 Oct 2026 12:43:44 +0600 Subject: [PATCH] Charts: a series' own data-label setting decides whether its labels show (#868) The reader stored a series-level `c:dLbls/c:showVal` in `Series.data_labels`, but every drawing path tested only the chart-wide `Chart.data_labels`: a series that asked for labels got none, and a series that asked for none got them. The series' own setting now wins, and a bare `` (no `val` attribute) means true, as the schema's default. The plot group's own setting is respected too: a group that asks for no labels keeps them off, even when another group of a combo chart asks for them. Before, any group that asked turned the chart-wide setting on, and the group that said no inherited it. Tests: `crates/pptx/tests/chart_data_labels.rs` - a bare series `showVal` shows that series' labels while its siblings stay quiet; an explicit `val="0"` on the first series keeps it quiet under a chart-wide on; a combo chart keeps each group's own setting; all three settings round-trip. Red-proof: with the series-level setting ignored, the first two fail; with the group's setting ignored, the combo chart's bar group shows labels it asked to keep quiet. The model gains `Chart::series_data_labels`, the one place that decides whether a series' labels show; the render paths (bar, column, line, area, pie, doughnut, radar, scatter) call it. --- ROADMAP.md | 1 + crates/model/src/chart.rs | 20 ++++ crates/pptx/src/read/chart.rs | 12 +- crates/pptx/src/write/chart.rs | 2 +- crates/pptx/tests/chart_data_labels.rs | 154 +++++++++++++++++++++++++ crates/render/src/chart.rs | 4 +- crates/render/src/chartex.rs | 4 +- crates/render/src/radar.rs | 2 +- 8 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 crates/pptx/tests/chart_data_labels.rs diff --git a/ROADMAP.md b/ROADMAP.md index 93bc931e..32fd15b8 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -137,6 +137,7 @@ app's JSON control channel (`deckcraft --control PORT`). | 2026-10-17 | Edit Points: turning a shape into a freeform is one undo step, so Undo brings the original shape back (it used to stay a freeform) | | 2026-10-17 | While a placeholder's text is being edited, a click on a shape in front of it selects that shape instead of moving the caret, as in PowerPoint (#931) | | 2026-10-15 | Native saves keep an imported chart's embedded workbook, drawings and relationships; headless PPTX → edit → native save/reopen → PPTX tests verify the payloads and stale-workbook rejection | +| 2026-10-18 | Charts: a series' own data-label setting is honoured — numbers PowerPoint shows on a graph no longer disappear, and a bare `c:showVal` means on (#868) | | 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-16 | Export: PNG with a transparent background — the slide background is left out, only the slide's content is drawn (Export… dialog, File › Export, web download, `file.export`/`file.render` `transparentBackground`, `deckcraft-cli render --transparent`) (#784) | | 2026-10-16 | Arrange ▸ Align: Align to Slide and Align Selected Objects, as in PowerPoint. One object aligns to the slide and several to each other until the other choice is ticked; the choice holds for that selection and also drives Distribute | diff --git a/crates/model/src/chart.rs b/crates/model/src/chart.rs index f5aeb1ee..a71ffec8 100644 --- a/crates/model/src/chart.rs +++ b/crates/model/src/chart.rs @@ -699,6 +699,12 @@ impl Chart { crate::numfmt::format(code.map_or("General", String::as_str), v, self.lang.as_deref()) } + /// Does series `s` show its data labels? The series' own `c:dLbls/c:showVal` wins — an + /// explicit `false` hides them even when the chart-wide setting is on (#868). + pub fn series_data_labels(&self, s: &Series) -> bool { + s.data_labels.unwrap_or(self.data_labels) + } + /// Value range across series (stacked sums for stacked types). pub fn value_range(&self) -> (f64, f64) { let mut lo: f64 = 0.0; @@ -850,6 +856,20 @@ mod tests { assert_eq!(c.log_axis_scale(2), Some((0.0, 6.0, 3.0))); } + /// #868: a series' own label setting wins; an unset series takes the chart-wide one. + #[test] + fn series_data_labels_override_the_chart_wide_setting() { + let mut c = Chart::sample(ChartType::Column); + c.data_labels = false; + let mut s = c.series[0].clone(); + assert!(!c.series_data_labels(&s), "unset series follows the chart"); + s.data_labels = Some(true); + assert!(c.series_data_labels(&s), "a series that asks for labels gets them"); + c.data_labels = true; + s.data_labels = Some(false); + assert!(!c.series_data_labels(&s), "an explicit series false hides them anyway"); + } + #[test] fn error_bar_ranges() { let vals = [Some(3.0), Some(5.0), Some(2.0), Some(4.0)]; diff --git a/crates/pptx/src/read/chart.rs b/crates/pptx/src/read/chart.rs index 73cc416c..44fd3124 100644 --- a/crates/pptx/src/read/chart.rs +++ b/crates/pptx/src/read/chart.rs @@ -213,7 +213,11 @@ fn chart_within(root: &El, mut budget: usize) -> Option { if let Some(v) = g.child_val("holeSize").and_then(|v| v.parse::().ok()) { c.hole_size = (v / 100.0).clamp(0.0, 0.9); } - if g.path(&["dLbls", "showVal"]).and_then(|v| v.bool("val")) == Some(true) { + // `` without a val attribute means true, as in the schema's default. + // The setting belongs to this plot group: a group that asks for no labels keeps them + // off, even when another group of a combo chart asks for them (#868). + let group_labels = g.path(&["dLbls", "showVal"]).map(|v| v.bool("val").unwrap_or(true)); + if group_labels == Some(true) { c.data_labels = true; } kinds.push(kind); @@ -230,8 +234,12 @@ fn chart_within(root: &El, mut budget: usize) -> Option { secondary_axis: secondary, ..Default::default() }; - if let Some(d) = s.child("dLbls").and_then(|d| d.child("showVal")).and_then(|v| v.bool("val")) { + // `` without a val attribute means true, as in the schema's default. + if let Some(d) = s.child("dLbls").and_then(|d| d.child("showVal")).map(|v| v.bool("val").unwrap_or(true)) { ser.data_labels = Some(d); + } else if let Some(false) = group_labels { + // The group asks for no labels and the series does not say otherwise. + ser.data_labels = Some(false); } for t in s.children_named("trendline").take(16) { if let Some(t) = trendline(t) { diff --git a/crates/pptx/src/write/chart.rs b/crates/pptx/src/write/chart.rs index 88917450..2ec0959a 100644 --- a/crates/pptx/src/write/chart.rs +++ b/crates/pptx/src/write/chart.rs @@ -857,7 +857,7 @@ pub fn build_chartex(c: &Chart) -> Vec { for (i, s) in c.series.iter().enumerate().take(if one { 1 } else { usize::MAX }) { w.open("cx:series", A::new().a("layoutId", layout).a("uniqueId", format!("{{DC00000{i}-0000-4000-8000-000000000000}}"))); cx_text(&mut w, &s.name); - if s.data_labels.unwrap_or(c.data_labels) { + if c.series_data_labels(s) { w.open("cx:dataLabels", A::new().a("pos", if c.kind == ChartType::Funnel { "ctr" } else { "outEnd" })); w.empty("cx:visibility", A::new().a("seriesName", "0").a("categoryName", "0").a("value", "1")); w.close("cx:dataLabels"); diff --git a/crates/pptx/tests/chart_data_labels.rs b/crates/pptx/tests/chart_data_labels.rs new file mode 100644 index 00000000..d89fae53 --- /dev/null +++ b/crates/pptx/tests/chart_data_labels.rs @@ -0,0 +1,154 @@ +//! Series-level data labels: `c:dLbls/c:showVal` on a series overrides the chart-wide setting, +//! and a bare `` (no `val` attribute) means true, as the schema's default (#868). + +use std::io::{Cursor, Read, Write}; +use std::sync::Arc; + +use deckcraft_geom::Xfrm; +use deckcraft_model::chart::{Chart, ChartType}; +use deckcraft_model::{LayoutType, Presentation, Shape, ShapeId, ShapeKind, defaults}; + +/// A one-slide deck whose bar chart shows no labels anywhere. +fn bar_chart_pptx() -> Vec { + let mut p = defaults::new_presentation(None); + p.slides.clear(); + let layout = defaults::layout_of_kind(&p, LayoutType::Blank).unwrap_or_default(); + let mut slide = defaults::new_slide(&mut p, layout).expect("slide"); + let mut shape = + Shape { id: ShapeId(p.alloc_id().expect("id")), name: "Bars".into(), xfrm: Some(Xfrm::new(40.0, 40.0, 400.0, 300.0)), ..Default::default() }; + let mut chart = Chart::sample(ChartType::Column); + chart.data_labels = false; + for s in &mut chart.series { + s.data_labels = None; + } + shape.kind = ShapeKind::Chart(Box::new(chart)); + slide.shapes.push(shape); + p.slides.push(Arc::new(slide)); + deckcraft_pptx::export(&p).expect("export") +} + +/// Rewrite the chart part's XML through `f` and re-import the deck. +fn reimport(bytes: &[u8], f: impl Fn(&str) -> String) -> Presentation { + let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip"); + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default(); + for i in 0..zip.len() { + let mut entry = zip.by_index(i).expect("entry"); + let name = entry.name().to_string(); + let mut data = vec![]; + entry.read_to_end(&mut data).expect("read"); + if name.starts_with("ppt/charts/chart") && name.ends_with(".xml") && !name.contains("_rels") { + let xml = String::from_utf8(data).expect("chart xml"); + data = f(&xml).into_bytes(); + } + out.start_file(&name, opts).expect("start"); + out.write_all(&data).expect("write"); + } + let patched = out.finish().expect("finish").into_inner(); + deckcraft_pptx::import(&patched).expect("import") +} + +fn chart_of(p: &Presentation) -> &Chart { + let s = p.slides.first().expect("slide"); + for sh in &s.shapes { + if let ShapeKind::Chart(c) = &sh.kind { + return c; + } + } + panic!("no chart in the deck"); +} + +/// `` on the first series: that series shows labels although the chart-wide +/// setting is off; the other series stay quiet. +#[test] +fn bare_series_showval_shows_labels() { + let p = reimport(&bar_chart_pptx(), |xml| xml.replacen("", r#""#, 1)); + let c = chart_of(&p); + assert!(!c.data_labels, "the chart-wide setting stays off"); + assert_eq!(c.series[0].data_labels, Some(true), "a bare showVal means true"); + assert!(c.series_data_labels(&c.series[0]), "the first series' labels show"); + for s in c.series.iter().skip(1) { + assert_eq!(s.data_labels, None, "other series keep the chart's setting"); + assert!(!c.series_data_labels(s), "and stay quiet"); + } +} + +/// `val="0"` on the series hides its labels although the chart-wide setting is on. +#[test] +fn series_showval_false_hides_labels() { + let p = reimport(&bar_chart_pptx(), |xml| { + xml.replacen("", r#""#, 1).replacen( + " Vec { + let mut p = defaults::new_presentation(None); + p.slides.clear(); + let layout = defaults::layout_of_kind(&p, LayoutType::Blank).unwrap_or_default(); + let mut slide = defaults::new_slide(&mut p, layout).expect("slide"); + let mut chart = Chart::sample(ChartType::Combo); + chart.series.truncate(2); + chart.series[1].kind = Some(ChartType::Line); + let mut shape = + Shape { id: ShapeId(p.alloc_id().expect("id")), name: "Bars".into(), xfrm: Some(Xfrm::new(40.0, 40.0, 400.0, 300.0)), ..Default::default() }; + shape.kind = ShapeKind::Chart(Box::new(chart)); + slide.shapes.push(shape); + p.slides.push(Arc::new(slide)); + deckcraft_pptx::export(&p).expect("export") +} + +/// A combo chart's groups keep their own label setting: the group that asks for no labels +/// keeps them off, even when the other group asks for them (#868). +#[test] +fn a_combo_chart_keeps_each_groups_own_label_setting() { + let p = reimport(&combo_chart_pptx(), |xml| { + // The bar group asks for no labels, the line group for labels. + xml.replacen("", + r#""#, + 1, + ) + }); + let c = chart_of(&p); + assert!(c.data_labels, "the line group's ask turns the chart-wide setting on"); + assert_eq!(c.series[0].data_labels, Some(false), "the bar group asks for no labels"); + assert!(!c.series_data_labels(&c.series[0]), "so the bars stay quiet"); + assert!(c.series_data_labels(&c.series[1]), "the line takes the chart-wide on"); +} diff --git a/crates/render/src/chart.rs b/crates/render/src/chart.rs index ac185780..bf047cb1 100644 --- a/crates/render/src/chart.rs +++ b/crates/render/src/chart.rs @@ -310,7 +310,7 @@ fn draw_inner(ctx: &mut RenderContext, rctx: &Ctx, c: &Chart, m: Affine, w: f64, ctx.set_paint(color(Rgba::WHITE, 1.0)); ctx.set_stroke(kurbo::Stroke::new(1.0)); ctx.stroke_path(&p); - if c.data_labels { + if c.series_data_labels(s) { let mid = a0 + sweep / 2.0; let lp = Point::new(cen.x + r * 0.65 * mid.cos(), cen.y + r * 0.65 * mid.sin()); label( @@ -656,7 +656,7 @@ fn draw_inner(ctx: &mut RenderContext, rctx: &Ctx, c: &Chart, m: Affine, w: f64, err_marks.push((mid, p0, p1, bar.min(base * 0.8) * 0.5, e.end_cap)); } } - if c.data_labels { + if c.series_data_labels(s) { let t = c.data_label(s, s.values.get(i).copied().flatten().unwrap_or(0.0)); let lr = if horiz { Rect::new(r.x1 + 2.0, r.y0, r.x1 + 40.0, r.y1) diff --git a/crates/render/src/chartex.rs b/crates/render/src/chartex.rs index e62105e8..b0f3fffd 100644 --- a/crates/render/src/chartex.rs +++ b/crates/render/src/chartex.rs @@ -24,7 +24,7 @@ pub fn draw(ctx: &mut RenderContext, rctx: &Ctx, c: &Chart, m: Affine, area: Rec return; } let value_label = |v: f64| c.data_label(s, v); - let labels = c.data_labels || c.kind == ChartType::Funnel; + let labels = c.series_data_labels(s) || c.kind == ChartType::Funnel; match c.kind { ChartType::Funnel => funnel(ctx, rctx, c, m, area, base, text_col, &vals, &value_label), ChartType::Treemap if !c.category_levels.is_empty() => tree_treemap(ctx, rctx, c, m, area, base, labels.then_some(&value_label)), @@ -107,7 +107,7 @@ fn region_tiles(ctx: &mut RenderContext, rctx: &Ctx, c: &Chart, m: Affine, area: ctx.fill_rect(&t); let dark = v.is_some_and(|v| (v - min) / span > 0.55); let mut text = name.clone(); - if c.data_labels + if c.series_data_labels(s) && let Some(v) = v { text = format!( diff --git a/crates/render/src/radar.rs b/crates/render/src/radar.rs index 3416c7ce..b99c644c 100644 --- a/crates/render/src/radar.rs +++ b/crates/render/src/radar.rs @@ -120,7 +120,7 @@ pub fn draw(ctx: &mut RenderContext, rctx: &Ctx, c: &Chart, m: Affine, area: Rec } } } - if c.data_labels || s.data_labels == Some(true) { + if c.series_data_labels(s) { for (i, q) in pts.iter().enumerate() { if let Some(v) = s.values.get(i).copied().flatten() { let t = c.data_label(s, v);