feat: optimize output rendering with direct byte buffer#1749
Conversation
Add ByteArrayBuilder utility and direct byte-level rendering path to eliminate String allocations and charset encoding overhead in the terminal output hot path. Key changes: - Add ByteArrayBuilder with appendInt(), appendAscii(), appendUtf8(), and csi() methods for zero-allocation ANSI sequence construction - Add toAnsiBytes() to AttributedCharSequence for direct byte output - Optimize toAnsi() to eliminate String concatenation in color codes (e.g., "38;2;" + r + ";" + g + ";" + b → sb.append() chaining) - Wire Display.update() to accumulate all output (rawPrint + cursor positioning) in a ByteArrayBuilder for UTF-8 terminals, writing directly to terminal.output() and bypassing PrintWriter/ OutputStreamWriter entirely - Extract substituteChar() and isBoxDrawing() helpers to share box-drawing character logic between toAnsi() and toAnsiBytes()
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces String-based ANSI rendering with a byte-oriented path: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Display as "Display"
participant Buf as "ByteArrayBuilder"
participant ACS as "AttributedCharSequence"
participant Terminal as "Terminal"
Display->>Display: update(...)
alt byte mode enabled
Display->>Buf: reset()
Display->>ACS: toAnsiBytes(buf, colors, force, palette, altIn, altOut)
ACS->>Buf: appendAscii()/appendInt()/appendUtf8()/csi()
ACS-->>Display: buf (ANSI + UTF‑8 bytes)
Display->>Terminal: writer().flush()
Display->>Terminal: output().write(buf.buffer(), 0, buf.length())
Display->>Terminal: output().flush()
else standard mode
Display->>Terminal: puts(...) / writer().print(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.java (1)
408-559: High cognitive complexity is acceptable here given parity requirement.SonarCloud flags this method for high complexity (196 vs 15 allowed), but this is a deliberate mirror of
toAnsi()to ensure byte-for-byte output parity. Refactoring to reduce complexity would risk divergence between the two rendering paths.The loop counter increment at line 547 for surrogate pair handling is a standard pattern - the
i++correctly skips the low surrogate after the pair is emitted as a combined code point.Consider adding a brief comment explaining the surrogate pair skip:
buf.appendUtf8(Character.toCodePoint(c, next)); - i++; + i++; // skip low surrogate continue;🤖 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/AttributedCharSequence.java` around lines 408 - 559, Add a short explanatory comment above the surrogate-pair handling in toAnsiBytes to clarify why the loop does i++ after emitting a combined code point: indicate this is the standard surrogate-pair skip (checking Character.isHighSurrogate(c) and Character.isLowSurrogate(next)) to avoid re-processing the low surrogate and to match the identical behavior in toAnsi(); keep the comment minimal and placed immediately before the if (Character.isHighSurrogate(...)) block.
🤖 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/ByteArrayBuilder.java`:
- Around line 90-112: The negative branch in appendInt(int value) uses
appendInt(-value), which overflows for Integer.MIN_VALUE and causes infinite
recursion; change the handling to avoid negating Integer.MIN_VALUE—either detect
value == Integer.MIN_VALUE and append its string literal via
appendAscii("-2147483648"), or convert to a long (e.g., long v = value; if (v<0)
{ appendAscii('-'); v = -v; }) and then render digits, or simply call
appendAscii(Integer.toString(value)) for negative values; update appendInt to
use one of these approaches to eliminate the recursive -value case while still
calling appendAscii for the numeric text.
In `@terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java`:
- Around line 73-82: Add a unit test for the Integer.MIN_VALUE edge case and fix
ByteArrayBuilder.appendInt to avoid infinite recursion: add the test method
testAppendIntMinValue that calls buf.appendInt(Integer.MIN_VALUE) and asserts
"-2147483648", then update the implementation of ByteArrayBuilder.appendInt to
handle Integer.MIN_VALUE specially (e.g., detect value == Integer.MIN_VALUE and
append the literal "-2147483648") or avoid negating negatives by delegating to
Integer.toString(value) for negative values instead of calling
appendInt(-value), so the recursion/overflow is prevented.
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/utils/AttributedCharSequence.java`:
- Around line 408-559: Add a short explanatory comment above the surrogate-pair
handling in toAnsiBytes to clarify why the loop does i++ after emitting a
combined code point: indicate this is the standard surrogate-pair skip (checking
Character.isHighSurrogate(c) and Character.isLowSurrogate(next)) to avoid
re-processing the low surrogate and to match the identical behavior in toAnsi();
keep the comment minimal and placed immediately before the if
(Character.isHighSurrogate(...)) block.
🪄 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: 3fef2ce1-47d4-46e9-afff-508f7bf8ee1e
📒 Files selected for processing (4)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/ByteArrayBuilder.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java
| @Test | ||
| public void testAppendIntNegative() { | ||
| ByteArrayBuilder buf = new ByteArrayBuilder(); | ||
| buf.appendInt(-1); | ||
| assertEquals("-1", buf.toStringUtf8()); | ||
|
|
||
| buf.reset(); | ||
| buf.appendInt(-128); | ||
| assertEquals("-128", buf.toStringUtf8()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if appendInt handles Integer.MIN_VALUE correctly
# Check the implementation to see if it falls back to Integer.toString for MIN_VALUE
ast-grep --pattern 'public ByteArrayBuilder appendInt(int $VAL) {
$$$
}'Repository: jline/jline3
Length of output: 2373
🏁 Script executed:
# Search the test file for Integer.MIN_VALUE
cat terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java | grep -n "MIN_VALUE\|Integer\."Repository: jline/jline3
Length of output: 38
🏁 Script executed:
# Show the complete test file to understand test coverage
wc -l terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.javaRepository: jline/jline3
Length of output: 124
🏁 Script executed:
# Search for any other tests of appendInt in the file
rg "testAppendInt" terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java -A 5Repository: jline/jline3
Length of output: 651
🏁 Script executed:
# View the complete test file
cat -n terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.javaRepository: jline/jline3
Length of output: 9377
Add test for Integer.MIN_VALUE edge case in appendInt.
The negative test covers -1 and -128, but appendInt(Integer.MIN_VALUE) causes infinite recursion. When appendInt(-2147483648) is called, it appends '-' and recursively calls appendInt(-(-2147483648)). Due to Java's two's complement overflow, -Integer.MIN_VALUE == Integer.MIN_VALUE, causing unbounded recursion and stack overflow.
Add this test case:
`@Test`
void testAppendIntMinValue() {
ByteArrayBuilder buf = new ByteArrayBuilder();
buf.appendInt(Integer.MIN_VALUE);
assertEquals("-2147483648", buf.toStringUtf8());
}The implementation should either handle Integer.MIN_VALUE as a special case or use Integer.toString() for all negative values to avoid the overflow.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 74-74: Remove this 'public' modifier.
🤖 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/ByteArrayBuilderTest.java` around
lines 73 - 82, Add a unit test for the Integer.MIN_VALUE edge case and fix
ByteArrayBuilder.appendInt to avoid infinite recursion: add the test method
testAppendIntMinValue that calls buf.appendInt(Integer.MIN_VALUE) and asserts
"-2147483648", then update the implementation of ByteArrayBuilder.appendInt to
handle Integer.MIN_VALUE specially (e.g., detect value == Integer.MIN_VALUE and
append the literal "-2147483648") or avoid negating negatives by delegating to
Integer.toString(value) for negative values instead of calling
appendInt(-value), so the recursion/overflow is prevented.
- Fix appendInt(Integer.MIN_VALUE) infinite recursion in ByteArrayBuilder - Eliminate code duplication by having toAnsi() delegate to toAnsiBytes() - Remove unused StringBuilder helper methods (attr, attrInt, attrRgb, attrIdx) - Add clarifying comment on surrogate pair skip - Add test for Integer.MIN_VALUE edge case
- Extract toAnsiBytes() into smaller methods to reduce cognitive complexity (emitStyleChange, appendDecorationAttrsB, appendColorB, appendRoundedColorB, usedBasicFgColor, appendBoldFaintB, emitAltCharset, emitUtf8Char) - Use while loop instead of for to avoid modifying loop counter (S127) - Remove public modifiers from test class and methods (S5786)
- Reduce appendDecorationAttrsB cognitive complexity using table-driven loop (S3776) - Reduce appendColorB params from 13 to 7 using boolean isForeground (S107) - Reduce appendRoundedColorB params from 9 to 7 using boolean isForeground (S107)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
terminal/src/main/java/org/jline/utils/ByteArrayBuilder.java (2)
220-247: Minor:Appendablecontract requires null handling.Per the
Appendableinterface contract,append(null)should append the literal string"null". The current implementation throws NPE. While the intended caller (Curses.tputs) never passes null, this deviates from the interface specification.🔧 Optional fix for interface compliance
`@Override` public Appendable append(CharSequence csq) { + if (csq == null) { + csq = "null"; + } int len = csq.length(); ensureCapacity(count + len); for (int i = 0; i < len; i++) { buffer[count++] = (byte) csq.charAt(i); } return this; } `@Override` public Appendable append(CharSequence csq, int start, int end) { + if (csq == null) { + csq = "null"; + } int len = end - start;🤖 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/ByteArrayBuilder.java` around lines 220 - 247, The AsciiAppendable implementation violates the Appendable contract by not handling null; update AsciiAppendable.append(CharSequence) and append(CharSequence, int, int) to treat a null CharSequence as the four-character literal "null" before proceeding (i.e., if csq == null, set csq = "null"), and ensure the substring overload uses that fallback before computing start/end and writing bytes; keep append(char) unchanged. This ensures methods in class AsciiAppendable conform to the Appendable interface.
45-52: Consider guarding against integer overflow in capacity calculation.The left-shift
buffer.length << 1overflows to negative whenbuffer.length > 2^30. While unlikely for terminal output, a defensive approach would handle this:🛡️ Optional defensive fix
private void ensureCapacity(int minCapacity) { if (minCapacity > buffer.length) { - int newCapacity = Math.max(buffer.length << 1, minCapacity); + int doubled = buffer.length << 1; + int newCapacity = Math.max(doubled > 0 ? doubled : Integer.MAX_VALUE, minCapacity); byte[] newBuffer = new byte[newCapacity]; System.arraycopy(buffer, 0, newBuffer, 0, count); buffer = newBuffer; } }🤖 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/ByteArrayBuilder.java` around lines 45 - 52, The ensureCapacity method can overflow when computing buffer.length << 1; update ByteArrayBuilder.ensureCapacity to compute the doubled size using a long (e.g., long doubleCap = (long) buffer.length * 2) and then clamp to Integer.MAX_VALUE if doubleCap exceeds it, compute newCapacity = (int) Math.max(doubleCap, minCapacity), and if newCapacity < minCapacity (or minCapacity is negative) handle by setting newCapacity = minCapacity or throwing an OutOfMemoryError/IllegalArgumentException as appropriate; ensure you still copy count bytes into the new buffer and reference the same buffer and count fields.
🤖 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/ByteArrayBuilder.java`:
- Around line 220-247: The AsciiAppendable implementation violates the
Appendable contract by not handling null; update
AsciiAppendable.append(CharSequence) and append(CharSequence, int, int) to treat
a null CharSequence as the four-character literal "null" before proceeding
(i.e., if csq == null, set csq = "null"), and ensure the substring overload uses
that fallback before computing start/end and writing bytes; keep append(char)
unchanged. This ensures methods in class AsciiAppendable conform to the
Appendable interface.
- Around line 45-52: The ensureCapacity method can overflow when computing
buffer.length << 1; update ByteArrayBuilder.ensureCapacity to compute the
doubled size using a long (e.g., long doubleCap = (long) buffer.length * 2) and
then clamp to Integer.MAX_VALUE if doubleCap exceeds it, compute newCapacity =
(int) Math.max(doubleCap, minCapacity), and if newCapacity < minCapacity (or
minCapacity is negative) handle by setting newCapacity = minCapacity or throwing
an OutOfMemoryError/IllegalArgumentException as appropriate; ensure you still
copy count bytes into the new buffer and reference the same buffer and count
fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 664cb885-ffbd-4df0-b67d-2fe0db9ea771
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/ByteArrayBuilder.javaterminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/src/main/java/org/jline/utils/AttributedCharSequence.java
- terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java
- AsciiAppendable.append(CharSequence) and append(CharSequence, int, int) now treat null as "null" per the Appendable contract - ensureCapacity uses long arithmetic to avoid overflow when doubling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.java (1)
267-270: Remove the per-rendercolorStateallocation.
long[] colorState = {0, 0}creates a new object for everytoAnsiBytes(...)call, which puts some GC pressure back on the exact path this PR is trying to make allocation-light. Keeping foreground/background state as locals in the caller would avoid that extra heap object.🤖 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/AttributedCharSequence.java` around lines 267 - 270, The code allocates a new long[] colorState inside toAnsiBytes(...) on every call; replace that heap allocation with two primitive local long variables (e.g., fgColor and bgColor) used in place of colorState[0] and colorState[1] inside the toAnsiBytes method (and update any internal references to colorState accordingly) so no per-render long[] is created; keep the method signature the same and ensure any logic that previously mutated colorState now mutates the two primitive locals.
🤖 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/AttributedCharSequence.java`:
- Around line 246-249: The public AttributedCharSequence.toAnsi(int colors,
ForceMode force, ColorPalette palette, String altIn, String altOut) must
preserve direct UTF-16 construction rather than round-tripping through
ByteArrayBuilder.toStringUtf8(); change to build the result on a StringBuilder
(or use the existing char/char[] rendering path) within toAnsi and only use
ByteArrayBuilder for internal/terminal byte output paths; update to call the
internal toAnsiBytes variant only for byte-oriented APIs and keep toAnsi
returning the String produced directly to avoid unpaired-surrogate replacement
(refer to method names toAnsi, toAnsiBytes and ByteArrayBuilder.toStringUtf8
when locating the change).
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/utils/AttributedCharSequence.java`:
- Around line 267-270: The code allocates a new long[] colorState inside
toAnsiBytes(...) on every call; replace that heap allocation with two primitive
local long variables (e.g., fgColor and bgColor) used in place of colorState[0]
and colorState[1] inside the toAnsiBytes method (and update any internal
references to colorState accordingly) so no per-render long[] is created; keep
the method signature the same and ensure any logic that previously mutated
colorState now mutates the two primitive locals.
🪄 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: 3f9f204f-61fc-41ca-8870-7580f16f9927
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/ByteArrayBuilder.javaterminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/src/main/java/org/jline/utils/ByteArrayBuilder.java
- terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java
| public String toAnsi(int colors, ForceMode force, ColorPalette palette, String altIn, String altOut) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| ByteArrayBuilder buf = new ByteArrayBuilder(); | ||
| toAnsiBytes(buf, colors, force, palette, altIn, altOut); | ||
| return buf.toStringUtf8(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java, what happens when a String containing an unpaired surrogate is converted to UTF-8 bytes and then decoded back with StandardCharsets.UTF_8? Does the original UTF-16 character sequence survive unchanged?
💡 Result:
No, the original UTF-16 character sequence containing an unpaired surrogate does not survive unchanged when converted to UTF-8 bytes using String.getBytes(StandardCharsets.UTF_8) and then decoded back with new String(bytes, StandardCharsets.UTF_8). When encoding, Java's UTF-8 encoder treats unpaired surrogates as unmappable characters (since surrogates are not valid standalone Unicode code points) and replaces each with the default replacement byte sequence for UTF-8, which is the 1-byte sequence for '?' (0x3F). This produces invalid UTF-8 if misinterpreted, but the decoder handles it. When decoding those bytes back, new String(bytes, StandardCharsets.UTF_8) interprets the 0x3F bytes as the Unicode character '?' (U+003F), replacing the original surrogate char(s). Thus, the resulting String contains '?' char(s) instead of the original surrogate value(s), so the sequence changes. This behavior is consistent across Java versions, including JDK 24+ as of 2026.
Citations:
- 1: https://stackoverflow.com/questions/31419860/java-utf-8-byte-length-of-a-single-code-point-surrogates-again
- 2: https://stackoverflow.com/questions/32741004/issue-with-java-string-getbytes-method
- 3: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/String.html
- 4: https://docs.oracle.com/javase/8/docs/api/java/lang/String.html
- 5: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/String.html
- 6: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/charset/CharsetEncoder.html
Preserve direct-string construction in the public toAnsi() method.
The new implementation delegates to ByteArrayBuilder.toStringUtf8(), which encodes to UTF-8 bytes and decodes back with StandardCharsets.UTF_8. This round-trip silently replaces unpaired surrogates with '?' (U+003F) instead of preserving the original UTF-16 sequence. While rare in terminal output, this is a breaking behavior change for the public API. Either keep the byte renderer for terminal output but preserve a direct StringBuilder path here, or add a regression test covering this edge case before switching the public 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/utils/AttributedCharSequence.java` around
lines 246 - 249, The public AttributedCharSequence.toAnsi(int colors, ForceMode
force, ColorPalette palette, String altIn, String altOut) must preserve direct
UTF-16 construction rather than round-tripping through
ByteArrayBuilder.toStringUtf8(); change to build the result on a StringBuilder
(or use the existing char/char[] rendering path) within toAnsi and only use
ByteArrayBuilder for internal/terminal byte output paths; update to call the
internal toAnsiBytes variant only for byte-oriented APIs and keep toAnsi
returning the String produced directly to avoid unpaired-surrogate replacement
(refer to method names toAnsi, toAnsiBytes and ByteArrayBuilder.toStringUtf8
when locating the change).
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @gnodet. The following files were modified: * `terminal/src/main/java/org/jline/utils/AttributedCharSequence.java` * `terminal/src/main/java/org/jline/utils/ByteArrayBuilder.java` * `terminal/src/main/java/org/jline/utils/Display.java` These files were ignored: * `terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java`
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.java (1)
239-242:⚠️ Potential issue | 🟡 MinorKeep the public
toAnsi()path UTF-16-based.This
StringAPI now round-trips through UTF-8 bytes, so lone surrogate code units no longer survive unchanged. That is a behavior change for a public method even if the byte-oriented terminal path is fine. Please keeptoAnsi(...)on a directStringBuilder/char path and reservetoAnsiBytes(...)for byte-oriented callers.🤖 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/AttributedCharSequence.java` around lines 239 - 242, The public String-returning method toAnsi(int colors, ForceMode force, ColorPalette palette, String altIn, String altOut) must preserve lone surrogate code units by building a Java String directly rather than round-tripping through UTF-8 bytes; change its implementation to use a StringBuilder (or a char-oriented helper, e.g., toAnsiChars or toAnsi(StringBuilder,...)) and have that helper produce chars appended to the StringBuilder and return builder.toString(), leaving toAnsiBytes(...) and ByteArrayBuilder for byte-oriented callers only so existing byte paths remain unchanged.
🤖 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/AttributedCharSequence.java`:
- Around line 444-451: The code currently only emits true-color SGR sequences
when colors >= HIGH_COLORS, ignoring ForceMode.ForceTrueColors; update the
rgb-paths so the ForceTrueColors flag overrides the colors check: in the blocks
handling (colorValue & rgbFlag) (and the similar occurrences around the other
ranges noted) change the decision to choose attrRgbB(...) when force ==
ForceMode.ForceTrueColors OR colors >= HIGH_COLORS, otherwise call
appendRoundedColorB(...); likewise ensure usedBasicFgColor(...) and the
bold-as-bright fallback treat ForceTrueColors as an override of the low-color
checks so forcing true colors always emits 38;2/48;2 regardless of the colors
value.
---
Duplicate comments:
In `@terminal/src/main/java/org/jline/utils/AttributedCharSequence.java`:
- Around line 239-242: The public String-returning method toAnsi(int colors,
ForceMode force, ColorPalette palette, String altIn, String altOut) must
preserve lone surrogate code units by building a Java String directly rather
than round-tripping through UTF-8 bytes; change its implementation to use a
StringBuilder (or a char-oriented helper, e.g., toAnsiChars or
toAnsi(StringBuilder,...)) and have that helper produce chars appended to the
StringBuilder and return builder.toString(), leaving toAnsiBytes(...) and
ByteArrayBuilder for byte-oriented callers only so existing byte paths remain
unchanged.
🪄 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: 0e24e8e7-4e3d-4101-92c8-8eb34e1479be
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/ByteArrayBuilder.javaterminal/src/main/java/org/jline/utils/Display.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/main/java/org/jline/utils/Display.java
| if ((colorValue & rgbFlag) != 0) { | ||
| int r = (int) (colorValue >> (colorExp + 16)) & 0xFF; | ||
| int g = (int) (colorValue >> (colorExp + 8)) & 0xFF; | ||
| int b = (int) (colorValue >> colorExp) & 0xFF; | ||
| if (colors >= HIGH_COLORS) { | ||
| return attrRgbB(buf, csiPrefix, r, g, b, first); | ||
| } | ||
| return appendRoundedColorB(buf, palette.round(r, g, b), isForeground, colors, force, palette, first); |
There was a problem hiding this comment.
ForceTrueColors no longer actually forces RGB output.
Both color paths still require colors >= HIGH_COLORS before emitting 38;2/48;2, so callers that explicitly pass ForceMode.ForceTrueColors with colors set to 8 or 256 are still rounded down to indexed/basic SGR. usedBasicFgColor(...) has the same guard, so the bold-as-bright fallback can still fire under ForceTrueColors.
Suggested fix
- if (colors >= HIGH_COLORS) {
+ if (force == ForceMode.ForceTrueColors || colors >= HIGH_COLORS) {
return attrRgbB(buf, csiPrefix, r, g, b, first);
}
return appendRoundedColorB(buf, palette.round(r, g, b), isForeground, colors, force, palette, first);
@@
- if (colors >= HIGH_COLORS && force == ForceMode.ForceTrueColors) {
+ if (force == ForceMode.ForceTrueColors) {
int col = palette.getColor(rounded);
return attrRgbB(buf, csiPrefix, (col >> 16) & 0xFF, (col >> 8) & 0xFF, col & 0xFF, first);
}
@@
- && !(colors >= HIGH_COLORS && force == ForceMode.ForceTrueColors)
+ && force != ForceMode.ForceTrueColors
&& force != ForceMode.Force256Colors;Also applies to: 485-488, 528-531
🤖 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/AttributedCharSequence.java` around
lines 444 - 451, The code currently only emits true-color SGR sequences when
colors >= HIGH_COLORS, ignoring ForceMode.ForceTrueColors; update the rgb-paths
so the ForceTrueColors flag overrides the colors check: in the blocks handling
(colorValue & rgbFlag) (and the similar occurrences around the other ranges
noted) change the decision to choose attrRgbB(...) when force ==
ForceMode.ForceTrueColors OR colors >= HIGH_COLORS, otherwise call
appendRoundedColorB(...); likewise ensure usedBasicFgColor(...) and the
bold-as-bright fallback treat ForceTrueColors as an override of the low-color
checks so forcing true colors always emits 38;2/48;2 regardless of the colors
value.
ForceMode only affects how indexed/palette colors are rendered, not whether direct RGB values use truecolor encoding. Document this on the enum values and add inline comments at the two key branch points in appendColorB and appendRoundedColorB.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.java (1)
269-270: Avoid the per-renderlong[2]allocation here.This path is now performance-sensitive, and Line 270 reintroduces heap churn on every render. Tracking foreground/background as locals and returning the updated state from
emitStyleChange(...)would keep the byte renderer closer to allocation-free.🤖 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/AttributedCharSequence.java` around lines 269 - 270, The code currently allocates a new long[] colorState = {0, 0} per render in AttributedCharSequence (line with style and colorState), causing heap churn; change this to use primitive local variables (e.g., long fg = 0, bg = 0) instead of allocating the array and update emitStyleChange(...) to accept and return the updated fg/bg state (or return a small packed long) so the renderer no longer allocates on each call; update all calls to emitStyleChange(...) to consume the returned state and remove the long[] allocation.
🤖 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/AttributedCharSequence.java`:
- Around line 269-270: The code currently allocates a new long[] colorState =
{0, 0} per render in AttributedCharSequence (line with style and colorState),
causing heap churn; change this to use primitive local variables (e.g., long fg
= 0, bg = 0) instead of allocating the array and update emitStyleChange(...) to
accept and return the updated fg/bg state (or return a small packed long) so the
renderer no longer allocates on each call; update all calls to
emitStyleChange(...) to consume the returned state and remove the long[]
allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ad609cf-8410-45c2-979c-ec05512fe84f
📒 Files selected for processing (1)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.java
1 participant
Summary
Implements #1746: optimizes the terminal output rendering hot path by introducing a
ByteArrayBuilderutility and a direct byte-level rendering path.ByteArrayBuilder— reusable byte buffer withappendInt()(digits → bytes withoutInteger.toString()),appendAscii()(bypass charset encoding),appendUtf8()(fast-path ASCII),csi()(ESC[), andasAsciiAppendable()forCurses.tputs()integrationAttributedCharSequence.toAnsiBytes()— parallel implementation oftoAnsi()that writes ANSI escape sequences directly as UTF-8 bytes to aByteArrayBuilder, avoiding all intermediate String allocationstoAnsi()optimization — eliminates String concatenation in color codes (e.g.,"38;2;" + r + ";" + g + ";" + b→sb.append()chaining withattrRgb()/attrIdx()/attrInt()helpers)Displaybyte path — for UTF-8 terminals, accumulates all output (styled text viatoAnsiBytes()+ cursor positioning viaCurses.tputs()) in a singleByteArrayBuilderper update cycle, then writes directly toterminal.output(), bypassingPrintWriter/OutputStreamWriterentirelysubstituteChar()andisBoxDrawing()to share box-drawing character logic betweentoAnsi()andtoAnsiBytes()Expected impact
Non-breaking guarantees
terminal.writer()still returnsPrintWriter— unchangedAttributedString.toAnsi()still works — unchanged (optimized internally)Displaypublic API — unchangedCloses #1746
Test plan
ByteArrayBuilderTestwith 18 tests covering all builder methods, UTF-8 encoding, ANSI sequence construction, buffer growth, and Appendable adaptertoAnsiBytes()produces identical output totoAnsi()for RGB colors, indexed colors, styled text, and multi-byte charactersAttributedCharSequenceTest,AttributedStringTest,AttributedStringBuilderTest, andDisplayTestpass unchangedSummary by CodeRabbit
New Features
Refactor
Tests