Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Resizing on the alternate screen can recreate scrollback and restore the cursor onto the wrong history line.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Implements DEC alternate-screen buffers so full-screen terminal applications preserve shell content and scrollback.
Changes:
- Handles DEC modes 47, 1047, and 1049.
- Saves/restores terminal content and cursor state.
- Adds snapshot notifications and alternate-screen tests.
| File | Description |
|---|---|
VT100EmulatorTest.java |
Tests escape-sequence switching. |
VT100EmulatorBackendTest.java |
Tests buffer restoration and resizing. |
TerminalTextData.java |
Notifies snapshots after copying. |
VT100EmulatorBackend.java |
Implements alternate-buffer lifecycle. |
VT100Emulator.java |
Dispatches DEC screen modes. |
VT100BackendTraceDecorator.java |
Traces screen switches. |
IVT100EmulatorBackend.java |
Adds the switching API. |
META-INF/MANIFEST.MF |
Increments the bundle version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results 33 files - 21 33 suites - 21 37m 5s ⏱️ - 21m 14s For more details on these errors, see this check. Results for commit 49aeb35. ± Comparison against base commit 470ac3d. This pull request skips 23 tests. |
Full screen programs - vi, less, htop, and lately CLIs that draw their own UI - ask for the alternate screen with CSI ? 1049 h and give it back with CSI ? 1049 l. The emulator accepted the sequences and ignored them, so such a program drew over the shell's scrollback and left its last screen behind when it exited. Switching to the alternate screen now saves the normal buffer and the cursor, clears the screen and caps the buffer at the screen height, so that scrolling drops the top line instead of growing history the alternate screen is not supposed to have. Switching back restores the saved buffer and cursor, lifts the cap again, and brings the buffer to the current width and at least the screen height, since the window may have been resized in the meantime: a narrower buffer made every write past its old margin throw, a shorter one put the top of the screen above its first line. The restored buffer tells its snapshots so the view redraws instead of keeping the program's last screen. Modes 47 and 1047 are treated the same; 1048 (save/restore cursor alone) stays ignored. Asking for the screen one is already on is a no-op, as programs do ask twice.
The alternate screen caps its buffer at the screen height so that nothing scrolls into history. The cap was set once, on entry, so after the window shrank the buffer grew back to the old height and left history behind. It now follows the screen height on every resize. The cursor of the normal screen was saved as a row of the screen. When the window grew meanwhile, the same row pointed into older history on return, and the next output overwrote it. It is now saved as a line of the normal buffer and turned back into a row of the screen as it is on return. Found by review of eclipse-platform#2899.
|
Rebased on current master and addressed both review comments in 0924176, with tests. |
The previous commit lowered the cap to the new screen height while the buffer could still be higher. TerminalTextDataFastScroll, the buffer the terminal actually uses, rejects that with an IllegalArgumentException, so shrinking the window while a full screen program was running threw from the resize handler. TerminalTextDataStore, used by the tests, accepts it, which is how it went unnoticed. The alternate screen now keeps its buffer exactly as high as the screen: on a shorter screen the lines above the cursor are dropped, as many as it takes to keep the cursor on the screen, and the cap follows only after. The test runs on both buffers.
|
Follow-up in 84b7af6: 0924176 lowered the cap while the buffer could still be higher than the new screen, which TerminalTextDataFastScroll (the buffer the terminal actually uses) rejects with an IllegalArgumentException, so shrinking the window under a full screen program threw from the resize handler. TerminalTextDataStore, which the tests use, accepts it. The alternate screen now keeps its buffer exactly as high as the screen, dropping lines above the cursor on a shorter one, and only then sets the cap. testAlternateScreenShrunkKeepsNoHistory now runs on both buffers. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Alternate-screen state can survive reconnection, preference updates can re-enable history, and duplicate disable sequences reset normal-screen state.
Review effort: Balanced
Findings: 1
Open (5)
| public void enableAlternateScreen(boolean enable) { | ||
| synchronized (fTerminal) { | ||
| if (enable == (fNormalScreen != null)) { |
There was a problem hiding this comment.
Fixed in 381accb: resetState() now leaves the alternate screen, which puts the normal screen and its history back, so the next connection starts there and a late CSI ? 1049 l no longer brings back the old program's screen. Covered by testResetStateLeavesAlternateScreen, on the terminal's real buffer.
| // Use Normal Screen Buffer, putting back what was on it. | ||
| text.setScrollRegion(-1, -1); | ||
| text.enableAlternateScreen(false); |
There was a problem hiding this comment.
Fixed in 381accb: the scroll region is now reset by the backend when it actually switches back to the normal screen, so a repeated request leaves the normal screen's margins alone. Covered by testRepeatedAlternateScreenOffKeepsMargins.
| fNormalMaxHeight = fTerminal.getMaxHeight(); | ||
| // clearAll leaves the buffer the size of the screen, which is what the | ||
| // alternate screen is: no history to scroll back through. Capping the | ||
| // buffer there keeps it so: scrolling drops the top line instead of | ||
| // growing the buffer, as a program on this screen expects. | ||
| clearAll(); | ||
| fTerminal.setMaxHeight(fLines); |
There was a problem hiding this comment.
Fixed in 381accb: VT100TerminalControl.setBufferLineLimit() now goes through the emulator to a new backend method. While the alternate screen is showing, the backend keeps the limit for the normal screen and applies it on return, so the cap stays and the new limit is not lost. Covered by testBufferLineLimitWhileAlternateScreen.
…ence changes Three ways the alternate screen state could leak, found by review: - resetState(), which runs before every connection, did not leave the alternate screen. A connection that ended while a full screen program had the screen left the next one on the capped buffer, without history, and a later CSI ? 1049 l brought back the old screen. Reset now leaves the alternate screen, putting the normal one back. - CSI ? 1049 l reset the scroll region before finding out whether the alternate screen was showing at all, so a repeated request cleared the margins of the normal screen. The backend now resets them when it actually switches back. - setBufferLineLimit(), called on every preference event, set the model's maximum height directly. On the alternate screen that lifted the cap and history accumulated, and on return the limit from before overwrote the new one. The limit now goes through the backend, which keeps it for the normal screen while the alternate one is showing. The tests run on the terminal's real buffer and fail without the change.
Full screen programs - vi, less, htop, and lately CLIs that draw their own UI - ask for the alternate screen with CSI ? 1049 h and give it back with CSI ? 1049 l. The emulator accepted the sequences and ignored them, so such a program drew over the shell's scrollback and left its last screen behind when it exited.
Switching to the alternate screen now saves the normal buffer and the cursor, clears the screen and caps the buffer at the screen height, so that scrolling drops the top line instead of growing history the alternate screen is not supposed to have. Switching back restores the saved buffer and cursor, lifts the cap again, and brings the buffer to the current width and at least the screen height, since the window may have been resized in the meantime: a narrower buffer made every write past its old margin throw, a shorter one put the top of the screen above its first line. The restored buffer tells its snapshots so the view redraws instead of keeping the program's last screen.
Modes 47 and 1047 are treated the same; 1048 (save/restore cursor alone) stays ignored. Asking for the screen one is already on is a no-op, as programs do ask twice.