editor: Judge horizontal on-screen position in rendered space - #62310
Closed
4ktLuffy wants to merge 1 commit into
Closed
editor: Judge horizontal on-screen position in rendered space#623104ktLuffy wants to merge 1 commit into
4ktLuffy wants to merge 1 commit into
Conversation
`newest_selection_on_screen` compared `DisplayPoint::column()`, a byte offset, against `visible_column_count`, which `EditorElement` derives as `editor_width / em_advance` — a count of rendered cells. Byte offsets meet or exceed rendered columns for any non-ASCII text, so a cursor that is plainly on screen is reported as off it once enough multi-byte text precedes it on the line. Vim's Ctrl-D / Ctrl-U read that answer to decide whether to move the cursor, so the cursor gets left behind. `autoscroll_horizontally` already answers the same question correctly, by mapping the column through the line layout. `x_for_display_point` is that mapping, so this uses it for both the head and the screen top and compares the pixel delta against `em_advance * visible_columns` — the same width the column count was derived from. `newest_selection_on_screen` needs a `Window` to reach the text layout and so takes one; its single in-tree caller already has one in scope. The test pairs a 40-character multi-byte line with an ASCII line of the same rendered width in a 60-column window. It fails without this change (multibyte=Greater vs ascii=Equal) and passes with it, and asserts the ASCII control independently so a harness that computed nothing would not pass.
Contributor
|
Thank you for the PR, the issue seem to have turned out to be a bit more complex and #61997 in the end got to fix it. |
This file contains hidden or 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
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.
Closes #62308.
Problem
Editor::newest_selection_on_screenreports a cursor as off screen when enough multi-byte text precedes it on the line, even though it is plainly visible. Vim'sCtrl-D/Ctrl-Ucall it to decide whether to move the cursor, so the cursor is left behind.Root cause
crates/editor/src/scroll.rscompared two values in different units:DisplayPointwrapsBlockPoint(pub Point), so.column()is a byte offset.visible_column_countis set fromeditor_width / em_advanceinEditorElement(crates/editor/src/element.rs), a count of rendered cells. Byte offsets meet or exceed rendered columns for any non-ASCII text, so the comparison fails on lines it should accept.Fix
autoscroll_horizontallyalready answers the same question correctly, by mapping the column through the line layout withx_for_index.DisplaySnapshot::x_for_display_pointis exactly that mapping, so this uses it for both the head and the screen top and compares the pixel delta againstem_advance * visible_columns— the same widthvisible_column_countwas divided out of.Proof
The test pairs a 40-character multi-byte line with an ASCII line of identical rendered width, in a 60-column window. Both must be judged on screen, and both must agree.
Without the change:
With the change:
The ASCII control is asserted separately, so a harness that silently computed nothing would fail rather than pass.
Scope and risk
cargo test -p editor— 887 passed, 0 failedcargo test -p vim— 562 passed, 0 failedcargo fmt --all --checkandcargo clippy -p editor -p vim --all-targetscleanBehaviour only changes for the case that was wrong: a cursor whose rendered position is inside the viewport but whose byte column exceeded the column budget. ASCII text is unaffected, since there byte offsets and rendered columns coincide.
One decision for reviewers
newest_selection_on_screennow takes a&mut Window, because reaching the text layout requires one. There is a single in-tree caller (crates/vim/src/normal/scroll.rs) and it already has awindowin scope, but this is apub fnonEditor, so it is a breaking change for any out-of-tree caller.The alternative is to store the viewport width in pixels alongside
visible_column_countand compare against that, which avoids the signature change at the cost of duplicating state that is already derivable. Happy to switch if you prefer that.Related
#62305 fixes the same class of bug — byte offsets used where rendered position is meant — in
align_selections. Different file, different command; the two do not overlap and can land independently.