Switch to a cleaner range-head moving abstraction.

Also fix a bunch of bugs related to it.

helix-core/src/movement.rs

@@ -42,20 +42,18 @@ pub fn move_horizontally(
     };
 
     // Compute the new position.
-    let mut new_pos = if dir == Direction::Backward {
+    let new_pos = if dir == Direction::Backward {
         nth_prev_grapheme_boundary(slice, pos, count)
     } else {
         nth_next_grapheme_boundary(slice, pos, count)
     };
 
-    // Shift forward one grapheme if needed, for the
-    // visual 1-width cursor.
-    if behaviour == Extend && new_pos >= range.anchor {
-        new_pos = next_grapheme_boundary(slice, new_pos);
-    };
-
     // Compute the final new range.
-    range.put(slice, new_pos, behaviour == Extend)
+    if behaviour == Extend {
+        range.move_head(slice, new_pos, true)
+    } else {
+        Range::point(new_pos)
+    }
 }
 
 pub fn move_vertically(
@@ -78,14 +76,17 @@ pub fn move_vertically(
     let horiz = range.horiz.unwrap_or(col as u32);
 
     // Compute the new position.
-    let new_pos = {
+    let (new_pos, new_row) = {
         let new_row = if dir == Direction::Backward {
             row.saturating_sub(count)
         } else {
             (row + count).min(slice.len_lines().saturating_sub(1))
         };
         let new_col = col.max(horiz as usize);
-        pos_at_coords(slice, Position::new(new_row, new_col), true)
+        (
+            pos_at_coords(slice, Position::new(new_row, new_col), true),
+            new_row,
+        )
     };
 
     // Compute the new range according to the type of movement.
@@ -97,15 +98,13 @@ pub fn move_vertically(
         },
 
         Movement::Extend => {
-            let new_head = if new_pos >= range.anchor {
+            if slice.line(new_row).len_chars() > 0 {
-                next_grapheme_boundary(slice, new_pos)
+                let mut new_range = range.move_head(slice, new_pos, true);
+                new_range.horiz = Some(horiz);
+                new_range
             } else {
-                new_pos
+                range
-            };
+            }
-
-            let mut new_range = range.put(slice, new_head, true);
-            new_range.horiz = Some(horiz);
-            new_range
         }
     }
 }

helix-core/src/search.rs

@@ -1,12 +1,6 @@
 use crate::RopeSlice;
 
-pub fn find_nth_next(
+pub fn find_nth_next(text: RopeSlice, ch: char, mut pos: usize, n: usize) -> Option<usize> {
-    text: RopeSlice,
-    ch: char,
-    mut pos: usize,
-    n: usize,
-    inclusive: bool,
-) -> Option<usize> {
     if pos >= text.len_chars() || n == 0 {
         return None;
     }
@@ -25,20 +19,10 @@ pub fn find_nth_next(
         }
     }
 
-    if !inclusive {
+    Some(pos - 1)
-        pos -= 1;
-    }
-
-    Some(pos)
 }
 
-pub fn find_nth_prev(
+pub fn find_nth_prev(text: RopeSlice, ch: char, mut pos: usize, n: usize) -> Option<usize> {
-    text: RopeSlice,
-    ch: char,
-    mut pos: usize,
-    n: usize,
-    inclusive: bool,
-) -> Option<usize> {
     if pos == 0 || n == 0 {
         return None;
     }
@@ -57,9 +41,5 @@ pub fn find_nth_prev(
         }
     }
 
-    if !inclusive {
-        pos += 1;
-    }
-
     Some(pos)
 }

helix-core/src/selection.rs

@@ -248,18 +248,18 @@ impl Range {
         }
     }
 
-    /// Moves the `Range` to `char_idx`.  If `extend == true`, then only the head
+    /// Moves the head of the `Range` to `char_idx`, adjusting the anchor
-    /// is moved to `char_idx`, and the anchor is adjusted only as needed to
+    /// as needed to preserve 1-width range semantics.
-    /// preserve 1-width range semantics.
+    ///
+    /// `block_cursor` specifies whether it should treat `char_idx` as a block
+    /// cursor position or as a range-end position.
     ///
     /// This method assumes that the range and `char_idx` are already properly
     /// grapheme-aligned.
     #[must_use]
     #[inline]
