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: the-mikedavis

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<NonZeroUsize>,
@@ -119,6 +119,22 @@ 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<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() {

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)

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 <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(())
+}

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;

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

helix-view/src/view.rs

@@ -351,6 +351,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
     }
 }