6a3f7f2 · helix · git.iko.soy

git.iko.soy/helix

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

commit6a3f7f2c399f0b92cef97b0c85ebe976fd7cfcac authorMatthew Toohey <contact@mtoohey.com> dateTue 21 Jun 2022, 12:36 (4 years ago) parentfa4934c

5 files changed+153−79

compare with master

unifiedsplit

helix-core/src/lib.rs+3−1 helix-core/src/movement.rs+21−22 helix-core/src/position.rs+103−3 helix-term/src/commands.rs+14−12 helix-view/src/view.rs+12−41

helix-core/src/lib.rs+3−1

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

6363pub use {regex, tree_sitter};

6464

6565pub use graphemes::RopeGraphemes;

66−pub use position::{coords_at_pos, pos_at_coords, visual_coords_at_pos, Position};

66+pub use position::{

67+ coords_at_pos, pos_at_coords, pos_at_visual_coords, visual_coords_at_pos, Position,

68+};

6769pub use selection::{Range, Selection};

6870pub use smallvec::{smallvec, SmallVec};

6971pub use syntax::Syntax;

helix-core/src/movement.rs+21−22

@@ -5,16 +5,15 @@ use tree_sitter::{Node, QueryCursor};

66use crate::{

77 chars::{categorize_char, char_is_line_ending, CharCategory},

8− coords_at_pos,

98 graphemes::{

109 next_grapheme_boundary, nth_next_grapheme_boundary, nth_prev_grapheme_boundary,

1110 prev_grapheme_boundary,

1211 },

1312 line_ending::rope_is_line_ending,

14− pos_at_coords,

13+ pos_at_visual_coords,

1514 syntax::LanguageConfiguration,

1615 textobject::TextObject,

17− Position, Range, RopeSlice,

16+ visual_coords_at_pos, Position, Range, RopeSlice,

1817};

1918

2019#[derive(Debug, Copy, Clone, PartialEq, Eq)]

