refactor: standardize terminal dimension naming to columns/rows#1755
Merged
refactor: standardize terminal dimension naming to columns/rows#1755
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (35)
📝 WalkthroughWalkthroughRenames terminal dimension APIs/fields from width/height to columns/rows, adds Size-based Display.resize(Size) overload and Terminal.getColumns()/getRows() defaults (with deprecated getWidth()/getHeight() aliases), updates constructors/signatures and call sites to use Size or columns/rows, and adds Javadoc; behavior otherwise unchanged. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)builtins/src/main/java/org/jline/builtins/Nano.javareader/src/main/java/org/jline/reader/impl/LineReaderImpl.javaComment |
![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
🤖 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 1749-1750: Clamp scroll_area_y0 to rows - 1 instead of rows to
prevent an out-of-bounds index after resize: change the adjustment logic for
scroll_area_y0 (and keep the existing handling for scroll_area_y1) so that
scroll_area_y0 = Math.min(rows - 1, scroll_area_y0) (ensuring rows > 0 as
needed) and preserve the scroll_area_y1 assignment logic using height/rows;
update any adjacent bounds checks that assume scroll_area_y0 < rows to rely on
the new clamp.
🪄 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: ccc3499a-2247-4780-bcdf-be73fbc5c1c2
📒 Files selected for processing (35)
builtins/src/main/java/org/jline/builtins/Commands.javabuiltins/src/main/java/org/jline/builtins/Less.javabuiltins/src/main/java/org/jline/builtins/Nano.javabuiltins/src/main/java/org/jline/builtins/PosixCommands.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/ScreenTerminalTest.javabuiltins/src/test/java/org/jline/example/Example.javaconsole-ui/src/main/java/org/jline/consoleui/prompt/AbstractPrompt.javaconsole-ui/src/main/java/org/jline/consoleui/prompt/ConsolePrompt.javademo/src/main/java/org/apache/felix/gogo/jline/Shell.javademo/src/main/java/org/jline/demo/Repl.javademo/src/main/java/org/jline/demo/examples/DisplayExample.javademo/src/main/java/org/jline/demo/examples/GraphemeClusterExample.javademo/src/main/java/org/jline/demo/examples/MultiSegmentStatusExample.javademo/src/main/java/org/jline/demo/examples/RemoteTerminalExample.javademo/src/main/java/org/jline/demo/examples/ResponsiveUIExample.javademo/src/main/java/org/jline/demo/examples/SwingTerminalExample.javademo/src/main/java/org/jline/demo/examples/TerminalCursorExample.javademo/src/main/java/org/jline/demo/examples/WebTerminalExample.javajansi-core/src/main/java/org/jline/jansi/AnsiConsole.javaprompt/src/main/java/org/jline/prompt/impl/DefaultPrompter.javareader/src/main/java/org/jline/reader/impl/LineReaderImpl.javaremote-ssh/src/main/java/org/jline/builtins/ssh/Ssh.javaremote-telnet/src/main/java/org/jline/builtins/telnet/ConnectionData.javaremote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.javaterminal/src/main/java/org/jline/terminal/Size.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/main/java/org/jline/utils/Status.javaterminal/src/test/java/org/jline/utils/DisplayTest.javaterminal/src/test/java/org/jline/utils/ScreenTerminal.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.java (1)
2026-2027: Consider using direct field access instead of deprecated getters.Since
dump()is already synchronized, accessing thewidthandheightfields directly would be more efficient than calling the synchronizedgetWidth()andgetHeight()methods (even though reentrant locks make this safe).♻️ Suggested optimization
- int w = getWidth(); - int h = getHeight(); + int w = width; + int h = height;🤖 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 2026 - 2027, The dump() method currently calls the synchronized getters getWidth() and getHeight(), which is unnecessary because dump() is already synchronized; replace those calls with direct field access to width and height (use the class fields width and height inside dump()) to avoid extra synchronization overhead while preserving thread-safety; update any nearby similar calls in dump() that read these dimensions to use the fields instead of getWidth()/getHeight().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java`:
- Around line 2026-2027: The dump() method currently calls the synchronized
getters getWidth() and getHeight(), which is unnecessary because dump() is
already synchronized; replace those calls with direct field access to width and
height (use the class fields width and height inside dump()) to avoid extra
synchronization overhead while preserving thread-safety; update any nearby
similar calls in dump() that read these dimensions to use the fields instead of
getWidth()/getHeight().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fbf85be-5ada-4d84-a788-1a1c06d0258e
📒 Files selected for processing (1)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.java
Contributor
|
Note Docstrings generation - SUCCESS |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
remote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.java (2)
875-886: Consider renaming local variables for consistency.For consistency with the broader standardization effort, consider renaming the local variables in
handleNAWS()fromwidth/heighttocolumns/rows. According to the Javadoc at Line 172, NAWS sends(columns, rows), so using those terms would align with both the protocol semantics and the new API naming.♻️ Optional refactor for naming consistency
private void handleNAWS() throws IOException { - int width = read16int(); - if (width == 255) { - width = read16int(); // handle doubled 255 value; + int columns = read16int(); + if (columns == 255) { + columns = read16int(); // handle doubled 255 value; } - int height = read16int(); - if (height == 255) { - height = read16int(); // handle doubled 255 value; + int rows = read16int(); + if (rows == 255) { + rows = read16int(); // handle doubled 255 value; } skipToSE(); - setTerminalGeometry(width, height); + setTerminalGeometry(columns, rows); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@remote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.java` around lines 875 - 886, Rename the local variables in handleNAWS() from width/height to columns/rows to match the NAWS semantics and the API naming; update the two places where read16int() results are assigned (currently using the 255-double handling comments) to assign to columns and rows respectively, and pass columns and rows into setTerminalGeometry(columns, rows); keep all existing logic (including skipToSE() and the doubled-255 handling) intact while only changing the variable names and their uses.
597-597: Optional: Consider moving method to IACHandler (per SonarCloud).SonarCloud suggests moving
setTerminalGeometryinto theIACHandlerinner class since it's only called fromIACHandler.handleNAWS(). This would improve encapsulation and cohesion, though it's unrelated to the current refactoring effort.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@remote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.java` at line 597, The setTerminalGeometry method is only used by IACHandler.handleNAWS(), so move its implementation into the IACHandler inner class to improve encapsulation: cut the private setTerminalGeometry(int columns, int rows) method from TelnetIO and paste it inside the IACHandler class (make it private there), update the call in IACHandler.handleNAWS() to call the new private method, and remove the original declaration in TelnetIO; ensure any references to outer-class members used by the method still compile (access via TelnetIO.this if needed) and run tests to verify no visibility/qualification issues remain.builtins/src/main/java/org/jline/builtins/WebTerminal.java (1)
546-550: Keep this override aligned withScreenTerminal.setSize(...).This now overrides the parent resize API. Adding
@Overrideand mirroring the parent'ssynchronizedmodifier would make future signature drift obvious and keep the subclass contract consistent.Proposed change
- public boolean setSize(int columns, int rows) { + `@Override` + public synchronized boolean setSize(int columns, int rows) { if (columns < 10 || rows < 5 || columns > 200 || rows > 100) { return false; } return super.setSize(columns, rows); }🤖 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/WebTerminal.java` around lines 546 - 550, The WebTerminal.setSize(int columns, int rows) method overrides the parent API but lacks the `@Override` annotation and the synchronized modifier present in ScreenTerminal.setSize(...); update the WebTerminal method to declare synchronized and add `@Override` while keeping the same parameter list and return behavior (preserve the bounds check and return super.setSize(columns, rows)) so the subclass method signature and concurrency contract match the parent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/WebTerminal.java`:
- Around line 546-550: The WebTerminal.setSize(int columns, int rows) method
overrides the parent API but lacks the `@Override` annotation and the synchronized
modifier present in ScreenTerminal.setSize(...); update the WebTerminal method
to declare synchronized and add `@Override` while keeping the same parameter list
and return behavior (preserve the bounds check and return super.setSize(columns,
rows)) so the subclass method signature and concurrency contract match the
parent.
In `@remote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.java`:
- Around line 875-886: Rename the local variables in handleNAWS() from
width/height to columns/rows to match the NAWS semantics and the API naming;
update the two places where read16int() results are assigned (currently using
the 255-double handling comments) to assign to columns and rows respectively,
and pass columns and rows into setTerminalGeometry(columns, rows); keep all
existing logic (including skipToSE() and the doubled-255 handling) intact while
only changing the variable names and their uses.
- Line 597: The setTerminalGeometry method is only used by
IACHandler.handleNAWS(), so move its implementation into the IACHandler inner
class to improve encapsulation: cut the private setTerminalGeometry(int columns,
int rows) method from TelnetIO and paste it inside the IACHandler class (make it
private there), update the call in IACHandler.handleNAWS() to call the new
private method, and remove the original declaration in TelnetIO; ensure any
references to outer-class members used by the method still compile (access via
TelnetIO.this if needed) and run tests to verify no visibility/qualification
issues remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dde99e2-1f6c-4174-a7c3-35bff24c8177
📒 Files selected for processing (33)
builtins/src/main/java/org/jline/builtins/Commands.javabuiltins/src/main/java/org/jline/builtins/Less.javabuiltins/src/main/java/org/jline/builtins/Nano.javabuiltins/src/main/java/org/jline/builtins/PosixCommands.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/example/Example.javaconsole-ui/src/main/java/org/jline/consoleui/prompt/AbstractPrompt.javaconsole-ui/src/main/java/org/jline/consoleui/prompt/ConsolePrompt.javademo/src/main/java/org/apache/felix/gogo/jline/Shell.javademo/src/main/java/org/jline/demo/Repl.javademo/src/main/java/org/jline/demo/examples/DisplayExample.javademo/src/main/java/org/jline/demo/examples/GraphemeClusterExample.javademo/src/main/java/org/jline/demo/examples/MultiSegmentStatusExample.javademo/src/main/java/org/jline/demo/examples/RemoteTerminalExample.javademo/src/main/java/org/jline/demo/examples/ResponsiveUIExample.javademo/src/main/java/org/jline/demo/examples/SwingTerminalExample.javademo/src/main/java/org/jline/demo/examples/TerminalCursorExample.javademo/src/main/java/org/jline/demo/examples/WebTerminalExample.javajansi-core/src/main/java/org/jline/jansi/AnsiConsole.javaprompt/src/main/java/org/jline/prompt/impl/DefaultPrompter.javareader/src/main/java/org/jline/reader/impl/LineReaderImpl.javaremote-ssh/src/main/java/org/jline/builtins/ssh/Ssh.javaremote-telnet/src/main/java/org/jline/builtins/telnet/ConnectionData.javaremote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.javaterminal/src/main/java/org/jline/terminal/Size.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/utils/Display.javaterminal/src/main/java/org/jline/utils/Status.javaterminal/src/test/java/org/jline/utils/ScreenTerminal.java
✅ Files skipped from review due to trivial changes (11)
- demo/src/main/java/org/jline/demo/examples/DisplayExample.java
- demo/src/main/java/org/jline/demo/examples/ResponsiveUIExample.java
- jansi-core/src/main/java/org/jline/jansi/AnsiConsole.java
- demo/src/main/java/org/jline/demo/examples/RemoteTerminalExample.java
- builtins/src/main/java/org/jline/builtins/PosixCommands.java
- builtins/src/main/java/org/jline/builtins/TTop.java
- builtins/src/main/java/org/jline/builtins/Less.java
- terminal/src/main/java/org/jline/utils/Status.java
- demo/src/main/java/org/jline/demo/Repl.java
- prompt/src/main/java/org/jline/prompt/impl/DefaultPrompter.java
- builtins/src/test/java/org/jline/example/Example.java
🚧 Files skipped from review as they are similar to previous changes (10)
- demo/src/main/java/org/jline/demo/examples/TerminalCursorExample.java
- demo/src/main/java/org/jline/demo/examples/WebTerminalExample.java
- demo/src/main/java/org/jline/demo/examples/SwingTerminalExample.java
- demo/src/main/java/org/jline/demo/examples/MultiSegmentStatusExample.java
- console-ui/src/main/java/org/jline/consoleui/prompt/AbstractPrompt.java
- remote-telnet/src/main/java/org/jline/builtins/telnet/ConnectionData.java
- terminal/src/test/java/org/jline/utils/ScreenTerminal.java
- reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java
- builtins/src/main/java/org/jline/builtins/SwingTerminal.java
- builtins/src/main/java/org/jline/builtins/Tmux.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.java (1)
2068-2072: Consider renaming local variable to avoid shadowing field.The local variable
screenat line 2070 shadows the instance fieldscreendeclared at line 83. While this doesn't cause a bug (the method logic is correct), it reduces readability. Consider renaming toscreenBufferordumpBuffer.♻️ Suggested rename
int cols = getColumns(); int rows = getRows(); - long[] screen = new long[cols * rows]; + long[] screenBuffer = new long[cols * rows]; int[] cursor = new int[2]; - if (!dump(timeout, forceDump, screen, cursor)) { + if (!dump(timeout, forceDump, screenBuffer, cursor)) { return null; }And update references at lines 2082:
- long d = screen[y * cols + x]; + long d = screenBuffer[y * cols + x];🤖 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 2068 - 2072, The local variable named "screen" in the method shadows the instance field "screen"; rename the local to a clearer name (e.g., screenBuffer or dumpBuffer) and update all its references in this method (including the call to dump(timeout, forceDump, screen, cursor) and subsequent usages) so the method no longer shadows the instance field and readability is improved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/ScreenTerminal.java`:
- Around line 2068-2072: The local variable named "screen" in the method shadows
the instance field "screen"; rename the local to a clearer name (e.g.,
screenBuffer or dumpBuffer) and update all its references in this method
(including the call to dump(timeout, forceDump, screen, cursor) and subsequent
usages) so the method no longer shadows the instance field and readability is
improved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94e8ba1d-6429-4f2d-a156-bc55123737b0
📒 Files selected for processing (1)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.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 196-199: The Javadoc for Display.reset() is misleading because the
method only clears the cached oldLines but does not force a full repaint; update
the implementation of Display.reset() to both clear the cached model (oldLines)
and set the internal repaint flag (reset = true) and/or issue a terminal clear
so the next render repaints the entire display; locate the reset() method and
ensure it resets oldLines and toggles the reset flag used by render() (or
invokes the same code path as a full clear) so no stale content remains.
🪄 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: 1018f654-29c0-41fe-82f1-437528c9ed0b
📒 Files selected for processing (9)
builtins/src/main/java/org/jline/builtins/ScreenTerminal.javabuiltins/src/main/java/org/jline/builtins/SwingTerminal.javademo/src/main/java/org/jline/demo/examples/DisplayExample.javademo/src/main/java/org/jline/demo/examples/MultiSegmentStatusExample.javademo/src/main/java/org/jline/demo/examples/ResponsiveUIExample.javademo/src/main/java/org/jline/demo/examples/TerminalCursorExample.javaremote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.javaterminal/src/main/java/org/jline/terminal/Terminal.javaterminal/src/main/java/org/jline/utils/Display.java
✅ Files skipped from review due to trivial changes (2)
- demo/src/main/java/org/jline/demo/examples/DisplayExample.java
- demo/src/main/java/org/jline/demo/examples/ResponsiveUIExample.java
🚧 Files skipped from review as they are similar to previous changes (5)
- demo/src/main/java/org/jline/demo/examples/TerminalCursorExample.java
- demo/src/main/java/org/jline/demo/examples/MultiSegmentStatusExample.java
- builtins/src/main/java/org/jline/builtins/SwingTerminal.java
- builtins/src/main/java/org/jline/builtins/ScreenTerminal.java
- remote-telnet/src/main/java/org/jline/builtins/telnet/TelnetIO.java
gnodet
force-pushed
the
dust-macaw
branch
2 times, most recently
from
d9bf560 to
bca81d1
Compare
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Standardize terminal dimension naming across the JLine API to use
columns/rowsconsistently, aligning with the convention used by POSIX (ws_col/ws_row), ncurses, Lanterna, crossterm, xterm.js, and Node.js.Inspired by the discussion in #1731.
API additions (non-breaking)
SizeSize(Size)copy constructorTerminaldefault int getColumns(),default int getRows()DisplaygetColumns(),getRows(),resize(Size)ScreenTerminalgetColumns(),getRows(),setSize(Size)Deprecations
TerminalgetWidth()getColumns()TerminalgetHeight()getRows()ScreenTerminalgetWidth()getColumns()ScreenTerminalgetHeight()getRows()Displayresize(int rows, int columns)resize(Size)Caller migrations
display.resize(size.getRows(), size.getColumns())→display.resize(size)terminal.getWidth()/getHeight()→terminal.getColumns()/getRows()width/heightparameters tocolumns/rowsin constructors and methods acrossScreenTerminal,SwingTerminal,WebTerminal,ConnectionData,TelnetIONot changed
TerminalGraphics.ImageOptions.size(int width, int height)— pixel dimensions, not terminal cellsSwingTerminal.dump(...)/Tmux.VirtualConsole.resize(...)— viewport rectanglesSummary by CodeRabbit
Refactor
New Features
Deprecation
User-visible fixes
Documentation
Tests