enso-org/enso
1
1
Fork 0
mirror of https://github.com/enso-org/enso.git synced 2024-11-22 21:51:05 +03:00
Code Issues Projects Releases Wiki Activity

Implement hiding context switch expression in graph editor (#6136)

Browse Source 
Closes #5927

Now context switch expressions are not visible in the nodes.

No visual changes to the IDE, the screencast is recorded with code modifications.

https://user-images.githubusercontent.com/6566674/228579581-f6a4c214-af56-40f8-af8e-af102cf476b3.mp4

# Important Notes
I fixed an issue when the state of the buttons near nodes was not updated after loading a project. See https://github.com/enso-org/enso/pull/6136#discussion_r1156920052
This commit is contained in:
Ilya Bogdanov 2023-04-04 21:25:46 +04:00 committed by GitHub
parent 2e08f734d7
commit 2531aeeece
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 155 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/gui/controller/double-representation/src/context_switch.rs
View File

@ -150,6 +150,18 @@ impl ContextSwitchExpression {
let args = vec![context, environment];
ast::prefix::Chain::new(func, args).into_ast()
}
/// Remove the context switch expression from the given AST. The unmodified `ast` is returned
/// if it does not contain any context switch expression.
pub fn without_expression(ast: &Ast) -> Ast {
if ContextSwitchExpression::parse(ast).is_some() {
let crumb = ast::crumbs::InfixCrumb::RightOperand.into();
let rarg = ast.get(&crumb).unwrap_or(ast);
rarg.clone()
} else {
ast.clone()
}
}
}

82
app/gui/controller/double-representation/src/node.rs
View File

