From 53a0532c1fbee425b27d2820ab0f56e85f04dba8 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Fri, 30 Oct 2026 12:15:50 +0700 Subject: [PATCH] Discrete property choices are one undo step each (#2152) Timeline and Effect Controls value editors sent every write with the property's scrub merge key, so two separate picks from a drop-down (or two check box toggles) on the same property folded into one undo step. Drop-down, check box, layer and mask picks now record their own step; scrubs and typed numbers still merge as before. Co-Authored-By: Claude Opus 5.5 --- .../src/panels/effect_controls/rows.rs | 21 +++++++++++---- crates/ui-egui/src/panels/timeline.rs | 20 ++++++++++---- .../tests/it/ui_layer_source_picker.rs | 27 +++++++++++++++++-- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/crates/ui-egui/src/panels/effect_controls/rows.rs b/crates/ui-egui/src/panels/effect_controls/rows.rs index 8a75dbff6..665922fcf 100644 --- a/crates/ui-egui/src/panels/effect_controls/rows.rs +++ b/crates/ui-egui/src/panels/effect_controls/rows.rs @@ -93,8 +93,19 @@ impl PropCx<'_> { self.t.for_value(self.prop.has_expression()) } + /// Queues the `prop.set` of one edit; consecutive edits of the parameter (a scrub) are one undo step. fn set(&self, actions: &mut Actions, v: serde_json::Value) { - let (layer, uid, merge, scrub) = (self.layer, self.uid, format!("ec-{}", self.uid), self.scrub); + self.push(actions, v, Some(format!("ec-{}", self.uid))); + } + + /// Queues the `prop.set` of a discrete choice (a pop-up pick, a check box): its own undo step, + /// never merged with the parameter's previous edit (#2152). + fn set_step(&self, actions: &mut Actions, v: serde_json::Value) { + self.push(actions, v, None); + } + + fn push(&self, actions: &mut Actions, v: serde_json::Value, merge: Option) { + let (layer, uid, scrub) = (self.layer, self.uid, self.scrub); actions.push(("prop.set".into(), json!({"layer": layer.id.0, "prop": uid, "value": v, "merge": merge, "selected": true, "offset": scrub}))) } @@ -240,7 +251,7 @@ fn value_mask(app: &mut EffectcraftApp, ui: &mut egui::Ui, cx: &PropCx, value: & } app.auto.add(&format!("effectControls.prop.{uid}.value"), dr, &prop.name); if let Some(ni) = widgets::popup_menu(ui, pop, dr.left_bottom(), &opts, Some(i)) { - cx.set(actions, json!(ni as f64)); + cx.set_step(actions, json!(ni as f64)); } } @@ -368,7 +379,7 @@ fn value_toggle(app: &mut EffectcraftApp, ui: &mut egui::Ui, cx: &PropCx, value: let PropCx { t, uid, cy, vx, p, prop, .. } = *cx; let cr = Rect::from_min_size(pos2(vx, cy - 8.0), vec2(16.0, 16.0)); if widgets::checkbox(ui, cr, *b, &t, egui::Id::unique(("ec-b", uid))).changed() { - cx.set(actions, json!(!b)); + cx.set_step(actions, json!(!b)); } p.text(pos2(cr.max.x + 6.0, cy), Align2::LEFT_CENTER, &prop.name, Tokens::ui(12.0), t.text_dim); app.auto.add(&format!("effectControls.prop.{uid}.value"), cr, &prop.name); @@ -388,7 +399,7 @@ fn value_popup(app: &mut EffectcraftApp, ui: &mut egui::Ui, cx: &PropCx, value: } app.auto.add(&format!("effectControls.prop.{uid}.value"), dr, &prop.name); if let Some(ni) = widgets::popup_menu(ui, pop, dr.left_bottom(), options, Some(*i as usize)) { - cx.set(actions, json!(ni)); + cx.set_step(actions, json!(ni)); } } } @@ -402,7 +413,7 @@ fn value_layer_ref(app: &mut EffectcraftApp, ui: &mut egui::Ui, cx: &PropCx, val if let Some(comp) = comp { let dr = Rect::from_min_size(pos2(vx, cy - 9.0), vec2(((r.max.x - vx) * 0.55).clamp(90.0, 170.0), 18.0)); if let Some(chosen) = widgets::layer_picker(ui, dr, &comp.layers, *l, &t, egui::Id::unique(("ec-l", uid))) { - cx.set(actions, chosen.map(|id| json!(id)).unwrap_or(serde_json::Value::Null)); + cx.set_step(actions, chosen.map(|id| json!(id)).unwrap_or(serde_json::Value::Null)); } app.auto.add(&format!("effectControls.prop.{uid}.value"), dr, &prop.name); // Source / Masks / Effects & Masks (the hidden companion parameter). diff --git a/crates/ui-egui/src/panels/timeline.rs b/crates/ui-egui/src/panels/timeline.rs index 19a8d9fc2..c4ad55192 100644 --- a/crates/ui-egui/src/panels/timeline.rs +++ b/crates/ui-egui/src/panels/timeline.rs @@ -5600,11 +5600,21 @@ impl<'a> ValueEdit<'a> { Self { p, layer, prop, at, uid, merge: format!("scrub-{uid}"), scrub, proportional } } - /// Queues the `prop.set` of one edit. + /// Queues the `prop.set` of one edit; consecutive edits of the property (a scrub) are one undo step. fn set(&self, actions: &mut Vec<(String, serde_json::Value)>, v: serde_json::Value) { + self.push(actions, v, Some(&self.merge)); + } + + /// Queues the `prop.set` of a discrete choice (a drop-down pick, a check box): its own undo + /// step, never merged with the property's previous edit (#2152). + fn set_step(&self, actions: &mut Vec<(String, serde_json::Value)>, v: serde_json::Value) { + self.push(actions, v, None); + } + + fn push(&self, actions: &mut Vec<(String, serde_json::Value)>, v: serde_json::Value, merge: Option<&str>) { actions.push(( "prop.set".into(), - json!({"layer": self.layer.id.0, "prop": self.uid, "value": v, "merge": self.merge, "selected": true, "offset": self.scrub, "proportional": self.proportional}), + json!({"layer": self.layer.id.0, "prop": self.uid, "value": v, "merge": merge, "selected": true, "offset": self.scrub, "proportional": self.proportional}), )) } } @@ -5785,7 +5795,7 @@ fn bool_editor(app: &mut EffectcraftApp, ui: &mut egui::Ui, ve: &ValueEdit<'_>, let x = at.x; let r = Rect::from_min_size(pos2(x, at.y - 8.0), vec2(16.0, 16.0)); if widgets::checkbox(ui, r, *b, &t, egui::Id::unique(("cb", uid))).changed() { - ve.set(actions, json!(!b)); + ve.set_step(actions, json!(!b)); } app.auto.add(&format!("timeline.prop.{uid}.value"), r, &prop.name); } @@ -5805,7 +5815,7 @@ fn enum_editor(app: &mut EffectcraftApp, ui: &mut egui::Ui, ve: &ValueEdit<'_>, } app.auto.add(&format!("timeline.prop.{uid}.value"), r, &prop.name); if let Some(ni) = widgets::popup_menu(ui, pop, r.left_bottom(), &options, Some(*i as usize)) { - ve.set(actions, json!(ni)); + ve.set_step(actions, json!(ni)); } } else { p.text(pos2(x, at.y), Align2::LEFT_CENTER, fmt_num(*i as f64, 0), Tokens::ui(12.0), t.for_value(prop.has_expression()).hot_text); @@ -5825,7 +5835,7 @@ fn layer_editor(app: &mut EffectcraftApp, ui: &mut egui::Ui, ve: &ValueEdit<'_>, if let Some(comp) = app.session.active_comp_arc() && let Some(chosen) = widgets::layer_picker(ui, r, &comp.layers, *l, &t, egui::Id::unique(("tl-lay", uid))) { - ve.set(actions, chosen.map(|id| json!(id)).unwrap_or(serde_json::Value::Null)); + ve.set_step(actions, chosen.map(|id| json!(id)).unwrap_or(serde_json::Value::Null)); } app.auto.add(&format!("timeline.prop.{uid}.value"), r, &prop.name); } diff --git a/crates/ui-egui/tests/it/ui_layer_source_picker.rs b/crates/ui-egui/tests/it/ui_layer_source_picker.rs index ca10e4249..2daa450c4 100644 --- a/crates/ui-egui/tests/it/ui_layer_source_picker.rs +++ b/crates/ui-egui/tests/it/ui_layer_source_picker.rs @@ -53,8 +53,7 @@ fn entry_for(h: &Harness<'_, EffectcraftApp>, layer: u64) -> String { format!("{}. {}", i + 1, comp.layers[i].name) } -/// Puts the layer-valued `prop` of `layer` back to `value` with a direct `prop.set` (no merge key, -/// so the pick below records its own step instead of folding into a picker write), picks `none` +/// Puts the layer-valued `prop` of `layer` back to `value` with a direct `prop.set`, picks `none` /// from the drop-down at `value_id`, and checks the clear takes one undo step that restores `value`. #[track_caller] fn none_pick_clears(h: &mut Harness<'_, EffectcraftApp>, value_id: &str, none: &str, layer: u64, prop: u64, value: u64) { @@ -73,6 +72,24 @@ fn none_pick_clears(h: &mut Harness<'_, EffectcraftApp>, value_id: &str, none: & assert_eq!(layer_value(h, layer, prop), Some(value), "undo restores the selected layer"); } +/// #2152: two picks in a row from the drop-down at `value_id` (`first`, the layer `value`, then +/// `none`) are two undo steps: one Undo goes back to the first pick, not past it. +#[track_caller] +fn picks_are_separate_steps(h: &mut Harness<'_, EffectcraftApp>, value_id: &str, first: &str, none: &str, layer: u64, prop: u64, value: u64) { + let steps = h.state().session.history.undo.len(); + for entry in [first, none] { + let at = rect_of(h, value_id).center(); + click(h, at); + h.get_by_role_and_label(egui::accesskit::Role::Button, entry).click(); + h.run_steps(3); + } + assert_eq!(layer_value(h, layer, prop), None); + assert_eq!(h.state().session.history.undo.len(), steps + 2, "each pick is its own undo step"); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + h.run_steps(2); + assert_eq!(layer_value(h, layer, prop), Some(value), "one undo goes back to the first pick"); +} + /// A light's Source (a layer-valued Light Options property) picks a comp layer from the /// Timeline and undo restores None. #[test] @@ -138,6 +155,9 @@ fn timeline_source_drop_down_picks_the_layer_and_undo_restores_none() { assert_eq!(layer_value(&h, light, source), None, "undo restores None"); none_pick_clears(&mut h, &value_id, "None", light, source, sky); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + h.run_steps(2); + picks_are_separate_steps(&mut h, &value_id, &entry, "None", light, source, sky); } /// The shared widget is the one Effect Controls already used: a Displacement Map's layer @@ -176,6 +196,9 @@ fn effect_controls_layer_param_picks_through_the_shared_widget() { assert_eq!(layer_value(&h, base, prop), None, "undo restores None"); none_pick_clears(&mut h, &value_id, "None", base, prop, map); + h.state_mut().session.execute("edit.undo", json!({})).unwrap(); + h.run_steps(2); + picks_are_separate_steps(&mut h, &value_id, &entry, "None", base, prop, map); } /// The visual record (the `ui_3d` ignored-snapshot convention): the Source drop-down open over