Align tabs to the next tab stop instead of a fixed advance - #612
Open
danipen wants to merge 11 commits into
Open
Align tabs to the next tab stop instead of a fixed advance#612danipen wants to merge 11 commits into
danipen wants to merge 11 commits into
Conversation
TextEditorOptions.GetIndentationString already knew how far away the next tab stop is. The tab rendering that follows needs the same answer, so give both one definition to call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tab width is layout, not decoration, so it is about to stop depending on ShowTabs. Give tabs their own generator first, while the behaviour is still identical, so that the change of behaviour arrives in a file that does nothing else. SingleCharacterElementGenerator keeps the two decorations it is named for. It has to keep excluding tabs explicitly, though: a tab is a control character, and the branch that used to shadow it has moved out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A tab's width depends on the column it starts at, so that column has to be known while the elements are being built. Carry it forward as they are added, the way the text is laid out, rather than deriving it backwards from the document: elements can render text of a different length than they occupy, and a visual line can span several document lines. Nothing reads it yet, so behaviour is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A tab advanced by a fixed width instead of reaching the next multiple of the indentation size, so text after a tab only lined up when it happened to start on a tab stop already. Trailing comments drifted one column per line and snapped back every fourth line. The text shaper gives every '\t' glyph the same advance (TextShaperOptions.IncrementalTabWidth) and knows nothing about the column the tab starts at, which is by design there. So expand the tab here instead: the element now spans the columns up to the next tab stop, drawing the glyph in the first one and padding the rest. The glyph run is one column wide rather than as wide as the glyph, so a wide ShowTabsGlyph still cannot stretch a tab (#206, #207), and a tab occupies the same columns whether or not the glyph is shown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Ollie Blanks <ollieb@unity3d.com>
Both render more than one column of text while occupying a single visual column, so a tab following them on the same visual line was measured from the wrong place: a collapsed folding showing "..." counted as one column, and so did a box showing "NUL". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every tab formatted its own copy of the glyph, on every redraw. A file indented with tabs pays that several times per line. The tab generator is about to run whether or not the glyph is shown, so pay it down first. No test: the glyph is only observable through Draw, which the headless tests do not exercise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The width was wrong in both cases, and ShowTabs is about showing a glyph, not about where text lands. The generator is therefore always present, and ShowTabs now only decides whether the glyph is drawn. This changes the default rendering for every consumer, which is why it is its own commit. Two consequences worth knowing about: - visual columns after a tab change, so anything that persists TextViewPosition.VisualColumn across versions, or keys on visual columns, shifts. Toggling ShowTabs no longer shifts them, which it used to. - the generator is added on construction, so at a tab offset it now takes precedence over an element generator added later by a consumer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generator is asked for its next tab once per element it produces, and each ask scanned to the end of the line character by character, so a line of k tabs cost k passes over it. IndexOf is vectorised, which the next commit relies on: it lets these scans run on lines long enough that element generation is otherwise skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lines over LENGTH_LIMIT skip element generation and become a single text element, so their tabs still reached the shaper as raw '\t' and kept the fixed advance. Tab separated data is exactly the content that has both long lines and tabs. Generators now say whether the layout depends on them, and long lines run those instead of none. With none left to run, the loop produces the single text element the early return used to, so nothing else changes. Cost, per build of one 2999 character line: all tabs 81ms against 0.2ms plain. That is the per element cost the visual line pipeline already has, not something new here: the same line in spaces with ShowSpaces on is 46ms. A realistic viewport, 50 lines of eight tabs and code, is 2.3ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A tab used to span two visual columns whatever its width. It now spans as many as it is wide, and it exists even when the glyph is hidden, so the caret has more room to get this wrong: it must step over a tab, and a column inside one must map to the tab's own offset for a click to land sensibly. This is a property of the branch as a whole rather than of one commit, which is why it is not with either of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
IncrementalTabWidth looks like the WPF property it was ported from, so the obvious conclusion is that Avalonia should align to it and this expansion is redundant. It is not: a fixed advance is what that property means there. Say so where the value is set, so the next reader does not take it upstream again, and state the monospaced assumption on the generator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Problem
Text after a tab does not line up on a tab stop.
A tab is handed to the text formatter as
"\t", and Avalonia's shaper gives every tab the same fixed advance (IndentationSizespaces) regardless of the column it starts at. A tab should instead advance to the next multiple ofIndentationSize.Leading indentation looks fine either way, which is why this is easy to miss: a tab at column 0, 4 or 8 lands on a stop under both behaviours. It breaks as soon as a tab starts anywhere else, for example when tabs align trailing comments:
Context
This was first fixed in #611 by @ollieb-unity. That PR is correct, and this one keeps the same idea: expand each tab here instead of letting the formatter size it. I reviewed it and wanted a few changes, so this is a rewrite of it rather than a different fix. #611 will be closed in favour of this.
What changed compared to #611
SingleCharacterElementGeneratorinto a newTabElementGenerator. Tab layout must always happen, whileSingleCharacterElementGeneratoris only attached whenShowSpacesorShowBoxForControlCharactersis on. Separating them keeps that on-demand behaviour intact.VisualLinenow tracks the display column while it builds the line, so the generator reads it directly instead of re-scanning the text before each tab. That makes it O(1) per tab instead of O(n). Elements that render wider or narrower than their visual length (foldings, control character boxes) report their ownDisplayColumnLength, so a tab after them still lands correctly.AsMemory, rather than allocating anew string(' ', n)for every tab.Also in this PR
ShowTabsis on — the width was wrong in both cases. The glyph is still drawn as a zero-width overlay, so the fix from Tab sign change the position of line #206 / Fix tab glyph extra width #207 still holds.VisualLine.LENGTH_LIMITfor performance; generators can now opt in withRunsOnLongLines, and only the tab generator does.TabStophelper, shared withTextEditorOptions.GetIndentationString.Tests
TextViewTestscovers: reaching the next tab stop from various columns, consecutive tabs, on-screen width, tabs after a control character box and after a collapsed folding, long lines, caret movement across a tab, and equal width with the glyph shown or hidden.TabStopTestscovers the arithmetic. Full suite passes (528 tests).