Merge pull request #3214 from vangel76/effect-rename-select-3198

Effect Controls: renaming an effect opens with the whole name selected

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

@@ -38,12 +38,14 @@ pub(crate) fn handle_disclosure_key(app: &mut EffectcraftApp, key: egui::Key) ->
 
 // ---------------------------------------------------------------------------------------------
 
-/// Open the rename field on effect `uid` (Enter with the effect selected), name selected.
+/// Open the rename field on effect `uid` (Enter with the effect selected). It takes the keyboard,
+/// its whole name selected, the pass it is first drawn.
 pub fn start_rename(ctx: &egui::Context, uid: u64, name: &str) {
     let id = egui::Id::unique(("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")));
+    ctx.data_mut(|d| {
+        d.insert_temp(id, name.to_string());
+        d.insert_temp(id.with("start"), true);
+    });
 }
 
 /// Where an effect header's parts sit, for its rename field.
@@ -62,16 +64,22 @@ fn rename_field(app: &mut EffectcraftApp, ui: &mut egui::Ui, h: &EffectHdr, mut
     let EffectHdr { g, shown, r, tw, reset, rename_id, layer_id } = *h;
     let ctx = ui.ctx().clone();
     let name_rect = Rect::from_min_max(pos2(tw.max.x + 6.0, r.min.y + 2.0), pos2(reset.min.x - 6.0, r.max.y - 2.0));
-    let response = ui.put(name_rect, egui::TextEdit::singleline(&mut value).id(rename_id.with("field")).font(Tokens::semibold(12.0)));
-    // Focus the field the frame it first exists: focusing an id not yet on screen leaves
-    // the accessibility tree with a focus it doesn't contain.
-    if ctx.data_mut(|d| d.remove_temp::<bool>(rename_id.with("start"))).is_some() {
-        crate::widgets::select_all(&ctx, response.id, &value);
-        response.request_focus();
+    // The field opens with the name the header draws (in the interface language), however it
+    // was opened, so committing it untouched is not a rename.
+    let opening = ctx.data_mut(|d| d.remove_temp::<bool>(rename_id.with("start"))).is_some();
+    if opening {
+        value = shown.to_string();
     }
+    let response = ui.put(name_rect, egui::TextEdit::singleline(&mut value).id(rename_id.with("field")).font(Tokens::semibold(12.0)));
     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) {
+    // Focus the field the frame it first exists, its whole name selected so typing replaces it:
+    // focusing an id not yet on screen leaves the accessibility tree with a focus it doesn't
+    // contain. The Enter that opened it (it is drawn in that pass) doesn't also commit it.
+    if opening {
         widgets::select_all(&ctx, response.id, &value);
+        response.request_focus();
+        ctx.data_mut(|d| d.insert_temp(rename_id, value));
+        return;
     }
     // 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.

crates/ui-egui/tests/it/ui_effect_controls.rs

@@ -1040,6 +1040,59 @@ fn effect_control_name_can_be_renamed_from_its_header() {
     assert_eq!(current_name(&h), "Point Control");
 }
 
+/// #3198: the effect rename field opens with the whole name selected, so typing replaces it (no
+/// Ctrl/Cmd+A first), whether Enter or a double-click on the header opens it, as in After
+/// Effects. Enter used to leave the field open without the keyboard, the caret after the name.
+#[test]
+fn effect_rename_opens_with_the_name_selected() {
+    let (app, x) = app();
+    let mut h = Harness::builder().with_size(egui::vec2(1700.0, 1100.0)).with_step_dt(1.0 / 60.0).build_eframe(|_| app);
+    settle(&mut h);
+    let edit = format!("effectControls.effect.{}.nameEdit", x.point_fx);
+    let current_name =
+        |h: &Harness<'_, EffectcraftApp>| layer(&h.state().session, x.small).effects().unwrap().groups().find(|g| g.uid == x.point_fx).unwrap().name.clone();
+    let type_and_commit = |h: &mut Harness<'_, EffectcraftApp>, text: &str| {
+        assert!(ids(h).contains(&edit), "the rename field is open");
+        assert!(h.ctx.egui_wants_keyboard_input(), "the rename field has the keyboard");
+        h.event(egui::Event::Text(text.into()));
+        h.step();
+        h.key_press(egui::Key::Enter);
+        h.run_steps(3);
+        assert!(!ids(h).contains(&edit), "Enter closes the rename field");
+    };
+    // Enter with the effect selected.
+    h.state_mut().session.execute("prop.select", json!({"layer": x.small, "prop": x.point_fx, "selectKeys": false})).unwrap();
+    h.run_steps(3);
+    h.key_press(egui::Key::Enter);
+    h.run_steps(3);
+    type_and_commit(&mut h, "Speed");
+    assert_eq!(current_name(&h), "Speed", "the typed text replaced the whole name");
+    // A double-click on the effect's name.
+    let point = rect_of(&h, &format!("effectControls.effect.{}.name", x.point_fx)).center();
+    double_click(&mut h, point);
+    h.run_steps(3);
+    type_and_commit(&mut h, "Drift");
+    assert_eq!(current_name(&h), "Drift", "the typed text replaced the whole name");
+    // The header's context menu ▸ Rename.
+    let header = rect_of(&h, &format!("effectControls.effect.{}", x.point_fx)).center();
+    right_click(&mut h, header);
+    let rename = h.query_by_label("Rename").expect("the effect menu's Rename").rect().center();
+    click(&mut h, rename);
+    h.run_steps(3);
+    type_and_commit(&mut h, "Wander");
+    assert_eq!(current_name(&h), "Wander", "the typed text replaced the whole name");
+    // Enter and Enter again without typing: no rename, no undo step.
+    let undo = h.state().session.history.undo.len();
+    h.key_press(egui::Key::Enter);
+    h.run_steps(3);
+    assert!(ids(&h).contains(&edit) && h.ctx.egui_wants_keyboard_input(), "Enter opens the rename field with the keyboard");
+    h.key_press(egui::Key::Enter);
+    h.run_steps(3);
+    assert!(!ids(&h).contains(&edit), "Enter closes the rename field");
+    assert_eq!(current_name(&h), "Wander", "the name stays");
+    assert_eq!(h.state().session.history.undo.len(), undo, "no undo step");
+}
+
 // Regression test for #1497: double-clicking an effect parameter's name in Effect Controls did
 // nothing; it should reveal that property in the Timeline, as After Effects does.
 #[test]

crates/ui-egui/tests/it/ui_effect_names.rs

@@ -9,7 +9,7 @@ use effectcraft_ui_egui::dock::PanelKind;
 use effectcraft_ui_egui::native_menu::{NativeNode, build as native_menu};
 use egui::vec2;
 use egui_kittest::Harness;
-use egui_kittest::kittest::Queryable;
+use egui_kittest::kittest::{NodeT, Queryable};
 use serde_json::json;
 
 /// The effect under test, and the names the registry and the catalog give it.
@@ -122,6 +122,37 @@ fn the_effect_context_menu_follows_the_interface_language() {
     }
 }
 
