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`.
pull/6446/merge
Pascal Kuthe 2 years ago committed by Blaž Hrastnik
parent 6842fd4c36
commit f8225ed921

@ -67,4 +67,4 @@ pub use syntax::Syntax;
pub use diagnostic::Diagnostic; pub use diagnostic::Diagnostic;
pub use line_ending::{LineEnding, DEFAULT_LINE_ENDING}; 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};

@ -5,6 +5,7 @@ use std::borrow::Cow;
/// (from, to, replacement) /// (from, to, replacement)
pub type Change = (usize, usize, Option<Tendril>); pub type Change = (usize, usize, Option<Tendril>);
pub type Deletion = (usize, usize);
// TODO: pub(crate) // TODO: pub(crate)
#[derive(Debug, Clone, PartialEq, Eq)] #[derive(Debug, Clone, PartialEq, Eq)]
@ -534,6 +535,41 @@ impl Transaction {
Self::from(changeset) Self::from(changeset)
} }
/// Generate a transaction from a set of potentially overlapping deletions
/// by merging overlapping deletions together.
pub fn delete<I>(doc: &Rope, deletions: I) -> Self
where
I: Iterator<Item = Deletion>,
{
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. /// Generate a transaction with a change per selection range.
pub fn change_by_selection<F>(doc: &Rope, selection: &Selection, f: F) -> Self pub fn change_by_selection<F>(doc: &Rope, selection: &Selection, f: F) -> Self
where 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<F>(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. /// Insert text at each selection head.
pub fn insert(doc: &Rope, selection: &Selection, text: Tendril) -> Self { pub fn insert(doc: &Rope, selection: &Selection, text: Tendril) -> Self {
Self::change_by_selection(doc, selection, |range| { Self::change_by_selection(doc, selection, |range| {

@ -2315,9 +2315,8 @@ fn delete_selection_impl(cx: &mut Context, op: Operation) {
}; };
// then delete // then delete
let transaction = Transaction::change_by_selection(doc.text(), selection, |range| { let transaction =
(range.from(), range.to(), None) Transaction::delete_by_selection(doc.text(), selection, |range| (range.from(), range.to()));
});
doc.apply(&transaction, view.id); doc.apply(&transaction, view.id);
match op { match op {
@ -2333,9 +2332,8 @@ fn delete_selection_impl(cx: &mut Context, op: Operation) {
#[inline] #[inline]
fn delete_selection_insert_mode(doc: &mut Document, view: &mut View, selection: &Selection) { fn delete_selection_insert_mode(doc: &mut Document, view: &mut View, selection: &Selection) {
let transaction = Transaction::change_by_selection(doc.text(), selection, |range| { let transaction =
(range.from(), range.to(), None) Transaction::delete_by_selection(doc.text(), selection, |range| (range.from(), range.to()));
});
doc.apply(&transaction, view.id); doc.apply(&transaction, view.id);
} }
@ -3422,10 +3420,10 @@ pub mod insert {
let auto_pairs = doc.auto_pairs(cx.editor); let auto_pairs = doc.auto_pairs(cx.editor);
let transaction = 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); let pos = range.cursor(text);
if pos == 0 { if pos == 0 {
return (pos, pos, None); return (pos, pos);
} }
let line_start_pos = text.line_to_char(range.cursor_line(text)); 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. // 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 !fragment.is_empty() && fragment.chars().all(|ch| ch == ' ' || ch == '\t') {
if text.get_char(pos.saturating_sub(1)) == Some('\t') { if text.get_char(pos.saturating_sub(1)) == Some('\t') {
// fast path, delete one char // fast path, delete one char
( (graphemes::nth_prev_grapheme_boundary(text, pos, 1), pos)
graphemes::nth_prev_grapheme_boundary(text, pos, 1),
pos,
None,
)
} else { } else {
let width: usize = fragment let width: usize = fragment
.chars() .chars()
@ -3464,7 +3458,7 @@ pub mod insert {
_ => break, _ => break,
} }
} }
(start, pos, None) // delete! (start, pos) // delete!
} }
} else { } else {
match ( match (
@ -3482,17 +3476,12 @@ pub mod insert {
( (
graphemes::nth_prev_grapheme_boundary(text, pos, count), graphemes::nth_prev_grapheme_boundary(text, pos, count),
graphemes::nth_next_grapheme_boundary(text, pos, count), graphemes::nth_next_grapheme_boundary(text, pos, count),
None,
) )
} }
_ => _ =>
// delete 1 char // delete 1 char
{ {
( (graphemes::nth_prev_grapheme_boundary(text, pos, count), pos)
graphemes::nth_prev_grapheme_boundary(text, pos, count),
pos,
None,
)
} }
} }
} }
@ -3508,13 +3497,9 @@ pub mod insert {
let (view, doc) = current!(cx.editor); let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..); let text = doc.text().slice(..);
let transaction = 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); let pos = range.cursor(text);
( (pos, graphemes::nth_next_grapheme_boundary(text, pos, count))
pos,
graphemes::nth_next_grapheme_boundary(text, pos, count),
None,
)
}); });
doc.apply(&transaction, view.id); doc.apply(&transaction, view.id);

@ -385,3 +385,47 @@ async fn test_character_info() -> anyhow::Result<()> {
Ok(()) 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<space><backspace><esc>",
platform_line("#[\n|]#").as_str(),
))
.await?;
test((
platform_line("#( |)##( |)#a#( |)#axx#[x|]#a").as_str(),
"li<backspace><esc>",
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<C-w><esc>",
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<A-d><esc>",
platform_line("fo#[\n|]#").as_str(),
))
.await?;
Ok(())
}

Loading…
Cancel
Save