Variables: Export Data Sets marks visibility and image columns with # and @

Squashed from #5141 onto main.

crates/engine/src/cmd/variablecmds.rs

@@ -5,7 +5,8 @@
 //! variable, or a graph root to a `graph` variable (CSV data), put a row of values in
 //! each dataset, and `dataset.select` swaps the whole document to that row in one undo
 //! step. `dataset.import` / `dataset.export` move those rows as CSV (header: `name`
-//! then variable names); import also reads tab-separated files, files without the
+//! then variable names, which export writes with `#` before a visibility variable and `@`
+//! before an image variable); import also reads tab-separated files, files without the
 //! `name` column, and header cells that define new variables (`#` for visibility, `@` for
 //! image, no prefix for text). `variable.loadLibrary` replaces the variables and data sets with the
 //! text variables and data sets of a variable library, the XML file of the reference app's Save
@@ -807,7 +808,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Export Data Sets…",
             ["Window", "Variables"],
             None,
-            "{path?} write every dataset as CSV (header: name, then variables); without path → {csv, datasets}. Refused when the document has more than 1000 data sets or 256 variables, the most dataset.import reads",
+            "{path?} write every dataset as CSV (header: name, then one column per variable: `#Name` for a visibility variable, `@Name` for an image variable, and the name alone for a text or graph variable, as dataset.import reads them). Refused when the document has more than 1000 data sets or 256 variables, the most dataset.import reads, or when a variable's header is another variable's name (a visibility variable `Show` next to a text variable `#Show`) → {path, datasets, warning?}; without path → {csv, datasets, warning?}. `warning` names the variables whose header or values make a document without them refuse the file (a % header, a # header whose values aren't true or false); otherwise it names the variables such a document doesn't read back with their kind (text variables whose names start with # or @, graph variables)",
             has_doc,
             dataset_export
         ),

crates/engine/src/cmd/variablecmds/csv.rs

@@ -187,12 +187,66 @@ fn value_to_cell(v: &DataValue) -> String {
     }
 }
 
-/// Build the CSV of every dataset (header `name` then variables in definition order).
-fn datasets_to_csv(vars: &Variables) -> String {
+/// The header cell that [`datasets_to_csv`] writes for `v`, as [`header_variable`] reads it: `#`
+/// before a visibility variable's name, `@` before an image variable's name, and the name alone
+/// for a text or graph variable.
+fn export_header(v: &Variable) -> String {
+    match v.kind {
+        VariableKind::Visibility => format!("#{}", v.name),
+        VariableKind::Image => format!("@{}", v.name),
+        VariableKind::Text | VariableKind::Graph => v.name.clone(),
+    }
+}
+
+/// What [`dataset_import`] does with each variable's column of the CSV that [`datasets_to_csv`]
+/// writes, in a document without the variables: the first list names the variables whose header
+/// or values it refuses, so it doesn't import the file (a `%` header, a `#` header whose values
+/// aren't true or false, a header that names a variable an earlier column named), and the second
+/// the variables it reads as another variable (a text variable whose name starts with `#` or `@`,
+/// and a graph variable, which no header cell defines).
+fn kinds_a_new_document_loses(vars: &Variables) -> (Vec<&str>, Vec<&str>) {
+    const C: &str = "dataset.import";
+    let none = BTreeMap::new();
+    let mut defined = Vec::new();
+    let (mut refused, mut other) = (Vec::new(), Vec::new());
+    let mut seen = BTreeSet::new();
+    for (i, v) in vars.variables.iter().enumerate() {
+        let header = export_header(v);
+        let cell = header.trim();
+        // Column 1 holds the dataset names.
+        let read = if cell.is_empty() { None } else { header_variable(cell, i + 2, &none, &mut defined, C).ok() };
+        // The import refuses a header that names a variable an earlier column named.
+        let Some((name, kind)) = read.filter(|(name, _)| seen.insert(name.clone())) else {
+            refused.push(v.name.as_str());
+            continue;
+        };
+        let cells_read = vars.datasets.iter().filter_map(|d| d.values.get(&v.name)).all(|val| {
+            let cell = value_to_cell(val);
+            cell_to_value(kind, &cell, cell.is_empty(), &name, 2, C).is_ok()
+        });
+        if !cells_read {
+            refused.push(v.name.as_str());
+        } else if name != v.name || kind != v.kind {
+            other.push(v.name.as_str());
+        }
+    }
+    (refused, other)
+}
+
+/// Build the CSV of every dataset (header `name` then each variable's [`export_header`] in
+/// definition order). Refused when a variable's header is another variable's name, which the
+/// import reads as that variable (a visibility variable `Show` next to a text variable `#Show`).
+fn datasets_to_csv(vars: &Variables, cmd: &str) -> Result<String> {
+    let names: BTreeSet<&str> = vars.variables.iter().map(|v| v.name.as_str()).collect();
     let mut out = String::from("name");
     for v in &vars.variables {
+        let header = export_header(v);
+        if header != v.name && names.contains(header.as_str()) {
+            let (name, other) = (&v.name, &header);
+            return Err(bad(cmd, format!("the header `{header}` for `{name}` would read back as the variable `{other}`: rename one of them")));
+        }
         out.push(',');
-        out.push_str(&csv_escape(&v.name));
+        out.push_str(&csv_escape(&header));
     }
     out.push('\n');
     for ds in &vars.datasets {
@@ -205,7 +259,7 @@ fn datasets_to_csv(vars: &Variables) -> String {
         }
         out.push('\n');
     }
-    out
+    Ok(out)
 }
 
 /// The variable that a datasets CSV header cell (trimmed, not empty) in 1-based `column` names,
@@ -343,7 +397,10 @@ pub(super) fn dataset_import(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({"imported": imported, "updated": updated, "names": names, "defined": new_variables}))
 }
 
