Correct import handling when selecting items from the component brows…

…er #182634001 (#3708)

Implements [Task #182634001 ](https://www.pivotaltracker.com/story/show/182634001)

Handles adding and removing of imports for the visualization preview of suggestions.


https://user-images.githubusercontent.com/1428930/190930045-fcbd44b7-d3d8-4f20-a178-8542190b86de.mp4

# Important Notes
[ci no changelog needed]

app/gui/controller/double-representation/src/module.rs

@@ -34,6 +34,11 @@ use serde::Serialize;
 #[allow(missing_docs)]
 pub struct ImportNotFound(pub ImportInfo);
 
+#[derive(Clone, Copy, Debug, Fail)]
+#[fail(display = "Import with ID `{}` was not found in the module.", _0)]
+#[allow(missing_docs)]
+pub struct ImportIdNotFound(pub ImportId);
+
 #[derive(Clone, Copy, Debug, Fail)]
 #[fail(display = "Line index is out of bounds.")]
 #[allow(missing_docs)]
@@ -387,12 +392,15 @@ impl PartialEq<tp::QualifiedName> for QualifiedName {
 // === ImportInfo ===
 // ==================
 
+/// Id for an import.
+pub type ImportId = u64;
+
 /// Representation of a single import declaration.
 // TODO [mwu]
 // Currently only supports the unqualified imports like `import Foo.Bar`. Qualified, restricted and
 // and hiding imports are not supported by the parser yet. In future when parser and engine
 // supports them, this structure should be adjusted as well.
-#[derive(Clone, Debug, Eq, PartialEq)]
+#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize, Hash)]
 pub struct ImportInfo {
     /// The segments of the qualified name of the imported target.
     ///
@@ -436,6 +444,16 @@ impl ImportInfo {
         ast::macros::is_match_import(&ast)
             .then(|| ImportInfo::from_target_str(ast.segs.head.body.repr().trim()))
     }
+
+    /// Return the ID of the import.
+    ///
+    /// The ID is based on a hash of the qualified name of the imported target. This ID is GUI
+    /// internal and not known in the engine.
+    pub fn id(&self) -> ImportId {
+        let mut hasher = DefaultHasher::new();
+        self.hash(&mut hasher);
+        hasher.finish()
+    }
 }
 
 impl Display for ImportInfo {
@@ -519,6 +537,17 @@ impl Info {
         Ok(())
     }
 
+    /// Remove a line that matches given import ID.
+    ///
+    /// If there is more than one line matching, only the first one will be removed.
+    /// Fails if there is no import matching given argument.
+    pub fn remove_import_by_id(&mut self, to_remove: ImportId) -> FallibleResult {
+        let lookup_result = self.enumerate_imports().find(|(_, import)| import.id() == to_remove);
+        let (crumb, _) = lookup_result.ok_or(ImportIdNotFound(to_remove))?;
+        self.remove_line(crumb.line_index)?;
+        Ok(())
+    }
+
     /// Add a new import declaration to a module.
     ///
     /// This function will try to keep imports in lexicographic order. It returns the index where

app/gui/src/controller/searcher.rs

@@ -16,6 +16,7 @@ use breadcrumbs::Breadcrumbs;
 use const_format::concatcp;
 use double_representation::graph::GraphInfo;
 use double_representation::graph::LocationHint;
+use double_representation::module::ImportInfo;
 use double_representation::module::QualifiedName;
 use double_representation::node::NodeInfo;
 use double_representation::project;
@@ -526,6 +527,11 @@ impl Searcher {
         Self::new_from_graph_controller(parent, ide, project, graph, mode)
     }
 
+    /// Abort editing and perform cleanup.
+    pub fn abort_editing(&self) {
+        self.clear_temporary_imports();
+    }
+
     /// Create new Searcher Controller, when you have Executed Graph Controller handy.
     #[profile(Task)]
     pub fn new_from_graph_controller(
@@ -769,13 +775,21 @@ impl Searcher {
     /// Use action at given index as a suggestion. The exact outcome depends on the action's type.
     pub fn preview_suggestion(&self, picked_suggestion: action::Suggestion) -> FallibleResult {
         tracing::debug!("Previewing suggestion: \"{picked_suggestion:?}\".");
+        self.clear_temporary_imports();
 
         let id = self.data.borrow().input.next_completion_id();
         let picked_completion = FragmentAddedByPickingSuggestion { id, picked_suggestion };
         let code_to_insert = self.code_to_insert(&picked_completion).code;
         tracing::debug!("Code to insert: \"{code_to_insert}\".",);
         let added_ast = self.ide.parser().parse_line_ast(&code_to_insert)?;
         let pattern_offset = self.data.borrow().input.pattern_offset;
+        {
+            // This block serves to limit the borrow of `self.data`.
+            let current_fragments = &self.data.borrow().fragments_added_by_picking;
+            let fragments_added_by_picking =
+                current_fragments.iter().chain(iter::once(&picked_completion));
+            self.add_required_imports(fragments_added_by_picking, false)?;
+        }
         let new_expression_chain = self.create_new_expression_chain(added_ast, pattern_offset);
         let expression = self.get_expression(Some(new_expression_chain));
         let intended_method = self.intended_method();
@@ -887,10 +901,12 @@ impl Searcher {
         if let Some(guard) = self.node_edit_guard.deref().as_ref() {
             guard.prevent_revert()
         }
-
+        self.clear_temporary_imports();
         // We add the required imports before we edit its content. This way, we avoid an
         // intermediate state where imports would already be in use but not yet available.
-        self.add_required_imports()?;
+        let data_borrowed = self.data.borrow();
+        let fragments = data_borrowed.fragments_added_by_picking.iter();
+        self.add_required_imports(fragments, true)?;
 
         let node_id = self.mode.node_id();
         let input_chain = self.data.borrow().input.as_prefix_chain(self.ide.parser());
@@ -977,9 +993,11 @@ impl Searcher {
     }
 
     #[profile(Debug)]
-    fn add_required_imports(&self) -> FallibleResult {
-        let data_borrowed = self.data.borrow();
-        let fragments = data_borrowed.fragments_added_by_picking.iter();
+    fn add_required_imports<'a>(
+        &self,
+        fragments: impl Iterator<Item = &'a FragmentAddedByPickingSuggestion>,
+        permanent: bool,
+    ) -> FallibleResult {
         let imports = fragments.flat_map(|frag| self.code_to_insert(frag).imports);
         let mut module = self.module();
         let here = self.module_qualified_name();
@@ -988,11 +1006,51 @@ impl Searcher {
         let without_enso_project = imports.filter(|i| i.to_string() != ENSO_PROJECT_SPECIAL_MODULE);
         for mut import in without_enso_project {
             import.remove_main_module_segment();
+            let import_info = ImportInfo::from_qualified_name(&import);
+
+            let already_exists = module.iter_imports().contains(&import_info);
+            if already_exists {
+                continue;
+            }
+
             module.add_module_import(&here, self.ide.parser(), &import);
+            self.graph.graph().module.with_import_metadata(
+                import_info.id(),
+                Box::new(|import_metadata| {
+                    import_metadata.is_temporary = !permanent;
+                }),
+            )?;
         }
         self.graph.graph().module.update_ast(module.ast)
     }
 
+    fn clear_temporary_imports(&self) {
+        let mut module = self.module();
+        let import_metadata = self.graph.graph().module.all_import_metadata();
+        let metadata_to_remove = import_metadata
+            .into_iter()
+            .filter_map(|(id, import_metadata)| {
+                import_metadata.is_temporary.then(|| {
+                    if let Err(e) = module.remove_import_by_id(id) {
+                        tracing::warn!("Failed to remove import because of: {e:?}");
+                    }
+                    id
+                })
+            })
+            .collect_vec();
+        if let Err(e) = self.graph.graph().module.update_ast(module.ast) {
+            tracing::warn!("Failed to update module ast when removing imports because of: {e:?}");
+        }
+        for id in metadata_to_remove {
+            if let Err(e) = self.graph.graph().module.remove_import_metadata(id) {
+                tracing::warn!(
+                    "Failed to remove import metadata for import id {id} because of: {e:?}"
+                );
+            }
+        }
+    }
+
+
     /// Reload Action List.
     ///
     /// The current list will be set as "Loading" and Language Server will be requested for a new

app/gui/src/model/module.rs

@@ -26,6 +26,7 @@ pub mod plain;
 pub mod synchronized;
 
 pub use double_representation::module::Id;
+use double_representation::module::ImportId;
 pub use double_representation::module::QualifiedName;
 pub use double_representation::tp::QualifiedName as TypeQualifiedName;
 
@@ -35,11 +36,16 @@ pub use double_representation::tp::QualifiedName as TypeQualifiedName;
 // == Errors ==
 // ============
 
-/// Failure for missing node metadata.
+#[allow(missing_docs)]
 #[derive(Debug, Clone, Copy, Fail)]
 #[fail(display = "Node with ID {} was not found in metadata.", _0)]
 pub struct NodeMetadataNotFound(pub ast::Id);
 
+#[allow(missing_docs)]
+#[derive(Debug, Clone, Copy, Fail)]
+#[fail(display = "Import with ID {} was not found in metadata.", _0)]
+pub struct ImportMetadataNotFound(pub ImportId);
+
 /// Failed attempt to tread a file path as a module path.
 #[derive(Clone, Debug, Fail)]
 #[fail(display = "The path `{}` is not a valid module path. {}", path, issue)]
@@ -363,6 +369,8 @@ pub struct IdeMetadata {
     /// Metadata that belongs to nodes.
     #[serde(deserialize_with = "enso_prelude::deserialize_or_default")]
     node:    HashMap<ast::Id, NodeMetadata>,
+    #[serde(default, deserialize_with = "enso_prelude::deserialize_or_default")]
+    import:  HashMap<ImportId, ImportMetadata>,
     /// The project metadata. This is stored only in the main module's metadata.
     #[serde(default, deserialize_with = "enso_prelude::deserialize_or_default")]
     project: Option<ProjectMetadata>,
@@ -515,6 +523,15 @@ pub struct UploadingFile {
     pub error:          Option<String>,
 }
 
+#[allow(missing_docs)]
+#[allow(missing_copy_implementations)]
+#[derive(Clone, Debug, Default, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub struct ImportMetadata {
+    #[serde(skip_serializing_if = "core::ops::Not::not")]
+    #[serde(default, deserialize_with = "enso_prelude::deserialize_or_default")]
+    pub is_temporary: bool,
+}
+
 
 // ==============
 // === Module ===
@@ -585,6 +602,19 @@ pub trait API: Debug + model::undo_redo::Aware {
         fun: Box<dyn FnOnce(&mut NodeMetadata) + '_>,
     ) -> FallibleResult;
 
+    /// Modify metadata of given import.
+    fn with_import_metadata(
+        &self,
+        id: ImportId,
+        fun: Box<dyn FnOnce(&mut ImportMetadata) + '_>,
+    ) -> FallibleResult;
+
+    /// Returns the import metadata fof the module.
+    fn all_import_metadata(&self) -> Vec<(ImportId, ImportMetadata)>;
+
+    /// Removes the import metadata of the import.
+    fn remove_import_metadata(&self, id: ImportId) -> FallibleResult<ImportMetadata>;
+
     /// This method exists as a monomorphication for [`with_project_metadata`]. Users are encouraged
     /// to use it rather then this method.
     ///

app/gui/src/model/module/plain.rs

@@ -3,6 +3,8 @@
 use crate::prelude::*;
 
 use crate::model::module::Content;
+use crate::model::module::ImportMetadata;
+use crate::model::module::ImportMetadataNotFound;
 use crate::model::module::Metadata;
 use crate::model::module::NodeMetadata;
 use crate::model::module::NodeMetadataNotFound;
@@ -14,6 +16,7 @@ use crate::model::module::TextChange;
 use crate::notification;
 
 use double_representation::definition::DefinitionInfo;
+use double_representation::module::ImportId;
 use flo_stream::Subscriber;
 use parser::api::ParsedSourceFile;
 use parser::api::SourceFile;
@@ -210,6 +213,32 @@ impl model::module::API for Module {
         })
     }
 
+    fn with_import_metadata(
+        &self,
+        id: ImportId,
+        fun: Box<dyn FnOnce(&mut ImportMetadata) + '_>,
+    ) -> FallibleResult {
+        self.update_content(NotificationKind::MetadataChanged, |content| {
+            let lookup = content.metadata.ide.import.remove(&id);
+            let mut data = lookup.unwrap_or_default();
+            fun(&mut data);
+            content.metadata.ide.import.insert(id, data);
+        })
+    }
+
+    fn all_import_metadata(&self) -> Vec<(ImportId, ImportMetadata)> {
+        let content = self.content.borrow();
+        content.metadata.ide.import.clone().into_iter().collect()
+    }
+
+    fn remove_import_metadata(&self, id: ImportId) -> FallibleResult<ImportMetadata> {
+        self.try_updating_content(NotificationKind::MetadataChanged, |content| {
+            let lookup = content.metadata.ide.import.remove(&id);
+            lookup.ok_or_else(|| ImportMetadataNotFound(id).into())
+        })
+    }
+
+
     fn boxed_with_project_metadata(&self, fun: Box<dyn FnOnce(&ProjectMetadata) + '_>) {
         let content = self.content.borrow();
         if let Some(metadata) = content.metadata.ide.project.as_ref() {

app/gui/src/model/module/synchronized.rs

@@ -4,6 +4,7 @@ use crate::prelude::*;
 use enso_text::unit::*;
 
 use crate::model::module::Content;
+use crate::model::module::ImportMetadata;
 use crate::model::module::NodeMetadata;
 use crate::model::module::Notification;
 use crate::model::module::NotificationKind;
@@ -15,6 +16,7 @@ use crate::model::module::API;
 use ast::IdMap;
 use double_representation::definition::DefinitionInfo;
 use double_representation::graph::Id;
+use double_representation::module::ImportId;
 use engine_protocol::language_server;
 use engine_protocol::language_server::TextEdit;
 use engine_protocol::types::Sha3_224;
@@ -241,6 +243,22 @@ impl API for Module {
         self.model.with_node_metadata(id, fun)
     }
 
+    fn with_import_metadata(
+        &self,
+        id: ImportId,
+        fun: Box<dyn FnOnce(&mut ImportMetadata) + '_>,
+    ) -> FallibleResult {
+        self.model.with_import_metadata(id, fun)
+    }
+
+    fn all_import_metadata(&self) -> Vec<(ImportId, ImportMetadata)> {
+        self.model.all_import_metadata()
+    }
+
+    fn remove_import_metadata(&self, id: ImportId) -> FallibleResult<ImportMetadata> {
+        self.model.remove_import_metadata(id)
+    }
+
     fn boxed_with_project_metadata(&self, fun: Box<dyn FnOnce(&ProjectMetadata) + '_>) {
         self.model.boxed_with_project_metadata(fun)
     }

app/gui/src/presenter/searcher.rs

@@ -541,7 +541,9 @@ impl Searcher {
     ///
     /// This method takes `self`, as the presenter (with the searcher view) should be dropped once
     /// editing finishes.
-    pub fn abort_editing(self) {}
+    pub fn abort_editing(self) {
+        self.model.controller.abort_editing()
+    }
 
     /// Returns the node view that is being edited by the searcher.
     pub fn input_view(&self) -> ViewNodeId {

app/gui/view/src/searcher.rs

@@ -229,8 +229,9 @@ impl View {
                 model.doc_provider.set(docs.clone_ref());
                 model.list.set_entries(entries);
             });
+            selected_entry <- model.list.selected_entry.on_change();
             eval frp.select_action ((id) model.list.select_entry(id));
-            source.selected_entry <+ model.list.selected_entry;
+            source.selected_entry <+ selected_entry;
             source.size           <+ height.value.map(|h| Vector2(SEARCHER_WIDTH,*h));
             source.is_visible     <+ model.list.size.map(|size| size.x*size.y > std::f32::EPSILON);
             // source.is_selected    <+ model.documentation.frp.is_selected.map(|&value|value);
@@ -240,11 +241,11 @@ impl View {
             eval frp.show     ((()) height.set_target_value(SEARCHER_HEIGHT));
             eval frp.hide     ((()) height.set_target_value(-list_view::SHADOW_PX));
 
-            is_selected               <- model.list.selected_entry.map(|e| e.is_some());
+            is_selected               <- selected_entry.map(|e| e.is_some());
             is_enabled                <- bool(&frp.hide,&frp.show);
             is_entry_enabled          <- is_selected && is_enabled;
-            displayed_doc             <- model.list.selected_entry.map(f!((id) model.docs_for(*id)));
-            opt_picked_entry          <- model.list.selected_entry.sample(&frp.use_as_suggestion);
+            displayed_doc             <- selected_entry.map(f!((id) model.docs_for(*id)));
+            opt_picked_entry          <- selected_entry.sample(&frp.use_as_suggestion);
             source.used_as_suggestion <+ opt_picked_entry.gate(&is_entry_enabled);
             source.editing_committed  <+ model.list.chosen_entry.gate(&is_entry_enabled);
 

build-config.yaml

@@ -1,6 +1,6 @@
 # Options intended to be common for all developers.
 
-wasm-size-limit: 14.77 MiB
+wasm-size-limit: 14.79 MiB
 
 required-versions:
   cargo-watch: ^8.1.1