Cursor aware Component Browser #5770

vitvakatu · 2023-02-27T09:07:01Z

Pull Request Description

Closes #5220

This PR implements the cursor-aware behavior of the CB, as described in pivotal issue

2023-02-28.13-12-48.mp4

2023-03-06.17-24-59.mp4

Important Notes

The intended_method of the node's metadata is marked as deprecated, and all usages are removed.
It seems all usages of Scala parser are removed from IDE. We no longer use it to parse documentation for snippets.

This is how the snippets docs look now:

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.

Fixes after rebase Add some docs

# Conflicts: # app/gui/src/controller/searcher.rs

# Conflicts: # app/gui/src/controller/searcher.rs # app/gui/src/presenter/searcher.rs

vitvakatu · 2023-02-27T09:22:35Z

app/gui/src/controller/searcher.rs

@@ -628,7 +439,8 @@ impl Searcher {
        let def_id = graph.graph().id;
        let def_span = double_representation::module::definition_span(&module_ast, &def_id)?;
        let module_repr: Rope = module_ast.repr().into();
-        let position = module_repr.offset_to_location_snapped(def_span.end);
+        // TODO: is it correct?
+        let position = module_repr.offset_to_location_snapped(def_span.end - text::ByteDiff(1));


Subtracting one byte from position helps to find local variables declared inside main function. Apparently the engine thinks that locals are out of scope otherwise and doesn't send them in completion response. This issue should be properly investigated separately.

Let's make a GH issue for that and put its number here.

I fixed the issue completely by searching position before creating a new node.

vitvakatu · 2023-02-27T09:24:56Z

app/gui/src/controller/searcher.rs

+        let old_context = old_input.context().map(|ctx| ctx.into_ast().repr());
+
+        self.invalidate_picked_suggestions();
+        let context_changed = old_context != new_context;


I'm not sure if we're doing the correct thing here. Previous code compared expressions, now we compare access chains, and they usually None. So we are reloading the list far less frequently now.

This is correct. Before we were filtering the components by expected argument type (and it depended on the left side of the expression), but now the filtering depends only on accessor chain (i.e. the context).

vitvakatu · 2023-02-27T09:28:27Z

app/gui/src/controller/searcher.rs

+        assert!(are_same(&arg1.entry, &test_var_1));
+        assert!(are_same(&arg2.entry, &test_var_1));
+
+        // TODO


I don't understand what's going on in this part of the test, I will fix it or remove it.

Seems it is not compatible with the new behavior, removed this piece

vitvakatu · 2023-02-27T09:30:16Z

app/gui/src/controller/searcher/component/hardcoded.rs

-        let doc_string = documentation.to_string();
-        let documentation_html = doc_parser.generate_html_doc_pure(doc_string);
-        self.documentation_html = Some(documentation_html.unwrap());
+        // TODO: we need the Scala parser in searcher tests only for HTML documentation.


This doesn't look like the correct change, but removing the doc parser usage allows to turn all searcher tests into native ones. This is a huge advantage while developing, though I probably should revert this afterwards and make searcher tests wasm_test again.

At the same time, the documentation for snippets is extremely primitive and doc parser doesn't really bring much.

We can stay with this change. Before the searcher, tests were usually wasm_bindgen anyway, because we used the Scala parser. But I guess it's no longer the case?

Yep, now the tests use Rust parser in general.

I decided to remove the scala parser from here, it does not bring us much.

vitvakatu · 2023-02-27T09:31:49Z

app/gui/src/controller/searcher/input.rs

+        let range_start = raw_range.start;
+        let range_end = raw_range.end;
+        Self::ensure_on_char_boundary(&new_input, range_start)?;
+        Self::ensure_on_char_boundary(&new_input, range_end)?;


I saw rare panics when one of the indices was not on char boundary inside the input, which was weird. This code helps to mitigate the issue if it ever happens again.

I'd "inline" the range_start and range_end variables. They won't make the lines too long.

vitvakatu · 2023-02-27T09:33:35Z

app/gui/src/model/module.rs

@@ -411,6 +411,7 @@ pub struct NodeMetadata {
    ///
    /// The methods may be defined for different types, so the name alone don't specify them.
    #[serde(default, deserialize_with = "enso_prelude::deserialize_or_default")]
+    // TODO: It is not used anywhere. Deprecate?
    pub intended_method: Option<MethodId>,


Now all usages of the intended_method are removed. We are no longer using it to communicate with the engine, so this field should be probably removed or marked as deprecated.

Marked it as deprecated and removed all usages from the code.

farmaazon · 2023-02-28T09:53:16Z

app/gui/src/controller/searcher/input.rs

+        let cursor_position_before =
+            (cursor_position > text::Byte(0)).then(|| cursor_position - text::ByteDiff(1));
+        let ast_stack_on_left =
+            cursor_position_before.map(|cp| AstAtPositionFinder::find(&ast.elem, cp));
+        let edited_ast_on_left = ast_stack_on_left.map_or_default(EditedAst::new);
+        let edited_ast = if edited_ast_on_left.edited_name.is_none() {
+            let ast_stack_on_right = AstAtPositionFinder::find(&ast.elem, cursor_position);
+            let edited_ast_on_right = EditedAst::new(ast_stack_on_right);
+            if edited_ast_on_right.edited_name.is_some() {
+                edited_ast_on_right
+            } else {
+                edited_ast_on_left
+            }
+        } else {
+            edited_ast_on_left
+        };
+        let ast = InputAst::Line(ast);
+        Self { ast, cursor_position, edited_ast }


This code deserves a bit of explanation in the comment.

farmaazon · 2023-02-28T09:58:46Z

app/gui/src/controller/searcher/input.rs

+        let range_start = raw_range.start;
+        let range_end = raw_range.end;
+        Self::ensure_on_char_boundary(&new_input, range_start)?;
+        Self::ensure_on_char_boundary(&new_input, range_end)?;


I'd "inline" the range_start and range_end variables. They won't make the lines too long.

farmaazon · 2023-02-28T10:00:29Z

app/gui/src/controller/searcher/input.rs

+#[derive(Clone, Debug, Fail)]
+#[allow(missing_docs)]
+#[fail(display = "Not a char boundary: index {} at '{}'.", index, string)]
+pub struct NotACharBoundary {
+    index:  usize,
+    string: String,
+}


Let's put it in Errors section somewhere at the top.

farmaazon · 2023-02-28T10:04:03Z

app/gui/src/controller/graph.rs

 impl span_tree::generate::Context for Handle {
-    fn call_info(&self, id: node::Id, name: Option<&str>) -> Option<CalledMethodInfo> {
-        let db = &self.suggestion_db;
-        let metadata = self.module.node_metadata(id).ok()?;
-        let db_entry = db.lookup_method(metadata.intended_method?)?;
-        // If the name is different than intended method than apparently it is not intended anymore
-        // and should be ignored.
-        let matching = if let Some(name) = name { name == db_entry.name } else { true };
-        matching.then(|| db_entry.invocation_info())
+    fn call_info(&self, _id: node::Id, _name: Option<&str>) -> Option<CalledMethodInfo> {
+        None
    }
 }


We already have Empty context for such cases: https://github.com/enso-org/enso/blob/d8f6f3fd52d87f63d8b067b609162423ed23ebcc/app/gui/language/span-tree/src/generate/context.rs

So, if we are not in some weird generics' machinery, the controller should not implement Context at all, and use Empty instead if itself where needed.

Done, also removed a couple of tests that relied on arguments information from node metadata.

farmaazon · 2023-02-28T10:10:28Z

app/gui/src/controller/searcher.rs

+        let old_context = old_input.context().map(|ctx| ctx.into_ast().repr());
+
+        self.invalidate_picked_suggestions();
+        let context_changed = old_context != new_context;


This is correct. Before we were filtering the components by expected argument type (and it depended on the left side of the expression), but now the filtering depends only on accessor chain (i.e. the context).

farmaazon · 2023-02-28T10:18:02Z

app/gui/src/test.rs

@@ -264,7 +264,7 @@ pub mod mock {
            let mut json_client = language_server::MockClient::default();
            let mut binary_client = binary::MockClient::new();
            // Creating a searcher controller always triggers a query for completion.
-            controller::searcher::test::expect_completion(&mut json_client, &[]);
+            // controller::searcher::test::expect_completion(&mut json_client, &[]);


Commented code. Remove if unneeded.

farmaazon · 2023-02-28T10:18:38Z

app/gui/view/graph-editor/src/component/node/input/area.rs

@@ -850,6 +852,8 @@ ensogl::define_endpoints! {
        /// Set the node expression.
        set_expression (node::Expression),

+        edit_expression (text::Range<Byte>, ImString),


Missing docs.

kazcw

Looks good. @farmaazon I think this answers my questions about how the frontend uses the Scala doc parser!

kazcw · 2023-02-28T23:07:53Z

app/gui/src/controller/searcher.rs

        self.graph.graph().set_expression(node_id, expression)?;
-        if let Mode::NewNode { .. } = self.mode.as_ref() {
+        let graph = self.graph.graph();


Is this bound for convenience, or to capture the value returned by graph() before a subsequent operation changes it? If it's for convenience it should be used consistently so it doesn't look like something tricky is happening; if it is intentionally capturing an older value, a comment would be helpful.

kazcw · 2023-02-28T23:54:49Z

app/gui/src/controller/searcher/input.rs

+    /// Return `true` if the `index` is on a char boundary inside `string` and can be used as a
+    /// range boundary. Otherwise, return `false` and report an error.
+    fn ensure_on_char_boundary(string: &str, index: usize) -> FallibleResult<()> {


Docs don't match return type.

kazcw · 2023-03-01T00:27:25Z

app/gui/src/controller/searcher/input.rs

+            let cursor_position_in_name = self.cursor_position - edited.range.start;
+            let name = ast::identifier::name(&edited.ast);
+            name.map_or("", |n| {
+                &n[text::Range::new(text::Byte(0), text::Byte(0) + cursor_position_in_name)]


This slice can panic if edited_ast and cursor_position are inconsistent. That shouldn't happen, but the invariant relied on is not trivial--slicing with get here would reduce the consequences of potential future bugs.

farmaazon · 2023-03-01T13:16:22Z

QA report

Issues

When starting editing node in the middle of identifier, I see CB with all entries first, and the filtering appears only after a moment.

cursor-aware-test-2023-03-01_13.47.34.mp4

Bad UX

Besides the above issue, the PR meets the acceptance criteria. Still, when testing, I found additional things worsening my UX:

When writing literals (text, number), the results are no longer filtered, so we consistently try to add the first selected entry (usually Column.from_vector) to the input. Before, because literals were part of pattern (and usually no entry matched the literal) the component browser was empty. The solution would be to not display suggestions when the cursor is in or next to a literal, or, even better, we could display a single entry, for example "foo bar" if user puts "foo bar, or 43 if user types 43 - but this is worth a separate task.
When editing existing static method invocation, accepting the same method adds the type name again, resulting in erroneous code. It may also be seen as error in visualization preview:

Solution for it is implementing #5356 we may consider keeping this branch unmerged until we implement and merge that task to it.

wdanilo · 2023-03-17T06:12:04Z

@vitvakatu what is the status of the PR? It was accepted 2 weeks ago (!), and the QA was done then. Why is it not merged yet?

vitvakatu · 2023-03-17T07:32:17Z

@wdanilo as we agreed with @farmaazon, we will merge #5356 into this PR, and then merge the whole thing. The QA done 2 weeks ago discovered a few issues, and they were addressed during that two weeks.

wdanilo · 2023-03-17T07:33:22Z

@vitvakatu gotcha! Sorry, I forgot about it. Please make short notes about agreements in the future so we will not be missing them. Thanks again! :)

wdanilo · 2023-03-22T00:18:04Z

@vitvakatu as we are making changes here, could you please move it to draft stage and request review again after the code is final, please?

Closes #5356 Implements recognition of qualified names in the input of the CB. Now accepting of the suggestion produces a valid code, and only necessary imports are added. https://user-images.githubusercontent.com/6566674/225487445-855dfb04-47c7-44c9-bc92-4bb5663d500a.mp4 https://user-images.githubusercontent.com/6566674/225487486-028d1ae0-02e0-4bbc-a623-7d4e620c87d9.mp4 # Important Notes - Filtering by inserted code no longer works, but instead, we filter out entries with qualified names not matching the edited accessor chain. It may result in worsened behavior when typing expressions with local variables as `this` argument.

vitvakatu · 2023-03-22T13:24:31Z

QA was done in #5960 on an identical state of the code.

Fix a stupid typo introduced in #5770. It affected the code generated by CB while previewing suggestions.

* develop: Layout fixes (#6066) Use new Enso Hash Codes and Comparable (#6060) Search suggestions by static attribute (#6036) Use .node-version for pinning Node.js version (#6057) Fix code generation for suggestion preview (#6054) Implementation of EnsoGL predefined Rectangle shape. (#6033) Tidy up the public module level statics (#6032) Cursor aware Component Browser (#5770)

@Frizi

The test was mistakenly removed during the merge with develop in #5770. The code is now restored from develop version without any modifications. Thanks to @Frizi for heads up.

farmaazon and others added 10 commits February 10, 2023 12:11

Input parsing and tests.

ff40a2e

Fixes after rebase Add some docs

fix after merge

e264c68

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

22726d2

# Conflicts: # app/gui/src/controller/searcher.rs

wip

0ea883f

wip

fb45fd3

wip

4938f4f

wip

c5d2e78

wip

00ed5ee

linting

724d39b

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

92928ca

# Conflicts: # app/gui/src/controller/searcher.rs # app/gui/src/presenter/searcher.rs

vitvakatu self-assigned this Feb 27, 2023

self-review & post-merge fixes

7e308a9

vitvakatu commented Feb 27, 2023

View reviewed changes

vitvakatu added 3 commits February 27, 2023 17:33

wip

ee6486c

Fix the issue with missing local scope vars

1c05ef5

Minor fixes and refactorings

04ebb63

vitvakatu added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 28, 2023

vitvakatu marked this pull request as ready for review February 28, 2023 09:21

vitvakatu requested review from MichaelMauderer, wdanilo and mwu-tow as code owners February 28, 2023 09:21

vitvakatu requested a review from farmaazon February 28, 2023 09:21

farmaazon approved these changes Feb 28, 2023

View reviewed changes

farmaazon and others added 3 commits February 28, 2023 11:53

Apply self-review

6a0c6c3

Address review comments & remove broken tests

533bca2

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

5aa681c

sylwiabr requested a review from kazcw February 28, 2023 18:39

kazcw approved these changes Mar 1, 2023

View reviewed changes

Apply review comments

4fed898

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

695015d

farmaazon mentioned this pull request Mar 10, 2023

Mapping value to elements of vector renders errror despite transforming data correctly #5888

Closed

2 tasks

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

a9083a7

vitvakatu requested a review from JaroslavTulach as a code owner March 15, 2023 23:54

vitvakatu mentioned this pull request Mar 16, 2023

support custom widget entry labels #5705

Merged

4 tasks

JaroslavTulach approved these changes Mar 17, 2023

View reviewed changes

vitvakatu added 2 commits March 17, 2023 11:45

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

feff9b4

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

5c6240e

vitvakatu marked this pull request as draft March 22, 2023 07:57

vitvakatu marked this pull request as ready for review March 22, 2023 13:15

vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Mar 22, 2023

Merge branch 'develop' into wip/farmaazon/cursor-aware-cb-183521918

b82cc34

vitvakatu removed the CI: Ready to merge This PR is eligible for automatic merge label Mar 22, 2023

Adjust and disable required imports tests.

ae1effd

vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Mar 22, 2023

mergify bot merged commit 1b30a52 into develop Mar 22, 2023

mergify bot deleted the wip/farmaazon/cursor-aware-cb-183521918 branch March 22, 2023 17:10

vitvakatu mentioned this pull request Mar 22, 2023

Fix code generation for suggestion preview #6054

Merged

5 tasks

mergify bot pushed a commit that referenced this pull request Mar 23, 2023

Fix code generation for suggestion preview (#6054)

de008eb

Fix a stupid typo introduced in #5770. It affected the code generated by CB while previewing suggestions.

farmaazon mentioned this pull request Mar 24, 2023

Visualisation evaluation failed: Type error: expected a function, but got 100000 (Integer) resulting in Error #5877

Closed

2 tasks

vitvakatu mentioned this pull request Mar 28, 2023

Restore mistakenly removed unit test for named arguments #6101

Merged

5 tasks

vitvakatu mentioned this pull request Apr 19, 2023

Cursor keys when editing a node move both the caret position and the selected item in the Component Browser #6124

Closed

Cursor aware Component Browser #5770

Cursor aware Component Browser #5770

Conversation

vitvakatu commented Feb 27, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

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

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

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

kazcw 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

farmaazon commented Mar 1, 2023

QA report

Issues

Bad UX

wdanilo commented Mar 17, 2023

vitvakatu commented Mar 17, 2023

wdanilo commented Mar 17, 2023

wdanilo commented Mar 22, 2023

vitvakatu commented Mar 22, 2023

vitvakatu commented Feb 27, 2023 •

edited

Loading