From 6e423bb1cdfbe973bc2a58905e65fab3b1d9940a Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Tue, 22 Nov 2022 19:38:01 -0600 Subject: [PATCH 1/4] Add a test case for updating jumplists across windows --- helix-term/tests/test/splits.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/helix-term/tests/test/splits.rs b/helix-term/tests/test/splits.rs index 5807413a267e..a51de365850e 100644 --- a/helix-term/tests/test/splits.rs +++ b/helix-term/tests/test/splits.rs @@ -127,3 +127,29 @@ async fn test_split_write_quit_same_file() -> anyhow::Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_changes_in_splits_apply_to_all_views() -> anyhow::Result<()> { + // See . + // Transactions must be applied to any view that has the changed document open. + // This sequence would panic since the jumplist entry would be modified in one + // window but not the other. Attempting to update the changelist in the other + // window would cause a panic since it would point outside of the document. + + // The key sequence here: + // * v Create a vertical split of the current buffer. + // Both views look at the same doc. + // * [ Add a line ending to the beginning of the document. + // The cursor is now at line 2 in window 2. + // * Save that selection to the jumplist in window 2. + // * w Switch to window 1. + // * kd Delete line 1 in window 1. + // * q Close window 1, focusing window 2. + // * d Delete line 1 in window 2. + // + // This panicked in the past because the jumplist entry on line 2 of window 2 + // was not updated and after the `kd` step, pointed outside of the document. + test(("#[|]#", "v[wkdqd", "#[|]#")).await?; + + Ok(()) +} From f650ce0cb6db913b86e91150ee10270b36b344d5 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Tue, 22 Nov 2022 20:37:37 -0600 Subject: [PATCH 2/4] Apply transactions to all views on history changes This ensures that jumplist selections follow changes in documents, even when there are multiple views (for example a split where both windows edit the same document). --- helix-core/src/history.rs | 18 ++++++++++++++++++ helix-term/src/ui/editor.rs | 26 +++++++++++++++++++++++--- helix-view/src/macros.rs | 2 +- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/helix-core/src/history.rs b/helix-core/src/history.rs index 51174c02475e..8a8702b9cc6a 100644 --- a/helix-core/src/history.rs +++ b/helix-core/src/history.rs @@ -119,6 +119,24 @@ impl History { self.current == 0 } + /// Returns the changes since the given revision composed into a transaction. + /// Returns None if there are no changes between the current and given revisions. + pub fn changes_since(&self, revision: usize) -> Option { + if self.at_root() || self.current >= revision { + return None; + } + + let mut transaction = self.revisions[revision].transaction.clone(); + + // The bounds are checked in the if condition above: + // `revision + 1` is known to be `<= self.current`. + for revision in &self.revisions[revision + 1..self.current] { + transaction = transaction.compose(revision.transaction.clone()); + } + + Some(transaction) + } + /// Undo the last edit. pub fn undo(&mut self) -> Option<&Transaction> { if self.at_root() { diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index 6c8ee2d95e9c..44f89b77ed15 100644 --- a/helix-term/src/ui/editor.rs +++ b/helix-term/src/ui/editor.rs @@ -1337,7 +1337,9 @@ impl Component for EditorView { cx.editor.status_msg = None; let mode = cx.editor.mode(); - let (view, _) = current!(cx.editor); + let (view, doc) = current!(cx.editor); + let original_doc_id = doc.id(); + let original_doc_revision = doc.get_current_revision(); let focus = view.id; if let Some(on_next_key) = self.on_next_key.take() { @@ -1413,13 +1415,31 @@ impl Component for EditorView { let view = view_mut!(cx.editor, focus); let doc = doc_mut!(cx.editor, &view.doc); - view.ensure_cursor_in_view(doc, config.scrolloff); - // Store a history state if not in insert mode. This also takes care of // committing changes when leaving insert mode. if mode != Mode::Insert { doc.append_changes_to_history(view.id); } + + // If the current document has been changed, apply the changes to all views. + // This ensures that selections in jumplists follow changes. + if doc.id() == original_doc_id + && doc.get_current_revision() > original_doc_revision + { + if let Some(transaction) = + doc.history.get_mut().changes_since(original_doc_revision) + { + let doc = doc!(cx.editor, &original_doc_id); + for (view, _focused) in cx.editor.tree.views_mut() { + view.apply(&transaction, doc); + } + } + } + + let view = view_mut!(cx.editor, focus); + let doc = doc_mut!(cx.editor, &view.doc); + + view.ensure_cursor_in_view(doc, config.scrolloff); } EventResult::Consumed(callback) diff --git a/helix-view/src/macros.rs b/helix-view/src/macros.rs index 53ab434622d4..ee9cd4111795 100644 --- a/helix-view/src/macros.rs +++ b/helix-view/src/macros.rs @@ -67,7 +67,7 @@ macro_rules! view { #[macro_export] macro_rules! doc { ($editor:expr, $id:expr) => {{ - $editor.documents[$id] + &$editor.documents[$id] }}; ($editor:expr) => {{ $crate::current_ref!($editor).1 From 8f08375a1b4cf247ac2f46519dec43796d9094fe Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Tue, 22 Nov 2022 20:37:48 -0600 Subject: [PATCH 3/4] Leave TODOs for cleaning up View::apply --- helix-view/src/lib.rs | 8 +++----- helix-view/src/view.rs | 1 + 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/helix-view/src/lib.rs b/helix-view/src/lib.rs index 4c32b356b9ea..9cf36ae0505c 100644 --- a/helix-view/src/lib.rs +++ b/helix-view/src/lib.rs @@ -71,12 +71,10 @@ pub fn align_view(doc: &Document, view: &mut View, align: Align) { pub fn apply_transaction( transaction: &helix_core::Transaction, doc: &mut Document, - view: &mut View, + view: &View, ) -> bool { - // This is a short function but it's easy to call `Document::apply` - // without calling `View::apply` or in the wrong order. The transaction - // must be applied to the document before the view. - doc.apply(transaction, view.id) && view.apply(transaction, doc) + // TODO remove this helper function. Just call Document::apply everywhere directly. + doc.apply(transaction, view.id) } pub use document::Document; diff --git a/helix-view/src/view.rs b/helix-view/src/view.rs index 6da4df1f0f03..21d742b45dea 100644 --- a/helix-view/src/view.rs +++ b/helix-view/src/view.rs @@ -345,6 +345,7 @@ impl View { /// which applies a transaction to the [`Document`] and view together. pub fn apply(&mut self, transaction: &Transaction, doc: &Document) -> bool { self.jumps.apply(transaction, doc); + // TODO: remove the boolean return. This is unused. true } } From 187858f75683aac9e1a850b198c8ba4cfbedc9f7 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Tue, 22 Nov 2022 21:13:50 -0600 Subject: [PATCH 4/4] Use Iterator::reduce to compose history transactions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Blaž Hrastnik --- helix-core/src/history.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/helix-core/src/history.rs b/helix-core/src/history.rs index 8a8702b9cc6a..697f29b4ec98 100644 --- a/helix-core/src/history.rs +++ b/helix-core/src/history.rs @@ -54,7 +54,7 @@ pub struct History { } /// A single point in history. See [History] for more information. -#[derive(Debug)] +#[derive(Debug, Clone)] struct Revision { parent: usize, last_child: Option, @@ -126,15 +126,13 @@ impl History { return None; } - let mut transaction = self.revisions[revision].transaction.clone(); - // The bounds are checked in the if condition above: - // `revision + 1` is known to be `<= self.current`. - for revision in &self.revisions[revision + 1..self.current] { - transaction = transaction.compose(revision.transaction.clone()); - } - - Some(transaction) + // `revision` is known to be `< self.current`. + self.revisions[revision..self.current] + .iter() + .map(|revision| &revision.transaction) + .cloned() + .reduce(|acc, transaction| acc.compose(transaction)) } /// Undo the last edit.