+/// #3198: Enter opens an effect's rename field on its translated name, as a double-click does,
+/// so Enter again without typing renames nothing: the effect keeps its English group name (and
+/// so its translation) and there is no undo step.
+#[test]
+fn renaming_a_translated_effect_without_typing_keeps_its_name() {
+    let (mut h, fx) = harness("zh-hans");
+    let name = |h: &Harness<'_, EffectcraftApp>| {
+        let s = &h.state().session;
+        let layer = s.state.selected_layers.first().copied().expect("the selected layer");
+        s.active_comp().and_then(|c| c.layer(layer)).and_then(|l| l.props.find_group(fx)).map(|g| g.name.clone())
+    };
+    let edit = format!("effectControls.effect.{fx}.nameEdit");
+    let editing = |h: &Harness<'_, EffectcraftApp>| h.state().auto.elements.iter().any(|e| e.id == edit);
+    let layer = h.state().session.state.selected_layers.first().map(|l| l.0).expect("the selected layer");
+    h.state_mut().session.execute("prop.select", json!({"layer": layer, "prop": fx, "selectKeys": false})).unwrap();
+    h.run_steps(3);
+    let undo = h.state().session.history.undo.len();
+    h.key_press(egui::Key::Enter);
+    h.run_steps(3);
+    assert!(editing(&h), "Enter opens the rename field");
+    assert!(h.ctx.egui_wants_keyboard_input(), "the rename field has the keyboard");
+    let field = h.query_by(|n| n.is_focused()).and_then(|n| n.accesskit_node().value());
+    assert_eq!(field.as_deref(), Some(BLUR_ZH), "the field opens on the drawn (Chinese) name");
+    h.key_press(egui::Key::Enter);
+    h.run_steps(3);
+    assert!(!editing(&h), "Enter closes the rename field");
+    assert_eq!(name(&h).as_deref(), Some(BLUR_EN), "the effect keeps its name");
+    assert_eq!(h.state().session.history.undo.len(), undo, "no undo step");
+    assert!(drawn(&h, BLUR_ZH) > 0, "the header still draws the Chinese name");
+}
+
 /// Everything an agent or a script addresses stays English: the automation label, the property
 /// tree's group name and `effect.list`.
 #[test]

crates/ui-egui/tests/it/ui_group_rename.rs

@@ -63,7 +63,8 @@ fn rename_with_enter(h: &mut Harness<'_, EffectcraftApp>, layer: u64, uid: u64,
     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);
+    // The field has the keyboard with the whole name selected: typing replaces it (#3198).
+    assert!(h.ctx.egui_wants_keyboard_input(), "the rename field on group {uid} has the keyboard");
     h.event(Event::Text(new.into()));
     h.step();
     key(h, Key::Enter);