Correct import handling when selecting items from the component browser #182634001 #3708

MichaelMauderer · 2022-09-14T12:21:41Z

Pull Request Description

Implements Task #182634001

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

Peek.2022-09-18.23-10.mp4

Important Notes

[ci no changelog needed]

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

…ort_Handling_when_selecting_Items_from_the_Component_Browser_#182634001

farmaazon

First, I would not keep a duplicated info in the source file.

farmaazon · 2022-09-21T07:52:20Z

CHANGELOG.md

 - [Selecting a suggestion from the searcher or component browser now updates the
  visualisation of the edited node to preview the results of applying the
  suggestion.][3691]
+- [Selecting a suggestion from the searcher or component browser now updates the 
+  visualisation of the edited node to preview the results of applying the 
+  suggestion.][3691]


Duplicated entry.

farmaazon · 2022-09-21T07:59:00Z

app/gui/src/controller/searcher.rs

+        for import_metadata in self.graph.graph().module.all_import_metadata() {
+            if import_metadata.is_temporary {
+                if let Some(import_info) = &import_metadata.info {
+                    if let Err(e) = module.remove_import(import_info) {
+                        tracing::warn!("Failed to remove import because of: {e:?}");
+                    }
+                    to_remove.push(import_info.id());
+                }
+            }
+        }


Instead of mutable to_remove field, we could take an "iterators" approach: let to_remove = self.graph.graph().module.all_import_metadata().into_iter().filter_map(|...|). This way, it is clear where to_remove is created and where is it used.

farmaazon · 2022-09-21T07:59:46Z

app/gui/src/controller/searcher.rs

+        to_remove.iter().for_each(|id| {
+            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:?}"
+                );
+            }
+        });


Here in turn, I would use for syntax, which is essentially the same as foreach except you avoid making closures.

farmaazon · 2022-09-21T08:01:17Z

app/gui/src/model/module.rs

@@ -39,6 +41,11 @@ pub use double_representation::tp::QualifiedName as TypeQualifiedName;
 #[derive(Debug, Clone, Copy, Fail)]
 #[fail(display = "Node with ID {} was not found in metadata.", _0)]
 pub struct NodeMetadataNotFound(pub ast::Id);
+/// Failure for missing node metadata.


Duplicated docs. Additionally, in other places we do not add docs for error structures, as the message in fail should be descriptive enough.

farmaazon · 2022-09-21T08:02:57Z

app/gui/src/model/module.rs

+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(default, deserialize_with = "enso_prelude::deserialize_or_default")]
+    pub info:         Option<ImportInfo>,


Why do we keep ImportInfo in metadata? The only field of ImportInfo should be deducible from the AST.

This made it easier to use the existing API for removing imports, which require ImportInfo. It also makes debugging the metadata slightly nicer, as it adds the text of the import to the metadata. But it is a bit of overhead, so I’ve removed it in favour of a new API to remove imports by ID.

farmaazon · 2022-09-21T08:06:34Z

build-config.yaml

@@ -2,6 +2,7 @@

 wasm-size-limit: 14.60 MiB

+


Is this an intentional change?

No. Removed.

farmaazon · 2022-09-22T09:54:01Z

app/gui/src/controller/searcher.rs

+        let to_remove = self
+            .graph
+            .graph()
+            .module
+            .all_import_metadata()
+            .into_iter()
+            .filter_map(|(id, import_metadata)| {
+                if import_metadata.is_temporary {
+                    if let Err(e) = module.remove_import_by_id(id) {
                        tracing::warn!("Failed to remove import because of: {e:?}");
                    }
-                    to_remove.push(import_info.id());
+                    return Some(id);
                }
-            }
-        }
+                None
+            })
+            .collect_vec();


Please split it into several lets to avoid "vertical noise". For example, self.graph.graph().module could be in a single line.

farmaazon · 2022-09-22T09:55:02Z

app/gui/src/controller/searcher.rs

+                    return Some(id);
                }
-            }
-        }
+                None


Let's do not return, it isn't even obvious what we're actually returning.

