diff --git a/crates/engine/src/cmd/styles.rs b/crates/engine/src/cmd/styles.rs index 1243c7f9..af029e5f 100644 --- a/crates/engine/src/cmd/styles.rs +++ b/crates/engine/src/cmd/styles.rs @@ -110,16 +110,26 @@ pub fn specs() -> Vec { )) }), 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 = 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})) diff --git a/crates/engine/src/variables_behaviour_tests.rs b/crates/engine/src/variables_behaviour_tests.rs index 4e24b3fe..3fe156f8 100644 --- a/crates/engine/src/variables_behaviour_tests.rs +++ b/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 = 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