Visualization Fixes (enso-org/ide#1804)

Original commit: enso-org/ide@838788a

ide/CHANGELOG.md

@@ -6,6 +6,8 @@
 
 #### Enso Compiler
 
+- [Improvements to visualization handling][1804]. These improvements are fixing
+  possible performance issues around attaching and detaching visualizations.
 - [GeoMap visualization will ignore points with `null` coordinates][1775]. Now
   the presence of such points in the dataset will not break initial map
   positioning.
@@ -15,6 +17,7 @@
 
 [1775]: https://github.com/enso-org/ide/pull/1775
 [1798]: https://github.com/enso-org/ide/pull/1798
+[1804]: https://github.com/enso-org/ide/pull/1804
 
 # Enso 2.0.0-alpha.11 (2021-08-09)
 

ide/src/rust/ensogl/lib/core/src/display/render/pass.rs

@@ -90,7 +90,7 @@ impl Instance {
         }
         context.draw_buffers(&draw_buffers);
         context.bind_framebuffer(target,None);
-        Framebuffer {native,context}
+        Framebuffer {context,native}
     }
 }
 

ide/src/rust/ensogl/lib/core/src/display/scene.rs

@@ -674,8 +674,8 @@ impl HardcodedLayers {
              , &tooltip_text
              , &cursor
              ]);
-        Self {viz,below_main,main,port_selection,label,above_nodes,above_nodes_text,panel,panel_text
-             ,node_searcher,node_searcher_mask,tooltip,tooltip_text,cursor,root,mask}
+        Self {root,viz,below_main,main,port_selection,label,above_nodes,above_nodes_text,panel
+             ,panel_text,node_searcher,node_searcher_mask,tooltip,tooltip_text,cursor,mask}
     }
 }
 

ide/src/rust/ide/lib/enso-protocol/src/language_server.rs

@@ -7,6 +7,7 @@
 //! This file tries to follow the scheme of the protocol specification.
 
 pub mod connection;
+pub mod constants;
 pub mod response;
 #[cfg(test)]
 mod tests;
@@ -162,3 +163,57 @@ trait API {
     , tags        : Option<Vec<SuggestionEntryType>>
     ) -> response::Completion;
 }}
+
+
+
+// ==============
+// === Errors ===
+// ==============
+
+/// Check if the given `Error` value corresponds to an RPC call timeout.
+/// 
+/// Recognizes both client- and server-side timeouts.
+pub fn is_timeout_error(error:&failure::Error) -> bool {
+    use json_rpc::messages;
+    use json_rpc::RpcError;
+    use json_rpc::RpcError::*;
+    const  TIMEOUT:i64 = constants::ErrorCodes::Timeout as i64;
+    matches!(error.downcast_ref::<RpcError>()
+      , Some(TimeoutError{..})
+      | Some(RemoteError(messages::Error{code:TIMEOUT,..})))
+}
+
+
+
+// =============
+// === Tests ===
+// =============
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn recognize_timeout_errors() {
+        type RpcError = json_rpc::RpcError::<serde_json::Value>;
+
+        // Server-side errors.
+        let text  = r#"{"code":11,"message":"Request timeout"}"#;
+        let msg   = serde_json::from_str::<json_rpc::messages::Error>(text).unwrap();
+        let error = RpcError::RemoteError(msg).into();
+        assert!(is_timeout_error(&error));
+
+        let text  = r#"{"code":2007,"message":"Evaluation of the visualisation expression failed"}"#;
+        let msg   = serde_json::from_str::<json_rpc::messages::Error>(text).unwrap();
+        let error = RpcError::RemoteError(msg).into();
+        assert!(!is_timeout_error(&error));
+
+
+        // Client-side errors.
+        let error = RpcError::TimeoutError {millis:500}.into();
+        assert!(is_timeout_error(&error));
+
+        let error = RpcError::LostConnection.into();
+        assert!(!is_timeout_error(&error));
+    }
+}

ide/src/rust/ide/lib/enso-protocol/src/language_server/constants.rs

@@ -0,0 +1,25 @@
+//! Constants defined for the Language Server JSON-RPC API.
+
+use crate::prelude::*;
+
+/// Recognized error codes used by the Language Server messages.
+/// 
+/// They follow `org.enso.jsonrpc.Error` object defined in the `enso` repository.
+#[derive(Clone,Copy,Debug)]
+pub enum ErrorCodes {
+    /// Server failed to parse JSON message.
+    ParseError = -32700,
+    /// JSON message sent was not a valid Request object.
+    InvalidRequest = -32600,
+    /// Requested method does not exist or is unavailable.
+    MethodNotFound = -32601,
+    /// Invalid method parameters.
+    InvalidParams = -32602,
+    /// Service error.
+    ServiceError = 1,
+    /// The requested method is not implemented.
+    NotImplementedError = 10,
+    /// Request timeout.
+    /// Note that timeout can also happen on the client side, as part of the Handler's logic.
+    Timeout = 11,
+}

ide/src/rust/ide/lib/enso-protocol/src/language_server/tests.rs

