Movement fixes, refactor and unit test suite (#217)

* Add convenience/clarity wrapper for Range initialization

* Test horizontal moves

* Add column jumping tests

* Add failing movement conditions for multi-word moves

* Refactor skip_over_next

* Add complex forward movement unit tests

* Add strict whitespace checks and edge case tests

* Restore formatting

* Remove unused function

* Add empty test case for deletion and fix nth_prev_word_boundary

* Add tests for backward motion

* Refactor word movement

* Address review comments and finish refactoring backwards move

* Finish unit test suite

* Fmt pass

* Fix lint erors

* Clean up diff restoring bad 'cargo fmt' actions

* Simplify movement closures (thanks Pickfire)

* Fmt pass

* Replace index-based movement with iterator based movement, ensuring that each move incurs a single call to the RopeSlice API

* Break down tuple function

* Extract common logic to all movement functions

* Split iterator helpers away into their own module

* WIP reducing clones

* Operate on spans

* WIP simplifying iterators

* Simplify motion helpers

* Fix iterator

* Fix all unit tests

* Refactor and simplify

* Simplify fold

helix-core/src/auto_pairs.rs

@@ -67,7 +67,7 @@ fn handle_open(
 
     let mut offs = 0;
 
-    let mut transaction = Transaction::change_by_selection(doc, selection, |range| {
+    let transaction = Transaction::change_by_selection(doc, selection, |range| {
         let pos = range.head;
         let next = next_char(doc, pos);
 
@@ -109,7 +109,7 @@ fn handle_close(doc: &Rope, selection: &Selection, _open: char, close: char) ->
 
     let mut offs = 0;
 
-    let mut transaction = Transaction::change_by_selection(doc, selection, |range| {
+    let transaction = Transaction::change_by_selection(doc, selection, |range| {
         let pos = range.head;
         let next = next_char(doc, pos);
 

helix-core/src/lib.rs

@@ -16,7 +16,6 @@ pub mod selection;
 mod state;
 pub mod syntax;
 mod transaction;
-pub mod words;
 
 pub fn find_first_non_whitespace_char(line: RopeSlice) -> Option<usize> {
     line.chars().position(|ch| !ch.is_whitespace())

helix-core/src/movement.rs

@@ -1,5 +1,12 @@
-use crate::graphemes::{nth_next_grapheme_boundary, nth_prev_grapheme_boundary, RopeGraphemes};
+use std::iter::{self, from_fn, Peekable, SkipWhile};
-use crate::{coords_at_pos, pos_at_coords, ChangeSet, Position, Range, Rope, RopeSlice, Selection};
+
+use ropey::iter::Chars;
+
+use crate::{
+    coords_at_pos,
+    graphemes::{nth_next_grapheme_boundary, nth_prev_grapheme_boundary},
+    pos_at_coords, Position, Range, RopeSlice,
+};
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum Direction {
@@ -7,39 +14,49 @@ pub enum Direction {
     Backward,
 }
 
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub enum Movement {
+    Extend,
+    Move,
+}
+
 pub fn move_horizontally(
-    text: RopeSlice,
+    slice: RopeSlice,
     range: Range,
     dir: Direction,
     count: usize,
-    extend: bool,
+    behaviour: Movement,
 ) -> Range {
     let pos = range.head;
-    let line = text.char_to_line(pos);
+    let line = slice.char_to_line(pos);
     // TODO: we can optimize clamping by passing in RopeSlice limited to current line. that way
     // we stop calculating past start/end of line.
     let pos = match dir {
         Direction::Backward => {
-            let start = text.line_to_char(line);
+            let start = slice.line_to_char(line);
-            nth_prev_grapheme_boundary(text, pos, count).max(start)
+            nth_prev_grapheme_boundary(slice, pos, count).max(start)
         }
         Direction::Forward => {
             // Line end is pos at the start of next line - 1
-            let end = text.line_to_char(line + 1).saturating_sub(1);
+            let end = slice.line_to_char(line + 1).saturating_sub(1);
-            nth_next_grapheme_boundary(text, pos, count).min(end)
+            nth_next_grapheme_boundary(slice, pos, count).min(end)
         }
     };
-    Range::new(if extend { range.anchor } else { pos }, pos)
+    let anchor = match behaviour {
+        Movement::Extend => range.anchor,
+        Movement::Move => pos,
+    };
+    Range::new(anchor, pos)
 }
 
 pub fn move_vertically(
-    text: RopeSlice,
+    slice: RopeSlice,
     range: Range,
     dir: Direction,
     count: usize,
-    extend: bool,
+    behaviour: Movement,
 ) -> Range {
-    let Position { row, col } = coords_at_pos(text, range.head);
+    let Position { row, col } = coords_at_pos(slice, range.head);
 
     let horiz = range.horiz.unwrap_or(col as u32);
 
@@ -47,138 +64,62 @@ pub fn move_vertically(
         Direction::Backward => row.saturating_sub(count),
         Direction::Forward => std::cmp::min(
             row.saturating_add(count),
-            text.len_lines().saturating_sub(2),
+            slice.len_lines().saturating_sub(2),
         ),
     };
 
     // convert to 0-indexed, subtract another 1 because len_chars() counts \n
-    let new_line_len = text.line(new_line).len_chars().saturating_sub(2);
+    let new_line_len = slice.line(new_line).len_chars().saturating_sub(2);
 
     let new_col = std::cmp::min(horiz as usize, new_line_len);
 
-    let pos = pos_at_coords(text, Position::new(new_line, new_col));
+    let pos = pos_at_coords(slice, Position::new(new_line, new_col));
 
-    let mut range = Range::new(if extend { range.anchor } else { pos }, pos);
+    let anchor = match behaviour {
+        Movement::Extend => range.anchor,
+        Movement::Move => pos,
+    };
+
+    let mut range = Range::new(anchor, pos);
     range.horiz = Some(horiz);
     range
 }
 
-pub fn move_next_word_start(slice: RopeSlice, mut begin: usize, count: usize) -> Option<Range> {
+pub fn move_next_word_start(slice: RopeSlice, range: Range, count: usize) -> Range {
-    let mut end = begin;
+    word_move(slice, range, count, WordMotionTarget::NextWordStart)
-
-    for _ in 0..count {
-        if begin + 1 == slice.len_chars() {
-            return None;
-        }
-
-        let mut ch = slice.char(begin);
-        let next = slice.char(begin + 1);
-
-        // if we're at the end of a word, or on whitespce right before new one
-        if categorize(ch) != categorize(next) {
-            begin += 1;
-        }
-
-        if !skip_over_next(slice, &mut begin, |ch| ch == '\n') {
-            return None;
-        };
-        ch = slice.char(begin);
-
-        end = begin + 1;
-
-        if is_word(ch) {
-            skip_over_next(slice, &mut end, is_word);
-        } else if is_punctuation(ch) {
-            skip_over_next(slice, &mut end, is_punctuation);
-        }
-
-        skip_over_next(slice, &mut end, char::is_whitespace);
-    }
-
-    Some(Range::new(begin, end - 1))
 }
 
-pub fn move_prev_word_start(slice: RopeSlice, mut begin: usize, count: usize) -> Option<Range> {
+pub fn move_next_word_end(slice: RopeSlice, range: Range, count: usize) -> Range {
-    let mut with_end = false;
+    word_move(slice, range, count, WordMotionTarget::NextWordEnd)
-    let mut end = begin;
-
-    for _ in 0..count {
-        if begin == 0 {
-            return None;
-        }
-
-        let ch = slice.char(begin);
-        let prev = slice.char(begin - 1);
-
-        if categorize(ch) != categorize(prev) {
-            begin -= 1;
-        }
-
-        // return if not skip while?
-        skip_over_prev(slice, &mut begin, |ch| ch == '\n');
-
-        end = begin;
-
-        with_end = skip_over_prev(slice, &mut end, char::is_whitespace);
-
-        // refetch
-        let ch = slice.char(end);
-
-        if is_word(ch) {
-            with_end = skip_over_prev(slice, &mut end, is_word);
-        } else if is_punctuation(ch) {
-            with_end = skip_over_prev(slice, &mut end, is_punctuation);
-        }
-    }
-
-    Some(Range::new(begin, if with_end { end } else { end + 1 }))
 }
 
-pub fn move_next_word_end(slice: RopeSlice, mut begin: usize, count: usize) -> Option<Range> {
+pub fn move_prev_word_start(slice: RopeSlice, range: Range, count: usize) -> Range {
-    let mut end = begin;
+    word_move(slice, range, count, WordMotionTarget::PrevWordStart)
-
+}
-    for _ in 0..count {
-        if begin + 2 >= slice.len_chars() {
-            return None;
-        }
-
-        let ch = slice.char(begin);
-        let next = slice.char(begin + 1);
-
-        if categorize(ch) != categorize(next) {
-            begin += 1;
-        }
-
-        if !skip_over_next(slice, &mut begin, |ch| ch == '\n') {
-            return None;
-        };
-
-        end = begin;
-
-        skip_over_next(slice, &mut end, char::is_whitespace);
-
-        // refetch
-        let ch = slice.char(end);
-
-        if is_word(ch) {
-            skip_over_next(slice, &mut end, is_word);
-        } else if is_punctuation(ch) {
-            skip_over_next(slice, &mut end, is_punctuation);
-        }
-    }
 
-    Some(Range::new(begin, end - 1))
+fn word_move(slice: RopeSlice, mut range: Range, count: usize, target: WordMotionTarget) -> Range {
+    (0..count).fold(range, |range, _| {
+        slice.chars_at(range.head).range_to_target(target, range)
+    })
 }
 
 // ---- util ------------
-
-// used for by-word movement
-
 #[inline]
 pub(crate) fn is_word(ch: char) -> bool {
     ch.is_alphanumeric() || ch == '_'
 }
 
+#[inline]
+pub(crate) fn is_end_of_line(ch: char) -> bool {
+    ch == '\n'
+}
+
+#[inline]
+// Whitespace, but not end of line
+pub(crate) fn is_strict_whitespace(ch: char) -> bool {
+    ch.is_whitespace() && !is_end_of_line(ch)
+}
+
 #[inline]
 pub(crate) fn is_punctuation(ch: char) -> bool {
     use unicode_general_category::{get_general_category, GeneralCategory};
@@ -199,7 +140,7 @@ pub(crate) fn is_punctuation(ch: char) -> bool {
 }
 
 #[derive(Debug, Eq, PartialEq)]
-pub(crate) enum Category {
+pub enum Category {
     Whitespace,
     Eol,
     Word,
@@ -209,7 +150,7 @@ pub(crate) enum Category {
 
 #[inline]
 pub(crate) fn categorize(ch: char) -> Category {
-    if ch == '\n' {
+    if is_end_of_line(ch) {
         Category::Eol
     } else if ch.is_whitespace() {
         Category::Whitespace
@@ -223,46 +164,160 @@ pub(crate) fn categorize(ch: char) -> Category {
 }
 
 #[inline]
-/// Returns true if there are more characters left after the new position.
+/// Returns first index that doesn't satisfy a given predicate when
-pub fn skip_over_next<F>(slice: RopeSlice, pos: &mut usize, fun: F) -> bool
+/// advancing the character index.
+///
+/// Returns none if all characters satisfy the predicate.
+pub fn skip_while<F>(slice: RopeSlice, pos: usize, fun: F) -> Option<usize>
 where
     F: Fn(char) -> bool,
 {
-    let mut chars = slice.chars_at(*pos);
+    let mut chars = slice.chars_at(pos).enumerate();
-
+    chars.find_map(|(i, c)| if !fun(c) { Some(pos + i) } else { None })
-    #[allow(clippy::while_let_on_iterator)]
-    while let Some(ch) = chars.next() {
-        if !fun(ch) {
-            break;
-        }
-        *pos += 1;
-    }
-    chars.next().is_some()
 }
 
 #[inline]
-/// Returns true if the final pos matches the predicate.
+/// Returns first index that doesn't satisfy a given predicate when
-pub fn skip_over_prev<F>(slice: RopeSlice, pos: &mut usize, fun: F) -> bool
+/// retreating the character index, saturating if all elements satisfy
+/// the condition.
+pub fn backwards_skip_while<F>(slice: RopeSlice, pos: usize, fun: F) -> Option<usize>
 where
     F: Fn(char) -> bool,
 {
-    // need to +1 so that prev() includes current char
+    let mut chars_starting_from_next = slice.chars_at(pos + 1);
-    let mut chars = slice.chars_at(*pos + 1);
+    let mut backwards = iter::from_fn(|| chars_starting_from_next.prev()).enumerate();
+    backwards.find_map(|(i, c)| {
+        if !fun(c) {
+            Some(pos.saturating_sub(i))
+        } else {
+            None
+        }
+    })
+}
 
-    #[allow(clippy::while_let_on_iterator)]
+/// Possible targets of a word motion
-    while let Some(ch) = chars.prev() {
+#[derive(Copy, Clone, Debug)]
-        if !fun(ch) {
+pub enum WordMotionTarget {
-            break;
+    NextWordStart,
+    NextWordEnd,
+    PrevWordStart,
+}
+
+pub trait CharHelpers {
+    fn range_to_target(&mut self, target: WordMotionTarget, origin: Range) -> Range;
+}
+
+enum WordMotionPhase {
+    Start,
+    SkipNewlines,
+    ReachTarget,
+}
+
+impl CharHelpers for Chars<'_> {
+    fn range_to_target(&mut self, target: WordMotionTarget, origin: Range) -> Range {
+        let range = origin;
+        // Characters are iterated forward or backwards depending on the motion direction.
+        let characters: Box<dyn Iterator<Item = char>> = match target {
+            WordMotionTarget::PrevWordStart => {
+                self.next();
+                Box::new(from_fn(|| self.prev()))
+            }
+            _ => Box::new(self),
+        };
+
+        // Index advancement also depends on the direction.
+        let advance: &dyn Fn(&mut usize) = match target {
+            WordMotionTarget::PrevWordStart => &|u| *u = u.saturating_sub(1),
+            _ => &|u| *u += 1,
+        };
+
+        let mut characters = characters.peekable();
+        let mut phase = WordMotionPhase::Start;
+        let mut head = origin.head;
+        let mut anchor: Option<usize> = None;
+        let is_boundary = |a: char, b: Option<char>| categorize(a) != categorize(b.unwrap_or(a));
+        while let Some(peek) = characters.peek().copied() {
+            phase = match phase {
+                WordMotionPhase::Start => {
+                    characters.next();
+                    if characters.peek().is_none() {
+                        break; // We're at the end, so there's nothing to do.
+                    }
+                    // Anchor may remain here if the head wasn't at a boundary
+                    if !is_boundary(peek, characters.peek().copied()) && !is_end_of_line(peek) {
+                        anchor = Some(head);
+                    }
+                    // First character is always skipped by the head
+                    advance(&mut head);
+                    WordMotionPhase::SkipNewlines
+                }
+                WordMotionPhase::SkipNewlines => {
+                    if is_end_of_line(peek) {
+                        characters.next();
+                        if characters.peek().is_some() {
+                            advance(&mut head);
+                        }
+                        WordMotionPhase::SkipNewlines
+                    } else {
+                        WordMotionPhase::ReachTarget
+                    }
+                }
+                WordMotionPhase::ReachTarget => {
+                    characters.next();
+                    anchor = anchor.or(Some(head));
+                    if reached_target(target, peek, characters.peek()) {
+                        break;
+                    } else {
+                        advance(&mut head);
+                    }
+                    WordMotionPhase::ReachTarget
+                }
+            }
+        }
+        Range::new(anchor.unwrap_or(origin.anchor), head)
+    }
+}
+
+fn reached_target(target: WordMotionTarget, peek: char, next_peek: Option<&char>) -> bool {
+    let next_peek = match next_peek {
+        Some(next_peek) => next_peek,
+        None => return true,
+    };
+
+    match target {
+        WordMotionTarget::NextWordStart => {
+            ((categorize(peek) != categorize(*next_peek))
+                && (is_end_of_line(*next_peek) || !next_peek.is_whitespace()))
+        }
+        WordMotionTarget::NextWordEnd | WordMotionTarget::PrevWordStart => {
+            ((categorize(peek) != categorize(*next_peek))
+                && (!peek.is_whitespace() || is_end_of_line(*next_peek)))
         }
-        *pos = pos.saturating_sub(1);
     }
-    fun(slice.char(*pos))
 }
 
 #[cfg(test)]
 mod test {
+    use std::array::{self, IntoIter};
+
+    use ropey::Rope;
+
     use super::*;
 
+    const SINGLE_LINE_SAMPLE: &str = "This is a simple alphabetic line";
+    const MULTILINE_SAMPLE: &str = "\
+        Multiline\n\
+        text sample\n\
+        which\n\
+        is merely alphabetic\n\
+        and whitespaced\n\
+    ";
+
+    const MULTIBYTE_CHARACTER_SAMPLE: &str = "\
+        パーティーへ行かないか\n\
+        The text above is Japanese\n\
+    ";
+
     #[test]
     fn test_vertical_move() {
         let text = Rope::from("abcd\nefg\nwrs");
@@ -273,17 +328,445 @@ mod test {
         assert_eq!(
             coords_at_pos(
                 slice,
-                move_vertically(slice, range, Direction::Forward, 1, false).head
+                move_vertically(slice, range, Direction::Forward, 1, Movement::Move).head
             ),
             (1, 2).into()
         );
     }
 
+    #[test]
+    fn horizontal_moves_through_single_line_in_single_line_text() {
+        let text = Rope::from(SINGLE_LINE_SAMPLE);
+        let slice = text.slice(..);
+        let position = pos_at_coords(slice, (0, 0).into());
+
+        let mut range = Range::point(position);
+
+        let moves_and_expected_coordinates = [
+            ((Direction::Forward, 1usize), (0, 1)),
+            ((Direction::Forward, 2usize), (0, 3)),
+            ((Direction::Forward, 0usize), (0, 3)),
+            ((Direction::Forward, 999usize), (0, 31)),
+            ((Direction::Forward, 999usize), (0, 31)),
+            ((Direction::Backward, 999usize), (0, 0)),
+        ];
+
+        for ((direction, amount), coordinates) in IntoIter::new(moves_and_expected_coordinates) {
+            range = move_horizontally(slice, range, direction, amount, Movement::Move);
+            assert_eq!(coords_at_pos(slice, range.head), coordinates.into())
+        }
+    }
+
+    #[test]
+    fn horizontal_moves_through_single_line_in_multiline_text() {
+        let text = Rope::from(MULTILINE_SAMPLE);
+        let slice = text.slice(..);
+        let position = pos_at_coords(slice, (0, 0).into());
+
+        let mut range = Range::point(position);
+
+        let moves_and_expected_coordinates = IntoIter::new([
+            ((Direction::Forward, 1usize), (0, 1)),    // M_ltiline
+            ((Direction::Forward, 2usize), (0, 3)),    // Mul_iline
+            ((Direction::Backward, 6usize), (0, 0)),   // _ultiline
+            ((Direction::Backward, 999usize), (0, 0)), // _ultiline
+            ((Direction::Forward, 3usize), (0, 3)),    // Mul_iline
+            ((Direction::Forward, 0usize), (0, 3)),    // Mul_iline
+            ((Direction::Backward, 0usize), (0, 3)),   // Mul_iline
+            ((Direction::Forward, 999usize), (0, 9)),  // Multilin_
+            ((Direction::Forward, 999usize), (0, 9)),  // Multilin_
+        ]);
+
+        for ((direction, amount), coordinates) in moves_and_expected_coordinates {
+            range = move_horizontally(slice, range, direction, amount, Movement::Move);
+            assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
+            assert_eq!(range.head, range.anchor);
+        }
+    }
+
+    #[test]
+    fn selection_extending_moves_in_single_line_text() {
+        let text = Rope::from(SINGLE_LINE_SAMPLE);
+        let slice = text.slice(..);
+        let position = pos_at_coords(slice, (0, 0).into());
+
+        let mut range = Range::point(position);
+        let original_anchor = range.anchor;
+
+        let moves = IntoIter::new([
+            (Direction::Forward, 1usize),
+            (Direction::Forward, 5usize),
+            (Direction::Backward, 3usize),
+        ]);
+
+        for (direction, amount) in moves {
+            range = move_horizontally(slice, range, direction, amount, Movement::Extend);
+            assert_eq!(range.anchor, original_anchor);
+        }
+    }
+
+    #[test]
+    fn vertical_moves_in_single_column() {
+        let text = Rope::from(MULTILINE_SAMPLE);
+        let slice = dbg!(&text).slice(..);
+        let position = pos_at_coords(slice, (0, 0).into());
+        let mut range = Range::point(position);
+        let moves_and_expected_coordinates = IntoIter::new([
+            ((Direction::Forward, 1usize), (1, 0)),
+            ((Direction::Forward, 2usize), (3, 0)),
+            ((Direction::Backward, 999usize), (0, 0)),
+            ((Direction::Forward, 3usize), (3, 0)),
+            ((Direction::Forward, 0usize), (3, 0)),
+            ((Direction::Backward, 0usize), (3, 0)),
+            ((Direction::Forward, 5), (4, 0)),
+            ((Direction::Forward, 999usize), (4, 0)),
+        ]);
+
+        for ((direction, amount), coordinates) in moves_and_expected_coordinates {
+            range = move_vertically(slice, range, direction, amount, Movement::Move);
+            assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
+            assert_eq!(range.head, range.anchor);
+        }
+    }
+
+    #[test]
+    fn vertical_moves_jumping_column() {
+        let text = Rope::from(MULTILINE_SAMPLE);
+        let slice = text.slice(..);
+        let position = pos_at_coords(slice, (0, 0).into());
+        let mut range = Range::point(position);
+
+        enum Axis {
+            H,
+            V,
+        }
+        let moves_and_expected_coordinates = IntoIter::new([
+            // Places cursor at the end of line
+            ((Axis::H, Direction::Forward, 8usize), (0, 8)),
+            // First descent preserves column as the target line is wider
+            ((Axis::V, Direction::Forward, 1usize), (1, 8)),
+            // Second descent clamps column as the target line is shorter
+            ((Axis::V, Direction::Forward, 1usize), (2, 4)),
+            // Third descent restores the original column
+            ((Axis::V, Direction::Forward, 1usize), (3, 8)),
+            // Behaviour is preserved even through long jumps
+            ((Axis::V, Direction::Backward, 999usize), (0, 8)),
+            ((Axis::V, Direction::Forward, 999usize), (4, 8)),
+        ]);
+
+        for ((axis, direction, amount), coordinates) in moves_and_expected_coordinates {
+            range = match axis {
+                Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move),
+                Axis::V => move_vertically(slice, range, direction, amount, Movement::Move),
+            };
+            assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
+            assert_eq!(range.head, range.anchor);
+        }
+    }
+
+    #[test]
+    fn multibyte_character_column_jumps() {
+        let text = Rope::from(MULTIBYTE_CHARACTER_SAMPLE);
+        let slice = text.slice(..);
+        let position = pos_at_coords(slice, (0, 0).into());
+        let mut range = Range::point(position);
+
+        // FIXME: The behaviour captured in this test diverges from both Kakoune and Vim. These
+        // will attempt to preserve the horizontal position of the cursor, rather than
+        // placing it at the same character index.
+        enum Axis {
+            H,
+            V,
+        }
+        let moves_and_expected_coordinates = IntoIter::new([
+            // Places cursor at the fourth kana
+            ((Axis::H, Direction::Forward, 4), (0, 4)),
+            // Descent places cursor at the fourth character.
+            ((Axis::V, Direction::Forward, 1usize), (1, 4)),
+        ]);
+
+        for ((axis, direction, amount), coordinates) in moves_and_expected_coordinates {
+            range = match axis {
+                Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move),
+                Axis::V => move_vertically(slice, range, direction, amount, Movement::Move),
+            };
+            assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
+            assert_eq!(range.head, range.anchor);
+        }
+    }
+
+    #[test]
+    #[should_panic]
+    fn nonsensical_ranges_panic_on_forward_movement_attempt_in_debug_mode() {
+        move_next_word_start(Rope::from("Sample").slice(..), Range::point(99999999), 1);
+    }
+
+    #[test]
+    #[should_panic]
+    fn nonsensical_ranges_panic_on_forward_to_end_movement_attempt_in_debug_mode() {
+        move_next_word_end(Rope::from("Sample").slice(..), Range::point(99999999), 1);
+    }
+
+    #[test]
+    #[should_panic]
+    fn nonsensical_ranges_panic_on_backwards_movement_attempt_in_debug_mode() {
+        move_prev_word_start(Rope::from("Sample").slice(..), Range::point(99999999), 1);
+    }
+
+    #[test]
+    fn test_behaviour_when_moving_to_start_of_next_words() {
+        let tests = array::IntoIter::new([
+            ("Basic forward motion stops at the first space",
+                vec![(1, Range::new(0, 0), Range::new(0, 5))]),
+            (" Starting from a boundary advances the anchor",
+                vec![(1, Range::new(0, 0), Range::new(1, 9))]),
+            ("Long       whitespace gap is bridged by the head",
+                vec![(1, Range::new(0, 0), Range::new(0, 10))]),
+            ("Previous anchor is irrelevant for forward motions",
+                vec![(1, Range::new(12, 0), Range::new(0, 8))]),
+            ("    Starting from whitespace moves to last space in sequence",
+                vec![(1, Range::new(0, 0), Range::new(0, 3))]),
+            ("Starting from mid-word leaves anchor at start position and moves head",
+                vec![(1, Range::new(3, 3), Range::new(3, 8))]),
+            ("Identifiers_with_underscores are considered a single word",
+                vec![(1, Range::new(0, 0), Range::new(0, 28))]),
+            ("Jumping\n    into starting whitespace selects the spaces before 'into'",
+                vec![(1, Range::new(0, 6), Range::new(8, 11))]),
+            ("alphanumeric.!,and.?=punctuation are considered 'words' for the purposes of word motion",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 11)),
+                    (1, Range::new(0, 11), Range::new(12, 14)),
+                    (1, Range::new(12, 14), Range::new(15, 17))
+                ]),
+            ("...   ... punctuation and spaces behave as expected",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 5)),
+                    (1, Range::new(0, 5), Range::new(6, 9)),
+                ]),
+            (".._.._ punctuation is not joined by underscores into a single block",
+                vec![(1, Range::new(0, 0), Range::new(0, 1))]),
+            ("Newlines\n\nare bridged seamlessly.",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 7)),
+                    (1, Range::new(0, 7), Range::new(10, 13)),
+                ]),
+            ("Jumping\n\n\n\n\n\n   from newlines to whitespace selects whitespace.",
+                vec![
+                    (1, Range::new(0, 8), Range::new(13, 15)),
+                ]),
+            ("A failed motion does not modify the range",
+                vec![
+                    (3, Range::new(37, 41), Range::new(37, 41)),
+                ]),
+            ("oh oh oh two character words!",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 2)),
+                    (1, Range::new(0, 2), Range::new(3, 5)),
+                    (1, Range::new(0, 1), Range::new(2, 2)),
+                ]),
+            ("Multiple motions at once resolve correctly",
+                vec![
+                    (3, Range::new(0, 0), Range::new(17, 19)),
+                ]),
+            ("Excessive motions are performed partially",
+                vec![
+                    (999, Range::new(0, 0), Range::new(32, 40)),
+                ]),
+            ("", // Edge case of moving forward in empty string
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 0)),
+                ]),
+            ("\n\n\n\n\n", // Edge case of moving forward in all newlines
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 4)),
+                ]),
+            ("\n   \n   \n Jumping through alternated space blocks and newlines selects the space blocks",
+                vec![
+                    (1, Range::new(0, 0), Range::new(1, 3)),
+                    (1, Range::new(1, 3), Range::new(5, 7)),
+                ]),
+            ("ヒーリクス multibyte characters behave as normal characters",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 5)),
+                ]),
+        ]);
+
+        for (sample, scenario) in tests {
+            for (count, begin, expected_end) in scenario.into_iter() {
+                let range = move_next_word_start(Rope::from(sample).slice(..), begin, count);
+                assert_eq!(range, expected_end, "Case failed: [{}]", sample);
+            }
+        }
+    }
+
+    #[test]
+    fn test_behaviour_when_moving_to_start_of_previous_words() {
+        let tests = array::IntoIter::new([
+            ("Basic backward motion from the middle of a word",
+                vec![(1, Range::new(3, 3), Range::new(3, 0))]),
+            ("Starting from after boundary retreats the anchor",
+                vec![(1, Range::new(0, 8), Range::new(7, 0))]),
+            ("    Jump to start of a word preceded by whitespace",
+                vec![(1, Range::new(5, 5), Range::new(5, 4))]),
+            ("    Jump to start of line from start of word preceded by whitespace",
+                vec![(1, Range::new(4, 4), Range::new(3, 0))]),
+            ("Previous anchor is irrelevant for backward motions",
+                vec![(1, Range::new(12, 5), Range::new(5, 0))]),
+            ("    Starting from whitespace moves to first space in sequence",
+                vec![(1, Range::new(0, 3), Range::new(3, 0))]),
+            ("Identifiers_with_underscores are considered a single word",
+                vec![(1, Range::new(0, 20), Range::new(20, 0))]),
+            ("Jumping\n    \nback through a newline selects whitespace",
+                vec![(1, Range::new(0, 13), Range::new(11, 8))]),
+            ("Jumping to start of word from the end selects the word",
+                vec![(1, Range::new(6, 6), Range::new(6, 0))]),
+            ("alphanumeric.!,and.?=punctuation are considered 'words' for the purposes of word motion",
+                vec![
+                    (1, Range::new(30, 30), Range::new(30, 21)),
+                    (1, Range::new(30, 21), Range::new(20, 18)),
+                    (1, Range::new(20, 18), Range::new(17, 15))
+                ]),
+
+            ("...   ... punctuation and spaces behave as expected",
+                vec![
+                    (1, Range::new(0, 10), Range::new(9, 6)),
+                    (1, Range::new(9, 6), Range::new(5, 0)),
+                ]),
+            (".._.._ punctuation is not joined by underscores into a single block",
+                vec![(1, Range::new(0, 5), Range::new(4, 3))]),
+            ("Newlines\n\nare bridged seamlessly.",
+                vec![
+                    (1, Range::new(0, 10), Range::new(7, 0)),
+                ]),
+            ("Jumping    \n\n\n\n\nback from within a newline group selects previous block",
+                vec![
+                    (1, Range::new(0, 13), Range::new(10, 0)),
+                ]),
+            ("Failed motions do not modify the range",
+                vec![
+                    (0, Range::new(3, 0), Range::new(3, 0)),
+                ]),
+            ("Multiple motions at once resolve correctly",
+                vec![
+                    (3, Range::new(18, 18), Range::new(8, 0)),
+                ]),
+            ("Excessive motions are performed partially",
+                vec![
+                    (999, Range::new(40, 40), Range::new(9, 0)),
+                ]),
+            ("", // Edge case of moving backwards in empty string
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 0)),
+                ]),
+            ("\n\n\n\n\n", // Edge case of moving backwards in all newlines
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 0)),
+                ]),
+            ("   \n   \nJumping back through alternated space blocks and newlines selects the space blocks",
+                vec![
+                    (1, Range::new(0, 7), Range::new(6, 4)),
+                    (1, Range::new(6, 4), Range::new(2, 0)),
+                ]),
+            ("ヒーリクス multibyte characters behave as normal characters",
+                vec![
+                    (1, Range::new(0, 5), Range::new(4, 0)),
+                ]),
+        ]);
+
+        for (sample, scenario) in tests {
+            for (count, begin, expected_end) in scenario.into_iter() {
+                let range = move_prev_word_start(Rope::from(sample).slice(..), begin, count);
+                assert_eq!(range, expected_end, "Case failed: [{}]", sample);
+            }
+        }
+    }
+
+    #[test]
+    fn test_behaviour_when_moving_to_end_of_next_words() {
+        let tests = array::IntoIter::new([
+            ("Basic forward motion from the start of a word to the end of it",
+                vec![(1, Range::new(0, 0), Range::new(0, 4))]),
+            ("Basic forward motion from the end of a word to the end of the next",
+                vec![(1, Range::new(0, 4), Range::new(5, 12))]),
+            ("Basic forward motion from the middle of a word to the end of it",
+                vec![(1, Range::new(2, 2), Range::new(2, 4))]),
+            ("    Jumping to end of a word preceded by whitespace",
+                vec![(1, Range::new(0, 0), Range::new(0, 10))]),
+            (" Starting from a boundary advances the anchor",
+                vec![(1, Range::new(0, 0), Range::new(1, 8))]),
+            ("Previous anchor is irrelevant for end of word motion",
+                vec![(1, Range::new(12, 2), Range::new(2, 7))]),
+            ("Identifiers_with_underscores are considered a single word",
+                vec![(1, Range::new(0, 0), Range::new(0, 27))]),
+            ("Jumping\n    into starting whitespace selects up to the end of next word",
+                vec![(1, Range::new(0, 6), Range::new(8, 15))]),
+            ("alphanumeric.!,and.?=punctuation are considered 'words' for the purposes of word motion",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 11)),
+                    (1, Range::new(0, 11), Range::new(12, 14)),
+                    (1, Range::new(12, 14), Range::new(15, 17))
+                ]),
+            ("...   ... punctuation and spaces behave as expected",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 2)),
+                    (1, Range::new(0, 2), Range::new(3, 8)),
+                ]),
+            (".._.._ punctuation is not joined by underscores into a single block",
+                vec![(1, Range::new(0, 0), Range::new(0, 1))]),
+            ("Newlines\n\nare bridged seamlessly.",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 7)),
+                    (1, Range::new(0, 7), Range::new(10, 12)),
+                ]),
+            ("Jumping\n\n\n\n\n\n   from newlines to whitespace selects to end of next word.",
+                vec![
+                    (1, Range::new(0, 8), Range::new(13, 19)),
+                ]),
+            ("A failed motion does not modify the range",
+                vec![
+                    (3, Range::new(37, 41), Range::new(37, 41)),
+                ]),
+            ("Multiple motions at once resolve correctly",
+                vec![
+                    (3, Range::new(0, 0), Range::new(16, 18)),
+                ]),
+            ("Excessive motions are performed partially",
+                vec![
+                    (999, Range::new(0, 0), Range::new(31, 40)),
+                ]),
+            ("", // Edge case of moving forward in empty string
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 0)),
+                ]),
+            ("\n\n\n\n\n", // Edge case of moving forward in all newlines
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 4)),
+                ]),
+            ("\n   \n   \n Jumping through alternated space blocks and newlines selects the space blocks",
+                vec![
+                    (1, Range::new(0, 0), Range::new(1, 3)),
+                    (1, Range::new(1, 3), Range::new(5, 7)),
+                ]),
+            ("ヒーリクス multibyte characters behave as normal characters",
+                vec![
+                    (1, Range::new(0, 0), Range::new(0, 4)),
+                ]),
+        ]);
+
+        for (sample, scenario) in tests {
+            for (count, begin, expected_end) in scenario.into_iter() {
+                let range = move_next_word_end(Rope::from(sample).slice(..), begin, count);
+                assert_eq!(range, expected_end, "Case failed: [{}]", sample);
+            }
+        }
+    }
+
     #[test]
     fn test_categorize() {
         const WORD_TEST_CASE: &'static str =
             "_hello_world_あいうえおー1234567890１２３４５６７８９０";
-        const PUNCTUATION_TEST_CASE: &'static str = "!\"#$%&\'()*+,-./:;<=>?@[\\]^`{|}~！”＃＄％＆’（）＊＋、。：；＜＝＞？＠「」＾｀｛｜｝～";
+        const PUNCTUATION_TEST_CASE: &'static str =
+            "!\"#$%&\'()*+,-./:;<=>?@[\\]^`{|}~！”＃＄％＆’（）＊＋、。：；＜＝＞？＠「」＾｀｛｜｝～";
         const WHITESPACE_TEST_CASE: &'static str = "  　   ";
 
         assert_eq!(Category::Eol, categorize('\n'));

helix-core/src/selection.rs

@@ -35,6 +35,10 @@ impl Range {
         }
     }
 
