terminal: change cell width when wider grapheme detected - #10465
Conversation
mitchellh
left a comment
There was a problem hiding this comment.
The unit tests are good, but we're going to definitely have to test throughput of this one since print is the number one hot path. I didn't look closely at the implementation yet to have concrete suggestions (or, to see if it's just perfect).
I think testing cat speeds of large files, vet-bench, and and the terminal-stream Ghostty benchmark (which handles print) will do great. Even just one to start would help.
|
sounds good i'll look at running these |
|
I've been integrating with this branch it is looking good and agreement with interpretation of unicode standards and my next update with python wcwidth. The margin wrap stuff is something else, it does get complex, only Kovid has written a specification and it makes clear about "VS-16 (and similar) can wrap, but VS-15 does not unwrap" and the like. |
|
Thanks for the follow ups. Due to what this is touching I really need to do a deep line by line review on this and haven't had the chance yet. |
e216440 to
1c3fc06
Compare
|
Just compared
|
|
Just to chime in that I researched more today, the source of foot and windows terminal, they also cap graphemes to a width of 2. I don't think this is technically correct, but a common limitation of terminals not to support cells that are 3, 4 or 5 cells wide. kitty does terrible with these, making them narrow even, but recovers excellently when using text sizing protocol to "fill out" the desired width (3,4,5) measured by python wcwidth for complex script like javanaese.
Previously I allowed complex graphemes to grow up to width 5 in python wcwidth, e.g. https://github.com/jquast/ucs-detect/blob/e6532f0dd7d93dcb8fcfe9e0bc582ed915875c5a/docs/ucs_example_files/ucs_graphemes_5.txt depicted above And although that may better compliment the font engine's result, like in ghostty this sequence seems to "spill out" and over to 4 cells even though it is capped at 2 for cursor advance:
But anyway these 3 terminals (ghostty, foot, terminal.exe) are the only that appear to support complex graphemes, they all cap to 2 cells, and so the next release of wcwidth + spec will have a correction to match this and change ucs-detect accordingly. Screenshot from foot, also allows only 2 for "cursor advance" but spills out the same as ghostty,
|
From, ghostty-org/ghostty#10465 (comment) ghostty, foot, and windows terminal all "cap" the final grapheme width to 2 cells for cursor advance. Even if sometimes they "spill out" and over adjacent cells, the measurement of cursor advance is limited to 2. <img width="800" height="228" alt="image" src="https://github.com/user-attachments/assets/00821b21-97c0-4a69-a196-a6fe2a8c548d" /> <img width="1024" height="159" alt="image" src="https://github.com/user-attachments/assets/a6106e6c-5519-4498-9a7d-fa28dc256787" />
From, ghostty-org/ghostty#10465 (comment) ghostty, foot, and windows terminal all clip all final grapheme widths to 2 cells for cursor advance. Even though they "spill" out and over adjacent cells, the measurement of cursor advance is limited to 2 by their engines. ghostty (2): <img width="800" height="228" alt="image" src="https://github.com/user-attachments/assets/00821b21-97c0-4a69-a196-a6fe2a8c548d" /> foot (2): <img width="1024" height="159" alt="image" src="https://github.com/user-attachments/assets/a6106e6c-5519-4498-9a7d-fa28dc256787" />







This PR updates the logic in Terminal
printto include more cases of changing a cell to be wide due to a grapheme cluster that needs to be wide but starts off narrow. The existing case of this is a text-presentation code point followed by VS16 to make it emoji presentation. This PR handles more cases that are found in scripts such as Devanagari where the correct grapheme width calculation sums up multiple code points of non-zero widths. An example, as seen from uucode's issue #1 is啶曕鈥嵿し, which now with #9680 merged is one grapheme cluster instead of two, but the U+0915 (first code point) is width one and U+0937 (final code point) is also width one, and the whole cluster should be width 1 + 1 = 2. This is important to address with the grapheme break change otherwise these scripts would show with narrow cells, incorrectly.Before:
After:
Note that the logic here just takes
width_zero_in_graphemeand if it's not zero width, makes the cell wide. This is actually wrong for graphemes withprepend(usually/always? zero width) followed by a character that should be narrow width, but that's affecting a much smaller number of graphemes. To address that, we would need to run the fullwcwidthfromuucodeon the grapheme, and compare the width output with the current cell'sWide. I figured it'd be better to incrementally just handle the bulk of the cases with thewidth_zero_in_graphemecheck.This also adds tests to make sure moving the cell is handled correctly, which was not the case for the existing VS16 logic.
There's a lot of code here to handle transferring the graphemes when the narrow cell should wrap to the next line to become wide. I'd like feedback on the approach here before attempting to clean anything up, if desired (pull it out into a separate method?).
AI was used in some of the uucode changes in #9678 (Amp--primarily for tests), but everything was carefully vetted and much of it done by hand. This PR was made without AI.