diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 31b190e8..546a386d 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -287,6 +287,23 @@ pub struct Dialog { pub collapsed: bool, /// Parameters the dialog doesn't show but the command needs (kept when editing). pub extra: serde_json::Map, + /// What the inputs were when OK was refused: `error` stays only while they still are. + pub refused: Option, +} + +/// The commands a dialog's inputs make (or why they make none), its picks, and the design's +/// revision (a parameter the commands name can change without changing them). +#[derive(Clone, Debug, PartialEq)] +pub struct Refused { + commands: Result, String>, + picks: Vec, + revision: u64, +} + +impl Refused { + fn now(app: &SolveApp, d: &Dialog) -> Refused { + Refused { commands: apply_commands(app, d), picks: d.inputs.clone(), revision: app.session.revision } + } } /// Can a selection go into an input that accepts `a`? (Planar-ness is checked when picking.) @@ -310,6 +327,12 @@ fn fits(a: Accept, s: &Sel) -> bool { } impl Dialog { + /// Show why OK (or starting the sketch) was refused, for as long as the inputs stay as they are. + fn refuse(&mut self, app: &SolveApp, error: String) { + self.refused = Some(Refused::now(app, self)); + self.error = Some(error); + } + pub(crate) fn new(kind: Kind, inputs: Vec) -> Dialog { Dialog { kind, @@ -321,6 +344,7 @@ impl Dialog { focus: true, collapsed: false, extra: serde_json::Map::new(), + refused: None, } } @@ -1323,6 +1347,21 @@ pub fn show(app: &mut SolveApp, ctx: &egui::Context) { auto_operation(app, &mut d); update_measure(app, &mut d); update_fill_cells(app, &mut d); + // A refused OK's error is about the inputs it ran with (#641). Once a value (in the dialog, + // or on the canvas, which the viewport applied earlier this frame), a pick or the design (a + // parameter the inputs use) changes, it gives way to what the inputs make now: their own + // error, or nothing and the live preview. `built`: whether they make commands, for OK below. + let mut built = None; + if d.error.is_some() + && let Some(was) = &d.refused + { + let now = Refused::now(app, &d); + built = Some(now.commands.is_ok()); + if now != *was { + d.error = now.commands.as_ref().err().cloned(); + d.refused = d.error.is_some().then_some(now); + } + } // Physical materials: the library's, then the design's own (material.create). let material_names = app.session.doc.material_names(); let t = Tokens::get(); @@ -1347,7 +1386,7 @@ pub fn show(app: &mut SolveApp, ctx: &egui::Context) { let horizontal_frame = frame.inner_margin.leftf() + frame.inner_margin.rightf() + 2.0 * frame.stroke.width; // OK is offered once the inputs make a feature. let has_ok = !matches!(d.kind, Kind::Sketch | Kind::Params { .. } | Kind::Measure { .. }); - let valid = !has_ok || !d.previews() || (apply_commands(app, &d).is_ok() && app.preview.error.is_none()); + let valid = !has_ok || !d.previews() || (built.unwrap_or_else(|| apply_commands(app, &d).is_ok()) && app.preview.error.is_none()); let width = if wide { crate::frame::Width::Wide } else { crate::frame::Width::Normal }; let wkey = egui::Id::new(("sc_dialog_w", wide)); let response = crate::frame::docked( @@ -1434,7 +1473,7 @@ pub fn show(app: &mut SolveApp, ctx: &egui::Context) { look_at_sketch(app); cancel = true; } - Err(error) => d.error = Some(error), + Err(error) => d.refuse(app, error), } } ui.end_row(); @@ -2106,7 +2145,7 @@ pub fn show(app: &mut SolveApp, ctx: &egui::Context) { { match start_sketch(app, &sel) { Ok(()) => cancel = true, - Err(error) => d.error = Some(error), + Err(error) => d.refuse(app, error), } } if ok { @@ -2115,7 +2154,7 @@ pub fn show(app: &mut SolveApp, ctx: &egui::Context) { cancel = true; applied = true; } - Err(e) => d.error = Some(e), + Err(e) => d.refuse(app, e), } } if !keep || cancel { @@ -3906,3 +3945,157 @@ mod param_refusal_tests { assert!(change_param(&mut app, &name, "41 mm").is_ok()); } } + +#[cfg(test)] +mod refused_ok_tests { + //! A refused OK's error lasts only as long as the inputs it was about (#641). + + use super::*; + use crate::scenario::Harness; + + fn steps(h: &mut Harness, steps: &[Value]) { + for s in steps { + h.step(s).unwrap(); + } + } + + /// A box, and a hole dialog with a position picked on its top. + fn hole_dialog() -> Harness { + let mut h = Harness::new(); + steps( + &mut h, + &[ + json!({"call": "engine.execute", "params": {"command": "solid.box", "params": {"length": 50, "width": 30, "height": 20}}}), + json!({"call": "ui.view", "params": {"view": "iso"}}), + json!({"start": "solid.hole"}), + json!({"click": {"world": [25, 15, 20]}}), + ], + ); + h + } + + fn set_depth(h: &mut Harness, depth: &str) { + steps(h, &[json!({"click": {"handle": "dialog:Depth"}}), json!({"key": "A", "cmd": true}), json!({"text": depth})]); + } + + /// What the dialog shows in red once the live preview has finished: its own error, else the + /// preview's. + fn shown(h: &mut Harness) -> Option { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60); + while h.app.preview.busy { + assert!(std::time::Instant::now() < deadline, "the preview did not finish"); + h.frames(1); + } + h.app.dialog.as_ref().and_then(|d| d.error.clone()).or_else(|| h.app.preview.error.clone()) + } + + fn refused(h: &mut Harness) { + set_depth(h, "-5"); + steps(h, &[json!({"key": "Enter"})]); + assert_eq!(h.app.dialog.as_ref().and_then(|d| d.error.as_deref()), Some("hole depth must be positive")); + } + + #[test] + fn the_error_stays_while_the_inputs_do() { + let mut h = hole_dialog(); + refused(&mut h); + h.frames(10); + assert_eq!(shown(&mut h).as_deref(), Some("hole depth must be positive")); + } + + #[test] + fn a_value_fixed_in_the_dialog_clears_it() { + let mut h = hole_dialog(); + refused(&mut h); + set_depth(&mut h, "5"); + h.frames(2); + assert_eq!(shown(&mut h), None); + } + + /// The canvas value box and the arrows change the dialog before `show` runs in a frame. + #[test] + fn a_value_fixed_outside_the_dialog_clears_it() { + let mut h = hole_dialog(); + refused(&mut h); + if let Some(Kind::Hole { depth, .. }) = h.app.dialog.as_mut().map(|d| &mut d.kind) { + *depth = "5 mm".into(); + } + h.frames(2); + assert_eq!(shown(&mut h), None); + } + + #[test] + fn a_refusal_for_a_missing_pick_clears_once_it_is_picked() { + let mut h = Harness::new(); + steps( + &mut h, + &[ + json!({"call": "engine.execute", "params": {"command": "solid.box", "params": {"length": 50, "width": 30, "height": 20}}}), + json!({"call": "ui.view", "params": {"view": "iso"}}), + json!({"start": "solid.hole"}), + json!({"key": "Enter"}), + ], + ); + let error = h.app.dialog.as_ref().and_then(|d| d.error.clone()); + assert!(error.as_deref().is_some_and(|e| e.contains("first")), "{error:?}"); + steps(&mut h, &[json!({"click": {"world": [25, 15, 20]}})]); + h.frames(2); + assert_eq!(shown(&mut h), None); + } + + /// A parameter the depth names changes the design, not the dialog's commands. + #[test] + fn a_parameter_the_inputs_use_clears_it() { + let mut h = hole_dialog(); + let add = json!({"command": "parameters.add", "params": {"name": "dep", "expression": "-5 mm"}}); + steps(&mut h, &[json!({"call": "engine.execute", "params": add})]); + set_depth(&mut h, "dep"); + steps(&mut h, &[json!({"key": "Enter"})]); + assert!(h.app.dialog.as_ref().and_then(|d| d.error.as_deref()).is_some_and(|e| e.contains("depth"))); + let change = json!({"command": "parameters.change", "params": {"name": "dep", "expression": "5 mm"}}); + steps(&mut h, &[json!({"call": "engine.execute", "params": change})]); + h.frames(2); + assert_eq!(shown(&mut h), None); + } + + /// Picks changed some other way than a click (a box selection, a cleared chip) are seen too, + /// and the dialog then says what the inputs lack now. + #[test] + fn a_pick_changed_outside_the_dialog_shows_what_is_missing_now() { + let mut h = hole_dialog(); + refused(&mut h); + if let Some(input) = h.app.dialog.as_mut().and_then(|d| d.inputs.first_mut()) { + input.items.clear(); + } + h.frames(2); + let error = h.app.dialog.as_ref().and_then(|d| d.error.clone()); + assert!(error.as_deref().is_some_and(|e| e.contains("first")), "{error:?}"); + } + + /// Editing an existing hole: the refusal clears, and OK then edits that hole. + #[test] + fn editing_a_hole_clears_it_and_keeps_the_feature() { + let mut h = Harness::new(); + let hole = json!({"command": "solid.hole", "params": {"position": [25, 15, 20], "diameter": "5 mm", "depth": "10 mm", "name": "Bore"}}); + steps( + &mut h, + &[ + json!({"call": "engine.execute", "params": {"command": "solid.box", "params": {"length": 50, "width": 30, "height": 20}}}), + json!({"call": "engine.execute", "params": hole}), + json!({"call": "ui.view", "params": {"view": "iso"}}), + json!({"call": "ui.editFeature", "params": {"feature": "Bore"}}), + ], + ); + let features = h.app.session.doc.features.len(); + refused(&mut h); + assert!(h.app.dialog.as_ref().is_some_and(|d| d.editing.is_some())); + set_depth(&mut h, "5"); + h.frames(2); + assert_eq!(shown(&mut h), None); + steps(&mut h, &[json!({"key": "Enter"})]); + assert!(h.app.dialog.is_none()); + assert_eq!(h.app.session.doc.features.len(), features); + let bore = h.app.session.execute("timeline.inspect", &json!({"feature": "Bore"})).unwrap(); + assert!(bore.to_string().contains("\"depth\":\"5\""), "{bore}"); + } +} diff --git a/crates/ui-egui/tests/scenarios/hole_error_clears_when_depth_is_fixed.json b/crates/ui-egui/tests/scenarios/hole_error_clears_when_depth_is_fixed.json new file mode 100644 index 00000000..9cf9ae5d --- /dev/null +++ b/crates/ui-egui/tests/scenarios/hole_error_clears_when_depth_is_fixed.json @@ -0,0 +1,18 @@ +[ + {"note": "#641: a refused OK's error must not outlive the inputs it was about. A negative depth is refused; once the depth is positive again the dialog shows no error and OK makes the hole (Ø5, 5 deep to the shoulder, 118° drill point: 30000 − 98.17 − 9.83 mm³)."}, + {"call": "engine.execute", "params": {"command": "solid.box", "params": {"length": 50, "width": 30, "height": 20}}}, + {"call": "ui.view", "params": {"view": "iso"}}, + {"start": "solid.hole"}, + {"click": {"world": [25, 15, 20]}}, + {"click": {"handle": "dialog:Depth"}}, + {"key": "A", "cmd": true}, + {"text": "-5"}, + {"key": "Enter"}, + {"expect": {"dialog": "error: Some(\"hole depth must be positive\")"}}, + {"click": {"handle": "dialog:Depth"}}, + {"key": "A", "cmd": true}, + {"text": "5"}, + {"expect": {"dialog": "error: None"}}, + {"key": "Enter"}, + {"expect": {"bodies": 1, "errors": 0, "dialog": null, "volume": [29891.99, 1e-4]}} +]