From 731629a4229a664a830873c4da26669c18dcec2c Mon Sep 17 00:00:00 2001 From: CamWight <91156642+CamWight@users.noreply.github.com> Date: Wed, 14 Oct 2026 22:36:02 +0100 Subject: [PATCH] A failed feature has a mark of its own in the timeline and the Feature History (#972) (#1180) A failed feature gets its own mark in the timeline and Feature History, distinct from warnings, with tests. --- assets/icons/ui/source/icons.rs | 7 ++ crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/timeline.rs | 4 + crates/ui-egui/src/timeline_left.rs | 5 + crates/ui-egui/src/timeline_marks_tests.rs | 107 +++++++++++++++++++++ 5 files changed, 125 insertions(+) create mode 100644 crates/ui-egui/src/timeline_marks_tests.rs diff --git a/assets/icons/ui/source/icons.rs b/assets/icons/ui/source/icons.rs index f82c9c5d..1d05483e 100644 --- a/assets/icons/ui/source/icons.rs +++ b/assets/icons/ui/source/icons.rs @@ -1363,6 +1363,13 @@ pub(crate) fn paint_with(p: &Painter, r: Rect, name: &str, ink: Color32, fill: C pen.line(&[(12.0, 9.0), (12.0, 14.0)], ink); pen.dot(12.0, 17.0, ink); } + // A failed feature: a disc with a cross, a different shape from the warning's triangle, so + // it reads without colour (#972). Drawn in the caller's colours only. + "error" => { + pen.circle(12.0, 12.0, 10.0, pen.fill, ink); + pen.line(&[(8.0, 8.0), (16.0, 16.0)], ink); + pen.line(&[(16.0, 8.0), (8.0, 16.0)], ink); + } _ => { pen.poly(&[(5.0, 5.0), (19.0, 5.0), (19.0, 19.0), (5.0, 19.0)], pen.fill, ink); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 7421b0ae..1a56df09 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -59,6 +59,8 @@ mod menu_tests; #[cfg(test)] mod surface_clamp_tests; #[cfg(test)] +mod timeline_marks_tests; +#[cfg(test)] mod status_tests { #[test] fn the_status_bar_names_commands_by_label() { diff --git a/crates/ui-egui/src/timeline.rs b/crates/ui-egui/src/timeline.rs index 2127a5c3..65dc6a17 100644 --- a/crates/ui-egui/src/timeline.rs +++ b/crates/ui-egui/src/timeline.rs @@ -242,6 +242,10 @@ pub fn timeline(app: &mut SolveApp, ui: &mut egui::Ui) { if *suppressed { p.line_segment([br.left_bottom() + vec2(4.0, -4.0), br.right_top() + vec2(-4.0, 4.0)], Stroke::new(1.5, t.text_dim)); } + // A failed feature has a mark of its own, not only its colour (#972). + if err.is_some() { + icons::paint(&p, Rect::from_min_size(br.right_top() - vec2(10.0, 0.0), vec2(10.0, 10.0)), "error", t.panel, t.error, t.error); + } if warn.is_some() && err.is_none() { icons::paint( &p, diff --git a/crates/ui-egui/src/timeline_left.rs b/crates/ui-egui/src/timeline_left.rs index dba193b7..4a1568c6 100644 --- a/crates/ui-egui/src/timeline_left.rs +++ b/crates/ui-egui/src/timeline_left.rs @@ -719,6 +719,11 @@ fn vertical_features(app: &mut SolveApp, ui: &mut egui::Ui) { egui::FontId::proportional(12.0), text_colour, ); + // A failed feature has a mark of its own, not only its colour (#972). + if error.is_some() { + let at = Rect::from_min_size(pos2(rect.right() - 15.0, rect.center().y - 6.0), vec2(12.0, 12.0)); + icons::paint(ui.painter(), at, "error", t.panel, t.error, t.error); + } if warning.is_some() && error.is_none() { icons::paint( ui.painter(), diff --git a/crates/ui-egui/src/timeline_marks_tests.rs b/crates/ui-egui/src/timeline_marks_tests.rs new file mode 100644 index 00000000..906b586b --- /dev/null +++ b/crates/ui-egui/src/timeline_marks_tests.rs @@ -0,0 +1,107 @@ +//! A failed feature is marked in the bottom timeline and the Feature History by a shape, not only +//! its colour (#972): the error mark (a disc with a cross) where a warning's mark goes. + +use egui::{Pos2, Rect, Shape, vec2}; +use serde_json::json; + +use crate::SolveApp; + +/// Every shape in `s`, the parts of a list of shapes too. +fn each(s: &Shape, f: &mut impl FnMut(&Shape)) { + match s { + Shape::Vec(v) => v.iter().for_each(|s| each(s, f)), + s => f(s), + } +} + +/// The sample with Fillet1 failing (the plate made narrower than its fillets). +fn failing_sample() -> SolveApp { + let mut s = solidcraft_engine::Session::default(); + s.run_script(&solidcraft_engine::sample::script()).unwrap(); + s.execute("parameters.change", &json!({"name": "width", "expression": "5 mm"})).unwrap(); + SolveApp::new(s, crate::Services::default()) +} + +/// Each feature's name and whether it failed. +fn features(app: &SolveApp) -> Vec<(String, bool)> { + app.session.doc.features.iter().map(|f| (f.name.clone(), app.session.model.result(f.id).is_some_and(|r| r.error.is_some()))).collect() +} + +/// The discs filled in the error colour (either theme's) that one frame of `draw` paints. +fn error_discs(app: &mut SolveApp, draw: fn(&mut SolveApp, &mut egui::Ui)) -> Vec { + crate::scenario::clear_handles(); + let ctx = egui::Context::default(); + let screen = Rect::from_min_size(Pos2::ZERO, vec2(1400.0, 900.0)); + let out = ctx.run_ui(egui::RawInput { screen_rect: Some(screen), ..Default::default() }, |ui| draw(app, ui)); + let errors = [crate::theme::Tokens::light().error, crate::theme::Tokens::dark().error]; + let mut found = Vec::new(); + for clipped in &out.shapes { + each(&clipped.shape, &mut |s| { + if let Shape::Circle(c) = s + && errors.contains(&c.fill) + { + found.push(c.center); + } + }); + } + found +} + +/// The features that have a disc where their mark goes (`at`: the mark's centre from the item's). +fn marked(app: &mut SolveApp, draw: fn(&mut SolveApp, &mut egui::Ui), at: impl Fn(Pos2, Pos2) -> bool) -> Vec<(String, bool)> { + let discs = error_discs(app, draw); + let handles = crate::scenario::handles("timeline:"); + features(app) + .into_iter() + .map(|(name, _)| { + let item = handles.iter().find(|(h, _)| *h == format!("timeline:{name}")).map(|(_, p)| *p); + let item = item.unwrap_or_else(|| panic!("{name} is not drawn")); + let has = discs.iter().any(|d| at(item, *d)); + (name, has) + }) + .collect() +} + +#[test] +fn a_failed_feature_is_marked_in_the_bottom_timeline() { + let mut app = failing_sample(); + let want = features(&app); + assert!(want.iter().any(|(n, failed)| *failed && n == "Fillet1"), "the sample's Fillet1 fails: {want:?}"); + assert!(want.iter().filter(|(_, failed)| !failed).count() >= 3, "and others don't"); + // The mark sits in the item's top-right corner: 10 points square, inside the 28-point item. + let got = marked(&mut app, crate::timeline::timeline, |item, d| (d.x - (item.x + 9.0)).abs() < 1.5 && (d.y - (item.y - 9.0)).abs() < 1.5); + assert_eq!(got, want, "an error mark on each failed feature and on no other"); +} + +#[test] +fn a_failed_feature_is_marked_in_the_feature_history() { + let mut app = failing_sample(); + let want = features(&app); + // The mark sits at the row's right end, on its centre line. + let got = marked(&mut app, crate::timeline_left::left_timeline, |item, d| (d.y - item.y).abs() < 1.5 && d.x > item.x); + assert_eq!(got, want, "an error mark on each failed feature and on no other"); +} + +/// The mark is its own drawing: a disc with a cross in the caller's colours, not the fallback +/// square, and a different shape from the warning's triangle. +#[test] +fn the_error_mark_is_a_disc_with_a_cross() { + let ctx = egui::Context::default(); + let (ink, fill) = (egui::Color32::from_rgb(1, 2, 3), egui::Color32::from_rgb(4, 5, 6)); + let draw = |name: &'static str| { + let out = ctx.run_ui(egui::RawInput::default(), |ui| { + crate::icons::paint(ui.painter(), Rect::from_min_size(Pos2::ZERO, vec2(24.0, 24.0)), name, ink, fill, fill); + }); + let mut shapes = Vec::new(); + for c in &out.shapes { + each(&c.shape, &mut |s| shapes.push(s.clone())); + } + shapes + }; + let error = draw("error"); + let discs = error.iter().filter(|s| matches!(s, Shape::Circle(c) if c.fill == fill && c.stroke.color == ink)).count(); + let strokes = error.iter().filter(|s| matches!(s, Shape::Path(p) if !p.closed && p.stroke.color == egui::epaint::ColorMode::Solid(ink))).count(); + assert_eq!((discs, strokes), (1, 2), "one disc and the two strokes of a cross: {error:?}"); + assert_ne!(format!("{error:?}"), format!("{:?}", draw("warning"))); + assert_ne!(format!("{error:?}"), format!("{:?}", draw("no such icon")), "drawn, not the fallback square"); +}