@@ -35,6 +34,7 @@ pub fn move_horizontally(

3534 dir: Direction,

3635 count: usize,

3736 behaviour: Movement,

37+ _: usize,

3838) -> Range {

3939 let pos = range.cursor(slice);

4040

@@ -54,15 +54,12 @@ pub fn move_vertically(

5454 dir: Direction,

5555 count: usize,

5656 behaviour: Movement,

57+ tab_width: usize,

5758) -> Range {

5859 let pos = range.cursor(slice);

5960

6061 // Compute the current position's 2d coordinates.

61− // TODO: switch this to use `visual_coords_at_pos` rather than

62− // `coords_at_pos` as this will cause a jerky movement when the visual

63− // position does not match, like moving from a line with tabs/CJK to

64− // a line without

65− let Position { row, col } = coords_at_pos(slice, pos);

62+ let Position { row, col } = visual_coords_at_pos(slice, pos, tab_width);

6663 let horiz = range.horiz.unwrap_or(col as u32);

6764

6865 // Compute the new position.

@@ -71,7 +68,7 @@ pub fn move_vertically(

7168 Direction::Backward => row.saturating_sub(count),

7269 };

7370 let new_col = col.max(horiz as usize);

74− let new_pos = pos_at_coords(slice, Position::new(new_row, new_col), true);

71+ let new_pos = pos_at_visual_coords(slice, Position::new(new_row, new_col), tab_width);

7572

7673 // Special-case to avoid moving to the end of the last non-empty line.

7774 if behaviour == Movement::Extend && slice.line(new_row).len_chars() == 0 {

@@ -446,6 +443,8 @@ pub fn goto_treesitter_object(

446443mod test {

447444 use ropey::Rope;

448445

446+ use crate::{coords_at_pos, pos_at_coords};

447+

449448 use super::*;

450449

451450 const SINGLE_LINE_SAMPLE: &str = "This is a simple alphabetic line";

@@ -472,7 +471,7 @@ mod test {

472471 assert_eq!(

473472 coords_at_pos(

474473 slice,

475− move_vertically(slice, range, Direction::Forward, 1, Movement::Move).head

474+ move_vertically(slice, range, Direction::Forward, 1, Movement::Move, 4).head

476475 ),

477476 (1, 3).into()

478477 );

@@ -496,7 +495,7 @@ mod test {

496495 ];

497496

498497 for ((direction, amount), coordinates) in moves_and_expected_coordinates {

499− range = move_horizontally(slice, range, direction, amount, Movement::Move);

498+ range = move_horizontally(slice, range, direction, amount, Movement::Move, 0);

500499 assert_eq!(coords_at_pos(slice, range.head), coordinates.into())

501500 }

502501 }

@@ -522,7 +521,7 @@ mod test {

522521 ];

523522

524523 for ((direction, amount), coordinates) in moves_and_expected_coordinates {

525− range = move_horizontally(slice, range, direction, amount, Movement::Move);

524+ range = move_horizontally(slice, range, direction, amount, Movement::Move, 0);

526525 assert_eq!(coords_at_pos(slice, range.head), coordinates.into());

527526 assert_eq!(range.head, range.anchor);

528527 }

@@ -544,7 +543,7 @@ mod test {

544543 ];

545544

546545 for (direction, amount) in moves {

547− range = move_horizontally(slice, range, direction, amount, Movement::Extend);

546+ range = move_horizontally(slice, range, direction, amount, Movement::Extend, 0);

548547 assert_eq!(range.anchor, original_anchor);

549548 }

550549 }

@@ -568,7 +567,7 @@ mod test {

568567 ];

569568

570569 for ((direction, amount), coordinates) in moves_and_expected_coordinates {

571− range = move_vertically(slice, range, direction, amount, Movement::Move);

570+ range = move_vertically(slice, range, direction, amount, Movement::Move, 4);

572571 assert_eq!(coords_at_pos(slice, range.head), coordinates.into());

573572 assert_eq!(range.head, range.anchor);

574573 }

@@ -602,8 +601,8 @@ mod test {

602601

603602 for ((axis, direction, amount), coordinates) in moves_and_expected_coordinates {

604603 range = match axis {

605− Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move),

606− Axis::V => move_vertically(slice, range, direction, amount, Movement::Move),

604+ Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move, 0),

605+ Axis::V => move_vertically(slice, range, direction, amount, Movement::Move, 4),

607606 };

608607 assert_eq!(coords_at_pos(slice, range.head), coordinates.into());

609608 assert_eq!(range.head, range.anchor);

@@ -627,18 +626,18 @@ mod test {

627626 let moves_and_expected_coordinates = [

628627 // Places cursor at the fourth kana.

629628 ((Axis::H, Direction::Forward, 4), (0, 4)),

630− // Descent places cursor at the 4th character.

631− ((Axis::V, Direction::Forward, 1usize), (1, 4)),

632− // Moving back 1 character.

633− ((Axis::H, Direction::Backward, 1usize), (1, 3)),

629+ // Descent places cursor at the 8th character.

630+ ((Axis::V, Direction::Forward, 1usize), (1, 8)),

631+ // Moving back 2 characters.

632+ ((Axis::H, Direction::Backward, 2usize), (1, 6)),

634633 // Jumping back up 1 line.

635634 ((Axis::V, Direction::Backward, 1usize), (0, 3)),

636635 ];

637636

638637 for ((axis, direction, amount), coordinates) in moves_and_expected_coordinates {

639638 range = match axis {

640− Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move),

641− Axis::V => move_vertically(slice, range, direction, amount, Movement::Move),

639+ Axis::H => move_horizontally(slice, range, direction, amount, Movement::Move, 0),

640+ Axis::V => move_vertically(slice, range, direction, amount, Movement::Move, 4),

642641 };

643642 assert_eq!(coords_at_pos(slice, range.head), coordinates.into());

644643 assert_eq!(range.head, range.anchor);

helix-core/src/position.rs+103−3

@@ -109,9 +109,6 @@ pub fn visual_coords_at_pos(text: RopeSlice, pos: usize, tab_width: usize) -> Po

109109/// with left-side block-cursor positions, as this prevents the the block cursor

110110/// from jumping to the next line. Otherwise you typically want it to be `false`,

111111/// such as when dealing with raw anchor/head positions.

112−///

113−/// TODO: this should be changed to work in terms of visual row/column, not

114−/// graphemes.

115112pub fn pos_at_coords(text: RopeSlice, coords: Position, limit_before_line_ending: bool) -> usize {

116113 let Position { mut row, col } = coords;

117114 if limit_before_line_ending {

@@ -135,6 +132,43 @@ pub fn pos_at_coords(text: RopeSlice, coords: Position, limit_before_line_ending

135132 line_start + col_char_offset

136133}

137134

135+/// Convert visual (line, column) coordinates to a character index.

136+///

137+/// If the `line` coordinate is beyond the end of the file, the EOF

138+/// position will be returned.

139+///

140+/// If the `column` coordinate is past the end of the given line, the

141+/// line-end position (in this case, just before the line ending

142+/// character) will be returned.

143+pub fn pos_at_visual_coords(text: RopeSlice, coords: Position, tab_width: usize) -> usize {

144+ let Position { mut row, col } = coords;

145+ row = row.min(text.len_lines() - 1);

146+ let line_start = text.line_to_char(row);

147+ let line_end = line_end_char_index(&text, row);

148+

149+ let mut col_char_offset = 0;

150+ let mut cols_remaining = col;

151+ for grapheme in RopeGraphemes::new(text.slice(line_start..line_end)) {

152+ let grapheme_width = if grapheme == "\t" {

153+ tab_width - ((col - cols_remaining) % tab_width)

154+ } else {

155+ let grapheme = Cow::from(grapheme);

156+ grapheme_width(&grapheme)

157+ };

158+

159+ // If pos is in the middle of a wider grapheme (tab for example)

160+ // return the starting offset.

161+ if grapheme_width > cols_remaining {

162+ break;

163+ }

164+

165+ cols_remaining -= grapheme_width;

166+ col_char_offset += grapheme.chars().count();

167+ }

168+

169+ line_start + col_char_offset

170+}

171+

138172#[cfg(test)]

139173mod test {

140174 use super::*;

@@ -305,4 +339,70 @@ mod test {

305339 assert_eq!(pos_at_coords(slice, (0, 10).into(), true), 0);

306340 assert_eq!(pos_at_coords(slice, (10, 10).into(), true), 0);

307341 }

342+

343+ #[test]

344+ fn test_pos_at_visual_coords() {

345+ let text = Rope::from("ḧëḷḷö\nẅöṛḷḋ");

346+ let slice = text.slice(..);

347+ assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);

348+ assert_eq!(pos_at_visual_coords(slice, (0, 5).into(), 4), 5); // position on \n

349+ assert_eq!(pos_at_visual_coords(slice, (0, 6).into(), 4), 5); // position after \n

350+ assert_eq!(pos_at_visual_coords(slice, (1, 0).into(), 4), 6); // position on w

351+ assert_eq!(pos_at_visual_coords(slice, (1, 1).into(), 4), 7); // position on o

352+ assert_eq!(pos_at_visual_coords(slice, (1, 4).into(), 4), 10); // position on d

353+

354+ // Test with wide characters.

355+ let text = Rope::from("今日はいい\n");

356+ let slice = text.slice(..);

357+ assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);

358+ assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 0);

359+ assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 1);

360+ assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 1);

361+ assert_eq!(pos_at_visual_coords(slice, (0, 4).into(), 4), 2);

362+ assert_eq!(pos_at_visual_coords(slice, (0, 5).into(), 4), 2);

363+ assert_eq!(pos_at_visual_coords(slice, (0, 6).into(), 4), 3);

364+ assert_eq!(pos_at_visual_coords(slice, (0, 7).into(), 4), 3);

365+ assert_eq!(pos_at_visual_coords(slice, (0, 8).into(), 4), 4);

366+ assert_eq!(pos_at_visual_coords(slice, (0, 9).into(), 4), 4);

367+ // assert_eq!(pos_at_visual_coords(slice, (0, 10).into(), 4, false), 5);

368+ // assert_eq!(pos_at_visual_coords(slice, (0, 10).into(), 4, true), 5);

369+ assert_eq!(pos_at_visual_coords(slice, (1, 0).into(), 4), 6);

370+

371+ // Test with grapheme clusters.

372+ let text = Rope::from("a̐éö̲\r\n");

373+ let slice = text.slice(..);

374+ assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);

375+ assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 2);

376+ assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 4);

377+ assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 7); // \r\n is one char here

