refactor: introduce Sized interface and Size.of() factory methods#1731
refactor: introduce Sized interface and Size.of() factory methods#1731gnodet merged 12 commits intojline:masterfrom
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
|
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:
📝 WalkthroughWalkthroughIntroduces a new ChangesSized-based sizing API migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
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. |
# Conflicts: # terminal/src/main/java/org/jline/terminal/Size.java
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 `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java`:
- Around line 1772-1776: Update the outer-class override annotation for the
deprecated method WebTerminal.setSize(int, int) so it suppresses both the
compiler deprecation warning and the Sonar rule; change the current
`@SuppressWarnings`("java:S1133") on the WebTerminal.setSize(int,int) override to
include "deprecation" as well (i.e.
`@SuppressWarnings`({"deprecation","java:S1133"})), matching the inner class
WebTerminalComponent usage and preventing javac -Werror deprecation failures.
In `@terminal/src/main/java/org/jline/terminal/Size.java`:
- Around line 174-187: Update the Javadoc for the method copy(Sized) so it
correctly refers to a "Sized object" (and examples like Terminal or other Sized
implementations) instead of "Size object": change the descriptive text and the
`@param` tag to mention "Sized" and that the method copies rows and columns from
the provided Sized instance (the method body uses size.getColumns()/getRows()
and calls setColumns/setRows).
🪄 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: f20d1008-0801-4e5e-a0f8-1bacf97ab02d
📒 Files selected for processing (13)
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.javaterminal/src/main/java/org/jline/terminal/Size.javaterminal/src/main/java/org/jline/terminal/Terminal.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 (3)
- terminal/src/test/java/org/jline/utils/DisplayTest.java
- builtins/src/test/java/org/jline/builtins/ScreenTerminalTest.java
- terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- builtins/src/main/java/org/jline/builtins/SwingTerminal.java
- builtins/src/main/java/org/jline/builtins/Tmux.java
- builtins/src/main/java/org/jline/builtins/WebTerminal.java
- terminal/src/main/java/org/jline/utils/Display.java
| * @deprecated Use {@link #setSize(Size)} instead. | ||
| */ | ||
| @Deprecated | ||
| @SuppressWarnings("java:S1133") // Intentional deprecation; removal planned for a future major version | ||
| public synchronized boolean setSize(int columns, int rows) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any override or direct call to setSize(int, int) in ScreenTerminal subclasses.
# Expect: zero matches in SwingTerminal.java, WebTerminal.java, and Tmux.java;
# if any exist without `@SuppressWarnings`("deprecation"), they are build errors under -Werror.
echo "=== Overrides of setSize(int ...) in ScreenTerminal subclasses ==="
rg -n -B2 "setSize\(int\s+columns" --type=java
echo ""
echo "=== Direct calls to setSize with two int args ==="
rg -n "\.setSize\s*\(\s*[a-zA-Z_][a-zA-Z0-9_]*\s*,\s*[a-zA-Z_][a-zA-Z0-9_]*\s*\)" --type=java \
-g "!ScreenTerminal.java"Repository: jline/jline3
Length of output: 1577
🏁 Script executed:
#!/bin/bash
# 1. Check WebTerminal.java:240 exact annotation context
echo "=== WebTerminal.java:238-242 ==="
sed -n '238,242p' builtins/src/main/java/org/jline/builtins/WebTerminal.java
echo ""
echo "=== WebTerminal.java:473-480 ==="
sed -n '473,480p' builtins/src/main/java/org/jline/builtins/WebTerminal.java
echo ""
# 2. Check DisplayTest.java around line 102
echo "=== DisplayTest.java:98-108 ==="
sed -n '98,108p' terminal/src/test/java/org/jline/utils/DisplayTest.java
echo ""
# 3. Search for SwingTerminal.java and check for setSize
echo "=== Looking for SwingTerminal files ==="
find . -name "SwingTerminal.java" -type f
echo ""
# 4. Check for setSize in SwingTerminal if it exists
echo "=== Searching for setSize in swing-related files ==="
rg -n "setSize" --type=java -g "*wingu*" -g "*Swing*"Repository: jline/jline3
Length of output: 1917
🏁 Script executed:
#!/bin/bash
# Check DisplayTest class declaration and any class-level annotations
echo "=== DisplayTest.java class declaration and context ==="
head -n 110 terminal/src/test/java/org/jline/utils/DisplayTest.java | tail -n 30
echo ""
# Check if WebTerminal outer class setSize is truly missing deprecation suppression
echo "=== Full context of WebTerminal.java:236-244 ==="
sed -n '236,244p' builtins/src/main/java/org/jline/builtins/WebTerminal.java
echo ""
# Check if there's a way the code currently builds successfully
echo "=== Check git log for recent changes to WebTerminal.java ==="
git log --oneline -n 20 -- builtins/src/main/java/org/jline/builtins/WebTerminal.javaRepository: jline/jline3
Length of output: 1735
Fix @SuppressWarnings annotation in WebTerminal.setSize(int, int) outer class override.
The outer class override of setSize(int, int) at line 240 in WebTerminal.java uses only @SuppressWarnings("java:S1133"), which suppresses the SonarQube rule but not javac's deprecation warning. With -Werror enabled, this produces a build failure when compiling from a separate compilation unit.
The inner class WebTerminalComponent correctly uses @SuppressWarnings("deprecation") at line 475. The outer class override should match this approach. SwingTerminal does not override the deprecated method and is unaffected.
Change line 239 from:
`@SuppressWarnings`("java:S1133") // Intentional deprecation; removal planned for a future major versionto:
`@SuppressWarnings`({"deprecation", "java:S1133"}) // Intentional deprecation; removal planned for a future major version🤖 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
1772 - 1776, Update the outer-class override annotation for the deprecated
method WebTerminal.setSize(int, int) so it suppresses both the compiler
deprecation warning and the Sonar rule; change the current
`@SuppressWarnings`("java:S1133") on the WebTerminal.setSize(int,int) override to
include "deprecation" as well (i.e.
`@SuppressWarnings`({"deprecation","java:S1133"})), matching the inner class
WebTerminalComponent usage and preventing javac -Werror deprecation failures.
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Skipping — the build passes as-is. The outer class WebTerminal.setSize(int, int) is itself @Deprecated, so calling deprecated methods from within it doesn't trigger javac deprecation warnings even with -Werror. No additional @SuppressWarnings("deprecation") is needed.
There was a problem hiding this comment.
@gnodet, you're absolutely right — I apologize for the incorrect suggestion.
The compiler suppresses deprecation warnings if a deprecated item is used within an entity which itself is deprecated, or is used within the same outermost class, or is used in an entity that is annotated to suppress the warning. Since WebTerminal.setSize(int, int) is itself annotated @Deprecated, calling other deprecated APIs within it does not produce javac deprecation warnings, even with -Werror. The existing @SuppressWarnings("java:S1133") (for SonarQube) is sufficient.
✏️ Learnings added
Learnt from: gnodet
Repo: jline/jline3 PR: 1731
File: builtins/src/main/java/org/jline/builtins/ScreenTerminal.java:0-0
Timestamp: 2026-05-05T11:48:17.358Z
Learning: In Java, the compiler suppresses ordinary deprecation warnings when a deprecated API is used within an entity that is itself Deprecated (per JLS / javac behavior). This means calling deprecated methods from within a Deprecated method does NOT produce a javac deprecation warning even with -Xlint:deprecation or -Werror. Therefore, SuppressWarnings("deprecation") is not needed on a Deprecated method that internally calls other deprecated APIs. This applies in jline/jline3 and in Java generally.
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: 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, follow the existing contract for sizing APIs like `setSize(Size/Sized)` and `Display.resize(Size)`: dereference the provided `Size`/`Sized` argument directly (no null checks/guards and no defensive handling for null parameters). When reviewing code in related terminal/screen classes (e.g., `ScreenTerminal`, `Terminal`, `LineDisciplineTerminal`), do not suggest adding null guards for these methods—assume the parameter is non-null per the library’s established pattern.
Required after master merge: the flattened terminal provider hierarchy means FfmUnixSysTerminal extends AbstractUnixSysTerminal directly, which must implement the Terminal.setSize(Sized) method.
Size fields are now final. Mutation methods (setRows, setColumns, copy) are deprecated and throw UnsupportedOperationException. All call sites updated to use Size.of() and direct assignment instead of copy(). Mutable Size fields changed to volatile for thread-safe signal handling. Defensive copies removed from getSize() since Size is now immutable.
…inal Move the real setSize logic into setSize(Sized) and have the deprecated setSize(int, int) delegate to it. Also reorder Nano fields to keep final fields grouped together.
Restore working setters and copy() methods (deprecated but functional) instead of throwing UnsupportedOperationException. Callers are already migrated to Size.of() but external code using the old API won't break. Restore defensive copies in getSize() via Size.of(size). Move setSize implementation to Sized overload in ScreenTerminal. Reorder Nano fields to keep finals grouped.
- Change Pty.setSize(Size) to Pty.setSize(Sized) and update all implementations (JniNativePty, FfmNativePty, ExecPty), removing unnecessary Size.of() wrapper in AbstractPosixTerminal - Fix TTop.handle() to capture size into local variables to avoid race between reads when SIGWINCH fires concurrently - Fix WebTerminal.setSize(int,int) to include checkClosed() and super.setSize() like the Sized overload - Fix Sized javadoc to not reference "terminal" specifically - Add defensive copy comment to Size.of(Sized) - Clean up Terminal.setSize(Size) bridge method comment - Replace wildcard imports in GraphemeClusterModeTest and DisplayTest
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
builtins/src/main/java/org/jline/builtins/SwingTerminal.java (1)
414-419:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNotify Swing after changing the preferred size.
updatePreferredSize()alone does not relayout or repaint parent containers. After a successful resize, the component can keep the old bounds until some unrelated UI event happens. Since the component is visible in a frame,revalidate()andrepaint()are required to reflect the size change immediately.Small UI refresh fix
public boolean setSize(Sized size) { if (screenTerminal.setSize(size)) { updatePreferredSize(); + revalidate(); + repaint(); return true; } return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java` around lines 414 - 419, The setSize(Sized size) method updates preferred size but doesn't notify Swing to relayout/repaint; after the existing updatePreferredSize() call (inside setSize) call the Swing component's revalidate() and repaint() so parent containers re-layout and the UI updates immediately (i.e., after screenTerminal.setSize(size) and updatePreferredSize(), invoke the component's revalidate() and repaint()).
🧹 Nitpick comments (3)
builtins/src/main/java/org/jline/builtins/Nano.java (1)
2033-2033: ⚡ Quick winPlease verify the editor is not caching a terminal-owned
Size.
Nanonow keeps the exact object returned byterminal.getSize(). If any terminal hands out a live mutableSize, the editor's layout state can be shared with terminal internals.Size.of(terminal.getSize())would preserve the new replacement pattern while keeping ownership local.Also applies to: 3791-3791
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@builtins/src/main/java/org/jline/builtins/Nano.java` at line 2033, Nano currently assigns the terminal-owned Size directly into its layout state by doing size = terminal.getSize(); to avoid sharing a potentially mutable object with terminal internals, replace direct assignment with a defensive copy (e.g., use Size.of(terminal.getSize()) or an equivalent copy constructor) wherever Nano assigns terminal.getSize() (references: Nano class, variable size, terminal.getSize()); do the same for the other occurrence referenced in the review (around the second occurrence) to ensure Nano owns its Size instances.terminal/src/main/java/org/jline/terminal/impl/exec/ExecPty.java (1)
348-384: ⚡ Quick winExtract
"columns"and"rows"string literals as constants to resolve the SonarCloud failure.
"columns"appears three times (line 348, and twice insetSize) and"rows"similarly. SonarCloud flags this (issueAZ31At19iIFEYGRFIa8V).♻️ Proposed refactor
+ private static final String COLUMNS = "columns"; + private static final String ROWS = "rows"; + static Size doGetSize(String cfg) throws IOException { - return Size.of(doGetInt("columns", cfg), doGetInt("rows", cfg)); + return Size.of(doGetInt(COLUMNS, cfg), doGetInt(ROWS, cfg)); }exec( true, OSUtils.STTY_COMMAND, - "columns", + COLUMNS, Integer.toString(size.getColumns()), - "rows", + ROWS, Integer.toString(size.getRows())); } else { exec( false, OSUtils.STTY_COMMAND, OSUtils.STTY_F_OPTION, getName(), - "columns", + COLUMNS, Integer.toString(size.getColumns()), - "rows", + ROWS, Integer.toString(size.getRows()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terminal/src/main/java/org/jline/terminal/impl/exec/ExecPty.java` around lines 348 - 384, Add two class-level constants for the repeated literals (e.g. private static final String COLUMNS = "columns"; private static final String ROWS = "rows";) in ExecPty and replace the three occurrences in doGetInt(...) and the four occurrences in setSize(...) with those constants; update the regex patterns in doGetInt to reference COLUMNS/ROWS instead of string literals and use COLUMNS and ROWS in the exec(...) argument lists (alongside existing symbols doGetInt, setSize, OSUtils.STTY_COMMAND, OSUtils.STTY_F_OPTION, getName).builtins/src/test/java/org/jline/builtins/TerminalTest.java (1)
297-300: ⚡ Quick winAssert the Swing invalid-size path here as well.
These calls currently only verify "no exception". If
SwingTerminal.setSize(Size.of(...))starts accepting out-of-range values, this test still passes. Snapshot the initial size and assert it stays unchanged, like theWebTerminalbranch.Minimal assertion coverage
+ Size swingValidSize = swingTerminal.getSize(); swingTerminal.setSize(Size.of(1, 10)); + assertEquals(swingValidSize, swingTerminal.getSize()); swingTerminal.setSize(Size.of(10, 1)); + assertEquals(swingValidSize, swingTerminal.getSize()); swingTerminal.setSize(Size.of(300, 10)); swingTerminal.setSize(Size.of(10, 300));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@builtins/src/test/java/org/jline/builtins/TerminalTest.java` around lines 297 - 300, Capture the initial size from swingTerminal (e.g., call swingTerminal.getSize() or the equivalent accessor) before the invalid-size setSize calls, then after each setSize(Size.of(...)) assert that the size remains equal to the initial snapshot (same approach used in the WebTerminal branch). Update the test around the four swingTerminal.setSize(Size.of(...)) calls to compare current size to the saved initialSize to ensure out-of-range values do not change the terminal size.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@builtins/src/main/java/org/jline/builtins/WebTerminal.java`:
- Around line 223-247: The WebTerminal's new setSize(Sized) path bypasses
WebTerminalComponent's stricter 10x5 minimum and uses only the generic
MIN_SIZE/MAX_SIZE checks from ScreenTerminal, changing behavior for both
setSize(Sized) and the deprecated setSize(int,int); restore the original
stricter validation by invoking WebTerminalComponent.setSize(int,int) or
explicitly enforcing the 10x5 floor before delegating: in
WebTerminal.setSize(Sized) and the deprecated setSize(int,int) ensure you call
component.setSize(width,height) (or check Sized.width/height and reject <10x5)
so the WebTerminalComponent's bounds are preserved, then call
super.setSize(component) when the component accepted the change (same pattern
used around component.setSize(...) in this class).
In `@terminal/src/main/java/org/jline/terminal/Size.java`:
- Around line 81-90: The Javadoc on the no-arg constructor incorrectly states
"Size is now immutable"; update the deprecation message for Size() to accurately
reflect current API: note that mutating setters (setRows, setColumns) are
deprecated but still present, and recommend using Size.of(int,int) for the
preferred immutable-style construction (e.g., "Deprecated — prefer
Size.of(int,int); setRows/setColumns are deprecated but still available."). Edit
the Javadoc on the Size() constructor to remove the assertive "now immutable"
claim and reference the of(int,int) factory and the deprecated setters (setRows,
setColumns) so callers aren't misled.
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`:
- Line 331: Add a one-line explanatory comment inside the empty stub method
setSize(Sized size) in GraphemeClusterModeTest to document that the no-op is
intentional (e.g., "// no-op: test stub intentionally does not resize") so
SonarCloud's empty-method rule is satisfied without changing behavior.
---
Duplicate comments:
In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java`:
- Around line 414-419: The setSize(Sized size) method updates preferred size but
doesn't notify Swing to relayout/repaint; after the existing
updatePreferredSize() call (inside setSize) call the Swing component's
revalidate() and repaint() so parent containers re-layout and the UI updates
immediately (i.e., after screenTerminal.setSize(size) and updatePreferredSize(),
invoke the component's revalidate() and repaint()).
---
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/Nano.java`:
- Line 2033: Nano currently assigns the terminal-owned Size directly into its
layout state by doing size = terminal.getSize(); to avoid sharing a potentially
mutable object with terminal internals, replace direct assignment with a
defensive copy (e.g., use Size.of(terminal.getSize()) or an equivalent copy
constructor) wherever Nano assigns terminal.getSize() (references: Nano class,
variable size, terminal.getSize()); do the same for the other occurrence
referenced in the review (around the second occurrence) to ensure Nano owns its
Size instances.
In `@builtins/src/test/java/org/jline/builtins/TerminalTest.java`:
- Around line 297-300: Capture the initial size from swingTerminal (e.g., call
swingTerminal.getSize() or the equivalent accessor) before the invalid-size
setSize calls, then after each setSize(Size.of(...)) assert that the size
remains equal to the initial snapshot (same approach used in the WebTerminal
branch). Update the test around the four swingTerminal.setSize(Size.of(...))
calls to compare current size to the saved initialSize to ensure out-of-range
values do not change the terminal size.
In `@terminal/src/main/java/org/jline/terminal/impl/exec/ExecPty.java`:
- Around line 348-384: Add two class-level constants for the repeated literals
(e.g. private static final String COLUMNS = "columns"; private static final
String ROWS = "rows";) in ExecPty and replace the three occurrences in
doGetInt(...) and the four occurrences in setSize(...) with those constants;
update the regex patterns in doGetInt to reference COLUMNS/ROWS instead of
string literals and use COLUMNS and ROWS in the exec(...) argument lists
(alongside existing symbols doGetInt, setSize, OSUtils.STTY_COMMAND,
OSUtils.STTY_F_OPTION, getName).
🪄 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: 78a9ab06-5dde-4b95-b497-fb8bfd3adf9c
📒 Files selected for processing (51)
builtins/src/main/java/org/jline/builtins/Less.javabuiltins/src/main/java/org/jline/builtins/Nano.javabuiltins/src/main/java/org/jline/builtins/ScreenTerminal.javabuiltins/src/main/java/org/jline/builtins/SwingTerminal.javabuiltins/src/main/java/org/jline/builtins/TTop.javabuiltins/src/main/java/org/jline/builtins/Tmux.javabuiltins/src/main/java/org/jline/builtins/WebTerminal.javabuiltins/src/test/java/org/jline/builtins/CommandsTest.javabuiltins/src/test/java/org/jline/builtins/NanoClasspathConfigTest.javabuiltins/src/test/java/org/jline/builtins/NanoSuggestionBoxTest.javabuiltins/src/test/java/org/jline/builtins/NanoTest.javabuiltins/src/test/java/org/jline/builtins/ScreenTerminalTest.javabuiltins/src/test/java/org/jline/builtins/TerminalTest.javabuiltins/src/test/java/org/jline/builtins/TmuxEncodingTest.javabuiltins/src/test/java/org/jline/example/Example.javaconsole-ui/src/main/java/org/jline/consoleui/prompt/AbstractPrompt.javaconsole/src/test/java/org/jline/widget/TailTipWidgetsTest.javademo/src/main/java/org/jline/demo/Repl.javaprompt/src/main/java/org/jline/prompt/impl/DefaultPrompter.javaprompt/src/test/java/org/jline/prompt/PrompterExecutionTest.javaprompt/src/test/java/org/jline/prompt/PrompterListExecutionTest.javareader/src/main/java/org/jline/reader/impl/LineReaderImpl.javareader/src/test/java/org/jline/keymap/BindingReaderTest.javareader/src/test/java/org/jline/keymap/KeyMapTest.javareader/src/test/java/org/jline/reader/impl/CompletionTest.javareader/src/test/java/org/jline/reader/impl/LineReaderTest.javareader/src/test/java/org/jline/reader/impl/ReaderTestSupport.javaremote-ssh/src/main/java/org/jline/builtins/ssh/ShellFactoryImpl.javaremote-telnet/src/main/java/org/jline/builtins/telnet/Telnet.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/CLibrary.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmNativePty.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmUnixSysTerminal.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/NativeWinSysTerminal.javaterminal-ffm/src/test/java/org/jline/terminal/impl/ffm/FfmTest.javaterminal-jni/src/main/java/org/jline/terminal/impl/jni/JniNativePty.javaterminal-jni/src/main/java/org/jline/terminal/impl/jni/JniUnixSysTerminal.javaterminal-jni/src/main/java/org/jline/terminal/impl/jni/win/NativeWinSysTerminal.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/AbstractUnixSysTerminal.javaterminal/src/main/java/org/jline/terminal/impl/DumbTerminal.javaterminal/src/main/java/org/jline/terminal/impl/LineDisciplineTerminal.javaterminal/src/main/java/org/jline/terminal/impl/exec/ExecPty.javaterminal/src/main/java/org/jline/terminal/spi/Pty.javaterminal/src/test/java/org/jline/terminal/impl/AbstractWindowsTerminalTest.javaterminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.javaterminal/src/test/java/org/jline/terminal/impl/exec/ExecPtyTest.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 (19)
- terminal/src/main/java/org/jline/terminal/Sized.java
- reader/src/test/java/org/jline/reader/impl/ReaderTestSupport.java
- builtins/src/test/java/org/jline/builtins/NanoTest.java
- terminal/src/test/java/org/jline/utils/DisplayBenchmarkTest.java
- console/src/test/java/org/jline/widget/TailTipWidgetsTest.java
- terminal/src/test/java/org/jline/terminal/impl/AbstractWindowsTerminalTest.java
- builtins/src/test/java/org/jline/builtins/CommandsTest.java
- builtins/src/test/java/org/jline/builtins/NanoSuggestionBoxTest.java
- reader/src/test/java/org/jline/reader/impl/LineReaderTest.java
- terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/NativeWinSysTerminal.java
- reader/src/test/java/org/jline/keymap/KeyMapTest.java
- demo/src/main/java/org/jline/demo/Repl.java
- builtins/src/test/java/org/jline/builtins/TmuxEncodingTest.java
- terminal-ffm/src/test/java/org/jline/terminal/impl/ffm/FfmTest.java
- prompt/src/test/java/org/jline/prompt/PrompterListExecutionTest.java
- builtins/src/test/java/org/jline/builtins/NanoClasspathConfigTest.java
- terminal-jni/src/main/java/org/jline/terminal/impl/jni/win/NativeWinSysTerminal.java
- remote-telnet/src/main/java/org/jline/builtins/telnet/Telnet.java
- prompt/src/test/java/org/jline/prompt/PrompterExecutionTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- builtins/src/test/java/org/jline/builtins/ScreenTerminalTest.java
| @Override | ||
| public void setSize(Sized sz) { | ||
| checkClosed(); | ||
| if (component.setSize(sz)) { | ||
| super.setSize(component); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Set the terminal dimensions in columns and rows. | ||
| * | ||
| * @param columns the new number of columns | ||
| * @param rows the new number of rows | ||
| * @return `true` if the terminal was resized, `false` otherwise | ||
| * @deprecated Use {@link #setSize(Sized)} instead. | ||
| */ | ||
| @Deprecated | ||
| @SuppressWarnings("java:S1133") | ||
| public boolean setSize(int columns, int rows) { | ||
| return component.setSize(columns, rows); | ||
| checkClosed(); | ||
| if (component.setSize(Size.of(columns, rows))) { | ||
| super.setSize(component); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Preserve WebTerminal’s stricter minimum-size validation for Sized callers.
WebTerminalComponent.setSize(int, int) still rejects sizes below 10x5, but both new outer paths now dispatch to component.setSize(Sized), which resolves to ScreenTerminal.setSize(Sized) and only applies the generic MIN_SIZE/MAX_SIZE checks. That means setSize(Sized) now accepts dimensions the WebTerminal used to reject, and the deprecated setSize(int, int) overload has changed behavior as well.
🐛 Keep the old bounds for both resize APIs
+ `@Override`
+ public synchronized boolean setSize(Sized sz) {
+ int columns = sz.getColumns();
+ int rows = sz.getRows();
+ if (columns < 10 || rows < 5 || columns > MAX_SIZE || rows > MAX_SIZE) {
+ return false;
+ }
+ return super.setSize(sz);
+ }
+
`@Override`
`@Deprecated`
`@SuppressWarnings`("deprecation")
public synchronized boolean setSize(int columns, int rows) {
if (columns < 10 || rows < 5 || columns > MAX_SIZE || rows > MAX_SIZE) {
return false;
}
return super.setSize(columns, rows);
}Also applies to: 478-485
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@builtins/src/main/java/org/jline/builtins/WebTerminal.java` around lines 223
- 247, The WebTerminal's new setSize(Sized) path bypasses WebTerminalComponent's
stricter 10x5 minimum and uses only the generic MIN_SIZE/MAX_SIZE checks from
ScreenTerminal, changing behavior for both setSize(Sized) and the deprecated
setSize(int,int); restore the original stricter validation by invoking
WebTerminalComponent.setSize(int,int) or explicitly enforcing the 10x5 floor
before delegating: in WebTerminal.setSize(Sized) and the deprecated
setSize(int,int) ensure you call component.setSize(width,height) (or check
Sized.width/height and reject <10x5) so the WebTerminalComponent's bounds are
preserved, then call super.setSize(component) when the component accepted the
change (same pattern used around component.setSize(...) in this class).
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Skipping this one. WebTerminal.setSize(Sized) dispatches to component.setSize(sz) which resolves to ScreenTerminal.setSize(Sized) — that method already validates MIN_SIZE/MAX_SIZE bounds (2–256). The stricter 10×5 minimums in WebTerminalComponent.setSize(int, int) are application-specific extras from the deprecated path, not a safety concern. The parent class validation is sufficient.
There was a problem hiding this comment.
@gnodet, understood — the ScreenTerminal.setSize(Sized) path's MIN_SIZE/MAX_SIZE bounds (2–256) are the authoritative validation, and the stricter 10×5 floor in the deprecated WebTerminalComponent.setSize(int, int) was an application-level extra tied to that now-deprecated code path. Thanks for the clarification.
✏️ Learnings added
Learnt from: gnodet
Repo: jline/jline3 PR: 1731
File: builtins/src/main/java/org/jline/builtins/WebTerminal.java:223-247
Timestamp: 2026-05-05T11:47:56.789Z
Learning: In jline/jline3 `WebTerminal` (builtins/src/main/java/org/jline/builtins/WebTerminal.java), the stricter 10-column × 5-row minimum enforced by `WebTerminalComponent.setSize(int, int)` is an application-specific constraint on the deprecated int-based path only. The new `setSize(Sized)` path intentionally relies solely on `ScreenTerminal`'s generic `MIN_SIZE`/`MAX_SIZE` bounds (2–256). Do not flag the absence of the 10×5 floor as a bug in the `Sized` path.
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: 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, follow the existing contract for sizing APIs like `setSize(Size/Sized)` and `Display.resize(Size)`: dereference the provided `Size`/`Sized` argument directly (no null checks/guards and no defensive handling for null parameters). When reviewing code in related terminal/screen classes (e.g., `ScreenTerminal`, `Terminal`, `LineDisciplineTerminal`), do not suggest adding null guards for these methods—assume the parameter is non-null per the library’s established pattern.
| } | ||
|
|
||
| public void setSize(Size size) {} | ||
| public void setSize(Sized size) {} |
There was a problem hiding this comment.
Add a brief comment to the empty setSize stub to satisfy the SonarCloud rule.
SonarCloud flags the empty method body (see linked issue AZ3yGSLOsbPvxn6XHB2u). A one-line comment documents intent and silences the check without changing behavior.
🔧 Proposed fix
- public void setSize(Sized size) {}
+ public void setSize(Sized size) { /* test stub – no-op */ }📝 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 void setSize(Sized size) {} | |
| public void setSize(Sized size) { /* test stub – no-op */ } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 331-331: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java`
at line 331, Add a one-line explanatory comment inside the empty stub method
setSize(Sized size) in GraphemeClusterModeTest to document that the no-op is
intentional (e.g., "// no-op: test stub intentionally does not resize") so
SonarCloud's empty-method rule is satisfied without changing behavior.
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Skipping — empty method bodies in test stubs are standard practice. Adding // no-op is noise. If SonarCloud's S1186 rule is problematic across the project, it should be addressed at the quality profile level rather than per-stub.
There was a problem hiding this comment.
@gnodet, that's a fair point — suppressing S1186 at the SonarCloud quality profile level is the cleaner, more maintainable approach rather than scattering // no-op comments across test stubs. Acknowledged, and I'll avoid flagging this pattern in future reviews for this project.
✏️ Learnings added
Learnt from: gnodet
Repo: jline/jline3 PR: 1731
File: terminal/src/test/java/org/jline/terminal/impl/GraphemeClusterModeTest.java:331-331
Timestamp: 2026-05-05T11:47:56.853Z
Learning: In jline/jline3, empty method bodies in test stubs (e.g., anonymous Terminal implementations in unit tests) are considered standard practice. Do not flag them for SonarCloud rule S1186 (empty method body). The project maintainer (gnodet) prefers addressing S1186 at the SonarCloud quality profile level rather than adding `// no-op` comments per stub.
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: Elec332
Repo: jline/jline3 PR: 1794
File: prompt/src/test/java/org/jline/prompt/PromptCommandsTest.java:63-66
Timestamp: 2026-04-17T16:57:01.118Z
Learning: For JUnit 5 tests, be aware that `AfterEach` runs even if `BeforeEach` throws an exception (the test is skipped, but cleanup still happens). In `AfterEach`, defensively handle partially initialized state created in `BeforeEach` (e.g., fields set after `TerminalBuilder.build()` may be null). Use null checks before cleanup/closing (e.g., `if (terminal != null) terminal.close();`) to avoid NPEs that could mask the original setup failure.
Learnt from: gnodet
Repo: jline/jline3 PR: 1857
File: terminal/src/test/java/org/jline/terminal/impl/SixelGraphicsTest.java:231-236
Timestamp: 2026-04-29T06:24:34.478Z
Learning: In JLine/JLine3 terminal tests, it’s acceptable to use `TerminalBuilder.builder().type("dumb").streams(input, output).build()` as a deterministic alternative to `LineDisciplineTerminal` for exercising terminal I/O without requiring a real terminal. If either approach is used to provide an in-memory/dummy terminal for the test, don’t flag it as incorrect—between these two patterns, the choice is stylistic as long as the test remains deterministic and doesn’t depend on an actual terminal device.
gnodet
changed the title
|
2 participants
Summary
Introduces a
Sizedinterface as a read-only abstraction for objects that expose terminal dimensions (getColumns()/getRows()), and addsSize.of()static factory methods as the preferred way to createSizeinstances.Key changes
Sizedinterface — implemented bySize,Terminal,Display,ScreenTerminal, and Swing/Web terminal components. Unifies dimension access across the API.Size.of(int, int)andSize.of(Sized)factory methods — preferred over constructors. Allnew Size(...)calls migrated toSize.of(...).Terminal.setSize(Sized)— replacessetSize(Size)as the primary contract.Pty.setSize()anddoSetSize()updated to acceptSized.Size()no-arg constructor,Size(int, int),Size(Sized),setRows(),setColumns(),copy(),ScreenTerminal.setSize(int, int).volatileSize fields — terminal implementations (LineDisciplineTerminal,DumbTerminal) and consumers (LineReaderImpl,Less,Nano,TTop, etc.) usevolatile+ reference-swap instead ofsynchronized+copy(), improving thread safety for SIGWINCH signal handlers.Size.of(size)ingetSize()sinceSizeis still mutable.Files changed
53 files across
terminal,reader,builtins,console-ui,prompt,demo,remote-ssh,remote-telnet,terminal-ffm,terminal-jni.