Repository navigation
Conversation
66aa868 to
4a0de20
Compare
4a0de20 to
2676b31
Compare
2676b31 to
30d40d1
Compare
Test Results 54 files ± 0 54 suites ±0 40m 23s ⏱️ + 6m 4s Results for commit 0f0b239. ± Comparison against base commit 63d22c8. This pull request removes 2 and adds 26 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
Rebased on master, now that #2894 is in: one commit, 30d40d1, the earlier commits squashed; all terminal tests pass. The writing loop combines this PR's handling of marks and clusters with #2894's final placement (room made run by run in insert mode, the margin cell erased on an early wrap); a mark or cluster part that goes onto the character before it makes no room. How to test, in a runtime Eclipse with this org.eclipse.terminal.control, Terminal view, local shell:
Unit tests: testCombiningMarks and testGraphemeClusters in VT100EmulatorBackendTest; the cluster bookkeeping (overwrite, scroll, clean, copy, window) in AbstractITerminalTextDataTest for every data implementation. |
9f6edf5 to
7a7a5cd
Compare
7a7a5cd to
93754ea
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved grapheme-cluster defects can lose or corrupt text during composition, editing and copying, while also mishandling cursor adjacency and right-margin wrapping.
9 open issues
Narrow-base combining marks and keycaps are dropped · New Cursor movement leaves stale ZWJ join state · New VS16 promotion overwrites text in insert mode · New Promoted heart fillers are missed by wide-character checks · New VS16 promotion at the margin clears pending wrap · New Combining marks discard earlier retained marks · New Character shifts discard cluster metadata · New Self-copy clears cluster metadata before reading it · New Selections omit promoted heart clusters · New
What changed in this PR
Extends the terminal’s two-cell character support from #2894 with combining-mark composition and grapheme-cluster storage, aiming to preserve column alignment and copied text.
Changes:
- Adds NFC composition and cluster handling for joined emoji, modifiers, presentation selectors and flags.
- Carries cluster metadata through terminal data implementations, snapshots and selection copying.
- Adds emulator and model tests, plus public cluster APIs and a bundle version bump.
| File | Description |
|---|---|
| terminal/tests/org.eclipse.terminal.test/src/org/eclipse/terminal/internal/model/TerminalTextDataWindowTest.java | Tests window-relative cluster access and invalidation. |
| terminal/tests/org.eclipse.terminal.test/src/org/eclipse/terminal/internal/model/AbstractITerminalTextDataTest.java | Tests cluster overwrite, scrolling, cleaning and copying. |
| terminal/tests/org.eclipse.terminal.test/src/org/eclipse/terminal/internal/emulator/VT100EmulatorBackendTest.java | Tests composition, emoji clusters and deferred wrapping. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/model/ITerminalTextDataReadOnly.java | Adds the default cluster-read API. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/model/ITerminalTextData.java | Adds the default cluster-write API. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/textcanvas/AbstractTextCanvasModel.java | Includes stored clusters in copied selections. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/TerminalTextDataWindow.java | Translates cluster access into window coordinates. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/TerminalTextDataStore.java | Stores, invalidates and transfers cluster metadata. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/TerminalTextDataSnapshot.java | Exposes snapshot cluster data. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/TerminalTextDataFastScroll.java | Maps cluster access through scrolling offsets. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/TerminalTextData.java | Delegates cluster access and notifies snapshots. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/model/SynchronizedTerminalTextData.java | Synchronizes cluster reads and writes. |
| terminal/bundles/org.eclipse.terminal.control/src/org/eclipse/terminal/internal/emulator/VT100EmulatorBackend.java | Implements composition, joining and two-cell promotion. |
| terminal/bundles/org.eclipse.terminal.control/META-INF/MANIFEST.MF | Increments bundle and model-package versions for the APIs. |
Review effort: Balanced
93754ea to
304dd72
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Active defects cause grapheme data loss across input boundaries and wrapping, corrupt cluster placement during scrolling, and break hover reconstruction after resizing or across empty cells.
6 open issues
Update cluster cells before advancing and scrolling the buffer · New Preserve zero-width prefixes across append calls · New Keep grapheme continuations attached across immediate wraps · New Clip hover line text to the current terminal width after resize · New Preserve empty-cell boundaries after wide-character clusters · New Character shifts discard cluster metadata
8 resolved since last review
Selections omit promoted heart clusters Self-copy clears cluster metadata before reading it Combining marks discard earlier retained marks VS16 promotion at the margin clears pending wrap Promoted heart fillers are missed by wide-character checks VS16 promotion overwrites text in insert mode Cursor movement leaves stale ZWJ join state Narrow-base combining marks and keycaps are dropped
Review effort: Balanced
304dd72 to
c3aef36
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved combining-cluster bugs can corrupt cell contents, trigger a cursor-rendering exception in one-column terminals, and prevent composition with characters moved into scrollback.
5 open issues
Use the absolute buffer row when locating the previous character · New Reject clusters wider than a one-column terminal · New Choose the first cell-bearing code point as the normalized base · New Restore filler after a cluster normalizes to one UTF-16 character · New Character shifts discard cluster metadata
5 resolved since last review
Review effort: Balanced
|
I owe you an apology for the last few rounds on this PR. Each time Copilot found an edge case I patched that one place instead of asking why it was possible. The cause is in the structure: whether a cell is the second half of a character is decided in about ten places (emulator, text store, selection, hover, copy, canvas, renderer, cursor), each with its own conditions, and every patch added another. That is why every review turned up new holes, and it cost you review time and notifications. I'm sorry. I'm rewriting the cluster support instead of patching it further:
I'll push it as one commit when it passes all of that, and not before. Until then there is no need to look at this PR or request another Copilot review; the open Copilot comments will be answered by the rewrite. |
c3aef36 to
6240c74
Compare
|
The rewrite is pushed as one commit, eb42d69. How many cells a character takes is now kept by the text data when the character is written (TerminalTextDataStore, read through getCellWidth/getCluster/isSimpleLine), and everything that reads cells goes through it; the scattered checks are gone. Cluster boundaries follow UAX #29 (BreakIterator), widths follow Windows Terminal, and nothing the program writes is dropped. Besides the unit tests there is a randomized test that mixes writes, cursor moves, erases, insert/delete, scrolling, resizing and the alternate screen and checks the cells, selection, copy and drawn text after every step. Writing and drawing CJK text is about 1.8 times as fast as on master; ASCII is written up to about 12 percent slower. I have answered the open Copilot comments; each is covered by the rewrite. |
f2e115d to
4e8c737
Compare
4e8c737 to
eb42d69
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved cluster-handling defects discard retained off-screen layout during line copying, reuse stale continuation state after cursor movement, and cause hover selection to cross visible gaps.
3 open issues
5 resolved since last review
Review effort: Balanced
| // read before writing: the source may be this very line | ||
| Layout layout = null; | ||
| boolean simple = source.isSimpleLine(sourceLine); | ||
| for (int col = 0; !simple && chars != null && col < chars.length && col < source.getWidth(); col++) { |
| if (fLastText == null || fCursorLine != fAfterLine || fCursorColumn != fAfterColumn | ||
| || fWrapPending != fAfterPending || fLastColumn >= fColumns) { | ||
| return -1; |
| return true; | ||
| } | ||
| return scrubbed.toString(); | ||
| char c = text.charAt(0); |
A combining mark has no cell of its own. It was dropped, which lost data: an accent typed as a separate mark, a Hangul syllable sent as conjoining jamo, a Japanese voicing mark. More than marks go with the character before them: a zero width joiner and what it joins, a skin tone, a presentation selector, the second regional indicator of a flag. What a reader takes for one character is a grapheme cluster, and where one ends is now decided by the Unicode rules (UAX eclipse-platform#29) through java.text.BreakIterator, over the text as it is written, the cluster written last going first, so a cluster goes on across writes but not across a cursor move. A cluster takes the cells its code points would, at least one and at most two, summed as Windows Terminal sums them: a mark takes none, VS16 makes it wide. Grown wide on the last cell it moves whole to the next line, as a wide character does; in insert mode its second cell is made room for. A cluster that starts with a code point taking no cell goes onto the one before it, or takes a cell of its own when there is none. Left out, before the clusters are found, are control characters, the format characters no cluster needs (a zero width space, bidirectional marks, overrides and isolates), and what only hides text: tag characters outside the tag sequence of a flag (UTS eclipse-platform#51), a variation selector that does not follow a character, a second prefix sign. None is drawn, and in copied text they could make a command read differently from how it runs or carry text no one saw. A cluster keeps at most 32 chars (the longest emoji sequence is 15), so a flood of marks cannot grow one cell without bound. How many cells each character takes was worked out again, from the chars around it, wherever it was needed: in the emulator, the selection, copying, the word under the pointer, the canvas and the renderer, each with its own conditions. The text data now keeps it. TerminalTextDataStore records, per line, the cells that are the second of a character (written with it in one call: a wide character and its filler, a surrogate pair, or a cluster made wide) and the clusters beside them; lines of one-cell characters keep nothing extra. Reading it is a lookup (getCellWidth, getCluster, isSimpleLine, new default methods of ITerminalTextDataReadOnly, hence the version bump), and everything that reads cells goes through it and through Cells, which puts characters back together; the old inference in CharWidth is gone. The emulator writes every character through one method, moves characters whole when cells are inserted or deleted, and keeps one record of the cluster written last. The cells hold what is drawn, the character composed with its marks (NFC) where it has such a form; the cluster is kept as the program wrote it, and it is what is drawn over its cells (a flag, a family, a keycap as one) and what selecting and copying give. Text of one-cell characters is drawn as before, from the line's chars. Runs of wide characters are written in one go, the boundary rules are consulted only for characters they could join (found once per character from BreakIterator itself), and character widths of the BMP are looked up. Writing CJK text is about 1.8 times as fast as before, and drawing it faster too; other text is drawn at least as fast and written up to about 12 percent slower, the cost of finding clusters. Tests: CellsTest, and a randomized test that mixes writes (split at any point), cursor moves, erases, insertions, deletions, scrolling, resizing and the alternate screen, checking after every step that the cells, the selection, the copy and the drawn text agree, and that written text is copied back unchanged.
eb42d69 to
0f0b239
Compare


Depends on #2894 (East Asian Width): builds on
CharWidthand the two-cell placement of wide characters. This PR shows that commit as well until #2894 is merged; only the second commit is new here.A combining mark has no cell of its own. It was dropped, which lost data: an accent typed as a separate mark, a Hangul syllable sent as conjoining jamo, a Japanese voicing mark. A mark now composes with the character before it (NFC) whenever the two have a single form, which covers all of those.
What has no composed form is a grapheme cluster: an emoji joined to others with a zero-width joiner, one with a skin tone, a character with a presentation selector, a keycap, a flag made of two regional indicators. Windows Terminal (1.22+) and the libraries programs use to lay text out give such a cluster two cells however many characters it runs to, so the emulator has to count the same way or the columns drift.
API:
ITerminalTextDataReadOnly.getCluster/ITerminalTextData.setCluster(default methods,@since 1.2, bundle version 1.2.0).Tests: combining marks and clusters in
VT100EmulatorBackendTest; cluster bookkeeping (overwrite, scroll, clean, copy, window) inAbstractITerminalTextDataTestfor every data implementation.