refactor: leave pos_at_coords unchanged and introduce separate pos_at…

…_visual_coords

helix-core/src/lib.rs

@@ -63,7 +63,9 @@ pub type Tendril = SmartString<smartstring::LazyCompact>;
 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

@@ -10,7 +10,7 @@ use crate::{
         prev_grapheme_boundary,
     },
     line_ending::rope_is_line_ending,
-    pos_at_coords,
+    pos_at_visual_coords,
     syntax::LanguageConfiguration,
     textobject::TextObject,
     visual_coords_at_pos, Position, Range, RopeSlice,
@@ -68,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), tab_width, 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 {
@@ -443,7 +443,7 @@ pub fn goto_treesitter_object(
 mod test {
     use ropey::Rope;
 
-    use crate::coords_at_pos;
+    use crate::{coords_at_pos, pos_at_coords};
 
     use super::*;
 
@@ -465,7 +465,7 @@ mod test {
     fn test_vertical_move() {
         let text = Rope::from("abcd\nefg\nwrs");
         let slice = text.slice(..);
-        let pos = pos_at_coords(slice, (0, 4).into(), 4, true);
+        let pos = pos_at_coords(slice, (0, 4).into(), true);
 
         let range = Range::new(pos, pos);
         assert_eq!(
@@ -481,7 +481,7 @@ mod test {
     fn horizontal_moves_through_single_line_text() {
         let text = Rope::from(SINGLE_LINE_SAMPLE);
         let slice = text.slice(..);
-        let position = pos_at_coords(slice, (0, 0).into(), 4, true);
+        let position = pos_at_coords(slice, (0, 0).into(), true);
 
         let mut range = Range::point(position);
 
@@ -504,7 +504,7 @@ mod test {
     fn horizontal_moves_through_multiline_text() {
         let text = Rope::from(MULTILINE_SAMPLE);
         let slice = text.slice(..);
-        let position = pos_at_coords(slice, (0, 0).into(), 4, true);
+        let position = pos_at_coords(slice, (0, 0).into(), true);
 
         let mut range = Range::point(position);
 
@@ -531,7 +531,7 @@ mod 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(), 4, true);
+        let position = pos_at_coords(slice, (0, 0).into(), true);
 
         let mut range = Range::point(position);
         let original_anchor = range.anchor;
@@ -552,7 +552,7 @@ mod test {
     fn vertical_moves_in_single_column() {
         let text = Rope::from(MULTILINE_SAMPLE);
         let slice = text.slice(..);
-        let position = pos_at_coords(slice, (0, 0).into(), 4, true);
+        let position = pos_at_coords(slice, (0, 0).into(), true);
         let mut range = Range::point(position);
         let moves_and_expected_coordinates = [
             ((Direction::Forward, 1usize), (1, 0)),
@@ -577,7 +577,7 @@ mod 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(), 4, true);
+        let position = pos_at_coords(slice, (0, 0).into(), true);
         let mut range = Range::point(position);
 
         enum Axis {
@@ -613,7 +613,7 @@ mod test {
     fn multibyte_character_wide_column_jumps() {
         let text = Rope::from(MULTIBYTE_CHARACTER_SAMPLE);
         let slice = text.slice(..);
-        let position = pos_at_coords(slice, (0, 0).into(), 4, true);
+        let position = pos_at_coords(slice, (0, 0).into(), true);
         let mut range = Range::point(position);
 
         // FIXME: The behaviour captured in this test diverges from both Kakoune and Vim. These

helix-core/src/position.rs

@@ -109,12 +109,7 @@ 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.
-pub fn pos_at_coords(
-    text: RopeSlice,
-    coords: Position,
-    tab_width: usize,
-    limit_before_line_ending: bool,
-) -> usize {
+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 {
         row = row.min(text.len_lines() - 1);
@@ -126,11 +121,36 @@ pub fn pos_at_coords(
         text.line_to_char((row + 1).min(text.len_lines()))
     };
 
+    let mut col_char_offset = 0;
+    for (i, g) in RopeGraphemes::new(text.slice(line_start..line_end)).enumerate() {
+        if i == col {
+            break;
+        }
+        col_char_offset += g.chars().count();
+    }
+
+    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
+            tab_width - ((col - cols_remaining) % tab_width)
         } else {
             let grapheme = Cow::from(grapheme);
             grapheme_width(&grapheme)
@@ -257,68 +277,130 @@ mod test {
     fn test_pos_at_coords() {
         let text = Rope::from("ḧëḷḷö\nẅöṛḷḋ");
         let slice = text.slice(..);
-        assert_eq!(pos_at_coords(slice, (0, 0).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 5).into(), 4, false), 5); // position on \n
-        assert_eq!(pos_at_coords(slice, (0, 6).into(), 4, false), 6); // position after \n
-        assert_eq!(pos_at_coords(slice, (0, 6).into(), 4, true), 5); // position after \n
-        assert_eq!(pos_at_coords(slice, (1, 0).into(), 4, false), 6); // position on w
-        assert_eq!(pos_at_coords(slice, (1, 1).into(), 4, false), 7); // position on o
-        assert_eq!(pos_at_coords(slice, (1, 4).into(), 4, false), 10); // position on d
+        assert_eq!(pos_at_coords(slice, (0, 0).into(), false), 0);
+        assert_eq!(pos_at_coords(slice, (0, 5).into(), false), 5); // position on \n
+        assert_eq!(pos_at_coords(slice, (0, 6).into(), false), 6); // position after \n
+        assert_eq!(pos_at_coords(slice, (0, 6).into(), true), 5); // position after \n
+        assert_eq!(pos_at_coords(slice, (1, 0).into(), false), 6); // position on w
+        assert_eq!(pos_at_coords(slice, (1, 1).into(), false), 7); // position on o
+        assert_eq!(pos_at_coords(slice, (1, 4).into(), false), 10); // position on d
+
+        // Test with wide characters.
+        // TODO: account for character width.
+        let text = Rope::from("今日はいい\n");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_coords(slice, (0, 0).into(), false), 0);
+        assert_eq!(pos_at_coords(slice, (0, 1).into(), false), 1);
+        assert_eq!(pos_at_coords(slice, (0, 2).into(), false), 2);
+        assert_eq!(pos_at_coords(slice, (0, 3).into(), false), 3);
+        assert_eq!(pos_at_coords(slice, (0, 4).into(), false), 4);
+        assert_eq!(pos_at_coords(slice, (0, 5).into(), false), 5);
+        assert_eq!(pos_at_coords(slice, (0, 6).into(), false), 6);
+        assert_eq!(pos_at_coords(slice, (0, 6).into(), true), 5);
+        assert_eq!(pos_at_coords(slice, (1, 0).into(), false), 6);
+
+        // Test with grapheme clusters.
+        let text = Rope::from("a̐éö̲\r\n");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_coords(slice, (0, 0).into(), false), 0);
+        assert_eq!(pos_at_coords(slice, (0, 1).into(), false), 2);
+        assert_eq!(pos_at_coords(slice, (0, 2).into(), false), 4);
+        assert_eq!(pos_at_coords(slice, (0, 3).into(), false), 7); // \r\n is one char here
+        assert_eq!(pos_at_coords(slice, (0, 4).into(), false), 9);
+        assert_eq!(pos_at_coords(slice, (0, 4).into(), true), 7);
+        assert_eq!(pos_at_coords(slice, (1, 0).into(), false), 9);
+
+        // Test with wide-character grapheme clusters.
+        // TODO: account for character width.
+        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_coords(slice, (0, 0).into(), false), 0);
+        assert_eq!(pos_at_coords(slice, (0, 1).into(), false), 2);
+        assert_eq!(pos_at_coords(slice, (0, 2).into(), false), 3);
+        assert_eq!(pos_at_coords(slice, (0, 3).into(), false), 5);
+        assert_eq!(pos_at_coords(slice, (0, 3).into(), true), 5);
+
+        // Test with tabs.
+        // Todo: account for tab stops.
+        let text = Rope::from("\tHello\n");
+        let slice = text.slice(..);
+        assert_eq!(pos_at_coords(slice, (0, 0).into(), false), 0);
+        assert_eq!(pos_at_coords(slice, (0, 1).into(), false), 1);
+        assert_eq!(pos_at_coords(slice, (0, 2).into(), false), 2);
+
+        // Test out of bounds.
+        let text = Rope::new();
+        let slice = text.slice(..);
+        assert_eq!(pos_at_coords(slice, (10, 0).into(), true), 0);
+        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_coords(slice, (0, 0).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 1).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 2).into(), 4, false), 1);
-        assert_eq!(pos_at_coords(slice, (0, 3).into(), 4, false), 1);
-        assert_eq!(pos_at_coords(slice, (0, 4).into(), 4, false), 2);
-        assert_eq!(pos_at_coords(slice, (0, 5).into(), 4, false), 2);
-        assert_eq!(pos_at_coords(slice, (0, 6).into(), 4, false), 3);
-        assert_eq!(pos_at_coords(slice, (0, 7).into(), 4, false), 3);
-        assert_eq!(pos_at_coords(slice, (0, 8).into(), 4, false), 4);
-        assert_eq!(pos_at_coords(slice, (0, 9).into(), 4, false), 4);
-        assert_eq!(pos_at_coords(slice, (0, 10).into(), 4, false), 5);
-        assert_eq!(pos_at_coords(slice, (0, 10).into(), 4, true), 5);
-        assert_eq!(pos_at_coords(slice, (1, 0).into(), 4, false), 6);
+        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_coords(slice, (0, 0).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 1).into(), 4, false), 2);
-        assert_eq!(pos_at_coords(slice, (0, 2).into(), 4, false), 4);
-        assert_eq!(pos_at_coords(slice, (0, 3).into(), 4, false), 7); // \r\n is one char here
-        assert_eq!(pos_at_coords(slice, (0, 4).into(), 4, false), 9);
-        assert_eq!(pos_at_coords(slice, (0, 4).into(), 4, true), 7);
-        assert_eq!(pos_at_coords(slice, (1, 0).into(), 4, false), 9);
+        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_coords(slice, (0, 0).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 1).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 2).into(), 4, false), 2);
-        assert_eq!(pos_at_coords(slice, (0, 3).into(), 4, false), 3);
-        assert_eq!(pos_at_coords(slice, (0, 3).into(), 4, true), 3);
+        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_coords(slice, (0, 0).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 1).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 2).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 3).into(), 4, false), 0);
-        assert_eq!(pos_at_coords(slice, (0, 4).into(), 4, false), 1);
-        assert_eq!(pos_at_coords(slice, (0, 5).into(), 4, false), 2);
+        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_coords(slice, (10, 0).into(), 4, true), 0);
-        assert_eq!(pos_at_coords(slice, (0, 10).into(), 4, true), 0);
-        assert_eq!(pos_at_coords(slice, (10, 10).into(), 4, true), 0);
+        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/application.rs

@@ -158,12 +158,7 @@ impl Application {
                         // with Action::Load all documents have the same view
                         let view_id = editor.tree.focus;
                         let doc = editor.document_mut(doc_id).unwrap();
-                        let pos = Selection::point(pos_at_coords(
-                            doc.text().slice(..),
-                            pos,
-                            doc.tab_width(),
-                            true,
-                        ));
+                        let pos = Selection::point(pos_at_coords(doc.text().slice(..), pos, true));
                         doc.set_selection(view_id, pos);
                     }
                 }

