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.

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

Conversation

mtoohey31
Copy link
Contributor

As the TODO in move_vertically previously noted, the current behaviour leads to jerky vertical movement on lines with wide characters and tabs. This pull request refactors a few of the related functions (and resolves some of their TODOs in the process) so that vertical movement is always smooth.

Since the parameters to pos_at_coords had to be modified to accept tab_width, quite a few tests had to be updated. Some of these tests also needed their expected values changed since pos_at_coords now operates based on visual row/column instead of graphemes, as the TODO called for. I'm not 100% sure if I updated a few of the tests correctly though, in particular, this assertion, and this block. If someone could offer a second opinion on those, I'd appreciate it!

@@ -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.

Comment on lines +159 to +161
if grapheme_width > cols_remaining {
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added in 3c17265.

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)
Copy link
Member

Choose a reason for hiding this comment

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

The logic here and in the conditional looks slightly different though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true, it is still slightly different. The other implementation keeps track of the current column, while mine keeps track of the columns remaining before we reach the target column, but I think the behaviour is still the same.

@mtoohey31
Copy link
Contributor Author

mtoohey31 commented Jun 7, 2022

I just realized that copy_selection_on_line also doesn't respect wide characters, so 274746e updates things to fix that too.

Are there any other commands that I've missed that should also be modified?

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, this looks correct to me and I couldn't find any more spots that need fixing. Great work! 🎉

@archseer archseer merged commit 6a3f7f2 into helix-editor:master Jun 21, 2022
@mtoohey31 mtoohey31 deleted the feat/widechar-aware-vertical-move branch December 10, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants