Skip to content

Commit

Permalink
Fix range offsets for multiple shell insertions (helix-editor#4619)
Browse files Browse the repository at this point in the history
d6323b7 introduced a regression for
shell commands like `|`, `!`, and `<A-!>` which caused the new
selections to be incorrect. This caused a panic when piping (`|`)
would cause the new range to extend past the document end.

The paste version of this bug was fixed in
48a3965.

This change also inherits the direction of the new range from the old
range and adds integration tests to ensure that the behavior isn't
broken in the future.
  • Loading branch information
the-mikedavis authored and Frederik Vestre committed Feb 6, 2023
1 parent eb6b3b7 commit ea98ec8
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 6 deletions.
24 changes: 18 additions & 6 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4773,6 +4773,8 @@ fn shell(cx: &mut compositor::Context, cmd: &str, behavior: &ShellBehavior) {
let mut ranges = SmallVec::with_capacity(selection.len());
let text = doc.text().slice(..);

let mut offset = 0isize;

for range in selection.ranges() {
let fragment = range.slice(text);
let (output, success) = match shell_impl(shell, cmd, pipe.then(|| fragment.into())) {
Expand All @@ -4788,13 +4790,23 @@ fn shell(cx: &mut compositor::Context, cmd: &str, behavior: &ShellBehavior) {
return;
}

let (from, to) = match behavior {
ShellBehavior::Replace => (range.from(), range.to()),
ShellBehavior::Insert => (range.from(), range.from()),
ShellBehavior::Append => (range.to(), range.to()),
_ => (range.from(), range.from()),
let output_len = output.chars().count();

let (from, to, deleted_len) = match behavior {
ShellBehavior::Replace => (range.from(), range.to(), range.len()),
ShellBehavior::Insert => (range.from(), range.from(), 0),
ShellBehavior::Append => (range.to(), range.to(), 0),
_ => (range.from(), range.from(), 0),
};
ranges.push(Range::new(to, to + output.chars().count()));

// These `usize`s cannot underflow because selection ranges cannot overlap.
// Once the MSRV is 1.66.0 (mixed_integer_ops is stabilized), we can use checked
// arithmetic to assert this.
let anchor = (to as isize + offset - deleted_len as isize) as usize;
let new_range = Range::new(anchor, anchor + output_len).with_direction(range.direction());
ranges.push(new_range);
offset = offset + output_len as isize - deleted_len as isize;

changes.push((from, to, Some(output)));
}

Expand Down
68 changes: 68 additions & 0 deletions helix-term/tests/test/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,71 @@ async fn test_multi_selection_paste() -> anyhow::Result<()> {

Ok(())
}

#[tokio::test(flavor = "multi_thread")]
async fn test_multi_selection_shell_commands() -> anyhow::Result<()> {
// pipe
test((
platform_line(indoc! {"\
#[|lorem]#
#(|ipsum)#
#(|dolor)#
"})
.as_str(),
"|echo foo<ret>",
platform_line(indoc! {"\
#[|foo
]#
#(|foo
)#
#(|foo
)#
"})
.as_str(),
))
.await?;

// insert-output
test((
platform_line(indoc! {"\
#[|lorem]#
#(|ipsum)#
#(|dolor)#
"})
.as_str(),
"!echo foo<ret>",
platform_line(indoc! {"\
#[|foo
]#lorem
#(|foo
)#ipsum
#(|foo
)#dolor
"})
.as_str(),
))
.await?;

// append-output
test((
platform_line(indoc! {"\
#[|lorem]#
#(|ipsum)#
#(|dolor)#
"})
.as_str(),
"<A-!>echo foo<ret>",
platform_line(indoc! {"\
lorem#[|foo
]#
ipsum#(|foo
)#
dolor#(|foo
)#
"})
.as_str(),
))
.await?;

Ok(())
}

0 comments on commit ea98ec8

Please sign in to comment.