helix-term/src/commands.rs

@@ -1351,7 +1351,7 @@ pub fn scroll(cx: &mut Context, offset: usize, direction: Direction) {
 
     // If cursor needs moving, replace primary selection
     if line != cursor.row {
-        let head = pos_at_coords(text, Position::new(line, cursor.col), doc.tab_width(), true); // this func will properly truncate to line end
+        let head = pos_at_coords(text, Position::new(line, cursor.col), true); // this func will properly truncate to line end
 
         let anchor = if doc.mode == Mode::Select {
             range.anchor
@@ -1442,18 +1442,8 @@ fn copy_selection_on_line(cx: &mut Context, direction: Direction) {
                 break;
             }
 
-            let anchor = pos_at_coords(
-                text,
-                Position::new(anchor_row, anchor_pos.col),
-                doc.tab_width(),
-                true,
-            );
-            let head = pos_at_coords(
-                text,
-                Position::new(head_row, head_pos.col),
-                doc.tab_width(),
-                true,
-            );
+            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);
 
             // skip lines that are too short
             if coords_at_pos(text, anchor).col == anchor_pos.col

helix-term/src/commands/typed.rs

@@ -48,12 +48,7 @@ fn open(
         let (path, pos) = args::parse_file(arg);
         let _ = cx.editor.open(path, Action::Replace)?;
         let (view, doc) = current!(cx.editor);
-        let pos = Selection::point(pos_at_coords(
-            doc.text().slice(..),
-            pos,
-            doc.tab_width(),
-            true,
-        ));
+        let pos = Selection::point(pos_at_coords(doc.text().slice(..), pos, true));
         doc.set_selection(view.id, pos);
         // does not affect opening a buffer without pos
         align_view(doc, view, Align::Center);