diff --git a/crates/ui-egui/src/panels/effect_controls/panel.rs b/crates/ui-egui/src/panels/effect_controls/panel.rs index 1904600df..71939b460 100644 --- a/crates/ui-egui/src/panels/effect_controls/panel.rs +++ b/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::(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 8f2910e43..188fb7085 100644 --- a/crates/ui-egui/tests/it/ui_effect_controls.rs +++ b/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] 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);