M3: component properties keep their order on rename; Make slot asks for the name (#1536, #1537) (#1565)

Properties were listed by name, so a rename moved one. Components now keep prop_order (the names as they were made; a rename keeps the place) and every listing follows it. Make slot opens a name field under the frame's Slot row; Enter makes the slot, Escape cancels, a taken name keeps the field open.

crates/engine/src/assets/components.rs

@@ -284,6 +284,16 @@ pub fn default_props(comp: &Node) -> BTreeMap<String, Value> {
     out
 }
 
+/// Component `c`'s property definitions, in the order they show ([`figcraft_doc::ordered_props`]).
+pub fn prop_list(doc: &Document, c: NodeId) -> Vec<(String, PropDef)> {
+    match doc.node(c).map(|n| &n.kind) {
+        Some(NodeKind::Component { props, prop_order, .. }) => {
+            figcraft_doc::ordered_props(props, prop_order).into_iter().map(|(k, d)| (k.clone(), d.clone())).collect()
+        }
+        _ => Vec::new(),
+    }
+}
+
 /// Whether instances store a value for the property (every kind but slots).
 pub fn takes_value(d: &PropDef) -> bool {
     !matches!(d, PropDef::Slot { .. })

crates/engine/src/assets/components/rebase_source_tests.rs

@@ -7,7 +7,7 @@ fn add(doc: &mut Document, parent: NodeId, kind: NodeKind, name: &str) -> NodeId
 }
 
 fn component(doc: &mut Document, parent: NodeId, name: &str) -> NodeId {
-    add(doc, parent, NodeKind::Component { description: String::new(), props: BTreeMap::new() }, name)
+    add(doc, parent, NodeKind::Component { description: String::new(), props: BTreeMap::new(), prop_order: Vec::new() }, name)
 }
 
 fn mirror(doc: &mut Document, source: NodeId, parent: NodeId) -> NodeId {

crates/engine/src/automation/inspect.rs

@@ -288,7 +288,7 @@ fn kind_props(doc: &Document, n: &Node, o: &mut serde_json::Map<String, Value>)
                 o.insert("componentProperties".into(), json!(props));
             }
         }
