display: one width computation, three CJK bugs (duplicate-path sweep, item 6) - #755
Merged
Conversation
Three functions in this file answered "how many columns does this text
occupy" three different ways, and they disagreed on the answers that matter:
- screen_buffer_calculate_visual_width walked TR#29 grapheme clusters and
asked the width table. CORRECT.
- screen_buffer_visual_width counted EVERY multi-byte character as one
column, so a CJK character measured 1 instead of 2 and a combining mark 1
instead of 0.
- line_index_visual_width walked CODEPOINTS rather than clusters, so it
weighed a combining mark on its own instead of as part of the character it
belongs to.
The middle one has a single caller: the right prompt's width. Its position
therefore drifted by one column for every wide character in it -- exactly the
kind of defect that looks like a rendering glitch and gets lived with.
The correct implementation becomes visual_width_core(text, byte_len,
start_col) and the other two become one-line wrappers. Both wrappers are kept
rather than deleted: screen_buffer_visual_width is a tested public API, and
the line index measures a bounded slice of a larger blob rather than a
NUL-terminated string, so the byte bound is load-bearing. Net -108 lines.
The core also carries the file's one ANSI-escape skip and the readline
`\001`/`\002` handling, so those stop being duplicated here too.
No behavior changes for ASCII, for ANSI escapes, or for the 2-byte characters
the existing tests cover -- those already agreed. What changes is that wide
and combining characters now measure the same everywhere.
test_screen_buffer.c gains a test that asserts the two exported functions
AGREE and that both give the right absolute answer, over ascii, a 2-byte
character, hiragana, CJK between ascii, two ideographs, a base plus combining
mark, and a CJK character inside ANSI escapes. Asserting agreement as well as
value means a future change that breaks both the same way still fails.
Mutation-proven: it fails against the parent build and passes here.
calculate_prompt_metrics counted one column per non-continuation BYTE, so a prompt containing wide characters reported a max_line_width narrower than it draws -- a CJK character measured 1 column instead of 2, a combining mark 1 instead of 0. estimated_command_column derives from that width, so the command started too far left. This is the same defect screen_buffer_visual_width carried, in a second file, which is what made it worth finding: two independent hand-rolled width walks with the same flaw. Per-line width now comes from screen_buffer_visual_width, the display subsystem's single width computation as of the previous commit. Its bounded (text, byte_length) form is what makes the reuse possible -- a prompt line is a slice of the content, not a NUL-terminated string -- so no new API was needed and no copy of the walk survives here. The line-count semantics are preserved exactly, including the two cases worth naming: content with no trailing newline counts its final line, and content ending in a newline does not count an extra empty one. has_ansi_sequences and has_unicode are properties of the whole content, so they are computed in one pass over it rather than folded into the per-line walk. test_prompt_layer.c gains a table covering ascii, one hiragana, two ideographs with a prompt suffix, a base plus combining mark, a wide character inside ANSI escapes, a multi-line prompt where the widest line wins, and the trailing newline case. Mutation-proven: reverting prompt_layer.c alone fails it.
The composer computed PS1, PS2 and RPROMPT widths with its own walk carrying two defects, both on the path that positions the interactive cursor. It assumed every 3-byte UTF-8 sequence is two columns. That is right for CJK and wrong for the many 3-byte characters that occupy one -- an em dash, a check mark, most currency signs -- so a prompt containing any of them measured too wide and the cursor sat too far right. Its ANSI skip ended a sequence only on `m`. Any other terminator -- `\033[K` to erase a line, `\033[H`, a cursor movement -- left it stuck in escape mode, and the entire remainder of the prompt measured as ZERO width. That one is not theoretical: a theme emitting anything but an SGR sequence hits it. Width now comes from lle_utf8_string_width, which asks the canonical per-codepoint table, and the escape skip runs to the CSI final byte (0x40-0x7E) instead of looking for one letter. The primitive used here is LLE's, deliberately, not the display subsystem's equivalent: display depends on LLE and not the other way round, so reaching across would invert the layering the design document describes. Two subsystems answering the same question is the thing being reduced -- but the seam between them is the constraint, and this respects it rather than collapsing it. VERIFICATION, stated precisely: the composer's own function is static and its render path needs a theme and segment registry whose output is environment dependent, so there is no composer-level assertion here. What is now pinned is the primitive it delegates to. lle_codepoint_width was already covered; the STRING-level walk that decodes UTF-8 and sums it was not, and callers measure prompts with it. tests/lle/unit/test_char_width.c gains that coverage: ascii, empty, a 2-byte character, wide 3-byte characters, the NARROW 3-byte characters the old guess got wrong (em dash, check mark, rupee), a base plus combining mark, and a mixed run.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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
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.
First slice of the duplicate-path backlog, item 6. Three independent hand-rolled width walks, three wrong answers for wide characters — found by consolidating them, not by a bug report.
The defects
screen_buffer_visual_widthline_index_visual_widthcalculate_prompt_metricscalculate_visual_width(composer)The composer also ended an ANSI sequence only on
m, so\033[K,\033[Hor any cursor movement left it stuck in escape mode and the entire rest of the prompt measured as zero width. Not theoretical — any theme emitting a non-SGR sequence hits it.screen_buffer_calculate_visual_widthwas already correct (TR#29 clusters + the width table + tab stops). It becomesvisual_width_core(text, byte_len, start_col); the other two display-side walks become one-line wrappers. Net −108 lines inscreen_buffer.c.Layering respected rather than collapsed
The composer is LLE, and display depends on LLE — not the reverse. So it routes to LLE's own canonical primitive (
lle_utf8_string_width→lle_codepoint_width) instead of reaching across into display. Reducing duplicate answers is the goal; the subsystem seam is the constraint.Two corrections to the tracker (from 2026-05-29)
screen_buffer_render_menu/calculate_menu_widthare already deleted — the only remaining reference is a comment saying so. Nothing to do.visual_width,calc_visual_width) for a follow-up.Verification
test_screen_buffer.c: a test asserting the two exported functions agree and give the right absolute answer — ascii, 2-byte, hiragana, CJK between ascii, two ideographs, base+combining, CJK inside ANSI. Asserting agreement as well as value means a change breaking both the same way still fails. Mutation-proven against the parent build.test_prompt_layer.c: a table over ascii, hiragana, two ideographs, base+combining, wide-inside-ANSI, widest-line-wins, and the trailing-newline line count. Mutation-proven (revertingprompt_layer.calone fails it).test_char_width.c: the string-level walk had no coverage though callers measure prompts with it — now pinned, including the narrow 3-byte characters the composer's guess got wrong.Honest limit: the composer's own function is static and its render path needs an environment-dependent theme/segment registry, so there is no composer-level assertion. What is pinned is the primitive it now delegates to.