Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jun 21, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
966a17f
feat: make `move_vertically` aware of tabs and wide characters
mtoohey31 May 31, 2022
ad244a4
refactor: replace unnecessary checked_sub with comparison
mtoohey31 May 31, 2022
33e8fa7
refactor: leave pos_at_coords unchanged and introduce separate pos_at…
mtoohey31 Jun 5, 2022
3c17265
style: include comment to explain `pos_at_visual_coords` breaking con…
mtoohey31 Jun 7, 2022
324a76b
refactor: use `pos_at_visual_coords` in `text_pos_at_screen_coords`
mtoohey31 Jun 7, 2022
274746e
feat: make `copy_selection_on_line` aware of wide characters
mtoohey31 Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 27 additions & 28 deletions helix-core/src/movement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use tree_sitter::{Node, QueryCursor};

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,
Expand All @@ -14,7 +13,7 @@ use crate::{
pos_at_coords,
syntax::LanguageConfiguration,
textobject::TextObject,
Position, Range, RopeSlice,
visual_coords_at_pos, Position, Range, RopeSlice,
};

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand All @@ -35,6 +34,7 @@ pub fn move_horizontally(
dir: Direction,
count: usize,
behaviour: Movement,
_: usize,
) -> Range {
let pos = range.cursor(slice);

Expand All @@ -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.
Expand All @@ -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_coords(slice, Position::new(new_row, new_col), tab_width, true);

// 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 {
Expand Down Expand Up @@ -446,6 +443,8 @@ pub fn goto_treesitter_object(
mod test {
use ropey::Rope;

use crate::coords_at_pos;

use super::*;

const SINGLE_LINE_SAMPLE: &str = "This is a simple alphabetic line";
Expand All @@ -466,13 +465,13 @@ 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(), true);
let pos = pos_at_coords(slice, (0, 4).into(), 4, true);

let range = Range::new(pos, pos);
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()
);
Expand All @@ -482,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(), true);
let position = pos_at_coords(slice, (0, 0).into(), 4, true);

let mut range = Range::point(position);

Expand All @@ -496,7 +495,7 @@ mod test {
];

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())
}
}
Expand All @@ -505,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(), true);
let position = pos_at_coords(slice, (0, 0).into(), 4, true);

let mut range = Range::point(position);

Expand All @@ -522,7 +521,7 @@ mod test {
];

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);
}
Expand All @@ -532,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(), true);
let position = pos_at_coords(slice, (0, 0).into(), 4, true);

let mut range = Range::point(position);
let original_anchor = range.anchor;
Expand All @@ -544,7 +543,7 @@ mod test {
];

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);
}
}
Expand All @@ -553,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(), true);
let position = pos_at_coords(slice, (0, 0).into(), 4, true);
let mut range = Range::point(position);
let moves_and_expected_coordinates = [
((Direction::Forward, 1usize), (1, 0)),
Expand All @@ -568,7 +567,7 @@ mod test {
];

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);
}
Expand All @@ -578,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(), true);
let position = pos_at_coords(slice, (0, 0).into(), 4, true);
let mut range = Range::point(position);

enum Axis {
Expand All @@ -602,8 +601,8 @@ mod test {

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);
Expand All @@ -614,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(), true);
let position = pos_at_coords(slice, (0, 0).into(), 4, true);
let mut range = Range::point(position);

// FIXME: The behaviour captured in this test diverges from both Kakoune and Vim. These
Expand All @@ -627,18 +626,18 @@ mod test {
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);
Expand Down
104 changes: 60 additions & 44 deletions helix-core/src/position.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,12 @@ 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.
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO seems wrong, pos_at_coords is intended for line, col calculation, and we already have pos_at_visual_coords for functions that need to deal with tab width and grapheme widths. The method should be kept unchanged, instead commands should use visual_ versions where necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've refactored things in 33e8fa7 so pos_at_visual_coords is a separate function. I removed the limit_before_line_ending parameter because I figure when we're dealing with visual coords we'll always want the behaviour corresponding to a true value for that parameter, right?

Also, I noticed this comment that mentions pos_at_visual_coords; is it fine if I update the code there to use that function now that it exists? It looks like the code there does the same thing as my implementation (at least, it does now, I found a bug in my code with the handling of tabs that don't start at a multiple of tab_width from looking at it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good catch! It should use that function, similar to how the inverse does: https://github.com/helix-editor/helix/blob/eba82250bb4403fcb2e3ade74ba7301a680bc561/helix-view/src/view.rs#L226-L232=

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated in 324a76b.

/// 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 {
pub fn pos_at_coords(
text: RopeSlice,
coords: Position,
tab_width: usize,
limit_before_line_ending: bool,
) -> usize {
let Position { mut row, col } = coords;
if limit_before_line_ending {
row = row.min(text.len_lines() - 1);
Expand All @@ -125,11 +127,21 @@ pub fn pos_at_coords(text: RopeSlice, coords: Position, limit_before_line_ending
};

let mut col_char_offset = 0;
for (i, g) in RopeGraphemes::new(text.slice(line_start..line_end)).enumerate() {
if i == col {
let mut cols_remaining = col;
for grapheme in RopeGraphemes::new(text.slice(line_start..line_end)) {
let grapheme_width = if grapheme == "\t" {
tab_width
} else {
let grapheme = Cow::from(grapheme);
grapheme_width(&grapheme)
};

if grapheme_width > cols_remaining {
break;
}
col_char_offset += g.chars().count();

cols_remaining -= grapheme_width;
col_char_offset += grapheme.chars().count();
}

line_start + col_char_offset
Expand Down Expand Up @@ -245,64 +257,68 @@ 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(), 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
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

// 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);
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);

// 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);
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);

// 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);
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);

// 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);
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);

// 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);
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);
}
}
7 changes: 6 additions & 1 deletion helix-term/src/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,12 @@ 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, true));
let pos = Selection::point(pos_at_coords(
doc.text().slice(..),
pos,
doc.tab_width(),
true,
));
doc.set_selection(view_id, pos);
}
}
Expand Down
Loading