diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 7051ebfb..7ef12398 100644 --- a/crates/ui-egui/src/lib.rs +++ b/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; diff --git a/crates/ui-egui/src/panels/format_shape.rs b/crates/ui-egui/src/panels/format_shape.rs index 03a2d2c9..110dd8fc 100644 --- a/crates/ui-egui/src/panels/format_shape.rs +++ b/crates/ui-egui/src/panels/format_shape.rs @@ -517,8 +517,22 @@ fn fx_slider( } else { ui.data_mut(|d| d.remove::(sid)); } - if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged()) { - ui.data_mut(|d| d.remove::(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::(edited_id)).unwrap_or(false); + if r.drag_stopped() || (r.lost_focus() && edited) || (shown_changed && !r.dragged()) { + ui.data_mut(|d| { + d.remove::(sid); + d.remove::(edited_id); + }); run(app, "shape.effects", json!({key: {prop: v * scale}})); } }); diff --git a/crates/ui-egui/src/reflection_pane_tests.rs b/crates/ui-egui/src/reflection_pane_tests.rs new file mode 100644 index 00000000..a0ef4b98 --- /dev/null +++ b/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"); +}