f8225ed · helix · git.iko.soy

git.iko.soy/helix

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`.

commitf8225ed9219f23cf04bd378ec43e1e1a1059a0ed authorPascal Kuthe <pascal.kuthe@semimod.de> dateThu 16 Feb 2023, 22:15 (4 years ago) parent6842fd4

4 files changed+102−27

compare with master

unifiedsplit

helix-core/src/lib.rs+1−1 helix-core/src/transaction.rs+46 helix-term/src/commands.rs+11−26 helix-term/tests/test/commands.rs+44

helix-core/src/lib.rs+1−1

@@ -67,4 +67,4 @@ pub use syntax::Syntax;

6767pub use diagnostic::Diagnostic;

6868

6969pub use line_ending::{LineEnding, DEFAULT_LINE_ENDING};

70−pub use transaction::{Assoc, Change, ChangeSet, Operation, Transaction};

70+pub use transaction::{Assoc, Change, ChangeSet, Deletion, Operation, Transaction};

helix-core/src/transaction.rs+46

@@ -5,6 +5,7 @@ use std::borrow::Cow;

66/// (from, to, replacement)

77pub type Change = (usize, usize, Option<Tendril>);

8+pub type Deletion = (usize, usize);

910// TODO: pub(crate)

1011#[derive(Debug, Clone, PartialEq, Eq)]

@@ -534,6 +535,41 @@ impl Transaction {

534535 Self::from(changeset)

535536 }

536537

538+ /// Generate a transaction from a set of potentially overlapping deletions

539+ /// by merging overlapping deletions together.

540+ pub fn delete<I>(doc: &Rope, deletions: I) -> Self

541+ where

542+ I: Iterator<Item = Deletion>,

543+ {

544+ let len = doc.len_chars();

545+

546+ let (lower, upper) = deletions.size_hint();

547+ let size = upper.unwrap_or(lower);

548+ let mut changeset = ChangeSet::with_capacity(2 * size + 1); // rough estimate

549+

550+ let mut last = 0;

551+ for (mut from, to) in deletions {

552+ if last > to {

553+ continue;

554+ }

555+ if last > from {

556+ from = last

557+ }

558+ debug_assert!(

559+ from <= to,

560+ "Edit end must end before it starts (should {from} <= {to})"

561+ );

562+ // Retain from last "to" to current "from"

563+ changeset.retain(from - last);

564+ changeset.delete(to - from);

565+ last = to;

566+ }

567+

568+ changeset.retain(len - last);

569+

570+ Self::from(changeset)

571+ }

572+

537573 /// Generate a transaction with a change per selection range.

538574 pub fn change_by_selection<F>(doc: &Rope, selection: &Selection, f: F) -> Self

539575 where

@@ -580,6 +616,16 @@ impl Transaction {

580616 )

581617 }

582618

619+ /// Generate a transaction with a deletion per selection range.

620+ /// Compared to using `change_by_selection` directly these ranges may overlap.

621+ /// In that case they are merged

622+ pub fn delete_by_selection<F>(doc: &Rope, selection: &Selection, f: F) -> Self

623+ where

624+ F: FnMut(&Range) -> Deletion,

625+ {

626+ Self::delete(doc, selection.iter().map(f))

627+ }

628+

583629 /// Insert text at each selection head.

584630 pub fn insert(doc: &Rope, selection: &Selection, text: Tendril) -> Self {

585631 Self::change_by_selection(doc, selection, |range| {

helix-term/src/commands.rs+11−26

@@ -2315,9 +2315,8 @@ fn delete_selection_impl(cx: &mut Context, op: Operation) {

23152315 };

23162316

23172317 // then delete

2318− let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {

2319− (range.from(), range.to(), None)

2320− });

2318+ let transaction =

2319+ Transaction::delete_by_selection(doc.text(), selection, |range| (range.from(), range.to()));

23212320 doc.apply(&transaction, view.id);

23222321

23232322 match op {

@@ -2333,9 +2332,8 @@ fn delete_selection_impl(cx: &mut Context, op: Operation) {

23332332

23342333#[inline]

23352334fn delete_selection_insert_mode(doc: &mut Document, view: &mut View, selection: &Selection) {

2336− let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {

2337− (range.from(), range.to(), None)

2338− });

2335+ let transaction =

2336+ Transaction::delete_by_selection(doc.text(), selection, |range| (range.from(), range.to()));

23392337 doc.apply(&transaction, view.id);

23402338}

23412339

@@ -3422,10 +3420,10 @@ pub mod insert {

34223420 let auto_pairs = doc.auto_pairs(cx.editor);

34233421

34243422 let transaction =

3425− Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {

3423+ Transaction::delete_by_selection(doc.text(), doc.selection(view.id), |range| {

34263424 let pos = range.cursor(text);

34273425 if pos == 0 {

3428− return (pos, pos, None);

3426+ return (pos, pos);

34293427 }

34303428 let line_start_pos = text.line_to_char(range.cursor_line(text));

34313429 // consider to delete by indent level if all characters before `pos` are indent units.

@@ -3433,11 +3431,7 @@ pub mod insert {

34333431 if !fragment.is_empty() && fragment.chars().all(|ch| ch == ' ' || ch == '\t') {

34343432 if text.get_char(pos.saturating_sub(1)) == Some('\t') {

34353433 // fast path, delete one char

3436− (

3437− graphemes::nth_prev_grapheme_boundary(text, pos, 1),

3438− pos,

3439− None,

3440− )

3434+ (graphemes::nth_prev_grapheme_boundary(text, pos, 1), pos)

34413435 } else {

34423436 let width: usize = fragment

34433437 .chars()

@@ -3464,7 +3458,7 @@ pub mod insert {

34643458 _ => break,

34653459 }

34663460 }

3467− (start, pos, None) // delete!

3461+ (start, pos) // delete!

34683462 }

34693463 } else {

34703464 match (

@@ -3482,17 +3476,12 @@ pub mod insert {

34823476 (

34833477 graphemes::nth_prev_grapheme_boundary(text, pos, count),

34843478 graphemes::nth_next_grapheme_boundary(text, pos, count),

3485− None,

34863479 )

34873480 }

34883481 _ =>

34893482 // delete 1 char

34903483 {

3491− (

3492− graphemes::nth_prev_grapheme_boundary(text, pos, count),

3493− pos,

3494− None,

3495− )

3484+ (graphemes::nth_prev_grapheme_boundary(text, pos, count), pos)

34963485 }

34973486 }

34983487 }

@@ -3508,13 +3497,9 @@ pub mod insert {

35083497 let (view, doc) = current!(cx.editor);

35093498 let text = doc.text().slice(..);

35103499 let transaction =

3511− Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {

3500+ Transaction::delete_by_selection(doc.text(), doc.selection(view.id), |range| {

35123501 let pos = range.cursor(text);

3513− (

3514− pos,

3515− graphemes::nth_next_grapheme_boundary(text, pos, count),

3516− None,

3517− )

3502+ (pos, graphemes::nth_next_grapheme_boundary(text, pos, count))

35183503 });

35193504 doc.apply(&transaction, view.id);

35203505

helix-term/tests/test/commands.rs+44

@@ -385,3 +385,47 @@ async fn test_character_info() -> anyhow::Result<()> {

385385

386386 Ok(())

387387}

388+

389+#[tokio::test(flavor = "multi_thread")]

390+async fn test_delete_char_backward() -> anyhow::Result<()> {

391+ // don't panic when deleting overlapping ranges

392+ test((

393+ platform_line("#(x|)# #[x|]#").as_str(),

394+ "c<space><backspace><esc>",

395+ platform_line("#[\n|]#").as_str(),

396+ ))

397+ .await?;

398+ test((

399+ platform_line("#( |)##( |)#a#( |)#axx#[x|]#a").as_str(),

400+ "li<backspace><esc>",

401+ platform_line("#(a|)##(|a)#xx#[|a]#").as_str(),

402+ ))

403+ .await?;

404+

405+ Ok(())

406+}

407+

408+#[tokio::test(flavor = "multi_thread")]

409+async fn test_delete_word_backward() -> anyhow::Result<()> {

410+ // don't panic when deleting overlapping ranges

411+ test((

412+ platform_line("fo#[o|]#ba#(r|)#").as_str(),

413+ "a<C-w><esc>",

414+ platform_line("#[\n|]#").as_str(),

415+ ))

416+ .await?;

417+ Ok(())

418+}

419+

420+#[tokio::test(flavor = "multi_thread")]

421+async fn test_delete_word_forward() -> anyhow::Result<()> {

422+ // don't panic when deleting overlapping ranges

423+ test((

424+ platform_line("fo#[o|]#b#(|ar)#").as_str(),

425+ "i<A-d><esc>",

426+ platform_line("fo#[\n|]#").as_str(),

427+ ))

428+ .await?;

429+ Ok(())

430+}

431+