Merge pull request #2655 from storytold/fix/2152-discrete-undo-steps

Discrete property choices are one undo step each (#2152)

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<String>) {
+        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).

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);
 }

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