Skip to content

Commit

Permalink
Undo redo fixes and improvements (#6950)
Browse files Browse the repository at this point in the history 
This PR adds facilities for controllers to be aware of what shortcut command is currently being processed. This allows grouping consequences of single user action into a single transaction without hard-coding it separately for all the separate paths case-by-case, which turned out to be challenging and error-prone.

Additionally, a number of minor fixes were carried over from #6877:
* workaround for #6718;
* avoiding creating spurious transactions when dealing with node positions;
* dropping any non-user user-triggered transactions that occur during the IDE project initialization.
  • Loading branch information
@mwu-tow
mwu-tow authored Jun 12, 2023
1 parent 174392d commit a31c671
Show file tree
Hide file tree
Showing 19 changed files with 407 additions and 133 deletions.
11 changes: 10 additions & 1 deletion app/gui/language/parser/src/translation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,9 +471,18 @@ impl Translate {

/// Translate a [`tree::BodyBlock`] into an [`Ast`] module.
fn translate_module(&mut self, block: &tree::BodyBlock) -> Ast {
// FIXME [mwu]
// The following code was changed as a workaround for the issue
// https://github.com/enso-org/enso/issues/6718.
// This makes the GUI follow the incorrect Engine behavior of assigning ID to the root
// `Module` AST node. It should have no ID, as it could collide with other node in case of
// trivial module code (like `foo`). See also: https://github.com/enso-org/enso/issues/2262
// In this case the workaround is safe, as GUI will never generate such a trivial module,
// it will contain at least the `main` function definition.
let module_builder = self.start_ast();
let (lines, _) =
self.translate_block_lines(&block.statements).unwrap_or_default().expect_unspaced();
Ast::new_no_id(ast::Module { lines })
self.finish_ast(ast::Module { lines }, module_builder)
}

/// Translate the lines of [`Tree`] block into the [`Ast`] block representation.
Expand Down
130 changes: 94 additions & 36 deletions app/gui/src/controller/module.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,48 +209,106 @@ mod test {
let ls = language_server::Connection::new_mock_rc(default());
let parser = Parser::new();
let location = Path::from_mock_module_name("Test");
let code = "2+2";
let uuid1 = Uuid::new_v4();
let uuid2 = Uuid::new_v4();
let uuid3 = Uuid::new_v4();
let uuid4 = Uuid::new_v4();

let mut index: u128 = 0;
let mut next_index = || {
index += 1;
Uuid::from_u128(index)
};
// Note: we need a leading empty line to avoid encountering AST id collision, see:
// https://github.com/enso-org/enso/issues/6718
let code = "\nmain = \n 2+2";
// FIXME: [mwu] Module id should be removed once
// https://github.com/enso-org/enso/issues/6718 is fixed.
let module_id = next_index();
let assign_infix_id = next_index();
let var_id = next_index();
let assign_opr_id = next_index();
let block_id = next_index();
let sum_infix_id = next_index();
let first_number_id = next_index();
let add_opr_id = next_index();
let second_number_id = next_index();

let id_map = ast::IdMap::new(vec![
((0.byte()..1.byte()).into(), uuid1),
((1.byte()..2.byte()).into(), uuid2),
((2.byte()..3.byte()).into(), uuid3),
((0.byte()..3.byte()).into(), uuid4),
((0.byte()..code.len().byte()).into(), module_id),
((1.byte()..code.len().byte()).into(), assign_infix_id),
((1.byte()..5.byte()).into(), var_id),
((6.byte()..7.byte()).into(), assign_opr_id),
((8.byte()..code.len().byte()).into(), block_id),
// next final 3 bytes
((13.byte()..16.byte()).into(), sum_infix_id),
((13.byte()..14.byte()).into(), first_number_id),
((14.byte()..15.byte()).into(), add_opr_id),
((15.byte()..16.byte()).into(), second_number_id),
]);
let controller =
Handle::new_mock(location, code, id_map, ls, parser, default()).unwrap();

// Change code from "2+2" to "22+2"
let change = enso_text::Change::inserted(0.byte(), "2".to_string());
// Change code from "\nmain = \n 2+2" to "\nmain = \n 22+2";
let change = enso_text::Change::inserted(13.byte(), "2".to_string());
controller.apply_code_change(change).unwrap();
let expected_ast = Ast::new_no_id(ast::Module {
lines: vec![BlockLine {
elem: Some(Ast::new(
ast::Infix {
larg: Ast::new(
ast::Number { base: None, int: "22".to_string() },
Some(uuid1),
),
loff: 0,
opr: Ast::new(
ast::Opr { name: "+".to_string(), right_assoc: false },
Some(uuid2),
),
roff: 0,
rarg: Ast::new(
ast::Number { base: None, int: "2".to_string() },
Some(uuid3),
),
},
Some(uuid4),
)),
off: 0,
}],
});
assert_eq!(expected_ast, controller.model.ast().into());
let resulting_ast: Ast = controller.model.ast().into();
let expected_ast = Ast::new(
ast::Module {
lines: vec![BlockLine { elem: None, off: 0 }, BlockLine {
elem: Some(Ast::new(
ast::Infix {
larg: Ast::new(ast::Var { name: "main".to_string() }, Some(var_id)),
loff: 1,
opr: Ast::new(
ast::Opr { name: "=".to_string(), right_assoc: true },
Some(assign_opr_id),
),
roff: 1,
rarg: Ast::new(
ast::Block {
indent: 4,
empty_lines: vec![],
first_line: BlockLine {
elem: Ast::new(
ast::Infix {
larg: Ast::new(
ast::Number {
base: None,
int: "22".to_string(),
},
Some(first_number_id),
),
loff: 0,
opr: Ast::new(
ast::Opr {
name: "+".to_string(),
right_assoc: false,
},
Some(add_opr_id),
),
roff: 0,
rarg: Ast::new(
ast::Number {
base: None,
int: "2".to_string(),
},
Some(second_number_id),
),
},
Some(sum_infix_id),
),
off: 0,
},
lines: vec![],
},
Some(block_id),
),
},
Some(assign_infix_id),
)),
off: 0,
}],
},
Some(module_id),
);
assert_eq!(expected_ast, resulting_ast);
});
}
}
25 changes: 20 additions & 5 deletions app/gui/src/controller/searcher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,10 @@ impl Searcher {
/// Otherwise it will be just the current searcher input.
pub fn preview(&self, suggestion: Option<action::Suggestion>) -> FallibleResult {
let transaction_name = "Previewing Component Browser suggestion.";
let _skip = self.graph.undo_redo_repository().open_ignored_transaction(transaction_name);
let _skip = self
.graph
.undo_redo_repository()
.open_ignored_transaction_or_ignore_current(transaction_name);

debug!("Updating node preview. Previewed suggestion: \"{suggestion:?}\".");
self.clear_temporary_imports();
Expand Down Expand Up @@ -898,7 +901,10 @@ impl Searcher {

fn clear_temporary_imports(&self) {
let transaction_name = "Clearing temporary imports after closing searcher.";
let _skip = self.graph.undo_redo_repository().open_ignored_transaction(transaction_name);
let _skip = self
.graph
.undo_redo_repository()
.open_ignored_transaction_or_ignore_current(transaction_name);
let mut module = self.module();
let import_metadata = self.graph.graph().module.all_import_metadata();
let metadata_to_remove = import_metadata
Expand Down Expand Up @@ -1163,7 +1169,10 @@ impl EditGuard {
/// be restored.
fn save_node_expression_to_metadata(&self, mode: &Mode) -> FallibleResult {
let transaction_name = "Storing edited node original expression.";
let _skip = self.graph.undo_redo_repository().open_ignored_transaction(transaction_name);
let _skip = self
.graph
.undo_redo_repository()
.open_ignored_transaction_or_ignore_current(transaction_name);
let module = &self.graph.graph().module;
match mode {
Mode::NewNode { .. } => module.with_node_metadata(
Expand All @@ -1188,7 +1197,10 @@ impl EditGuard {
/// Mark the node as no longer edited and discard the edit metadata.
fn clear_node_edit_metadata(&self) -> FallibleResult {
let transaction_name = "Storing edited node original expression.";
let _skip = self.graph.undo_redo_repository().open_ignored_transaction(transaction_name);
let _skip = self
.graph
.undo_redo_repository()
.open_ignored_transaction_or_ignore_current(transaction_name);
let module = &self.graph.graph().module;
module.with_node_metadata(
self.node_id,
Expand All @@ -1205,7 +1217,10 @@ impl EditGuard {

fn revert_node_expression_edit(&self) -> FallibleResult {
let transaction_name = "Reverting node expression to original.";
let _skip = self.graph.undo_redo_repository().open_ignored_transaction(transaction_name);
let _skip = self
.graph
.undo_redo_repository()
.open_ignored_transaction_or_ignore_current(transaction_name);
let edit_status = self.get_saved_expression()?;
match edit_status {
None => {
Expand Down
16 changes: 15 additions & 1 deletion app/gui/src/controller/upload.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,13 +197,25 @@ impl NodeFromDroppedFileHandler {
file: FileToUpload<impl DataProvider + 'static>,
position: Position,
) -> FallibleResult {
let _transaction = self
.graph
.module
.get_or_open_transaction(&format!("Create a node from dropped file '{}'.", file.name));
let node = self.graph.add_node(Self::new_node_info(&file, position))?;
let this = self.clone_ref();
executor::global::spawn(async move {
if let Err(err) = this.upload_file(node, file).await {
error!("Error while uploading file: {err}");
this.update_metadata(node, |md| md.error = Some(err.to_string()));
}
// TODO [mwu]: In general this does play good with undo/redo. The transaction will get
// dropped as soon as the asynchronous task is spawned, which means that the
// post-completion steps will end up in a different transaction.
// This is a case where text-snapshot-based undo/redo model does not play nicely
// with asynchronous text edits being made.
// As the file upload (particularly with cloud backend) can take a long time, we
// do not try to fix this by simply holding the transaction alive until the upload
// is finished.
});
Ok(())
}
Expand Down Expand Up @@ -263,7 +275,9 @@ impl NodeFromDroppedFileHandler {

fn update_metadata(&self, node: ast::Id, f: impl FnOnce(&mut UploadingFile)) {
//TODO[ao] see the TODO comment in update_expression.
let _tr = self.undo_redo_repository().open_ignored_transaction("Upload Metadata Update");
let _tr = self
.undo_redo_repository()
.open_ignored_transaction_or_ignore_current("Upload Metadata Update");
let update_md = Box::new(|md: &mut NodeMetadata| {
if let Some(uploading_md) = &mut md.uploading_file {
f(uploading_md)
Expand Down
29 changes: 27 additions & 2 deletions app/gui/src/model/module/plain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ mod test {

use enso_text::index::*;

#[wasm_bindgen_test]
#[test]
fn applying_code_change() {
let _test = TestWithLocalPoolExecutor::set_up();
let module = model::module::test::plain_from_code("2 + 2");
Expand All @@ -398,7 +398,7 @@ mod test {
assert_eq!("2 - abc", module.ast().repr());
}

#[wasm_bindgen_test]
#[test]
fn notifying() {
let mut test = TestWithLocalPoolExecutor::set_up();
let module = model::module::test::plain_from_code("");
Expand Down Expand Up @@ -480,4 +480,29 @@ mod test {
.unwrap();
assert_eq!(Some(new_pos), module.node_metadata(id).unwrap().position);
}

#[test]
fn test_metadata_stability() {
// TODO [mwu]
// This tests makes sure that the program included below properly round-trips through
// AST <-> Text conversions. This is specifically for testing workaround for the bug
// https://github.com/enso-org/enso/issues/6718. After proper fix is implemented, this
// test should be removed or accordingly modified (remove the module ast id).
const PROGRAM: &str = r#"from Standard.Base import all
from Standard.Table import all
from Standard.Database import all
from Standard.AWS import all
import Standard.Visualization
main =
operator1 = "Press TAB key to create a new node"
#### METADATA ####
[[{"index":{"value":5},"size":{"value":8}},"2237ae5e-903c-4a9f-9fd0-fc38b8042d7c"],[{"index":{"value":13},"size":{"value":1}},"5dd326f5-5914-401c-8ef3-517ba857d6aa"],[{"index":{"value":14},"size":{"value":4}},"b928d231-d0da-4349-b1d5-e02915ca9d4a"],[{"index":{"value":5},"size":{"value":13}},"42c59577-b318-4d87-bfe2-45db95b58be9"],[{"index":{"value":0},"size":{"value":29}},"50648f66-fa28-4585-b48e-fa742d0b7e06"],[{"index":{"value":35},"size":{"value":8}},"9d1366e1-7e1d-45fb-9eb0-c8ff1a9e86c5"],[{"index":{"value":43},"size":{"value":1}},"8ee6b8fc-dee5-4544-ae2e-b99539f1433d"],[{"index":{"value":44},"size":{"value":5}},"21832d23-bc1f-4801-8575-4001e33b94b6"],[{"index":{"value":35},"size":{"value":14}},"f0266d43-7e5b-497a-9ad0-58be68948d0a"],[{"index":{"value":30},"size":{"value":30}},"5291f4d1-3b72-49e9-bbae-9a66c31ae53e"],[{"index":{"value":66},"size":{"value":8}},"6234fc57-8953-4477-afa3-269b8217bc0f"],[{"index":{"value":74},"size":{"value":1}},"0b2dea39-fa5f-4234-9e12-dc2aee306fd4"],[{"index":{"value":75},"size":{"value":8}},"459efd89-3b04-4f0c-b43b-c9f1a14d53e9"],[{"index":{"value":66},"size":{"value":17}},"a001484a-fec9-48e8-a1b4-259d3b8884ba"],[{"index":{"value":61},"size":{"value":33}},"c80868fb-9f2f-41dc-ab45-33f9536462fc"],[{"index":{"value":100},"size":{"value":8}},"f5ee7eec-897d-489c-9c86-3ee9f40769cf"],[{"index":{"value":108},"size":{"value":1}},"0065ad69-4045-4c67-80a8-a723de221dcc"],[{"index":{"value":109},"size":{"value":3}},"83f63951-e783-4c04-835f-f4842cd19aa5"],[{"index":{"value":100},"size":{"value":12}},"1b119185-e823-429e-bc0a-e4e67d6b2619"],[{"index":{"value":95},"size":{"value":28}},"85fbfdc1-666d-47df-bcc9-f7a4653774f0"],[{"index":{"value":131},"size":{"value":8}},"aaa27a07-8bf9-4015-889e-7f3ad93ba83a"],[{"index":{"value":139},"size":{"value":1}},"2f16eb27-c7e5-4def-8982-9c8d3bcff8be"],[{"index":{"value":140},"size":{"value":13}},"b9bd5554-eb2d-4d60-99c6-e9796abbde3a"],[{"index":{"value":131},"size":{"value":22}},"e828a1d8-90f4-4350-8144-8e0c85448c33"],[{"index":{"value":124},"size":{"value":29}},"9ec14bd9-bff9-4485-96b0-0dd2b7c711a1"],[{"index":{"value":155},"size":{"value":4}},"de34419e-7ab4-4080-aad4-26a0960fc7b0"],[{"index":{"value":160},"size":{"value":1}},"53c348a5-a048-4f25-b1b0-43aa4f4bdddb"],[{"index":{"value":166},"size":{"value":9}},"ce3e86b8-9106-4ecf-9b35-a064ce435162"],[{"index":{"value":176},"size":{"value":1}},"4b428a34-3e14-4c89-9fea-7fb8b0c8d010"],[{"index":{"value":178},"size":{"value":36}},"f32c12eb-0fab-4e9b-ace6-718c34ad5726"],[{"index":{"value":166},"size":{"value":48}},"8ee5a81e-5428-4b18-8b52-5c4f66f77792"],[{"index":{"value":161},"size":{"value":53}},"1f9ab84d-6918-4245-8fdc-2b4c435f3252"],[{"index":{"value":155},"size":{"value":59}},"5938d821-22bc-4408-94b6-2f1f70f6fdc6"],[{"index":{"value":0},"size":{"value":215}},"16bb0e35-6793-49e7-b850-2b23d02a377e"]]
{"ide":{"node":{"f32c12eb-0fab-4e9b-ace6-718c34ad5726":{"position":{"vector":[-116.0,105.0]},"intended_method":null,"uploading_file":null,"selected":false,"visualization":{"name":{"content":{"content":"JSON"}},"project":"Builtin"}}},"import":{},"project":null}}"#;
let ast = Parser::new().parse_with_metadata::<Metadata>(PROGRAM);
assert_eq!(ast.to_string(), PROGRAM);
}
}
Loading

0 comments on commit a31c671

Please sign in to comment.