Repository navigation
fix: retain reader cursor tracking on status resize - #2326
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preserving reader cursor tracking during status-row terminal resizing. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. No issues found.
The core fix is correct: replacing doDisplay() (which created a new Display object, discarding cursor tracking) with display.resize(size) + display.reset() preserves the cursor offset via reflow while forcing a full repaint — exactly what's needed.
The synchronized → ReentrantLock migration is the right call. The old code used synchronized(this) while all other concurrency went through the lock field, leaving a window where WINCH could mutate display while a widget was executing. The new code uses the same lock as the rest of the reader.
Test suite is comprehensive: 6 geometry variants, prompt-in-cleanup-area edge case, wrapped input, a full readLine integration test with post-resize input acceptance, and a lock-contention test that directly verifies mutual exclusion. Pre-fix validation: the assertStatusResize tests check that -- user -- appears exactly once after WINCH — the old doDisplay() path would have produced a duplicate prompt, so these tests would have failed before the fix.
One minor suggestion: the handleSignal Javadoc (which is protected and thus part of the subclass API) could gain an @implNote documenting the locking contract:
* @implNote This method acquires the reader's {@code lock} to serialize
* with the input thread. Subclasses that override should do the same.
Not a blocker.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Resizing a terminal with active
Statuslines recreates the reader'sDisplay, losing the cursor offset into the existing prompt and input. The next redisplay starts from the current cursor and can leave a second copy of the prompt.Resize the existing display to preserve its cursor offset, then invalidate its rendered lines so redisplay also repaints any prompt rows cleared by
Status.resize(). Serialize signal handling with the existing reader lock so resize/reset cannot mutate a display while the input thread is rendering it. Use that lock instead of the separate object monitor to avoid reversing lock order for in-band resize callbacks. The no-status, empty-status and unchanged-grid rendering paths retain their existing behavior.Add native virtual-terminal regressions for width/height changes where the prompt stays in the usable scroll region, status borders, prompt rows inside the cleanup area, wrapped input, and continued editing. A separate
readLinetest exercises the installed WINCH handler and accepts further input after resizing.Existing limitation: shrinking a full screen far enough that the prompt falls into the new status rows can still corrupt the status area. This patch does not address that pre-existing case.
Validation (Linux, JDK 25):
The duplicate-prompt, misplaced-prompt and concurrent-resize cases were also checked against unchanged upstream; unchanged-grid handling is a passing control. Testing uses JLine's virtual terminal; physical-terminal and cross-platform runs were not performed. Width-change behavior uses the same terminal-reflow assumption as the existing no-status path. Non-reflowing terminals with wrapped input are not covered; width-change tests use unwrapped input and the wrapped-input test changes height.
Refs #2319
Summary by CodeRabbit