From 2b888e1d30c5f1876cedd1ddac18bb050a568ae2 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Tue, 22 Jul 2025 10:45:42 -0500 Subject: [PATCH] Fix redo after noop format (#34898) Closes #31917 Previously, as of #28457 we used a hack, creating an empty transaction in the history that we then merged formatting changes into in order to correctly identify concurrent edits to the buffer while formatting was happening. This caused issues with noop formatting however, as using the normal API of the buffer history (in an albeit weird way) resulted in the redo stack being cleared, regardless of whether the formatting transaction included edits or not, which is the correct behavior in all other contexts. This PR fixes the redo issue by codifying the behavior formatting wants, that being the ability to push an empty transaction to the history with no other side-effects (i.e. clearing the redo stack) to detect concurrent edits, with the tradeoff being that it must then manually remove the transaction later if no changes occurred from the formatting. The redo stack is still cleared when there are formatting edits, as the individual format steps use the normal `{start,end}_transaction` methods which clear the redo stack if the finished transaction isn't empty. Release Notes: - Fixed an issue where redo would not work after buffer formatting (including formatting on save) when the formatting did not result in any changes --- crates/editor/src/editor_tests.rs | 70 ++++++++++++++++++++++++++++++- crates/language/src/buffer.rs | 15 +++++++ crates/project/src/lsp_store.rs | 8 +--- crates/text/src/text.rs | 52 ++++++++++++++++++++++- 4 files changed, 137 insertions(+), 8 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 4efb052c71..fbb877796c 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -9570,6 +9570,74 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) { } } +#[gpui::test] +async fn test_redo_after_noop_format(cx: &mut TestAppContext) { + init_test(cx, |settings| { + settings.defaults.ensure_final_newline_on_save = Some(false); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_file(path!("/file.txt"), "foo".into()).await; + + let project = Project::test(fs, [path!("/file.txt").as_ref()], cx).await; + + let buffer = project + .update(cx, |project, cx| { + project.open_local_buffer(path!("/file.txt"), cx) + }) + .await + .unwrap(); + + let buffer = cx.new(|cx| MultiBuffer::singleton(buffer, cx)); + let (editor, cx) = cx.add_window_view(|window, cx| { + build_editor_with_project(project.clone(), buffer, window, cx) + }); + editor.update_in(cx, |editor, window, cx| { + editor.change_selections(SelectionEffects::default(), window, cx, |s| { + s.select_ranges([0..0]) + }); + }); + assert!(!cx.read(|cx| editor.is_dirty(cx))); + + editor.update_in(cx, |editor, window, cx| { + editor.handle_input("\n", window, cx) + }); + cx.run_until_parked(); + save(&editor, &project, cx).await; + assert_eq!("\nfoo", editor.read_with(cx, |editor, cx| editor.text(cx))); + + editor.update_in(cx, |editor, window, cx| { + editor.undo(&Default::default(), window, cx); + }); + save(&editor, &project, cx).await; + assert_eq!("foo", editor.read_with(cx, |editor, cx| editor.text(cx))); + + editor.update_in(cx, |editor, window, cx| { + editor.redo(&Default::default(), window, cx); + }); + cx.run_until_parked(); + assert_eq!("\nfoo", editor.read_with(cx, |editor, cx| editor.text(cx))); + + async fn save(editor: &Entity, project: &Entity, cx: &mut VisualTestContext) { + let save = editor + .update_in(cx, |editor, window, cx| { + editor.save( + SaveOptions { + format: true, + autosave: false, + }, + project.clone(), + window, + cx, + ) + }) + .unwrap(); + cx.executor().start_waiting(); + save.await; + assert!(!cx.read(|cx| editor.is_dirty(cx))); + } +} + #[gpui::test] async fn test_multibuffer_format_during_save(cx: &mut TestAppContext) { init_test(cx, |_| {}); @@ -22708,7 +22776,7 @@ pub(crate) fn init_test(cx: &mut TestAppContext, f: fn(&mut AllLanguageSettingsC workspace::init_settings(cx); crate::init(cx); }); - + zlog::init_test(); update_test_language_settings(cx, f); } diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 59aa63ff38..83517accc2 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -2072,6 +2072,21 @@ impl Buffer { self.text.push_transaction(transaction, now); } + /// Differs from `push_transaction` in that it does not clear the redo + /// stack. Intended to be used to create a parent transaction to merge + /// potential child transactions into. + /// + /// The caller is responsible for removing it from the undo history using + /// `forget_transaction` if no edits are merged into it. Otherwise, if edits + /// are merged into this transaction, the caller is responsible for ensuring + /// the redo stack is cleared. The easiest way to ensure the redo stack is + /// cleared is to create transactions with the usual `start_transaction` and + /// `end_transaction` methods and merging the resulting transactions into + /// the transaction created by this method + pub fn push_empty_transaction(&mut self, now: Instant) -> TransactionId { + self.text.push_empty_transaction(now) + } + /// Prevent the last transaction from being grouped with any subsequent transactions, /// even if they occur with the buffer's undo grouping duration. pub fn finalize_last_transaction(&mut self) -> Option<&Transaction> { diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 28cbfcdd18..0cd375e0c5 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1274,15 +1274,11 @@ impl LocalLspStore { // grouped with the previous transaction in the history // based on the transaction group interval buffer.finalize_last_transaction(); - let transaction_id = buffer + buffer .start_transaction() .context("transaction already open")?; - let transaction = buffer - .get_transaction(transaction_id) - .expect("transaction started") - .clone(); buffer.end_transaction(cx); - buffer.push_transaction(transaction, cx.background_executor().now()); + let transaction_id = buffer.push_empty_transaction(cx.background_executor().now()); buffer.finalize_last_transaction(); anyhow::Ok(transaction_id) })??; diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index a2742081f4..aa9682029e 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -320,7 +320,39 @@ impl History { last_edit_at: now, suppress_grouping: false, }); - self.redo_stack.clear(); + } + + /// Differs from `push_transaction` in that it does not clear the redo + /// stack. Intended to be used to create a parent transaction to merge + /// potential child transactions into. + /// + /// The caller is responsible for removing it from the undo history using + /// `forget_transaction` if no edits are merged into it. Otherwise, if edits + /// are merged into this transaction, the caller is responsible for ensuring + /// the redo stack is cleared. The easiest way to ensure the redo stack is + /// cleared is to create transactions with the usual `start_transaction` and + /// `end_transaction` methods and merging the resulting transactions into + /// the transaction created by this method + fn push_empty_transaction( + &mut self, + start: clock::Global, + now: Instant, + clock: &mut clock::Lamport, + ) -> TransactionId { + assert_eq!(self.transaction_depth, 0); + let id = clock.tick(); + let transaction = Transaction { + id, + start, + edit_ids: Vec::new(), + }; + self.undo_stack.push(HistoryEntry { + transaction, + first_edit_at: now, + last_edit_at: now, + suppress_grouping: false, + }); + id } fn push_undo(&mut self, op_id: clock::Lamport) { @@ -1495,6 +1527,24 @@ impl Buffer { self.history.push_transaction(transaction, now); } + /// Differs from `push_transaction` in that it does not clear the redo stack. + /// The caller responsible for + /// Differs from `push_transaction` in that it does not clear the redo + /// stack. Intended to be used to create a parent transaction to merge + /// potential child transactions into. + /// + /// The caller is responsible for removing it from the undo history using + /// `forget_transaction` if no edits are merged into it. Otherwise, if edits + /// are merged into this transaction, the caller is responsible for ensuring + /// the redo stack is cleared. The easiest way to ensure the redo stack is + /// cleared is to create transactions with the usual `start_transaction` and + /// `end_transaction` methods and merging the resulting transactions into + /// the transaction created by this method + pub fn push_empty_transaction(&mut self, now: Instant) -> TransactionId { + self.history + .push_empty_transaction(self.version.clone(), now, &mut self.lamport_clock) + } + pub fn edited_ranges_for_transaction_id( &self, transaction_id: TransactionId,