From af50ad43f32a1bd55643da276a6deaa250b0f9a0 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sat, 7 Nov 2026 12:22:14 +0100 Subject: [PATCH] Effect Controls: renaming an effect opens with the whole name selected Enter on a selected effect (in Effect Controls, or in the Timeline while Effect Controls shows the effect) focused the effect's rename field before the field existed. The field was then drawn in the same pass as that Enter, took it and gave the keyboard straight back, so it stayed open without the keyboard and with the caret after the name: typing didn't replace the name. The field now takes the keyboard and selects its whole name the pass it is first drawn, as a double-click or the header menu's Rename already opened it, and that pass doesn't commit it. It also always opens on the name the header draws: Enter opened it on the English registry name, so in a translated interface the field showed a different name than the header, and committing it untouched counted as a rename. Fixes #3198 --- .../src/panels/effect_controls/panel.rs | 30 +++++++---- crates/ui-egui/tests/it/ui_effect_controls.rs | 53 +++++++++++++++++++ crates/ui-egui/tests/it/ui_effect_names.rs | 33 +++++++++++- crates/ui-egui/tests/it/ui_group_rename.rs | 3 +- 4 files changed, 106 insertions(+), 13 deletions(-) diff --git a/crates/ui-egui/src/panels/effect_controls/panel.rs b/crates/ui-egui/src/panels/effect_controls/panel.rs index 0857dfa3f..cf12711e3 100644 --- a/crates/ui-egui/src/panels/effect_controls/panel.rs +++ b/crates/ui-egui/src/panels/effect_controls/panel.rs @@ -5,12 +5,14 @@ use super::{Align2, Color32, EffectcraftApp, EvalCtx, GroupKind, Icon, Layer, Pr // --------------------------------------------------------------------------------------------- -/// 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. @@ -29,16 +31,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::(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::(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::(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. diff --git a/crates/ui-egui/tests/it/ui_effect_controls.rs b/crates/ui-egui/tests/it/ui_effect_controls.rs index 1db059220..22f9a3d50 100644 --- a/crates/ui-egui/tests/it/ui_effect_controls.rs +++ b/crates/ui-egui/tests/it/ui_effect_controls.rs @@ -866,6 +866,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] diff --git a/crates/ui-egui/tests/it/ui_effect_names.rs b/crates/ui-egui/tests/it/ui_effect_names.rs index d57f9c581..01b57178b 100644 --- a/crates/ui-egui/tests/it/ui_effect_names.rs +++ b/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] diff --git a/crates/ui-egui/tests/it/ui_group_rename.rs b/crates/ui-egui/tests/it/ui_group_rename.rs index cf56ac6f4..999ed1e3e 100644 --- a/crates/ui-egui/tests/it/ui_group_rename.rs +++ b/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);