Error Visualizations #1215
Error Visualizations #1215
Conversation
farmaazon
added
Category: Controllers
src/rust/ensogl/lib/theme/src/lib.rs
Outdated
| dataflow.color = Rgba(1.0,0.647,0.0,0.7), Rgba(1.0,0.647,0.0,0.7); | ||
| panic.color = Rgba(1.0,0.27,0.0,0.7) , Rgba(1.0,0.27,0.0,0.7); |
There was a problem hiding this comment.
We are not using .color anywhere else. This is a thee definition, colors are the most basic setting here. We should skip .color imo
src/rust/ide/src/ide/integration.rs
Outdated
| let endpoint = self.view.graph().frp.set_error_visualization_data.clone_ref(); | ||
| let preprocessor = graph_editor::builtin::visualization::native::error::PREPROCESSOR; | ||
| let preprocessor = Some(graph_editor::data::enso::Code::new(preprocessor)); | ||
| let metadata = Some(visualization::Metadata {preprocessor}); |
src/rust/ide/src/ide/integration.rs
Outdated
| None | | ||
| Some(Value ) => None, | ||
| Some(DataflowError { trace }) => Some((Kind::Dataflow, None ,trace)), | ||
| Some(Panic { message,trace }) => Some((Kind::Panic , Some(message),trace)), | ||
| }?; |
src/rust/ide/src/ide/integration.rs
Outdated
| let err = || VisualizationAlreadyAttached(node_id); | ||
| (!visualizations_map.contains_key(&node_id)).ok_or_else(err)?; | ||
|
|
||
| debug!(self.logger, "Attaching visualization on {node_id}."); |
There was a problem hiding this comment.
is node_id just a number? if so, I'd use here "Attaching visualization on node {node_id}." instead.
src/rust/ide/src/ide/integration.rs
Outdated
| , endpoint : frp::Any<(graph_editor::NodeId,visualization::Data)> | ||
| , node_id : graph_editor::NodeId | ||
| ) -> impl FnMut(VisualizationUpdateData) -> futures::future::Ready<()> { |
src/rust/ide/src/ide/integration.rs
Outdated
| let metadata_expression = metadata.and_then(|m| m.preprocessor.as_ref().map(|e| e.to_string())); | ||
| let default_expression = crate::constants::SERIALIZE_TO_JSON_EXPRESSION; | ||
| let expression = metadata_expression.unwrap_or_else(||default_expression.into()); | ||
| let ast_id = self.get_controller_node_id(node_id)?; | ||
| Ok(Visualization{ast_id,expression,id,visualisation_module}) | ||
| } | ||
|
|
||
| fn detach_visualization | ||
| ( &self | ||
| , node_id : graph_editor::NodeId | ||
| , visualizations_map : SharedHashMap<graph_editor::NodeId,VisualizationId> | ||
| ) -> FallibleResult { | ||
| debug!(self.logger,"Node editor wants to detach visualization on {node_id}."); | ||
| let err = || NoSuchVisualization(node_id); |
There was a problem hiding this comment.
We have 4 lambdas here and they are using or not using space after pipes. This should be unified in the codebase. I'd go with no space. If you agree, please add it to styling docs too.
There was a problem hiding this comment.
Well, we put space everywhere, and it makes sense to me: consider |foo,x,y|foo.blah(x,y), here separating arguments and body would be helpful
|
|
||
| eval frp.set_visualization ((t) model.visualization.frp.set_visualization.emit(t)); | ||
| visualization_enabled <- bool(&frp.disable_visualization,&frp.enable_visualization); | ||
| visualization_visible <- visualization_enabled.all_with(&is_error_set, |l,r| *l && !*r); |
There was a problem hiding this comment.
use visualization_visible <- visualization_enabled && ! is_error_set; instead :) I'm not sure whether we support ! in macros, but if we don't, add it there please. We support && for sure.
| }; | ||
| style.get_color(path) | ||
| } else { | ||
| color::Lcha::transparent() |
There was a problem hiding this comment.
Does it ean that we color error messages as transparent instead of hiding them using display object functions when errors are not provided? If so, we should hide errors by upsetting parents, so we are rendering less things then.
There was a problem hiding this comment.
The transparent color is for animations. But I added code for hinding when animation is complete.
| /// visualization won't work (e.g. in case of panics). | ||
| pub message : Rc<Option<String>>, | ||
| /// Flag indicating that the error is propagated from another node visible on the scene. | ||
| pub propagated : Immutable<bool>, |
There was a problem hiding this comment.
I think you dont need Imutable here
There was a problem hiding this comment.
I wanted it to be clone_ref-able, because this structure is heavily propagated trough the FRP network. And deriving CloneRef requires wrapping in immutable.
Original commit: enso-org/ide@76968f2
Pull Request Description
Important Notes
Cannot be merged before new Engine's release 0.2.4!
Checklist
Please include the following checklist in your PR:
CHANGELOG.mdwas updated with the changes introduced in this PR.