feat: make `move_vertically` aware of tabs and wide characters (#2620)

* feat: make `move_vertically` aware of tabs and wide characters

* refactor: replace unnecessary checked_sub with comparison

* refactor: leave pos_at_coords unchanged and introduce separate pos_at_visual_coords

* style: include comment to explain `pos_at_visual_coords` breaking condition

* refactor: use `pos_at_visual_coords` in `text_pos_at_screen_coords`

* feat: make `copy_selection_on_line` aware of wide characters

helix-core/src/lib.rs

@@ -63,7 +63,9 @@ pub fn find_root(root: Option<&str>, root_markers: &[String]) -> Option<std::pat
 pub use {regex, tree_sitter};
 
 pub use graphemes::RopeGraphemes;
-pub use position::{coords_at_pos, pos_at_coords, visual_coords_at_pos, Position};
+pub use position::{
+    coords_at_pos, pos_at_coords, pos_at_visual_coords, visual_coords_at_pos, Position,
+};
 pub use selection::{Range, Selection};
 pub use smallvec::{smallvec, SmallVec};
 pub use syntax::Syntax;

helix-core/src/movement.rs

@@ -5,16 +5,15 @@
 
 use crate::{
     chars::{categorize_char, char_is_line_ending, CharCategory},
-    coords_at_pos,
     graphemes::{
         next_grapheme_boundary, nth_next_grapheme_boundary, nth_prev_grapheme_boundary,
         prev_grapheme_boundary,
     },
     line_ending::rope_is_line_ending,
-    pos_at_coords,
+    pos_at_visual_coords,
     syntax::LanguageConfiguration,
     textobject::TextObject,
-    Position, Range, RopeSlice,
+    visual_coords_at_pos, Position, Range, RopeSlice,
 };
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
@@ -35,6 +34,7 @@ pub fn move_horizontally(
     dir: Direction,
     count: usize,
     behaviour: Movement,
+    _: usize,
 ) -> Range {
     let pos = range.cursor(slice);
 
@@ -54,15 +54,12 @@ pub fn move_vertically(
     dir: Direction,
     count: usize,
     behaviour: Movement,
+    tab_width: usize,
 ) -> Range {
     let pos = range.cursor(slice);
 
     // Compute the current position's 2d coordinates.
-    // TODO: switch this to use `visual_coords_at_pos` rather than
-    // `coords_at_pos` as this will cause a jerky movement when the visual
-    // position does not match, like moving from a line with tabs/CJK to
-    // a line without
-    let Position { row, col } = coords_at_pos(slice, pos);
+    let Position { row, col } = visual_coords_at_pos(slice, pos, tab_width);
     let horiz = range.horiz.unwrap_or(col as u32);
 
     // Compute the new position.
@@ -71,7 +68,7 @@ pub fn move_vertically(
         Direction::Backward => row.saturating_sub(count),
     };
     let new_col = col.max(horiz as usize);
-    let new_pos = pos_at_coords(slice, Position::new(new_row, new_col), true);
+    let new_pos = pos_at_visual_coords(slice, Position::new(new_row, new_col), tab_width);
 
     // Special-case to avoid moving to the end of the last non-empty line.
     if behaviour == Movement::Extend && slice.line(new_row).len_chars() == 0 {
@@ -446,6 +443,8 @@ pub fn goto_treesitter_object(
 mod test {
     use ropey::Rope;
 
+    use crate::{coords_at_pos, pos_at_coords};
+
     use super::*;
 
     const SINGLE_LINE_SAMPLE: &str = "This is a simple alphabetic line";
@@ -472,7 +471,7 @@ fn test_vertical_move() {
         assert_eq!(
             coords_at_pos(
                 slice,
-                move_vertically(slice, range, Direction::Forward, 1, Movement::Move).head
+                move_vertically(slice, range, Direction::Forward, 1, Movement::Move, 4).head
             ),
             (1, 3).into()
         );
@@ -496,7 +495,7 @@ fn horizontal_moves_through_single_line_text() {
         ];
 
         for ((direction, amount), coordinates) in moves_and_expected_coordinates {
-            range = move_horizontally(slice, range, direction, amount, Movement::Move);
+            range = move_horizontally(slice, range, direction, amount, Movement::Move, 0);
             assert_eq!(coords_at_pos(slice, range.head), coordinates.into())
         }
     }
@@ -522,7 +521,7 @@ fn horizontal_moves_through_multiline_text() {
         ];
 
         for ((direction, amount), coordinates) in moves_and_expected_coordinates {
-            range = move_horizontally(slice, range, direction, amount, Movement::Move);
+            range = move_horizontally(slice, range, direction, amount, Movement::Move, 0);
             assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
             assert_eq!(range.head, range.anchor);
         }
@@ -544,7 +543,7 @@ fn selection_extending_moves_in_single_line_text() {
         ];
 
         for (direction, amount) in moves {
-            range = move_horizontally(slice, range, direction, amount, Movement::Extend);
+            range = move_horizontally(slice, range, direction, amount, Movement::Extend, 0);
             assert_eq!(range.anchor, original_anchor);
         }
     }
@@ -568,7 +567,7 @@ fn vertical_moves_in_single_column() {
         ];
 
         for ((direction, amount), coordinates) in moves_and_expected_coordinates {
-            range = move_vertically(slice, range, direction, amount, Movement::Move);
+            range = move_vertically(slice, range, direction, amount, Movement::Move, 4);
             assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
             assert_eq!(range.head, range.anchor);
         }
@@ -602,8 +601,8 @@ enum Axis {
 
         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),
+                Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move, 0),
+                Axis::V => move_vertically(slice, range, direction, amount, Movement::Move, 4),
             };
             assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
             assert_eq!(range.head, range.anchor);
@@ -627,18 +626,18 @@ enum Axis {
         let moves_and_expected_coordinates = [
             // Places cursor at the fourth kana.
             ((Axis::H, Direction::Forward, 4), (0, 4)),
-            // Descent places cursor at the 4th character.
-            ((Axis::V, Direction::Forward, 1usize), (1, 4)),
-            // Moving back 1 character.
-            ((Axis::H, Direction::Backward, 1usize), (1, 3)),
+            // Descent places cursor at the 8th character.
+            ((Axis::V, Direction::Forward, 1usize), (1, 8)),
+            // Moving back 2 characters.
+            ((Axis::H, Direction::Backward, 2usize), (1, 6)),
             // Jumping back up 1 line.
             ((Axis::V, Direction::Backward, 1usize), (0, 3)),
         ];
 
         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),
+                Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move, 0),
+                Axis::V => move_vertically(slice, range, direction, amount, Movement::Move, 4),
             };
             assert_eq!(coords_at_pos(slice, range.head), coordinates.into());
             assert_eq!(range.head, range.anchor);

helix-core/src/position.rs

@@ -109,9 +109,6 @@ pub fn visual_coords_at_pos(text: RopeSlice, pos: usize, tab_width: usize) -> Po
 /// with left-side block-cursor positions, as this prevents the the block cursor
 /// from jumping to the next line.  Otherwise you typically want it to be `false`,
 /// such as when dealing with raw anchor/head positions.
-///
-/// TODO: this should be changed to work in terms of visual row/column, not
-/// graphemes.
 pub fn pos_at_coords(text: RopeSlice, coords: Position, limit_before_line_ending: bool) -> usize {
     let Position { mut row, col } = coords;
     if limit_before_line_ending {
@@ -135,6 +132,43 @@ pub fn pos_at_coords(text: RopeSlice, coords: Position, limit_before_line_ending
     line_start + col_char_offset
 }
 
+/// Convert visual (line, column) coordinates to a character index.
+///
+/// If the `line` coordinate is beyond the end of the file, the EOF
+/// position will be returned.
+///
+/// If the `column` coordinate is past the end of the given line, the
+/// line-end position (in this case, just before the line ending
+/// character) will be returned.
+pub fn pos_at_visual_coords(text: RopeSlice, coords: Position, tab_width: usize) -> usize {
+    let Position { mut row, col } = coords;
+    row = row.min(text.len_lines() - 1);
+    let line_start = text.line_to_char(row);
+    let line_end = line_end_char_index(&text, row);
+
+    let mut col_char_offset = 0;
+    let mut cols_remaining = col;
+    for grapheme in RopeGraphemes::new(text.slice(line_start..line_end)) {
+        let grapheme_width = if grapheme == "\t" {
+            tab_width - ((col - cols_remaining) % tab_width)
+        } else {
+            let grapheme = Cow::from(grapheme);
+            grapheme_width(&grapheme)
+        };
+
+        // If pos is in the middle of a wider grapheme (tab for example)
+        // return the starting offset.
+        if grapheme_width > cols_remaining {
+            break;
+        }
+
+        cols_remaining -= grapheme_width;
+        col_char_offset += grapheme.chars().count();
+    }
+
+    line_start + col_char_offset
+}
+
 #[cfg(test)]
 mod test {
     use super::*;
@@ -305,4 +339,70 @@ fn test_pos_at_coords() {
         assert_eq!(pos_at_coords(slice, (0, 10).into(), true), 0);
         assert_eq!(pos_at_coords(slice, (10, 10).into(), true), 0);
     }
+
+    #[test]
+    fn test_pos_at_visual_coords() {
+        let text = Rope::from("ḧëḷḷö\nẅöṛḷḋ");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 5).into(), 4), 5); // position on \n
+        assert_eq!(pos_at_visual_coords(slice, (0, 6).into(), 4), 5); // position after \n
+        assert_eq!(pos_at_visual_coords(slice, (1, 0).into(), 4), 6); // position on w
+        assert_eq!(pos_at_visual_coords(slice, (1, 1).into(), 4), 7); // position on o
+        assert_eq!(pos_at_visual_coords(slice, (1, 4).into(), 4), 10); // position on d
+
+        // Test with wide characters.
+        let text = Rope::from("今日はいい\n");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 1);
+        assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 1);
+        assert_eq!(pos_at_visual_coords(slice, (0, 4).into(), 4), 2);
+        assert_eq!(pos_at_visual_coords(slice, (0, 5).into(), 4), 2);
+        assert_eq!(pos_at_visual_coords(slice, (0, 6).into(), 4), 3);
+        assert_eq!(pos_at_visual_coords(slice, (0, 7).into(), 4), 3);
+        assert_eq!(pos_at_visual_coords(slice, (0, 8).into(), 4), 4);
+        assert_eq!(pos_at_visual_coords(slice, (0, 9).into(), 4), 4);
+        // assert_eq!(pos_at_visual_coords(slice, (0, 10).into(), 4, false), 5);
+        // assert_eq!(pos_at_visual_coords(slice, (0, 10).into(), 4, true), 5);
+        assert_eq!(pos_at_visual_coords(slice, (1, 0).into(), 4), 6);
+
+        // Test with grapheme clusters.
+        let text = Rope::from("a̐éö̲\r\n");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 2);
+        assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 4);
+        assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 7); // \r\n is one char here
+        assert_eq!(pos_at_visual_coords(slice, (0, 4).into(), 4), 7);
+        assert_eq!(pos_at_visual_coords(slice, (1, 0).into(), 4), 9);
+
+        // Test with wide-character grapheme clusters.
+        let text = Rope::from("किमपि");
+        // 2 - 1 - 2 codepoints
+        // TODO: delete handling as per https://news.ycombinator.com/item?id=20058454
+        let slice = text.slice(..);
+        assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 2);
+        assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 3);
+
+        // Test with tabs.
+        let text = Rope::from("\tHello\n");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 4).into(), 4), 1);
+        assert_eq!(pos_at_visual_coords(slice, (0, 5).into(), 4), 2);
+
+        // Test out of bounds.
+        let text = Rope::new();
+        let slice = text.slice(..);
+        assert_eq!(pos_at_visual_coords(slice, (10, 0).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (0, 10).into(), 4), 0);
+        assert_eq!(pos_at_visual_coords(slice, (10, 10).into(), 4), 0);
+    }
 }

