Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughRefactored ANSI rendering to range- and state-aware APIs, exposed reusable style-buffer access, added allocation-free diff primitives and grapheme-break iterator reuse, overhauled Display.update to reduce allocations and add intra-line skip/overwrite optimizations, and added tests plus a micro-benchmark. Changes
Sequence DiagramsequenceDiagram
participant Display as "Display (renderer)"
participant Diff as "DiffHelper"
participant ACSeq as "AttributedCharSequence"
participant WC as "WCWidth"
participant Terminal as "Terminal/TTY"
Display->>Diff: diff(oldLinesRange, newLinesRange)
Diff-->>Display: prefix/suffix lengths (range result)
Display->>WC: columnLength(rangeStart, rangeEnd)
WC-->>Display: visible width
Display->>ACSeq: toAnsiBytes(rangeStart, rangeEnd, colors, state)
ACSeq-->>Display: ANSI bytes + updated state
Display->>Terminal: move cursor & write bytes (rawPrint)
Terminal-->>Display: acknowledge / cursor pos
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
terminal/src/main/java/org/jline/utils/Display.java (1)
667-693: Consider returning an empty array sentinel instead of null.SonarCloud flags returning
nullat line 692. The current pattern requires callers to null-check. While this is existing behavior, a sentinel value (e.g.,{-1, -1, 0}) could simplify caller code.However, this would require updating the caller at line 337 and potentially other callers. Given this is a performance-focused PR, preserving the existing null-check pattern is acceptable.
🤖 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/Display.java` around lines 667 - 693, The method longestCommon currently returns null when no common substring is found; change it to return a sentinel int[] (e.g., new int[]{-1, -1, 0}) instead of null, assign that sentinel to lcsResult before returning, and update all callers (including the caller referenced at line 337 and any other places that check for null) to check for the sentinel (start indices == -1 or length == 0) rather than a null reference to avoid NPEs and simplify control flow.terminal/src/main/java/org/jline/utils/WCWidth.java (1)
533-541: Consider usingsuper.clone()or a copy factory method.The
clone()implementation manually copies fields rather than callingsuper.clone(). While this works correctly since the class isfinaland package-private, it doesn't follow the standardCloneablecontract. SonarCloud flags this as a potential issue.Since
BreakIterator.setText(CharacterIterator)callsclone()internally, this implementation is functionally correct. However, a copy factory or usingsuper.clone()would be more conventional.♻️ Alternative using super.clone()
`@Override` public Object clone() { - CharSequenceCharacterIterator copy = new CharSequenceCharacterIterator(); - copy.cs = this.cs; - copy.begin = this.begin; - copy.end = this.end; - copy.pos = this.pos; - return copy; + try { + return super.clone(); + } catch (CloneNotSupportedException e) { + throw new AssertionError(e); // Cannot happen for Cloneable + } }Note: The class would need to implement
Cloneable:- static final class CharSequenceCharacterIterator implements CharacterIterator { + static final class CharSequenceCharacterIterator implements CharacterIterator, Cloneable {🤖 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 533 - 541, The clone() method in CharSequenceCharacterIterator manually constructs a copy by copying fields instead of following the standard Cloneable contract; replace this manual copying with a conventional approach: either implement Cloneable and call super.clone() inside clone() (ensuring CharSequenceCharacterIterator is cloneable and returns a proper CharSequenceCharacterIterator) or add a copy factory method (e.g., a private copy constructor or a public static copyOf(CharSequenceCharacterIterator)) and have clone() delegate to that factory; update the clone() implementation in CharSequenceCharacterIterator accordingly and ensure the returned object is the correct concrete type.terminal/src/test/java/org/jline/utils/DisplayBenchmarkTest.java (1)
30-127: Consider adding a minimal assertion for test validity.This benchmark test has no assertions, which SonarCloud flags. While benchmarks typically focus on measurement rather than verification, adding a minimal assertion ensures the test fails if something goes catastrophically wrong rather than silently producing invalid measurements.
♻️ Add minimal assertion
System.out.printf(" Heap growth: %,d bytes%n", memAfter - memBefore); } + // Minimal assertion to ensure the benchmark actually ran + org.junit.jupiter.api.Assertions.assertTrue(true, "Benchmark completed successfully"); }Or more meaningfully, verify the display state is valid after updates:
// After the benchmark loop org.junit.jupiter.api.Assertions.assertNotNull(display, "Display should remain valid");🤖 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/DisplayBenchmarkTest.java` around lines 30 - 127, The test benchmarkDisplayUpdate contains no assertions—add a minimal JUnit assertion at the end of the test to ensure the Display instance and its state remain valid after updates (e.g., assert display is not null and/or verify the current buffer equals the expected screen A/C), referencing the Display variable named display and the benchmarkDisplayUpdate test method so the test will fail on catastrophic errors rather than silently passing.
🤖 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/DiffHelper.java`:
- Around line 110-163: Remove the dangling/duplicate Javadoc that was left
before the new range-based diff method: delete the old comment block that
described "Compute a list of difference between two lines" (the Javadoc that no
longer precedes any declaration) so only the proper Javadoc remains for the new
static void diff(AttributedString, int, int, AttributedString, int, int, int[])
and the existing public static List<Diff> diff(AttributedString,
AttributedString) stays correctly documented.
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/utils/Display.java`:
- Around line 667-693: The method longestCommon currently returns null when no
common substring is found; change it to return a sentinel int[] (e.g., new
int[]{-1, -1, 0}) instead of null, assign that sentinel to lcsResult before
returning, and update all callers (including the caller referenced at line 337
and any other places that check for null) to check for the sentinel (start
indices == -1 or length == 0) rather than a null reference to avoid NPEs and
simplify control flow.
In `@terminal/src/main/java/org/jline/utils/WCWidth.java`:
- Around line 533-541: The clone() method in CharSequenceCharacterIterator
manually constructs a copy by copying fields instead of following the standard
Cloneable contract; replace this manual copying with a conventional approach:
either implement Cloneable and call super.clone() inside clone() (ensuring
CharSequenceCharacterIterator is cloneable and returns a proper
CharSequenceCharacterIterator) or add a copy factory method (e.g., a private
copy constructor or a public static copyOf(CharSequenceCharacterIterator)) and
have clone() delegate to that factory; update the clone() implementation in
CharSequenceCharacterIterator accordingly and ensure the returned object is the
correct concrete type.
In `@terminal/src/test/java/org/jline/utils/DisplayBenchmarkTest.java`:
- Around line 30-127: The test benchmarkDisplayUpdate contains no assertions—add
a minimal JUnit assertion at the end of the test to ensure the Display instance
and its state remain valid after updates (e.g., assert display is not null
and/or verify the current buffer equals the expected screen A/C), referencing
the Display variable named display and the benchmarkDisplayUpdate test method so
the test will fail on catastrophic errors rather than silently passing.
🪄 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: 727fc339-d267-47e2-a2eb-975549b3f142
📒 Files selected for processing (5)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/DiffHelper.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/utils/DisplayBenchmarkTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Display.java`:
- Around line 513-516: The condition comparing currentPos to columns uses
absolute cursor position instead of the per-line remaining width, so
deleteChars(...) optimization is skipped for non-first lines; modify the check
in the block that computes suffixWidth (uses oldLine.columnLength(...)) to
compute the visual column within the current line (e.g., visualPos = currentPos
- startVisualOfCurrentLine or compute remaining = columns - visualPos) and use
remaining columns (not absolute currentPos) when deciding to call
moveVisualCursorTo(...) and deleteChars(...); update references in this block to
use the per-line visual offset rather than currentPos vs columns.
🪄 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: a4ee91f2-9468-4f35-a298-fcbc0b39d48c
📒 Files selected for processing (2)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.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/AttributedCharSequence.java
gnodet
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
terminal/src/main/java/org/jline/utils/Display.java (1)
525-529:⚠️ Potential issue | 🟠 MajorUse a line-relative column when gating
deleteChars(...).At Line 528,
currentPosis absolute, butcolumnsis the width of the current row. On any row after the first this makes the condition almost always false, so the delete fast path is skipped even when the suffix fits. Downstream, that turns second-and-later-line deletes back into clear/repaint work.Suggested fix
- if (currentPos + suffixWidth < columns) { + if ((currentPos - curCol) + suffixWidth < columns) { moveVisualCursorTo(currentPos); ensureDefaultAnsiStyle(); if (deleteChars(deleteWidth)) {🤖 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/Display.java` around lines 525 - 529, The condition gating the fast delete path uses absolute currentPos against columns (the per-row width), causing it to fail on rows beyond the first; compute a line-relative column (e.g. int lineCol = currentPos % columns or currentPos - rowStart) and use that when checking suffixWidth and moving the visual cursor before calling deleteChars(...). Update references around hasEqSuffix, oldLine.columnLength(...), moveVisualCursorTo(currentPos) to use the computed lineCol (or compute the remaining space as columns - lineCol) so the suffix-fit check and cursor move are relative to the current row.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/utils/Display.java (1)
829-834: Either cost-check CUP here or relax the heuristic/comment.This branch now takes
cursor_addressfor every diagonal move, but the comment says “when it's shorter than relative”. For small deltas, relativecursor_*sequences can still be cheaper, so this can increase output bytes on local edits even though the PR is trying to minimize them.🤖 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/Display.java` around lines 829 - 834, The current branch unconditionally uses absolute cursor_address (puts(Capability.cursor_address, l1, c1)) for diagonal moves (when hasCursorAddress && l0 != l1 && c0 != c1) despite the comment that it should only be used "when it's shorter than relative"; modify this to compute and compare the byte-cost of the absolute CUP sequence versus the combined relative sequences using the existing capability outputs for cursor_up/down/left/right (or their cached byte lengths) and only call puts(Capability.cursor_address, l1, c1) and set cursorPos = i1 when the CUP cost is strictly less (otherwise fall back to emitting the shorter relative moves), or if you prefer to keep the heuristic, update the comment to remove the "when it's shorter than relative" claim and explain the chosen heuristic.
🤖 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/utils/Display.java`:
- Around line 525-529: The condition gating the fast delete path uses absolute
currentPos against columns (the per-row width), causing it to fail on rows
beyond the first; compute a line-relative column (e.g. int lineCol = currentPos
% columns or currentPos - rowStart) and use that when checking suffixWidth and
moving the visual cursor before calling deleteChars(...). Update references
around hasEqSuffix, oldLine.columnLength(...), moveVisualCursorTo(currentPos) to
use the computed lineCol (or compute the remaining space as columns - lineCol)
so the suffix-fit check and cursor move are relative to the current row.
---
Nitpick comments:
In `@terminal/src/main/java/org/jline/utils/Display.java`:
- Around line 829-834: The current branch unconditionally uses absolute
cursor_address (puts(Capability.cursor_address, l1, c1)) for diagonal moves
(when hasCursorAddress && l0 != l1 && c0 != c1) despite the comment that it
should only be used "when it's shorter than relative"; modify this to compute
and compare the byte-cost of the absolute CUP sequence versus the combined
relative sequences using the existing capability outputs for
cursor_up/down/left/right (or their cached byte lengths) and only call
puts(Capability.cursor_address, l1, c1) and set cursorPos = i1 when the CUP cost
is strictly less (otherwise fall back to emitting the shorter relative moves),
or if you prefer to keep the heuristic, update the comment to remove the "when
it's shorter than relative" claim and explain the chosen heuristic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e00fbaf0-6589-4b1d-89fb-d83904aea8c0
📒 Files selected for processing (3)
terminal/src/main/java/org/jline/utils/DiffHelper.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/test/java/org/jline/utils/DisplayTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/main/java/org/jline/utils/DiffHelper.java
Contributor
|
Is there a specific reason why AttributedCharSequence#toAnsiBytes()'s state of long[4] is defined as Considering that color information is included in the style, it could be simplified to To then avoid checking colors at all (bypassing most of that function) you could move all color attributes t0 the upper 13 nibbles of the state. By then only checking the upper 13 nibbles of the old and new state all the color checks could often be bypassed. This would require the "hidden" state to be moved down a nibble, and the color "set" states to be moved up a nibble. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.java (1)
953-968: Minor Javadoc inaccuracy: method still allocates iterators.The Javadoc states this is an "allocation-free alternative" but the method allocates a
BreakIteratorandCharSequenceCharacterIteratoron each call. The allocation-free path is the package-private overload that accepts pre-allocated iterators.Consider updating the Javadoc to clarify that this avoids substring allocation, not all allocation:
📝 Suggested Javadoc clarification
/** - * Returns the display width in columns for a range of this attributed string. - * - * <p>This is an allocation-free alternative to creating a subSequence and calling - * {`@link` `#columnLength`(Terminal)} on it.</p> + * Returns the display width in columns for a range of this attributed string + * without creating a subSequence. * * `@param` terminal the terminal to query for grapheme cluster mode, or {`@code` null} * `@param` rangeStart start index in this sequence (inclusive)🤖 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 953 - 968, Update the Javadoc for AttributedCharSequence.columnLength(Terminal, int, int) to remove the claim that it is "allocation-free" and instead state that it avoids creating a substring but still allocates helper iterators (e.g., BreakIterator and WCWidth.CharSequenceCharacterIterator); mention that the truly allocation-free path is the package-private overload columnLength(Terminal, BreakIterator, CharSequenceCharacterIterator, int, int) which accepts pre-allocated iterators. Ensure the text references BreakIterator and WCWidth.CharSequenceCharacterIterator so readers know which allocations occur and which overload to use to avoid them.
🤖 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 953-968: Update the Javadoc for
AttributedCharSequence.columnLength(Terminal, int, int) to remove the claim that
it is "allocation-free" and instead state that it avoids creating a substring
but still allocates helper iterators (e.g., BreakIterator and
WCWidth.CharSequenceCharacterIterator); mention that the truly allocation-free
path is the package-private overload columnLength(Terminal, BreakIterator,
CharSequenceCharacterIterator, int, int) which accepts pre-allocated iterators.
Ensure the text references BreakIterator and
WCWidth.CharSequenceCharacterIterator so readers know which allocations occur
and which overload to use to avoid them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a8d4b34-61fd-4122-ae0c-310989b1b34a
📒 Files selected for processing (2)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/Display.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/DiffHelper.java`:
- Around line 155-170: In commonSuffixLength(AttributedString text1, int e1,
AttributedString text2, int e2, int n) the unconditional rollback to
startHiddenRange when the scan ends in a hidden run is too aggressive; change
the final rollback so it only happens if the mismatch that terminated the loop
is itself inside a hidden region in both strings (e.g., check text1.isHidden(e1
- commonEnd - 1) && text2.isHidden(e2 - commonEnd - 1) before assigning
commonEnd = startHiddenRange), leaving bona fide equal visible suffixes intact.
🪄 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: 9f6c1d3a-6cc5-45dc-92b8-50f4806ba831
📒 Files selected for processing (1)
terminal/src/main/java/org/jline/utils/DiffHelper.java
gnodet
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/org/jline/utils/DisplayBenchmarkTest.java`:
- Around line 19-22: The documentation comment in DisplayBenchmarkTest (class
DisplayBenchmarkTest) has a typo in the Maven run command; update the comment
text to replace "./mvx mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest"
with the correct "./mvnw mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest"
(i.e., change "./mvx" to "./mvnw") so the run instructions are accurate.
🪄 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: 4bdf71a8-d22b-412c-8416-e3093b1233a8
📒 Files selected for processing (9)
terminal/src/main/java/org/jline/utils/AttributedCharSequence.javaterminal/src/main/java/org/jline/utils/AttributedString.javaterminal/src/main/java/org/jline/utils/AttributedStringBuilder.javaterminal/src/main/java/org/jline/utils/DiffHelper.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/main/java/org/jline/utils/WCWidth.javaterminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.javaterminal/src/test/java/org/jline/utils/DisplayBenchmarkTest.javaterminal/src/test/java/org/jline/utils/DisplayTest.java
✅ Files skipped from review due to trivial changes (1)
- terminal/src/test/java/org/jline/utils/ByteArrayBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- terminal/src/main/java/org/jline/utils/AttributedString.java
- terminal/src/main/java/org/jline/utils/DiffHelper.java
- terminal/src/main/java/org/jline/utils/AttributedCharSequence.java
Comment on lines
+19
to
+22
| /** | ||
| * Micro-benchmark for Display.update to measure throughput. | ||
| * Run with: ./mvx mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest | ||
| */ |
Contributor
There was a problem hiding this comment.
Typo in documentation comment.
The run command contains a typo: ./mvx should be ./mvnw (Maven wrapper).
📝 Suggested fix
-* Run with: ./mvx mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest
+* Run with: ./mvnw test -B -pl terminal -Dtest=DisplayBenchmarkTest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Micro-benchmark for Display.update to measure throughput. | |
| * Run with: ./mvx mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest | |
| */ | |
| /** | |
| * Micro-benchmark for Display.update to measure throughput. | |
| * Run with: ./mvnw test -B -pl terminal -Dtest=DisplayBenchmarkTest | |
| */ |
🤖 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/DisplayBenchmarkTest.java` around
lines 19 - 22, The documentation comment in DisplayBenchmarkTest (class
DisplayBenchmarkTest) has a typo in the Maven run command; update the comment
text to replace "./mvx mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest"
with the correct "./mvnw mvn test -B -pl terminal -Dtest=DisplayBenchmarkTest"
(i.e., change "./mvx" to "./mvnw") so the run instructions are accurate.
Key optimizations in Display.update(): - Intra-line skip: when a row has a long unchanged gap between two changed regions, emit a cursor-movement sequence instead of re-sending the unchanged characters, reducing output bytes - Accept AttributedCharSequence (not just AttributedString) to avoid per-frame snapshot copies when the caller already holds an immutable sequence - Fast-path for List<AttributedString> input to skip snapshotLines overhead - Cell-grid update API: flat char[]/long[] arrays with row-equality check that reuses oldLines entries for unchanged rows Allocation reduction in AttributedCharSequence: - Consolidate toAnsiBytes state from long[4] to long[2] (style + altCharset flag) - Color fast-path in emitStyleChange(): skip color extraction when only text decoration attributes changed - Deduplicate columnLength overloads into a delegation chain using bare BreakIterator + CharSequenceCharacterIterator to avoid toString() allocation DiffHelper improvements: - Deduplicate public diff() to delegate to allocation-free variant - Fix commonSuffixLength unconditional hidden-range rollback: make rollback conditional on whether the next character is actually hidden, matching the prefix logic Other: - Add CharSequenceCharacterIterator and resetGraphemeBreakIterator to WCWidth for allocation-free BreakIterator usage - Add @SInCE 4.1.0 tags to new public API methods - Add DisplayBenchmarkTest for update throughput measurement - Add tests for intra-line skip, cell-grid API, and AttributedCharSequence input
|
gnodet
added a commit
to gnodet/jline3
that referenced
this pull request
Apr 16, 2026
Remove public methods and classes added in the recent optimization PRs (jline#1746, jline#1785) that have no callers within JLine: - Make ByteArrayBuilder package-private (only used by Display and AttributedCharSequence internally) - Remove Display.updateSequences() overloads and Display.update(char[], long[], ...) cell-grid method - Remove AttributedCharSequence.columnLength(Terminal, int, int) public 3-arg overload (internal 5-arg version is still used) - Remove Size(Size) copy constructor (Size.copy() already exists) - Remove spurious (short) casts in Size setters that silently truncated values while storing into int fields
This was referenced Apr 16, 2026
gnodet
added a commit
to gnodet/jline3
that referenced
this pull request
Apr 16, 2026
Remove public methods and classes added in the recent optimization PRs (jline#1746, jline#1785) that have no callers within JLine: - Make ByteArrayBuilder package-private (only used by Display and AttributedCharSequence internally) - Remove Display.updateSequences() overloads and Display.update(char[], long[], ...) cell-grid method - Remove AttributedCharSequence.columnLength(Terminal, int, int) public 3-arg overload (internal 5-arg version is still used) - Remove Size(Size) copy constructor (Size.copy() already exists)
gnodet
added a commit
that referenced
this pull request
Apr 16, 2026
Remove public methods and classes added in the recent optimization PRs (#1746, #1785) that have no callers within JLine: - Make ByteArrayBuilder package-private (only used by Display and AttributedCharSequence internally) - Remove Display.updateSequences() overloads and Display.update(char[], long[], ...) cell-grid method - Remove AttributedCharSequence.columnLength(Terminal, int, int) public 3-arg overload (internal 5-arg version is still used) - Remove Size(Size) copy constructor (Size.copy() already exists)
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
2 participants
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
Optimizes
Display.update()to minimize both heap allocation and terminal output bytes.Display.update() optimizations
CUF/CHA) instead of re-sending the unchanged charactersAttributedCharSequence(not justAttributedString) to avoid per-frame snapshot copiessnapshotLinesoverhead when input is already a list ofAttributedStringupdate(char[], long[], int[], ...)overload with flat row-major arrays; reusesoldLinesentries for unchanged rows viarowEqualscheckAttributedCharSequence allocation reduction
toAnsiBytesuseslong[2](style + altCharset) instead oflong[4]emitStyleChange()skips color extraction when(diff & COLOR_BITS) == 0(only text decoration changed)BreakIterator+CharSequenceCharacterIterator, avoidingtoString()allocationDiffHelper
diff(text1, text2)delegates to the allocation-freediff(text1, s1, e1, text2, s2, e2, result)variantcommonSuffixLengthhidden-range rollback was unconditional; now conditional on whether the next character is actually hidden, matching the prefix logicOther
CharSequenceCharacterIteratorandresetGraphemeBreakIteratorinWCWidthfor allocation-freeBreakIteratorusage@since 4.1.0tags on new public API methodsDisplayBenchmarkTestfor update throughput measurementAttributedCharSequenceinputTest plan
mvn test -pl terminal— all 267 tests passtestIntraLineSkipOptimization— verifies cursor movement skips unchanged gapstestCellGridUpdate/testCellGridWithWidths/testCellGridParityWithListUpdate— cell-grid API correctnesstestUpdateWithAttributedCharSequence— AttributedCharSequence input pathi737test — regression check for full-screen updateSummary by CodeRabbit
New Features
Improvements & Optimizations
Tests