Make vertical selection movement work properly.

3 files changed, 60 insertions, 24 deletions

diff --git a/helix-core/src/line_ending.rs b/helix-core/src/line_ending.rs
index e3ff6478..18ea5f9f 100644
--- a/helix-core/src/line_ending.rs
+++ b/helix-core/src/line_ending.rs
@@ -159,6 +159,13 @@ pub fn line_end_char_index(slice: &RopeSlice, line: usize) -> usize {
             .unwrap_or(0)
 }
 
+/// Fetches line `line_idx` from the passed rope slice, sans any line ending.
+pub fn line_without_line_ending<'a>(slice: &'a RopeSlice, line_idx: usize) -> RopeSlice<'a> {
+    let start = slice.line_to_char(line_idx);
+    let end = line_end_char_index(slice, line_idx);
+    slice.slice(start..end)
+}
+
 /// Returns the char index of the end of the given RopeSlice, not including
 /// any final line ending.
 pub fn rope_end_without_line_ending(slice: &RopeSlice) -> usize {
diff --git a/helix-core/src/movement.rs b/helix-core/src/movement.rs
index e01786eb..62311ee4 100644
--- a/helix-core/src/movement.rs
+++ b/helix-core/src/movement.rs
@@ -7,9 +7,9 @@ use crate::{
     coords_at_pos,
     graphemes::{
         next_grapheme_boundary, nth_next_grapheme_boundary, nth_prev_grapheme_boundary,
-        prev_grapheme_boundary,
+        prev_grapheme_boundary, RopeGraphemes,
     },
-    line_ending::get_line_ending,
+    line_ending::line_without_line_ending,
     pos_at_coords, Position, Range, RopeSlice,
 };
 
@@ -95,36 +95,61 @@ pub fn move_vertically(
     count: usize,
     behaviour: Movement,
 ) -> Range {
-    let Position { row, col } = coords_at_pos(slice, range.head);
+    // Shift back one grapheme if needed, to account for
+    // the cursor being visually 1-width.
+    let pos = if range.head > range.anchor {
+        prev_grapheme_boundary(slice, range.head)
+    } else {
+        range.head
+    };
 
+    // Compute the current position's 2d coordinates.
+    let Position { row, col } = coords_at_pos(slice, pos);
     let horiz = range.horiz.unwrap_or(col as u32);
 
-    let new_line = match dir {
-        Direction::Backward => row.saturating_sub(count),
-        Direction::Forward => std::cmp::min(
-            row.saturating_add(count),
-            slice.len_lines().saturating_sub(1),
-        ),
-    };
+    // Compute the new position.
+    let new_pos = {
+        let new_row = if dir == Direction::Backward {
+            row.saturating_sub(count)
+        } else {
+            (row + count).min(slice.len_lines().saturating_sub(1))
+        };
+        let max_col = RopeGraphemes::new(line_without_line_ending(&slice, new_row)).count();
+        let new_col = col.max(horiz as usize).min(max_col);
 
-    // Length of the line sans line-ending.
-    let new_line_len = {
-        let line = slice.line(new_line);
-        line.len_chars() - get_line_ending(&line).map(|le| le.len_chars()).unwrap_or(0)
+        pos_at_coords(slice, Position::new(new_row, new_col))
     };
 
-    let new_col = std::cmp::min(horiz as usize, new_line_len);
-
-    let pos = pos_at_coords(slice, Position::new(new_line, new_col));
+    // Compute the new range according to the type of movement.
+    match behaviour {
+        Movement::Move => Range {
+            anchor: new_pos,
+            head: new_pos,
+            horiz: Some(horiz),
+        },
+
+        Movement::Extend => {
+            let new_head = if new_pos >= range.anchor {
+                next_grapheme_boundary(slice, new_pos)
+            } else {
+                new_pos
+            };
 
-    let anchor = match behaviour {
-        Movement::Extend => range.anchor,
-        Movement::Move => pos,
-    };
+            let new_anchor = if range.anchor <= range.head && range.anchor > new_head {
+                next_grapheme_boundary(slice, range.anchor)
+            } else if range.anchor > range.head && range.anchor < new_head {
+                prev_grapheme_boundary(slice, range.anchor)
+            } else {
+                range.anchor
+            };
 
-    let mut range = Range::new(anchor, pos);
-    range.horiz = Some(horiz);
-    range
+            Range {
+                anchor: new_anchor,
+                head: new_head,
+                horiz: Some(horiz),
+            }
+        }
+    }
 }
 
 pub fn move_next_word_start(slice: RopeSlice, range: Range, count: usize) -> Range {
diff --git a/helix-core/src/position.rs b/helix-core/src/position.rs
index 3d114b52..c4e8c9d6 100644
--- a/helix-core/src/position.rs
+++ b/helix-core/src/position.rs
@@ -53,6 +53,8 @@ impl From<Position> for tree_sitter::Point {
 }
 /// Convert a character index to (line, column) coordinates.
 pub fn coords_at_pos(text: RopeSlice, pos: usize) -> Position {
+    // TODO: this isn't correct.  This needs to work in terms of
+    // visual horizontal position, not graphemes.
     let line = text.char_to_line(pos);
     let line_start = text.line_to_char(line);
     let col = RopeGraphemes::new(text.slice(line_start..pos)).count();
@@ -61,6 +63,8 @@ pub fn coords_at_pos(text: RopeSlice, pos: usize) -> Position {
 
 /// Convert (line, column) coordinates to a character index.
 pub fn pos_at_coords(text: RopeSlice, coords: Position) -> usize {
+    // TODO: this isn't correct.  This needs to work in terms of
+    // visual horizontal position, not graphemes.
     let Position { row, col } = coords;
     let line_start = text.line_to_char(row);
     // line_start + col