378+ assert_eq!(pos_at_visual_coords(slice, (0, 4).into(), 4), 7);

379+ assert_eq!(pos_at_visual_coords(slice, (1, 0).into(), 4), 9);

380+

381+ // Test with wide-character grapheme clusters.

382+ let text = Rope::from("किमपि");

383+ // 2 - 1 - 2 codepoints

384+ // TODO: delete handling as per https://news.ycombinator.com/item?id=20058454

385+ let slice = text.slice(..);

386+ assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);

387+ assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 0);

388+ assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 2);

389+ assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 3);

390+

391+ // Test with tabs.

392+ let text = Rope::from("\tHello\n");

393+ let slice = text.slice(..);

394+ assert_eq!(pos_at_visual_coords(slice, (0, 0).into(), 4), 0);

395+ assert_eq!(pos_at_visual_coords(slice, (0, 1).into(), 4), 0);

396+ assert_eq!(pos_at_visual_coords(slice, (0, 2).into(), 4), 0);

397+ assert_eq!(pos_at_visual_coords(slice, (0, 3).into(), 4), 0);

398+ assert_eq!(pos_at_visual_coords(slice, (0, 4).into(), 4), 1);

399+ assert_eq!(pos_at_visual_coords(slice, (0, 5).into(), 4), 2);