-        NodeKind::Component { description, props } => {
+        NodeKind::Component { description, props, .. } => {
             if !description.is_empty() {
                 o.insert("description".into(), json!(description));
             }

crates/engine/src/cmd/component.rs

@@ -342,7 +342,7 @@ fn create(s: &mut Session, p: &Value) -> Result<Value> {
     let doc = s.doc_mut()?;
     let n = doc.get_mut(target)?;
     let was_group = matches!(n.kind, NodeKind::Group);
-    n.kind = NodeKind::Component { description: String::new(), props: Default::default() };
+    n.kind = NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() };
     if was_group {
         n.fills.clear();
         n.clip = false;

crates/engine/src/cmd/component/instance.rs

@@ -331,7 +331,7 @@ pub(super) fn restore_main(s: &mut Session, p: &Value) -> Result<Value> {
         let Some(mut n) = doc.node(*d).cloned() else { continue };
         if *d == i {
             let defs = restore_prop_defs(doc, &props);
-            n.kind = NodeKind::Component { description: String::new(), props: defs };
+            n.kind = NodeKind::Component { description: String::new(), props: defs, prop_order: Vec::new() };
             n.source = None;
             n.overrides.clear();
             n.prop_refs.clear();

crates/engine/src/cmd/component/props.rs

@@ -1,3 +1,5 @@
+use figcraft_doc::ordered_props;
+
 use super::instance::preferred_arg;
 use super::variants::{fit_set, wrap_in_set};
 use super::{
@@ -6,6 +8,7 @@ use super::{
     req_string, req_targets, set_boolean_default, slot_edited, slots_of, string, takes_value, targets, variant_conflicts, variant_name,
     variant_options, variant_props, variant_set,
 };
+use crate::components::prop_list;
 
 /// Whether instance `id` has a component property `name` that a `kind` variable can drive (a
 /// boolean property takes a boolean variable, a text or variant property a string one).
@@ -195,9 +198,22 @@ pub(super) fn prop_defs(doc: &Document, c: NodeId) -> BTreeMap<String, PropDef>
     }
 }
 
-fn set_prop_defs(doc: &mut Document, c: NodeId, f: impl FnOnce(&mut BTreeMap<String, PropDef>)) -> Result<()> {
-    if let NodeKind::Component { props, .. } = &mut doc.get_mut(c)?.kind {
+/// Change component `c`'s property definitions with `f`, keeping their order: the property
+/// `rename` names (old, new) keeps its place, new ones go last.
+fn set_prop_defs(doc: &mut Document, c: NodeId, rename: Option<(&str, &str)>, f: impl FnOnce(&mut BTreeMap<String, PropDef>)) -> Result<()> {
+    if let NodeKind::Component { props, prop_order, .. } = &mut doc.get_mut(c)?.kind {
+        let mut order: Vec<String> = ordered_props(props, prop_order)
+            .into_iter()
+            .map(|(k, _)| match rename {
+                Some((old, new)) if k == old => new.to_string(),
+                _ => k.clone(),
+            })
+            .collect();
         f(props);
+        order.retain(|k| props.contains_key(k));
+        let added: Vec<String> = props.keys().filter(|k| !order.contains(k)).cloned().collect();
+        order.extend(added);
+        *prop_order = order;
     }
     Ok(())
 }
@@ -280,7 +296,7 @@ pub(super) fn add_prop_for(cmd: &str, s: &mut Session, p: &Value) -> Result<Valu
         _ => return Err(bad(cmd, "type: boolean|text|instanceSwap|variant|slot")),
     };
     for c in &comps {
-        set_prop_defs(doc, *c, |m| {
+        set_prop_defs(doc, *c, None, |m| {
             m.insert(name.clone(), def.clone());
         })?;
         // Existing instances get the default.
@@ -508,10 +524,9 @@ pub(super) fn edit_prop(s: &mut Session, p: &Value) -> Result<Value> {
         }
         let key = new_name.clone().unwrap_or(name.clone());
         let nd = new_def.clone();
-        let old = name.clone();
-        set_prop_defs(doc, *c, move |m| {
-            m.remove(&old);
-            m.insert(key, nd);
+        set_prop_defs(doc, *c, Some((&name, &key)), |m| {
+            m.remove(&name);
+            m.insert(key.clone(), nd);
         })?;
         if let Some(nn) = &new_name {
             rename_prop_refs(doc, *c, &name, Some(nn))?;
@@ -547,7 +562,7 @@ pub(super) fn delete_prop(s: &mut Session, p: &Value) -> Result<Value> {
     for c in &comps {
         if prop_defs(doc, *c).contains_key(&name) {
             found = true;
-            set_prop_defs(doc, *c, |m| {
+            set_prop_defs(doc, *c, None, |m| {
                 m.remove(&name);
             })?;
             rename_prop_refs(doc, *c, &name, None)?;
@@ -591,7 +606,7 @@ pub(crate) fn describe_props(doc: &Document, id: NodeId) -> Value {
     }
     // Slots: the frames (on a main) or this instance's copy, with whether it holds its own content.
     let slots = slots_of(doc, if values.is_some() { id } else { variant_set(doc, comp).unwrap_or(comp) });
-    for (k, d) in prop_defs(doc, comp) {
+    for (k, d) in prop_list(doc, comp) {
         let mut v = def_json(&k, &d);
         if !takes_value(&d) {
             let layers: Vec<NodeId> = slots.iter().filter(|(_, n)| *n == k).map(|(l, _)| *l).collect();

crates/engine/src/cmd/create.rs

@@ -173,7 +173,7 @@ pub fn kind_for(t: &str) -> Option<(NodeKind, &'static str)> {
         "text" => (NodeKind::Text(TextData::default()), "Text"),
         "frame" => (NodeKind::Frame, "Frame"),
         "section" => (NodeKind::Section { contents_hidden: false }, "Section"),
-        "component" => (NodeKind::Component { description: String::new(), props: Default::default() }, "Component"),
+        "component" => (NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() }, "Component"),
         "group" => (NodeKind::Group, "Group"),
         "slice" => (NodeKind::Slice, "Slice"),
         _ => return None,

crates/engine/src/cmd/file/rest_import_tests.rs

@@ -31,7 +31,15 @@ fn cached_import() -> Document {
     let page = doc.first_page().unwrap();
     let store = doc.ensure_library_store();
     let main = doc
-        .create(store, NodeKind::Component { description: String::new(), props: Default::default() }, "Cached main", 0.0, 0.0, 20.0, 20.0)
+        .create(
+            store,
+            NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() },
+            "Cached main",
+            0.0,
+            0.0,
+            20.0,
+            20.0,
+        )
         .unwrap();
     let body = doc.create(main, NodeKind::Rectangle, "Body", 0.0, 0.0, 20.0, 20.0).unwrap();
     let inst = doc.create(page, NodeKind::Instance { component: main, props: Default::default() }, "Cached instance", 0.0, 0.0, 20.0, 20.0).unwrap();

crates/engine/src/cmd/file/thumbnail_tests.rs

@@ -66,7 +66,8 @@ fn invalid_flags_and_non_frames_keep_the_default_thumbnail() {
     choose(&mut s, wide);
     s.doc_mut().unwrap().get_mut(wide).unwrap().kind = NodeKind::Rectangle;
     assert_eq!(thumbnail(s.doc().unwrap()).unwrap(), default, "only frames can be thumbnails");
-    s.doc_mut().unwrap().get_mut(wide).unwrap().kind = NodeKind::Component { description: String::new(), props: Default::default() };
+    s.doc_mut().unwrap().get_mut(wide).unwrap().kind =
+        NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() };
     assert_ne!(thumbnail(s.doc().unwrap()).unwrap(), default, "a component counts as a frame");
 }
 

crates/engine/src/content/synthetic.rs

@@ -82,8 +82,15 @@ pub fn large_document(layers: usize) -> DocResult<(Document, NodeId)> {
     let page = doc.first_page().ok_or(figcraft_doc::DocError::Invalid("new document has no page".into()))?;
     let layers = layers.clamp(10, 2_000_000);
     // The shared button component (3 layers), above the screens.
-    let button =
-        doc.create(page, NodeKind::Component { description: String::new(), props: Default::default() }, "Button", 0.0, -120.0, 96.0, 32.0)?;
+    let button = doc.create(
+        page,
+        NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() },
+        "Button",
+        0.0,
+        -120.0,
+        96.0,
+        32.0,
+    )?;
     let bg = doc.create(button, NodeKind::Rectangle, "Background", 0.0, 0.0, 96.0, 32.0)?;
     fill(&mut doc, bg, Color::from_rgb8_const(0x0D, 0x99, 0xFF));
     if let Some(n) = doc.node_mut(bg) {

crates/engine/src/content/synthetic_ds.rs

@@ -118,7 +118,7 @@ fn var_id(name: &str) -> String {
 }
 
 fn component() -> NodeKind {
-    NodeKind::Component { description: String::new(), props: BTreeMap::new() }
+    NodeKind::Component { description: String::new(), props: BTreeMap::new(), prop_order: Vec::new() }
 }
 
 fn text(chars: &str) -> NodeKind {

crates/engine/src/lib.rs

@@ -427,6 +427,8 @@ pub(crate) mod plugins_tests;
 #[cfg(test)]
 mod progressive_blur_tests;
 #[cfg(test)]
+mod prop_order_tests;
+#[cfg(test)]
 mod property_copy_tests;
 #[cfg(test)]
 mod proto_behaviour_tests;

crates/engine/src/node_find_tests.rs

@@ -83,7 +83,8 @@ fn node_find_other_page_requires_root_and_does_not_switch_pages() {
 fn node_find_instances_match_main_name_and_visit_actual_children_only() {
     let mut s = Session::new();
     let page = s.page().unwrap();
-    let main = add(&mut s, page, NodeKind::Component { description: String::new(), props: Default::default() }, "CAFÉ [Button]");
+    let main =
+        add(&mut s, page, NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() }, "CAFÉ [Button]");
     let original = add(&mut s, main, NodeKind::Text(TextData::new("Original")), "Original");
     let inst = idv(&ex(&mut s, "instance.create", json!({"component": main})));
     let child = s.doc().unwrap().get(inst).unwrap().children[0];
@@ -116,7 +117,7 @@ fn node_find_accepts_every_canonical_type() {
         NodeKind::Vector { network: Default::default() },
         NodeKind::Text(TextData::new("Text")),
         NodeKind::BooleanOp { op: BoolOp::Union },
-        NodeKind::Component { description: String::new(), props: Default::default() },
+        NodeKind::Component { description: String::new(), props: Default::default(), prop_order: Vec::new() },
         NodeKind::ComponentSet { description: String::new() },
         NodeKind::Instance { component: NodeId(u64::MAX), props: Default::default() },
         NodeKind::Slice,

crates/engine/src/prop_order_tests.rs

@@ -0,0 +1,135 @@
+//! Component properties keep the order they were made in: renaming one never moves it (#1536),
+//! and Make slot takes the name it is given (#1537).
+
+use figcraft_doc::{NodeId, NodeKind};
+use serde_json::{Value, json};
+
+use crate::Session;
+
+fn ex(s: &mut Session, cmd: &str, p: Value) -> Value {
+    s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd} {p}: {e}"))
+}
+
+fn idv(v: &Value) -> NodeId {
+    NodeId(v["id"].as_u64().unwrap())
+}
+
+/// The property names `component.properties` lists for `id`, in order.
+fn names(s: &mut Session, id: NodeId) -> Vec<String> {
+    ex(s, "component.properties", json!({"id": id}))["properties"]
+        .as_array()
+        .unwrap()
+        .iter()
+        .map(|p| p["name"].as_str().unwrap().to_string())
+        .collect()
+}
+
+fn rename(s: &mut Session, id: NodeId, name: &str, new_name: &str) {
+    ex(s, "component.editProperty", json!({"id": id, "name": name, "newName": new_name}));
+}
+
+/// A "Card" component with five frames made slots one by one ("Slot 1" … "Slot 5"). Returns
+/// (card, frames).
+fn card_with_slots(s: &mut Session) -> (NodeId, Vec<NodeId>) {
+    let f = idv(&ex(s, "frame.create", json!({"x": 0, "y": 0, "width": 500, "height": 100, "name": "Card"})));
+    let frames: Vec<NodeId> = (1..=5)
+        .map(|k| {
+            idv(&ex(s, "frame.create", json!({"x": (k - 1) * 100, "y": 0, "width": 90, "height": 90, "parent": f, "name": format!("Slot {k}")})))
+        })
+        .collect();
+    let card = idv(&ex(s, "component.create", json!({"ids": [f]})));
+    for fr in &frames {
+        ex(s, "component.makeSlot", json!({"id": fr}));
+    }
+    (card, frames)
+}
+
+#[test]
+fn renaming_a_slot_keeps_its_place() {
+    let mut s = Session::new();
+    let (card, _) = card_with_slots(&mut s);
+    assert_eq!(names(&mut s, card), ["Slot 1", "Slot 2", "Slot 3", "Slot 4", "Slot 5"]);
+    rename(&mut s, card, "Slot 1", "Icon 1");
+    rename(&mut s, card, "Slot 2", "Icon 2");
+    rename(&mut s, card, "Icon 1", "Icone 1");
+    assert_eq!(names(&mut s, card), ["Icone 1", "Icon 2", "Slot 3", "Slot 4", "Slot 5"], "the issue's steps");
+    // Renamed to sort first by name, the last one still stays last.
+    rename(&mut s, card, "Slot 5", "A");
+    assert_eq!(names(&mut s, card), ["Icone 1", "Icon 2", "Slot 3", "Slot 4", "A"]);
+    // Undo puts the old name back in its place; a deleted property leaves the others in order.
+    s.undo().unwrap();
+    assert_eq!(names(&mut s, card), ["Icone 1", "Icon 2", "Slot 3", "Slot 4", "Slot 5"]);
+    ex(&mut s, "component.deleteProperty", json!({"id": card, "name": "Icon 2"}));
+    assert_eq!(names(&mut s, card), ["Icone 1", "Slot 3", "Slot 4", "Slot 5"]);
+    // The order is saved with the file.
+    let saved = serde_json::to_string(s.doc().unwrap()).unwrap();
+    let back: figcraft_doc::Document = serde_json::from_str(&saved).unwrap();
+    assert_eq!(back.node(card).map(|n| &n.kind), s.doc().unwrap().node(card).map(|n| &n.kind));
+}
+
+#[test]
+fn every_kind_of_property_keeps_its_place_on_rename() {
+    let mut s = Session::new();
+    let f = idv(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 200, "height": 100, "name": "Size=Small"})));
+    let label = idv(&ex(&mut s, "text.create", json!({"text": "Go", "x": 10, "y": 10, "parent": f})));
+    let body = idv(&ex(&mut s, "frame.create", json!({"x": 100, "y": 0, "width": 90, "height": 90, "parent": f, "name": "Body"})));
+    let icon_f = idv(&ex(&mut s, "frame.create", json!({"x": 0, "y": 300, "width": 20, "height": 20, "name": "Icon"})));
+    let icon = idv(&ex(&mut s, "component.create", json!({"ids": [icon_f]})));
+    let nested = idv(&ex(&mut s, "instance.create", json!({"component": icon, "x": 60, "y": 10, "parent": f})));
+    let small = idv(&ex(&mut s, "component.create", json!({"ids": [f]})));
+    // Made in an order that isn't by name: Zoom (boolean), Label (text), Glyph (instance swap), Body (slot).
+    ex(&mut s, "component.addProperty", json!({"id": small, "name": "Zoom", "type": "boolean"}));
+    ex(&mut s, "component.addProperty", json!({"id": small, "name": "Label", "type": "text", "layer": label}));
+    ex(&mut s, "component.addProperty", json!({"id": small, "name": "Glyph", "type": "instanceSwap", "layer": nested}));
+    ex(&mut s, "component.makeSlot", json!({"id": body}));
+    assert_eq!(names(&mut s, small), ["Zoom", "Label", "Glyph", "Body"]);
+    for (old, new) in [("Zoom", "A zoom"), ("Label", "B label"), ("Glyph", "C glyph"), ("Body", "D body")] {
+        rename(&mut s, small, old, new);
+    }
+    assert_eq!(names(&mut s, small), ["A zoom", "B label", "C glyph", "D body"]);
+    rename(&mut s, small, "A zoom", "Zz");
+    assert_eq!(names(&mut s, small), ["Zz", "B label", "C glyph", "D body"]);
+    // In a set the shared properties keep their order on every variant, after the variant ones
+    // (which keep theirs when renamed too).
+    ex(&mut s, "component.addVariant", json!({"id": small, "property": "Size", "value": "Large"}));
+    let set = s.doc().unwrap().node(small).and_then(|n| n.parent).unwrap();
+    ex(&mut s, "component.addProperty", json!({"id": set, "name": "State", "type": "variant", "default": "Rest"}));
+    assert_eq!(names(&mut s, set), ["Size", "State", "Zz", "B label", "C glyph", "D body"]);
+    rename(&mut s, set, "Size", "Width");
+    rename(&mut s, set, "C glyph", "Alpha");
+    assert_eq!(names(&mut s, set), ["Width", "State", "Zz", "B label", "Alpha", "D body"]);
+    let variants: Vec<NodeId> = s.doc().unwrap().node(set).unwrap().children.clone();
+    for v in variants {
+        assert_eq!(names(&mut s, v), ["Width", "State", "Zz", "B label", "Alpha", "D body"]);
+    }
+    // An instance lists them in the same order.
+    let inst = idv(&ex(&mut s, "instance.create", json!({"component": set, "x": 0, "y": 600})));
+    assert_eq!(names(&mut s, inst), ["Width", "State", "Zz", "B label", "Alpha", "D body"]);
+}
+
+#[test]
+fn make_slot_takes_the_name_typed() {
+    let mut s = Session::new();
+    let (card, frames) = card_with_slots(&mut s);
+    let f = idv(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 40, "height": 40, "parent": card, "name": "Frame 12"})));
+    let r = ex(&mut s, "component.makeSlot", json!({"id": f, "name": "  Avatar  "}));
+    assert_eq!(r["name"], "Avatar");
+    assert_eq!(crate::components::slot_name(s.doc().unwrap(), f).as_deref(), Some("Avatar"));
+    assert_eq!(names(&mut s, card).last().map(String::as_str), Some("Avatar"), "a new slot goes last");
+    // A name taken, or one a property can't have, is refused.
+    for bad in ["Slot 1", "a=b", ""] {
+        let other = frames[4];
+        ex(&mut s, "component.removeSlot", json!({"id": other}));
+        assert!(s.execute("component.makeSlot", &json!({"id": other, "name": bad})).is_err(), "{bad:?}");
+        ex(&mut s, "component.makeSlot", json!({"id": other, "name": "Slot 5"}));
+    }
+    // Files from before the order was kept list their properties by name.
+    let doc = s.doc_mut().unwrap();
+    if let Some(NodeKind::Component { prop_order, .. }) = doc.node_mut(card).map(|n| &mut n.kind) {
+        prop_order.clear();
+    }
+    let mut by_name = names(&mut s, card);
+    let listed = by_name.clone();
+    by_name.sort();
+    assert_eq!(listed, by_name);
+}