Fix surround cursor position calculation (#1183)

Fixes #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.

helix-core/src/selection.rs

@@ -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,
+            anchor,
-            head: tuple.1,
+            head,
             horiz: None,
         }
     }

helix-core/src/surround.rs

@@ -1,4 +1,4 @@
-use crate::{search, Selection};
+use crate::{search, Range, Selection};
 use ropey::RopeSlice;
 
 pub const PAIRS: &[(char, char)] = &[
@@ -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 || range.to() >= text.len_chars() {
-
-    if text.len_chars() < 2 || pos >= 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.
+            // Cursor is directly on match char. We return no match
-            match pos {
+            // because there's no way to know which side of the char
-                0 => Some((pos, search::find_nth_next(text, close, pos + 1, n)?)),
+            // we should be searching on.
-                _ if (pos + 1) == text.len_chars() => {
+            return None;
-                    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)?,
-            ))
         }
+        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)?,
@@ -160,8 +154,8 @@ pub fn get_surround_pos(
 ) -> Option<Vec<usize>> {
     let mut change_pos = Vec::new();
 
-    for range in selection {
+    for &range in selection {
-        let (open_pos, close_pos) = find_nth_pairs_pos(text, ch, range.head, skip)?;
+        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;
         }
@@ -178,67 +172,91 @@ mod test {
     use ropey::Rope;
     use smallvec::SmallVec;
 
-    #[test]
+    fn check_find_nth_pair_pos(
-    fn test_find_nth_pairs_pos() {
+        text: &str,
-        let doc = Rope::from("some (text) here");
+        cases: Vec<(usize, char, usize, Option<(usize, usize)>)>,
+    ) {
+        let doc = Rope::from(text);
         let slice = doc.slice(..);
 
-        // cursor on [t]ext
+        for (cursor_pos, ch, n, expected_range) in cases {
-        assert_eq!(find_nth_pairs_pos(slice, '(', 6, 1), Some((5, 10)));
+            let range = find_nth_pairs_pos(slice, ch, (cursor_pos, cursor_pos + 1).into(), n);
-        assert_eq!(find_nth_pairs_pos(slice, ')', 6, 1), Some((5, 10)));
+            assert_eq!(
-        // cursor on so[m]e
+                range, expected_range,
-        assert_eq!(find_nth_pairs_pos(slice, '(', 2, 1), None);
+                "Expected {:?}, got {:?}",
-        // cursor on bracket itself
+                expected_range, range
-        assert_eq!(find_nth_pairs_pos(slice, '(', 5, 1), Some((5, 10)));
+            );
-        assert_eq!(find_nth_pairs_pos(slice, '(', 10, 1), Some((5, 10)));
+        }
     }
 
     #[test]
-    fn test_find_nth_pairs_pos_skip() {
+    fn test_find_nth_pairs_pos() {
-        let doc = Rope::from("(so (many (good) text) here)");
+        check_find_nth_pair_pos(
-        let slice = doc.slice(..);
+            "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
+    #[test]
-        assert_eq!(find_nth_pairs_pos(slice, '(', 13, 1), Some((10, 15)));
+    fn test_find_nth_pairs_pos_skip() {
-        assert_eq!(find_nth_pairs_pos(slice, '(', 13, 2), Some((4, 21)));
+        check_find_nth_pair_pos(
-        assert_eq!(find_nth_pairs_pos(slice, '(', 13, 3), Some((0, 27)));
+            "(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'");
+        check_find_nth_pair_pos(
-        let slice = doc.slice(..);
+            "'so 'many 'good' text' here'",
-
+            vec![
-        // cursor on go[o]d
+                // cursor on go[o]d
-        assert_eq!(find_nth_pairs_pos(slice, '\'', 13, 1), Some((10, 15)));
+                (13, '\'', 1, Some((10, 15))),
-        assert_eq!(find_nth_pairs_pos(slice, '\'', 13, 2), Some((4, 21)));
+                (13, '\'', 2, Some((4, 21))),
-        assert_eq!(find_nth_pairs_pos(slice, '\'', 13, 3), Some((0, 27)));
+                (13, '\'', 3, Some((0, 27))),
-        // cursor on the quotes
+                // cursor on the quotes
-        assert_eq!(find_nth_pairs_pos(slice, '\'', 10, 1), None);
+                (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)));
     }
 
     #[test]
     fn test_find_nth_pairs_pos_step() {
-        let doc = Rope::from("((so)((many) good (text))(here))");
+        check_find_nth_pair_pos(
-        let slice = doc.slice(..);
+            "((so)((many) good (text))(here))",
-
+            vec![
-        // cursor on go[o]d
+                // cursor on go[o]d
-        assert_eq!(find_nth_pairs_pos(slice, '(', 15, 1), Some((5, 24)));
+                (15, '(', 1, Some((5, 24))),
-        assert_eq!(find_nth_pairs_pos(slice, '(', 15, 2), Some((0, 31)));
+                (15, '(', 2, Some((0, 31))),
+            ],
+        )
     }
 
     #[test]
     fn test_find_nth_pairs_pos_mixed() {
-        let doc = Rope::from("(so [many {good} text] here)");
+        check_find_nth_pair_pos(
-        let slice = doc.slice(..);
+            "(so [many {good} text] here)",
-
+            vec![
-        // cursor on go[o]d
+                // cursor on go[o]d
-        assert_eq!(find_nth_pairs_pos(slice, '{', 13, 1), Some((10, 15)));
+                (13, '{', 1, Some((10, 15))),
-        assert_eq!(find_nth_pairs_pos(slice, '[', 13, 1), Some((4, 21)));
+                (13, '[', 1, Some((4, 21))),
-        assert_eq!(find_nth_pairs_pos(slice, '(', 13, 1), Some((0, 27)));
+                (13, '(', 1, Some((0, 27))),
+            ],
+        )
     }
 
     #[test]

helix-core/src/textobject.rs

@@ -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)),
@@ -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",
@@ -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(),
@@ -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",