…ort_Handling_when_selecting_Items_from_the_Component_Browser_#182634001

MichaelMauderer self-assigned this Sep 14, 2022

MichaelMauderer force-pushed the wip/michaelmauderer/Implement_Correct_Import_Handling_when_selecting_Items_from_the_Component_Browser_#182634001 branch from 87209e3 to 6a7c977 Compare September 17, 2022 12:51

MichaelMauderer added 3 commits September 17, 2022 15:11

Update changelog.

b014390

Implement import metadata.

1b2de5d

Implement proper import handling for searcher previews.

ccedc51

MichaelMauderer force-pushed the wip/michaelmauderer/Implement_Correct_Import_Handling_when_selecting_Items_from_the_Component_Browser_#182634001 branch from 6a7c977 to ccedc51 Compare September 17, 2022 14:18

MichaelMauderer added 3 commits September 17, 2022 15:24

Add missing docs.

4b7f9ef

Remove commented code.

4dbc07f

Fix re-borrow.

9a3421b

MichaelMauderer marked this pull request as ready for review September 17, 2022 22:25

MichaelMauderer requested review from wdanilo, farmaazon and mwu-tow as code owners September 17, 2022 22:25

Bump wasm size.

e5e479f

MichaelMauderer marked this pull request as draft September 17, 2022 22:27

Add missing serde annotation.

149959f

MichaelMauderer marked this pull request as ready for review September 20, 2022 09:02

Merge branch 'develop' into wip/michaelmauderer/Implement_Correct_Imp…

f79b89e

…ort_Handling_when_selecting_Items_from_the_Component_Browser_#182634001

farmaazon requested changes Sep 21, 2022

View reviewed changes

Implement review feedback.

bd30479

MichaelMauderer requested a review from farmaazon September 21, 2022 15:00

farmaazon approved these changes Sep 22, 2022

View reviewed changes

MichaelMauderer and others added 3 commits September 22, 2022 12:21

Implement review feedback.

78ef754

Merge branch 'develop' into wip/michaelmauderer/Implement_Correct_Imp…

725a27d

…ort_Handling_when_selecting_Items_from_the_Component_Browser_#182634001

Bump wasm size.

1443abe

MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Sep 22, 2022

mergify bot and others added 4 commits September 22, 2022 20:56

Merge branch 'develop' into wip/michaelmauderer/Implement_Correct_Imp…

92de32c

…ort_Handling_when_selecting_Items_from_the_Component_Browser_#182634001

Merge branch 'develop' into wip/michaelmauderer/Implement_Correct_Imp…

3d69c26

…ort_Handling_when_selecting_Items_from_the_Component_Browser_#182634001

Bump wasm size.

f88aa4b

Add missing serde annotation.

18c964a

Fix test anotation.

12157d4

mergify bot merged commit ff3c481 into develop Sep 23, 2022

mergify bot deleted the wip/michaelmauderer/Implement_Correct_Import_Handling_when_selecting_Items_from_the_Component_Browser_#182634001 branch September 23, 2022 14:30

wdanilo mentioned this pull request Feb 6, 2023

Implement Correct Import Handling when selecting Items from the Component Browser #4574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct import handling when selecting items from the component browser #182634001 #3708

Correct import handling when selecting items from the component browser #182634001 #3708

MichaelMauderer commented Sep 14, 2022 •

edited

Loading

farmaazon left a comment

farmaazon Sep 21, 2022

farmaazon Sep 21, 2022

farmaazon Sep 21, 2022

farmaazon Sep 21, 2022

farmaazon Sep 21, 2022

MichaelMauderer Sep 21, 2022

farmaazon Sep 21, 2022

MichaelMauderer Sep 21, 2022

farmaazon Sep 22, 2022

farmaazon Sep 22, 2022

Correct import handling when selecting items from the component browser #182634001 #3708

Correct import handling when selecting items from the component browser #182634001 #3708

Conversation

MichaelMauderer commented Sep 14, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

farmaazon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelMauderer commented Sep 14, 2022 •

edited

Loading