mirror of
https://github.com/storytold/deckcraft.git
synced 2026-11-01 06:26:59 -06:00
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 `<c:showVal/>` (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.
This commit is contained in:
@@ -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 |
|
||||
|
||||
@@ -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)];
|
||||
|
||||
@@ -213,7 +213,11 @@ fn chart_within(root: &El, mut budget: usize) -> Option<Chart> {
|
||||
if let Some(v) = g.child_val("holeSize").and_then(|v| v.parse::<f64>().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) {
|
||||
// `<c:showVal/>` 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<Chart> {
|
||||
secondary_axis: secondary,
|
||||
..Default::default()
|
||||
};
|
||||
if let Some(d) = s.child("dLbls").and_then(|d| d.child("showVal")).and_then(|v| v.bool("val")) {
|
||||
// `<c:showVal/>` 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) {
|
||||
|
||||
@@ -857,7 +857,7 @@ pub fn build_chartex(c: &Chart) -> Vec<u8> {
|
||||
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");
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
//! Series-level data labels: `c:dLbls/c:showVal` on a series overrides the chart-wide setting,
|
||||
//! and a bare `<c:showVal/>` (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<u8> {
|
||||
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");
|
||||
}
|
||||
|
||||
/// `<c:showVal/>` 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("<c:cat>", r#"<c:dLbls><c:showVal/></c:dLbls><c:cat>"#, 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("<c:cat>", r#"<c:dLbls><c:showVal val="0"/></c:dLbls><c:cat>"#, 1).replacen(
|
||||
"<c:gapWidth",
|
||||
r#"<c:dLbls><c:showVal/></c:dLbls><c:gapWidth"#,
|
||||
1,
|
||||
)
|
||||
});
|
||||
let c = chart_of(&p);
|
||||
assert!(c.data_labels, "the bare chart-wide showVal turns labels on");
|
||||
assert_eq!(c.series[0].data_labels, Some(false), "the explicit series false is read");
|
||||
assert!(!c.series_data_labels(&c.series[0]), "and hides that series' labels");
|
||||
assert!(c.series_data_labels(&c.series[1]), "the next series takes the chart-wide on");
|
||||
}
|
||||
|
||||
/// The series' label setting survives a save and load.
|
||||
#[test]
|
||||
fn series_label_settings_round_trip() {
|
||||
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::Column);
|
||||
chart.data_labels = true;
|
||||
chart.series[0].data_labels = Some(false);
|
||||
chart.series[1].data_labels = Some(true);
|
||||
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));
|
||||
|
||||
let bytes = deckcraft_pptx::export(&p).expect("export");
|
||||
let back = deckcraft_pptx::import(&bytes).expect("import");
|
||||
let c = chart_of(&back);
|
||||
assert!(c.data_labels);
|
||||
assert_eq!(c.series[0].data_labels, Some(false), "the explicit off survives");
|
||||
assert_eq!(c.series[1].data_labels, Some(true), "the explicit on survives");
|
||||
assert!(!c.series_data_labels(&c.series[0]));
|
||||
assert!(c.series_data_labels(&c.series[1]));
|
||||
}
|
||||
|
||||
/// A one-slide deck with a combo chart: a bar group and a line group.
|
||||
fn combo_chart_pptx() -> Vec<u8> {
|
||||
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("<c:gapWidth", r#"<c:dLbls><c:showVal val="0"/></c:dLbls><c:gapWidth"#, 1).replacen(
|
||||
"<c:marker val=\"1\"/>",
|
||||
r#"<c:dLbls><c:showVal/></c:dLbls><c:marker val="1"/>"#,
|
||||
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");
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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!(
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user