400+

401+ // Test out of bounds.

402+ let text = Rope::new();

403+ let slice = text.slice(..);

404+ assert_eq!(pos_at_visual_coords(slice, (10, 0).into(), 4), 0);

405+ assert_eq!(pos_at_visual_coords(slice, (0, 10).into(), 4), 0);

406+ assert_eq!(pos_at_visual_coords(slice, (10, 10).into(), 4), 0);

407+ }

308408}

helix-term/src/commands.rs+14−12

@@ -16,14 +16,14 @@ use helix_core::{

1616 line_ending::{get_line_ending_of_str, line_end_char_index, str_is_line_ending},

1717 match_brackets,

1818 movement::{self, Direction},

19− object, pos_at_coords,

19+ object, pos_at_coords, pos_at_visual_coords,

2020 regex::{self, Regex, RegexBuilder},

2121 search::{self, CharMatcher},

2222 selection, shellwords, surround, textobject,

2323 tree_sitter::Node,

2424 unicode::width::UnicodeWidthChar,

25− LineEnding, Position, Range, Rope, RopeGraphemes, RopeSlice, Selection, SmallVec, Tendril,

26− Transaction,

25+ visual_coords_at_pos, LineEnding, Position, Range, Rope, RopeGraphemes, RopeSlice, Selection,

26+ SmallVec, Tendril, Transaction,

2727};