+    pub fn point(head: usize) -> Self {
+        Self::new(head, head)
+    }
+
     /// Start of the range.
     #[inline]
     #[must_use]

helix-core/src/words.rs

@@ -1,68 +0,0 @@
-use crate::movement::{categorize, is_punctuation, is_word, skip_over_prev};
-use ropey::RopeSlice;
-
-#[must_use]
-pub fn nth_prev_word_boundary(slice: RopeSlice, mut char_idx: usize, count: usize) -> usize {
-    let mut with_end = false;
-
-    for _ in 0..count {
-        if char_idx == 0 {
-            break;
-        }
-
-        // return if not skip while?
-        skip_over_prev(slice, &mut char_idx, |ch| ch == '\n');
-
-        with_end = skip_over_prev(slice, &mut char_idx, char::is_whitespace);
-
-        // refetch
-        let ch = slice.char(char_idx);
-
-        if is_word(ch) {
-            with_end = skip_over_prev(slice, &mut char_idx, is_word);
-        } else if is_punctuation(ch) {
-            with_end = skip_over_prev(slice, &mut char_idx, is_punctuation);
-        }
-    }
-
-    if with_end || char_idx == 0 {
-        char_idx
-    } else {
-        char_idx + 1
-    }
-}
-
-#[test]
-fn different_prev_word_boundary() {
-    use ropey::Rope;
-    let t = |x, y| {
-        let text = Rope::from(x);
-        let out = nth_prev_word_boundary(text.slice(..), text.len_chars().saturating_sub(1), 1);
-        assert_eq!(text.slice(..out), y, r#"from "{}""#, x);
-    };
-    t("abcd\nefg\nwrs", "abcd\nefg\n");
-    t("abcd\nefg\n", "abcd\n");
-    t("abcd\n", "");
-    t("hello, world!", "hello, world");
-    t("hello, world", "hello, ");
-    t("hello, ", "hello");
-    t("hello", "");
-    t(",", "");
-    t("こんにちは、世界！", "こんにちは、世界");
-    t("こんにちは、世界", "こんにちは、");
-    t("こんにちは、", "こんにちは");
-    t("こんにちは", "");
-    t("この世界。", "この世界");
-    t("この世界", "");
-    t("お前はもう死んでいる", "");
-    t("その300円です", ""); // TODO: should stop at 300
-    t("唱k", ""); // TODO: should stop at 唱
-    t("，", "");
-    t("1 + 1 = 2", "1 + 1 = ");
-    t("1 + 1 =", "1 + 1 ");
-    t("1 + 1", "1 + ");
-    t("1 + ", "1 ");
-    t("1 ", "");
-    t("1+1=2", "1+1=");
-    t("", "");
-}

helix-term/src/commands.rs

@@ -4,8 +4,8 @@ use helix_core::{
     movement::{self, Direction},
     object, pos_at_coords,
     regex::{self, Regex},
-    register, search, selection, words, Change, ChangeSet, Position, Range, Rope, RopeSlice,
+    register, search, selection, Change, ChangeSet, Position, Range, Rope, RopeSlice, Selection,
-    Selection, SmallVec, Tendril, Transaction,
+    SmallVec, Tendril, Transaction,
 };
 
 use helix_view::{
@@ -19,6 +19,7 @@ use helix_lsp::{
     util::{lsp_pos_to_pos, pos_to_lsp_pos, range_to_lsp_range},
     OffsetEncoding,
 };
+use movement::Movement;
 
 use crate::{
     compositor::{Callback, Component, Compositor},
@@ -29,8 +30,10 @@ use crate::application::{LspCallbackWrapper, LspCallbacks};
 use futures_util::FutureExt;
 use std::future::Future;
 
-use std::borrow::Cow;
+use std::{
-use std::path::{Path, PathBuf};
+    borrow::Cow,
+    path::{Path, PathBuf},
+};
 
 use crossterm::event::{KeyCode, KeyEvent};
 use once_cell::sync::Lazy;
@@ -132,13 +135,7 @@ pub fn move_char_left(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_horizontally(
+        movement::move_horizontally(text, range, Direction::Backward, count, Movement::Move)
-            text,
-            range,
-            Direction::Backward,
-            count,
-            false, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -148,13 +145,7 @@ pub fn move_char_right(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_horizontally(
+        movement::move_horizontally(text, range, Direction::Forward, count, Movement::Move)
-            text,
-            range,
-            Direction::Forward,
-            count,
-            false, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -164,13 +155,7 @@ pub fn move_line_up(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_vertically(
+        movement::move_vertically(text, range, Direction::Backward, count, Movement::Move)
-            text,
-            range,
-            Direction::Backward,
-            count,
-            false, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -180,13 +165,7 @@ pub fn move_line_down(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_vertically(
+        movement::move_vertically(text, range, Direction::Forward, count, Movement::Move)
-            text,
-            range,
-            Direction::Forward,
-            count,
-            false, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -249,9 +228,9 @@ pub fn move_next_word_start(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
 
-    let selection = doc.selection(view.id).transform(|range| {
+    let selection = doc
-        movement::move_next_word_start(text, range.head, count).unwrap_or(range)
+        .selection(view.id)
-    });
+        .transform(|range| movement::move_next_word_start(text, range, count));
 
     doc.set_selection(view.id, selection);
 }
@@ -261,9 +240,9 @@ pub fn move_prev_word_start(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
 
-    let selection = doc.selection(view.id).transform(|range| {
+    let selection = doc
-        movement::move_prev_word_start(text, range.head, count).unwrap_or(range)
+        .selection(view.id)
-    });
+        .transform(|range| movement::move_prev_word_start(text, range, count));
 
     doc.set_selection(view.id, selection);
 }
@@ -275,7 +254,7 @@ pub fn move_next_word_end(cx: &mut Context) {
 
     let selection = doc
         .selection(view.id)
-        .transform(|range| movement::move_next_word_end(text, range.head, count).unwrap_or(range));
+        .transform(|range| movement::move_next_word_end(text, range, count));
 
     doc.set_selection(view.id, selection);
 }
@@ -300,7 +279,7 @@ pub fn extend_next_word_start(cx: &mut Context) {
     let text = doc.text().slice(..);
 
     let selection = doc.selection(view.id).transform(|mut range| {
-        let word = movement::move_next_word_start(text, range.head, count).unwrap_or(range);
+        let word = movement::move_next_word_start(text, range, count);
         let pos = word.head;
         Range::new(range.anchor, pos)
     });
@@ -314,7 +293,7 @@ pub fn extend_prev_word_start(cx: &mut Context) {
     let text = doc.text().slice(..);
 
     let selection = doc.selection(view.id).transform(|mut range| {
-        let word = movement::move_prev_word_start(text, range.head, count).unwrap_or(range);
+        let word = movement::move_prev_word_start(text, range, count);
         let pos = word.head;
         Range::new(range.anchor, pos)
     });
@@ -327,7 +306,7 @@ pub fn extend_next_word_end(cx: &mut Context) {
     let text = doc.text().slice(..);
 
     let selection = doc.selection(view.id).transform(|mut range| {
-        let word = movement::move_next_word_end(text, range.head, count).unwrap_or(range);
+        let word = movement::move_next_word_end(text, range, count);
         let pos = word.head;
         Range::new(range.anchor, pos)
     });
@@ -578,13 +557,7 @@ pub fn extend_char_left(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_horizontally(
+        movement::move_horizontally(text, range, Direction::Backward, count, Movement::Extend)
-            text,
-            range,
-            Direction::Backward,
-            count,
-            true, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -594,13 +567,7 @@ pub fn extend_char_right(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_horizontally(
+        movement::move_horizontally(text, range, Direction::Forward, count, Movement::Extend)
-            text,
-            range,
-            Direction::Forward,
-            count,
-            true, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -610,13 +577,7 @@ pub fn extend_line_up(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_vertically(
+        movement::move_vertically(text, range, Direction::Backward, count, Movement::Extend)
-            text,
-            range,
-            Direction::Backward,
-            count,
-            true, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -626,13 +587,7 @@ pub fn extend_line_down(cx: &mut Context) {
     let (view, doc) = cx.current();
     let text = doc.text().slice(..);
     let selection = doc.selection(view.id).transform(|range| {
-        movement::move_vertically(
+        movement::move_vertically(text, range, Direction::Forward, count, Movement::Extend)
-            text,
-            range,
-            Direction::Forward,
-            count,
-            true, /* extend */
-        )
     });
     doc.set_selection(view.id, selection);
 }
@@ -1920,15 +1875,11 @@ pub mod insert {
         let count = cx.count();
         let (view, doc) = cx.current();
         let text = doc.text().slice(..);
-        let transaction =
+        let selection = doc
-            Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {
+            .selection(view.id)
-                (
+            .transform(|range| movement::move_prev_word_start(text, range, count));
-                    words::nth_prev_word_boundary(text, range.head, count),
+        doc.set_selection(view.id, selection);
-                    range.head,
+        delete_selection(cx)
-                    None,
-                )
-            });
-        doc.apply(&transaction, view.id);
     }
 }
 
@@ -2183,7 +2134,7 @@ pub fn format_selections(cx: &mut Context) {
 }
 
 pub fn join_selections(cx: &mut Context) {
-    use movement::skip_over_next;
+    use movement::skip_while;
     let (view, doc) = cx.current();
     let text = doc.text();
     let slice = doc.text().slice(..);
@@ -2204,7 +2155,7 @@ pub fn join_selections(cx: &mut Context) {
         for line in lines {
             let mut start = text.line_to_char(line + 1).saturating_sub(1);
             let mut end = start + 1;
-            skip_over_next(slice, &mut end, |ch| matches!(ch, ' ' | '\t'));
+            end = skip_while(slice, end, |ch| matches!(ch, ' ' | '\t')).unwrap_or(end);
 
             // need to skip from start, not end
             let change = (start, end, Some(fragment.clone()));