From e1c9c33fb150d38777e957913fa9baafcda7ad8e Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Mon, 12 Oct 2026 18:49:48 -0400 Subject: [PATCH] Keyframes: a refused key drag step doesn't count towards the drag (#1012) keys.quickOffset and keys.move stored the drag's new total before the edit ran. When the edit refused the step (keys.quickOffset with a key landing past the end of time), the project stayed as it was but the total kept the refused delta, and with incremental steps (merge without fromStart) the next step moved by it too. The drag state is now stored only after the edit applies. Test: a_refused_step_does_not_count_towards_the_drag (+1 s, a refused step, -1 s: the keys end where they started; fails before: the last key ends near the end of time). --- crates/engine/src/commands/prop.rs | 9 ++++++--- crates/engine/src/commands/quick_offset.rs | 9 ++++++--- crates/engine/src/tests_quick_offset.rs | 17 +++++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/commands/prop.rs b/crates/engine/src/commands/prop.rs index 57ea8deb4..5248949a4 100644 --- a/crates/engine/src/commands/prop.rs +++ b/crates/engine/src/commands/prop.rs @@ -941,8 +941,8 @@ pub(crate) fn layer_shift(l: &Layer, d: Tick) -> Tick { pub(crate) fn shift_keys(s: &mut Session, label: &str, merge: Option<&str>, d: Tick) -> Result { let cid = s.active_comp_id().ok_or(EngineError::NoComp)?; let (from, total, base) = key_gesture(s, merge, d, false); - s.key_move = merge.map(|m| crate::KeyMove { merge: m.to_string(), from: from.clone(), total }); - s.edit(label, merge, |proj, st| { + let next = merge.map(|m| crate::KeyMove { merge: m.to_string(), from: from.clone(), total }); + let moved = s.edit(label, merge, |proj, st| { if let Some(b) = base { *proj = (*b).clone(); } @@ -950,7 +950,10 @@ pub(crate) fn shift_keys(s: &mut Session, label: &str, merge: Option<&str>, d: T let d = comp.frame_rate.snap_nearest(total); st.selected_keys = move_key_refs(comp, &from, |l, t| Ok(t + layer_shift(l, d)))?; Ok(json!(st.selected_keys.len())) - }) + })?; + // Only a step that applied counts towards the drag. + s.key_move = next; + Ok(moved) } fn move_keys(s: &mut Session, p: &Value) -> Result { diff --git a/crates/engine/src/commands/quick_offset.rs b/crates/engine/src/commands/quick_offset.rs index c3c377f7f..312013455 100644 --- a/crates/engine/src/commands/quick_offset.rs +++ b/crates/engine/src/commands/quick_offset.rs @@ -101,8 +101,8 @@ fn keys(s: &mut Session, p: &Value) -> Result { let comp = start.comp(cid).ok_or(EngineError::NoComp)?; let order = order(NAME, from.iter().map(|k| k.layer), |id| comp.layer(id).is_some_and(|l| !l.switches.locked))?; let by_layer: HashMap = order.iter().copied().zip(shares(total, order.len())).collect(); - s.key_move = merge.map(|m| crate::KeyMove { merge: m.to_string(), from: from.clone(), total }); - s.edit("Quick Offset Keyframes", merge, |proj, st| { + let next = merge.map(|m| crate::KeyMove { merge: m.to_string(), from: from.clone(), total }); + let moved = s.edit("Quick Offset Keyframes", merge, |proj, st| { if let Some(b) = &base { *proj = (**b).clone(); } @@ -114,7 +114,10 @@ fn keys(s: &mut Session, p: &Value) -> Result { shifted(t, layer_shift(l, *d), NAME, &what) })?; Ok(json!({"keys": st.selected_keys.len(), "layers": order.iter().map(|id| json!({"layer": id.0, "offset": by_layer.get(id).copied().unwrap_or_default().seconds()})).collect::>()})) - }) + })?; + // Only a step that applied counts towards the drag (a refused one is as if never sent). + s.key_move = next; + Ok(moved) } pub fn specs() -> Vec { diff --git a/crates/engine/src/tests_quick_offset.rs b/crates/engine/src/tests_quick_offset.rs index aeff851a7..164bf8753 100644 --- a/crates/engine/src/tests_quick_offset.rs +++ b/crates/engine/src/tests_quick_offset.rs @@ -268,3 +268,20 @@ fn moving_staggered_keys_keeps_their_subframe_spacing() { assert_eq!(key_times(&s, *id, "transform/opacity"), vec![2 * F + F * i as i64 / 3], "layer {i}"); } } + +#[test] +fn a_refused_step_does_not_count_towards_the_drag() { + let (mut s, ids) = setup(2); + for id in &ids { + add_key(&mut s, *id, "transform/opacity", 1.0, json!(0)); + } + select_keys(&mut s, &ids.iter().map(|id| (*id, "transform/opacity", 1.0)).collect::>()); + let before = s.project.clone(); + // The second step's last key would land past the end of time (one second short of it, its + // own delta isn't); with it counted, the third would still move the key that far. + let far = (Tick::MAX.seconds() - 1.0).floor(); + s.execute_checked("keys.quickOffset", json!({"delta": 1.0, "merge": "steps"})).unwrap(); + assert!(s.execute_checked("keys.quickOffset", json!({"delta": far, "merge": "steps"})).is_err()); + s.execute_checked("keys.quickOffset", json!({"delta": -1.0, "merge": "steps"})).unwrap(); + assert!(s.project.same_content(&before), "only the steps that applied count: {:?}", key_times(&s, ids[1], "transform/opacity")); +}