@@ -512,7 +512,7 @@ fn test_execution_context() {
     let old_version = Sha3_224::new(b"Hello world!");
     let new_version = Sha3_224::new(b"Hello, world!");
     let path        = main.clone();
-    let edit        = FileEdit {path,edits,old_version,new_version:new_version};
+    let edit        = FileEdit {path,edits,old_version,new_version};
     test_request(
         |client| client.apply_text_file_edit(&edit),
         "text/applyEdit",

ide/src/rust/ide/lib/json-rpc/src/error.rs

@@ -34,6 +34,9 @@ pub enum RpcError<Payload:Debug+Send+Sync+'static = serde_json::Value> {
     MismatchedResponseType,
 
     /// Response timeout.
+    /// 
+    /// Note that this represents the client-side timeout. Server-side timeout will be treated as 
+    /// an [`RemoteError`]. 
     #[allow(missing_docs)]
     #[fail(display = "Response timed out after {} ms.", millis)]
     TimeoutError{millis:u128},

ide/src/rust/ide/lib/json-rpc/src/lib.rs

@@ -24,10 +24,11 @@ pub use api::RemoteMethodCall;
 pub use api::Result;
 pub use enso_prelude as prelude;
 pub use ensogl_system_web as ensogl;
-pub use transport::Transport;
-pub use transport::TransportEvent;
+pub use error::RpcError;
 pub use handler::Event;
 pub use handler::Handler;
+pub use transport::Transport;
+pub use transport::TransportEvent;
 
 #[cfg(test)] pub use utils::test::traits::*;
 

ide/src/rust/ide/src/constants.rs

@@ -22,11 +22,12 @@ pub const PROJECTS_MAIN_MODULE:&str = "Main";
 /// Visualization folder where IDE can look for user-defined visualizations per project.
 pub const VISUALIZATION_DIRECTORY:&str = "visualization";
 
-/// How many times IDE will try to attach initial visualization on loading project.
-///
-/// This is necessary, because the request will be timing out for a while, until the stdlib
-/// compilation is done.
-pub const INITIAL_VISUALIZATION_ATTACH_ATTEMPTS:usize = 50;
+/// How many times IDE will try attaching visualization when there is a timeout error.
+/// 
+/// Timeout error suggests that there might be nothing wrong with the request, just that the backend
+/// is currently too busy to reply or that there is some connectivity hiccup. Thus, it makes sense
+/// to give it a few more tries.
+pub const ATTACHING_TIMEOUT_RETRIES:usize = 50;
 
 /// A module with language-specific constants.
 pub mod keywords {

ide/src/rust/ide/src/ide/integration/project.rs

@@ -1733,11 +1733,11 @@ impl Model {
         executor::global::spawn(task);
         Ok(id)
     }
-
-
-    /// Repeatedly try to attach visualization to the node (as defined by the map).
-    ///
-    /// Up to `attempts` will be made.
+    
+    /// Try attaching visualization to the node.
+    /// 
+    /// In case of timeout failure, retries up to total `attempts` count will be made. 
+    /// For other kind of errors no further attempts will be made.
     fn try_attaching_visualization_task
     (&self, node_id:graph_editor::NodeId, visualizations_map:VisualizationMap, attempts:usize)
     -> impl Future<Output=AttachingResult<impl Stream<Item=VisualizationUpdateData>>> {
@@ -1756,11 +1756,16 @@ impl Model {
                             {node_id}.");
                             return AttachingResult::Attached(stream);
                         }
-                        Err(e) => {
-                            error!(logger, "Failed to attach visualization {id} for node {node_id} \
-                            (attempt {i}): {e}");
+                        Err(e) if enso_protocol::language_server::is_timeout_error(&e) => {
+                            warning!(logger, "Failed to attach visualization {id} for node \
+                            {node_id} (attempt {i}). Will retry, as it is a timeout error.");
                             last_error = Some(e);
                         }
+                        Err(e) => {
+                            warning!(logger, "Failed to attach visualization {id} for node \
+                            {node_id}: {e}");
+                            return AttachingResult::Failed(e)
+                        }
                     }
                 } else {
                     // If visualization is not present in the map, it means that UI detached it
@@ -1789,7 +1794,7 @@ impl Model {
     ) -> impl Future<Output=()> {
         let graph_frp  = self.view.graph().frp.clone_ref();
         let map        = visualizations_map.clone();
-        let attempts   = crate::constants::INITIAL_VISUALIZATION_ATTACH_ATTEMPTS;
+        let attempts   = crate::constants::ATTACHING_TIMEOUT_RETRIES;
         let stream_fut = self.try_attaching_visualization_task(node_id,map,attempts);
         async move {
             // No need to log anything here, as `try_attaching_visualization_task` does this.

ide/src/rust/ide/view/graph-editor/src/lib.rs

@@ -1270,8 +1270,7 @@ impl GraphEditorModelWithNetwork {
             // new one has been enabled.
             // TODO: Create a better API for updating the controller about visualisation changes
             // (see #896)
-            visualization_hidden_changed       <- visualization_hidden.on_change();
-            output.source.visualization_hidden <+ visualization_hidden_changed.constant(node_id);
+            output.source.visualization_hidden <+ visualization_hidden.constant(node_id);
             output.source.visualization_shown  <+
                 visualization_shown.map2(&metadata,move |_,metadata| (node_id,metadata.clone()));