From c848ed7abc7605c1bb1c6c98dfac23cadcb9e439 Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Tue, 20 Jul 2021 18:15:34 -0700 Subject: [PATCH] Fixes for misc bugs with view movement. --- helix-core/src/movement.rs | 39 +++++----- helix-core/src/position.rs | 141 +++++++++++++++++++++++++++--------- helix-core/src/selection.rs | 2 +- helix-term/src/commands.rs | 2 +- helix-view/src/view.rs | 4 +- 5 files changed, 132 insertions(+), 56 deletions(-) diff --git a/helix-core/src/movement.rs b/helix-core/src/movement.rs index 21e97ae8..01bec47a 100644 --- a/helix-core/src/movement.rs +++ b/helix-core/src/movement.rs @@ -7,9 +7,8 @@ use crate::{ coords_at_pos, graphemes::{ next_grapheme_boundary, nth_next_grapheme_boundary, nth_prev_grapheme_boundary, - prev_grapheme_boundary, RopeGraphemes, + prev_grapheme_boundary, }, - line_ending::line_without_line_ending, pos_at_coords, Position, Range, RopeSlice, }; @@ -85,10 +84,8 @@ pub fn move_vertically( } 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); - - pos_at_coords(slice, Position::new(new_row, new_col)) + let new_col = col.max(horiz as usize); + pos_at_coords(slice, Position::new(new_row, new_col), true) }; // Compute the new range according to the type of movement. @@ -365,7 +362,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()); + let pos = pos_at_coords(slice, (0, 4).into(), true); let range = Range::new(pos, pos); assert_eq!( @@ -381,7 +378,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()); + let position = pos_at_coords(slice, (0, 0).into(), true); let mut range = Range::point(position); @@ -404,7 +401,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()); + let position = pos_at_coords(slice, (0, 0).into(), true); let mut range = Range::point(position); @@ -431,7 +428,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()); + let position = pos_at_coords(slice, (0, 0).into(), true); let mut range = Range::point(position); let original_anchor = range.anchor; @@ -452,7 +449,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()); + let position = pos_at_coords(slice, (0, 0).into(), true); let mut range = Range::point(position); let moves_and_expected_coordinates = IntoIter::new([ ((Direction::Forward, 1usize), (1, 0)), @@ -477,7 +474,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()); + let position = pos_at_coords(slice, (0, 0).into(), true); let mut range = Range::point(position); enum Axis { @@ -510,10 +507,10 @@ mod test { } #[test] - fn multibyte_character_column_jumps() { + 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()); + 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 @@ -524,10 +521,16 @@ mod test { 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)), + // Places cursor at the fourth kana (each of which are double-wide, + // so the visual column is 8). + ((Axis::H, Direction::Forward, 4), (0, 8)), + // Descent places cursor at the 8th character. + ((Axis::V, Direction::Forward, 1usize), (1, 8)), + // Moving back a single-width character. + ((Axis::H, Direction::Backward, 1usize), (1, 7)), + // Jumping back up into the middle of a double-width character shifts + // the column to the start of that character. + ((Axis::V, Direction::Backward, 1usize), (0, 6)), ]); for ((axis, direction, amount), coordinates) in moves_and_expected_coordinates { diff --git a/helix-core/src/position.rs b/helix-core/src/position.rs index c4e8c9d6..15627687 100644 --- a/helix-core/src/position.rs +++ b/helix-core/src/position.rs @@ -1,6 +1,9 @@ +use std::borrow::Cow; + use crate::{ chars::char_is_line_ending, - graphemes::{nth_next_grapheme_boundary, RopeGraphemes}, + graphemes::{ensure_grapheme_boundary_prev, grapheme_width, RopeGraphemes}, + line_ending::line_end_char_index, RopeSlice, }; @@ -52,23 +55,58 @@ impl From for tree_sitter::Point { } } /// Convert a character index to (line, column) coordinates. +/// +/// TODO: this should be split into two methods: one for visual +/// row/column, and one for "objective" row/column (possibly with +/// the column specified in `char`s). The former would be used +/// for cursor movement, and the latter would be used for e.g. the +/// row:column display in the status line. 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(); + let pos = ensure_grapheme_boundary_prev(text, pos); + let col = RopeGraphemes::new(text.slice(line_start..pos)) + .map(|g| { + let g: Cow = g.into(); + grapheme_width(&g) + }) + .sum(); + Position::new(line, col) } /// 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. +/// +/// `is_1_width` specifies whether the position should be treated +/// as a block cursor or not. This effects how line-ends are handled. +/// `false` corresponds to properly round-tripping with `coords_at_pos()`, +/// whereas `true` will ensure that block cursors don't jump off the +/// end of the line. +pub fn pos_at_coords(text: RopeSlice, coords: Position, is_1_width: bool) -> usize { let Position { row, col } = coords; let line_start = text.line_to_char(row); - // line_start + col - nth_next_grapheme_boundary(text, line_start, col) + let line_end = if is_1_width { + line_end_char_index(&text, row) + } else { + text.line_to_char((row + 1).min(text.len_lines())) + }; + + let mut prev_col = 0; + let mut col_char_offset = 0; + for g in RopeGraphemes::new(text.slice(line_start..line_end)) { + let g: Cow = g.into(); + let next_col = prev_col + grapheme_width(&g); + + if next_col > col { + break; + } + + prev_col = next_col; + col_char_offset += g.chars().count(); + } + + line_start + col_char_offset } #[cfg(test)] @@ -84,13 +122,24 @@ mod test { #[test] fn test_coords_at_pos() { - // let text = Rope::from("ḧëḷḷö\nẅöṛḷḋ"); - // let slice = text.slice(..); - // assert_eq!(coords_at_pos(slice, 0), (0, 0).into()); - // assert_eq!(coords_at_pos(slice, 5), (0, 5).into()); // position on \n - // assert_eq!(coords_at_pos(slice, 6), (1, 0).into()); // position on w - // assert_eq!(coords_at_pos(slice, 7), (1, 1).into()); // position on o - // assert_eq!(coords_at_pos(slice, 10), (1, 4).into()); // position on d + let text = Rope::from("ḧëḷḷö\nẅöṛḷḋ"); + let slice = text.slice(..); + assert_eq!(coords_at_pos(slice, 0), (0, 0).into()); + assert_eq!(coords_at_pos(slice, 5), (0, 5).into()); // position on \n + assert_eq!(coords_at_pos(slice, 6), (1, 0).into()); // position on w + assert_eq!(coords_at_pos(slice, 7), (1, 1).into()); // position on o + assert_eq!(coords_at_pos(slice, 10), (1, 4).into()); // position on d + + // Test with wide characters. + let text = Rope::from("今日はいい\n"); + let slice = text.slice(..); + assert_eq!(coords_at_pos(slice, 0), (0, 0).into()); + assert_eq!(coords_at_pos(slice, 1), (0, 2).into()); + assert_eq!(coords_at_pos(slice, 2), (0, 4).into()); + assert_eq!(coords_at_pos(slice, 3), (0, 6).into()); + assert_eq!(coords_at_pos(slice, 4), (0, 8).into()); + assert_eq!(coords_at_pos(slice, 5), (0, 10).into()); + assert_eq!(coords_at_pos(slice, 6), (1, 0).into()); // test with grapheme clusters let text = Rope::from("a̐éö̲\r\n"); @@ -99,40 +148,64 @@ mod test { assert_eq!(coords_at_pos(slice, 2), (0, 1).into()); assert_eq!(coords_at_pos(slice, 4), (0, 2).into()); assert_eq!(coords_at_pos(slice, 7), (0, 3).into()); + assert_eq!(coords_at_pos(slice, 9), (1, 0).into()); - let text = Rope::from("किमपि"); + let text = Rope::from("किमपि\n"); let slice = text.slice(..); assert_eq!(coords_at_pos(slice, 0), (0, 0).into()); - assert_eq!(coords_at_pos(slice, 2), (0, 1).into()); - assert_eq!(coords_at_pos(slice, 3), (0, 2).into()); - assert_eq!(coords_at_pos(slice, 5), (0, 3).into()); + assert_eq!(coords_at_pos(slice, 2), (0, 2).into()); + assert_eq!(coords_at_pos(slice, 3), (0, 3).into()); + assert_eq!(coords_at_pos(slice, 5), (0, 5).into()); + assert_eq!(coords_at_pos(slice, 6), (1, 0).into()); } #[test] fn test_pos_at_coords() { let text = Rope::from("ḧëḷḷö\nẅöṛḷḋ"); let slice = text.slice(..); - assert_eq!(pos_at_coords(slice, (0, 0).into()), 0); - assert_eq!(pos_at_coords(slice, (0, 5).into()), 5); // position on \n - assert_eq!(pos_at_coords(slice, (1, 0).into()), 6); // position on w - assert_eq!(pos_at_coords(slice, (1, 1).into()), 7); // position on o - assert_eq!(pos_at_coords(slice, (1, 4).into()), 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. + 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), 0); + assert_eq!(pos_at_coords(slice, (0, 2).into(), false), 1); + assert_eq!(pos_at_coords(slice, (0, 3).into(), false), 1); + assert_eq!(pos_at_coords(slice, (0, 4).into(), false), 2); + assert_eq!(pos_at_coords(slice, (0, 6).into(), false), 3); + assert_eq!(pos_at_coords(slice, (0, 8).into(), false), 4); + assert_eq!(pos_at_coords(slice, (0, 10).into(), false), 5); + assert_eq!(pos_at_coords(slice, (0, 11).into(), false), 6); + assert_eq!(pos_at_coords(slice, (0, 11).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()), 0); - assert_eq!(pos_at_coords(slice, (0, 1).into()), 2); - assert_eq!(pos_at_coords(slice, (0, 2).into()), 4); - assert_eq!(pos_at_coords(slice, (0, 3).into()), 7); // \r\n is one char here - assert_eq!(pos_at_coords(slice, (0, 4).into()), 9); + 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); + 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()), 0); - assert_eq!(pos_at_coords(slice, (0, 1).into()), 2); - assert_eq!(pos_at_coords(slice, (0, 2).into()), 3); - assert_eq!(pos_at_coords(slice, (0, 3).into()), 5); // eol + assert_eq!(pos_at_coords(slice, (0, 0).into(), false), 0); + assert_eq!(pos_at_coords(slice, (0, 1).into(), false), 0); + 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), 3); + assert_eq!(pos_at_coords(slice, (0, 5).into(), false), 5); // eol } } diff --git a/helix-core/src/selection.rs b/helix-core/src/selection.rs index 074b6199..beade27a 100644 --- a/helix-core/src/selection.rs +++ b/helix-core/src/selection.rs @@ -286,7 +286,7 @@ impl Selection { // For 1-width cursor semantics. if range.anchor < range.head { - prev_grapheme_boundary(text, range.head) + prev_grapheme_boundary(text, range.head).max(range.anchor) } else { range.head } diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 72462741..0ea78f6b 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -917,7 +917,7 @@ fn scroll(cx: &mut Context, offset: usize, direction: Direction) { .min(last_line.saturating_sub(scrolloff)); let text = doc.text().slice(..); - let pos = pos_at_coords(text, Position::new(line, cursor.col)); // this func will properly truncate to line end + let pos = pos_at_coords(text, Position::new(line, cursor.col), true); // this func will properly truncate to line end // TODO: only manipulate main selection doc.set_selection(view.id, Selection::point(pos)); diff --git a/helix-view/src/view.rs b/helix-view/src/view.rs index 67585ed3..66214691 100644 --- a/helix-view/src/view.rs +++ b/helix-view/src/view.rs @@ -97,9 +97,9 @@ impl View { const OFFSET: usize = 7; // 1 diagnostic + 5 linenr + 1 gutter let last_col = self.first_col + (self.area.width as usize - OFFSET); - if line > last_line.saturating_sub(scrolloff) { + if line > last_line.saturating_sub(scrolloff + 1) { // scroll down - self.first_line += line - (last_line.saturating_sub(scrolloff)); + self.first_line += line - (last_line.saturating_sub(scrolloff + 1)); } else if line < self.first_line + scrolloff { // scroll up self.first_line = line.saturating_sub(scrolloff);