fix(TextAreaControl): measure glyph width for the horizontal scroll extent#63
Open
DraftingDreamer wants to merge 1 commit into
Open
Conversation
…xtent The horizontal scrollbar works in WideSpaceWidth units (the width of 'x'), but its extent was derived from GetVisualColumnFast, which counts characters and assumes every one of them is exactly one column wide. Any glyph wider than 'x' - CJK, emoji, glyphs coming from a fallback font, or any proportional font - therefore makes the scroll range shorter than the line actually painted. Depending on how far off it is, either the scrollbar is not shown at all, or its thumb cannot be dragged to the end of the line. Setting HScrollBar.Value from code (caret movement, selection drag) uses the accurate GetVisualColumn and can reach Maximum, so those paths scroll further right than the thumb allows - which is how the mismatch becomes visible to the user. Replace the character count with GetVisualWidthColumns, which measures the glyphs using the existing per-character width cache and mirrors the tab handling of PaintLinePart. The cached line widths now depend on the font, so they are invalidated on OptionsChanged.
There was a problem hiding this comment.
Pull request overview
This PR fixes horizontal scrollbar sizing in TextAreaControl by measuring the painted glyph widths (rather than counting characters) so wide glyphs (e.g., CJK) properly contribute to horizontal scroll extent, addressing gitextensions/gitextensions#13200.
Changes:
- Add
TextView.GetVisualWidthColumns()to compute a line’s visual width inWideSpaceWidthunits using glyph measurements and tab-stop arithmetic. - Update
TextAreaControl.UpdateLayout()to use the measured visual width (with lazyGraphicscreation) and clear cached widths on option changes. - Add two scrollbar regression tests covering wide-glyph overflow and rightmost reachability.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Test/TextEditorControl/ScrollBarTests.cs | Adds regression tests to ensure wide glyphs trigger and correctly size horizontal scrolling. |
| Project/Src/Gui/TextView.cs | Introduces a new API to compute measured visual line width in scrollbar column units. |
| Project/Src/Gui/TextAreaControl.cs | Switches scrollbar extent calculation to measured widths and invalidates cached widths when editor options change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+713
to
+731
| public int GetVisualWidthColumns(Graphics g, LineSegment line) | ||
| { | ||
| var font = TextEditorProperties.FontContainer.RegularFont; | ||
| var tabWidth = WideSpaceWidth*Document.TextEditorProperties.TabIndent; | ||
| var lineOffset = line.Offset; | ||
| var width = 0; | ||
|
|
||
| for (var i = 0; i < line.Length; ++i) | ||
| { | ||
| var ch = Document.GetCharAt(lineOffset + i); | ||
| if (ch == '\t') | ||
| width = (width + MinTabWidth)/tabWidth*tabWidth + tabWidth; | ||
| else | ||
| width += GetWidth(g, ch, font); | ||
| } | ||
|
|
||
| // round up so that a glyph which only partially reaches into the last column stays reachable | ||
| return (width + WideSpaceWidth - 1)/WideSpaceWidth; | ||
| } |
| { | ||
| TextArea.OptionsChanged(); | ||
|
|
||
| // the cached line widths are measured with the current font, so they are stale after a font change |
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.
Fixes gitextensions/gitextensions#13200
Proposed changes
TextView.GetVisualWidthColumns()(new) returns the painted width of a line inWideSpaceWidthunits, measuring the glyphs through the existing per-character width cache (GetWidth(Graphics, char, Font)) and mirroring the tab-stop arithmetic ofPaintLinePart.TextAreaControl.UpdateLayout()uses it instead ofGetVisualColumnFast(), which counted characters and so underestimated every glyph wider thanx(CJK, emoji, fallback-font glyphs). TheGraphicsis created lazily, so a warmlineLengthCachestill measures nothing.TextAreaControl.OptionsChanged()clearslineLengthCache: the cached widths are now font-dependent.Screenshots
All shots are Git Extensions built from
masterat 200% scaling (192 dpi), on a throwaway repository containing CJK source lines, cropped to the diff pane.Before
The scrollbar has been dragged fully right and the right arrow clicked repeatedly afterwards - the view does not move any further, yet the string literal is cut off mid-sentence:
A file whose longest line has fewer characters than the viewer has columns, but whose glyphs need about twice that width: the line is clipped and there is no scrollbar at all.
The Commit dialog, same story:
After
Dragging fully right now reaches the end of the line, closing quote and semicolon included:
Same file and same scroll position as the second "before" shot - the scrollbar is now there:
Test methodology
Two new tests in
ScrollBarTests, both failing without the change:HScrollBar_should_be_shown_when_wide_glyphs_exceed_the_visible_widthHScrollBar_should_allow_scrolling_to_the_end_of_a_line_of_wide_glyphsThey derive their thresholds from the control at run time (glyph width,
WideSpaceWidth,DrawingPosition) rather than hard-coding pixels, and they assert the precondition that the glyph really is wider than one column, so on a machine without CJK fallback fonts they fail loudly instead of silently passing.Whole suite: 54/54 pass (
dotnet test --test-adapter-path:.).Pure ASCII is provably unchanged: with Consolas 10,
TextRenderer.MeasureTextreports exactly 100 xWideSpaceWidthfor 100 ASCII characters, so the computed extent is identical to before.Verified end to end in Git Extensions itself - Diff tab and Commit dialog, before and after builds - see the screenshots above.
Notes for the reviewer
GetVisualColumnFast()has no callers left, here or in gitextensions. I left it in place because it is public API - happy to remove it in this PR if you would rather.lineLengthCache(the visible lines right after a document change or resize).Test environment(s)
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.