Mercurial > forks > helix
changeset 3822:dbde7f6292c8
Apply transactions to all views (#4733)
* Add a test case for updating jumplists across windows
* 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).
* Leave TODOs for cleaning up View::apply
* Use Iterator::reduce to compose history transactions
Co-authored-by: Bla? Hrastnik <[email protected]>
Co-authored-by: Bla? Hrastnik <[email protected]>
author | Michael Davis <mcarsondavis@gmail.com> |
---|---|
date | Tue, 22 Nov 2022 21:28:49 -0600 |
parents | f342a55bfd9b |
children | 0c3b44553bbc |
files | helix-core/src/history.rs helix-term/src/ui/editor.rs helix-term/tests/test/splits.rs helix-view/src/lib.rs helix-view/src/macros.rs helix-view/src/view.rs |
diffstat | 6 files changed, 71 insertions(+), 10 deletions(-) [+] |
line wrap: on
line diff
--- a/helix-core/src/history.rs Tue Nov 22 13:24:37 2022 -0600 +++ b/helix-core/src/history.rs Tue Nov 22 21:28:49 2022 -0600 @@ -54,7 +54,7 @@ } /// A single point in history. See [History] for more information. -#[derive(Debug)] +#[derive(Debug, Clone)] struct Revision { parent: usize, last_child: Option<NonZeroUsize>, @@ -119,6 +119,22 @@ 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<Transaction> { + if self.at_root() || self.current >= revision { + return None; + } + + // The bounds are checked in the if condition above: + // `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. pub fn undo(&mut self) -> Option<&Transaction> { if self.at_root() {
--- a/helix-term/src/ui/editor.rs Tue Nov 22 13:24:37 2022 -0600 +++ b/helix-term/src/ui/editor.rs Tue Nov 22 21:28:49 2022 -0600 @@ -1337,7 +1337,9 @@ 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 @@ 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)
--- a/helix-term/tests/test/splits.rs Tue Nov 22 13:24:37 2022 -0600 +++ b/helix-term/tests/test/splits.rs Tue Nov 22 21:28:49 2022 -0600 @@ -127,3 +127,29 @@ Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_changes_in_splits_apply_to_all_views() -> anyhow::Result<()> { + // See <https://github.com/helix-editor/helix/issues/4732>. + // 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: + // * <C-w>v Create a vertical split of the current buffer. + // Both views look at the same doc. + // * [<space> Add a line ending to the beginning of the document. + // The cursor is now at line 2 in window 2. + // * <C-s> Save that selection to the jumplist in window 2. + // * <C-w>w Switch to window 1. + // * kd Delete line 1 in window 1. + // * <C-w>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(("#[|]#", "<C-w>v[<space><C-s><C-w>wkd<C-w>qd", "#[|]#")).await?; + + Ok(()) +}
--- a/helix-view/src/lib.rs Tue Nov 22 13:24:37 2022 -0600 +++ b/helix-view/src/lib.rs Tue Nov 22 21:28:49 2022 -0600 @@ -71,12 +71,10 @@ 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;
--- a/helix-view/src/macros.rs Tue Nov 22 13:24:37 2022 -0600 +++ b/helix-view/src/macros.rs Tue Nov 22 21:28:49 2022 -0600 @@ -67,7 +67,7 @@ #[macro_export] macro_rules! doc { ($editor:expr, $id:expr) => {{ - $editor.documents[$id] + &$editor.documents[$id] }}; ($editor:expr) => {{ $crate::current_ref!($editor).1
--- a/helix-view/src/view.rs Tue Nov 22 13:24:37 2022 -0600 +++ b/helix-view/src/view.rs Tue Nov 22 21:28:49 2022 -0600 @@ -351,6 +351,7 @@ /// 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 } }