fix: grapheme-cluster-aware width with per-category emoji detection (#1753)#1754
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds optional JDK‑21+ BreakIterator grapheme‑cluster segmentation for width/counting (including null‑terminal paths), terminal‑side emoji grouping detection with a CPR cursor‑position fallback probe, a forceable grapheme‑mode API, builder support for force/skip probe semantics, and expanded tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant WCWidth
participant BreakIterator
participant Terminal
participant AbstractTerminal
App->>WCWidth: wcwidthForDisplay(text, idx, terminal)
alt terminal != null and terminal.getGraphemeClusterMode()
WCWidth->>Terminal: isClusterGrouped(cs, idx, charCount)
WCWidth->>BreakIterator: getCharacterInstance() (JDK21+)
BreakIterator-->>WCWidth: grapheme clusters
WCWidth->>WCWidth: classify cluster (emoji/ideograph/grouping)
WCWidth-->>App: return width and advance
else terminal == null and HAS_JDK_GRAPHEME_SUPPORT
WCWidth->>BreakIterator: getCharacterInstance() (JDK21+)
BreakIterator-->>WCWidth: grapheme clusters
WCWidth->>WCWidth: classify cluster (emoji/ideograph)
WCWidth-->>App: return width and advance
else
WCWidth->>WCWidth: fallback per‑codepoint wcwidth sum
WCWidth-->>App: return width and advance
end
Note over AbstractTerminal,Terminal: Mode 2027 probe with CPR cursor fallback
App->>AbstractTerminal: setGraphemeClusterMode(enable, force)
AbstractTerminal->>AbstractTerminal: probeMode2027()
alt Mode2027 SUPPORTED
AbstractTerminal->>AbstractTerminal: enable Mode2027, set state
else Mode2027 NOT_SUPPORTED
AbstractTerminal->>Terminal: probeCursorPosition() (emit emojis + DSR)
Terminal-->>AbstractTerminal: CPR response (parsed column)
AbstractTerminal->>AbstractTerminal: infer grouping, set native & flags
else NO_RESPONSE
AbstractTerminal->>AbstractTerminal: probe fails (no CPR fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.java (1)
234-259: Remove unnecessarypublicmodifiers from test methods.JUnit 5 allows package-private test methods; the
publicmodifier is unnecessary. SonarCloud flagged this.♻️ Proposed fix
- `@Test` - public void testColumnSubSequenceNoTerminal() { + `@Test` + void testColumnSubSequenceNoTerminal() {- `@Test` - public void testColumnSplitLengthNoTerminal() { + `@Test` + void testColumnSplitLengthNoTerminal() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.java` around lines 234 - 259, Remove the unnecessary public modifiers from the two JUnit test methods in AttributedCharSequenceTest: change the signatures of testColumnSubSequenceNoTerminal and testColumnSplitLengthNoTerminal to package-private (no access modifier); keep all logic intact (including references to WCWidth.HAS_JDK_GRAPHEME_SUPPORT, FAMILY_EMOJI, and use of AttributedString and columnSubSequence/columnSplitLength) so only the method access modifiers are updated to satisfy SonarCloud.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.java`:
- Around line 234-259: Remove the unnecessary public modifiers from the two
JUnit test methods in AttributedCharSequenceTest: change the signatures of
testColumnSubSequenceNoTerminal and testColumnSplitLengthNoTerminal to
package-private (no access modifier); keep all logic intact (including
references to WCWidth.HAS_JDK_GRAPHEME_SUPPORT, FAMILY_EMOJI, and use of
AttributedString and columnSubSequence/columnSplitLength) so only the method
access modifiers are updated to satisfy SonarCloud.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc840e1a-199a-4663-8221-694275ccc0c1
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.javaterminal/src/test/java/org/jline/utils/WCWidthTest.java
gnodet
force-pushed
the
ci-issue-1753
branch
2 times, most recently
from
1cf552e to
7acfad7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.java (1)
232-255: Consider using JUnit 5 assumptions instead of early return.The early
returnpattern works, but JUnit 5'sAssumptions.assumeTrue()would be more idiomatic and would properly mark tests as "skipped" rather than silently passing when the condition isn't met.Regarding SonarCloud's hint about removing
publicmodifiers: the existing tests in this file consistently usepublic, so keeping them maintains file-level consistency. Either approach is acceptable.♻️ Optional: Use JUnit 5 assumptions
+import static org.junit.jupiter.api.Assumptions.assumeTrue; + `@Test` -public void testColumnSubSequenceNoTerminal() { - if (!WCWidth.HAS_JDK_GRAPHEME_SUPPORT) return; - +void testColumnSubSequenceNoTerminal() { + assumeTrue(WCWidth.HAS_JDK_GRAPHEME_SUPPORT, "Requires JDK 21+ grapheme support"); // JDK 21+: grapheme cluster-aware without terminal String text = "AB" + FAMILY_EMOJI + "CD"; ... } `@Test` -public void testColumnSplitLengthNoTerminal() { - if (!WCWidth.HAS_JDK_GRAPHEME_SUPPORT) return; - +void testColumnSplitLengthNoTerminal() { + assumeTrue(WCWidth.HAS_JDK_GRAPHEME_SUPPORT, "Requires JDK 21+ grapheme support"); // JDK 21+: grapheme cluster-aware without terminal ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.java` around lines 232 - 255, Replace the early-return guards in testColumnSubSequenceNoTerminal and testColumnSplitLengthNoTerminal with JUnit 5 assumptions: call Assumptions.assumeTrue(WCWidth.HAS_JDK_GRAPHEME_SUPPORT) at the start of each test so the test is marked skipped when grapheme support is absent; leave the rest of the logic (creation of AttributedString and assertions) unchanged and retain the existing public test methods for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.java`:
- Around line 232-255: Replace the early-return guards in
testColumnSubSequenceNoTerminal and testColumnSplitLengthNoTerminal with JUnit 5
assumptions: call Assumptions.assumeTrue(WCWidth.HAS_JDK_GRAPHEME_SUPPORT) at
the start of each test so the test is marked skipped when grapheme support is
absent; leave the rest of the logic (creation of AttributedString and
assertions) unchanged and retain the existing public test methods for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7e3e4f1-4c50-422c-bd30-600c4718233e
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.javaterminal/src/test/java/org/jline/utils/WCWidthTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/src/test/java/org/jline/utils/WCWidthTest.java
- terminal/src/main/java/org/jline/utils/WCWidth.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
403-420:⚠️ Potential issue | 🟠 MajorKeep the new CPR fallback reachable on Terminal.app.
Lines 408-410 return before any probe runs, so
Apple_Terminalnever reachesprobeCursorPosition(). That avoids the DECRQMpleak, but it also forcessupportsGraphemeClusterMode()to stayfalseon that terminal even though the new fallback path does not use the$-intermediate sequence at all. Move this special-case intoprobeMode2027()or otherwise keep the DSR probe reachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 403 - 420, The early return for Apple_Terminal prevents the DSR CPR fallback from running; remove the "if (\"Apple_Terminal\".equals(termProgram)) return false;" from this block so probeMode2027() and probeCursorPosition() still execute, and instead handle the Terminal.app quirk inside probeMode2027() (or modify probeMode2027() to check System.getenv("TERM_PROGRAM") and avoid sending the DECRQM/`$`-intermediate sequence there) so the Apple_Terminal path uses the new CPR fallback without triggering the DECRQM 'p' leak; update references in probeMode2027() and any related methods (probeCursorPosition(), enterRawMode(), setAttributes(Attributes)) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 495-516: The probeCursorPosition() path can leave a late CPR reply
in the input when readCursorPosition(timeout) times out; update
probeCursorPosition() to always perform a final input drain/fence after the DSR
fallback path (whether readCursorPosition() returns a column or times out)
before returning false or setting graphemeCluster flags: after the initial read
attempt, ensure you consume any pending CPR sequences (or flush the input
buffer) so that an ESC[row;colR cannot leak into subsequent application reads;
reference probeCursorPosition(), readCursorPosition(), and the cleanup
writer().write("\r\033[K\0338") sequence and make the drain happen on the
timeout branch as well as the success branch.
---
Outside diff comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 403-420: The early return for Apple_Terminal prevents the DSR CPR
fallback from running; remove the "if (\"Apple_Terminal\".equals(termProgram))
return false;" from this block so probeMode2027() and probeCursorPosition()
still execute, and instead handle the Terminal.app quirk inside probeMode2027()
(or modify probeMode2027() to check System.getenv("TERM_PROGRAM") and avoid
sending the DECRQM/`$`-intermediate sequence there) so the Apple_Terminal path
uses the new CPR fallback without triggering the DECRQM 'p' leak; update
references in probeMode2027() and any related methods (probeCursorPosition(),
enterRawMode(), setAttributes(Attributes)) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 421c925a-df45-48f1-80af-d455113d70d6
📒 Files selected for processing (2)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
521-525: Add rationale for the intentionally ignored cleanup exception.At Line 524, the empty catch makes failure intent unclear and triggers static-analysis noise. A short “best-effort cleanup” comment is enough.
Suggested minimal tweak
} catch (IOException e) { try { writer().write("\r\033[K\0338"); writer().flush(); } catch (Exception ignored) { + // Best-effort cleanup only: probing failure should not propagate. } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 521 - 525, The empty catch in AbstractTerminal around the writer().write(...) / writer().flush() cleanup should include a brief comment explaining this is a best-effort cleanup and that any exception is intentionally ignored (to avoid masking real errors elsewhere and to silence static-analysis noise); update the catch block in the AbstractTerminal class (the try that calls writer().write("\r\033[K\0338") and writer().flush()) to keep the ignore but add a one-line rationale comment like "best-effort cleanup — ignore exceptions" referencing the same writer() calls so future readers and linters understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 521-525: The empty catch in AbstractTerminal around the
writer().write(...) / writer().flush() cleanup should include a brief comment
explaining this is a best-effort cleanup and that any exception is intentionally
ignored (to avoid masking real errors elsewhere and to silence static-analysis
noise); update the catch block in the AbstractTerminal class (the try that calls
writer().write("\r\033[K\0338") and writer().flush()) to keep the ignore but add
a one-line rationale comment like "best-effort cleanup — ignore exceptions"
referencing the same writer() calls so future readers and linters understand the
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e85a405c-7fda-4896-8e6d-7e5a960ebeec
📒 Files selected for processing (2)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
gnodet
changed the title
gnodet
force-pushed
the
ci-issue-1753
branch
5 times, most recently
from
4d14d35 to
9f9a546
Compare
gnodet
changed the title
Contributor
|
Note Docstrings generation - SUCCESS |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java (1)
642-653:⚠️ Potential issue | 🟡 MinorMake the EOF responder fail the test on timeout.
Lines 647-650 swallow every exception from
waitForOutput(...). If\033[6nis never emitted, the background thread times out and this test still passes without exercising the EOF path. Feed that failure intoResponderHandlethe same way the shared helpers do.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java` around lines 642 - 653, The background thread swallowing exceptions hides timeouts from waitForOutput; update the Thread passed to ResponderHandle so its catch block stores the caught Throwable into the provided AtomicReference instead of ignoring it (i.e., call the same atomicRef.set(e) behavior used by the shared helpers), using the same AtomicReference instance you pass into the ResponderHandle constructor so the test harness can observe and fail on timeouts/failures from waitForOutput (references: ResponderHandle, waitForOutput, terminal.slaveInputPipe.close).terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
541-599:⚠️ Potential issue | 🟠 MajorDrain pending CPR replies before leaving
probeCursorPosition().If one of the batched
readCprColumn()calls times out, this method restores the cursor and returns without any final input fence. A CPR that arrives just after the last read stays buffered asESC[row;colRand can leak into the next application read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 541 - 599, probeCursorPosition currently can return with outstanding CPR bytes buffered if a readCprColumn call times out; before restoring state or returning (both in normal and exception paths around puts(Capability.restore_cursor)/writer().flush()), drain any pending CPR responses by repeatedly calling readCprColumn with a short non-blocking timeout (or otherwise consuming input until no more ESC '[' ... 'R' sequences are available) to flush leftover ESC[row;colR tokens; then proceed to set emojiGroupings/graphemeClusterNative as before and return. Target the probeCursorPosition method and the places that call puts(Capability.restore_cursor)/writer().flush() so cleanup always includes the CPR-drain step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 398-404: The method getEmojiGroupings currently returns the live
emojiGroupings field allowing external mutation that affects width logic (e.g.,
WCWidth.isGroupedByTerminal()); change it to return a defensive copy or an
unmodifiable view instead of the raw set — locate getEmojiGroupings() and
replace the direct return of emojiGroupings with either
Collections.unmodifiableSet(new EnumSet.copyOf(emojiGroupings)) or
EnumSet.copyOf(emojiGroupings) so callers cannot mutate the internal
emojiGroupings state.
In `@terminal/src/main/java/org/jline/terminal/TerminalBuilder.java`:
- Around line 753-769: The grapheme-cluster handling must respect the existing
terminal state and the success/failure of setGraphemeClusterMode: for the gc ==
FALSE branch, attempt to disable grapheme mode by calling
terminal.setGraphemeClusterMode(false) and if that returns false (or is
unsupported) log that it remained enabled; for the TRUE and auto-detect
branches, check the boolean result returned by
terminal.setGraphemeClusterMode(...) and only log "enabled" when it actually
returned true, otherwise log that enabling failed; also if build() returned a
preconfigured terminal (e.g., TERMINAL_OVERRIDE or any Terminal already
reporting grapheme mode via terminal.isGraphemeClusterModeEnabled()), skip
probing/forcing and log the current state accordingly so configuration is
honored.
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 677-710: The classifyCluster method currently returns
EmojiGrouping.ZWJ_SEQUENCE whenever U+200D is present, which misclassifies
non-emoji ZWJ sequences; change the ZWJ handling so you only return ZWJ_SEQUENCE
if the cluster actually contains emoji characters (for example, an emoji base or
a VS16 presentation selector). In practice, inside classifyCluster (and where
the code checks ncp == 0x200D), scan the cluster for an emoji indicator (use
existing helpers like isSkinToneModifier/isRegionalIndicator or add/use an
isEmojiBase/isEmoji property check or require U+FE0F) and only return
EmojiGrouping.ZWJ_SEQUENCE when that emoji guard succeeds; otherwise continue
scanning and ultimately return null for non-emoji ZWJ clusters.
---
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 541-599: probeCursorPosition currently can return with outstanding
CPR bytes buffered if a readCprColumn call times out; before restoring state or
returning (both in normal and exception paths around
puts(Capability.restore_cursor)/writer().flush()), drain any pending CPR
responses by repeatedly calling readCprColumn with a short non-blocking timeout
(or otherwise consuming input until no more ESC '[' ... 'R' sequences are
available) to flush leftover ESC[row;colR tokens; then proceed to set
emojiGroupings/graphemeClusterNative as before and return. Target the
probeCursorPosition method and the places that call
puts(Capability.restore_cursor)/writer().flush() so cleanup always includes the
CPR-drain step.
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`:
- Around line 642-653: The background thread swallowing exceptions hides
timeouts from waitForOutput; update the Thread passed to ResponderHandle so its
catch block stores the caught Throwable into the provided AtomicReference
instead of ignoring it (i.e., call the same atomicRef.set(e) behavior used by
the shared helpers), using the same AtomicReference instance you pass into the
ResponderHandle constructor so the test harness can observe and fail on
timeouts/failures from waitForOutput (references: ResponderHandle,
waitForOutput, terminal.slaveInputPipe.close).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: baf7ad3c-f4d1-43ab-95e3-06629b1c146b
📒 Files selected for processing (7)
terminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/TerminalBuilder.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.javaterminal/src/test/java/org/jline/utils/WCWidthTest.java
Comment on lines
+753
to
+769
| // Enable grapheme cluster mode if supported | ||
| Boolean gc = this.graphemeCluster; | ||
| if (gc == null) { | ||
| gc = getBoolean(PROP_GRAPHEME_CLUSTER, null); | ||
| } | ||
| if (!Boolean.FALSE.equals(gc) && terminal.supportsGraphemeClusterMode()) { | ||
| if (Boolean.TRUE.equals(gc)) { | ||
| // Force-enable: skip probing, treat as native grapheme support | ||
| terminal.setGraphemeClusterMode(true, true); | ||
| Log.debug(() -> "Grapheme cluster mode: force-enabled (skipping probe)"); | ||
| } else if (Boolean.FALSE.equals(gc)) { | ||
| Log.debug(() -> "Grapheme cluster mode: disabled by configuration"); | ||
| } else if (terminal.supportsGraphemeClusterMode()) { | ||
| // Auto-detect: probe terminal and enable if supported | ||
| terminal.setGraphemeClusterMode(true); | ||
| Log.debug(() -> "Grapheme cluster mode: enabled (auto-detected)"); | ||
| } else { | ||
| Log.debug(() -> "Grapheme cluster mode: not supported by terminal"); |
Contributor
There was a problem hiding this comment.
Honor the configured grapheme mode on preconfigured terminals.
This block only logs the requested state. If build() returns TERMINAL_OVERRIDE or any Terminal that is already in grapheme mode, the FALSE branch leaves it enabled, and the TRUE/auto branches still log success even when setGraphemeClusterMode(...) returns false.
Suggested fix
- if (Boolean.TRUE.equals(gc)) {
- // Force-enable: skip probing, treat as native grapheme support
- terminal.setGraphemeClusterMode(true, true);
- Log.debug(() -> "Grapheme cluster mode: force-enabled (skipping probe)");
- } else if (Boolean.FALSE.equals(gc)) {
- Log.debug(() -> "Grapheme cluster mode: disabled by configuration");
- } else if (terminal.supportsGraphemeClusterMode()) {
- // Auto-detect: probe terminal and enable if supported
- terminal.setGraphemeClusterMode(true);
- Log.debug(() -> "Grapheme cluster mode: enabled (auto-detected)");
+ if (Boolean.TRUE.equals(gc)) {
+ boolean enabled = terminal.setGraphemeClusterMode(true, true);
+ Log.debug(() -> enabled
+ ? "Grapheme cluster mode: force-enabled (skipping probe)"
+ : "Grapheme cluster mode: force-enable failed");
+ } else if (Boolean.FALSE.equals(gc)) {
+ if (terminal.getGraphemeClusterMode()) {
+ terminal.setGraphemeClusterMode(false);
+ }
+ Log.debug(() -> "Grapheme cluster mode: disabled by configuration");
+ } else if (terminal.supportsGraphemeClusterMode()) {
+ boolean enabled = terminal.setGraphemeClusterMode(true);
+ Log.debug(() -> enabled
+ ? "Grapheme cluster mode: enabled (auto-detected)"
+ : "Grapheme cluster mode: enable failed after support probe");
} else {
Log.debug(() -> "Grapheme cluster mode: not supported by terminal");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/src/main/java/org/jline/terminal/TerminalBuilder.java` around lines
753 - 769, The grapheme-cluster handling must respect the existing terminal
state and the success/failure of setGraphemeClusterMode: for the gc == FALSE
branch, attempt to disable grapheme mode by calling
terminal.setGraphemeClusterMode(false) and if that returns false (or is
unsupported) log that it remained enabled; for the TRUE and auto-detect
branches, check the boolean result returned by
terminal.setGraphemeClusterMode(...) and only log "enabled" when it actually
returned true, otherwise log that enabling failed; also if build() returned a
preconfigured terminal (e.g., TERMINAL_OVERRIDE or any Terminal already
reporting grapheme mode via terminal.isGraphemeClusterModeEnabled()), skip
probing/forcing and log the current state accordingly so configuration is
honored.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
terminal/src/main/java/org/jline/utils/WCWidth.java (1)
695-724:⚠️ Potential issue | 🟡 MinorNon-emoji ZWJ clusters are still misclassified.
The ZWJ handling at line 711-712 returns
ZWJ_SEQUENCEfor any multi-codepoint cluster containing U+200D, without verifying the cluster actually contains emoji characters. This misclassifies non-emoji ZWJ sequences (e.g., Indic consonant + virama + ZWJ + consonant clusters used for font shaping), which then flow intowcwidthUngrouped()and may produce incorrect width calculations.Consider adding an emoji guard before returning
ZWJ_SEQUENCE:🔧 Suggested approach
if (ncp == 0x200D) { - return EmojiGrouping.ZWJ_SEQUENCE; + // Only classify as ZWJ_SEQUENCE if the cluster contains emoji + // (either the base codepoint or a previous VS16 selector) + if (result == EmojiGrouping.VS16_PRESENTATION || isEmojiBase(cp)) { + return EmojiGrouping.ZWJ_SEQUENCE; + } + // Non-emoji ZWJ (e.g., Indic shaping) — continue scanning }This requires an
isEmojiBase()helper or checking if the first codepoint is in an emoji range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/utils/WCWidth.java` around lines 695 - 724, The ZWJ handling in classifyCluster currently returns EmojiGrouping.ZWJ_SEQUENCE whenever U+200D appears in the cluster, which misclassifies non-emoji ZWJ sequences; update classifyCluster to verify the cluster actually contains emoji before returning ZWJ_SEQUENCE by adding an emoji guard (e.g., call a new isEmojiBase(int cp) or analogous check against known emoji ranges on the initial codepoint cp or scanning codepoints) and only return EmojiGrouping.ZWJ_SEQUENCE when that guard is true; implement an isEmojiBase helper (or inline range checks) and use it in the ZWJ branch inside classifyCluster to avoid misclassifying Indic/ligature ZWJ sequences.terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
406-413:⚠️ Potential issue | 🟠 MajorReturn a defensive copy to prevent external mutation.
The method returns the live
emojiGroupingsset. External callers can mutate it and silently change width calculation behavior inWCWidth.isGroupedByTerminal().🛡️ Suggested fix
`@Override` public Set<EmojiGrouping> getEmojiGroupings() { if (emojiGroupings != null) { - return emojiGroupings; + return EnumSet.copyOf(emojiGroupings); } return getGraphemeClusterMode() ? EnumSet.allOf(EmojiGrouping.class) : EnumSet.noneOf(EmojiGrouping.class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 406 - 413, The method getEmojiGroupings() currently returns the live emojiGroupings set which allows external mutation; change it to return a defensive copy when emojiGroupings is non-null (e.g., return EnumSet.copyOf(emojiGroupings)) so callers cannot mutate the terminal's internal set and affect WCWidth.isGroupedByTerminal(); keep the existing behavior for the null branch that constructs new EnumSet instances.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
617-635: Consider validating digit characters in column parsing.At line 632, the column parsing assumes all characters before 'R' are digits:
col = col * 10 + (c - '0'). If a malformed CPR response contains non-digit characters, this produces incorrect values rather than returning -1.🛡️ Optional defensive fix
while ((c = reader().read(timeout)) != 'R') { if (c < 0) return -1; + if (c < '0' || c > '9') return -1; col = col * 10 + (c - '0'); }In practice, terminal CPR responses are well-formed, so this is a minor robustness improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 617 - 635, The readCprColumn method currently assumes every character read before 'R' is a digit and does col = col * 10 + (c - '0'), which yields wrong results on malformed CPR responses; modify readCprColumn to validate each character returned by reader().read(timeout) (e.g., check c is between '0' and '9' or use Character.isDigit) and if a non-digit (and not the terminating 'R') is encountered return -1 instead of using it in the numeric parsing, keeping the rest of the ESC '[' and ';' handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 406-413: The method getEmojiGroupings() currently returns the live
emojiGroupings set which allows external mutation; change it to return a
defensive copy when emojiGroupings is non-null (e.g., return
EnumSet.copyOf(emojiGroupings)) so callers cannot mutate the terminal's internal
set and affect WCWidth.isGroupedByTerminal(); keep the existing behavior for the
null branch that constructs new EnumSet instances.
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 695-724: The ZWJ handling in classifyCluster currently returns
EmojiGrouping.ZWJ_SEQUENCE whenever U+200D appears in the cluster, which
misclassifies non-emoji ZWJ sequences; update classifyCluster to verify the
cluster actually contains emoji before returning ZWJ_SEQUENCE by adding an emoji
guard (e.g., call a new isEmojiBase(int cp) or analogous check against known
emoji ranges on the initial codepoint cp or scanning codepoints) and only return
EmojiGrouping.ZWJ_SEQUENCE when that guard is true; implement an isEmojiBase
helper (or inline range checks) and use it in the ZWJ branch inside
classifyCluster to avoid misclassifying Indic/ligature ZWJ sequences.
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 617-635: The readCprColumn method currently assumes every
character read before 'R' is a digit and does col = col * 10 + (c - '0'), which
yields wrong results on malformed CPR responses; modify readCprColumn to
validate each character returned by reader().read(timeout) (e.g., check c is
between '0' and '9' or use Character.isDigit) and if a non-digit (and not the
terminating 'R') is encountered return -1 instead of using it in the numeric
parsing, keeping the rest of the ESC '[' and ';' handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e53da316-a296-442d-8c0d-742254bb8d4c
📒 Files selected for processing (4)
terminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/TerminalBuilder.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/main/java/org/jline/terminal/TerminalBuilder.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 582-586: When classifyCluster returns a non-null EmojiGrouping
(e.g., ZWJ_SEQUENCE) but the terminal doesn't support that grouping, don't call
wcwidthUngrouped(cs, index, charCount) which sums raw codepoints; instead pass
the terminal's grouping set into the fallback and split the cluster into
sub-runs, reapplying category-aware width logic so supported subcomponents
(e.g., SKIN_TONE_MODIFIER, VS16_PRESENTATION) are counted as grouped when the
terminal supports them; update the code paths that handle this fallback
(references: EmojiGrouping, classifyCluster, wcwidthUngrouped, ZWJ_SEQUENCE,
SKIN_TONE_MODIFIER, VS16_PRESENTATION) to iterate the cluster, detect supported
sub-runs and sum their widths with existing per-category handlers rather than
summing individual codepoints.
- Around line 518-520: The JDK 21 branch currently reuses legacy grapheme
functions (charCountForGraphemeCluster and wcwidthForDisplayWithGroupings) and
must instead route through a new JDK21-specific grapheme helper; add a method
(e.g., charCountForGraphemeClusterJdk21 or charCountForBreakIterator) that uses
BreakIterator.getCharacterInstance() and Character property checks
(Character.getType(), etc.) to compute grapheme cluster lengths per UAX#29, then
change the condition handling (the branch where terminal == null &&
HAS_JDK_GRAPHEME_SUPPORT) to call this new helper and its corresponding wcwidth
path, and make the same replacement where the duplicate logic appears (the other
occurrence referenced in the comment) so all JDK21 null-terminal paths use the
BreakIterator-based implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06ff2d9c-6827-4b99-8d7c-c56e23e9e9e7
📒 Files selected for processing (2)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.java
✅ Files skipped from review due to trivial changes (1)
- terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/utils/WCWidth.java (1)
425-435: Consider cachingBreakIteratorand avoiding repeatedtoString()for bulk operations.This method allocates a new
BreakIteratorand converts the entireCharSequenceto aStringon every call. For typical single-cluster lookups this is acceptable, but width-computation loops (e.g.,columnLength()) will invoke this repeatedly, creating allocation pressure.If profiling shows this as a hot path, consider:
- Thread-local
BreakIteratorcaching- A bulk overload that processes multiple indices with a single
BreakIteratorsetup💡 Sketch for ThreadLocal caching
private static final ThreadLocal<BreakIterator> GRAPHEME_ITERATOR = ThreadLocal.withInitial(BreakIterator::getCharacterInstance); private static int charCountForGraphemeClusterBreakIterator(CharSequence cs, int index) { int len = cs.length(); if (index >= len) return 0; BreakIterator bi = GRAPHEME_ITERATOR.get(); bi.setText(cs.toString()); int next = bi.following(index); if (next == BreakIterator.DONE) { return len - index; } return next - index; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/utils/WCWidth.java` around lines 425 - 435, The method charCountForGraphemeClusterBreakIterator currently allocates a new BreakIterator and calls cs.toString() on every invocation, causing allocation churn in tight loops; fix it by introducing a ThreadLocal<BreakIterator> (e.g., GRAPHEME_ITERATOR) and reuse that BreakIterator instance instead of creating one per call, and avoid repeated toString() by accepting a precomputed String or by adding a bulk overload (e.g., charCountForGraphemeClusterBreakIterator(CharSequence cs, int index, BreakIterator bi) or a method that computes multiple cluster ends in one pass) so callers can setText once and call following() repeatedly; update charCountForGraphemeClusterBreakIterator to get the thread-local BreakIterator, setText once with the String representation provided by the caller (or by a caller-local variable) and then call following(index) to compute the length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 425-435: The method charCountForGraphemeClusterBreakIterator
currently allocates a new BreakIterator and calls cs.toString() on every
invocation, causing allocation churn in tight loops; fix it by introducing a
ThreadLocal<BreakIterator> (e.g., GRAPHEME_ITERATOR) and reuse that
BreakIterator instance instead of creating one per call, and avoid repeated
toString() by accepting a precomputed String or by adding a bulk overload (e.g.,
charCountForGraphemeClusterBreakIterator(CharSequence cs, int index,
BreakIterator bi) or a method that computes multiple cluster ends in one pass)
so callers can setText once and call following() repeatedly; update
charCountForGraphemeClusterBreakIterator to get the thread-local BreakIterator,
setText once with the String representation provided by the caller (or by a
caller-local variable) and then call following(index) to compute the length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 519f683f-5580-4637-9fce-728e9fb7ddda
📒 Files selected for processing (1)
terminal/src/main/java/org/jline/utils/WCWidth.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
terminal/src/main/java/org/jline/utils/WCWidth.java (2)
612-616: Partial emoji grouping support not fully honored in ungrouped fallback.When a terminal supports some emoji groupings but not others (e.g.,
SKIN_TONE_MODIFIERbut notZWJ_SEQUENCE), the currentwcwidthUngrouped()call sums all codepoint widths without considering the terminal's supported categories.For example, with
👩🏽💻(woman technologist with medium skin tone):
- Classified as
ZWJ_SEQUENCE(ZWJ takes priority)- If terminal doesn't support ZWJ but does support skin tones, ideally
👩🏽should render as width 2 and💻as width 2 → total 4- Currently returns sum of individual codepoints via
wcwidthUngrouped()This edge case primarily affects terminals with partial Mode 2027 support, which is uncommon. Consider passing the terminal's grouping set to
wcwidthUngrouped()in a future iteration if partial support becomes more prevalent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/utils/WCWidth.java` around lines 612 - 616, The fallback path currently calls wcwidthUngrouped(cs, index, charCount) which blindly sums all codepoint widths and ignores which emoji grouping categories the terminal actually supports (e.g., SKIN_TONE_MODIFIER vs ZWJ_SEQUENCE). Change the code and API so wcwidthUngrouped accepts the terminal's supported grouping set (e.g., add a parameter like Set<EmojiGrouping> supportedGroups or an EnumSet) and pass that from the call site in classifyCluster handling (where EmojiGrouping cat is checked). Update wcwidthUngrouped to respect supportedGroups when splitting clusters (treat supported categories as grouped and unsupported ones as separate codepoints) so clusters like a skin-tone + ZWJ sequence are computed correctly when the terminal supports some but not all groupings.
425-435: Consider caching or avoiding full string conversion for performance.
cs.toString()allocates a newStringon every call. Since this method may be invoked repeatedly during display calculations (once per grapheme cluster), this could create allocation pressure for large buffers.Potential mitigations:
- If the caller already has the string, pass it directly.
- For
AttributedCharSequence, consider adding asubSequence-based approach to limit the conversion scope.- Cache
BreakIteratorinstances per-thread if profiling shows this as a hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/utils/WCWidth.java` around lines 425 - 435, charCountForGraphemeClusterBreakIterator currently calls cs.toString() on every invocation which allocates a new String and can cause significant allocation pressure; change the API/implementation to avoid repeated String creation by (a) accepting a String when the caller already has one or adding an overload that takes String, (b) detecting when cs is already a String and using it directly instead of cs.toString(), or (c) for AttributedCharSequence provide a subSequence-based path so only the needed slice is converted; additionally, avoid creating a new BreakIterator each call by caching one per-thread (e.g., ThreadLocal<BreakIterator>) and reusing it via BreakIterator.setText(…) to reduce allocation and improve throughput (refer to charCountForGraphemeClusterBreakIterator, BreakIterator, and CharSequence cs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 612-616: The fallback path currently calls wcwidthUngrouped(cs,
index, charCount) which blindly sums all codepoint widths and ignores which
emoji grouping categories the terminal actually supports (e.g.,
SKIN_TONE_MODIFIER vs ZWJ_SEQUENCE). Change the code and API so wcwidthUngrouped
accepts the terminal's supported grouping set (e.g., add a parameter like
Set<EmojiGrouping> supportedGroups or an EnumSet) and pass that from the call
site in classifyCluster handling (where EmojiGrouping cat is checked). Update
wcwidthUngrouped to respect supportedGroups when splitting clusters (treat
supported categories as grouped and unsupported ones as separate codepoints) so
clusters like a skin-tone + ZWJ sequence are computed correctly when the
terminal supports some but not all groupings.
- Around line 425-435: charCountForGraphemeClusterBreakIterator currently calls
cs.toString() on every invocation which allocates a new String and can cause
significant allocation pressure; change the API/implementation to avoid repeated
String creation by (a) accepting a String when the caller already has one or
adding an overload that takes String, (b) detecting when cs is already a String
and using it directly instead of cs.toString(), or (c) for
AttributedCharSequence provide a subSequence-based path so only the needed slice
is converted; additionally, avoid creating a new BreakIterator each call by
caching one per-thread (e.g., ThreadLocal<BreakIterator>) and reusing it via
BreakIterator.setText(…) to reduce allocation and improve throughput (refer to
charCountForGraphemeClusterBreakIterator, BreakIterator, and CharSequence cs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce7dbc3e-d4a0-4202-a110-1b48f115b0e6
📒 Files selected for processing (2)
terminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/utils/WCWidthTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/test/java/org/jline/utils/WCWidthTest.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java (1)
643-652:⚠️ Potential issue | 🟡 MinorDon't swallow responder failures in the EOF test.
If
waitForOutput(masterOutput, "\033[6n")times out here, the thread exits quietly and the test still passes without ever exercising the EOF-after-DSR branch. Capture that failure in theResponderHandlethe same way the other helpers do, and only keepclose()as best-effort if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java` around lines 643 - 652, The responder thread currently swallows exceptions from waitForOutput(masterOutput, "\033[6n") causing the EOF test to pass silently; modify the Thread lambda used to construct ResponderHandle so it catches any Exception and stores it into the same AtomicReference passed to ResponderHandle (like the other helpers do) instead of ignoring it, then attempt terminal.slaveInputPipe.close() as a best-effort in a separate try/catch that does not override the stored exception; reference the ResponderHandle construction, the Thread lambda, waitForOutput, masterOutput, and terminal.slaveInputPipe.close() when making the change.terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
597-605:⚠️ Potential issue | 🟠 MajorFence each CPR probe so late replies can't poison the next read.
Each
\033[6ncurrently stands alone. If the terminal answers after the 200 ms timeout, that staleESC[row;colRcan be consumed by the second probe or leak into normal input, which reintroduces the late-CPR problem. Batch a per-probe sentinel the same wayprobeMode2027()does, or do a final bounded drain before returning.Possible direction
- writer().write("\033[6n"); // DSR — request cursor position + writer().write("\033[6n\033[c"); // DSR + DA1 fence writer().flush(); int col = readCprColumn(200); + drainUntilDA1(200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 597 - 605, The CPR probe must be fenced so late ESC[ row;col R replies can't be consumed by subsequent reads; modify the sequence around writer().write("\033[6n") in the CPR probe (the code that calls puts(Capability.save_cursor), puts(Capability.carriage_return), puts(Capability.clr_eol), writer().write(emoji), writer().flush(), and then readCprColumn(200)) to append a per-probe sentinel (as probeMode2027() does) or to perform a bounded drain after the readCprColumn(200) call; ensure readCprColumn or its caller ignores any data until the sentinel or drains leftover bytes before returning so late CPR replies cannot leak into the next probe or normal input.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
408-412: Avoid cloning the grouping set on every width check.
terminal/src/main/java/org/jline/utils/WCWidth.javacallsTerminal.getEmojiGroupings()for each cluster it measures, soEnumSet.copyOf(...)here turns grapheme-aware rendering into an allocation-heavy hot path. Since this field is replaced wholesale, consider storing immutable snapshots (plus shared ALL/NONE constants) and returning them directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 408 - 412, The current getEmojiGroupings() clones emojiGroupings on every call (EnumSet.copyOf), causing heavy allocations in the hot WCWidth width-check path; change the implementation to return immutable/shared snapshots instead: maintain emojiGroupings as an immutable EnumSet (or replace it with a volatile reference to an immutable Set) and add shared constants for ALL (EnumSet.allOf(EmojiGrouping.class)) and NONE (EnumSet.noneOf(EmojiGrouping.class)) to return directly when grapheme mode applies or when no custom grouping is set; update getEmojiGroupings() to return those precomputed immutable instances (or the stored immutable snapshot) without copying so callers like WCWidth avoid per-cluster allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 597-605: The CPR probe must be fenced so late ESC[ row;col R
replies can't be consumed by subsequent reads; modify the sequence around
writer().write("\033[6n") in the CPR probe (the code that calls
puts(Capability.save_cursor), puts(Capability.carriage_return),
puts(Capability.clr_eol), writer().write(emoji), writer().flush(), and then
readCprColumn(200)) to append a per-probe sentinel (as probeMode2027() does) or
to perform a bounded drain after the readCprColumn(200) call; ensure
readCprColumn or its caller ignores any data until the sentinel or drains
leftover bytes before returning so late CPR replies cannot leak into the next
probe or normal input.
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`:
- Around line 643-652: The responder thread currently swallows exceptions from
waitForOutput(masterOutput, "\033[6n") causing the EOF test to pass silently;
modify the Thread lambda used to construct ResponderHandle so it catches any
Exception and stores it into the same AtomicReference passed to ResponderHandle
(like the other helpers do) instead of ignoring it, then attempt
terminal.slaveInputPipe.close() as a best-effort in a separate try/catch that
does not override the stored exception; reference the ResponderHandle
construction, the Thread lambda, waitForOutput, masterOutput, and
terminal.slaveInputPipe.close() when making the change.
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 408-412: The current getEmojiGroupings() clones emojiGroupings on
every call (EnumSet.copyOf), causing heavy allocations in the hot WCWidth
width-check path; change the implementation to return immutable/shared snapshots
instead: maintain emojiGroupings as an immutable EnumSet (or replace it with a
volatile reference to an immutable Set) and add shared constants for ALL
(EnumSet.allOf(EmojiGrouping.class)) and NONE
(EnumSet.noneOf(EmojiGrouping.class)) to return directly when grapheme mode
applies or when no custom grouping is set; update getEmojiGroupings() to return
those precomputed immutable instances (or the stored immutable snapshot) without
copying so callers like WCWidth avoid per-cluster allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4674d54d-5d72-41c4-bd4b-88cfc07ea7fa
📒 Files selected for processing (4)
terminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
✅ Files skipped from review due to trivial changes (1)
- terminal/src/main/java/org/jline/utils/WCWidth.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
452-467: Please sync the public probing contract.This path now probes any non-dumb terminal, and
GraphemeClusterModeTest.testProbeSentForNonXtermTerminal()codifies that.Terminal.supportsGraphemeClusterMode()still documents DECRQM as xterm-only, so callers will read the old contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 452 - 467, The public contract for grapheme cluster probing is out of sync: probeGraphemeClusterMode() now probes any non-dumb terminal (using probeMode2027() and falling back to probeCursorPosition()), but Terminal.supportsGraphemeClusterMode() still claims DECRQM is xterm-only; update the API docs for supportsGraphemeClusterMode() to reflect the new behavior (that non-dumb terminals are probed, DECRQM is attempted and if NOT_SUPPORTED we fall back to a cursor-position probe, and that probing occurs in raw mode to avoid echo/blocking), and mention GraphemeClusterModeTest.testProbeSentForNonXtermTerminal() as the behavior test; keep the description concise and accurate to the probeGraphemeClusterMode() implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 691-708: The code currently ties capability flags
(graphemeClusterNative / groupsRegionalIndicators / groupsZwjSequences) to the
requested enable state in setGraphemeClusterMode, which clears the forced
capability on a force-disable and prevents a later non-force enable from
restoring grouping; update setGraphemeClusterMode so that when force is true you
mark capability as supported/native (set graphemeClusterModeSupported = true and
graphemeClusterNative = true) but do NOT overwrite the capability grouping flags
(groupsRegionalIndicators, groupsZwjSequences) based on the enable
parameter—only set graphemeClusterModeEnabled = enable; ensure the non-force
path still updates groupsRegionalIndicators/groupsZwjSequences when actually
probing or sending Mode 2027 (leave the writer() escape/send logic in the
supportsGraphemeClusterMode() branch), and add a regression test exercising the
force-disable → non-force-enable cycle to confirm grouping is preserved and
escape sequences are sent when appropriate; refer to setGraphemeClusterMode,
graphemeClusterNative, graphemeClusterModeSupported, graphemeClusterModeEnabled,
groupsRegionalIndicators, and groupsZwjSequences when making changes.
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 452-467: The public contract for grapheme cluster probing is out
of sync: probeGraphemeClusterMode() now probes any non-dumb terminal (using
probeMode2027() and falling back to probeCursorPosition()), but
Terminal.supportsGraphemeClusterMode() still claims DECRQM is xterm-only; update
the API docs for supportsGraphemeClusterMode() to reflect the new behavior (that
non-dumb terminals are probed, DECRQM is attempted and if NOT_SUPPORTED we fall
back to a cursor-position probe, and that probing occurs in raw mode to avoid
echo/blocking), and mention
GraphemeClusterModeTest.testProbeSentForNonXtermTerminal() as the behavior test;
keep the description concise and accurate to the probeGraphemeClusterMode()
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6db1592-79bd-4905-a78b-1b92bd764cf8
📒 Files selected for processing (4)
terminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/main/java/org/jline/utils/WCWidth.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java (1)
645-654:⚠️ Potential issue | 🟡 MinorDon't swallow responder failures in the EOF test.
This thread ignores every exception, so a timeout waiting for
\033[6nstill looks like success and the test can pass without ever exercising the EOF branch. Capture the failure in theAtomicReferencelike the shared helpers do.Possible fix
- ResponderHandle responder = new ResponderHandle( - new Thread(() -> { - try { - waitForOutput(masterOutput, "\033[6n"); - terminal.slaveInputPipe.close(); - } catch (Exception e) { - // pipe close may throw; ignore since the test validates the outcome - } - }), - new AtomicReference<>()); + AtomicReference<Exception> error = new AtomicReference<>(); + ResponderHandle responder = new ResponderHandle( + new Thread(() -> { + try { + waitForOutput(masterOutput, "\033[6n"); + terminal.slaveInputPipe.close(); + } catch (Exception e) { + error.set(e); + } + }), + error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java` around lines 645 - 654, The thread in the ResponderHandle swallows all exceptions so timeouts or other failures are hidden; change the catch block to record the thrown Throwable into the provided AtomicReference (the same way shared helpers do) instead of ignoring it — for the ResponderHandle created here capture exceptions from waitForOutput(masterOutput, "\033[6n") and terminal.slaveInputPipe.close() by calling atomicRef.set(e) (where atomicRef is the AtomicReference passed into the ResponderHandle) so the test harness can detect and fail the test when the responder thread failed.terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
619-625:⚠️ Potential issue | 🟠 MajorFence late CPR replies before leaving the fallback probe.
If
readCprColumn(200)times out,probeEmojiWidth()restores the cursor and returns immediately, but a delayedESC[row;colRcan still arrive afterwards and remain buffered as application input. The DECRQM path already has a DA1 fence; the DSR fallback needs equivalent cleanup on the timeout path too.Also applies to: 636-653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 619 - 625, probeEmojiWidth currently calls readCprColumn(200) and returns immediately on timeout, which can leave a delayed "ESC[row;colR" CPR buffered; update probeEmojiWidth so that when readCprColumn signals a timeout you perform the same CPR-fence/cleanup used on the DECRQM path: drain or consume any pending CPR reply (non-blocking read loop that looks for and discards an ESC...[0-9;]*R sequence) before calling puts(Capability.restore_cursor)/puts(Capability.clr_eol)/puts(Capability.carriage_return) and returning; make the identical fix for the other fallback block around the 636-653 region so both timeout paths fence late CPR replies.
🧹 Nitpick comments (1)
terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java (1)
543-550: Make the responder observe both cursor-position queries.These helpers emit both CPR payloads as soon as the first
\033[6nappears. That lets the secondreadCprColumn()succeed even if the code never sends the second DSR, so the new per-category probe tests won't catch that regression.Also applies to: 591-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java` around lines 543 - 550, The test responder currently writes the combined CPR payloads as soon as it sees the first DSR query ("\033[6n"), which can let the second read succeed without a second DSR; update the helper logic in GraphemeClusterModeTest (the block using waitForOutput(masterOutput, "\033[6n") and writing terminal.slaveInputPipe.write(cprResponses...)) to observe and count two separate "\033[6n" occurrences on masterOutput before writing the CPR responses to terminal.slaveInputPipe, ensuring the responder only emits the CPR payloads after the second DSR is seen; apply the same change to the other occurrence mentioned (around the 591-593 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 612-614: The probe that checks cursor behavior is erasing the
current prompt by calling puts(Capability.carriage_return) and
puts(Capability.clr_eol) after puts(Capability.save_cursor); remove those two
calls from the probe (and the analogous calls at lines referencing
puts(Capability.carriage_return)/puts(Capability.clr_eol) later) so the code
only saves and later restores the cursor (puts(Capability.save_cursor) ...
puts(Capability.restore_cursor)) instead of clearing the active line; locate the
probe code using the puts(...) calls and Capability.save_cursor /
Capability.restore_cursor to apply this change.
---
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 619-625: probeEmojiWidth currently calls readCprColumn(200) and
returns immediately on timeout, which can leave a delayed "ESC[row;colR" CPR
buffered; update probeEmojiWidth so that when readCprColumn signals a timeout
you perform the same CPR-fence/cleanup used on the DECRQM path: drain or consume
any pending CPR reply (non-blocking read loop that looks for and discards an
ESC...[0-9;]*R sequence) before calling
puts(Capability.restore_cursor)/puts(Capability.clr_eol)/puts(Capability.carriage_return)
and returning; make the identical fix for the other fallback block around the
636-653 region so both timeout paths fence late CPR replies.
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`:
- Around line 645-654: The thread in the ResponderHandle swallows all exceptions
so timeouts or other failures are hidden; change the catch block to record the
thrown Throwable into the provided AtomicReference (the same way shared helpers
do) instead of ignoring it — for the ResponderHandle created here capture
exceptions from waitForOutput(masterOutput, "\033[6n") and
terminal.slaveInputPipe.close() by calling atomicRef.set(e) (where atomicRef is
the AtomicReference passed into the ResponderHandle) so the test harness can
detect and fail the test when the responder thread failed.
---
Nitpick comments:
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`:
- Around line 543-550: The test responder currently writes the combined CPR
payloads as soon as it sees the first DSR query ("\033[6n"), which can let the
second read succeed without a second DSR; update the helper logic in
GraphemeClusterModeTest (the block using waitForOutput(masterOutput, "\033[6n")
and writing terminal.slaveInputPipe.write(cprResponses...)) to observe and count
two separate "\033[6n" occurrences on masterOutput before writing the CPR
responses to terminal.slaveInputPipe, ensuring the responder only emits the CPR
payloads after the second DSR is seen; apply the same change to the other
occurrence mentioned (around the 591-593 region).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47121f80-38ab-4b3c-b1d2-81d06d9f6bdf
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
611-624:⚠️ Potential issue | 🟠 MajorKeep the CPR fallback off the active line.
Lines 613-614 and 621-622 clear the current line around each probe.
save_cursor/restore_cursoronly restore the cursor position, so a latesupportsGraphemeClusterMode()call can erase an already-rendered prompt or status line just to measure emoji width. Please move this probe to a non-destructive scratch area, or constrain it to terminal construction before anything user-visible is painted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 611 - 624, probeEmojiWidth currently writes and clears on the active line (uses puts(Capability.save_cursor)/puts(Capability.carriage_return)/puts(Capability.clr_eol)/puts(Capability.restore_cursor)), which can erase a user-visible prompt if called late (e.g. from supportsGraphemeClusterMode()); change the probe so it does not touch the active line by performing the measurement in a non-destructive scratch area or only during terminal construction before anything is painted: for example, switch to an off-screen/alternate buffer or move the cursor to a safe blank row, write the emoji and request CPR there, then restore the original screen/cursor; update probeEmojiWidth to use that approach and remove any clearing of the active line during normal runtime.
🧹 Nitpick comments (1)
terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java (1)
39-42: Make this test helper mirror the full probe response and fail fast.At Line 40 only the DECRPM bytes are queued, but
probeMode2027()now drains a trailing DA1 sentinel as well. That makes everycreate()call depend on the timeout path, and Line 42 ignores the enable result, so the helper can silently return a terminal that is not configured as advertised.♻️ Suggested update
- terminal.slaveInputPipe.write("\033[?2027;2$y".getBytes(StandardCharsets.UTF_8)); + terminal.slaveInputPipe.write("\033[?2027;2$y\033[?64c".getBytes(StandardCharsets.UTF_8)); terminal.slaveInputPipe.flush(); - terminal.setGraphemeClusterMode(true, false); + if (!terminal.setGraphemeClusterMode(true, false)) { + throw new IllegalStateException("Failed to enable grapheme cluster mode for test terminal"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java` around lines 39 - 42, The helper queues only the DECRPM probe bytes but probeMode2027() now expects the full probe response including the trailing DA1 sentinel, and the call to setGraphemeClusterMode(true, false) ignores the boolean result; update the helper to write the complete probe response including the DA1 sentinel sequence (matching what probeMode2027() drains) into terminal.slaveInputPipe and flush, then call terminal.setGraphemeClusterMode(true, false) and immediately check its boolean return (or assert/throw) to fail fast if enabling did not succeed so the created terminal truly reflects the advertised configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 611-624: probeEmojiWidth currently writes and clears on the active
line (uses
puts(Capability.save_cursor)/puts(Capability.carriage_return)/puts(Capability.clr_eol)/puts(Capability.restore_cursor)),
which can erase a user-visible prompt if called late (e.g. from
supportsGraphemeClusterMode()); change the probe so it does not touch the active
line by performing the measurement in a non-destructive scratch area or only
during terminal construction before anything is painted: for example, switch to
an off-screen/alternate buffer or move the cursor to a safe blank row, write the
emoji and request CPR there, then restore the original screen/cursor; update
probeEmojiWidth to use that approach and remove any clearing of the active line
during normal runtime.
---
Nitpick comments:
In
`@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java`:
- Around line 39-42: The helper queues only the DECRPM probe bytes but
probeMode2027() now expects the full probe response including the trailing DA1
sentinel, and the call to setGraphemeClusterMode(true, false) ignores the
boolean result; update the helper to write the complete probe response including
the DA1 sentinel sequence (matching what probeMode2027() drains) into
terminal.slaveInputPipe and flush, then call
terminal.setGraphemeClusterMode(true, false) and immediately check its boolean
return (or assert/throw) to fail fast if enabling did not succeed so the created
terminal truly reflects the advertised configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2942cc6-50a0-45af-b3ca-5ee003aa7d86
📒 Files selected for processing (6)
demo/src/main/java/org/jline/demo/examples/GraphemeClusterExample.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/TerminalBuilder.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java
✅ Files skipped from review due to trivial changes (1)
- terminal/src/main/java/org/jline/terminal/TerminalBuilder.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
619-623:⚠️ Potential issue | 🟠 MajorDrain late CPR replies before returning from each emoji probe.
If
readCprColumn(200)times out or parses only part of the reply, the eventualESC[row;colRstays buffered. The second probe can consume the first reply, and any remainder can leak into normal input unless this path does a final CPR drain/fence before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 619 - 623, The CPR reply can remain buffered if readCprColumn(200) times out/partials — before restoring cursor and returning you must drain any late CPR bytes; after calling readCprColumn(200) (the existing call), loop calling readCprColumn with a short timeout (e.g. 0 or small value) until it indicates no more CPR data, then call puts(Capability.restore_cursor) and writer().flush(); update the code paths around readCprColumn and the restore_cursor sequence to perform this final CPR drain/fence so subsequent probes or normal input won't see stale CPR tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 583-588: The code incorrectly treats probeEmojiWidth(...) results
as absolute CPR columns (using flagCol == 3 / zwjCol == 3); instead compute the
cursor displacement from a known starting column before running the probe and
test that displacement equals the expected width (e.g., 2) rather than comparing
the absolute column. Update the probing flow around probeEmojiWidth, capture the
start cursor position, call
probeEmojiWidth(flagEmoji)/probeEmojiWidth(zwjEmoji), compute delta =
measuredColumn - startColumn (normalized for wrapping), and set
groupsRegionalIndicators/groupsZwjSequences based on that displacement instead
of equality to 3; alternatively change probeEmojiWidth to return displacement
directly and use that return value to set groupsRegionalIndicators and
groupsZwjSequences.
- Around line 568-572: The probeCursorPosition() method currently only checks
Capability.user6 and Capability.user7 but must also ensure save/restore cursor
capabilities are present because probeEmojiWidth() relies on sc/rc and puts(...)
may be a no-op without them; update probeCursorPosition() to additionally check
getStringCapability(Capability.save_cursor) and
getStringCapability(Capability.restore_cursor) (or Capability.sc/Capability.rc
if those enums are used) and return false if either is missing so the probe
won't modify the live screen or leave stray output.
---
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 619-623: The CPR reply can remain buffered if readCprColumn(200)
times out/partials — before restoring cursor and returning you must drain any
late CPR bytes; after calling readCprColumn(200) (the existing call), loop
calling readCprColumn with a short timeout (e.g. 0 or small value) until it
indicates no more CPR data, then call puts(Capability.restore_cursor) and
writer().flush(); update the code paths around readCprColumn and the
restore_cursor sequence to perform this final CPR drain/fence so subsequent
probes or normal input won't see stale CPR tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63aedea2-129e-4e6c-8054-3fb8183cc5df
📒 Files selected for processing (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (1)
568-572:⚠️ Potential issue | 🟡 MinorRequire
save_cursor/restore_cursorbefore probing on the live screen.This check only validates
u6/u7, butprobeEmojiWidth()relies onsave_cursor/restore_cursor. If either cursor capability is missing,puts(...)becomes a no-op and the probe leaves emoji characters and cursor movement visible on the user's terminal.🔧 Suggested fix
private boolean probeCursorPosition() { // Need u6 (CPR response pattern) and u7 (DSR query) capabilities - if (getStringCapability(Capability.user6) == null || getStringCapability(Capability.user7) == null) { + if (getStringCapability(Capability.user6) == null + || getStringCapability(Capability.user7) == null + || getStringCapability(Capability.save_cursor) == null + || getStringCapability(Capability.restore_cursor) == null) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 568 - 572, probeCursorPosition() currently only checks for Capability.user6 and Capability.user7 but probeEmojiWidth() uses save/restore cursor sequences; add checks for the cursor save/restore capabilities before probing: verify getStringCapability(Capability.save_cursor) and getStringCapability(Capability.restore_cursor) are non-null (in addition to Capability.user6/user7) and return false if either is missing so puts(...) won't be a no-op and the probe won't leave visible artifacts; update probeCursorPosition() to include these capability checks (and reference probeEmojiWidth() as the caller that requires them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 568-572: probeCursorPosition() currently only checks for
Capability.user6 and Capability.user7 but probeEmojiWidth() uses save/restore
cursor sequences; add checks for the cursor save/restore capabilities before
probing: verify getStringCapability(Capability.save_cursor) and
getStringCapability(Capability.restore_cursor) are non-null (in addition to
Capability.user6/user7) and return false if either is missing so puts(...) won't
be a no-op and the probe won't leave visible artifacts; update
probeCursorPosition() to include these capability checks (and reference
probeEmojiWidth() as the caller that requires them).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65a1f16d-e3c6-4df2-826a-f8c4fda2b171
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 418-434: The grouped-width decision in isClusterGrouped currently
ignores explicit disables because it only checks
groupsRegionalIndicators/groupsZwjSequences; add an early guard that returns
false when graphemeClusterModeEnabled is false to honor explicit/forced
disablement (so within AbstractTerminal.isClusterGrouped, before consulting
groupsRegionalIndicators/groupsZwjSequences and before the partial-support
logic, check the instance field/method graphemeClusterModeEnabled and return
false if disabled), ensuring WCWidth.isGroupedByTerminal behaves correctly after
native/forced disable while leaving the cached capability flags intact.
In
`@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java`:
- Around line 43-45: The create() path currently throws an IllegalStateException
when terminal.setGraphemeClusterMode(true, false) fails but never closes the
constructed LineDisciplineTerminal, leaking terminal state; update the failure
branch inside the create() method so that before throwing the
IllegalStateException you call terminal.close() (or terminal.shutdown/cleanup if
a specific close method exists) to ensure the LineDisciplineTerminal is cleaned
up when setGraphemeClusterMode(...) fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a696ac0-0b41-4c50-966a-7655b107ab85
📒 Files selected for processing (9)
demo/src/main/java/org/jline/demo/examples/GraphemeClusterExample.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/TerminalBuilder.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.javaterminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.javaterminal/src/test/java/org/jline/utils/WCWidthTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/src/main/java/org/jline/utils/WCWidth.java
- terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java (2)
517-520:⚠️ Potential issue | 🟠 MajorTimed-out DECRQM probes can still leak escape bytes into later reads.
The
peek(timeout) < 0andIOExceptionexits returnNO_RESPONSEwithout a final DA1 drain. If the terminal responds just after the 100 ms window, the queued DECRPM/DA1 bytes stay buffered and the next application read can see rawESC [input. Do the same best-effort sentinel drain on these early-return paths too.Also applies to: 548-549
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 517 - 520, In AbstractTerminal (around the DECRQM probe code using reader().peek and returning ProbeResult.NO_RESPONSE), ensure you perform the same best-effort DA1 sentinel drain before every early return path: after the timeout branch (peek(timeout) < 0) and in the catch(IOException) branch add the DA1 drain logic used elsewhere (consume any pending DECRQM/DA1 bytes from the reader) before returning ProbeResult.NO_RESPONSE so late-arriving bytes are not left buffered for subsequent reads; locate the existing DA1-drain implementation used in the successful-probe path and call or inline that same cleanup here.
588-604:⚠️ Potential issue | 🟠 MajorThe CPR fallback still misdetects near the right margin.
probeCursorPosition()compares two 1-based columns, butreadCprColumn()throws away the row. If the probe starts close to the wrap boundary, a correctly grouped emoji can advance onto the next line and produce the wrong delta; becausesupportsGraphemeClusterMode()caches that result, one false negative disables the feature for the terminal lifetime. Parse row + column or run the probe from a known safe column before comparing displacement.Also applies to: 650-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java` around lines 588 - 604, probeCursorPosition() is misdetecting when probes wrap to the next line because readCprColumn() discards the row; update the probe logic so you either (a) parse and return both row and column from the CPR reader used by readCprColumn() and compare both row and column deltas in probeCursorPosition()/probeEmojiWidth(), or (b) move the probe to a guaranteed-safe column before probing (e.g., seek to a known safe column) so wrapping cannot occur; ensure supportsGraphemeClusterMode() uses the corrected probe result to set groupsRegionalIndicators and groupsZwjSequences reliably and adjust readCprColumn()/probeCursorPosition()/probeEmojiWidth() accordingly.terminal/src/main/java/org/jline/utils/WCWidth.java (1)
607-615:⚠️ Potential issue | 🟠 MajorUnsupported emoji clusters are still unsplittable.
When a terminal does not group a cluster,
wcwidthForDisplayWithGroupings()falls back to summed per-codepoint width, butcharCountForDisplayWithGroupings()still returns the full cluster length. On a flags-only terminal,👩🔬becomes width 4 but is still consumed as one display unit, socolumnSubSequence()/columnSplitLength()cannot split at the visible 2-column boundary. This helper needs to return the first visible sub-run instead of the whole cluster, and the currentcp >= 0x2000heuristic makes the same problem hit non-emoji clusters as well.Also applies to: 628-637, 694-700
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/utils/WCWidth.java` around lines 607 - 615, The issue: when the terminal doesn't group a multi-codepoint cluster wcwidthForDisplayWithGroupings returns the summed width but charCountForDisplayWithGroupings still returns the full cluster length, preventing splits at visible sub-run boundaries; also the cp >= 0x2000 heuristic incorrectly treats non-emoji clusters. Fix by changing charCountForDisplayWithGroupings to, when isGroupedByTerminal(...) is false and isMultiCodepointEmoji(...) (or otherwise multi-codepoint cluster) is true, iterate the cluster's codepoints and accumulate per-codepoint widths using wcwidthUngrouped(...) (or the same per-codepoint width logic used by wcwidthForDisplayWithGroupings) and return the number of codepoints up to the first visible sub-run boundary (e.g., when a non-zero width or terminal column split would occur), rather than the full grapheme length; remove or replace the cp >= 0x2000 heuristic and rely on isMultiCodepointEmoji/isGroupedByTerminal to decide when to split so non-emoji clusters aren’t misclassified. Ensure this same sub-run logic is applied in the corresponding branches noted (around the other ranges: 628-637 and 694-700) and keep wcwidthForDisplayWithGroupings using wcwidthForGraphemeCluster only when grouping is supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/src/main/java/org/jline/terminal/Terminal.java`:
- Line 1362: The WCWidth.isGroupedByTerminal() call currently downcasts to
org.jline.terminal.impl.AbstractTerminal to detect per-category partial
grouping, which breaks for custom Terminal implementations; add a new boolean
accessor on the Terminal interface (e.g., isGraphemeClusterGroupedPerCategory()
or similar) and document its contract, implement it in AbstractTerminal
(delegating to existing setGraphemeClusterMode/state) and any other Terminal
implementations, and then update WCWidth.isGroupedByTerminal(), plus related
call sites around setGraphemeClusterMode usages (references: Terminal,
AbstractTerminal, WCWidth.isGroupedByTerminal(), setGraphemeClusterMode) to call
the new Terminal-level accessor instead of performing an instanceof/downcast so
custom Terminals honor partial grouping.
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 413-415: The BreakIterator-based branch in
charCountForGraphemeClusterBreakIterator is causing heavy allocations because
each call creates a new BreakIterator and converts the sequence to a String;
update the code to reuse a BreakIterator per sequence (or let the caller drive a
single BreakIterator walk) so repeated calls from charCountForDisplay and
wcwidthForDisplay (and the AttributedCharSequence scanning loops) do not
recreate objects; specifically, refactor
charCountForGraphemeClusterBreakIterator to accept a reusable BreakIterator (or
return cluster boundaries in a reusable iterator/consumer), add a cached
BreakIterator or a higher-level loop in the callers (charCountForDisplay,
wcwidthForDisplay, and the AttributedCharSequence scanning code) to walk
boundaries once per sequence, and ensure thread-safety by creating/clearing the
BreakIterator per sequence usage rather than per character.
---
Duplicate comments:
In `@terminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.java`:
- Around line 517-520: In AbstractTerminal (around the DECRQM probe code using
reader().peek and returning ProbeResult.NO_RESPONSE), ensure you perform the
same best-effort DA1 sentinel drain before every early return path: after the
timeout branch (peek(timeout) < 0) and in the catch(IOException) branch add the
DA1 drain logic used elsewhere (consume any pending DECRQM/DA1 bytes from the
reader) before returning ProbeResult.NO_RESPONSE so late-arriving bytes are not
left buffered for subsequent reads; locate the existing DA1-drain implementation
used in the successful-probe path and call or inline that same cleanup here.
- Around line 588-604: probeCursorPosition() is misdetecting when probes wrap to
the next line because readCprColumn() discards the row; update the probe logic
so you either (a) parse and return both row and column from the CPR reader used
by readCprColumn() and compare both row and column deltas in
probeCursorPosition()/probeEmojiWidth(), or (b) move the probe to a
guaranteed-safe column before probing (e.g., seek to a known safe column) so
wrapping cannot occur; ensure supportsGraphemeClusterMode() uses the corrected
probe result to set groupsRegionalIndicators and groupsZwjSequences reliably and
adjust readCprColumn()/probeCursorPosition()/probeEmojiWidth() accordingly.
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 607-615: The issue: when the terminal doesn't group a
multi-codepoint cluster wcwidthForDisplayWithGroupings returns the summed width
but charCountForDisplayWithGroupings still returns the full cluster length,
preventing splits at visible sub-run boundaries; also the cp >= 0x2000 heuristic
incorrectly treats non-emoji clusters. Fix by changing
charCountForDisplayWithGroupings to, when isGroupedByTerminal(...) is false and
isMultiCodepointEmoji(...) (or otherwise multi-codepoint cluster) is true,
iterate the cluster's codepoints and accumulate per-codepoint widths using
wcwidthUngrouped(...) (or the same per-codepoint width logic used by
wcwidthForDisplayWithGroupings) and return the number of codepoints up to the
first visible sub-run boundary (e.g., when a non-zero width or terminal column
split would occur), rather than the full grapheme length; remove or replace the
cp >= 0x2000 heuristic and rely on isMultiCodepointEmoji/isGroupedByTerminal to
decide when to split so non-emoji clusters aren’t misclassified. Ensure this
same sub-run logic is applied in the corresponding branches noted (around the
other ranges: 628-637 and 694-700) and keep wcwidthForDisplayWithGroupings using
wcwidthForGraphemeCluster only when grouping is supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7afbd4fc-446f-4eda-9fd9-4917d9965ae5
📒 Files selected for processing (9)
demo/src/main/java/org/jline/demo/examples/GraphemeClusterExample.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/TerminalBuilder.javaterminal/src/main/java/org/jline/terminal/impl/AbstractTerminal.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.javaterminal/src/test/java/org/jline/utils/AttributedCharSequenceTest.javaterminal/src/test/java/org/jline/utils/WCWidthTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterTestTerminal.java
- terminal/src/main/java/org/jline/terminal/TerminalBuilder.java
- terminal/src/test/java/org/jline/utils/WCWidthTest.java
- terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
| * | ||
| * @return {@code true} if the terminal supports mode 2027, {@code false} otherwise | ||
| * @see #setGraphemeClusterMode(boolean) | ||
| * @see #setGraphemeClusterMode(boolean, boolean) |
Contributor
There was a problem hiding this comment.
Expose partial grouping on Terminal, not just on AbstractTerminal.
WCWidth.isGroupedByTerminal() now has to downcast to org.jline.terminal.impl.AbstractTerminal to recover the per-category behavior. Any custom Terminal implementation that does not inherit that base class silently falls back to all-or-nothing grouping, so the new partial-support path is unavailable outside JLine’s own terminal hierarchy. Please move that contract onto Terminal itself instead of hiding it behind an implementation-specific type check. A Terminal-level grouping accessor would remove the downcast entirely.
Also applies to: 1373-1373, 1400-1400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/src/main/java/org/jline/terminal/Terminal.java` at line 1362, The
WCWidth.isGroupedByTerminal() call currently downcasts to
org.jline.terminal.impl.AbstractTerminal to detect per-category partial
grouping, which breaks for custom Terminal implementations; add a new boolean
accessor on the Terminal interface (e.g., isGraphemeClusterGroupedPerCategory()
or similar) and document its contract, implement it in AbstractTerminal
(delegating to existing setGraphemeClusterMode/state) and any other Terminal
implementations, and then update WCWidth.isGroupedByTerminal(), plus related
call sites around setGraphemeClusterMode usages (references: Terminal,
AbstractTerminal, WCWidth.isGroupedByTerminal(), setGraphemeClusterMode) to call
the new Terminal-level accessor instead of performing an instanceof/downcast so
custom Terminals honor partial grouping.
Comment on lines
+413
to
+415
| if (HAS_JDK_GRAPHEME_SUPPORT) { | ||
| return charCountForGraphemeClusterBreakIterator(cs, index); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check file size to determine reading strategy
wc -l terminal/src/main/java/org/jline/utils/WCWidth.javaRepository: jline/jline3
Length of output: 111
🏁 Script executed:
# Check the BSD 3-Clause header (lines 1-8 typically)
head -20 terminal/src/main/java/org/jline/utils/WCWidth.javaRepository: jline/jline3
Length of output: 616
🏁 Script executed:
# Read the problematic section around lines 413-440
sed -n '400,450p' terminal/src/main/java/org/jline/utils/WCWidth.javaRepository: jline/jline3
Length of output: 2092
🏁 Script executed:
# Search for the charCountForGraphemeClusterBreakIterator implementation
rg -n "charCountForGraphemeClusterBreakIterator|HAS_JDK_GRAPHEME_SUPPORT" terminal/src/main/java/org/jline/utils/WCWidth.java -A 10Repository: jline/jline3
Length of output: 3501
🏁 Script executed:
# Search for calls to charCountForGraphemeCluster to understand the hot path
rg -n "charCountForGraphemeCluster|charCountForDisplay" terminal/src/main/java/org/jline/utils/ -B 2 -A 2Repository: jline/jline3
Length of output: 7857
This BreakIterator path creates unnecessary allocations in column-width hot paths.
Every call to charCountForGraphemeClusterBreakIterator() allocates a new BreakIterator instance, converts the entire sequence to String, and searches for a single boundary. Since charCountForDisplay() and wcwidthForDisplay() invoke this repeatedly while scanning lines (as seen in AttributedCharSequence loops at lines 665, 709, 721, 791), this reallocation overhead accumulates for long inputs on JDK 21+.
Consider caching one iterator per sequence or lifting the BreakIterator walk up to the caller to avoid repeated initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/src/main/java/org/jline/utils/WCWidth.java` around lines 413 - 415,
The BreakIterator-based branch in charCountForGraphemeClusterBreakIterator is
causing heavy allocations because each call creates a new BreakIterator and
converts the sequence to a String; update the code to reuse a BreakIterator per
sequence (or let the caller drive a single BreakIterator walk) so repeated calls
from charCountForDisplay and wcwidthForDisplay (and the AttributedCharSequence
scanning loops) do not recreate objects; specifically, refactor
charCountForGraphemeClusterBreakIterator to accept a reusable BreakIterator (or
return cluster boundaries in a reusable iterator/consumer), add a cached
BreakIterator or a higher-level loop in the callers (charCountForDisplay,
wcwidthForDisplay, and the AttributedCharSequence scanning code) to walk
boundaries once per sequence, and ensure thread-safety by creating/clearing the
BreakIterator per sequence usage rather than per character.
…#1753) When a terminal does not support Mode 2027, fall back to cursor-position probing (DSR/CPR) to detect native emoji grouping, and use JDK 21+ BreakIterator for grapheme cluster segmentation. Key changes: - Probe emoji width via save_cursor/write/CPR/restore_cursor using displacement from the current cursor column (not absolute position) - Two probes (flag + ZWJ) detect partial support (e.g. Tabby, Alacritty) - Use terminfo capabilities (u6, u7, sc, rc) instead of hardcoded escapes - JDK 21+ BreakIterator fallback for multi-codepoint emoji segmentation - Force-enable/disable semantics for setGraphemeClusterMode(boolean, boolean) - Remove EmojiGrouping enum from public Terminal API; keep grouping detection internal via isClusterGrouped()
|
This was referenced Apr 8, 2026
This was referenced Apr 17, 2026
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.
Summary
Fixes #1753 — improves grapheme cluster width computation for terminals that don't support Mode 2027 (DECRQM), with per-category emoji grouping detection for terminals with partial support.
Three-tier detection strategy
Mode 2027 (DECRQM/DECRPM) — probes the terminal with
CSI ? 2027 $ pusing DA1 as a sentinel fence. If the terminal reports the mode as set/reset/recognized, grapheme cluster mode is enabled withCSI ? 2027 h/lescape sequences.Cursor position probe — when Mode 2027 is not supported but the terminal did respond to the DECRQM/DA1 query (proving it handles escape sequences), writes two representative emoji and queries cursor position via DSR/CPR:
REGIONAL_INDICATOR— flag emoji (🇫🇷)ZWJ_SEQUENCE— Zero Width Joiner sequence (👩🔬), which also covers skin tone modifiers and VS16 presentationIf the cursor advances by 2 columns for a category, that category is marked as grouped. Terminals with at least one grouped category get grapheme cluster mode enabled without Mode 2027 escapes.
JDK 21+ BreakIterator fallback — on JDK 21+,
WCWidth.charCountForDisplay()andwcwidthForDisplay()always useBreakIterator-based grapheme cluster segmentation, regardless of whether the terminal probe succeeded. This gives correct widths for terminals that visually group emoji but don't report it via standard probing.Per-category width computation
When a terminal partially groups emoji (e.g., groups flags but not ZWJ sequences), width computation adapts per cluster:
wcwidthUngrouped(), with special handling for skin tone modifiers and regional indicators that havewcwidth=0in the combining table but render as width-2 standalonewcwidth()Terminal compatibility results
System property
org.jline.terminal.graphemeClusteris a tri-state property:true— force-enable grapheme cluster mode, skip probing (no Mode 2027 escapes sent)false— disable grapheme cluster mode, no probingThe
setGraphemeClusterMode(boolean, boolean)overload onTerminalsupports the force flag programmatically.API additions
Terminal.EmojiGroupingenum —REGIONAL_INDICATOR(flag pairs) andZWJ_SEQUENCE(ZWJ, skin tone, VS16)Terminal.getEmojiGroupings()— returns the set of grouped categories (all when Mode 2027, probed set otherwise)Terminal.setGraphemeClusterMode(boolean, boolean)— force flag to skip probingOther changes
drainUntilDA1()— reads until DA1 terminator 'c' instead of peek-based drain, eliminating 100ms blocking timeoutreadCprColumn()— lightweight CPR parser (replacesCursorSupport.getCursorPosition()for the probe)probeMode2027()returns tri-stateProbeResultto distinguish "not supported" from "no response"graphemeClusterNativeflag suppresses Mode 2027 escapes on enable/disable/closeBreakIterator.getCharacterInstance()used for grapheme cluster segmentation (replaces custom heuristic when available)Test coverage
30 tests in
GraphemeClusterModeTestcovering:setGraphemeClusterMode(true, true)sets all groupingsgetEmojiGroupings()returns correct sets for Mode 2027 / cursor probe / dumb terminal52 tests in
WCWidthTestcovering:wcwidthfor ASCII, CJK, control chars, combining marks, ZWJ, emojicharCountForGraphemeClustervia legacy heuristic and JDK 21+ BreakIteratorwcwidthForGraphemeClusterwith variation selectors (VS15/VS16)charCountForDisplayandwcwidthForDisplaywith and without terminalTest plan
GraphemeClusterModeTest— 30 tests passWCWidthTest— 52 tests passSummary by CodeRabbit
New Features
Improvements
Tests