Show Visualisation Preview when Selecting Item on Searcher #3691

MichaelMauderer · 2022-09-07T12:37:44Z

Pull Request Description

Enables visualization previews for selections in the searcher.

Works for the old Searcher and the new Component Browser, but it was easier to show the examples for the old searcher, as the suggestions seem to be better/faster available currently (this will improve with the new implementation using better strings and the new GridView).

Peek.2022-09-07.12-59.mp4

Aborting an edit now also correctly reverts a node.

Peek.2022-09-07.13-00.mp4

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.

wdanilo · 2022-09-08T03:39:41Z

Michael, this is amazing. One thing that is to be fixed is that sometimes the result needs to be computed for a longer time. Until its computed, a spinner sohuld be shown. Any spinner - could be any shape animated in GLSL. BTW, if you'll be animating in GLSL (like our. text cursor, the animation will stop when you stop moving mouse - this is a bug I'll fix in my PR).

wdanilo

Added comment above.

MichaelMauderer · 2022-09-08T08:29:19Z

Michael, this is amazing. One thing that is to be fixed is that sometimes the result needs to be computed for a longer time. Until its computed, a spinner sohuld be shown. Any spinner - could be any shape animated in GLSL. BTW, if you'll be animating in GLSL (like our. text cursor, the animation will stop when you stop moving mouse - this is a bug I'll fix in my PR).

@wdanilo Should we plan this as a separate task, though? While I agree that this would be a super useful feature, it seems somewhat unrelated to this task, from a planning standpoint. If anything, I would add this feature as part of the "always show the visualization" task, as that actually touches visualization code.

wdanilo · 2022-09-09T09:18:11Z

app/gui/src/controller/searcher.rs

