display: end an escape sequence where the specification says it ends - #756
Merged
Conversation
composition_engine carried its own width walk beside a call to the canonical one 300 lines away in the same file. The copy enumerated ANSI terminators -- m K J H A B C D G f s u -- so a sequence ending in `h` or `l` never terminated, and `\033[?25l` / `\033[?25h` is the cursor hide/show pair a shell emits constantly. Everything after one measured as ZERO width. It also carried a standing `TODO: Proper UTF-8 width calculation` and counted one column per character. Deleted; both sites in that file now call the shared computation. Auditing the terminator sets then found the same class of error in the SHARED walk. It ended a sequence at the first letter, which looks equivalent because most final bytes are letters -- but ECMA-48 defines the final byte as the range 0x40-0x7E, and `\033[3~` ends in `~`. The shared function measured the whole remainder of such a text as zero width, so consolidating onto it would have spread that one case rather than fixing it. The core now implements the rule the specification states: consume the introducer, and for a CSI or OSC run to the first byte in 0x40-0x7E. A two-character escape ends at once. This is what the completion menu renderer already did -- the correct implementation existed, in the copy nobody had consolidated onto. Curation, recorded: no reference shell is the authority on how lush measures its own prompt, so this is not a consensus question. The question is whether lush can do better than the heuristic it had, and it can -- the specification defines the answer exactly, and every hand-rolled enumeration in this tree disagreed with it in a different way. tests/unit/test_screen_buffer.c pins the terminators that broke each copy: SGR colour, cursor hide/show ending in `l` and `h`, erase-line `K`, clear-screen `J`, cursor-position `H`, multi-parameter SGR, and `\033[3~` -- the final byte that is not a letter, which the shared function itself failed until now.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Second slice of duplicate-path item 6, and the more interesting one: consolidating onto the shared function exposed a bug in the shared function.
What the copy did
composition_engine.ccarried its own width walk beside a call to the canonical one 300 lines away in the same file. The copy:m K J H A B C D G f s u, so a sequence ending inhorlnever terminated — and\033[?25l/\033[?25his the cursor hide/show pair a shell emits constantly. Everything after one measured as zero width.TODO: Proper UTF-8 width calculationand counted one column per character.Deleted. Both sites in that file now call the shared computation.
What auditing the terminators then found
The shared walk had the same class of error. It ended a sequence at the first letter — which looks equivalent, because most final bytes are letters. But ECMA-48 defines the final byte as 0x40–0x7E, and
\033[3~ends in~.So the shared function measured the remainder of such text as zero width too. Consolidating onto it without checking would have spread that case rather than fixing it.
The core now implements the specified rule: consume the introducer, and for CSI/OSC run to the first byte in 0x40–0x7E; a two-character escape ends at once. This is exactly what
completion_menu_rendereralready did — the correct implementation existed, in the copy nobody had consolidated onto.Curation
No reference shell is the authority on how lush measures its own prompt, so this is not a consensus question. The question is whether lush can beat the heuristic it had — and it can, because the specification defines the answer exactly. Every hand-rolled enumeration in this tree disagreed with it in a different way, which is the argument for having one.
Verification
tests/unit/test_screen_buffer.cpins the terminators that broke each copy: SGR colour, cursor hide/show (l,h), erase-lineK, clear-screenJ, cursor-positionH, multi-parameter SGR, and\033[3~— the non-letter final byte the shared function itself failed until this change.Full suite 197/197. ASan 65/65.