-    pub fn put(self, text: RopeSlice, char_idx: usize, extend: bool) -> Range {
+    pub fn move_head(self, text: RopeSlice, char_idx: usize, block_cursor: bool) -> Range {
-        let anchor = if !extend {
+        let anchor = if self.head >= self.anchor && char_idx < self.anchor {
-            char_idx
-        } else if self.head >= self.anchor && char_idx < self.anchor {
             next_grapheme_boundary(text, self.anchor)
         } else if self.head < self.anchor && char_idx >= self.anchor {
             prev_grapheme_boundary(text, self.anchor)
@@ -267,7 +267,11 @@ impl Range {
             self.anchor
         };
 
-        Range::new(anchor, char_idx)
+        if block_cursor && anchor <= char_idx {
+            Range::new(anchor, next_grapheme_boundary(text, char_idx))
+        } else {
+            Range::new(anchor, char_idx)
+        }
     }
 
     // groupAt

helix-core/src/surround.rs

@@ -49,19 +49,18 @@ pub fn find_nth_pairs_pos(
         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, true)?)),
+                0 => Some((pos, search::find_nth_next(text, close, pos + 1, n)? + 1)),
-                _ if (pos + 1) == text.len_chars() => Some((
+                _ if (pos + 1) == text.len_chars() => {
-                    search::find_nth_prev(text, open, pos, n, true)?,
+                    Some((search::find_nth_prev(text, open, pos, n)?, text.len_chars()))
-                    text.len_chars(),
+                }
-                )),
                 // 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, true)?,
+                search::find_nth_prev(text, open, pos, n)?,
-                search::find_nth_next(text, close, pos, n, true)?,
+                search::find_nth_next(text, close, pos, n)? + 1,
             ))
         }
     } else {

helix-term/src/commands.rs

@@ -373,15 +373,16 @@ fn goto_line_end(cx: &mut Context) {
 
     let selection = doc.selection(view.id).clone().transform(|range| {
         let line = range.head_line(text);
+        let line_start = text.line_to_char(line);
 
-        let mut pos = line_end_char_index(&text, line);
+        let pos = graphemes::prev_grapheme_boundary(text, line_end_char_index(&text, line))
-        if doc.mode != Mode::Select {
+            .max(line_start);
-            pos = graphemes::prev_grapheme_boundary(text, pos);
-        }
-
-        pos = range.head.max(pos).max(text.line_to_char(line));
 
-        range.put(text, pos, doc.mode == Mode::Select)
+        if doc.mode == Mode::Select {
+            range.move_head(text, pos, true)
+        } else {
+            Range::point(pos)
+        }
     });
     doc.set_selection(view.id, selection);
 }
@@ -392,12 +393,13 @@ fn goto_line_end_newline(cx: &mut Context) {
 
     let selection = doc.selection(view.id).clone().transform(|range| {
         let line = range.head_line(text);
+        let pos = line_end_char_index(&text, line);
 
-        let mut pos = text.line_to_char((line + 1).min(text.len_lines()));
+        if doc.mode == Mode::Select {
-        if doc.mode != Mode::Select {
+            range.move_head(text, pos, true)
-            pos = graphemes::prev_grapheme_boundary(text, pos);
+        } else {
+            Range::point(pos)
         }
-        range.put(text, pos, doc.mode == Mode::Select)
     });
     doc.set_selection(view.id, selection);
 }
@@ -411,7 +413,11 @@ fn goto_line_start(cx: &mut Context) {
 
         // adjust to start of the line
         let pos = text.line_to_char(line);
-        range.put(text, pos, doc.mode == Mode::Select)
+        if doc.mode == Mode::Select {
+            range.move_head(text, pos, true)
+        } else {
+            Range::point(pos)
+        }
     });
     doc.set_selection(view.id, selection);
 }
