Fixes #4387. Runes should not be used on a cell, but rather should use a single grapheme rendering 1 or 2 columns #4388
Conversation
…urn the total text width and not the sum of all runes width
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Please note that this feature is not compatible with legacy consoles, such as cmd.exe and conhost.exe, due to known limitations, such as true colors. 
|
|
@tig I did an update branch from here and it broken my local branch when I pulled. Do you know what's happened? |
I managed to solve it, but I don't understand how it happened. |
|
@tig the below message appear sometimes when any test fail in racing conditions. Do you know it exist any static configuration that can be changed while another unit test is running? |
Parallelizable, right? Might have something to do with GlobalTestSetup. |
|
Damn, testhost.exe take 80% CPU resources. I'll only running all tests in the Terminal.Gui CI actions. |
I think I fix this issue in b6072bb. |
|
After upgrading Wcwidth to version 4.0.0, I had to disable a few unit tests until I see if there's a solution or better explanation, as commented in #4259 (comment). But there are more advantages to using this version than disadvantages because there are many corrections regarding code points that were previously considered narrow and are now correctly considered zero-length. |
|
@BDisp this ready to merge? |
tig
left a comment
tig
left a comment
There was a problem hiding this comment.
Good stuff. I think. Hard for me to grok it all.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the core text handling in Terminal.Gui by replacing the Rune-based Cell representation with a grapheme-based approach using string. The primary goal is to properly support grapheme clusters (complex Unicode sequences like emojis with zero-width joiners) that should render as a single visual unit occupying 1 or 2 columns.
Key Changes:
- Replaced
Cell.Runeproperty withCell.Grapheme(typestring) - Updated text processing throughout the codebase to work with grapheme strings instead of individual runes
- Enhanced
StringExtensions.GetColumns()to properly calculate display width for grapheme clusters - Updated test assertions to compare graphemes (strings) instead of runes
Reviewed Changes
Copilot reviewed 53 out of 54 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTestsParallelizable/Views/TextViewTests.cs | Updated test assertions from Rune to Grapheme property access |
| Tests/UnitTestsParallelizable/Views/TextFieldTests.cs | Changed driver contents access to use Grapheme instead of Rune |
| Tests/UnitTestsParallelizable/Text/TextFormatterTests.cs | Updated test data and renamed test methods for grapheme handling |
| Tests/UnitTestsParallelizable/Text/StringTests.cs | Added comprehensive grapheme-related tests |
| Tests/UnitTestsParallelizable/Drawing/CellTests.cs | Rewrote cell tests to validate grapheme functionality |
| Terminal.Gui/Views/TreeView/Branch.cs | Updated tree rendering to work with grapheme strings |
| Terminal.Gui/Views/TextInput/TextView.cs | Refactored text handling from runes to graphemes |
| Terminal.Gui/Views/TextInput/TextModel.cs | Updated text model operations for grapheme support |
| Terminal.Gui/Views/TextInput/TextField.cs | Changed internal text storage from List<Rune> to List<string> |
| Terminal.Gui/Text/StringExtensions.cs | Enhanced grapheme enumeration and column calculation |
| Terminal.Gui/Drivers/*.cs | Updated driver interfaces and implementations for grapheme support |
Comments suppressed due to low confidence (1)
Terminal.Gui/Drawing/LineCanvas/LineCanvas.cs:180
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var line in _lines)
{
if (line.Intersects (x, y) is { } intersect)
{
intersectionsBufferList.Add (intersect);
}
}
Thanks. You will find that it will be much easier to manipulate virtually all code points and all valid combinations without worrying about the complexity of handling all possible combinations. It also resolves all situations where, using combinations with 
|
|
@tig I think this PR is finalized. Please understand that any changes you make to the Content or Cell will overwhelm me with resolving additional merges. If possible, I would appreciate it if you could perform the merge as soon as possible. Thanks. |
2 participants
Fixes
Proposed Changes/Todos
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)