2828use helix_view::{

2929 clipboard::ClipboardType,

@@ -511,7 +511,7 @@ fn no_op(_cx: &mut Context) {}

511511

512512fn move_impl<F>(cx: &mut Context, move_fn: F, dir: Direction, behaviour: Movement)

513513where

514− F: Fn(RopeSlice, Range, Direction, usize, Movement) -> Range,

514+ F: Fn(RopeSlice, Range, Direction, usize, Movement, usize) -> Range,

515515{

516516 let count = cx.count();

517517 let (view, doc) = current!(cx.editor);

@@ -520,7 +520,7 @@ where

520520 let selection = doc

521521 .selection(view.id)

522522 .clone()

523− .transform(|range| move_fn(text, range, dir, count, behaviour));

523+ .transform(|range| move_fn(text, range, dir, count, behaviour, doc.tab_width()));

524524 doc.set_selection(view.id, selection);

525525}

526526

@@ -1412,9 +1412,10 @@ fn copy_selection_on_line(cx: &mut Context, direction: Direction) {

14121412 range.head

14131413 };

14141414

1415− // TODO: this should use visual offsets / pos_at_screen_coords

1416− let head_pos = coords_at_pos(text, head);

1417− let anchor_pos = coords_at_pos(text, range.anchor);

1415+ let tab_width = doc.tab_width();

1416+

1417+ let head_pos = visual_coords_at_pos(text, head, tab_width);

1418+ let anchor_pos = visual_coords_at_pos(text, range.anchor, tab_width);

14181419

14191420 let height = std::cmp::max(head_pos.row, anchor_pos.row)

14201421 - std::cmp::min(head_pos.row, anchor_pos.row)

@@ -1444,12 +1445,13 @@ fn copy_selection_on_line(cx: &mut Context, direction: Direction) {

14441445 break;

14451446 }

14461447

1447− let anchor = pos_at_coords(text, Position::new(anchor_row, anchor_pos.col), true);

1448− let head = pos_at_coords(text, Position::new(head_row, head_pos.col), true);

1448+ let anchor =

1449+ pos_at_visual_coords(text, Position::new(anchor_row, anchor_pos.col), tab_width);

1450+ let head = pos_at_visual_coords(text, Position::new(head_row, head_pos.col), tab_width);

14491451

14501452 // skip lines that are too short

1451− if coords_at_pos(text, anchor).col == anchor_pos.col

1452− && coords_at_pos(text, head).col == head_pos.col

1453+ if visual_coords_at_pos(text, anchor, tab_width).col == anchor_pos.col

1454+ && visual_coords_at_pos(text, head, tab_width).col == head_pos.col

14531455 {

14541456 if is_primary {

14551457 primary_index = ranges.len();

helix-view/src/view.rs+12−41

@@ -1,15 +1,9 @@

1−use std::borrow::Cow;

2−

31use crate::{

42 graphics::Rect,

53 gutter::{self, Gutter},

64 Document, DocumentId, ViewId,

75};

8−use helix_core::{

9− graphemes::{grapheme_width, RopeGraphemes},

10− line_ending::line_end_char_index,

11− visual_coords_at_pos, Position, RopeSlice, Selection,

12−};

6+use helix_core::{pos_at_visual_coords, visual_coords_at_pos, Position, RopeSlice, Selection};

137

148use std::fmt;

159

@@ -251,44 +245,21 @@ impl View {

251245 return None;

252246 }

253247

254− let line_number = (row - inner.y) as usize + self.offset.row;

255−

256− if line_number > text.len_lines() - 1 {

248+ let text_row = (row - inner.y) as usize + self.offset.row;

249+ if text_row > text.len_lines() - 1 {

257250 return Some(text.len_chars());

258251 }

259252

260− let mut pos = text.line_to_char(line_number);

261−

262− let current_line = text.line(line_number);

263−

264− let target = (column - inner.x) as usize + self.offset.col;

265− let mut col = 0;

266−

267− // TODO: extract this part as pos_at_visual_coords

268− for grapheme in RopeGraphemes::new(current_line) {

269− if col >= target {

270− break;

271− }

272−

273− let width = if grapheme == "\t" {

274− tab_width - (col % tab_width)

275− } else {

276− let grapheme = Cow::from(grapheme);

277− grapheme_width(&grapheme)

278− };

279−

280− // If pos is in the middle of a wider grapheme (tab for example)

281− // return the starting offset.

282− if col + width > target {

283− break;

284− }

253+ let text_col = (column - inner.x) as usize + self.offset.col;

285254

286− col += width;

287− // TODO: use byte pos that converts back to char pos?

288− pos += grapheme.chars().count();

289− }

290−

291− Some(pos.min(line_end_char_index(&text.slice(..), line_number)))

255+ Some(pos_at_visual_coords(

256+ *text,

257+ Position {

258+ row: text_row,

259+ col: text_col,

260+ },

261+ tab_width,

262+ ))

292263 }

293264

294265 /// Translates a screen position to position in the text document.