@@ -425,7 +431,11 @@ fn goto_first_nonwhitespace(cx: &mut Context) {
 
         if let Some(pos) = find_first_non_whitespace_char(text.line(line)) {
             let pos = pos + text.line_to_char(line);
-            range.put(text, pos, doc.mode == Mode::Select)
+            if doc.mode == Mode::Select {
+                range.move_head(text, pos, true)
+            } else {
+                Range::point(pos)
+            }
         } else {
             range
         }
@@ -569,8 +579,8 @@ fn extend_next_word_start(cx: &mut Context) {
         .min_width_1(text)
         .transform(|range| {
             let word = movement::move_next_word_start(text, range, count);
-            let pos = word.head;
+            let pos = graphemes::prev_grapheme_boundary(text, word.head);
-            range.put(text, pos, true)
+            range.move_head(text, pos, true)
         });
     doc.set_selection(view.id, selection);
 }
@@ -587,7 +597,7 @@ fn extend_prev_word_start(cx: &mut Context) {
         .transform(|range| {
             let word = movement::move_prev_word_start(text, range, count);
             let pos = word.head;
-            range.put(text, pos, true)
+            range.move_head(text, pos, true)
         });
     doc.set_selection(view.id, selection);
 }
@@ -603,8 +613,8 @@ fn extend_next_word_end(cx: &mut Context) {
         .min_width_1(text)
         .transform(|range| {
             let word = movement::move_next_word_end(text, range, count);
-            let pos = word.head;
+            let pos = graphemes::prev_grapheme_boundary(text, word.head);
-            range.put(text, pos, true)
+            range.move_head(text, pos, true)
         });
     doc.set_selection(view.id, selection);
 }
@@ -652,11 +662,17 @@ where
         let text = doc.text().slice(..);
 
         let selection = doc.selection(view.id).clone().transform(|range| {
+            let range = if range.anchor < range.head {
+                // For 1-width cursor semantics.
+                Range::new(range.anchor, range.head - 1)
+            } else {
+                range
+            };
             search_fn(text, ch, range.head, count, inclusive).map_or(range, |pos| {
                 if extend {
-                    range.put(text, pos, true)
+                    range.move_head(text, pos, true)
                 } else {
-                    range.put(text, pos.saturating_sub(1), false)
+                    Range::point(pos)
                 }
             })
         });
@@ -664,10 +680,39 @@ where
     })
 }
 
+fn find_next_char_impl(
+    text: RopeSlice,
+    ch: char,
+    pos: usize,
+    n: usize,
+    inclusive: bool,
+) -> Option<usize> {
+    let pos = (pos + 1).min(text.len_chars());
+    if inclusive {
+        search::find_nth_next(text, ch, pos, n)
+    } else {
+        search::find_nth_next(text, ch, pos, n).map(|n| n.saturating_sub(1))
+    }
+}
+
+fn find_prev_char_impl(
+    text: RopeSlice,
+    ch: char,
+    pos: usize,
+    n: usize,
+    inclusive: bool,
+) -> Option<usize> {
+    if inclusive {
+        search::find_nth_prev(text, ch, pos, n)
+    } else {
+        search::find_nth_prev(text, ch, pos, n).map(|n| (n + 1).min(text.len_chars()))
+    }
+}
+
 fn find_till_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_next,
+        find_next_char_impl,
         false, /* inclusive */
         false, /* extend */
     )
@@ -676,7 +721,7 @@ fn find_till_char(cx: &mut Context) {
 fn find_next_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_next,
+        find_next_char_impl,
         true,  /* inclusive */
         false, /* extend */
     )
@@ -685,7 +730,7 @@ fn find_next_char(cx: &mut Context) {
 fn extend_till_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_next,
+        find_next_char_impl,
         false, /* inclusive */
         true,  /* extend */
     )
@@ -694,7 +739,7 @@ fn extend_till_char(cx: &mut Context) {
 fn extend_next_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_next,
+        find_next_char_impl,
         true, /* inclusive */
         true, /* extend */
     )
@@ -703,7 +748,7 @@ fn extend_next_char(cx: &mut Context) {
 fn till_prev_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_prev,
+        find_prev_char_impl,
         false, /* inclusive */
         false, /* extend */
     )
@@ -712,7 +757,7 @@ fn till_prev_char(cx: &mut Context) {
 fn find_prev_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_prev,
+        find_prev_char_impl,
         true,  /* inclusive */
         false, /* extend */
     )
@@ -721,7 +766,7 @@ fn find_prev_char(cx: &mut Context) {
 fn extend_till_prev_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_prev,
+        find_prev_char_impl,
         false, /* inclusive */
         true,  /* extend */
     )
@@ -730,7 +775,7 @@ fn extend_till_prev_char(cx: &mut Context) {
 fn extend_prev_char(cx: &mut Context) {
     find_char_impl(
         cx,
-        search::find_nth_prev,
+        find_prev_char_impl,
         true, /* inclusive */
         true, /* extend */
     )