Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show Visualisation Preview when Selecting Item on Searcher #3691

4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@
new node is created.][3645]
- [IDE uses new visualization API.][3661]
- [Visualization of long textual values improved][3665]
- [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]

#### EnsoGL (rendering engine)

Expand Down Expand Up @@ -305,6 +308,7 @@
[3647]: https://github.com/enso-org/enso/pull/3647
[3673]: https://github.com/enso-org/enso/pull/3673
[3684]: https://github.com/enso-org/enso/pull/3684
[3691]: https://github.com/enso-org/enso/pull/3691

#### Enso Compiler

Expand Down
72 changes: 66 additions & 6 deletions app/gui/src/controller/searcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



// ==============
Expand Down Expand Up @@ -662,7 +663,10 @@ impl Searcher {
}
Some(mut expression) => {
let new_argument = ast::prefix::Argument {
sast: ast::Shifted::new(pattern_offset.max(1), added_ast),
sast: ast::Shifted::new(
pattern_offset.max(MINIMUM_PATTERN_OFFSET),
added_ast,
Comment on lines +684 to +686
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export to var to make it shorter.

),
prefix_id: default(),
};
expression.args.push(new_argument);
Expand Down Expand Up @@ -696,15 +700,71 @@ impl Searcher {
}

/// Preview the suggestion in the searcher.
pub fn preview_entry_as_suggestion(&self, index: usize) {
pub fn preview_entry_as_suggestion(&self, index: usize) -> FallibleResult {
tracing::debug!("Previewing entry: {:?}", index);
//TODO[MM] the actual functionality here will be implemented as part of task #182634050.
let error = || NoSuchAction { index };
let suggestion = {
let data = self.data.borrow();
let list = data.actions.list().ok_or_else(error)?;
list.get_cloned(index).ok_or_else(error)?.action
};
if let Action::Suggestion(picked_suggestion) = suggestion {
self.preview_suggestion(picked_suggestion)?;
};

Ok(())
}

/// Use action at given index as a suggestion. The exact outcome depends on the action's type.
pub fn preview_suggestion(&self, selected_suggestion: action::Suggestion) {
//TODO[MM] the actual functionality here will be implemented as part of task #182634050.
tracing::debug!("Previewing suggestion: {:?}", selected_suggestion);
pub fn preview_suggestion(&self, picked_suggestion: action::Suggestion) -> FallibleResult {
tracing::debug!("Previewing suggestion: {:?}", picked_suggestion);

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing . at the end.

let added_ast = self.ide.parser().parse_line_ast(&code_to_insert)?;
let pattern_offset = self.data.borrow().input.pattern_offset;
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
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it is.

)?;
tracing::debug!("Previewing expression: {:?}", expression);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Earlier, the code was enclosed in ", here it is not.
  2. Missing dot.


self.graph.graph().set_expression(self.mode.node_id(), expression)?;

Ok(())
}

/// Execute given action.
Expand Down
22 changes: 15 additions & 7 deletions app/gui/src/presenter/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl Model {

/// Node position was changed in view.
fn node_position_changed(&self, id: ViewNodeId, position: Vector2) {
self.update_ast(
self.log_action(
|| {
let ast_id = self.state.update_from_view().set_node_position(id, position)?;
Some(self.controller.graph().set_node_position(ast_id, position))
Expand All @@ -125,7 +125,7 @@ impl Model {
}

fn node_visualization_changed(&self, id: ViewNodeId, path: Option<visualization_view::Path>) {
self.update_ast(
self.log_action(
|| {
let ast_id =
self.state.update_from_view().set_node_visualization(id, path.clone())?;
Expand All @@ -141,9 +141,15 @@ impl Model {
);
}

/// Node expression was edited in the view. Should be called whenever the user changes the
/// contents of a node during editing.
fn node_expression_set(&self, id: ViewNodeId, expression: String) {
self.state.update_from_view().set_node_expression(id, expression);
}

/// Node was removed in view.
fn node_removed(&self, id: ViewNodeId) {
self.update_ast(
self.log_action(
|| {
let ast_id = self.state.update_from_view().remove_node(id)?;
Some(self.controller.graph().remove_node(ast_id))
Expand All @@ -154,7 +160,7 @@ impl Model {

/// Connection was created in view.
fn new_connection_created(&self, id: ViewConnection) {
self.update_ast(
self.log_action(
|| {
let connection = self.view.model.edges.get_cloned_ref(&id)?;
let ast_to_create = self.state.update_from_view().create_connection(connection)?;
Expand All @@ -166,7 +172,7 @@ impl Model {

/// Connection was removed in view.
fn connection_removed(&self, id: ViewConnection) {
self.update_ast(
self.log_action(
|| {
let ast_to_remove = self.state.update_from_view().remove_connection(id)?;
Some(self.controller.disconnect(&ast_to_remove))
Expand All @@ -176,7 +182,7 @@ impl Model {
}

fn nodes_collapsed(&self, collapsed: &[ViewNodeId]) {
self.update_ast(
self.log_action(
|| {
debug!(self.logger, "Collapsing node.");
let ids = collapsed.iter().filter_map(|node| self.state.ast_node_id_of_view(*node));
Expand All @@ -190,7 +196,7 @@ impl Model {
);
}

fn update_ast<F>(&self, f: F, action: &str)
fn log_action<F>(&self, f: F, action: &str)
where F: FnOnce() -> Option<FallibleResult> {
if let Some(Err(err)) = f() {
error!(self.logger, "Failed to {action} in AST: {err}");
Expand Down Expand Up @@ -550,6 +556,7 @@ impl Graph {
eval view.on_edge_endpoint_unset(((edge_id,_)) model.connection_removed(*edge_id));
eval view.nodes_collapsed(((nodes, _)) model.nodes_collapsed(nodes));
eval view.enabled_visualization_path(((node_id, path)) model.node_visualization_changed(*node_id, path.clone()));
eval view.node_expression_set(((node_id, expression)) model.node_expression_set(*node_id, expression.clone()));


// === Dropping Files ===
Expand Down Expand Up @@ -617,6 +624,7 @@ impl Graph {
/// content of a node. For example, the searcher uses this to allow the edit field to have a
/// preview that is different from the actual node AST.
pub fn allow_expression_auto_updates(&self, id: AstNodeId, allow: bool) {
tracing::debug!("Setting auto updates for {id:?} to {allow}");
self.model.state.allow_expression_auto_updates(id, allow);
}
}
Expand Down
21 changes: 21 additions & 0 deletions app/gui/src/presenter/graph/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,12 @@ impl<'a> ControllerChange<'a> {
};
let mut nodes = self.nodes.borrow_mut();
let displayed = nodes.get_mut_or_create(ast_id);
tracing::debug!(
"Setting node expression from controller: {} -> {}",
displayed.expression,
new_displayed_expr
Comment on lines +447 to +450
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export msg to var to make it shorter.

);

if displayed.expression != new_displayed_expr {
displayed.expression = new_displayed_expr.clone();
let new_expressions =
Expand Down Expand Up @@ -650,6 +656,21 @@ impl<'a> ViewChange<'a> {
None
}
}

/// Set the node expression.
pub fn set_node_expression(&self, id: ViewNodeId, expression: String) -> Option<AstNodeId> {
let mut nodes = self.nodes.borrow_mut();
let ast_id = nodes.ast_id_of_view(id)?;
let displayed = nodes.get_mut(ast_id)?;
let expression = node_view::Expression::new_plain(expression);
tracing::debug!(
"Setting node expression from view: {} -> {}",
displayed.expression,
expression
Comment on lines +666 to +669
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export msg to var to make it shorter

);
Comment on lines +666 to +670
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let expression_has_changed = displayed.expression != expression;
expression_has_changed.as_some(ast_id)
}
}


Expand Down
10 changes: 8 additions & 2 deletions app/gui/src/presenter/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,15 @@ impl Model {
fn editing_aborted(&self) {
let searcher = self.searcher.take();
if let Some(searcher) = searcher {
searcher.abort_editing();
let input_node_view = searcher.input_view();
if let Some(node) = self.graph.ast_node_of_view(input_node_view) {
self.graph.allow_expression_auto_updates(node, true);
searcher.abort_editing();
} else {
tracing::warn!("When porting editing the AST node of the node view {input_node_view} could not be found.");
}
} else {
warning!(self.logger, "Editing aborted without searcher controller.");
tracing::warn!("Editing aborted without searcher controller.");
}
}

Expand Down
21 changes: 17 additions & 4 deletions app/gui/src/presenter/searcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:?}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. double space
  2. Does it end with dot?

}
}

fn commit_editing(&self, entry_id: Option<view::searcher::entry::Id>) -> Option<AstNodeId> {
Expand Down Expand Up @@ -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:?}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it end with. dot?

}
}

fn suggestion_accepted(
Expand Down Expand Up @@ -404,7 +408,6 @@ impl Searcher {
let created_node = graph_controller.add_node(new_node)?;

graph.assign_node_view_explicitly(input, created_node);
graph.allow_expression_auto_updates(created_node, false);

let source_node = source_node.and_then(|id| graph.ast_node_of_view(id.node));

Expand All @@ -425,14 +428,19 @@ impl Searcher {
let SearcherParams { input, .. } = parameters;
let ast_node = graph.ast_node_of_view(input);

match ast_node {
let mode = match ast_node {
Some(node_id) => Ok(Mode::EditNode { node_id }),
None => {
let (new_node, source_node) =
Self::create_input_node(parameters, graph, graph_editor, graph_controller)?;
Ok(Mode::NewNode { node_id: new_node, source_node })
}
};
let target_node = mode.as_ref().map(|mode| mode.node_id());
if let Ok(target_node) = target_node {
graph.allow_expression_auto_updates(target_node, false);
}
mode
}

/// Setup new, appropriate searcher controller for the edition of `node_view`, and construct
Expand Down Expand Up @@ -509,6 +517,11 @@ impl Searcher {
/// editing finishes.
pub fn abort_editing(self) {}

/// Returns the node view that is being edited by the searcher.
pub fn input_view(&self) -> ViewNodeId {
self.model.input_view
}

/// Returns true if the entry under given index is one of the examples.
pub fn is_entry_an_example(&self, entry: view::searcher::entry::Id) -> bool {
use crate::controller::searcher::action::Action::Example;
Expand Down
1 change: 0 additions & 1 deletion app/gui/view/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,6 @@ impl View {
graph.deselect_all_nodes();
graph.select_node(node);
});
eval adding_aborted ((node) graph.remove_node(node));


// === Editing ===
Expand Down