Merge pull request #1437

fix(format): Reflection None clears after another preset (#1356)

# Conflicts:
#	crates/ui-egui/src/lib.rs

crates/ui-egui/src/lib.rs

@@ -462,6 +462,8 @@ mod last_color_tests;
 #[cfg(test)]
 mod presenter_tests;
 #[cfg(test)]
+mod reflection_pane_tests;
+#[cfg(test)]
 mod replacefonts_tests;
 #[cfg(test)]
 mod reuse_picker_tests;

crates/ui-egui/src/panels/format_shape.rs

@@ -517,8 +517,22 @@ fn fx_slider(
         } else {
             ui.data_mut(|d| d.remove::<f64>(sid));
         }
-        if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged()) {
-            ui.data_mut(|d| d.remove::<f64>(sid));
+        // `changed()` also fires when the slider only rounds the value it shows (55.000000000000007 to 55,
+        // or 0.5 to 1 on focus, which re-added a reflection the pane had just cleared): only a move to
+        // another shown value is an edit. Accessibility and keyboard edits are such moves too.
+        let shown_changed = r.changed() && v.round() != value.round();
+        // A slider that only had focus when another control took it (the Reflection "None" button)
+        // must not commit its stale value: only an edit the user made is committed on losing focus.
+        let edited_id = sid.with("edited");
+        if shown_changed {
+            ui.data_mut(|d| d.insert_temp(edited_id, true));
+        }
+        let edited = ui.data_mut(|d| d.get_temp::<bool>(edited_id)).unwrap_or(false);
+        if r.drag_stopped() || (r.lost_focus() && edited) || (shown_changed && !r.dragged()) {
+            ui.data_mut(|d| {
+                d.remove::<f64>(sid);
+                d.remove::<bool>(edited_id);
+            });
             run(app, "shape.effects", json!({key: {prop: v * scale}}));
         }
     });

crates/ui-egui/src/reflection_pane_tests.rs

@@ -0,0 +1,73 @@
+//! Format Shape ▸ Effects (#1356): Reflection "None" clears the effect after another preset.
+#![allow(clippy::unwrap_used, clippy::expect_used)]
+
+use egui_kittest::Harness;
+use egui_kittest::kittest::Queryable;
+use serde_json::json;
+
+use crate::{Services, SlideApp};
+use deckcraft_engine::Session;
+
+fn has_reflection(h: &Harness<'_, SlideApp>) -> bool {
+    let st = h.state().session.active().unwrap();
+    let id = *st.selection.shapes.first().unwrap();
+    st.shape(id).unwrap().effects.as_ref().is_some_and(|e| e.reflection.is_some())
+}
+
+#[test]
+fn reflection_none_clears_after_a_second_preset() {
+    let mut s = Session::with_new();
+    s.execute("slide.new", &json!({"layout": "blank"})).unwrap();
+    let id = s.execute("shape.insert", &json!({"preset": "rect", "rect": [72.0, 36.0, 144.0, 108.0]})).unwrap()["id"].as_u64().unwrap();
+    s.execute("edit.select", &json!({"ids": [id]})).unwrap();
+    let mut app = SlideApp::new(s, Services::default());
+    app.ui.pane = Some("format".into());
+    app.ui.format_tab = "effects".into();
+    let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_ui_state(
+        |ui, app: &mut SlideApp| {
+            app.logic(ui.ctx());
+            app.ui(ui);
+        },
+        app,
+    );
+    h.run_steps(3);
+    // The Shadow row has a "None" too; Reflection's is the later one.
+    for preset in ["Tight", "Half", "Full"] {
+        h.get_by_label(preset).click();
+        h.run_steps(4);
+        assert!(has_reflection(&h), "{preset} adds a reflection");
+        h.get_all_by_label("None").last().unwrap().click();
+        h.run_steps(4);
+        assert!(!has_reflection(&h), "None clears the {preset} reflection");
+    }
+}
+
+#[test]
+fn reflection_none_stays_cleared_when_a_slider_only_had_focus() {
+    let mut s = Session::with_new();
+    s.execute("slide.new", &json!({"layout": "blank"})).unwrap();
+    let id = s.execute("shape.insert", &json!({"preset": "rect", "rect": [72.0, 36.0, 144.0, 108.0]})).unwrap()["id"].as_u64().unwrap();
+    s.execute("edit.select", &json!({"ids": [id]})).unwrap();
+    let mut app = SlideApp::new(s, Services::default());
+    app.ui.pane = Some("format".into());
+    app.ui.format_tab = "effects".into();
+    let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_ui_state(
+        |ui, app: &mut SlideApp| {
+            app.logic(ui.ctx());
+            app.ui(ui);
+        },
+        app,
+    );
+    h.run_steps(3);
+    h.get_by_label("Tight").click();
+    h.run_steps(4);
+    // Focus a reflection slider without editing it, then click "None": the slider's value must not
+    // be written back over the cleared reflection.
+    // The reflection's Size comes before the glow's (the same label).
+    h.get_all_by_role_and_label(egui::accesskit::Role::Slider, "Size").next().unwrap().focus();
+    h.run_steps(4);
+    assert!(h.get_all_by_role_and_label(egui::accesskit::Role::Slider, "Size").next().unwrap().is_focused(), "the reflection size slider has focus");
+    h.get_all_by_label("None").last().unwrap().click();
+    h.run_steps(4);
+    assert!(!has_reflection(&h), "a slider that only had focus does not re-add the reflection");
+}