Fix transaction application on undo/redo/earlier/later

helix-core/src/history.rs

@@ -122,17 +122,32 @@ impl History {
     /// 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;
-        }
+        use std::cmp::Ordering::*;
 
-        // 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))
+        match revision.cmp(&self.current) {
+            Equal => None,
+            Less => {
+                let mut child = self.revisions[revision].last_child?.get();
+                let mut transaction = self.revisions[child].transaction.clone();
+                while child != self.current {
+                    child = self.revisions[child].last_child?.get();
+                    transaction = transaction.compose(self.revisions[child].transaction.clone());
+                }
+                Some(transaction)
+            }
+            Greater => {
+                let mut inversion = self.revisions[revision].inversion.clone();
+                let mut parent = self.revisions[revision].parent;
+                while parent != self.current {
+                    parent = self.revisions[parent].parent;
+                    if parent == 0 {
+                        return None;
+                    }
+                    inversion = inversion.compose(self.revisions[parent].inversion.clone());
+                }
+                Some(inversion)
+            }
+        }
     }
 
     /// Undo the last edit.

helix-term/src/commands.rs

@@ -3302,7 +3302,7 @@ fn undo(cx: &mut Context) {
     let count = cx.count();
     let (view, doc) = current!(cx.editor);
     for _ in 0..count {
-        if !doc.undo(view) {
+        if !doc.undo(view.id) {
             cx.editor.set_status("Already at oldest change");
             break;
         }
@@ -3313,7 +3313,7 @@ fn redo(cx: &mut Context) {
     let count = cx.count();
     let (view, doc) = current!(cx.editor);
     for _ in 0..count {
-        if !doc.redo(view) {
+        if !doc.redo(view.id) {
             cx.editor.set_status("Already at newest change");
             break;
         }
@@ -3325,7 +3325,7 @@ fn earlier(cx: &mut Context) {
     let (view, doc) = current!(cx.editor);
     for _ in 0..count {
         // rather than doing in batch we do this so get error halfway
-        if !doc.earlier(view, UndoKind::Steps(1)) {
+        if !doc.earlier(view.id, UndoKind::Steps(1)) {
             cx.editor.set_status("Already at oldest change");
             break;
         }
@@ -3337,7 +3337,7 @@ fn later(cx: &mut Context) {
     let (view, doc) = current!(cx.editor);
     for _ in 0..count {
         // rather than doing in batch we do this so get error halfway
-        if !doc.later(view, UndoKind::Steps(1)) {
+        if !doc.later(view.id, UndoKind::Steps(1)) {
             cx.editor.set_status("Already at newest change");
             break;
         }

helix-term/src/commands/typed.rs

@@ -481,7 +481,7 @@ fn earlier(
     let uk = args.join(" ").parse::<UndoKind>().map_err(|s| anyhow!(s))?;
 
     let (view, doc) = current!(cx.editor);
-    let success = doc.earlier(view, uk);
+    let success = doc.earlier(view.id, uk);
     if !success {
         cx.editor.set_status("Already at oldest change");
     }
@@ -500,7 +500,7 @@ fn later(
 
     let uk = args.join(" ").parse::<UndoKind>().map_err(|s| anyhow!(s))?;
     let (view, doc) = current!(cx.editor);
-    let success = doc.later(view, uk);
+    let success = doc.later(view.id, uk);
     if !success {
         cx.editor.set_status("Already at newest change");
     }

helix-term/src/ui/editor.rs

@@ -1424,7 +1424,7 @@ impl Component for EditorView {
                     // 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
+                        && doc.get_current_revision() != original_doc_revision
                     {
                         if let Some(transaction) =
                             doc.history.get_mut().changes_since(original_doc_revision)

helix-term/tests/test/commands.rs

@@ -283,3 +283,31 @@ async fn test_multi_selection_shell_commands() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_undo_redo() -> anyhow::Result<()> {
+    // A jumplist selection is created at a point which is undone.
+    //
+    // * 2[<space>   Add two newlines at line start. We're now on line 3.
+    // * <C-s>       Save the selection on line 3 in the jumplist.
+    // * u           Undo the two newlines. We're now on line 1.
+    // * <C-o><C-i>  Jump forward an back again in the jumplist. This would panic
+    //               if the jumplist were not being updated correctly.
+    test(("#[|]#", "2[<space><C-s>u<C-o><C-i>", "#[|]#")).await?;
+
+    // A jumplist selection is passed through an edit and then an undo and then a redo.
+    //
+    // * [<space>    Add a newline at line start. We're now on line 2.
+    // * <C-s>       Save the selection on line 2 in the jumplist.
+    // * kd          Delete line 1. The jumplist selection should be adjusted to the new line 1.
+    // * uU          Undo and redo the `kd` edit.
+    // * <C-o>       Jump back in the jumplist. This would panic if the jumplist were not being
+    //               updated correctly.
+    // * <C-i>       Jump forward to line 1.
+    test(("#[|]#", "[<space><C-s>kduU<C-o><C-i>", "#[|]#")).await?;
+
+    // In this case we 'redo' manually to ensure that the transactions are composing correctly.
+    test(("#[|]#", "[<space>u[<space>u", "#[|]#")).await?;
+
+    Ok(())
+}

helix-view/src/document.rs

@@ -857,11 +857,11 @@ impl Document {
         success
     }
 
-    fn undo_redo_impl(&mut self, view: &mut View, undo: bool) -> bool {
+    fn undo_redo_impl(&mut self, view_id: ViewId, undo: bool) -> bool {
         let mut history = self.history.take();
         let txn = if undo { history.undo() } else { history.redo() };
         let success = if let Some(txn) = txn {
-            self.apply_impl(txn, view.id) && view.apply(txn, self)
+            self.apply_impl(txn, view_id)
         } else {
             false
         };
@@ -875,13 +875,13 @@ impl Document {
     }
 
     /// Undo the last modification to the [`Document`]. Returns whether the undo was successful.
-    pub fn undo(&mut self, view: &mut View) -> bool {
-        self.undo_redo_impl(view, true)
+    pub fn undo(&mut self, view_id: ViewId) -> bool {
+        self.undo_redo_impl(view_id, true)
     }
 
     /// Redo the last modification to the [`Document`]. Returns whether the redo was successful.
-    pub fn redo(&mut self, view: &mut View) -> bool {
-        self.undo_redo_impl(view, false)
+    pub fn redo(&mut self, view_id: ViewId) -> bool {
+        self.undo_redo_impl(view_id, false)
     }
 
     pub fn savepoint(&mut self) {
@@ -894,15 +894,15 @@ impl Document {
         }
     }
 
-    fn earlier_later_impl(&mut self, view: &mut View, uk: UndoKind, earlier: bool) -> bool {
+    fn earlier_later_impl(&mut self, view_id: ViewId, uk: UndoKind, earlier: bool) -> bool {
         let txns = if earlier {
             self.history.get_mut().earlier(uk)
         } else {
             self.history.get_mut().later(uk)
         };
         let mut success = false;
         for txn in txns {
-            if self.apply_impl(&txn, view.id) && view.apply(&txn, self) {
+            if self.apply_impl(&txn, view_id) {
                 success = true;
             }
         }
@@ -914,13 +914,13 @@ impl Document {
     }
 
     /// Undo modifications to the [`Document`] according to `uk`.
-    pub fn earlier(&mut self, view: &mut View, uk: UndoKind) -> bool {
-        self.earlier_later_impl(view, uk, true)
+    pub fn earlier(&mut self, view_id: ViewId, uk: UndoKind) -> bool {
+        self.earlier_later_impl(view_id, uk, true)
     }
 
     /// Redo modifications to the [`Document`] according to `uk`.
-    pub fn later(&mut self, view: &mut View, uk: UndoKind) -> bool {
-        self.earlier_later_impl(view, uk, false)
+    pub fn later(&mut self, view_id: ViewId, uk: UndoKind) -> bool {
+        self.earlier_later_impl(view_id, uk, false)
     }
 
     /// Commit pending changes to history