From 21104553cf7d290b02873289256b13e72cf68de3 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Wed, 23 Nov 2022 08:57:09 -0600 Subject: [PATCH 1/4] Add test case that panics on undo This case panics since undo/redo call View::apply and here, the edit that moves the jumplist selection out-of-bounds is not yet applied when View::apply is called in undo/redo. View::apply should only be called by the EditorView now. --- helix-term/tests/test/commands.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/helix-term/tests/test/commands.rs b/helix-term/tests/test/commands.rs index 114bf22211a5..27dbd9d7c3c5 100644 --- a/helix-term/tests/test/commands.rs +++ b/helix-term/tests/test/commands.rs @@ -283,3 +283,19 @@ 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 passed through an edit and then an undo and then a redo. + // + // * [ Add a newline at line start. We're now on line 2. + // * 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. + // * Jump back in the jumplist. This would panic if the jumplist were not being + // updated correctly. + // * Jump forward to line 1. + test(("#[|]#", "[kduU", "#[|]#")).await?; + + Ok(()) +} From 8e62264c237a9c12bca53a75a21fe90e76ab86e6 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Wed, 23 Nov 2022 08:57:03 -0600 Subject: [PATCH 2/4] Don't apply transactions to Views in undo/redo View::apply should only be called by EditorView after 42e37a571e75aaf4feb1717dfebe8cf215e535dd. This change removes the duplicate calls within undo/redo which could cause a panic. --- helix-term/src/commands.rs | 8 ++++---- helix-term/src/commands/typed.rs | 4 ++-- helix-view/src/document.rs | 24 ++++++++++++------------ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 8af5a7e3e400..4eb9742ff70e 100644 --- a/helix-term/src/commands.rs +++ b/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; } diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index b8f99ff369d2..475f14d1de3c 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -481,7 +481,7 @@ fn earlier( let uk = args.join(" ").parse::().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::().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"); } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 087085283391..0eb54f25fa6e 100644 --- a/helix-view/src/document.rs +++ b/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,7 +894,7 @@ 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 { @@ -902,7 +902,7 @@ impl Document { }; 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 From 2c835697385dda9f0c9280ebc2138f1094176c69 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Wed, 23 Nov 2022 09:35:07 -0600 Subject: [PATCH 3/4] Apply inversions to Views on undo/redo When using undo/redo, the history revision can be decremented. In that case we should apply the inversions since the given revision in History::changes_since. This prevents panics with jumplist operations when a session uses undo/redo to move the jumplist selection outside of the document. --- helix-core/src/history.rs | 24 ++++++++++++++---------- helix-term/src/ui/editor.rs | 2 +- helix-term/tests/test/commands.rs | 9 +++++++++ 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/helix-core/src/history.rs b/helix-core/src/history.rs index 697f29b4ec98..5f9fa71e4b9b 100644 --- a/helix-core/src/history.rs +++ b/helix-core/src/history.rs @@ -122,17 +122,21 @@ 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 { - 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, + Greater => self.revisions[self.current + 1..=revision] + .iter() + .map(|revision| &revision.inversion) + .cloned() + .reduce(|acc, inversion| acc.compose(inversion)), + Less => self.revisions[revision + 1..=self.current] + .iter() + .map(|revision| &revision.transaction) + .cloned() + .reduce(|acc, transaction| acc.compose(transaction)), + } } /// Undo the last edit. diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index 44f89b77ed15..737125031e99 100644 --- a/helix-term/src/ui/editor.rs +++ b/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) diff --git a/helix-term/tests/test/commands.rs b/helix-term/tests/test/commands.rs index 27dbd9d7c3c5..012957045587 100644 --- a/helix-term/tests/test/commands.rs +++ b/helix-term/tests/test/commands.rs @@ -286,6 +286,15 @@ async fn test_multi_selection_shell_commands() -> anyhow::Result<()> { #[tokio::test(flavor = "multi_thread")] async fn test_undo_redo() -> anyhow::Result<()> { + // A jumplist selection is created at a point which is undone. + // + // * 2[ Add two newlines at line start. We're now on line 3. + // * Save the selection on line 3 in the jumplist. + // * u Undo the two newlines. We're now on line 1. + // * Jump forward an back again in the jumplist. This would panic + // if the jumplist were not being updated correctly. + test(("#[|]#", "2[u", "#[|]#")).await?; + // A jumplist selection is passed through an edit and then an undo and then a redo. // // * [ Add a newline at line start. We're now on line 2. From a61726184a7129f3d80cd7b9887b15b985c705a0 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Wed, 23 Nov 2022 13:02:00 -0600 Subject: [PATCH 4/4] Follow parent links when calculating changes since a revision The 'revisions' field on History can't be treated as linear: each Revision in the revisions Vec has a parent link and an optional child link. We can follow those to unroll the recent history. --- helix-core/src/history.rs | 31 +++++++++++++++++++++---------- helix-term/tests/test/commands.rs | 3 +++ 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/helix-core/src/history.rs b/helix-core/src/history.rs index 5f9fa71e4b9b..825092423e00 100644 --- a/helix-core/src/history.rs +++ b/helix-core/src/history.rs @@ -126,16 +126,27 @@ impl History { match revision.cmp(&self.current) { Equal => None, - Greater => self.revisions[self.current + 1..=revision] - .iter() - .map(|revision| &revision.inversion) - .cloned() - .reduce(|acc, inversion| acc.compose(inversion)), - Less => self.revisions[revision + 1..=self.current] - .iter() - .map(|revision| &revision.transaction) - .cloned() - .reduce(|acc, transaction| acc.compose(transaction)), + 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) + } } } diff --git a/helix-term/tests/test/commands.rs b/helix-term/tests/test/commands.rs index 012957045587..95bd95b73a7e 100644 --- a/helix-term/tests/test/commands.rs +++ b/helix-term/tests/test/commands.rs @@ -306,5 +306,8 @@ async fn test_undo_redo() -> anyhow::Result<()> { // * Jump forward to line 1. test(("#[|]#", "[kduU", "#[|]#")).await?; + // In this case we 'redo' manually to ensure that the transactions are composing correctly. + test(("#[|]#", "[u[u", "#[|]#")).await?; + Ok(()) }