Conversation
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the PR! I appreciate the thought behind unifying size access, but I'm not convinced this abstraction carries its weight.
Core concern: no concrete consumer for Sized
The new interface introduces a common type across Terminal, Size, Display, and ScreenTerminal — but there's no code that actually uses Sized as a type. The only consumer is the Size(Sized) constructor, which doesn't add much over the existing new Size(cols, rows) or size.copy(other) patterns.
In practice, these types are never used interchangeably — terminal code always knows whether it's working with a Terminal, a Display, or a Size. There's no method that would benefit from accepting a generic "anything with dimensions" parameter.
The PR adds API surface (a new public interface, final modifiers on existing methods, duplicate getter pairs on Size) without simplifying existing code or enabling new capabilities.
Additional issues if the PR moves forward:
- Making
getColumns()/getRows()finalis a binary-incompatible change for any external code that subclassesSize Sized.javahas broken Javadoc HTML (<li>tags without a<ul>wrapper)- The naming tension between
getWidth()/getHeight()(pixel-oriented) andgetColumns()/getRows()(terminal-standard) adds confusion, especially with both pairs now onSize
Could you share a concrete use case where polymorphism over Sized simplifies existing code or enables something new? That would help evaluate whether the added API surface is justified.
Claude Code on behalf of Guillaume Nodet
📝 WalkthroughWalkthroughIntroduce a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
|
This pull is intended to be part of a (very slow) wider push towards standardizing some names, in this case dimension names. Currently JLine uses width/height and rows/columns (and sometimes "line" as row name) without any clear definition when which name is supposed to be used, see: Builtins: Console: Jansi-Core Reader: Terminal: FFM Terminal: JNI Terminal: Additionally, some classes also have places where they use x/y instead of col/row. Also: Other point: I appreciate that there is a lot of legacy here, and that e.g. the x/y order is perhaps more "rendering oriented" and that the x/y order makes sense in some places. This 1 small pull might seem a bit random on its own, and im sorry for not explaining it further in my initial message (I hope this message explains better where this comes from), but I kept it small as you indicated you prefer smaller pulls. PS: Will will open a PR for some set/getSize related bugs in Swing/WebTerminal later, preferably after this is merged. |
|
#1755 looks like a great step in the right direction concerning some of the things listed above. I still think the Sized interface has merit, though the functions would need to be renamed getRows/Columns to match the new direction. Combined with #1755 the new resize functions & new Size constructor could then also take a Sized instead of a size. Going back to my previous example: |
|
We can revisit once #1755 has been merged. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.java (1)
2111-2116:⚠️ Potential issue | 🔴 CriticalFix compile blocker: wrong
dump(...)argument type.At Line 2115,
dump(screen, cursor)passeslong[][](screen is declared asprivate long[][] screen), but the dump overloads acceptlong[]. This causes a type mismatch compilation error.Minimal fix
- dump(screen, cursor); + dump(dumpBuffer, cursor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java` around lines 2111 - 2116, The call dump(screen, cursor) is passing the 2D field screen (long[][]) to an overload that expects a 1D long[]; replace that call with dump(dumpBuffer, cursor) (using the previously created long[] dumpBuffer) so the argument types match the dump(long[] buffer, int[] cursor) overload and the cursor is filled correctly.
🧹 Nitpick comments (2)
terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java (1)
326-326: Add a comment explaining why this stub method is empty.SonarCloud flags this empty method. Since this is an intentional stub for testing
Terminalinterface default methods, add a brief comment to clarify.📝 Proposed fix
- public void setSize(Sized size) {} + public void setSize(Sized size) { + // Intentional no-op: stub for testing default interface methods + }🤖 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` at line 326, The empty setSize(Sized size) method is an intentional stub used to exercise Terminal interface default-method behavior in tests; update the stub to include a brief comment above the method (e.g., "Intentionally empty: stub implementation for testing default methods of Terminal/Sized") so static analysis (SonarCloud) understands it is deliberate and not a missing implementation—place the comment directly above the setSize(Sized size) method in the anonymous test implementation.builtins/src/main/java/org/jline/builtins/ScreenTerminal.java (1)
1758-1759: Make thesetSize(Sized)null contract explicit.At Line 1759, passing
nullcauses an implicit NPE. Consider either validating withObjects.requireNonNull(...)or returningfalseconsistently for invalid input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java` around lines 1758 - 1759, The setSize(Sized) method currently throws an implicit NPE when passed null; make the null contract explicit by validating the input at the start of setSize(Sized) (either call Objects.requireNonNull(sized, "sized must not be null") to throw a clear NPE or check sized == null and return false) and then forward to the existing setSize(int, int) call; update the method body around setSize(Sized) to perform this check so callers get a consistent, documented behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java`:
- Around line 250-255: In setSize(Sized sz) ensure you call checkClosed() before
mutating the component: move the closed-state check to the top of
SwingTerminal.setSize(Sized) so that checkClosed() runs before invoking
component.setSize(sz) and before calling super.setSize(component), preventing
state changes when the terminal is already closed.
In `@builtins/src/main/java/org/jline/builtins/WebTerminal.java`:
- Around line 211-216: In setSize(Sized sz) ensure the terminal closed-guard
runs before mutating the component: call checkClosed() at the start of
WebTerminal.setSize(Sized) so you validate the terminal state prior to invoking
component.setSize(sz); keep the existing conditional and subsequent
super.setSize(component) behavior (i.e., call checkClosed(), then if
(component.setSize(sz)) { super.setSize(component); }).
In `@builtins/src/test/java/org/jline/builtins/TerminalTest.java`:
- Around line 250-251: The test currently calls
webTerminal.getComponent().setSize(new Size(40, 20)) which mismatches the API
used for swingTerminal and may not exist on TerminalComponent; change the test
to use the same API as swingTerminal by calling webTerminal.setSize(new Size(40,
20)) (or, if TerminalComponent only exposes layout sizing, call
webTerminal.getComponent().setPreferredSize(new Size(40, 20)) and remove the
assertTrue) so the call matches available methods on TerminalComponent/Terminal.
- Around line 270-277: The test wrongly assumes
webTerminal.getComponent().setSize(Size) returns a boolean; replace those
boolean assertions with direct size-validity checks against the terminal bounds
(use ScreenTerminal.MAX_SIZE and the same Size values). For example, instead of
asserting the return of setSize, call the validation logic (e.g., a static
predicate like ScreenTerminal.isValidSize(Size) or inline checks: width/height
>= minimum && <= ScreenTerminal.MAX_SIZE) for each Size used; keep the same Size
inputs and assertions (assertTrue/assertFalse) but applied to the validation
predicate rather than webTerminal.getComponent().setSize(...).
In `@terminal/src/main/java/org/jline/terminal/Terminal.java`:
- Around line 759-765: The change introduced a new abstract method
setSize(Sized) on the Terminal interface which breaks source compatibility for
existing implementations that only implement setSize(Size); to fix, revert to
keeping the original abstract contract by making setSize(Size) the required
method on Terminal (remove the default implementation) and provide
setSize(Sized) as a default helper that delegates to setSize(Size) (i.e.,
implement setSize(Sized) to call setSize((Size) sized) or equivalent), so
existing implementers need not change while new callers can use the Sized
overload; update the Terminal interface methods setSize(Size) and setSize(Sized)
accordingly.
---
Outside diff comments:
In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java`:
- Around line 2111-2116: The call dump(screen, cursor) is passing the 2D field
screen (long[][]) to an overload that expects a 1D long[]; replace that call
with dump(dumpBuffer, cursor) (using the previously created long[] dumpBuffer)
so the argument types match the dump(long[] buffer, int[] cursor) overload and
the cursor is filled correctly.
---
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java`:
- Around line 1758-1759: The setSize(Sized) method currently throws an implicit
NPE when passed null; make the null contract explicit by validating the input at
the start of setSize(Sized) (either call Objects.requireNonNull(sized, "sized
must not be null") to throw a clear NPE or check sized == null and return false)
and then forward to the existing setSize(int, int) call; update the method body
around setSize(Sized) to perform this check so callers get a consistent,
documented behavior.
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`:
- Line 326: The empty setSize(Sized size) method is an intentional stub used to
exercise Terminal interface default-method behavior in tests; update the stub to
include a brief comment above the method (e.g., "Intentionally empty: stub
implementation for testing default methods of Terminal/Sized") so static
analysis (SonarCloud) understands it is deliberate and not a missing
implementation—place the comment directly above the setSize(Sized size) method
in the anonymous test 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: f879dcfc-31b0-427f-aea3-f8dfdf7a9d61
📒 Files selected for processing (17)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.javabuiltins/src/main/java/org/jline/builtins/SwingTerminal.javabuiltins/src/main/java/org/jline/builtins/Tmux.javabuiltins/src/main/java/org/jline/builtins/WebTerminal.javabuiltins/src/test/java/org/jline/builtins/ScreenTerminalTest.javabuiltins/src/test/java/org/jline/builtins/TerminalTest.javaconsole/src/test/java/org/jline/widget/TailTipWidgetsTest.javaterminal/src/main/java/org/jline/terminal/Size.javaterminal/src/main/java/org/jline/terminal/Sized.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/terminal/impl/AbstractPosixTerminal.javaterminal/src/main/java/org/jline/terminal/impl/AbstractWindowsTerminal.javaterminal/src/main/java/org/jline/terminal/impl/DumbTerminal.javaterminal/src/main/java/org/jline/terminal/impl/LineDisciplineTerminal.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/utils/DisplayTest.java
✅ Files skipped from review due to trivial changes (1)
- console/src/test/java/org/jline/widget/TailTipWidgetsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/main/java/org/jline/terminal/Sized.java
| // Deprecation is awkward, as in its current state setSize(Size) would always cause deprecation warnings, | ||
| // because it doesn't auto-map to setSize(Sized) | ||
| // @Deprecated | ||
| // @SuppressWarnings("java:S1133") // Intentional deprecation; removal planned for a future major version | ||
| default void setSize(Size size) { | ||
| setSize((Sized) size); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is adding a new abstract method to an existing public Java interface binary/source compatible with already-compiled implementations?
💡 Result:
No.
- Source compatibility: Not compatible. Any existing class that
implementsthe interface will fail to recompile until it provides an implementation of the new abstract method. - Binary compatibility (already-compiled implementations): In the JLS sense of “will preexisting binaries still link?”, adding a method is binary compatible—old class files that implemented the interface can still load/link. However, if code compiled against the new interface ever tries to call the newly-added method on an old implementation, the call can fail at runtime (typically
AbstractMethodError) because the old class file doesn’t provide that method. [1] - How Java 8+ solves this for interface evolution: Add the new method as a
defaultmethod (with an implementation) to avoid breaking existing implementers. [2]
Sources: [1] Java Language Specification, Binary Compatibility (§13.4.12). [2] Default methods rationale/usage (Java interface evolution).
🏁 Script executed:
# First, let's locate the Terminal.java file
fd -type f -name "Terminal.java" | head -20Repository: jline/jline3
Length of output: 226
🏁 Script executed:
# Then read the specific lines mentioned in the review (759-775 and surrounding context)
if [ -f "terminal/src/main/java/org/jline/terminal/Terminal.java" ]; then
sed -n '750,780p' "terminal/src/main/java/org/jline/terminal/Terminal.java" | cat -n
fiRepository: jline/jline3
Length of output: 1540
Public API compatibility break: new abstract interface method.
Line 775 introduces setSize(Sized) as an abstract method on the Terminal interface. Existing implementations that only implement setSize(Size) will fail to recompile, as they must now provide an implementation of the new abstract method. This breaks source compatibility.
The suggested alternative preserves backward compatibility by making setSize(Size) abstract (the original contract) and setSize(Sized) a default method, ensuring existing implementations continue to work without modification.
🤖 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` around lines 759 -
765, The change introduced a new abstract method setSize(Sized) on the Terminal
interface which breaks source compatibility for existing implementations that
only implement setSize(Size); to fix, revert to keeping the original abstract
contract by making setSize(Size) the required method on Terminal (remove the
default implementation) and provide setSize(Sized) as a default helper that
delegates to setSize(Size) (i.e., implement setSize(Sized) to call
setSize((Size) sized) or equivalent), so existing implementers need not change
while new callers can use the Sized overload; update the Terminal interface
methods setSize(Size) and setSize(Sized) accordingly.
There was a problem hiding this comment.
This was intentional to prevent having to use
@Override
public void setSize(Size size) {
setSize((Sized) size);
}
@Override
public void setSize(Sized sz) {
// Actual implementation
}
everywhere.
7d21f09 already broke source compatibility in the Terminal interface, so 1 more slight change isn't a big issue.
There was a problem hiding this comment.
@Elec332, understood — the design is intentional: by making setSize(Sized) the abstract contract, implementors only need to provide a single setSize(Sized) method instead of two overloads with boilerplate delegation. The setSize(Size) default then serves as a transparent bridge for legacy callers.
Given that the previous commit already introduced a source-compatibility break in Terminal, the incremental impact here is consistent with that direction. The tradeoff (one-time migration for implementors vs. perpetual boilerplate in every implementation) clearly favors your approach.
🧠 Learnings used
Learnt from: CR
Repo: jline/jline3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T12:03:07.932Z
Learning: Applies to **/terminal/**/*.java : Follow existing patterns for terminal feature pairs, such as hasFocusSupport()/trackFocus() method naming
|
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 `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java`:
- Around line 1758-1759: The public synchronized method setSize(Sized)
dereferences its Sized parameter without a null check; add a defensive null
guard at the start of setSize(Sized) (e.g., if sized == null return false or
throw IllegalArgumentException) before calling sized.getColumns()/getRows(), so
callers cannot cause an NPE; update the method body of setSize(Sized) (which
delegates to setSize(int,int)) to perform this check.
🪄 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: 1284a8f2-ce40-4778-b040-8fe001575fdf
📒 Files selected for processing (3)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.javabuiltins/src/main/java/org/jline/builtins/SwingTerminal.javabuiltins/src/main/java/org/jline/builtins/WebTerminal.java
🚧 Files skipped from review as they are similar to previous changes (2)
- builtins/src/main/java/org/jline/builtins/WebTerminal.java
- builtins/src/main/java/org/jline/builtins/SwingTerminal.java
| public synchronized boolean setSize(Sized sized) { | ||
| return setSize(sized.getColumns(), sized.getRows()); |
There was a problem hiding this comment.
Guard against null in setSize(Sized)
Line [1758] accepts a nullable reference and Line [1759] dereferences it unconditionally. A defensive check avoids avoidable runtime NPEs from public API callers.
Suggested fix
public synchronized boolean setSize(Sized sized) {
+ if (sized == null) {
+ return false;
+ }
return setSize(sized.getColumns(), sized.getRows());
}📝 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.
| public synchronized boolean setSize(Sized sized) { | |
| return setSize(sized.getColumns(), sized.getRows()); | |
| public synchronized boolean setSize(Sized sized) { | |
| if (sized == null) { | |
| return false; | |
| } | |
| return setSize(sized.getColumns(), sized.getRows()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java` around lines
1758 - 1759, The public synchronized method setSize(Sized) dereferences its
Sized parameter without a null check; add a defensive null guard at the start of
setSize(Sized) (e.g., if sized == null return false or throw
IllegalArgumentException) before calling sized.getColumns()/getRows(), so
callers cannot cause an NPE; update the method body of setSize(Sized) (which
delegates to setSize(int,int)) to perform this check.
There was a problem hiding this comment.
While valid, this method mimics the logic in all similar methods introduced in 7d21f09
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh api repos/jline/jline3/commits/7d21f09e65bb4305db1c87086ee7e5a2ed872215 --jq '.files[].filename' 2>/dev/null
gh api "repos/jline/jline3/commits/7d21f09e65bb4305db1c87086ee7e5a2ed872215" 2>/dev/null | python3 -c "
import sys, json
data = json.load(sys.stdin)
for f in data.get('files', []):
print('=== FILE:', f['filename'], '===')
print(f.get('patch',''))
print()
" 2>/dev/null | head -200
Length of output: 11790
🏁 Script executed:
#!/bin/bash
gh api "repos/jline/jline3/commits/7d21f09e65bb4305db1c87086ee7e5a2ed872215" 2>/dev/null | python3 -c "
import sys, json
data = json.load(sys.stdin)
for f in data.get('files', []):
if any(x in f['filename'] for x in ['Terminal.java', 'LineDiscipline', 'AbstractPosix', 'AbstractWindows', 'DumbTerminal', 'Display.java', 'Size.java']):
print('=== FILE:', f['filename'], '===')
print(f.get('patch',''))
print()
" 2>/dev/null
Length of output: 46937
@Elec332, that's a fair point. Looking at the commit you referenced, the established pattern throughout the codebase — including setSize(Size size) in ScreenTerminal, Terminal.setSize(Size), and Display.resize(Size) — consistently omits null guards and dereferences the parameter directly. The current setSize(Sized sized) faithfully follows that same convention. I'll withdraw this suggestion.
✏️ Learnings added
Learnt from: Elec332
Repo: jline/jline3 PR: 1731
File: builtins/src/main/java/org/jline/builtins/ScreenTerminal.java:1758-1759
Timestamp: 2026-04-09T00:49:13.994Z
Learning: In jline/jline3, the established pattern for `setSize(Size/Sized)` and similar methods (e.g., `Display.resize(Size)`) across `ScreenTerminal`, `Terminal`, `LineDisciplineTerminal`, and related classes is to dereference the parameter directly without null checks. Do not suggest null guards for these methods.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: jline/jline3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T12:03:07.932Z
Learning: Applies to **/terminal/**/*.java : Follow existing patterns for terminal feature pairs, such as hasFocusSupport()/trackFocus() method naming
|
Could be fixed by renaming the new method Size.copyFrom(Sized), which would both allow a deprecated tag and also make it a bit more clear what the function does (as "copy" could both nean copy from or to). Since you may find that a bit drastic I did not go this route yet, but this could easily be done upon request. |
2 participants
This pull requests attempts to unify the way you can access the size of objects (like terminals).
Additional benefits:
You can now access the size of Display.
This would later also allow places that have:
to change to:
or
Summary by CodeRabbit