helix-term/src/commands.rs

@@ -16,14 +16,14 @@
     line_ending::{get_line_ending_of_str, line_end_char_index, str_is_line_ending},
     match_brackets,
     movement::{self, Direction},
-    object, pos_at_coords,
+    object, pos_at_coords, pos_at_visual_coords,
     regex::{self, Regex, RegexBuilder},
     search::{self, CharMatcher},
     selection, shellwords, surround, textobject,
     tree_sitter::Node,
     unicode::width::UnicodeWidthChar,
-    LineEnding, Position, Range, Rope, RopeGraphemes, RopeSlice, Selection, SmallVec, Tendril,
-    Transaction,
+    visual_coords_at_pos, LineEnding, Position, Range, Rope, RopeGraphemes, RopeSlice, Selection,
+    SmallVec, Tendril, Transaction,
 };
 use helix_view::{
     clipboard::ClipboardType,
@@ -511,7 +511,7 @@ fn no_op(_cx: &mut Context) {}
 
 fn move_impl<F>(cx: &mut Context, move_fn: F, dir: Direction, behaviour: Movement)
 where
-    F: Fn(RopeSlice, Range, Direction, usize, Movement) -> Range,
+    F: Fn(RopeSlice, Range, Direction, usize, Movement, usize) -> Range,
 {
     let count = cx.count();
     let (view, doc) = current!(cx.editor);
@@ -520,7 +520,7 @@ fn move_impl<F>(cx: &mut Context, move_fn: F, dir: Direction, behaviour: Movemen
     let selection = doc
         .selection(view.id)
         .clone()
-        .transform(|range| move_fn(text, range, dir, count, behaviour));
+        .transform(|range| move_fn(text, range, dir, count, behaviour, doc.tab_width()));
     doc.set_selection(view.id, selection);
 }
 
@@ -1412,9 +1412,10 @@ fn copy_selection_on_line(cx: &mut Context, direction: Direction) {
             range.head
         };
 
-        // TODO: this should use visual offsets / pos_at_screen_coords
-        let head_pos = coords_at_pos(text, head);
-        let anchor_pos = coords_at_pos(text, range.anchor);
+        let tab_width = doc.tab_width();
+
+        let head_pos = visual_coords_at_pos(text, head, tab_width);
+        let anchor_pos = visual_coords_at_pos(text, range.anchor, tab_width);
 
         let height = std::cmp::max(head_pos.row, anchor_pos.row)
             - std::cmp::min(head_pos.row, anchor_pos.row)
@@ -1444,12 +1445,13 @@ fn copy_selection_on_line(cx: &mut Context, direction: Direction) {
                 break;
             }
 
-            let anchor = pos_at_coords(text, Position::new(anchor_row, anchor_pos.col), true);
-            let head = pos_at_coords(text, Position::new(head_row, head_pos.col), true);
+            let anchor =
+                pos_at_visual_coords(text, Position::new(anchor_row, anchor_pos.col), tab_width);
+            let head = pos_at_visual_coords(text, Position::new(head_row, head_pos.col), tab_width);
 
             // skip lines that are too short
-            if coords_at_pos(text, anchor).col == anchor_pos.col
-                && coords_at_pos(text, head).col == head_pos.col
+            if visual_coords_at_pos(text, anchor, tab_width).col == anchor_pos.col
+                && visual_coords_at_pos(text, head, tab_width).col == head_pos.col
             {
                 if is_primary {
                     primary_index = ranges.len();

helix-view/src/view.rs

@@ -1,15 +1,9 @@
-use std::borrow::Cow;
-
 use crate::{
     graphics::Rect,
     gutter::{self, Gutter},
     Document, DocumentId, ViewId,
 };
-use helix_core::{
-    graphemes::{grapheme_width, RopeGraphemes},
-    line_ending::line_end_char_index,
-    visual_coords_at_pos, Position, RopeSlice, Selection,
-};
+use helix_core::{pos_at_visual_coords, visual_coords_at_pos, Position, RopeSlice, Selection};
 
 use std::fmt;
 
@@ -251,44 +245,21 @@ pub fn text_pos_at_screen_coords(
             return None;
         }
 
-        let line_number = (row - inner.y) as usize + self.offset.row;
-
-        if line_number > text.len_lines() - 1 {
+        let text_row = (row - inner.y) as usize + self.offset.row;
+        if text_row > text.len_lines() - 1 {
             return Some(text.len_chars());
         }
 
-        let mut pos = text.line_to_char(line_number);
+        let text_col = (column - inner.x) as usize + self.offset.col;
 
-        let current_line = text.line(line_number);
-
-        let target = (column - inner.x) as usize + self.offset.col;
-        let mut col = 0;
-
-        // TODO: extract this part as pos_at_visual_coords
-        for grapheme in RopeGraphemes::new(current_line) {
-            if col >= target {
-                break;
-            }
-
-            let width = if grapheme == "\t" {
-                tab_width - (col % tab_width)
-            } else {
-                let grapheme = Cow::from(grapheme);
-                grapheme_width(&grapheme)
-            };
-
-            // If pos is in the middle of a wider grapheme (tab for example)
-            // return the starting offset.
-            if col + width > target {
-                break;
-            }
-
-            col += width;
-            // TODO: use byte pos that converts back to char pos?
-            pos += grapheme.chars().count();
-        }
-
-        Some(pos.min(line_end_char_index(&text.slice(..), line_number)))
+        Some(pos_at_visual_coords(
+            *text,
+            Position {
+                row: text_row,
+                col: text_col,
+            },
+            tab_width,
+        ))
     }
 
     /// Translates a screen position to position in the text document.