Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when deleting with multiple cursors #6024

Merged
merged 4 commits into from
May 18, 2023
Merged

Fix crash when deleting with multiple cursors #6024

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c7c3bd
fix panic when deleting overlapping ranges
pascalkuthe Feb 16, 2023
d445aa2
cleanup delete_by_selection_insert_mode function
pascalkuthe Feb 16, 2023
013fa65
don't move cursor while forward deleting in append mode
pascalkuthe Feb 20, 2023
d605a34
cleanup integration tests
pascalkuthe May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion helix-core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
51 changes: 51 additions & 0 deletions helix-core/src/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::borrow::Cow;

/// (from, to, replacement)
pub type Change = (usize, usize, Option<Tendril>);
pub type Deletion = (usize, usize);

// TODO: pub(crate)
#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -534,6 +535,46 @@ impl Transaction {
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,
dead10ck marked this conversation as resolved.
Show resolved Hide resolved
"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)
}

pub fn insert_at_eof(mut self, text: Tendril) -> Transaction {
self.changes.insert(text);
self
}

/// Generate a transaction with a change per selection range.
pub fn change_by_selection<F>(doc: &Rope, selection: &Selection, f: F) -> Self
where
Expand Down Expand Up @@ -580,6 +621,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.
pub fn insert(doc: &Rope, selection: &Selection, text: Tendril) -> Self {
Self::change_by_selection(doc, selection, |range| {
Expand Down
212 changes: 113 additions & 99 deletions helix-term/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ use helix_core::{
textobject,
tree_sitter::Node,
unicode::width::UnicodeWidthChar,
visual_offset_from_block, LineEnding, Position, Range, Rope, RopeGraphemes, RopeSlice,
Selection, SmallVec, Tendril, Transaction,
visual_offset_from_block, Deletion, LineEnding, Position, Range, Rope, RopeGraphemes,
RopeSlice, Selection, SmallVec, Tendril, Transaction,
};
use helix_view::{
clipboard::ClipboardType,
Expand Down Expand Up @@ -788,54 +788,50 @@ fn extend_to_line_start(cx: &mut Context) {
}

fn kill_to_line_start(cx: &mut Context) {
let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..);

let selection = doc.selection(view.id).clone().transform(|range| {
let line = range.cursor_line(text);
let first_char = text.line_to_char(line);
let anchor = range.cursor(text);
let head = if anchor == first_char && line != 0 {
// select until previous line
line_end_char_index(&text, line - 1)
} else if let Some(pos) = find_first_non_whitespace_char(text.line(line)) {
if first_char + pos < anchor {
// select until first non-blank in line if cursor is after it
first_char + pos
delete_by_selection_insert_mode(
cx,
move |text, range| {
let line = range.cursor_line(text);
let first_char = text.line_to_char(line);
let anchor = range.cursor(text);
let head = if anchor == first_char && line != 0 {
// select until previous line
line_end_char_index(&text, line - 1)
} else if let Some(pos) = find_first_non_whitespace_char(text.line(line)) {
if first_char + pos < anchor {
// select until first non-blank in line if cursor is after it
first_char + pos
} else {
// select until start of line
first_char
}
} else {
// select until start of line
first_char
}
} else {
// select until start of line
first_char
};
Range::new(head, anchor)
});
delete_selection_insert_mode(doc, view, &selection);

lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
};
(head, anchor)
},
Direction::Backward,
);
}

fn kill_to_line_end(cx: &mut Context) {
let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..);

let selection = doc.selection(view.id).clone().transform(|range| {
let line = range.cursor_line(text);
let line_end_pos = line_end_char_index(&text, line);
let pos = range.cursor(text);

let mut new_range = range.put_cursor(text, line_end_pos, true);
// don't want to remove the line separator itself if the cursor doesn't reach the end of line.
if pos != line_end_pos {
new_range.head = line_end_pos;
}
new_range
});
delete_selection_insert_mode(doc, view, &selection);
delete_by_selection_insert_mode(
cx,
|text, range| {
let line = range.cursor_line(text);
let line_end_pos = line_end_char_index(&text, line);
let pos = range.cursor(text);

lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
// if the cursor is on the newline char delete that
if pos == line_end_pos {
(pos, text.line_to_char(line + 1))
} else {
(pos, line_end_pos)
}
},
Direction::Forward,
);
}

fn goto_first_nonwhitespace(cx: &mut Context) {
Expand Down Expand Up @@ -2308,9 +2304,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 {
Expand All @@ -2325,11 +2320,49 @@ 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)
});
fn delete_by_selection_insert_mode(
cx: &mut Context,
mut f: impl FnMut(RopeSlice, &Range) -> Deletion,
direction: Direction,
) {
let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..);
let mut selection = SmallVec::new();
let mut insert_newline = false;
let text_len = text.len_chars();
let mut transaction =
Transaction::delete_by_selection(doc.text(), doc.selection(view.id), |range| {
let (start, end) = f(text, range);
if direction == Direction::Forward {
let mut range = *range;
if range.head > range.anchor {
insert_newline |= end == text_len;
// move the cursor to the right so that the selection
// doesn't shrink when deleting forward (so the text appears to
// move to left)
// += 1 is enough here as the range is normalized to grapheme boundaries
// later anyway
range.head += 1;
}
selection.push(range);
}
(start, end)
});

// in case we delete the last character and the cursor would be moved to the EOF char
// insert a newline, just like when entering append mode
if insert_newline {
transaction = transaction.insert_at_eof(doc.line_ending.as_str().into());
}

if direction == Direction::Forward {
doc.set_selection(
view.id,
Selection::new(selection, doc.selection(view.id).primary_index()),
dead10ck marked this conversation as resolved.
Show resolved Hide resolved
);
}
doc.apply(&transaction, view.id);
lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
}

