Merge pull request #5136 from dotCooCoo/fix/vars-p5

Variables: a graph variable's data set applies its CSV as graph.setData does

crates/engine/src/cmd/graph/data.rs

@@ -86,7 +86,8 @@ pub(super) fn to_csv(g: &GraphSpec) -> String {
     g.to_csv()
 }
 
-/// Apply CSV graph data to the graph root `id` and rebuild its art (Variables data merge).
+/// Apply CSV graph data to the graph root `id` and rebuild its art (Variables data merge), as `graph.setData` with
+/// `csv` does.
 pub(crate) fn apply_csv(d: &mut Document, id: NodeId, csv: &str) -> Result<()> {
     let Some(node) = d.node(id) else {
         return Err(EngineError::NoNode(id));
@@ -94,13 +95,17 @@ pub(crate) fn apply_csv(d: &mut Document, id: NodeId, csv: &str) -> Result<()> {
     let Some(mut spec) = node.extras().graph.as_deref().cloned() else {
         return Err(bad("graph.applyCsv", format!("node {} is not a graph", id.0)));
     };
-    let (series, categories, rows) = parse_csv(csv);
-    spec.series = series;
-    spec.categories = categories;
-    spec.set_cells(rows);
+    apply_data(&mut spec, &json!({ "csv": csv }));
+    keep_right_series(&mut spec);
     regenerate(d, id, spec)
 }
 
+/// Drop right-axis series the data no longer has, so a series added later starts on the left.
+fn keep_right_series(g: &mut GraphSpec) {
+    let count = g.rows.iter().map(Vec::len).max().unwrap_or(0);
+    g.right_series.retain(|i| *i < count);
+}
+
 pub(super) fn apply_data(g: &mut GraphSpec, p: &Value) -> bool {
     let mut changed = false;
     if let Some(csv) = str_param(p, "csv") {
@@ -131,9 +136,17 @@ pub(super) fn apply_data(g: &mut GraphSpec, p: &Value) -> bool {
     changed
 }
 
-/// Drop rows with neither a value nor a label; a label-only row stays, its values blank.
+/// Drop rows with neither a value nor a label; a label-only row stays, its values blank. Each kept row keeps its own
+/// width, so a dropped row wider than the rest adds no blank series.
 fn drop_empty_rows(g: &mut GraphSpec) {
-    let mut cells = g.cells();
+    let blank: std::collections::HashSet<[usize; 2]> = g.blanks.iter().copied().collect();
+    let mut cells: Vec<Vec<Option<f64>>> = g
+        .rows
+        .iter()
+        .take(vectorcraft_doc::MAX_GRAPH_CATEGORIES)
+        .enumerate()
+        .map(|(c, r)| r.iter().take(vectorcraft_doc::MAX_GRAPH_SERIES).enumerate().map(|(s, v)| (!blank.contains(&[c, s])).then_some(*v)).collect())
+        .collect();
     let keep: Vec<bool> = (0..cells.len())
         .map(|c| cells.get(c).is_some_and(|r| r.iter().any(Option::is_some)) || g.categories.get(c).is_some_and(|l| !l.is_empty()))
         .collect();
@@ -185,10 +198,7 @@ pub(super) fn set_data(s: &mut Session, p: &Value) -> Result<Value> {
         // Blank cells come back as null, so the rows can be edited and sent back as they are.
         return Ok(json!({ "csv": to_csv(&spec), "series": spec.series, "categories": spec.categories, "rows": spec.cells() }));
     }
-    // Series assigned to the right axis that the new data no longer has are dropped, so a series added later starts
-    // on the left.
-    let count = spec.rows.iter().map(Vec::len).max().unwrap_or(0);
-    spec.right_series.retain(|i| *i < count);
+    keep_right_series(&mut spec);
     s.edit("Graph Data", |d, _| regenerate(d, id, spec))?;
     Ok(json!({ "id": id.0 }))
 }

crates/engine/src/tests/variables.rs

@@ -1646,6 +1646,37 @@ fn locked_variables_refuse_a_library_that_deletes_or_defines_variables() {
     assert_eq!(vars.datasets.iter().map(|d| d.name.as_str()).collect::<Vec<_>>(), ["B"]);
 }
 
+/// A graph variable's data set applies its CSV as `graph.setData` does: a right-axis series the row's data no longer
+/// has is dropped, and a row with neither a label nor a value is not drawn as an empty slot.
+#[test]
+fn graph_variable_applies_csv_as_set_data() {
+    let mut s = session();
+    let create = json!({"type": "column", "x": 10, "y": 10, "width": 120, "height": 80, "csv": ",a,b\nQ1,10,1000\nQ2,5,500"});
+    let id = s.execute("graph.create", &create).unwrap()["id"].as_u64().unwrap();
+    let other = s.execute("graph.create", &create).unwrap()["id"].as_u64().unwrap();
+    for g in [id, other] {
+        s.execute("graph.setType", &json!({"id": g, "seriesIndexes": [1], "valueAxis": "right"})).unwrap();
+    }
+    s.execute("select.set", &json!({"ids": [id]})).unwrap();
+    s.execute("variable.makeGraphDynamic", &json!({})).unwrap();
+    let name = s.execute("variable.list", &json!({})).unwrap()["variables"][0]["name"].as_str().unwrap().to_string();
+    let spec = |s: &Session, g: u64| s.doc().unwrap().doc.node(NodeId(g)).unwrap().extras().graph.as_deref().unwrap().clone();
+    // A dropped empty row wider than the others (`,,`) adds no blank series, so the right axis keeps nothing.
+    for (row, csv) in [("Wide", ",a\nQ1,10\n,,\nQ2,5"), ("One", ",a\nQ1,10\nQ2,5"), ("Blank", ",a\nQ1,10\n,\nQ2,5")] {
+        s.execute("dataset.new", &json!({"name": row, "values": {name.clone(): csv}})).unwrap();
+        s.execute("dataset.select", &json!({"name": row})).unwrap();
+        s.execute("graph.setData", &json!({"id": other, "csv": csv})).unwrap();
+        let (merged, set) = (spec(&s, id), spec(&s, other));
+        assert_eq!(merged.right_series, set.right_series, "{row}");
+        assert_eq!(merged.categories, set.categories, "{row}");
+        assert_eq!(merged.cells(), set.cells(), "{row}");
+        assert!(merged.right_series.is_empty(), "{row}");
+        assert_eq!(merged.cells(), [[Some(10.0)], [Some(5.0)]], "{row}");
+    }
+    assert!(spec(&s, id).right_series.is_empty());
+    assert_eq!(spec(&s, id).categories, ["Q1", "Q2"]);
+}
+
 #[test]
 fn graph_variable_applies_csv_and_captures() {
     let mut s = session();