Optimizes View::screen_coords_at_pos by not computing char_to_line two times #4896
Conversation
kirawi
added
A-helix-term
| fn rope_graphemes(text: RopeSlice, pos: usize, row: usize) -> RopeGraphemes { | ||
| let row_start = text.line_to_char(row); | ||
| let pos = ensure_grapheme_boundary_prev(text, pos); | ||
| RopeGraphemes::new(text.slice(row_start..pos)) | ||
| } |
There was a problem hiding this comment.
This is not so much code that it needs a helper function IMO, and it's a bit confusing that there are other calls to RopeGraphemes::new that don't use rope_graphemes in this module. This can be inlined into the two callsites
There was a problem hiding this comment.
Respectfully, I disagree. This function encapsulates an important concept that text should be sliced taking into account grapheme boundaries. Maybe the name for the function and the place should be different but otherwise...
I don't know why in other places that's not the case, haven't looked deep.
Let's close this PR because neither I nor you don't have time to argue.
| let line = text.char_to_line(pos); | ||
|
|
There was a problem hiding this comment.
This is a small stylistic nit but it's more common in the codebase to see line when translating a position to a line number (rather than row)
There was a problem hiding this comment.
Maybe this is just a language barrier but for me it was confusing because there is row in Position. In any case I'm fine with any name.
|
Just a small heads: As Part of #5008 I will be replacing entire positioning code (including this function) to take virtual text, softwrapping and other kinds of decoration into account. Even if my specific PR doesn't get merged. Any implementation of these features will have to replace this code. A quick-fix could be nice but it's probably not worth investing a lot of time into optimizing a functiion that will be replaced in the future anyway |
|
Good to know. Thanks! Closing |
This solves a todo in
View::screen_coords_at_posmethod to not computechar_to_linetwo times.I've introduced a new function
core::visual_col_positionwhich additionally accepts a row index. Also I've made corresponding changes incore::positionmodule to remove some code duplication.