diff --git a/crates/engine/src/cmd/component.rs b/crates/engine/src/cmd/component.rs index 06e6bf166..f68fac714 100644 --- a/crates/engine/src/cmd/component.rs +++ b/crates/engine/src/cmd/component.rs @@ -25,7 +25,7 @@ pub(crate) use instance::{swap_at, swap_instance}; use props::{add_prop, bind_prop, delete_prop, edit_prop, set_props}; pub(crate) use props::{apply_prop_bindings, check_prop_binding, describe_props}; use variants::{add_variant, combine, create_set}; -pub use variants::{can_add_variant, can_combine_variants}; +pub use variants::{can_add_variant, can_combine_variants, can_create_set}; /// `component.hideWhenPublishing`: the leading `.` of the names of this file's components and /// sets (those `ids` are or are in), put on or taken off. diff --git a/crates/engine/src/cmd/component/variants.rs b/crates/engine/src/cmd/component/variants.rs index cfba5accf..96de6ba3a 100644 --- a/crates/engine/src/cmd/component/variants.rs +++ b/crates/engine/src/cmd/component/variants.rs @@ -94,6 +94,32 @@ pub(super) fn combine(s: &mut Session, p: &Value) -> Result { Ok(json!({"id": set})) } +/// Why `ids` (distinct) can't each become a component of a new set (`component.createSet`), or +/// `None` when they can: layers inside an instance, library copies, instances, components and +/// component sets, and a layer inside another one of `ids`. +fn create_set_refusal(doc: &Document, ids: &[NodeId]) -> Option<&'static str> { + if ids.iter().any(|i| inside_instance(doc, *i)) { + Some("layers inside an instance can't become components") + } else if ids.iter().any(|i| doc.is_remote(*i)) { + Some(crate::library::READ_ONLY) + } else if ids.iter().any(|i| { + doc.node(*i).is_some_and(|n| matches!(n.kind, NodeKind::Instance { .. } | NodeKind::Component { .. } | NodeKind::ComponentSet { .. })) + }) { + Some("select layers that aren't components, component sets or instances") + } else if roots(doc, ids).len() != ids.len() { + Some("a layer can't be in a set with one inside it") + } else { + None + } +} + +/// Whether "Create component set" is on offer for `ids`: at least one layer that +/// [`create_set_refusal`] lets through. +pub fn can_create_set(doc: &Document, ids: &[NodeId]) -> bool { + let distinct: Vec = ids.iter().copied().collect::>().into_iter().collect(); + !distinct.is_empty() && distinct.iter().all(|i| doc.contains(*i)) && create_set_refusal(doc, &distinct).is_none() +} + /// `component.createSet`: each target becomes a component (as `component.createMultiple` makes /// them) and all are wrapped in place in a component set, 20 px around, named as Figma names them: /// variants "Property 1=" in left-to-right order (a name holding `=` is kept); layers @@ -104,19 +130,8 @@ pub(super) fn create_set(s: &mut Session, p: &Value) -> Result { let mut seen = BTreeSet::new(); let ids: Vec = req_targets(cmd, s, p)?.into_iter().filter(|i| seen.insert(*i)).collect(); let doc = s.doc()?; - if ids.iter().any(|i| inside_instance(doc, *i)) { - return Err(EngineError::Disabled(cmd.into(), "layers inside an instance can't become components".into())); - } - if ids.iter().any(|i| doc.is_remote(*i)) { - return Err(EngineError::Disabled(cmd.into(), crate::library::READ_ONLY.into())); - } - if ids.iter().any(|i| { - doc.node(*i).is_some_and(|n| matches!(n.kind, NodeKind::Instance { .. } | NodeKind::Component { .. } | NodeKind::ComponentSet { .. })) - }) { - return Err(EngineError::Disabled(cmd.into(), "select layers that aren't components, component sets or instances".into())); - } - if roots(doc, &ids).len() != ids.len() { - return Err(EngineError::Disabled(cmd.into(), "a layer can't be in a set with one inside it".into())); + if let Some(why) = create_set_refusal(doc, &ids) { + return Err(EngineError::Disabled(cmd.into(), why.into())); } let mut made = Vec::new(); for i in roots(doc, &ids) { diff --git a/crates/engine/src/components_behaviour_tests.rs b/crates/engine/src/components_behaviour_tests.rs index 510fb2e74..3565a3165 100644 --- a/crates/engine/src/components_behaviour_tests.rs +++ b/crates/engine/src/components_behaviour_tests.rs @@ -1153,3 +1153,93 @@ fn create_set_refuses_a_target_inside_another_target_and_leaves_the_document_alo assert!(matches!(doc.node(frames[0]).unwrap().kind, NodeKind::Frame)); assert!(matches!(doc.node(frames[1]).unwrap().kind, NodeKind::Frame)); } + +/// Every node's id, parent, name and kind, to compare a document before and after a refused command. +fn snapshot(s: &Session) -> Vec { + s.doc().unwrap().nodes().map(|n| format!("{:?}|{:?}|{}|{:?}", n.id, n.parent, n.name, n.kind)).collect() +} + +/// A refused `component.createSet`: `Disabled`, no undo step, the document and the selection as they were. +fn assert_create_set_refused(s: &mut Session, ids: &[NodeId]) { + let (before, undo, selection) = (snapshot(s), undo_len(s), s.selection()); + let e = s.execute("component.createSet", &json!({"ids": ids})).unwrap_err(); + assert!(matches!(e, crate::EngineError::Disabled(..)), "{e:?}"); + assert_eq!(undo_len(s), undo, "no undo step"); + assert_eq!(snapshot(s), before, "the document is unchanged"); + assert_eq!(s.selection(), selection, "the selection is unchanged"); +} + +/// Nothing to wrap is refused before anything is created. +#[test] +fn create_set_refuses_no_layers_and_leaves_the_document_alone() { + let mut s = Session::new(); + named_frames(&mut s, &["A"], false); + s.execute("select.set", &json!({"ids": []})).unwrap(); + assert_create_set_refused(&mut s, &[]); +} + +/// One layer is allowed: a set of one variant, as many layers would make. +#[test] +fn create_set_with_one_layer_makes_a_one_variant_set() { + let mut s = Session::new(); + let frames = named_frames(&mut s, &["Solo"], false); + let before = undo_len(&s); + let out = s.execute("component.createSet", &json!({"ids": frames})).unwrap(); + assert_eq!(undo_len(&s), before + 1, "one undo step"); + let set = id_of(&out["id"]).unwrap(); + let doc = s.doc().unwrap(); + assert_eq!(doc.node(set).unwrap().children.len(), 1); + assert_eq!(names_by_x(&s, set), ["Property 1=Default"], "a lone layer shares its name with itself"); + assert!(matches!(doc.node(doc.node(set).unwrap().children[0]).unwrap().kind, NodeKind::Component { .. })); + s.undo().unwrap(); + assert!(matches!(s.doc().unwrap().node(frames[0]).unwrap().kind, NodeKind::Frame)); +} + +/// Each refusal branch of `create_set`, with `can_create_set` agreeing with it. +#[test] +fn create_set_refuses_every_ineligible_selection_and_leaves_the_document_alone() { + use crate::cmd::component::can_create_set; + let mut s = Session::new(); + let frames = named_frames(&mut s, &["A", "B", "C"], false); + let comp = id_of(&s.execute("component.create", &json!({"ids": [frames[0]]})).unwrap()["id"]).unwrap(); + let inst = id_of(&s.execute("instance.create", &json!({"component": comp})).unwrap()["id"]).unwrap(); + let set_frames = named_frames(&mut s, &["V1", "V2"], false); + let set = id_of(&s.execute("component.createSet", &json!({"ids": set_frames})).unwrap()["id"]).unwrap(); + // A layer inside the instance (its copy of the card's content). + let in_inst = s.doc().unwrap().node(inst).unwrap().children.first().copied(); + // A library copy. + let remote = { + let doc = s.doc_mut().unwrap(); + let store = doc.ensure_library_store(); + let remote = doc.alloc_id(); + doc.insert(Node::new(remote, NodeKind::Frame, "Remote", 10.0, 10.0), store, None).unwrap(); + remote + }; + let ok = frames[1]; + assert!(can_create_set(s.doc().unwrap(), &[ok, frames[2]])); + assert!(!can_create_set(s.doc().unwrap(), &[])); + let mut cases = vec![("instance", inst), ("library copy", remote), ("component", comp), ("component set", set)]; + if let Some(i) = in_inst { + cases.push(("inside an instance", i)); + } + for (what, bad) in cases { + assert!(!can_create_set(s.doc().unwrap(), &[ok, bad]), "{what}: not offered"); + assert_create_set_refused(&mut s, &[ok, bad]); + assert_create_set_refused(&mut s, &[bad]); + } +} + +/// A command that fails leaves no trace: the checks before the creation loop catch every +/// refusal the loop's own commands could hit, and `Session::execute` restores the document, the +/// selection and the undo stack on any error, so no half-made components can survive. +#[test] +fn a_failing_create_set_leaves_no_components_behind() { + let mut s = Session::new(); + let frames = named_frames(&mut s, &["A", "B"], false); + let (before, undo, selection) = (snapshot(&s), undo_len(&s), s.selection()); + let e = s.execute("component.createSet", &json!({"ids": [frames[0], NodeId(u64::MAX)]})); + assert!(e.is_err()); + assert_eq!(undo_len(&s), undo); + assert_eq!(snapshot(&s), before); + assert_eq!(s.selection(), selection); +} diff --git a/crates/ui-egui/src/create_multiple_menu_tests.rs b/crates/ui-egui/src/create_multiple_menu_tests.rs index 409b67e54..60d22a993 100644 --- a/crates/ui-egui/src/create_multiple_menu_tests.rs +++ b/crates/ui-egui/src/create_multiple_menu_tests.rs @@ -134,3 +134,32 @@ fn the_third_entry_creates_one_component_set() { let doc = u.app.session.doc().unwrap(); assert!(ids.iter().all(|i| matches!(doc.node(*i).map(|n| &n.kind), Some(NodeKind::Frame))), "one undo reverts it all"); } + +/// The component button's width in the Design panel: wider with a dropdown chevron beside the icon. +fn button_width(u: &mut Ui) -> f32 { + let all = nodes(u); + let button = all.iter().find(|n| n.label().is_some_and(|l| l.starts_with("Create component"))).expect("the Design panel's component button"); + bounds(button).width() +} + +#[test] +fn the_dropdown_chevron_is_there_only_for_several_layers() { + let (mut one, _) = frames(1); + let (mut two, _) = frames(2); + let (single, several) = (button_width(&mut one), button_width(&mut two)); + assert_eq!(single, 24.0, "one layer: the plain icon button"); + assert_eq!(several, 34.0, "several layers: room for the chevron"); +} + +#[test] +fn create_component_set_is_offered_only_where_it_works() { + let (mut u, ids) = frames(3); + // Make the first a component: the set can't take it, so the entry is left out. + u.app.session.execute("component.create", &json!({"ids": [ids[0]]})).unwrap(); + u.app.session.execute("select.set", &json!({"ids": ids})).unwrap(); + u.frame(vec![]); + click_button(&mut u); + let all = nodes(&mut u); + assert!(all.iter().any(|n| n.label() == Some("Create multiple components")), "the menu is open"); + assert!(all.iter().all(|n| n.label() != Some("Create component set")), "left out for a selection with a component"); +} diff --git a/crates/ui-egui/src/panels/right.rs b/crates/ui-egui/src/panels/right.rs index 55b36ebf5..4d7ce1b19 100644 --- a/crates/ui-egui/src/panels/right.rs +++ b/crates/ui-egui/src/panels/right.rs @@ -494,7 +494,10 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) { // Layers in boolean groups, or one boolean group: the operation they use (checked in the menu). let current = figcraft_engine::booleans::shown_op(doc, sel); for (icon, label, cmd) in actions.into_iter().rev() { - let br = Rect::from_min_size(pos2(x - 24.0, r.center().y - 12.0), vec2(24.0, 24.0)); + // With several layers the component button is a dropdown: a small chevron sits beside its icon. + let dropdown = cmd == "component.create" && sel.len() > 1; + let bw = if dropdown { 34.0 } else { 24.0 }; + let br = Rect::from_min_size(pos2(x - bw, r.center().y - 12.0), vec2(bw, 24.0)); let tip = if cmd == "bool.union" { label } else { crate::shortcuts::tip(label, cmd) }; let bresp = ui.interact(br, ui.scope_id().with(("qa", cmd)), Sense::click()).tip(tip); if bresp.hovered() { @@ -505,7 +508,11 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) { ("bool.union", Some(op)) => crate::layers::op_icon(op), _ => icon, }; - icons::paint(ui.painter(), br.shrink(5.0), icon, t.icon); + let icon_rect = Rect::from_min_size(br.min, vec2(24.0, 24.0)).shrink(5.0); + icons::paint(ui.painter(), icon_rect, icon, t.icon); + if dropdown { + icons::paint(ui.painter(), Rect::from_center_size(pos2(br.max.x - 6.0, br.center().y), vec2(8.0, 8.0)), Icon::Chevron, t.icon_secondary); + } if cmd == "bool.union" { menu_popup(&bresp).width(180.0).show(|ui| { for (op, c) in [ @@ -532,14 +539,24 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) { }); } else if cmd == "component.create" && sel.len() > 1 { // Several layers: the button opens a menu, as Figma's does (#2088). One layer keeps the plain click. - menu_popup(&bresp).width(220.0).show(|ui| { - let multiple = crate::i18n::command("component.createMultiple", "Create multiple components"); - let set = crate::i18n::command("component.createSet", "Create component set"); - for (c, label) in - [("component.create", tr("Create component")), ("component.createMultiple", &multiple), ("component.createSet", &set)] - { + let create = crate::i18n::command("component.create", "Create component"); + let multiple = crate::i18n::command("component.createMultiple", "Create multiple components"); + let set = crate::i18n::command("component.createSet", "Create component set"); + // Like the layers menu, leave out what the selection can't use. + let can_set = figcraft_engine::cmd::component::can_create_set(doc, sel); + let mut entries = vec![("component.create", &create), ("component.createMultiple", &multiple)]; + if can_set { + entries.push(("component.createSet", &set)); + } + // Wide enough for the longest translated label (the rows grow with it, the menu never clips). + let widest = entries + .iter() + .map(|(_, l)| ui.painter().layout_no_wrap(l.to_string(), regular(11.5), Color32::PLACEHOLDER).size().x) + .fold(0.0f32, f32::max); + menu_popup(&bresp).width(menu::menu_row_width(widest, 0.0).ceil().max(160.0)).show(|ui| { + for (c, label) in &entries { let key = crate::shortcuts::shortcut(c).unwrap_or(""); - if menu_icon_item(ui, None, None, label, key) { + if menu_item(ui, label, key) { app.run(c, json!({})); ui.close(); } @@ -548,7 +565,7 @@ fn header(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId]) { } else if bresp.clicked() { app.run(cmd, json!({})); } - x -= 26.0; + x -= bw + 2.0; } crate::component_panel::instance_section(app, ui, doc, sel); }