Skip to content

Commit

Permalink
Fix surround cursor position calculation
Browse files Browse the repository at this point in the history 
Fixes helix-editor#1077. This was caused by the assumption that a block
cursor is represented as zero width internally and simply
rendered to be a single width selection, where as in reality
a block cursor is an actual single width selection in form and
function.

Behavioural changes:

1. Surround selection no longer works when cursor is _on_ a
    surround character that has matching pairs (like `'`
    or `"`). This was the intended behaviour from the start
    but worked till now because of the cursor position
    calculation mismatch.
  • Loading branch information
@sudormrfbin
sudormrfbin committed Nov 27, 2021
1 parent f24e5a3 commit cd4caea
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 72 deletions.
6 changes: 3 additions & 3 deletions helix-core/src/selection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,10 @@ impl Range {
}

impl From<(usize, usize)> for Range {
fn from(tuple: (usize, usize)) -> Self {
fn from((anchor, head): (usize, usize)) -> Self {
Self {
anchor: tuple.0,
head: tuple.1,
anchor,
head,
horiz: None,
}
}
Expand Down
148 changes: 83 additions & 65 deletions helix-core/src/surround.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{search, Selection};
use crate::{search, Range, Selection};
use ropey::RopeSlice;

pub const PAIRS: &[(char, char)] = &[
Expand Down Expand Up @@ -35,33 +35,27 @@ pub fn get_pair(ch: char) -> (char, char) {
pub fn find_nth_pairs_pos(
text: RopeSlice,
ch: char,
pos: usize,
range: Range,
n: usize,
) -> Option<(usize, usize)> {
let (open, close) = get_pair(ch);

if text.len_chars() < 2 || pos >= text.len_chars() {
if text.len_chars() < 2 || range.to() >= text.len_chars() {
return None;
}

let (open, close) = get_pair(ch);
let pos = range.cursor(text);

if open == close {
if Some(open) == text.get_char(pos) {
// Special case: cursor is directly on a matching char.
match pos {
0 => Some((pos, search::find_nth_next(text, close, pos + 1, n)?)),
_ if (pos + 1) == text.len_chars() => {
Some((search::find_nth_prev(text, open, pos, n)?, pos))
}
// We return no match because there's no way to know which
// side of the char we should be searching on.
_ => None,
}
} else {
Some((
search::find_nth_prev(text, open, pos, n)?,
search::find_nth_next(text, close, pos, n)?,
))
// Cursor is directly on match char. We return no match
// because there's no way to know which side of the char
// we should be searching on.
return None;
}
Some((
search::find_nth_prev(text, open, pos, n)?,
search::find_nth_next(text, close, pos, n)?,
))
} else {
Some((
find_nth_open_pair(text, open, close, pos, n)?,
Expand Down Expand Up @@ -160,8 +154,8 @@ pub fn get_surround_pos(
) -> Option<Vec<usize>> {
let mut change_pos = Vec::new();

for range in selection {
let (open_pos, close_pos) = find_nth_pairs_pos(text, ch, range.head, skip)?;
for &range in selection {
let (open_pos, close_pos) = find_nth_pairs_pos(text, ch, range, skip)?;
if change_pos.contains(&open_pos) || change_pos.contains(&close_pos) {
return None;
}
Expand All @@ -178,67 +172,91 @@ mod test {
use ropey::Rope;
use smallvec::SmallVec;

#[test]
fn test_find_nth_pairs_pos() {
let doc = Rope::from("some (text) here");
fn check_find_nth_pair_pos(
text: &str,
cases: Vec<(usize, char, usize, Option<(usize, usize)>)>,
) {
let doc = Rope::from(text);
let slice = doc.slice(..);

// cursor on [t]ext
assert_eq!(find_nth_pairs_pos(slice, '(', 6, 1), Some((5, 10)));
assert_eq!(find_nth_pairs_pos(slice, ')', 6, 1), Some((5, 10)));
// cursor on so[m]e
assert_eq!(find_nth_pairs_pos(slice, '(', 2, 1), None);
// cursor on bracket itself
assert_eq!(find_nth_pairs_pos(slice, '(', 5, 1), Some((5, 10)));
assert_eq!(find_nth_pairs_pos(slice, '(', 10, 1), Some((5, 10)));
for (cursor_pos, ch, n, expected_range) in cases {
let range = find_nth_pairs_pos(slice, ch, (cursor_pos, cursor_pos + 1).into(), n);
assert_eq!(
range, expected_range,
"Expected {:?}, got {:?}",
expected_range, range
);
}
}

#[test]
fn test_find_nth_pairs_pos_skip() {
let doc = Rope::from("(so (many (good) text) here)");
let slice = doc.slice(..);
fn test_find_nth_pairs_pos() {
check_find_nth_pair_pos(
"some (text) here",
vec![
// cursor on [t]ext
(6, '(', 1, Some((5, 10))),
(6, ')', 1, Some((5, 10))),
// cursor on so[m]e
(2, '(', 1, None),
// cursor on bracket itself
(5, '(', 1, Some((5, 10))),
(10, '(', 1, Some((5, 10))),
],
);
}

// cursor on go[o]d
assert_eq!(find_nth_pairs_pos(slice, '(', 13, 1), Some((10, 15)));
assert_eq!(find_nth_pairs_pos(slice, '(', 13, 2), Some((4, 21)));
assert_eq!(find_nth_pairs_pos(slice, '(', 13, 3), Some((0, 27)));
#[test]
fn test_find_nth_pairs_pos_skip() {
check_find_nth_pair_pos(
"(so (many (good) text) here)",
vec![
// cursor on go[o]d
(13, '(', 1, Some((10, 15))),
(13, '(', 2, Some((4, 21))),
(13, '(', 3, Some((0, 27))),
],
);
}

#[test]
fn test_find_nth_pairs_pos_same() {
let doc = Rope::from("'so 'many 'good' text' here'");
let slice = doc.slice(..);

// cursor on go[o]d
assert_eq!(find_nth_pairs_pos(slice, '\'', 13, 1), Some((10, 15)));
assert_eq!(find_nth_pairs_pos(slice, '\'', 13, 2), Some((4, 21)));
assert_eq!(find_nth_pairs_pos(slice, '\'', 13, 3), Some((0, 27)));
// cursor on the quotes
assert_eq!(find_nth_pairs_pos(slice, '\'', 10, 1), None);
// this is the best we can do since opening and closing pairs are same
assert_eq!(find_nth_pairs_pos(slice, '\'', 0, 1), Some((0, 4)));
assert_eq!(find_nth_pairs_pos(slice, '\'', 27, 1), Some((21, 27)));
check_find_nth_pair_pos(
"'so 'many 'good' text' here'",
vec![
// cursor on go[o]d
(13, '\'', 1, Some((10, 15))),
(13, '\'', 2, Some((4, 21))),
(13, '\'', 3, Some((0, 27))),
// cursor on the quotes
(10, '\'', 1, None),
],
)
}

#[test]
fn test_find_nth_pairs_pos_step() {
let doc = Rope::from("((so)((many) good (text))(here))");
let slice = doc.slice(..);

// cursor on go[o]d
assert_eq!(find_nth_pairs_pos(slice, '(', 15, 1), Some((5, 24)));
assert_eq!(find_nth_pairs_pos(slice, '(', 15, 2), Some((0, 31)));
check_find_nth_pair_pos(
"((so)((many) good (text))(here))",
vec![
// cursor on go[o]d
(15, '(', 1, Some((5, 24))),
(15, '(', 2, Some((0, 31))),
],
)
}

#[test]
fn test_find_nth_pairs_pos_mixed() {
let doc = Rope::from("(so [many {good} text] here)");
let slice = doc.slice(..);

// cursor on go[o]d
assert_eq!(find_nth_pairs_pos(slice, '{', 13, 1), Some((10, 15)));
assert_eq!(find_nth_pairs_pos(slice, '[', 13, 1), Some((4, 21)));
assert_eq!(find_nth_pairs_pos(slice, '(', 13, 1), Some((0, 27)));
check_find_nth_pair_pos(
"(so [many {good} text] here)",
vec![
// cursor on go[o]d
(13, '{', 1, Some((10, 15))),
(13, '[', 1, Some((4, 21))),
(13, '(', 1, Some((0, 27))),
],
)
}

#[test]
Expand Down
10 changes: 6 additions & 4 deletions helix-core/src/textobject.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ pub fn textobject_surround(
ch: char,
count: usize,
) -> Range {
surround::find_nth_pairs_pos(slice, ch, range.head, count)
surround::find_nth_pairs_pos(slice, ch, range, count)
.map(|(anchor, head)| match textobject {
TextObject::Inside => Range::new(next_grapheme_boundary(slice, anchor), head),
TextObject::Around => Range::new(anchor, next_grapheme_boundary(slice, head)),
Expand Down Expand Up @@ -170,7 +170,7 @@ mod test {

#[test]
fn test_textobject_word() {
// (text, [(cursor position, textobject, final range), ...])
// (text, [(char position, textobject, final range), ...])
let tests = &[
(
"cursor at beginning of doc",
Expand Down Expand Up @@ -269,7 +269,9 @@ mod test {
let slice = doc.slice(..);
for &case in scenario {
let (pos, objtype, expected_range) = case;
let result = textobject_word(slice, Range::point(pos), objtype, 1, false);
// cursor is a single width selection
let range = Range::new(pos, pos + 1);
let result = textobject_word(slice, range, objtype, 1, false);
assert_eq!(
result,
expected_range.into(),
Expand All @@ -283,7 +285,7 @@ mod test {

#[test]
fn test_textobject_surround() {
// (text, [(cursor position, textobject, final range, count), ...])
// (text, [(cursor position, textobject, final range, surround char, count), ...])
let tests = &[
(
"simple (single) surround pairs",
Expand Down

0 comments on commit cd4caea

Please sign in to comment.