From 5a28760b1e2fda330f2696ab59642439d7b6b9c4 Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:29:31 -0500 Subject: [PATCH 1/5] fix(format): Reflection None clears after another preset (#1356) The effect sliders rounded 55.000000000000007 to 55 and read that as a change, so each frame re-added the reflection the None button had just cleared. --- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/panes.rs | 6 +++ crates/ui-egui/src/reflection_pane_tests.rs | 43 +++++++++++++++++++++ 3 files changed, 51 insertions(+) create mode 100644 crates/ui-egui/src/reflection_pane_tests.rs diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 4c5a7a68..ebff5dd1 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -3571,6 +3571,8 @@ mod ime_tests; #[cfg(test)] mod last_color_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/panes.rs b/crates/ui-egui/src/panes.rs index 74482f93..cb294a3b 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -658,8 +658,14 @@ fn fx_slider( } else { ui.data_mut(|d| d.remove::(sid)); } + // The slider rounds to whole numbers, which reads as a change for a value such as 55.000000000000007; + // acting on it would re-add an effect the pane just cleared. + let moved = v.round() != value.round(); if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged()) { ui.data_mut(|d| d.remove::(sid)); + if !moved { + return; + } 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..dbedd524 --- /dev/null +++ b/crates/ui-egui/src/reflection_pane_tests.rs @@ -0,0 +1,43 @@ +//! 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 reflection"); +} From 1b2e5ea9e0ffc902a4afb22ca1b158453de2a3b2 Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:36:59 -0500 Subject: [PATCH 2/5] fix(format): keep explicit slider edits, skip only the rounding --- crates/ui-egui/src/panes.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index cb294a3b..57fbb5e1 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -658,14 +658,10 @@ fn fx_slider( } else { ui.data_mut(|d| d.remove::(sid)); } - // The slider rounds to whole numbers, which reads as a change for a value such as 55.000000000000007; - // acting on it would re-add an effect the pane just cleared. - let moved = v.round() != value.round(); - if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged()) { + // `changed()` alone also fires when the slider only rounds the shown value (55.000000000000007 to 55), + // which re-added a reflection the pane had just cleared; a keyboard edit has focus. + if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged() && r.has_focus()) { ui.data_mut(|d| d.remove::(sid)); - if !moved { - return; - } run(app, "shape.effects", json!({key: {prop: v * scale}})); } }); From faf4c0ee52d2a30b76b7da7aadd92fe366843274 Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:10:49 -0500 Subject: [PATCH 3/5] fix(format): Reflection slider with only focus does not re-add the reflection A reflection slider that had focus and lost it to the None button committed its stale value. Commit on focus loss only after an edit the user made, and cover it with a kittest. --- crates/ui-egui/src/panels/format_shape.rs | 14 ++++++++-- crates/ui-egui/src/reflection_pane_tests.rs | 30 +++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/panels/format_shape.rs b/crates/ui-egui/src/panels/format_shape.rs index 48d5df9d..002226d9 100644 --- a/crates/ui-egui/src/panels/format_shape.rs +++ b/crates/ui-egui/src/panels/format_shape.rs @@ -477,10 +477,20 @@ fn fx_slider( } else { ui.data_mut(|d| d.remove::(sid)); } + // 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 r.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); // `changed()` alone also fires when the slider only rounds the shown value (55.000000000000007 to 55), // which re-added a reflection the pane had just cleared; a keyboard edit has focus. - if r.drag_stopped() || r.lost_focus() || (r.changed() && !r.dragged() && r.has_focus()) { - ui.data_mut(|d| d.remove::(sid)); + if r.drag_stopped() || (r.lost_focus() && edited) || (r.changed() && !r.dragged() && r.has_focus()) { + 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 index dbedd524..dda97afd 100644 --- a/crates/ui-egui/src/reflection_pane_tests.rs +++ b/crates/ui-egui/src/reflection_pane_tests.rs @@ -41,3 +41,33 @@ fn reflection_none_clears_after_a_second_preset() { h.run_steps(4); assert!(!has_reflection(&h), "None clears the 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"); +} From 90ce7dcb9b00f56c4d895fc1102c4870b9f158f8 Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:18:14 -0500 Subject: [PATCH 4/5] test(format): clear the reflection after each preset, as #1356 reports --- crates/ui-egui/src/reflection_pane_tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/reflection_pane_tests.rs b/crates/ui-egui/src/reflection_pane_tests.rs index dda97afd..a0ef4b98 100644 --- a/crates/ui-egui/src/reflection_pane_tests.rs +++ b/crates/ui-egui/src/reflection_pane_tests.rs @@ -36,10 +36,10 @@ fn reflection_none_clears_after_a_second_preset() { 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"); } - h.get_all_by_label("None").last().unwrap().click(); - h.run_steps(4); - assert!(!has_reflection(&h), "None clears the reflection"); } #[test] From d98938944126978db2e3538eef28f01c58bd12b9 Mon Sep 17 00:00:00 2001 From: Harrison Booth <73434116+Harrison-Booth@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:31:36 -0500 Subject: [PATCH 5/5] fix(format): a slider commits only a move to another shown value changed() also fires when the slider rounds the value it shows, on focus too. Compare the rounded values, so keyboard and accessibility edits commit without focus and rounding does not. --- crates/ui-egui/src/panels/format_shape.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/panels/format_shape.rs b/crates/ui-egui/src/panels/format_shape.rs index 002226d9..4204f1a3 100644 --- a/crates/ui-egui/src/panels/format_shape.rs +++ b/crates/ui-egui/src/panels/format_shape.rs @@ -477,16 +477,18 @@ fn fx_slider( } else { 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 r.changed() { + 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); - // `changed()` alone also fires when the slider only rounds the shown value (55.000000000000007 to 55), - // which re-added a reflection the pane had just cleared; a keyboard edit has focus. - if r.drag_stopped() || (r.lost_focus() && edited) || (r.changed() && !r.dragged() && r.has_focus()) { + if r.drag_stopped() || (r.lost_focus() && edited) || (shown_changed && !r.dragged()) { ui.data_mut(|d| { d.remove::(sid); d.remove::(edited_id);