Merge pull request #1075 from vangel76/fix-1061-rename

crates/engine/src/commands/prop_groups.rs

@@ -2,7 +2,7 @@
 //! effect, mask, shape item or text animator (what scripting's `PropertyGroup.name`, `enabled`,
 //! `remove()`, `duplicate()` and `moveTo()` do, and what agents need without a UI selection).
 
-use effectcraft_project::{GroupKind, ItemId, LayerId, LayerSource, Node, PropGroup, Uid};
+use effectcraft_project::{ItemId, LayerId, LayerSource, Node, PropGroup, Uid};
 use serde_json::{Value, json};
 
 use super::{CommandSpec, b_p, bad, has_layers, layer_mut, layer_p, layers_p, str_p};
@@ -10,7 +10,7 @@ use crate::{EngineError, Result, Session, cmd};
 
 /// Groups that can be renamed, removed, duplicated and reordered: instances in indexed lists.
 fn is_instance(g: &PropGroup) -> bool {
-    matches!(g.kind, GroupKind::Indexed | GroupKind::Effect { .. } | GroupKind::Mask { .. } | GroupKind::Tracker { .. })
+    g.is_instance()
 }
 
 /// `{layer, prop: uid}` → (comp, layer, group uid), checking that the group is an instance.

crates/engine/src/tests_project_items.rs

@@ -167,3 +167,26 @@ fn delete_removes_selected_groups_not_their_layer() {
     delete(&mut s);
     assert!(s.active_comp().unwrap().layer(LayerId(t)).is_none(), "a fixed group selected: the layer goes");
 }