@ -150,6 +150,38 @@ pub fn locate_many<'a>(
// ===================
// === NodeAstInfo ===
// ===================
/// The information about AST content of the node.
#[derive(Debug, Clone, PartialEq, Default)]
pub struct NodeAstInfo {
/// Information about SKIP and FREEZE macros used in the code.
pub macros_info: MacrosInfo,
/// Existing context switch expression, if any.
pub context_switch: Option<ContextSwitchExpression>,
}
impl NodeAstInfo {
/// Constructor.
pub fn from_ast(ast: &Ast) -> Self {
let macros_info = MacrosInfo::from_ast(ast);
let without_macros = without_macros(ast);
let context_switch = ContextSwitchExpression::parse(&without_macros);
Self { macros_info, context_switch }
}
/// Specifies the count of AST crumbs to bypass in order to reach the displayed AST node.
pub fn ast_crumbs_to_skip(&self) -> usize {
let skip_for_context_switch_expr = self.context_switch.as_ref().map_or(0, |_| 1);
let skip_for_macros = self.macros_info.macros_count();
skip_for_macros + skip_for_context_switch_expr
}
}
// ================
// === MainLine ===
// ================
@ -161,14 +193,12 @@ pub fn locate_many<'a>(
/// must be set and it serves as the whole node's expression.
#[derive(Debug, Clone, Deref, DerefMut)]
pub struct MainLine {
/// Information about SKIP and FREEZE macros used in the code.
pub macros_info: MacrosInfo,
/// Existing context switch expression, if any.
pub context_switch: Option<ContextSwitchExpression>,
/// Node AST, contains a node's expression and an optional pattern binding.
#[deref]
#[deref_mut]
pub ast: NodeAst,
pub ast: NodeAst,
/// Additional information about the AST.
pub ast_info: NodeAstInfo,
}
impl MainLine {
@ -176,22 +206,18 @@ impl MainLine {
/// expression.
pub fn new_binding(infix: known::Infix) -> Option<MainLine> {
infix.rarg.id?;
let macros_info = MacrosInfo::from_ast(&infix.rarg);
let without_macros = without_macros(&infix.rarg);
let context_switch = ContextSwitchExpression::parse(&without_macros);
let ast_info = NodeAstInfo::from_ast(&infix.rarg);
let ast = NodeAst::Binding { infix };
Some(Self { macros_info, context_switch, ast })
Some(Self { ast, ast_info })
}
/// Tries to interpret AST as node, treating whole AST as an expression.
pub fn new_expression(ast: Ast) -> Option<MainLine> {
ast.id?;
let macros_info = MacrosInfo::from_ast(&ast);
let without_macros = without_macros(&ast);
let context_switch = ContextSwitchExpression::parse(&without_macros);
let ast_info = NodeAstInfo::from_ast(&ast);
let ast = NodeAst::Expression { ast };
// TODO what if we are given an assignment.
Some(Self { macros_info, context_switch, ast })
Some(Self { ast, ast_info })
}
/// Tries to interpret AST as node, treating whole AST as a node's primary line.
@ -314,7 +340,7 @@ impl NodeInfo {
/// The info about macro calls in the expression.
pub fn macros_info(&self) -> &MacrosInfo {
&self.main_line.macros_info
&self.main_line.ast_info.macros_info
}
// Modify AST, adding or removing `SKIP` macro call. Does nothing if [`skip`] argument already
@ -326,7 +352,7 @@ impl NodeInfo {
} else {
self.main_line.remove_skip_macro();
}
self.main_line.macros_info.skip = skip;
self.main_line.ast_info.macros_info.skip = skip;
}
}
@ -339,7 +365,7 @@ impl NodeInfo {
} else {
self.main_line.remove_freeze_macro();
}
self.main_line.macros_info.freeze = freeze;
self.main_line.ast_info.macros_info.freeze = freeze;
}
}
@ -352,7 +378,7 @@ impl NodeInfo {
/// Add context switch expression to the node. Replaces the existing one, if any.
pub fn set_context_switch(&mut self, context_switch_expr: ContextSwitchExpression) {
if self.main_line.context_switch.is_some() {
if self.main_line.ast_info.context_switch.is_some() {
self.clear_context_switch_expression();
}
self.main_line.modify_expression(|ast| {
@ -368,12 +394,12 @@ impl NodeInfo {
*ast = infix.into();
});
});
self.main_line.context_switch = Some(context_switch_expr);
self.main_line.ast_info.context_switch = Some(context_switch_expr);
}
/// Remove existing context switch expression from the node.
pub fn clear_context_switch_expression(&mut self) {
if self.main_line.context_switch.is_some() {
if self.main_line.ast_info.context_switch.is_some() {
self.main_line.modify_expression(|ast| {
*ast = preserving_skip_and_freeze(ast, |ast| {
if ContextSwitchExpression::parse(ast).is_some() {
@ -383,7 +409,7 @@ impl NodeInfo {
}
});
});
self.main_line.context_switch = None;
self.main_line.ast_info.context_switch = None;
}
}
}
@ -423,9 +449,11 @@ impl NodeAst {
};
}
/// Visible portion of the node's expression. Does not contain `SKIP` and `FREEZE` macro calls.
/// Represents the visible portion of a node's expression. This excludes SKIP and FREEZE macro
/// calls, as well as any context switch expressions.
pub fn expression(&self) -> Ast {
without_macros(self.whole_expression())
let ast = without_macros(self.whole_expression());
ContextSwitchExpression::without_expression(&ast)
}
/// AST of the node's expression. Typically no external user wants to access it directly. Use
@ -742,7 +770,7 @@ mod tests {
let line_ast = Ast::infix(larg, ASSIGNMENT, rarg);
let node = NodeInfo::from_main_line_ast(&line_ast).unwrap();
assert_eq!(node.repr(), "foo = bar");
assert!(node.main_line.context_switch.is_none());
assert!(node.main_line.ast_info.context_switch.is_none());
node
};
fn test_round_trip(
@ -752,11 +780,11 @@ mod tests {
) {
let original_repr = node.repr();
node.set_context_switch(context_switch.clone());
assert_eq!(node.main_line.context_switch, Some(context_switch));
assert_eq!(node.main_line.ast_info.context_switch, Some(context_switch));
assert_eq!(node.repr(), expected);
node.clear_context_switch_expression();
assert_eq!(node.repr(), original_repr);
assert!(node.main_line.context_switch.is_none());
assert!(node.main_line.ast_info.context_switch.is_none());
}
let expected = format!("foo = {ENABLE_CONTEXT} {OUTPUT_CONTEXT} \"design\" <| bar");
@ -835,10 +863,10 @@ mod tests {
context_switch = context_switch(),
);
assert_eq!(node.repr(), expected, "{case:?}");
assert!(node.main_line.context_switch.is_some());
assert!(node.main_line.ast_info.context_switch.is_some());
node.clear_context_switch_expression();
assert!(node.main_line.context_switch.is_none());
assert!(node.main_line.ast_info.context_switch.is_none());
let expected = format!(
"foo = {skip}{space}{freeze} bar",
space = if case.skip && case.freeze { " " } else { "" },

23
app/gui/src/controller/graph.rs
View File

@ -10,7 +10,6 @@ use crate::model::module::NodeMetadata;
use ast::crumbs::InfixCrumb;
use ast::crumbs::Located;
use ast::macros::skip_and_freeze::MacrosInfo;
use ast::macros::DocumentationCommentInfo;
use double_representation::connection;
use double_representation::context_switch::ContextSwitchExpression;
@ -22,6 +21,7 @@ use double_representation::module;
use double_representation::node;
use double_representation::node::MainLine;
use double_representation::node::NodeAst;
use double_representation::node::NodeAstInfo;
use double_representation::node::NodeInfo;
use double_representation::node::NodeLocation;
use engine_protocol::language_server;
@ -221,20 +221,20 @@ pub struct NodeTrees {
/// Describes node outputs, i.e. its pattern. `None` if a node is not an assignment.
pub outputs: Option<SpanTree>,
/// Info about macros used in the node's expression.
macros_info: MacrosInfo,
ast_info: NodeAstInfo,
}
impl NodeTrees {
#[allow(missing_docs)]
pub fn new(node: &NodeInfo, context: &impl SpanTreeContext) -> Option<NodeTrees> {
let inputs = SpanTree::new(&node.expression(), context).ok()?;
let macros_info = *node.macros_info();
let ast_info = node.main_line.ast_info.clone();
let outputs = if let Some(pat) = node.pattern() {
Some(SpanTree::new(pat, context).ok()?)
} else {
None
};
Some(NodeTrees { inputs, outputs, macros_info })
Some(NodeTrees { inputs, outputs, ast_info })
}
/// Converts AST crumbs (as obtained from double rep's connection endpoint) into the
@ -244,9 +244,10 @@ impl NodeTrees {
ast_crumbs: &'b [ast::Crumb],
) -> Option<span_tree::node::NodeFoundByAstCrumbs<'a, 'b>> {
use ast::crumbs::Crumb::Infix;
// If we have macros in the expression, we need to skip their crumbs, as [`SKIP`] and
// [`FREEZE`] macros are not displayed in the expression.
let skip_macros = self.macros_info.macros_count();
// We can display only a part of the expression to the user. We hide [`SKIP`] and [`FREEZE`]
// macros and context switch expressions. In this case, we skip an additional
// number of AST crumbs.
let expression_crumbs_to_skip = self.ast_info.ast_crumbs_to_skip();
if let Some(outputs) = self.outputs.as_ref() {
// Node in assignment form. First crumb decides which span tree to use.
let first_crumb = ast_crumbs.get(0);
@ -256,10 +257,10 @@ impl NodeTrees {
Some(Infix(InfixCrumb::RightOperand)) => Some(&self.inputs),
_ => None,
};
let skip = if is_input { skip_macros + 1 } else { 1 };
let skip = if is_input { expression_crumbs_to_skip + 1 } else { 1 };
tree.and_then(|tree| tree.root_ref().get_descendant_by_ast_crumbs(&ast_crumbs[skip..]))
} else {
let skip = skip_macros;
let skip = expression_crumbs_to_skip;
// Expression node - there is only inputs span tree.
self.inputs.root_ref().get_descendant_by_ast_crumbs(&ast_crumbs[skip..])
}
@ -1712,8 +1713,8 @@ main =
for (code, expected_name) in &cases {
let ast = parser.parse_line_ast(*code).unwrap();
let node = MainLine::from_ast(&ast).unwrap();
let name = Handle::variable_name_base_for(&node);
let node_info = NodeInfo::from_main_line_ast(&ast).unwrap();
let name = Handle::variable_name_base_for(&node_info);
assert_eq!(&name, expected_name);
}
}

8
app/gui/src/controller/searcher.rs
View File

@ -1201,7 +1201,7 @@ impl EditGuard {
),
Mode::EditNode { .. } => {
let node = self.graph.graph().node(self.node_id)?;
let previous_expression = node.info.main_line.expression().to_string();
let previous_expression = node.info.expression().to_string();
module.with_node_metadata(
self.node_id,
Box::new(|metadata| {
@ -2065,7 +2065,7 @@ pub mod test {
let Fixture { test: _test, mut searcher, .. } = Fixture::new();
let graph = searcher.graph.graph();
let node = graph.nodes().unwrap().last().unwrap().clone();
let initial_node_expression = node.main_line.expression();
let initial_node_expression = node.expression();
let node_id = node.info.id();
searcher.mode = Immutable(Mode::EditNode { node_id });
searcher.node_edit_guard =
@ -2103,7 +2103,7 @@ pub mod test {
// Verify the node was reverted.
let node = graph.nodes().unwrap().last().unwrap().clone();
let final_node_expression = node.main_line.expression();
let final_node_expression = node.expression();
assert_eq!(initial_node_expression.to_string(), final_node_expression.to_string());
}
@ -2126,7 +2126,7 @@ pub mod test {
// Verify the node is not reverted after the searcher is dropped.
drop(searcher);
let node = graph.nodes().unwrap().last().unwrap().clone();
let final_node_expression = node.main_line.expression();
let final_node_expression = node.expression();
assert_eq!(final_node_expression.to_string(), new_expression);
}

52
app/gui/src/presenter/graph.rs
View File

@ -2,6 +2,8 @@
//! about presenters in general.
use crate::prelude::*;
use double_representation::context_switch::Context;
use double_representation::context_switch::ContextSwitch;
use enso_web::traits::*;
use crate::controller::graph::widget::Request as WidgetRequest;
@ -9,8 +11,6 @@ use crate::controller::upload::NodeFromDroppedFileHandler;
use crate::executor::global::spawn_stream_handler;
use crate::presenter::graph::state::State;
use double_representation::context_switch::Context;
use double_representation::context_switch::ContextSwitch;
use double_representation::context_switch::ContextSwitchExpression;
use engine_protocol::language_server::SuggestionId;
use enso_frp as frp;
@ -168,7 +168,7 @@ impl Model {
/// TODO(#5930): Provide the state of the output context in the current environment.
fn output_context_enabled(&self) -> bool {
false
true
}
/// TODO(#5930): Provide the current execution environment of the project.
@ -512,27 +512,46 @@ impl Model {
/// Helper struct storing information about node's expression updates.
/// Includes not only the updated expression, but also an information about `SKIP` and
/// `FREEZE` macros updates.
/// `FREEZE` macros updates, and also execution context updates.
#[derive(Clone, Debug, Default)]
struct ExpressionUpdate {
id: ViewNodeId,
expression: node_view::Expression,
skip_updated: Option<bool>,
freeze_updated: Option<bool>,
id: ViewNodeId,
expression: node_view::Expression,
skip_updated: Option<bool>,
freeze_updated: Option<bool>,
context_switch_updated: Option<Option<ContextSwitchExpression>>,
}
impl ExpressionUpdate {
/// The updated expression.
fn expression(&self) -> (ViewNodeId, node_view::Expression) {
(self.id, self.expression.clone())
}
/// An updated status of `SKIP` macro. `None` if the status was not updated.
fn skip(&self) -> Option<(ViewNodeId, bool)> {
self.skip_updated.map(|skip| (self.id, skip))
}
/// An updated status of `FREEZE` macro. `None` if the status was not updated.
fn freeze(&self) -> Option<(ViewNodeId, bool)> {
self.freeze_updated.map(|freeze| (self.id, freeze))
}
/// An updated status of output context switch (`true` if output context is explicitly enabled
/// for the node, `false` otherwise). `None` if the status was not updated.
fn output_context(&self) -> Option<(ViewNodeId, bool)> {
self.context_switch_updated.as_ref().map(|context_switch_expr| {
use Context::*;
use ContextSwitch::*;
let enabled = match context_switch_expr {
Some(ContextSwitchExpression { switch: Enable, context: Output, .. }) => true,
Some(ContextSwitchExpression { switch: Disable, context: Output, .. }) => false,
None => false,
};
(self.id, enabled)
})
}
}
@ -587,7 +606,14 @@ impl ViewUpdate {
if let Some((id, expression)) = change.set_node_expression(node, trees) {
let skip_updated = change.set_node_skip(node);
let freeze_updated = change.set_node_freeze(node);
Some(ExpressionUpdate { id, expression, skip_updated, freeze_updated })
let context_switch_updated = change.set_node_context_switch(node);
Some(ExpressionUpdate {
id,
expression,
skip_updated,
freeze_updated,
context_switch_updated,
})
} else {
None
}
@ -701,6 +727,12 @@ impl Graph {
update_node_expression <- expression_update.map(ExpressionUpdate::expression);
set_node_skip <- expression_update.filter_map(ExpressionUpdate::skip);
set_node_freeze <- expression_update.filter_map(ExpressionUpdate::freeze);
// TODO(#5930): Use project model to retrieve a current state of the output context.
output_context_enabled <- update_view.constant(true);
output_context_updated <- expression_update.filter_map(ExpressionUpdate::output_context);
_context_switch_highlighted <- output_context_updated.map2(&output_context_enabled,
|(node_id, enabled_for_node), enabled_globally| (*node_id, enabled_for_node != enabled_globally)
);
set_node_position <= update_data.map(|update| update.set_node_positions());
set_node_visualization <= update_data.map(|update| update.set_node_visualizations());
enable_vis <- set_node_visualization.filter_map(|(id,path)| path.is_some().as_some(*id));
@ -709,6 +741,8 @@ impl Graph {
view.set_node_expression <+ update_node_expression;
view.set_node_skip <+ set_node_skip;
view.set_node_freeze <+ set_node_freeze;
// TODO (#5929): Connect to the view when the API is ready.
// view.highlight_output_context_switch <+ context_switch_highlighted;
view.set_node_position <+ set_node_position;
view.set_visualization <+ set_node_visualization;
view.enable_visualization <+ enable_vis;

32
app/gui/src/presenter/graph/state.rs
View File

@ -454,14 +454,19 @@ impl<'a> ControllerChange<'a> {
let new_displayed_expr = node_view::Expression {
pattern: node.info.pattern().map(|t| t.repr()),
code: node.info.expression().repr().into(),
whole_expression_id: node.info.expression().id,
whole_expression_id: Some(node.info.id()),
input_span_tree: trees.inputs,
output_span_tree: trees.outputs.unwrap_or_else(default),
};
let mut nodes = self.nodes.borrow_mut();
let displayed = nodes.get_mut_or_create(ast_id);
if displayed.expression != new_displayed_expr {
let displayed_updated = displayed.expression != new_displayed_expr;
let context_switch_updated = displayed.context_switch != node.info.ast_info.context_switch;
let skip_updated = displayed.is_skipped != node.info.macros_info().skip;
let freeze_updated = displayed.is_frozen != node.info.macros_info().freeze;
if displayed_updated || context_switch_updated || skip_updated || freeze_updated {
debug!(
"Setting node expression from controller: {} -> {}",
displayed.expression, new_displayed_expr
@ -506,6 +511,27 @@ impl<'a> ControllerChange<'a> {
}
}
/// Check if context switch expression is present in the expression and return it.
/// Returns a nested option:
/// - `None` if no changes to the state are needed.
/// - `Some(None)` if the expression was removed.
/// - `Some(Some(_))` if the expression was added.
pub fn set_node_context_switch(
&self,
node: &controller::graph::Node,
) -> Option<Option<ContextSwitchExpression>> {
let ast_id = node.main_line.id();
let mut nodes = self.nodes.borrow_mut();
let displayed = nodes.get_mut_or_create(ast_id);
let expr = node.info.ast_info.context_switch.clone();
if displayed.context_switch != expr {
displayed.context_switch = expr.clone();
Some(expr)
} else {
None
}
}
/// Set the node error basing of the given expression's payload. If the error is actually
/// changed, the to-be-updated node view is returned with the proper error description. If the
/// expression is not a whole expression of any node, nothing is updated and `None` is returned.
@ -1033,7 +1059,7 @@ mod tests {
use ast::crumbs::InfixCrumb;
let Fixture { state, nodes } = Fixture::setup_nodes(&["2 + 3"]);
let view = nodes[0].view;
let node_ast = nodes[0].node.main_line.expression();
let node_ast = nodes[0].node.expression();
let left_operand = node_ast.get(&InfixCrumb::LeftOperand.into()).unwrap().id.unwrap();
let right_operand = node_ast.get(&InfixCrumb::RightOperand.into()).unwrap().id.unwrap();
let updater = state.update_from_controller();