From c979f585ac4cb3c456394dda151889af5a9e5846 Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Tue, 13 Oct 2026 13:47:22 -0400 Subject: [PATCH] Timeline: focus newly added expressions after Alt-click (refs #1072) Alt-clicking a stopwatch added an expression, but its editor needed another click before typing went into it. In After Effects the new expression's field takes the keyboard at once, its text selected. The pick whip's one-shot refocus (the editor takes the keyboard back after a reference goes in) is now an ExprFocus request for any property: which text to select, counted from the end, and the pass it was asked for in. An Alt-click that adds an expression asks for it with the whole text selected, so typing replaces it; the expression row is opened if it was closed and scrolled into view (it is added below the stopwatch's row, which can be the last one shown). The request is taken once, when the editor is drawn, and dropped if its expression doesn't appear or it isn't taken within a few passes, so it never takes the keyboard later; an Alt-click that removes the expression clears it. Tests (ui_inline_edit): typing straight after the Alt-click replaces the new expression; a field clicked right after keeps the keyboard; after removing it nothing has the keyboard; an expression added at the Timeline's bottom brings its editor into view with the keyboard. Both fail on main. --- crates/ui-egui/src/panels/timeline.rs | 58 +++++++++++++--- crates/ui-egui/tests/ui_inline_edit.rs | 92 ++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/panels/timeline.rs b/crates/ui-egui/src/panels/timeline.rs index 163e6507d..72c071107 100644 --- a/crates/ui-egui/src/panels/timeline.rs +++ b/crates/ui-egui/src/panels/timeline.rs @@ -265,10 +265,25 @@ fn whip_into_id(prop: u64) -> egui::Id { egui::Id::new(("tl-pickwhip-into", prop)) } -/// After a pick whip into the expression being edited: its editor takes the focus back, the -/// cursor this many characters before the end (just after the reference). -fn expr_refocus_id(prop: u64) -> egui::Id { - egui::Id::new(("tl-expr-refocus", prop)) +/// An expression editor to take the keyboard when it is next drawn: its property, the text to +/// select as counts of characters before the end (so it fits the expression as it will be then) +/// and the pass it was asked for in. After a pick whip into the expression being edited, the +/// cursor goes just after the reference; after an Alt-click on a stopwatch adds an expression, +/// its whole text is selected, so typing replaces it. +#[derive(Clone, Copy, Debug)] +struct ExprFocus { + prop: u64, + select: [usize; 2], + pass: u64, +} + +fn expr_focus_id() -> egui::Id { + egui::Id::new("tl-expr-focus") +} + +fn focus_expression(ctx: &egui::Context, prop: u64, select: [usize; 2]) { + let pass = ctx.cumulative_pass_nr(); + ctx.data_mut(|d| d.insert_temp(expr_focus_id(), ExprFocus { prop, select, pass })); } #[derive(Clone, Debug)] @@ -1888,6 +1903,19 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { let rh = t.row_h; let total_h: f32 = rows.iter().map(|r| row_height(r, rh)).sum(); let max_scroll = (total_h - rows_rect.height() + rh).max(0.0); + // An expression editor asked to take the keyboard comes into view; a request whose + // expression didn't appear, or that wasn't taken within a few passes, is dropped (it never + // takes the keyboard later). + if let Some(f) = ctx.data(|d| d.get_temp::(expr_focus_id())) { + match rows.iter().position(|r| matches!(r.kind, RowKind::Expr { uid, .. } if uid == f.prop)) { + Some(i) if ctx.cumulative_pass_nr() <= f.pass.saturating_add(8) => { + let top: f32 = rows.iter().take(i).map(|r| row_height(r, rh)).sum(); + let fits = (top + rows.get(i).map_or(rh, |r| row_height(r, rh)) - rows_rect.height()).max(0.0).min(top); + app.ui.timeline.scroll_y = app.ui.timeline.scroll_y.min(top).max(fits); + } + _ => ctx.data_mut(|d| d.remove::(expr_focus_id())), + } + } if scroll_to == Some(crate::state::ScrollTo::Layer) { // The topmost selected layer, in stack order. let sel = &app.session.state.selected_layers; @@ -2630,12 +2658,15 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { gp.rect_filled(Rect::from_min_max(pos2(graph_x0, r.min.y), r.max), 0.0, t.expr_bg); let er = Rect::from_min_max(pos2(graph_x0 + 8.0, r.min.y + 3.0), pos2(rect.max.x - 14.0, r.max.y - 3.0)); let mut buf: String = ctx.data(|d| d.get_temp(buf_id)).unwrap_or_else(|| ex.text.clone()); - if let Some(after) = ctx.data_mut(|d| d.remove_temp::(expr_refocus_id(*uid))) { - // A pick whip just put a reference in: editing goes on in the new text, - // the cursor after the reference. + if let Some(f) = ctx.data(|d| d.get_temp::(expr_focus_id())).filter(|f| f.prop == *uid) { + // Asked for once (a pick whip put a reference in, or an Alt-click added the + // expression): editing starts in the expression as it is now. + ctx.data_mut(|d| d.remove::(expr_focus_id())); buf = ex.text.clone(); + let n = buf.chars().count(); + let [a, b] = f.select.map(|before_end| egui::text::CCursor::new(n.saturating_sub(before_end))); let mut st = egui::text_edit::TextEditState::load(&ctx, edit_id).unwrap_or_default(); - st.cursor.set_char_range(Some(egui::text::CCursorRange::one(egui::text::CCursor::new(buf.chars().count().saturating_sub(after))))); + st.cursor.set_char_range(Some(egui::text::CCursorRange::two(a, b))); st.store(&ctx, edit_id); ctx.memory_mut(|m| m.request_focus(edit_id)); } @@ -2733,6 +2764,15 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { if ui.input(|i| i.modifiers.alt) { if resp.clicked() { actions.push(("prop.setExpression".into(), json!({"layer": layer.id.0, "prop": uid}))); + if prop.expr.is_none() { + // Adding one: its editor opens, comes into view and takes the + // keyboard with the new expression selected (#1072). + app.ui.timeline.expr_closed.remove(uid); + focus_expression(&ctx, *uid, [usize::MAX, 0]); + } else if ctx.data(|d| d.get_temp::(expr_focus_id())).is_some_and(|f| f.prop == *uid) { + // Removing it: nothing is left to take the keyboard. + ctx.data_mut(|d| d.remove::(expr_focus_id())); + } } } else if let Some((value, merge)) = switch_gesture(ui, &resp, r, "stopwatch", prop.is_animated()) { actions.push(("prop.toggleAnimation".into(), json!({"layer": layer.id.0, "prop": uid, "value": value, "merge": merge}))); @@ -3057,7 +3097,7 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { params["expression"] = json!(text); params["range"] = json!([a, b]); let after = text.chars().count().saturating_sub(b); - ctx.data_mut(|d| d.insert_temp(expr_refocus_id(src_prop), after)); + focus_expression(&ctx, src_prop, [after, after]); } actions.push(("prop.pickWhip".into(), params)); } diff --git a/crates/ui-egui/tests/ui_inline_edit.rs b/crates/ui-egui/tests/ui_inline_edit.rs index dd20e55b0..7bc5505ed 100644 --- a/crates/ui-egui/tests/ui_inline_edit.rs +++ b/crates/ui-egui/tests/ui_inline_edit.rs @@ -539,3 +539,95 @@ fn arrow_keys_move_the_caret_in_rename_fields() { let layer_name = h.state().session.active_comp().unwrap().layer(effectcraft_engine::project::LayerId(layer)).unwrap().name.clone(); assert_eq!(layer_name, "Pla-te"); } + +/// A solid named `name` with its Transform group open in the Timeline; returns the layer, its +/// Opacity uid and its Rotation uid. +fn open_transform(h: &mut Harness<'_, EffectcraftApp>, name: &str) -> (u64, u64, u64) { + let (layer, opacity, rotation, transform) = { + let s = &mut h.state_mut().session; + let l = s.execute("layer.newSolid", json!({"name": name, "color": "#406080"})).unwrap()["layer"].as_u64().unwrap(); + let tr = s.active_comp().unwrap().layer(effectcraft_engine::project::LayerId(l)).unwrap().props.sub("transform").unwrap().clone(); + (l, tr.get("opacity").unwrap().uid, tr.get("rotation").unwrap().uid, tr.uid) + }; + h.state_mut().ui.timeline.open_layers.insert(layer); + h.state_mut().ui.timeline.open_groups.insert(transform); + h.run_steps(3); + (layer, opacity, rotation) +} + +fn expression(h: &Harness<'_, EffectcraftApp>, layer: u64, prop: u64) -> Option { + let comp = h.state().session.active_comp().unwrap(); + comp.layer(effectcraft_engine::project::LayerId(layer)).unwrap().props.find(prop).unwrap().expr.as_ref().map(|e| e.text.clone()) +} + +fn commit_expression(h: &mut Harness<'_, EffectcraftApp>) { + h.input_mut().events.push(Event::ModifiersChanged(Modifiers::COMMAND)); + h.input_mut().events.push(Event::Key { key: Key::Enter, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::COMMAND }); + h.step(); + h.input_mut().events.push(Event::ModifiersChanged(Modifiers::NONE)); + h.run_steps(3); +} + +/// #1072: Alt-clicking a stopwatch adds an expression and its editor takes the keyboard at once, +/// the new expression selected, so typing replaces it (it took another click). That happens +/// once: a field clicked next keeps the keyboard, and removing the expression with another +/// Alt-click leaves nothing to take it. +#[test] +fn alt_clicking_a_stopwatch_focuses_the_new_expression() { + let (mut h, _) = harness(); + let (layer, opacity, rotation) = open_transform(&mut h, "Plate"); + let alt = Modifiers { alt: true, ..Modifiers::NONE }; + let stopwatch = |h: &Harness<'_, EffectcraftApp>, prop: u64| center(h, &format!("timeline.prop.{prop}.stopwatch")); + let at = stopwatch(&h, opacity); + click_with(&mut h, at, alt); + assert!(expression(&h, layer, opacity).is_some(), "an expression was added"); + type_text(&mut h, "50"); + commit_expression(&mut h); + assert_eq!(expression(&h, layer, opacity).as_deref(), Some("50"), "typing replaced the new expression"); + // A field clicked right after an Alt-click keeps the keyboard. + let at = stopwatch(&h, rotation); + click_with(&mut h, at, alt); + let added = expression(&h, layer, rotation).expect("an expression on Rotation"); + let p = center(&h, "timeline.timecode"); + click(&mut h, p, 2); + type_text(&mut h, "0:00:01:00"); + key(&mut h, Key::Enter); + let frame = h.state().session.active_comp().unwrap().frame_rate.frame_at(h.state().session.time()); + assert_eq!((frame, expression(&h, layer, rotation)), (24, Some(added)), "the time took the typing, not the expression"); + // Alt-click again removes the expression; nothing takes the keyboard after it. + let at = stopwatch(&h, opacity); + click_with(&mut h, at, alt); + h.run_steps(5); + assert_eq!(expression(&h, layer, opacity), None); + assert!(!h.ctx.egui_wants_keyboard_input(), "nothing has the keyboard"); +} + +/// #1072: an expression added at the bottom of the Timeline brings its editor into view to take +/// the keyboard (its row is added below the last one shown). +#[test] +fn an_expression_added_at_the_bottom_of_the_timeline_comes_into_view() { + let (mut h, _) = harness(); + let (layer, opacity, _) = open_transform(&mut h, "Bottom"); + for i in 0..40 { + h.state_mut().session.execute("layer.newSolid", json!({"name": format!("Layer {i}"), "color": "#406080"})).unwrap(); + } + h.run_steps(3); + // Scroll to the end (the layer is at the bottom of the stack), then until Opacity is the + // last row shown. + h.state_mut().ui.timeline.scroll_y = 1.0e6; + h.run_steps(3); + let rows = h.state().auto.find("timeline.scroll").unwrap().rect; + let bottom = rows[1] + rows[3]; + let row = h.state().auto.find(&format!("timeline.prop.{opacity}.row")).unwrap().rect; + h.state_mut().ui.timeline.scroll_y += row[1] + row[3] - bottom; + h.run_steps(3); + let row = h.state().auto.find(&format!("timeline.prop.{opacity}.row")).unwrap().rect; + assert!((row[1] + row[3] - bottom).abs() < 1.0, "Opacity is at the bottom: {row:?}"); + let at = center(&h, &format!("timeline.prop.{opacity}.stopwatch")); + click_with(&mut h, at, Modifiers { alt: true, ..Modifiers::NONE }); + let editor = h.state().auto.find(&format!("timeline.prop.{opacity}.expression")).expect("the editor is drawn").rect; + assert!(editor[1] >= rows[1] - 0.5 && editor[1] + editor[3] <= bottom + 0.5, "the editor is in view: {editor:?} in {rows:?}"); + type_text(&mut h, "25"); + commit_expression(&mut h); + assert_eq!(expression(&h, layer, opacity).as_deref(), Some("25")); +}