+
+/// #1061: text animators, masks and shape groups rename like effects, with undo; fixed groups and
+/// empty names are refused.
+#[test]
+fn text_animators_masks_and_shape_groups_rename() {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "C", "width": 64, "height": 64, "duration": 1})).unwrap();
+    let t = s.execute("layer.newText", json!({"text": "ABCD"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.addTextAnimator", json!({"layer": t, "properties": ["tracking"]})).unwrap();
+    let sh = s.execute("layer.newShape", json!({"kind": "star"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.addMask", json!({"layer": sh, "shape": "ellipse"})).unwrap();
+    let uid = |s: &Session, l: u64, path: &str| s.active_comp().unwrap().layer(LayerId(l)).unwrap().props.group(path).unwrap().groups().next().unwrap().uid;
+    let name = |s: &Session, l: u64, u: u64| s.active_comp().unwrap().layer(LayerId(l)).unwrap().props.find_group(u).unwrap().name.clone();
+    for (l, path, new) in [(t, "text/animators", "Wiggle In"), (sh, "masks", "Reveal"), (sh, "contents", "Star Body")] {
+        let u = uid(&s, l, path);
+        let old = name(&s, l, u);
+        s.execute("prop.renameGroup", json!({"layer": l, "prop": u, "name": new})).unwrap();
+        assert_eq!(name(&s, l, u), new, "{path}");
+        s.execute("edit.undo", json!({})).unwrap();
+        assert_eq!(name(&s, l, u), old, "{path} undone");
+        assert!(s.execute("prop.renameGroup", json!({"layer": l, "prop": u, "name": "  "})).is_err());
+    }
+}

crates/project/src/props.rs

@@ -337,6 +337,12 @@ pub fn parse_path(s: &str) -> Vec<Seg> {
 }
 
 impl PropGroup {
+    /// An instance in an indexed list (effect, mask, shape item, text animator, tracker): these
+    /// can be renamed, removed, duplicated and reordered; fixed groups (Transform…) can't.
+    pub fn is_instance(&self) -> bool {
+        matches!(self.kind, GroupKind::Indexed | GroupKind::Effect { .. } | GroupKind::Mask { .. } | GroupKind::Tracker { .. })
+    }
+
     pub fn new(uid: Uid, match_id: &str, name: &str) -> PropGroup {
         PropGroup { uid, match_id: match_id.into(), name: name.into(), kind: GroupKind::Plain, enabled: true, children: vec![] }
     }

crates/ui-egui/src/panels/effect_controls.rs

@@ -1070,6 +1070,14 @@ fn histogram_only(app: &mut EffectcraftApp, ui: &mut egui::Ui, p: &egui::Painter
 
 // ---------------------------------------------------------------------------------------------
 
+/// Open the rename field on effect `uid` (Enter with the effect selected), name selected.
+pub fn start_rename(ctx: &egui::Context, uid: u64, name: &str) {
+    let id = egui::Id::new(("ec-rename", uid));
+    ctx.data_mut(|d| d.insert_temp(id, name.to_string()));
+    ctx.data_mut(|d| d.insert_temp(id.with("select"), true));
+    ctx.memory_mut(|m| m.request_focus(id.with("field")));
+}
+
 pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) {
     let t = app.tokens;
     let p = ui.painter().with_clip_rect(rect);
@@ -1147,6 +1155,9 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) {
                 response.request_focus();
             }
             app.auto.add(&format!("effectControls.effect.{}.nameEdit", g.uid), name_rect, "Effect name");
+            if ctx.data_mut(|d| d.remove_temp::<bool>(rename_id.with("select"))).unwrap_or(false) {
+                widgets::select_all(&ctx, response.id, &value);
+            }
             // Escape cancels. egui drops the field's focus on Escape in the same frame, so the
             // key is checked on the focus being lost as well as held.
             let escape = (response.has_focus() || response.lost_focus()) && ui.input(|i| i.key_pressed(egui::Key::Escape));

crates/ui-egui/src/panels/timeline.rs

@@ -596,6 +596,41 @@ pub fn locate(app: &EffectcraftApp, layer: u64, time: Option<f64>) -> Option<(f3
 
 /// Start renaming the first selected layer (Enter in the Timeline).
 pub fn begin_rename(app: &mut EffectcraftApp, ctx: &egui::Context) {
+    // A selected effect, mask, shape group or text animator renames before its layer (as in
+    // After Effects, #1061).
+    let group = app.session.state.selected_props.iter().rev().find_map(|(lid, uid)| {
+        let g = app.session.active_comp()?.layer(*lid)?.props.find_group(*uid).filter(|g| g.is_instance())?;
+        Some((lid.0, g.uid, g.name.clone(), matches!(g.kind, GroupKind::Effect { .. })))
+    });
+    if let Some((layer, uid, name, is_fx)) = group {
+        // An effect in a visible Effect Controls panel renames there, the rest in the Timeline.
+        if is_fx && app.auto.previous.iter().chain(app.auto.elements.iter()).any(|e| e.id == format!("effectControls.effect.{uid}.name")) {
+            super::effect_controls::start_rename(ctx, uid, &name);
+        } else {
+            // Show the group's row first: the field lives in it.
+            fn ancestors(g: &PropGroup, uid: u64, out: &mut Vec<u64>) -> bool {
+                for sg in g.groups() {
+                    if sg.uid == uid || ancestors(sg, uid, out) {
+                        out.push(sg.uid);
+                        return true;
+                    }
+                }
+                false
+            }
+            let mut open = vec![];
+            if let Some(l) = app.session.active_comp().and_then(|c| c.layer(LayerId(layer))) {
+                ancestors(&l.props, uid, &mut open);
+            }
+            open.retain(|u| *u != uid);
+            app.ui.timeline.open_layers.insert(layer);
+            app.ui.timeline.open_groups.extend(open);
+            ctx.data_mut(|d| {
+                d.insert_temp(group_rename_id(), (layer, uid, name));
+                d.insert_temp(rename_focus_id(), true);
+            });
+        }
+        return;
+    }
     if let Some(l) = app.session.state.selected_layers.first().and_then(|id| app.session.active_comp().and_then(|c| c.layer(*id))) {
         start_rename(ctx, l.id.0, &l.name);
     }
@@ -621,6 +656,11 @@ fn start_rename(ctx: &egui::Context, id: u64, name: &str) {
     });
 }
 
+/// The Timeline's open group rename field: (layer, group uid, text).
+fn group_rename_id() -> egui::Id {
+    egui::Id::new("tl-group-rename")
+}
+
 fn rename_focus_id() -> egui::Id {
     egui::Id::new("tl-rename-focus")
 }
@@ -2304,7 +2344,27 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) {
                     }
                     app.auto.add(&format!("timeline.group.{uid}.eye"), er, name);
                 }
-                lp.text(pos2(indent + 10.0, cy), Align2::LEFT_CENTER, name, Tokens::ui(12.0), t.text);
+                let group_edit: Option<(u64, u64, String)> = ctx.data(|d| d.get_temp(group_rename_id()));
+                if let Some((_, _, mut buf)) = group_edit.filter(|(rl, ru, _)| *rl == layer.id.0 && ru == uid) {
+                    let field = Rect::from_min_max(pos2(indent + 8.0, r.min.y + 2.0), pos2((cw.switches - 4.0).max(indent + 60.0), r.max.y - 2.0));
+                    let mut child = ui.new_child(egui::UiBuilder::new().max_rect(field));
+                    let er = child.add(egui::TextEdit::singleline(&mut buf).font(Tokens::ui(12.0)).desired_width(field.width()));
+                    app.auto.add(&format!("timeline.group.{uid}.nameEdit"), er.rect, &buf);
+                    if ctx.data_mut(|d| d.remove_temp::<bool>(rename_focus_id())).unwrap_or(false) {
+                        er.request_focus();
+                        widgets::select_all(&ctx, er.id, &buf);
+                    }
+                    if er.lost_focus() {
+                        if !ui.input(|i| i.key_pressed(egui::Key::Escape)) && !buf.trim().is_empty() && buf != *name {
+                            actions.push(("prop.renameGroup".into(), json!({"layer": layer.id.0, "prop": uid, "name": buf})));
+                        }
+                        ctx.data_mut(|d| d.remove::<(u64, u64, String)>(group_rename_id()));
+                    } else {
+                        ctx.data_mut(|d| d.insert_temp(group_rename_id(), (layer.id.0, *uid, buf)));
+                    }
+                } else {
+                    lp.text(pos2(indent + 10.0, cy), Align2::LEFT_CENTER, name, Tokens::ui(12.0), t.text);
+                }
                 text_anim_popups(app, ui, &lp, layer, *uid, cw.switches, cy, &mut actions);
                 shape_add_popup(app, ui, &lp, layer, *uid, cw.switches, cy, &mut actions);
                 if *uid == GEOM_BIT | layer.id.0 {

crates/ui-egui/tests/ui_group_rename.rs

@@ -0,0 +1,106 @@
+//! Enter renames the selected effect, text animator, mask or shape group, not only the layer
+//! (#1061). Enter commits, Escape cancels.
+
+use effectcraft_engine::Session;
+use effectcraft_ui_egui::EffectcraftApp;
+use egui::{Event, Key, Modifiers};
+use egui_kittest::Harness;
+use serde_json::json;
+
+struct Fx {
+    text: u64,
+    shape: u64,
+    effect: u64,
+    anim: u64,
+    mask: u64,
+    group: u64,
+}
+
+fn harness() -> (Harness<'static, EffectcraftApp>, Fx) {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "Main", "width": 320, "height": 180, "duration": 4, "fps": 24})).unwrap();
+    let text = s.execute("layer.newText", json!({"text": "ABCD"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.addTextAnimator", json!({"layer": text, "properties": ["tracking"]})).unwrap();
+    let effect = s.execute("effect.apply", json!({"layer": text, "effect": "Gaussian Blur"})).unwrap()["effects"][0].as_u64().unwrap();
+    let shape = s.execute("layer.newShape", json!({"kind": "star"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.addMask", json!({"layer": shape, "shape": "ellipse"})).unwrap();
+    let first = |s: &Session, l: u64, path: &str| {
+        s.active_comp().unwrap().layer(effectcraft_engine::project::LayerId(l)).unwrap().props.group(path).unwrap().groups().next().unwrap().uid
+    };
+    let (anim, mask, group) = (first(&s, text, "text/animators"), first(&s, shape, "masks"), first(&s, shape, "contents"));
+    let mut h = Harness::builder().with_size(egui::vec2(1700.0, 1100.0)).with_step_dt(1.0 / 60.0).build_eframe(|_| EffectcraftApp::new(s));
+    h.run_steps(6);
+    (h, Fx { text, shape, effect, anim, mask, group })
+}
+
+fn key(h: &mut Harness<'_, EffectcraftApp>, key: Key) {
+    h.input_mut().events.push(Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE });
+    h.input_mut().events.push(Event::Key { key, physical_key: None, pressed: false, repeat: false, modifiers: Modifiers::NONE });
+    h.run_steps(3);
+}
+
+fn name(h: &Harness<'_, EffectcraftApp>, layer: u64, uid: u64) -> String {
+    h.state().session.active_comp().unwrap().layer(effectcraft_engine::project::LayerId(layer)).unwrap().props.find_group(uid).unwrap().name.clone()
+}
+
+fn layer_name(h: &Harness<'_, EffectcraftApp>, layer: u64) -> String {
+    h.state().session.active_comp().unwrap().layer(effectcraft_engine::project::LayerId(layer)).unwrap().name.clone()
+}
+
+fn select(h: &mut Harness<'_, EffectcraftApp>, layer: u64, uid: u64) {
+    let s = &mut h.state_mut().session;
+    s.execute("layer.select", json!({"layers": [layer]})).unwrap();
+    s.execute("prop.select", json!({"layer": layer, "prop": uid, "selectKeys": false})).unwrap();
+    h.run_steps(3);
+}
+
+fn editing(h: &Harness<'_, EffectcraftApp>, uid: u64) -> bool {
+    let ids = [format!("timeline.group.{uid}.nameEdit"), format!("effectControls.effect.{uid}.nameEdit")];
+    h.state().auto.previous.iter().chain(h.state().auto.elements.iter()).any(|e| ids.contains(&e.id))
+}
+
+fn rename_with_enter(h: &mut Harness<'_, EffectcraftApp>, layer: u64, uid: u64, new: &str) {
+    select(h, layer, uid);
+    key(h, Key::Enter);
+    assert!(editing(h, uid), "Enter opens a rename field on group {uid}");
+    h.key_press_modifiers(Modifiers::COMMAND, Key::A);
+    h.event(Event::Text(new.into()));
+    h.step();
+    key(h, Key::Enter);
+    assert!(!editing(h, uid), "Enter closes the field");
+}
+
+#[test]
+fn enter_renames_the_selected_group_not_the_layer() {
+    let (mut h, x) = harness();
+    let before = (layer_name(&h, x.text), layer_name(&h, x.shape));
+    for (layer, uid, new) in [(x.text, x.effect, "Delay"), (x.text, x.anim, "Wiggle"), (x.shape, x.mask, "Reveal"), (x.shape, x.group, "Body")] {
+        rename_with_enter(&mut h, layer, uid, new);
+        assert_eq!(name(&h, layer, uid), new);
+    }
+    assert_eq!((layer_name(&h, x.text), layer_name(&h, x.shape)), before, "the layers keep their names");
+}
+
+#[test]
+fn escape_cancels_a_group_rename_and_a_layer_still_renames_without_a_group() {
+    let (mut h, x) = harness();
+    let old = name(&h, x.text, x.anim);
+    select(&mut h, x.text, x.anim);
+    key(&mut h, Key::Enter);
+    assert!(editing(&h, x.anim));
+    h.key_press_modifiers(Modifiers::COMMAND, Key::A);
+    h.event(Event::Text("Nope".into()));
+    h.step();
+    key(&mut h, Key::Escape);
+    assert!(!editing(&h, x.anim));
+    assert_eq!(name(&h, x.text, x.anim), old);
+    // No group selected: Enter renames the layer as before.
+    h.state_mut().session.execute("layer.select", json!({"layers": [x.shape]})).unwrap();
+    h.run_steps(3);
+    key(&mut h, Key::Enter);
+    h.key_press_modifiers(Modifiers::COMMAND, Key::A);
+    h.event(Event::Text("Plate".into()));
+    h.step();
+    key(&mut h, Key::Enter);
+    assert_eq!(layer_name(&h, x.shape), "Plate");
+}