From 89dc7a57266281469d77cc4289ba464042c531a8 Mon Sep 17 00:00:00 2001 From: Ilya Bogdanov Date: Thu, 2 Feb 2023 21:03:46 +0400 Subject: [PATCH] Mark nodes with yellow stripes if their evaluation produced warnings (#4101) [Task](https://www.pivotaltracker.com/story/show/184237388) Nodes with warnings are highlighted with yellow stripes. Colors for dataflow errors and panics also changed. https://user-images.githubusercontent.com/6566674/216285009-9d29eb84-f663-4f2e-a6c8-4c4f593bd1ef.mp4 --- .../src/language_server/tests.rs | 4 +-- .../src/language_server/types.rs | 22 +++++++++++++--- app/gui/src/model/execution_context.rs | 12 ++++++--- app/gui/src/presenter/graph/state.rs | 25 +++++++++++-------- app/gui/view/debug_scene/interface/src/lib.rs | 8 ++++++ .../src/builtin/visualization/native/error.rs | 7 +++--- .../view/graph-editor/src/component/node.rs | 9 +++++-- .../graph-editor/src/component/node/error.rs | 6 +++++ .../ensogl/app/theme/hardcoded/src/lib.rs | 10 +++++--- 9 files changed, 76 insertions(+), 27 deletions(-) diff --git a/app/gui/controller/engine-protocol/src/language_server/tests.rs b/app/gui/controller/engine-protocol/src/language_server/tests.rs index a7009ffcaf..94d216eddf 100644 --- a/app/gui/controller/engine-protocol/src/language_server/tests.rs +++ b/app/gui/controller/engine-protocol/src/language_server/tests.rs @@ -329,7 +329,7 @@ fn test_computed_value_update() { "methodPointer" : null, "profilingInfo" : [], "fromCache" : true, - "payload" : ExpressionUpdatePayload::Value, + "payload" : ExpressionUpdatePayload::Value { warnings: None }, }] } }); @@ -350,7 +350,7 @@ fn test_computed_value_update() { assert_eq!(update.typename.as_deref(), Some(typename)); assert!(update.method_pointer.is_none()); assert!(update.from_cache); - assert!(matches!(update.payload, ExpressionUpdatePayload::Value)) + assert!(matches!(update.payload, ExpressionUpdatePayload::Value { warnings: None })) } _ => panic!("Expected Notification::ExpressionUpdates"), } diff --git a/app/gui/controller/engine-protocol/src/language_server/types.rs b/app/gui/controller/engine-protocol/src/language_server/types.rs index efb1b729e0..fc8095dff5 100644 --- a/app/gui/controller/engine-protocol/src/language_server/types.rs +++ b/app/gui/controller/engine-protocol/src/language_server/types.rs @@ -232,7 +232,9 @@ pub enum ProfilingInfo { #[allow(missing_docs)] #[serde(tag = "type")] pub enum ExpressionUpdatePayload { - Value, + Value { + warnings: Option, + }, #[serde(rename_all = "camelCase")] DataflowError { trace: Vec, @@ -249,6 +251,20 @@ pub enum ExpressionUpdatePayload { }, } +/// Information about warnings associated with the value. +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[allow(missing_docs)] +pub struct Warnings { + /// The number of attached warnings. + pub count: usize, + /// If the value has a single warning attached, this field contains textual representation of + /// gg + /// + /// the attached warning. In general, warning values should be obtained by attaching an + /// appropriate visualization to a value. + pub value: Option, +} + // ======================= @@ -1226,7 +1242,7 @@ pub mod test { method_pointer: None, profiling_info: default(), from_cache: false, - payload: ExpressionUpdatePayload::Value, + payload: ExpressionUpdatePayload::Value { warnings: None }, } } @@ -1242,7 +1258,7 @@ pub mod test { method_pointer: Some(method_pointer), profiling_info: default(), from_cache: false, - payload: ExpressionUpdatePayload::Value, + payload: ExpressionUpdatePayload::Value { warnings: None }, } } diff --git a/app/gui/src/model/execution_context.rs b/app/gui/src/model/execution_context.rs index 218a825c31..4d740e9292 100644 --- a/app/gui/src/model/execution_context.rs +++ b/app/gui/src/model/execution_context.rs @@ -580,9 +580,13 @@ mod tests { let update2 = value_update_with_type(expr2, &typename2); registry.apply_updates(vec![update1, update2]); assert_eq!(registry.get(&expr1).unwrap().typename, Some(typename1.clone().into())); - assert!(matches!(registry.get(&expr1).unwrap().payload, ExpressionUpdatePayload::Value)); + assert!(matches!(registry.get(&expr1).unwrap().payload, ExpressionUpdatePayload::Value { + warnings: None, + })); assert_eq!(registry.get(&expr2).unwrap().typename, Some(typename2.into())); - assert!(matches!(registry.get(&expr2).unwrap().payload, ExpressionUpdatePayload::Value)); + assert!(matches!(registry.get(&expr2).unwrap().payload, ExpressionUpdatePayload::Value { + warnings: None, + })); let notification = test.expect_completion(subscriber.next()).unwrap(); assert_eq!(notification, vec![expr1, expr2]); @@ -591,7 +595,9 @@ mod tests { let update2 = value_update_with_dataflow_panic(expr3, error_msg); registry.apply_updates(vec![update1, update2]); assert_eq!(registry.get(&expr1).unwrap().typename, Some(typename1.into())); - assert!(matches!(registry.get(&expr1).unwrap().payload, ExpressionUpdatePayload::Value)); + assert!(matches!(registry.get(&expr1).unwrap().payload, ExpressionUpdatePayload::Value { + warnings: None, + })); assert!(registry.get(&expr2).unwrap().typename.is_none()); assert!(matches!( registry.get(&expr2).unwrap().payload, diff --git a/app/gui/src/presenter/graph/state.rs b/app/gui/src/presenter/graph/state.rs index 97ac0d0dd0..41d47de709 100644 --- a/app/gui/src/presenter/graph/state.rs +++ b/app/gui/src/presenter/graph/state.rs @@ -528,20 +528,25 @@ impl<'a> ControllerChange<'a> { ) -> Option { use node_view::error::Kind; use ExpressionUpdatePayload::*; - let (kind, message, trace) = match payload { - None | Some(Value) => None, - Some(DataflowError { trace }) => Some((Kind::Dataflow, None, trace)), - Some(Panic { message, trace }) => Some((Kind::Panic, Some(message), trace)), - Some(Pending { .. }) => None, - }?; - let propagated = if kind == Kind::Panic { + let is_propagated = |trace: Vec| { let nodes = self.nodes.borrow(); let root_cause = trace.iter().find(|id| nodes.get(**id).is_some()); !root_cause.contains(&&node_id) - } else { - // TODO[ao]: traces are not available for Dataflow errors. - false }; + let (kind, message, propagated) = match payload { + Some(Value { warnings: Some(warnings) }) if warnings.count > 0 => { + // We return `None` as message, even though we have a warning text available. We + // don't want to replace the visualization of the value with a warning text though. + Some((Kind::Warning, None, false)) + } + Some(DataflowError { trace }) => Some((Kind::Dataflow, None, is_propagated(trace))), + Some(Panic { message, trace }) => { + let message = Some(message); + let is_propagated = is_propagated(trace); + Some((Kind::Panic, message, is_propagated)) + } + _ => None, + }?; let kind = Immutable(kind); let message = Rc::new(message); diff --git a/app/gui/view/debug_scene/interface/src/lib.rs b/app/gui/view/debug_scene/interface/src/lib.rs index a968072a1e..8624775133 100644 --- a/app/gui/view/debug_scene/interface/src/lib.rs +++ b/app/gui/view/debug_scene/interface/src/lib.rs @@ -155,9 +155,17 @@ fn init(app: &Application) { let foo_node = graph_editor.model.add_node_below(node3_id); graph_editor.set_node_expression.emit((foo_node, Expression::new_plain("foo"))); + let kind = Immutable(graph_editor::component::node::error::Kind::Dataflow); + let message = Rc::new(Some("Dataflow Error".to_owned())); + let error = graph_editor::component::node::Error { kind, message, propagated }; + graph_editor.frp.set_node_error_status.emit((foo_node, Some(error))); let baz_node = graph_editor.model.add_node_below(node3_id); graph_editor.set_node_expression.emit((baz_node, Expression::new_plain("baz"))); + let kind = Immutable(graph_editor::component::node::error::Kind::Warning); + let message = Rc::new(Some("Warning".to_owned())); + let error = graph_editor::component::node::Error { kind, message, propagated }; + graph_editor.frp.set_node_error_status.emit((baz_node, Some(error))); let (_, baz_position) = graph_editor.node_position_set.value(); let styles = StyleWatch::new(&scene.style_sheet); let min_spacing = styles.get_number(theme::graph_editor::minimal_x_spacing_for_new_nodes); diff --git a/app/gui/view/graph-editor/src/builtin/visualization/native/error.rs b/app/gui/view/graph-editor/src/builtin/visualization/native/error.rs index 94c0be8f94..688bf534b0 100644 --- a/app/gui/view/graph-editor/src/builtin/visualization/native/error.rs +++ b/app/gui/view/graph-editor/src/builtin/visualization/native/error.rs @@ -231,10 +231,11 @@ impl Model { } fn display_kind(&self, new: Kind) { + use ensogl_hardcoded_theme::graph_editor::visualization::error as theme; let color_style = match new { - Kind::Panic => ensogl_hardcoded_theme::graph_editor::visualization::error::panic::text, - Kind::Dataflow => - ensogl_hardcoded_theme::graph_editor::visualization::error::dataflow::text, + Kind::Panic => theme::panic::text, + Kind::Dataflow => theme::dataflow::text, + Kind::Warning => theme::warning::text, }; let default = ""; let opt_message = self.messages.get_cloned_ref(&new); diff --git a/app/gui/view/graph-editor/src/component/node.rs b/app/gui/view/graph-editor/src/component/node.rs index 70ace376b4..f636690763 100644 --- a/app/gui/view/graph-editor/src/component/node.rs +++ b/app/gui/view/graph-editor/src/component/node.rs @@ -664,7 +664,9 @@ impl NodeModel { if let Some(error_data) = error.visualization_data() { self.error_visualization.set_data(&error_data); } - self.display_object.add_child(&self.error_visualization); + if error.should_display() { + self.display_object.add_child(&self.error_visualization); + } } else { self.error_visualization.unset_parent(); } @@ -809,7 +811,9 @@ impl Node { frp::extend! { network out.error <+ input.set_error; - is_error_set <- input.set_error.map(|err| err.is_some()); + is_error_set <- input.set_error.map( + |err| err.as_ref().map_or(false, Error::should_display) + ); no_error_set <- not(&is_error_set); error_color_anim.target <+ all_with(&input.set_error,&input.set_view_mode, f!([style](error,&mode) @@ -1006,6 +1010,7 @@ impl Node { let path = match *error.kind { error::Kind::Panic => error_theme::panic, error::Kind::Dataflow => error_theme::dataflow, + error::Kind::Warning => error_theme::warning, }; style.get_color(path).into() } else { diff --git a/app/gui/view/graph-editor/src/component/node/error.rs b/app/gui/view/graph-editor/src/component/node/error.rs index a004d5f5f4..b889db47a9 100644 --- a/app/gui/view/graph-editor/src/component/node/error.rs +++ b/app/gui/view/graph-editor/src/component/node/error.rs @@ -28,6 +28,7 @@ use serde::Serialize; pub enum Kind { Panic, Dataflow, + Warning, } /// Additional error information (beside the error value itself) for some erroneous node. @@ -51,6 +52,11 @@ impl Error { message: self.message.as_ref().as_ref()?.clone(), }) } + + /// Whether we should display the error in a special error visualization attached to the node. + pub fn should_display(&self) -> bool { + !matches!(*self.kind, Kind::Warning) + } } diff --git a/lib/rust/ensogl/app/theme/hardcoded/src/lib.rs b/lib/rust/ensogl/app/theme/hardcoded/src/lib.rs index b3b983f5a2..0f68f3a613 100644 --- a/lib/rust/ensogl/app/theme/hardcoded/src/lib.rs +++ b/lib/rust/ensogl/app/theme/hardcoded/src/lib.rs @@ -541,8 +541,9 @@ define_themes! { [light:0, dark:1] edited = Lcha::yellow(0.9,1.0), Lcha::yellow(0.9,1.0); } error { - dataflow = Rgba(1.0,0.655,0.141,1.0), Rgba(1.0,0.655,0.141,1.0); - panic = Rgba(1.0,0.341,0.125,1.0), Rgba(1.0,0.341,0.125,1.0); + dataflow = Rgba(1.0,0.341,0.125,1.0), Rgba(1.0,0.341,0.125,1.0); + panic = Rgba(0.7,0.235,0.08,1.0), Rgba(0.7,0.235,0.08,1.0); + warning = Rgba(1.0,0.655,0.141,1.0), Rgba(1.0,0.655,0.141,1.0); width = 4.0 , 4.0; repeat_x = 20.0 , 20.0; repeat_y = 20.0 , 20.0; @@ -564,8 +565,9 @@ define_themes! { [light:0, dark:1] text = Lcha(0.0,0.0,0.0,0.7) , Lcha(1.0,0.0,0.0,0.7); text.selection = Lcha(0.7,0.0,0.125,0.7) , Lcha(0.7,0.0,0.125,0.7); error { - dataflow.text = Rgba(1.0,0.655,0.141,1.0), Rgba(1.0,0.655,0.141,1.0); - panic.text = Rgba(1.0,0.341,0.125,1.0), Rgba(1.0,0.341,0.125,1.0); + dataflow.text = Rgba(1.0,0.341,0.125,1.0), Rgba(1.0,0.341,0.125,1.0); + panic.text = Rgba(0.7,0.235,0.08,1.0), Rgba(0.7,0.235,0.08,1.0); + warning.text = Rgba(1.0,0.655,0.141,1.0), Rgba(1.0,0.655,0.141,1.0); } action_bar { // Original RGB values (for reference after fixing color-conversion issues)