From f8225ed9219f23cf04bd378ec43e1e1a1059a0ed Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 16 Feb 2023 22:15:06 +0100 Subject: [PATCH] fix panic when deleting overlapping ranges Some deletion operations (especially those that use indentation) can generate overlapping deletion ranges when using multiple cursors. To fix that problem a new `Transaction::delete` and `Transaction:delete_by_selection` function were added. These functions merge overlapping deletion ranges instead of generating an invalid transaction. This merging of changes is only possible for deletions and not for other changes and therefore require its own function. The function has been used in all commands that currently delete text by using `Transaction::change_by_selection`. --- helix-core/src/lib.rs | 2 +- helix-core/src/transaction.rs | 46 +++++++++++++++++++++++++++++++ helix-term/src/commands.rs | 37 ++++++++----------------- helix-term/tests/test/commands.rs | 44 +++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 27 deletions(-) diff --git a/helix-core/src/lib.rs b/helix-core/src/lib.rs index b67e2c8a3..14abf0162 100644 --- a/helix-core/src/lib.rs +++ b/helix-core/src/lib.rs @@ -67,4 +67,4 @@ pub use syntax::Syntax; pub use diagnostic::Diagnostic; pub use line_ending::{LineEnding, DEFAULT_LINE_ENDING}; -pub use transaction::{Assoc, Change, ChangeSet, Operation, Transaction}; +pub use transaction::{Assoc, Change, ChangeSet, Deletion, Operation, Transaction}; diff --git a/helix-core/src/transaction.rs b/helix-core/src/transaction.rs index d8e581aae..06efe259f 100644 --- a/helix-core/src/transaction.rs +++ b/helix-core/src/transaction.rs @@ -5,6 +5,7 @@ use std::borrow::Cow; /// (from, to, replacement) pub type Change = (usize, usize, Option); +pub type Deletion = (usize, usize); // TODO: pub(crate) #[derive(Debug, Clone, PartialEq, Eq)] @@ -534,6 +535,41 @@ impl Transaction { Self::from(changeset) } + /// Generate a transaction from a set of potentially overlapping deletions + /// by merging overlapping deletions together. + pub fn delete(doc: &Rope, deletions: I) -> Self + where + I: Iterator, + { + let len = doc.len_chars(); + + let (lower, upper) = deletions.size_hint(); + let size = upper.unwrap_or(lower); + let mut changeset = ChangeSet::with_capacity(2 * size + 1); // rough estimate + + let mut last = 0; + for (mut from, to) in deletions { + if last > to { + continue; + } + if last > from { + from = last + } + debug_assert!( + from <= to, + "Edit end must end before it starts (should {from} <= {to})" + ); + // Retain from last "to" to current "from" + changeset.retain(from - last); + changeset.delete(to - from); + last = to; + } + + changeset.retain(len - last); + + Self::from(changeset) + } + /// Generate a transaction with a change per selection range. pub fn change_by_selection(doc: &Rope, selection: &Selection, f: F) -> Self where @@ -580,6 +616,16 @@ impl Transaction { ) } + /// Generate a transaction with a deletion per selection range. + /// Compared to using `change_by_selection` directly these ranges may overlap. + /// In that case they are merged + pub fn delete_by_selection(doc: &Rope, selection: &Selection, f: F) -> Self + where + F: FnMut(&Range) -> Deletion, + { + Self::delete(doc, selection.iter().map(f)) + } + /// Insert text at each selection head. pub fn insert(doc: &Rope, selection: &Selection, text: Tendril) -> Self { Self::change_by_selection(doc, selection, |range| { diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 80774ceae..964d87ff9 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -2315,9 +2315,8 @@ fn delete_selection_impl(cx: &mut Context, op: Operation) { }; // then delete - let transaction = Transaction::change_by_selection(doc.text(), selection, |range| { - (range.from(), range.to(), None) - }); + let transaction = + Transaction::delete_by_selection(doc.text(), selection, |range| (range.from(), range.to())); doc.apply(&transaction, view.id); match op { @@ -2333,9 +2332,8 @@ fn delete_selection_impl(cx: &mut Context, op: Operation) { #[inline] fn delete_selection_insert_mode(doc: &mut Document, view: &mut View, selection: &Selection) { - let transaction = Transaction::change_by_selection(doc.text(), selection, |range| { - (range.from(), range.to(), None) - }); + let transaction = + Transaction::delete_by_selection(doc.text(), selection, |range| (range.from(), range.to())); doc.apply(&transaction, view.id); } @@ -3422,10 +3420,10 @@ pub mod insert { let auto_pairs = doc.auto_pairs(cx.editor); let transaction = - Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| { + Transaction::delete_by_selection(doc.text(), doc.selection(view.id), |range| { let pos = range.cursor(text); if pos == 0 { - return (pos, pos, None); + return (pos, pos); } let line_start_pos = text.line_to_char(range.cursor_line(text)); // consider to delete by indent level if all characters before `pos` are indent units. @@ -3433,11 +3431,7 @@ pub mod insert { if !fragment.is_empty() && fragment.chars().all(|ch| ch == ' ' || ch == '\t') { if text.get_char(pos.saturating_sub(1)) == Some('\t') { // fast path, delete one char - ( - graphemes::nth_prev_grapheme_boundary(text, pos, 1), - pos, - None, - ) + (graphemes::nth_prev_grapheme_boundary(text, pos, 1), pos) } else { let width: usize = fragment .chars() @@ -3464,7 +3458,7 @@ pub mod insert { _ => break, } } - (start, pos, None) // delete! + (start, pos) // delete! } } else { match ( @@ -3482,17 +3476,12 @@ pub mod insert { ( graphemes::nth_prev_grapheme_boundary(text, pos, count), graphemes::nth_next_grapheme_boundary(text, pos, count), - None, ) } _ => // delete 1 char { - ( - graphemes::nth_prev_grapheme_boundary(text, pos, count), - pos, - None, - ) + (graphemes::nth_prev_grapheme_boundary(text, pos, count), pos) } } } @@ -3508,13 +3497,9 @@ pub mod insert { let (view, doc) = current!(cx.editor); let text = doc.text().slice(..); let transaction = - Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| { + Transaction::delete_by_selection(doc.text(), doc.selection(view.id), |range| { let pos = range.cursor(text); - ( - pos, - graphemes::nth_next_grapheme_boundary(text, pos, count), - None, - ) + (pos, graphemes::nth_next_grapheme_boundary(text, pos, count)) }); doc.apply(&transaction, view.id); diff --git a/helix-term/tests/test/commands.rs b/helix-term/tests/test/commands.rs index 342a849be..1efb204e6 100644 --- a/helix-term/tests/test/commands.rs +++ b/helix-term/tests/test/commands.rs @@ -385,3 +385,47 @@ async fn test_character_info() -> anyhow::Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_delete_char_backward() -> anyhow::Result<()> { + // don't panic when deleting overlapping ranges + test(( + platform_line("#(x|)# #[x|]#").as_str(), + "c", + platform_line("#[\n|]#").as_str(), + )) + .await?; + test(( + platform_line("#( |)##( |)#a#( |)#axx#[x|]#a").as_str(), + "li", + platform_line("#(a|)##(|a)#xx#[|a]#").as_str(), + )) + .await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_delete_word_backward() -> anyhow::Result<()> { + // don't panic when deleting overlapping ranges + test(( + platform_line("fo#[o|]#ba#(r|)#").as_str(), + "a", + platform_line("#[\n|]#").as_str(), + )) + .await?; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_delete_word_forward() -> anyhow::Result<()> { + // don't panic when deleting overlapping ranges + test(( + platform_line("fo#[o|]#b#(|ar)#").as_str(), + "i", + platform_line("fo#[\n|]#").as_str(), + )) + .await?; + Ok(()) +} +