From 86af55c379c531df2d5dbc72841e28a10fc7938e Mon Sep 17 00:00:00 2001 From: PabloMansanet Date: Fri, 11 Jun 2021 14:57:07 +0200 Subject: [PATCH] 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 --- book/src/keymap.md | 42 +- helix-core/src/auto_pairs.rs | 4 +- helix-core/src/lib.rs | 1 - helix-core/src/movement.rs | 775 ++++++++++++++++++++++++++++------- helix-core/src/selection.rs | 4 + helix-core/src/words.rs | 68 --- helix-term/src/commands.rs | 113 ++--- 7 files changed, 688 insertions(+), 319 deletions(-) delete mode 100644 helix-core/src/words.rs diff --git a/book/src/keymap.md b/book/src/keymap.md index c7ffbb26e..65d01b86a 100644 --- a/book/src/keymap.md +++ b/book/src/keymap.md @@ -19,7 +19,7 @@ | F | find previous char | | Home | move to the start of the line | | End | move to the end of the line | -| m | Jump to matching bracket | +| m | Jump to matching bracket | | PageUp | Move page up | | PageDown | Move page down | | ctrl-u | Move half page up | @@ -42,10 +42,10 @@ | R | replace with yanked text | | i | Insert before selection | | a | Insert after selection (append) | -| I | Insert at the start of the line | -| A | Insert at the end of the line | -| o | Open new line below selection | -| o | Open new line above selection | +| I | Insert at the start of the line | +| A | Insert at the end of the line | +| o | Open new line below selection | +| o | Open new line above selection | | u | Undo change | | U | Redo change | | y | Yank selection | @@ -54,26 +54,26 @@ | > | Indent selection | | < | Unindent selection | | = | Format selection | -| d | Delete selection | -| c | Change selection (delete and enter insert mode) | +| d | Delete selection | +| c | Change selection (delete and enter insert mode) | ### Selection manipulation | Key | Description | |-----|-----------| -| s | Select all regex matches inside selections | -| S | Split selection into subselections on regex matches | -| alt-s | Split selection on newlines | -| ; | Collapse selection onto a single cursor | -| alt-; | Flip selection cursor and anchor | -| % | Select entire file | -| x | Select current line | -| X | Extend to next line | -| [ | Expand selection to parent syntax node TODO: pick a key | +| s | Select all regex matches inside selections | +| S | Split selection into subselections on regex matches | +| alt-s | Split selection on newlines | +| ; | Collapse selection onto a single cursor | +| alt-; | Flip selection cursor and anchor | +| % | Select entire file | +| x | Select current line | +| X | Extend to next line | +| [ | Expand selection to parent syntax node TODO: pick a key | | J | join lines inside selection | | K | keep selections matching the regex TODO: overlapped by hover help | | space | keep only the primary selection TODO: overlapped by space mode | -| ctrl-c | Comment/uncomment the selections | +| ctrl-c | Comment/uncomment the selections | ### Search @@ -82,10 +82,10 @@ in reverse, or searching via smartcase. | Key | Description | |-----|-----------| -| / | Search for regex pattern | -| n | Select next search match | -| N | Add next search match to selection | -| * | Use current selection as the search pattern | +| / | Search for regex pattern | +| n | Select next search match | +| N | Add next search match to selection | +| * | Use current selection as the search pattern | ### Diagnostics diff --git a/helix-core/src/auto_pairs.rs b/helix-core/src/auto_pairs.rs index a04b0d3e9..74e25ac90 100644 --- a/helix-core/src/auto_pairs.rs +++ b/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); diff --git a/helix-core/src/lib.rs b/helix-core/src/lib.rs index f5e7de520..8f4239b11 100644 --- a/helix-core/src/lib.rs +++ b/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 { line.chars().position(|ch| !ch.is_whitespace()) diff --git a/helix-core/src/movement.rs b/helix-core/src/movement.rs index 32dfcae34..8b1e802f4 100644 --- a/helix-core/src/movement.rs +++ b/helix-core/src/movement.rs @@ -1,5 +1,12 @@ -use crate::graphemes::{nth_next_grapheme_boundary, nth_prev_grapheme_boundary, RopeGraphemes}; -use crate::{coords_at_pos, pos_at_coords, ChangeSet, Position, Range, Rope, RopeSlice, Selection}; +use std::iter::{self, from_fn, Peekable, SkipWhile}; + +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); - nth_prev_grapheme_boundary(text, pos, count).max(start) + let start = slice.line_to_char(line); + 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); - nth_next_grapheme_boundary(text, pos, count).min(end) + let end = slice.line_to_char(line + 1).saturating_sub(1); + 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 { - let mut end = begin; - - 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_next_word_start(slice: RopeSlice, range: Range, count: usize) -> Range { + word_move(slice, range, count, WordMotionTarget::NextWordStart) } -pub fn move_prev_word_start(slice: RopeSlice, mut begin: usize, count: usize) -> Option { - let mut with_end = false; - 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, range: Range, count: usize) -> Range { + word_move(slice, range, count, WordMotionTarget::NextWordEnd) } -pub fn move_next_word_end(slice: RopeSlice, mut begin: usize, count: usize) -> Option { - let mut end = begin; - - 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); - } - } +pub fn move_prev_word_start(slice: RopeSlice, range: Range, count: usize) -> Range { + word_move(slice, range, count, WordMotionTarget::PrevWordStart) +} - 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. -pub fn skip_over_next(slice: RopeSlice, pos: &mut usize, fun: F) -> bool +/// Returns first index that doesn't satisfy a given predicate when +/// advancing the character index. +/// +/// Returns none if all characters satisfy the predicate. +pub fn skip_while(slice: RopeSlice, pos: usize, fun: F) -> Option where F: Fn(char) -> bool, { - let mut chars = slice.chars_at(*pos); - - #[allow(clippy::while_let_on_iterator)] - while let Some(ch) = chars.next() { - if !fun(ch) { - break; - } - *pos += 1; - } - chars.next().is_some() + let mut chars = slice.chars_at(pos).enumerate(); + chars.find_map(|(i, c)| if !fun(c) { Some(pos + i) } else { None }) } #[inline] -/// Returns true if the final pos matches the predicate. -pub fn skip_over_prev(slice: RopeSlice, pos: &mut usize, fun: F) -> bool +/// Returns first index that doesn't satisfy a given predicate when +/// retreating the character index, saturating if all elements satisfy +/// the condition. +pub fn backwards_skip_while(slice: RopeSlice, pos: usize, fun: F) -> Option where F: Fn(char) -> bool, { - // need to +1 so that prev() includes current char - let mut chars = slice.chars_at(*pos + 1); + let mut chars_starting_from_next = 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)] - while let Some(ch) = chars.prev() { - if !fun(ch) { - break; +/// Possible targets of a word motion +#[derive(Copy, Clone, Debug)] +pub enum WordMotionTarget { + 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> = 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 = None; + let is_boundary = |a: char, b: Option| 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')); diff --git a/helix-core/src/selection.rs b/helix-core/src/selection.rs index 7dafc97a9..e452c2e28 100644 --- a/helix-core/src/selection.rs +++ b/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] diff --git a/helix-core/src/words.rs b/helix-core/src/words.rs deleted file mode 100644 index 7ecdacba3..000000000 --- a/helix-core/src/words.rs +++ /dev/null @@ -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("", ""); -} diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index e5a306877..070342147 100644 --- a/helix-term/src/commands.rs +++ b/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, - Selection, SmallVec, Tendril, Transaction, + register, search, selection, Change, ChangeSet, Position, Range, Rope, RopeSlice, Selection, + 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::path::{Path, PathBuf}; +use std::{ + 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( - text, - range, - Direction::Backward, - count, - false, /* extend */ - ) + movement::move_horizontally(text, range, Direction::Backward, count, Movement::Move) }); 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( - text, - range, - Direction::Forward, - count, - false, /* extend */ - ) + movement::move_horizontally(text, range, Direction::Forward, count, Movement::Move) }); 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( - text, - range, - Direction::Backward, - count, - false, /* extend */ - ) + movement::move_vertically(text, range, Direction::Backward, count, Movement::Move) }); 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( - text, - range, - Direction::Forward, - count, - false, /* extend */ - ) + movement::move_vertically(text, range, Direction::Forward, count, Movement::Move) }); 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| { - movement::move_next_word_start(text, range.head, count).unwrap_or(range) - }); + let selection = doc + .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| { - movement::move_prev_word_start(text, range.head, count).unwrap_or(range) - }); + let selection = doc + .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( - text, - range, - Direction::Backward, - count, - true, /* extend */ - ) + movement::move_horizontally(text, range, Direction::Backward, count, Movement::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( - text, - range, - Direction::Forward, - count, - true, /* extend */ - ) + movement::move_horizontally(text, range, Direction::Forward, count, Movement::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( - text, - range, - Direction::Backward, - count, - true, /* extend */ - ) + movement::move_vertically(text, range, Direction::Backward, count, Movement::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( - text, - range, - Direction::Forward, - count, - true, /* extend */ - ) + movement::move_vertically(text, range, Direction::Forward, count, Movement::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 = - Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| { - ( - words::nth_prev_word_boundary(text, range.head, count), - range.head, - None, - ) - }); - doc.apply(&transaction, view.id); + let selection = doc + .selection(view.id) + .transform(|range| movement::move_prev_word_start(text, range, count)); + doc.set_selection(view.id, selection); + delete_selection(cx) } } @@ -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()));