-/// Export every dataset as CSV (`name` then variables). With `path`, write the file; else return `{csv}`.
+/// Export every dataset as CSV ([`datasets_to_csv`]). With `path`, write the file; else return
+/// `{csv}`. The reply's `warning` names the variables whose column makes a document without them
+/// refuse the file, or else the variables it doesn't read back with their kind
+/// ([`kinds_a_new_document_loses`]); the app shows it in its status bar.
 /// A document with more data sets or variables than [`dataset_import`] reads is refused.
 pub(super) fn dataset_export(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "dataset.export";
@@ -358,16 +415,26 @@ pub(super) fn dataset_export(s: &mut Session, p: &Value) -> Result<Value> {
     if st.doc.variables.variables.len() > MAX_CSV_COLS {
         return Err(bad(C, format!("more than {MAX_CSV_COLS} variables: Import Data Sets reads at most {MAX_CSV_COLS}")));
     }
-    let csv = datasets_to_csv(&st.doc.variables);
+    let csv = datasets_to_csv(&st.doc.variables, C)?;
     if csv.len() > MAX_CSV_BYTES {
         return Err(bad(C, format!("CSV would be larger than {MAX_CSV_BYTES} bytes")));
     }
-    match str_param(p, "path") {
+    let mut out = match str_param(p, "path") {
         Some(path) => {
             let path = absolute_path(path);
             write_file(&path, csv.as_bytes())?;
-            Ok(json!({"path": path, "datasets": st.doc.variables.datasets.len()}))
+            json!({"path": path, "datasets": st.doc.variables.datasets.len()})
         }
-        None => Ok(json!({"csv": csv, "datasets": st.doc.variables.datasets.len()})),
+        None => json!({"csv": csv, "datasets": st.doc.variables.datasets.len()}),
+    };
+    let (refused, lost) = kinds_a_new_document_loses(&st.doc.variables);
+    let list = |vars: &[&str]| vars.iter().map(|n| format!("`{n}`")).collect::<Vec<_>>().join(", ");
+    if !refused.is_empty() {
+        let names = list(&refused);
+        out["warning"] = json!(format!("a new document refuses this file because of these variables: {names}"));
+    } else if !lost.is_empty() {
+        let names = list(&lost);
+        out["warning"] = json!(format!("a new document won't read these variables back with their kind: {names}"));
     }
+    Ok(out)
 }

crates/engine/src/tests/variables.rs

@@ -1521,13 +1521,13 @@ fn datasets_csv_header_prefixes_define_variables() {
 #[test]
 fn datasets_csv_exact_names_win_over_prefixes() {
     let mut s = session();
-    for (name, kind) in [("#Shown", "text"), ("@Photo", "visibility"), ("%Chart", "graph"), ("Shown", "visibility")] {
+    for (name, kind) in [("#Shown", "text"), ("@Photo", "visibility"), ("%Chart", "graph"), ("Shown", "image")] {
         s.execute("variable.define", &json!({"name": name, "kind": kind})).unwrap();
     }
-    let values = json!({"#Shown": "hello", "@Photo": true, "%Chart": ",Q\nX,3", "Shown": false});
+    let values = json!({"#Shown": "hello", "@Photo": true, "%Chart": ",Q\nX,3", "Shown": "s.png"});
     s.execute("dataset.new", &json!({"name": "A", "values": values})).unwrap();
     let csv = s.execute("dataset.export", &json!({})).unwrap()["csv"].as_str().unwrap().to_string();
-    assert!(csv.starts_with("name,#Shown,@Photo,%Chart,Shown\n"), "{csv}");
+    assert!(csv.starts_with("name,#Shown,#@Photo,%Chart,@Shown\n"), "{csv}");
     s.execute("dataset.delete", &json!({"names": ["A"]})).unwrap();
     let imp = s.execute("dataset.import", &json!({"csv": csv})).unwrap();
     assert_eq!((imp["imported"].as_u64(), imp["defined"].clone()), (Some(1), json!([])));
@@ -1535,10 +1535,91 @@ fn datasets_csv_exact_names_win_over_prefixes() {
     assert_eq!(value("#Shown"), Some(vectorcraft_doc::DataValue::Text("hello".into())));
     assert_eq!(value("@Photo"), Some(vectorcraft_doc::DataValue::Visible(true)));
     assert_eq!(value("%Chart"), Some(vectorcraft_doc::DataValue::Graph(",Q\nX,3".into())));
-    assert_eq!(value("Shown"), Some(vectorcraft_doc::DataValue::Visible(false)));
+    assert_eq!(value("Shown"), Some(vectorcraft_doc::DataValue::Image("s.png".into())));
     assert_eq!(s.doc().unwrap().doc.variables.variables.len(), 4);
 }
 
+/// `dataset.export` writes `#` before a visibility variable's header and `@` before an image
+/// variable's, so a document without the variables reads the file back with their kinds. The
+/// reply's `warning` names the variables it doesn't: a text variable whose name starts with `#`,
+/// `@` or `%`, and a graph variable.
+#[test]
+fn datasets_csv_export_marks_visibility_and_image_columns() {
+    let mut s = session();
+    for (name, kind) in [("Name", "text"), ("Show", "visibility"), ("Photo", "image")] {
+        s.execute("variable.define", &json!({"name": name, "kind": kind})).unwrap();
+    }
+    s.execute("dataset.new", &json!({"name": "A", "values": {"Name": "Ann", "Show": false, "Photo": "a.png"}})).unwrap();
+    let out = s.execute("dataset.export", &json!({})).unwrap();
+    assert_eq!(out["csv"], "name,Name,#Show,@Photo\nA,Ann,false,a.png\n");
+    assert!(out.get("warning").is_none(), "{out}");
+
+    let mut fresh = session();
+    let imp = fresh.execute("dataset.import", &json!({"csv": out["csv"]})).unwrap();
+    assert_eq!(imp["defined"], json!(["Name", "Show", "Photo"]));
+    let listed = fresh.execute("variable.list", &json!({})).unwrap();
+    let kinds: Vec<_> = listed["variables"].as_array().unwrap().iter().map(|v| v["kind"].clone()).collect();
+    assert_eq!(kinds, [json!("text"), json!("visibility"), json!("image")]);
+    let value = |var: &str| fresh.doc().unwrap().doc.variables.dataset("A").and_then(|d| d.values.get(var)).cloned();
+    assert_eq!(value("Show"), Some(vectorcraft_doc::DataValue::Visible(false)));
+    assert_eq!(value("Photo"), Some(vectorcraft_doc::DataValue::Image("a.png".into())));
+
+    for (name, kind) in [("#Tag", "text"), ("Chart", "graph"), ("@Note", "text")] {
+        s.execute("variable.define", &json!({"name": name, "kind": kind})).unwrap();
+    }
+    let out = s.execute("dataset.export", &json!({})).unwrap();
+    assert_eq!(out["warning"], "a new document won't read these variables back with their kind: `#Tag`, `Chart`, `@Note`");
+    assert!(out["csv"].as_str().unwrap().starts_with("name,Name,#Show,@Photo,#Tag,Chart,@Note\n"), "{out}");
+}
+
+/// The export's `warning` names the variables whose column a document without the variables
+/// refuses, so it doesn't import the file: a `%` header, a `#` header whose values aren't true or
+/// false, and a header that names a variable an earlier column defined with another kind. A
+/// header that names such a variable with its kind is read with that kind, and is listed as one
+/// read back with another kind.
+#[test]
+fn datasets_csv_export_warns_of_headers_a_new_document_refuses() {
+    let export = |vars: &[(&str, &str)], values: Value| {
+        let mut s = session();
+        for (name, kind) in vars {
+            s.execute("variable.define", &json!({"name": name, "kind": kind})).unwrap();
+        }
+        s.execute("dataset.new", &json!({"name": "A", "values": values})).unwrap();
+        let out = s.execute("dataset.export", &json!({})).unwrap();
+        let refused = session().execute("dataset.import", &json!({"csv": out["csv"]})).is_err();
+        (out["warning"].as_str().unwrap_or("").to_string(), refused)
+    };
+    let refuses = |names: &str| format!("a new document refuses this file because of these variables: {names}");
+    let loses = |names: &str| format!("a new document won't read these variables back with their kind: {names}");
+
+    assert_eq!(export(&[("%Rate", "text"), ("Chart", "graph")], json!({})), (refuses("`%Rate`"), true));
+    assert_eq!(export(&[("#Tag", "text")], json!({"#Tag": "hello"})), (refuses("`#Tag`"), true));
+    assert_eq!(export(&[("#Tag", "text")], json!({"#Tag": "yes"})), (loses("`#Tag`"), false));
+    assert_eq!(export(&[("#Seen", "text"), ("Seen", "image")], json!({})), (refuses("`Seen`"), true));
+    assert_eq!(export(&[("#Seen", "text"), ("Seen", "text")], json!({})), (refuses("`Seen`"), true));
+}
+
+/// `dataset.export` is refused, naming both variables, when a variable's header is another
+/// variable's name, which an import reads as that variable; with `path`, no file is written.
+#[test]
+fn datasets_csv_export_refuses_a_header_that_names_another_variable() {
+    let dir = Folder::new("vars-csv-header");
+    for (first, second) in
+        [(("Show", "visibility"), ("#Show", "text")), (("Photo", "image"), ("@Photo", "graph")), (("X", "visibility"), ("#X", "image"))]
+    {
+        let mut s = session();
+        for (name, kind) in [first, second] {
+            s.execute("variable.define", &json!({"name": name, "kind": kind})).unwrap();
+        }
+        let e = s.execute("dataset.export", &json!({})).unwrap_err().to_string();
+        let (name, other) = (first.0, second.0);
+        assert!(e.contains(&format!("for `{name}` would read back as the variable `{other}`")), "{e}");
+        let out = dir.file("sets.csv");
+        assert!(s.execute("dataset.export", &json!({"path": out})).is_err());
+        assert!(!std::path::Path::new(&out).exists(), "{out}");
+    }
+}
+
 /// A `%` header cell that isn't a variable's name (graph data files) is refused with its column,
 /// and the import defines nothing.
 #[test]
@@ -1621,7 +1702,7 @@ fn datasets_csv_round_trips_and_rejects_bad_input() {
 
     let exported = s.execute("dataset.export", &json!({})).unwrap();
     let csv = exported["csv"].as_str().unwrap().to_string();
-    assert!(csv.starts_with("name,Name,Show,Photo\n"), "{csv}");
+    assert!(csv.starts_with("name,Name,#Show,@Photo\n"), "{csv}");
     assert!(csv.contains("Alice") && csv.contains("Bob") && csv.contains("false"), "{csv}");
 
     // Clear rows, import again, apply B.