Fix minor oversights in positioning code #6440
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This assert was added during early development of helix-editor#5420 and makes no sense with the current code. We simply forgot to remove it.
The top of a view is marked by a char idx anchor. That char idx is usually the first character of the visual line it's on. We use a char index instead of a line index because the view may start in the middle of a line with soft wrapping. However, it's possible to temporarily endup in a state where this anchor is not the first character of the first visual line. This is pretty rare because edits usually happen inside/after the view. In most cases we handle this case correctly. However, if the cursor is before the anchor (but still in view) there can be crashes or visual artifacts. This is caused by the fact that visual_offset_from_anchor (and the positioning code in view.rs) incorrectly assumed that the (cursor) position is always after the view anchor if the cursor is in view. But if the anchor is not the first character of the first visual line this is not the case anymore. In that case crashes and visual artifacts are possible. This commit fixes that problem by changing `visual_offset_from_anchor` (and callsites) to properly consider that case.
Using `partition_point` ensures we always find the first entry. With binary search it is "random" (deterministic but implementation specific) which index is retruned if there are multiple equal elements. `partition_point` was added to the standard library to cover extactly the usecase here.
Virtual text lines (either caused by softwrapped inlay hints that take multiple or line annotations) currently block scrolling downwards. if the visual offset passed to char_idx_at_visual_offset or visual_offset_from_block is within a virtual text line then the char position before the virtual text and a visual offset are returned. We previously ignored that visual offset and as a result the cursor would be stuck at the start of the virtual text. This commit fixes that by simply moving the cursor to the next char (so past the virtual text) if this visual offset is non-zero
While scrolling (with the `scroll`) command scrolloff was calculated slightly differently than in `ensure_cursor_in_view` which could cause the cursor to get stuck while scrolling
char_idx_at_visual_row_offset asssumed that a single line/block break always corresponded to a vertical offset of 1. However conceal can hide the line break (in which case the certical offset would be 0) and line annotations (or softwrapped inlay hints at the end of the line) can insert addtional vertical lines. To correctly account for these cases we simply compute the visual offset of the start of the next block from the previous block instead of the visual offset of the block end. This means that the line breaks at the end of the block (however many there may be) are automatically included and we don't need to manually add 1 to the `row_offset` anymore.
pascalkuthe
added
E-medium
Merged
dead10ck
approved these changes
Mar 26, 2023
| @@ -98,7 +98,7 @@ pub use {regex, tree_sitter}; | |||
| pub use graphemes::RopeGraphemes; | |||
| pub use position::{ | |||
| char_idx_at_visual_offset, coords_at_pos, pos_at_coords, visual_offset_from_anchor, | |||
| visual_offset_from_block, Position, | |||
| visual_offset_from_block, Position, VisualOffsetError, | |||
There was a problem hiding this comment.
the position module is private so I wouldn't be able to access this type at all if it wasn't reexported here (which is needed for matching on it, see view.rs).
There was a problem hiding this comment.
Oh right, sorry, I didn't see this was a pub use
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #6417
Fixes #6397
This is a collection of bugfixes for oversights introduced in #5420 that I noticed while working on #6417. I backported these to a standalone PR, so they can merged independently of the larger changes in #6417. It's a bit of a kitchen sink PR in that these fixes are pretty independent of each other, but it didn't seem worth splitting each commit into seperate PR because they are mostly not large changes and didn't really contain any design decisions (just bugfixes) so it didn't seem like they would need separate discussion.
Each commit in this PR is a standalone bugfix mostly independent of the other fixes. As such I added a detailed description to each commit to describe what it's doing from a technical point of view. A short summery below