Conversation
e36f2ef to
94faeaf
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Combining characters are discarded, and supplementary or edited wide characters can corrupt storage, rendering, cursor positioning, and copied text.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (5)
What changed in this PR
Adds East Asian wide-character support to the terminal’s model, emulator, rendering, copying, and repaint behavior.
Changes:
- Introduces Unicode width classification and filler cells.
- Updates emulator positioning, wrapping, insertion, rendering, and copying.
- Adds width and emulator tests plus the required bundle version increment.
| File | Description |
|---|---|
CharWidth.java |
Defines Unicode display widths and filler detection. |
VT100EmulatorBackend.java |
Adds cell-aware writing and wide-character handling. |
TextLineRenderer.java |
Renders wide and supplementary characters. |
TextCanvas.java |
Expands repaint ranges across wide glyphs. |
AbstractTextCanvasModel.java |
Removes fillers from copied text. |
CharWidthTest.java |
Tests width classification and fillers. |
AllTestSuite.java |
Registers the new width tests. |
VT100EmulatorBackendTest.java |
Tests placement, wrapping, overwriting, and insertion. |
META-INF/MANIFEST.MF |
Increments the bundle service version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Rebased on current master and addressed the Copilot review in 5bb7086. The GitHub Actions failures on the previous head were not from this change: the build stopped in org.eclipse.core.filesystem with "Baseline and reactor have the same fully qualified version, but different content", because the old base still had 1.11.500 while master has moved to 1.12.0. The rebase should clear it. @akurtakov thanks for triggering the review. |
The emulator assumed every character occupies one cell, so Hangul, Han and Kana text, fullwidth forms and emoji were placed one column short per character and the screen fell apart as soon as a program laid text out for a real terminal (line editors, curses UIs, Ink based CLIs). Add CharWidth, a UAX eclipse-platform#11 East Asian Width lookup: Wide and Fullwidth count as two columns, combining marks and controls as zero, Ambiguous as one, as UAX eclipse-platform#11 recommends outside an East Asian legacy context. The emulator advances the cursor by that width and stores a NUL filler in the second cell of a wide character, never splits one across the right margin, blanks the other half when either half is overwritten and counts insert mode in cells. The renderer skips the fillers so a fixed width font draws a wide glyph over both cells, falls back to placing each character at its own cell when the font does not advance exactly one cell per column, and draws a character beyond the BMP whole. A partial repaint that starts on the second cell of a wide character is widened to its first, and copying drops the fillers. Tests cover the width table, placement, the margin, overwriting halves and insert mode.
A supplementary character is stored as its two surrogates, one per cell, so it covers both of its cells itself. The null after it is an ordinary empty cell, not a filler, and copying must keep it as a space. A repaint that starts on the low surrogate is widened to the high one, like a repaint starting on a filler. The proportional font path draws the surrogate pair as one string instead of one half at a time. Found by review of eclipse-platform#2894.
|
I would like to be able to test this myself before going on further. Do you think you can give me clear instruction how to test and see the effect of this change? |
|
Thanks for looking at it! Here is a way to see the effect. Setup Run an Eclipse application with 1. Column alignment: every Without the change the 2. Cursor movement over wide characters: move the cursor 4 cells to the right from the line start and overwrite: Expected output is 3. Line wrap: this wraps at the right edge, after half as many characters as the terminal has columns: Without the change the line does not wrap: each character is drawn one cell after the previous one, so they overlap and only about half of them can be read. 4. Interactive: type The model tests in the PR ( Before and after, with the three commands above (Linux, D2Coding 11pt): |
Insert mode made room with CharWidth.ofString, while the write path puts every printable surrogate pair in two cells, so inserting a narrow character beyond the BMP (U+1D400) overwrote the next character. Both now use the same cell width. Writing over one cell of a surrogate pair also blanks the other half, as for a wide character.
The terminal emulator assumes every character occupies one cell. Hangul, Han and Kana text, fullwidth forms and emoji are East Asian Wide and occupy two, so every such character put the cursor one column short and the screen fell apart as soon as a program laid text out for a real terminal — line editors, curses UIs, Ink-based CLIs.
This adds
CharWidth, a UAX #11 East Asian Width lookup (Wide/Fullwidth = 2, combining marks and controls = 0, Ambiguous = 1 as UAX #11 recommends outside an East Asian legacy context), and makes the emulator and renderer follow it:Tests:
CharWidthTest(width table) and four cases inVT100EmulatorBackendTest(placement, margin, overwriting halves, insert mode). All existing terminal tests pass.Verified on Windows 11 with a Korean shell session, vim, htop and Claude Code inside the Eclipse Terminal; compared against Windows Terminal for column alignment.