fn delete_selection(cx: &mut Context) {
Expand Down Expand Up @@ -3415,22 +3448,18 @@ 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.
let fragment = Cow::from(text.slice(line_start_pos..pos));
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()
Expand All @@ -3457,7 +3486,7 @@ pub mod insert {
_ => break,
}
}
(start, pos, None) // delete!
(start, pos) // delete!
}
} else {
match (
Expand All @@ -3475,17 +3504,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)
}
}
}
Expand All @@ -3498,50 +3522,40 @@ pub mod insert {

pub fn delete_char_forward(cx: &mut Context) {
let count = cx.count();
let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..);
let transaction =
Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {
delete_by_selection_insert_mode(
cx,
|text, range| {
let pos = range.cursor(text);
(
pos,
graphemes::nth_next_grapheme_boundary(text, pos, count),
None,
)
});
doc.apply(&transaction, view.id);

lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
(pos, graphemes::nth_next_grapheme_boundary(text, pos, count))
},
Direction::Forward,
)
}

pub fn delete_word_backward(cx: &mut Context) {
let count = cx.count();
let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..);

let selection = doc.selection(view.id).clone().transform(|range| {
let anchor = movement::move_prev_word_start(text, range, count).from();
let next = Range::new(anchor, range.cursor(text));
exclude_cursor(text, next, range)
});
delete_selection_insert_mode(doc, view, &selection);

lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
delete_by_selection_insert_mode(
cx,
|text, range| {
let anchor = movement::move_prev_word_start(text, *range, count).from();
let next = Range::new(anchor, range.cursor(text));
let range = exclude_cursor(text, next, *range);
(range.from(), range.to())
},
Direction::Backward,
);
}

pub fn delete_word_forward(cx: &mut Context) {
let count = cx.count();
let (view, doc) = current!(cx.editor);
let text = doc.text().slice(..);

let selection = doc.selection(view.id).clone().transform(|range| {
let head = movement::move_next_word_end(text, range, count).to();
Range::new(range.cursor(text), head)
});

delete_selection_insert_mode(doc, view, &selection);

lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
delete_by_selection_insert_mode(
cx,
|text, range| {
let head = movement::move_next_word_end(text, *range, count).to();
(range.cursor(text), head)
},
Direction::Forward,
);
}
}

Expand Down
Loading