Merge pull request #842 from Silvertree2010/fix/h2814-variable-createcollection-accept

Refuse empty mode lists and blank names in variable.createCollection

crates/engine/src/cmd/styles.rs

@@ -110,16 +110,26 @@ pub fn specs() -> Vec<CommandSpec> {
             ))
         }),
         spec!("variable.createCollection", "Create variable collection", "{name, modes? [names]}", true, |s, p| {
-            let name = req_string("variable.createCollection", p, "name")?;
-            let doc = s.doc_mut()?;
-            if doc.variables.collections.iter().any(|c| c.name == name) {
-                return Err(bad("variable.createCollection", format!("a collection named `{name}` already exists")));
+            let cmd = "variable.createCollection";
+            let name = req_string(cmd, p, "name")?.trim().to_string();
+            if name.is_empty() {
+                return Err(bad(cmd, "name can't be empty"));
             }
-            let id = doc.new_collection_id();
             let modes: Vec<String> = match get(p, "modes") {
-                Some(Value::Array(a)) => a.iter().filter_map(|m| m.as_str().map(str::to_string)).collect(),
+                Some(Value::Array(a)) => a.iter().filter_map(|m| m.as_str().map(|m| m.trim().to_string())).collect(),
                 _ => vec!["Mode 1".into()],
             };
+            if modes.is_empty() {
+                return Err(bad(cmd, "a collection needs at least one mode"));
+            }
+            if modes.iter().any(String::is_empty) {
+                return Err(bad(cmd, "mode names can't be empty"));
+            }
+            let doc = s.doc_mut()?;
+            if doc.variables.collections.iter().any(|c| c.name == name) {
+                return Err(bad(cmd, format!("a collection named `{name}` already exists")));
+            }
+            let id = doc.new_collection_id();
             let modes = modes.into_iter().enumerate().map(|(i, m)| Mode { id: format!("{id}/mode{}", i + 1), name: m }).collect();
             doc.variables.collections.push(Collection { id: id.clone(), name, modes, extends: None });
             Ok(json!({"id": id}))

crates/engine/src/variables_behaviour_tests.rs

@@ -394,6 +394,27 @@ fn number_variables_are_pixels_and_edits_are_one_undo_step() {
     assert_eq!(lh(&s), LineHeight::Pixels(20.0), "the value and the bound layer go back together");
 }
 
+#[test]
+fn create_collection_keeps_at_least_one_named_mode() {
+    // A collection keeps at least one mode (removeMode) and mode names can't be empty (renameMode),
+    // so createCollection refuses an empty mode list, blank mode names and a blank collection name.
+    let mut s = Session::new();
+    for p in [
+        json!({"name": "C", "modes": []}),
+        json!({"name": "C", "modes": [1]}),
+        json!({"name": "C", "modes": ["", "  "]}),
+        json!({"name": "C", "modes": ["A", " "]}),
+        json!({"name": "  "}),
+        json!({"name": ""}),
+    ] {
+        assert!(s.execute("variable.createCollection", &p).is_err(), "{p} should be refused");
+    }
+    assert!(s.doc().unwrap().variables.collections.is_empty(), "a refused call creates nothing");
+    run(&mut s, "variable.createCollection", json!({"name": "C", "modes": [" Light ", "Dark"]}));
+    let names: Vec<String> = s.doc().unwrap().variables.collections[0].modes.iter().map(|m| m.name.clone()).collect();
+    assert_eq!(names, ["Light", "Dark"]);
+}
+
 #[test]
 fn explicit_edits_of_bound_fields_stick_and_detach_that_binding() {
     // #809: an edit of a bound field detaches its binding (as a paint edit does) instead of