Give the status line a row to be written to - #148
Merged
Conversation
JohnCampionJr
force-pushed
the
feat/status-line
branch
from
September 1, 2026 00:48
357cd8e to
d9dc2fd
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cursor preservation, alternate-buffer routing, event batching, reset reporting, zero-size handling, and disposal have correctness gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements DEC status-line routing while keeping it outside the main terminal grid.
Changes:
- Adds status-line state, cursor routing, events, resizing, and reset handling.
- Connects DECSSDT/DECSASD parsing to terminal behavior.
- Adds status-line tests.
File summaries
| File | Description |
|---|---|
src/XTerm.NET/Terminal.cs |
Implements status-line lifecycle and routing. |
src/XTerm.NET/InputHandler.cs |
Applies parsed status-display controls. |
src/XTerm.NET.Tests/StatusLineTests.cs |
Tests status-line behavior. |
Review details
Suppressed comments (6)
src/XTerm.NET/Terminal.cs:128
- This does not actually enforce “at most once per batch.”
_statusLineDirtyis never set, while both status setters raise synchronously; a singleWrite(Type(2) + Select(1))therefore emits two setter events and a third event here because the line is active. Queue setter changes for this flush (or otherwise introduce an explicit batching scope) so listeners receive the documented single notification.
private void FlushStatusLineChange()
{
if (!_statusLineDirty && !_statusLineActive)
return;
_statusLineDirty = false;
RaiseStatusLineChanged();
src/XTerm.NET/Terminal.cs:199
- DECSASD is documented here as giving each display its own cursor, but every selection homes the status cursor. For example, writing
abc, selecting the main display, then selecting the status line and writingdoverwrites column 0 instead of continuing at column 3. The new buffer already starts at (0,0), so preserve its cursor on subsequent selections.
_buffer.SetCursor(0, 0);
src/XTerm.NET/Terminal.cs:192
- Saving only X/Y loses the main display's pending-wrap state. If the cursor is at the phantom column after filling a line, the restore through
SetCursorclamps it onto the last cell and clears pending wrap, so the next character overwrites that cell instead of wrapping. Save and restorePendingWrapas part of this display cursor (using the raw cursor restore path where necessary).
_cursorXBeforeStatus = _buffer.X;
_cursorYBeforeStatus = _buffer.Y;
src/XTerm.NET/Terminal.cs:198
- A later DECSET 47/1049 breaks this routing:
SwitchToAltBufferunconditionally replaces both_bufferand the input handler's buffer while_statusLineActiveremains true. Subsequent text then lands in the alternate screen even though DECRQSS still reports the status display selected. Keep the status buffer as the write target while updating only which underlying main display will be restored.
_buffer = _statusBuffer;
_inputHandler.SetBuffer(_buffer);
src/XTerm.NET/Terminal.cs:1098
- RIS resets only the new
Terminalfields. DECRQSS still readsInputHandler._activeStatusDisplayand_statusDisplayType, andResetStoredModesdoes not clear them, so after selecting type 2/status 1 and issuing RIS,$}and$~incorrectly continue reporting 1 and 2. Reset the cached values too, or make DECRQSS read the terminal properties as the single source of truth.
SetActiveStatusDisplay(0);
SetStatusDisplayType(0);
src/XTerm.NET/Terminal.cs:206
Resizeexplicitly permits zero dimensions, but restoring the cursor after such a resize callsTerminalBuffer.SetCursor, whose clamp uses maxima ofCols - 1/Rows - 1and throws when either dimension is zero. Guard this restore soResize(0, 0)followed by DECSASD 0 remains valid.
_buffer.SetCursor(_cursorXBeforeStatus, _cursorYBeforeStatus);
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DECSSDT and DECSASD were parsed, stored so DECRQSS could report them, and otherwise ignored -- which was worse than not implementing them at all. A program asked whether its status line had been accepted, was told yes, wrote to it, and the text went wherever the cursor happened to be: into the middle of its own display. vttest's simple status-line test shows it as one row reading TEXT IN THE STATUS LINEThere should be TEXT IN THE STATUS LINE. There is a row now. DECSSDT 2 creates it, DECSASD 1 sends what is written next into it, DECSASD 0 hands the cursor back. The buffer swap is the alternate screen's, for the same reason: the input handler writes through whichever buffer it was given, so redirecting output is a matter of handing it a different one rather than teaching every write about status lines. What the two controls now refuse is as much of the fix as what they accept. DECSASD 1 is declined unless a HOST-WRITABLE status line exists -- type 0 has no row and the indicator's contents belong to the terminal -- and the value stored for DECRQSS is the one that was ACCEPTED, so a program is no longer told yes and then given nowhere to put its text. Each display keeps its own cursor, or a program that writes a status message loses its place mid-screen. Removing the status line hands the cursor back first, or the row stops existing while it still has it. RIS takes the whole thing with everything else it undoes. The row is deliberately not one of Rows. An application told it has N rows must have N rows it can write to, and every size report is computed from that count -- so the status line is exposed as a property for a host to draw below the text area, with a change event shaped like TitleChanged. That event is raised at the BATCH boundary, and the first version of this asked the question on the print path instead. Per CLAUDE.md's first section: a per-character test for a state that is almost never on pays a compare for every character the terminal ever prints, to catch a message that arrives once a second. Being selected is enough to count as changed -- while DECSASD has the status line, every printed character goes into it by definition. 2159 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six findings, all real, all mine. The batching the comment described was not happening. The dirty flag was never set and both setters raised synchronously, so one write carrying DECSSDT, DECSASD and a message emitted three events -- two from the setters and one from the flush. The setters now record instead of raising while a batch is being parsed, and a host calling the API outside one is told immediately, because outside a batch there is no end to wait for. Selecting the status line homed its cursor every time, which contradicted the per-display cursor the same method documents: a program writing half a message, stepping back to the screen and returning began again at column one and overwrote what it had written. The buffer starts at (0, 0), which is where a first selection finds it, and later selections leave it alone. Saving only X and Y lost the main display's pending wrap. The phantom column past the last cell is a real cursor position and the one the print path reads to decide whether to wrap, and SetCursor clamps it onto the last cell and clears the flag -- so a line about to wrap came back as one that overwrote its own last character. Restored raw, clamped to Cols rather than Cols - 1, which lets SetCursorRaw derive the flag itself. DECSET 1049 while the status line was selected reassigned the write target, sending the program's next characters to the alternate screen while DECRQSS still reported the status line. The review suggested keeping the status row as the target; that is not available, because the switch's own work -- 1049's erase among it -- runs through the input handler's buffer, and a full-screen erase against a one-row buffer indexes off the end of it. Switching screens ends the status line's turn with the cursor instead, so one rule holds: the status line is selected, or the screen is. The row itself survives. DECRQSS read cached copies in the input handler that nothing reset, so after RIS it went on reporting the status line selected and host-writable when both had been undone -- the same lie this PR exists to remove, in a new place. It reads the terminal now, and the cached fields are gone rather than left unused. Restoring the cursor after Resize(0, 0) threw: the clamp is Clamp(x, 0, Cols - 1), whose maximum is negative at zero, and Resize explicitly allows zero because a host reports it before its control has been laid out. Eight tests added, one per finding plus the two halves of the screen switch. 2167 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StatusLineChanged was added without being added to Dispose, so a disposed terminal held its subscribers. The subscriber is usually the host, which holds a control, which holds a window -- a leak with the terminal on the far end of it. The test that should have caught it is called Dispose_ClearsAllEvents and checks two events. It has been right about those two since it was written, and it cannot be wrong about the others because it never looks at them, which is how this got in. So it is enumerated now rather than listed. The new test subscribes to every public event on Terminal through reflection -- so a field that happened to be null already cannot pass by accident -- disposes, and asserts every backing field came back null. An event added tomorrow is covered the day it is declared, and one that is forgotten is named in the failure. Confirmed it bites: with the new line removed the test fails and reports the retained event, which is the whole point of writing it this way rather than appending a third name to the old one. 2168 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JohnCampionJr
force-pushed
the
feat/status-line
branch
from
September 1, 2026 01:05
05a56e4 to
7d1880a
Compare
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
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.
Closes #140.
DECSSDT and DECSASD were parsed, stored so DECRQSS could report them, and otherwise ignored — which, as the issue says, is worse than not implementing them. A program asked whether its status line had been accepted, was told yes, wrote to it, and the text went wherever the cursor happened to be: into the middle of its own display.
This is the issue's third option — implement it — rather than the first two.
What it does
DECSSDT 2creates the row,DECSASD 1sends what is written next into it,DECSASD 0hands the cursor back. The buffer swap is the alternate screen's, for the same reason: the input handler writes through whichever buffer it was given, so redirecting output is a matter of handing it a different one rather than teaching every write about status lines.What it refuses, which is as much of the fix
DECSASD 1is declined unless a host-writable status line exists — type 0 has no row, and the indicator's contents belong to the terminal, not the application. The value stored for DECRQSS is the one that was accepted, so a program is no longer told yes and then given nowhere to put its text. That was the specific mechanism that turned a missing feature into corruption of the application's screen.The parts that bite
Rows. An application told it has N rows must have N rows it can write to, and every size report is computed from that count. It is exposed as a property for a host to draw below the text area, with aStatusLineChangedevent shaped likeTitleChanged.One note on where the event is raised
The first version of this asked "did that write touch the status line?" on the print path. CLAUDE.md's opening section is about exactly that: a per-character test for a state that is almost never on pays a compare for every character the terminal ever prints, to catch a message that arrives once a second. It is asked at the batch boundary instead, where it is asked once — and being selected is enough to count as changed, because while DECSASD has the status line every printed character goes into it by definition. The cost of not tracking it precisely is a repaint of an unchanged row; the cost of tracking it precisely is paid by every character.
Tests
Ten, including vttest's own sequence asserting the capitals land on the status line and the sentence stays intact on the screen; the cursor returning to where the program left it; both refusals; the removal ordering; the row staying out of
Rows; width following resize; one event per batch; and RIS.2159 passed, 0 failed.Not in this PR
The rendering half is tomlm/Iciclecreek.Avalonia.Terminal#151, which is written against this API and waiting on it.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com