f8225ed · helix · git.iko.soy

git.iko.soy/helix

on master help sign in

fix panic when deleting overlapping ranges

f8225edPascal Kuthe4 years ago

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

compare with master

unifiedsplit

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

67pub use diagnostic::Diagnostic;

69pub 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;

6/// (from, to, replacement)

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

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

9// TODO: pub(crate)

10// TODO: pub(crate)

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

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

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

534 Self::from(changeset)

535 Self::from(changeset)

535 }

536 }

536

537

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+

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

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

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

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

539 where

575 where

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

580 )

616 )

581 }

617 }

582

618

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+

583 /// Insert text at each selection head.

629 /// Insert text at each selection head.

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

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

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

631 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) {

2315 };

2316

2317 // then delete

2318+ let transaction =

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

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

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

2320− });

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

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

2322

2321

2323 match op {

2322 match op {

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

2333

2332

2334#[inline]

2333#[inline]

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

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

2335+ let transaction =

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

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

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

2338− });

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

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

2340}

2338}

2341

2339

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

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

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

3423

3421

3424 let transaction =

3422 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| {

3426 let pos = range.cursor(text);

3424 let pos = range.cursor(text);

3427 if pos == 0 {

3425 if pos == 0 {

3428− return (pos, pos, None);

3426+ return (pos, pos);

3429 }

3427 }

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

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

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

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

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

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

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

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

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

3435 // fast path, delete one char

3433 // fast path, delete one char

3436− (

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

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

3438− pos,

3439− None,

3440− )

3441 } else {

3435 } else {

3442 let width: usize = fragment

3436 let width: usize = fragment

3443 .chars()

3437 .chars()

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

3464 _ => break,

3458 _ => break,

3465 }

3459 }

3466 }

3460 }

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

3461+ (start, pos) // delete!

3468 }

3462 }

3469 } else {

3463 } else {

3470 match (

3464 match (

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

3482 (

3476 (

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

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

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

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

3485− None,

3486 )

3479 )

3487 }

3480 }

3488 _ =>

3481 _ =>

3489 // delete 1 char

3482 // delete 1 char

3490 {

3483 {

3491− (

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

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

3493− pos,

3494− None,

3495− )

3496 }

3485 }

3497 }

3486 }

3498 }

3487 }

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

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

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

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

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

3510 let transaction =

3499 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| {

3512 let pos = range.cursor(text);

3501 let pos = range.cursor(text);

3513− (

3514− pos,

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

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

3516− None,

3517− )

3518 });

3503 });

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

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

3520

3505

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

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

385

386 Ok(())

387}

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+