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
This commit is contained in:
Ilya Bogdanov 2023-02-02 21:03:46 +04:00 committed by GitHub
parent 0a6e58fc99
commit 89dc7a5726
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 76 additions and 27 deletions

View File

@ -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"),
}

View File

@ -232,7 +232,9 @@ pub enum ProfilingInfo {
#[allow(missing_docs)]
#[serde(tag = "type")]
pub enum ExpressionUpdatePayload {
Value,
Value {
warnings: Option<Warnings>,
},
#[serde(rename_all = "camelCase")]
DataflowError {
trace: Vec<ExpressionId>,
@ -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<String>,
}
// =======================
@ -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 },
}
}

View File

@ -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,

View File

@ -528,20 +528,25 @@ impl<'a> ControllerChange<'a> {
) -> Option<node_view::error::Error> {
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<AstNodeId>| {
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);

View File

@ -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);

View File

@ -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);

View File

@ -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 {

View File

@ -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)
}
}

View File

@ -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)