+            }
+            Some(mut expression) => {
+                let new_argument = ast::prefix::Argument {
+                    sast:      ast::Shifted::new(pattern_offset.max(1), added_ast),


wdanilo · 2022-09-09T09:19:26Z

app/gui/src/presenter/graph/state.rs

+        if displayed.expression != expression {
+            displayed.expression = expression;
+            Some(ast_id)
+        } else {
+            None


use .as_some on bool instead in such cases.

wdanilo · 2022-09-09T09:19:59Z

app/gui/src/presenter/graph/state.rs

+        tracing::debug!(
+            "Setting node expression from view: {} -> {}",
+            displayed.expression,
+            expression
+        );


you can make it 2 lines instead of 5 lines by extracting &str to var

For me, extracting the message makes the debug statement much less readable.

let debug_message = "Setting node expression from view: {} -> {}"; tracing::debug!(debug_message, displayed.expression, expression);

The extra variable does not add any useful contextual information, but now when scanning the debug statement, one has to trace back to the variable to read its content.
Before, one could just read the debug statement linearly top to bottom.

you can make it 2 lines instead of 5 lines by extracting &str to var

I'm not sure if it has the same performance: the format macro can adjust allocated space when knows the string on compilation time.

I believe it has the same performance - this is &'static str, it is not allocated, it is build-into the binary, it sohuld be:

let debug_message = "Setting node expression from view:"; tracing::debug!("{debug_message} {} -> {}", displayed.expression, expression);

It's sharter to look at and I think we should optimize the code lengh, otherwise it's hard to read. If we can change it, I'd be thankful.

wdanilo · 2022-09-09T09:20:11Z

app/gui/src/presenter/project.rs

@@ -129,6 +129,8 @@ impl Model {
    fn editing_aborted(&self) {
        let searcher = self.searcher.take();
        if let Some(searcher) = searcher {
+            let node = self.graph.ast_node_of_view(searcher.input_view()).unwrap();


unwrap left in the code

farmaazon

You should be awarded with "creator of the branch with the longest name" badge.

farmaazon · 2022-09-12T14:45:50Z

app/gui/src/controller/searcher.rs

+        let new_expression = match self.data.borrow_mut().input.expression.clone() {
+            None => {
+                let ast = ast::prefix::Chain::from_ast_non_strict(&added_ast);
+                ast::Shifted::new(pattern_offset, ast)
+            }
+            Some(mut expression) => {
+                let new_argument = ast::prefix::Argument {
+                    sast:      ast::Shifted::new(
+                        pattern_offset.max(MINIMUM_PATTERN_OFFSET),
+                        added_ast,
+                    ),
+                    prefix_id: default(),
+                };
+                expression.args.push(new_argument);
+                expression
+            }
+        };


It looks like duplicated code, doesn't it? Perhaps there should be a function returning new_expression.

Refactored.

farmaazon · 2022-09-12T14:47:16Z

app/gui/src/controller/searcher.rs

+        let expr_and_method = || {
+            let input_chain = Some(new_expression);
+
+            let expression = match (self.this_var(), input_chain) {
+                (Some(this_var), Some(input)) =>
+                    apply_this_argument(this_var, &input.wrapped.into_ast()).repr(),
+                (None, Some(input)) => input.wrapped.into_ast().repr(),
+                (_, None) => "".to_owned(),
+            };
+            let intended_method = self.intended_method();
+            (expression, intended_method)
+        };
+        let (expression, intended_method) = expr_and_method();


Why are we doing it in lambda? I don't even see need for block...

This was taken from the original code in commit_node. But after doing the refactoring into a separate method, it is gone now.

farmaazon · 2022-09-12T14:48:01Z

app/gui/src/controller/searcher.rs

+
+        self.graph.graph().module.with_node_metadata(
+            self.mode.node_id(),
+            Box::new(|md| md.intended_method = intended_method),


Just to make sure: is this restored after editing aborting?

farmaazon · 2022-09-12T14:50:32Z

app/gui/src/presenter/graph/state.rs

+        tracing::debug!(
+            "Setting node expression from view: {} -> {}",
+            displayed.expression,
+            expression
+        );


you can make it 2 lines instead of 5 lines by extracting &str to var

I'm not sure if it has the same performance: the format macro can adjust allocated space when knows the string on compilation time.

farmaazon · 2022-09-13T12:51:10Z

app/gui/src/controller/searcher.rs

+        let input_chain = self.data.borrow().input.as_prefix_chain(self.ide.parser());
        // 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 (expression, intended_method) = expr_and_method();
+        let expression = self.get_expression(input_chain);
+        let intended_method = self.intended_method();
+


These lines are strangely mixed. I think the add_required_imports should go before variables which are not used by it.

farmaazon · 2022-09-13T12:53:00Z

app/gui/src/controller/searcher.rs

-            (expression, intended_method)
-        };
-        let (expression, intended_method) = expr_and_method();
+        let expression = self.get_expression(new_expression);


The name is unclear. Why, having new_expression we must yet get expression from it?

Additionally, I think new_expression construction is still duplicated.

…rowser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050 # Conflicts: # CHANGELOG.md

wdanilo · 2022-09-14T13:54:01Z

app/gui/src/controller/searcher.rs

@@ -49,6 +49,7 @@ pub const ASSIGN_NAMES_FOR_NODES: bool = true;
 const ENSO_PROJECT_SPECIAL_MODULE: &str =
    concatcp!(project::STANDARD_BASE_LIBRARY_PATH, ".Enso_Project");

+const MINIMUM_PATTERN_OFFSET: usize = 1;


No docs, and the name does not explain what it is.

wdanilo · 2022-09-14T13:54:28Z

app/gui/src/controller/searcher.rs

+                    sast:      ast::Shifted::new(
+                        pattern_offset.max(MINIMUM_PATTERN_OFFSET),
+                        added_ast,


export to var to make it shorter.

wdanilo · 2022-09-14T13:55:02Z

app/gui/src/controller/searcher.rs

+        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);


missing . at the end.

wdanilo · 2022-09-14T13:55:34Z

app/gui/src/controller/searcher.rs

+            Some(mut expression) => {
+                let new_argument = ast::prefix::Argument {
+                    sast:      ast::Shifted::new(
+                        pattern_offset.max(MINIMUM_PATTERN_OFFSET),
+                        added_ast,
+                    ),
+                    prefix_id: default(),
+                };
+                expression.args.push(new_argument);
+                Some(expression)


duplicated code - I've seen it above.

wdanilo · 2022-09-14T13:56:06Z

app/gui/src/controller/searcher.rs

+            self.mode.node_id(),
+            Box::new(|md| md.intended_method = intended_method),
+        )?;
+        tracing::debug!("Previewing expression: {:?}", expression);


Earlier, the code was enclosed in ", here it is not.

Missing dot.

wdanilo · 2022-09-14T13:57:24Z

app/gui/src/presenter/graph/state.rs

+        tracing::debug!(
+            "Setting node expression from controller: {} -> {}",
+            displayed.expression,
+            new_displayed_expr


export msg to var to make it shorter.

wdanilo · 2022-09-14T13:57:35Z

app/gui/src/presenter/graph/state.rs

+        tracing::debug!(
+            "Setting node expression from view: {} -> {}",
+            displayed.expression,
+            expression


export msg to var to make it shorter

wdanilo · 2022-09-14T13:58:03Z

app/gui/src/presenter/searcher.rs

@@ -120,7 +120,9 @@ impl Model {
    /// Should be called if an entry is selected but not used yet. Only used for the old searcher
    /// API.
    fn entry_selected_as_suggestion(&self, entry_id: view::searcher::entry::Id) {
-        self.controller.preview_entry_as_suggestion(entry_id);
+        if let Err(error) = self.controller.preview_entry_as_suggestion(entry_id) {
+            tracing::warn!("Failed to preview  entry {entry_id:?} because of error: {error:?}");


double space

Does it end with dot?

wdanilo · 2022-09-14T13:58:10Z

app/gui/src/presenter/searcher.rs

@@ -151,7 +153,9 @@ impl Model {
    /// Should be called if a suggestion is selected but not used yet.
    fn suggestion_selected(&self, entry_id: list_panel::EntryId) {
        let suggestion = self.suggestion_for_entry_id(entry_id).unwrap();
-        self.controller.preview_suggestion(suggestion);
+        if let Err(error) = self.controller.preview_suggestion(suggestion) {
+            tracing::warn!("Failed to preview suggestion {entry_id:?} because of error: {error:?}");


does it end with. dot?

…rowser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050

MichaelMauderer added 2 commits September 7, 2022 15:13

Implement working preview of selected item in Searcher.

b09c6ae

Update changelog.

c200c14

MichaelMauderer force-pushed the wip/michaelmauderer/Propagate_Component_Browser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050 branch from 2b7e26e to c200c14 Compare September 7, 2022 14:13

MichaelMauderer marked this pull request as ready for review September 7, 2022 14:27

MichaelMauderer requested review from wdanilo, farmaazon and mwu-tow as code owners September 7, 2022 14:27

Prettier.

0853b27

wdanilo requested changes Sep 8, 2022

View reviewed changes

MichaelMauderer self-assigned this Sep 8, 2022

MichaelMauderer requested a review from wdanilo September 9, 2022 08:39

wdanilo reviewed Sep 9, 2022

View reviewed changes

MichaelMauderer added 3 commits September 12, 2022 11:17

Implement review feedback.

33bd774

Implement review feedback.

3afd1ca

Implement review feedback.

4a8935e

MichaelMauderer requested a review from wdanilo September 12, 2022 10:28

farmaazon approved these changes Sep 12, 2022

View reviewed changes

Implement Adam's review feedback.

c85e8ac

MichaelMauderer requested a review from farmaazon September 13, 2022 11:50

farmaazon reviewed Sep 13, 2022

View reviewed changes

MichaelMauderer added 2 commits September 14, 2022 13:17

Code cleanup based on review feedback.

2794f3f

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

0c6151f

…rowser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050 # Conflicts: # CHANGELOG.md

MichaelMauderer mentioned this pull request Sep 14, 2022

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

Merged

4 tasks

wdanilo approved these changes Sep 14, 2022

View reviewed changes

MichaelMauderer added 2 commits September 15, 2022 11:27

Review feedback.

6babdff

Bump wasm-size.

27651d0

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

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

e13d48f

…rowser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050

mergify bot merged commit 545e1d7 into develop Sep 16, 2022

mergify bot deleted the wip/michaelmauderer/Propagate_Component_Browser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050 branch September 16, 2022 16:05

MichaelMauderer restored the wip/michaelmauderer/Propagate_Component_Browser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050 branch September 20, 2022 09:54

MichaelMauderer deleted the wip/michaelmauderer/Propagate_Component_Browser_Selections_to_Source_and_Disassociate_the_Node_That_Is_Edited_by_the_Searcher_From_any_Changes_Not_Made_by_the_Searcher_#182634050 branch December 9, 2022 10:48

wdanilo mentioned this pull request Feb 6, 2023

Propagate Component Browser Selections to Source and Disassociate the Node That Is Edited by the Searcher From any Changes Not Made by the Searcher #4560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show Visualisation Preview when Selecting Item on Searcher #3691

Show Visualisation Preview when Selecting Item on Searcher #3691

MichaelMauderer commented Sep 7, 2022 •

edited

Loading

wdanilo commented Sep 8, 2022

wdanilo left a comment

MichaelMauderer commented Sep 8, 2022 •

edited

Loading

wdanilo Sep 9, 2022

wdanilo Sep 9, 2022

wdanilo Sep 9, 2022

MichaelMauderer Sep 12, 2022

farmaazon Sep 12, 2022

wdanilo Sep 14, 2022

wdanilo Sep 9, 2022

farmaazon left a comment

farmaazon Sep 12, 2022

MichaelMauderer Sep 13, 2022

farmaazon Sep 12, 2022

MichaelMauderer Sep 13, 2022

farmaazon Sep 12, 2022

MichaelMauderer Sep 13, 2022

farmaazon Sep 12, 2022

farmaazon Sep 13, 2022

farmaazon Sep 13, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

wdanilo Sep 14, 2022

Show Visualisation Preview when Selecting Item on Searcher #3691

Show Visualisation Preview when Selecting Item on Searcher #3691

Conversation

MichaelMauderer commented Sep 7, 2022 • edited Loading

Pull Request Description

Checklist

wdanilo commented Sep 8, 2022

wdanilo left a comment

Choose a reason for hiding this comment

MichaelMauderer commented Sep 8, 2022 • edited Loading

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

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

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 7, 2022 •

edited

Loading

MichaelMauderer commented Sep 8, 2022 •

edited

Loading