Answer the last five vttest queries, and trim blanks from area checksums - #151
Merged
JohnCampionJr merged 1 commit intoSep 1, 2026
Merged
Conversation
Fixes tomlm#123 Fixes tomlm#128 Fixes tomlm#139 Fixes tomlm#142 Fixes tomlm#145 The remaining open issues, all found sweeping vttest. Four are queries that produced silence; the fifth is an answer that was wrong by a fixed amount. DA3, CSI = c, was not in the identifier table, so it never reached the handler that was already shaped for it. It now replies DCS ! | 00000000 ST -- xterm's DECRPTUI, zeros for the site code and serial number, because there is no unit to identify. VT400 and up, as in xterm: the control arrived with the VT420, and a program that dropped the level with DECSCL is entitled to a VT200's silence. A non-zero parameter is still ignored, since that is another terminal's reply arriving on our input. DECRQCRA counted every blank cell. The correct rule is that a run of blanks at the end of a row is trimmed rather than summed, so a blank is worth 0x20 only when something further along its own row follows it. That is the whole of the discrepancy vttest reported: its own test computes the two rows it checks with the blanks worth nothing, and this emulator came back exactly 95 spaces high. The first cell counted is exempt and adds whatever it holds. That is not decoration -- esctest reads content back one cell at a time and expects a written space as 0x20, so trimming unconditionally would answer zero for every space on screen. vttest builds its expectation with the same exception in the same place. xterm and its own documentation disagree here, and the divergence is recorded on the method. xtermCheckRect only ever accumulates its held-back run under csNOTRIM -- the flag meaning do not trim at all, under which the trimmed total is then discarded -- so its default mode drops interior blanks too. The two readings agree on every screen either tool checks; this follows the documented one. Attributes still contribute nothing, where xterm weights six of them into the cell value, because esctest compares a cell's checksum to the bare codepoint of the character it expects. DECRQDE answers CSI rows ; cols ; 1 ; 1 ; 1 " w. The window is the page, so the corner is 1;1 and there is one page -- the DEC spelling of what CSI 18 t has always answered in the dtterm dialect, and the only one of the two forms that stayed quiet. DECRQUPSS answers DCS 1 ! u A ST. The choice UPSS offers is DEC Supplemental or ISO Latin-1, and a UTF-8 terminal reaches Latin-1, which is the conclusion xterm reaches for the same reason. DECAUPSS is not implemented, so the default is the only value this can report; saying so still beats the silence a client waits on forever. DECRQTSR answers DCS 0 $ s ST. DECTSR serialises state for DECRSTS to restore, there is no DECRSTS, and a terminal that cannot restore state has no business claiming it can report it -- so this is a refusal rather than a payload nothing can consume. But refusing is not saying nothing: DECRQSS already declines with DCS 0 $ r ST, and this is that shape one final character over. Ps absent or zero asks for nothing and gets nothing, per the VT510 manual. DECCARA and DECRARA were the two members of the rectangle family that change attributes rather than characters, and both were missing. They honour DECSACE, which until now was parsed, stored, reported back by DECRQSS and read by nothing: 2 is the rectangle the coordinates describe, anything else the stream running from the top-left position to the bottom-right one across whole intervening lines. Only DEC's set is touched -- 0, 1, 4, 5, 7, 8 and the resets 22, 24, 25, 27, 28, with the resets ignored under DECRARA because reversing an attribute already says both directions. Parameter 0 covers xterm's SGR_MASK, which is bold, underline, blink and inverse, and deliberately not invisible. Two things fell out of giving DECSACE teeth. It now resets on RIS and not on DECSTR, which is where xterm clears it -- the full-reset branch of ReallyReset and nowhere else -- and a stale rectangle setting would otherwise turn the next program's stream into a box. And unlike xterm every cell in the area is marked, the trailing half of a wide character included: xterm skips cells it has never drawn, but a line here is born full of spaces, so the only cell that reads as empty is that half, and skipping it would leave one character disagreeing with itself about its own rendition. OSC 50 asks the host through a new FontQueryRequested event, on the OSC 52 model -- the emulator has no font, whatever draws its cells does. The part worth having is what happens when the host declines: xterm answers a font query it cannot satisfy with a NAMELESS OSC 50, and vttest reads exactly that, skipping replies that carry no name. An indexed query is always nameless, since there is no font menu to index, and the host is not asked. Setting a font is left to the host and reported as unrecognised, so a listener on OscReceived can act on it without having to work out whether the terminal already did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JohnCampionJr
force-pushed
the
fix/vttest-remaining-reports
branch
from
September 1, 2026 02:09
2f1a426 to
8464257
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Attribute-only updates can delete Sixel placements, and several VT420 controls bypass conformance-level gating.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the remaining vttest query responses, rectangular attribute operations, OSC 50 host integration, and corrected checksum trimming.
Changes:
- Implements DA3, DECRQDE, DECRQUPSS, and DECRQTSR responses.
- Adds DECCARA/DECRARA with DECSACE behavior and reset handling.
- Adds OSC 50 font queries and trims trailing blanks from area checksums.
File summaries
| File | Description |
|---|---|
src/XTerm.NET/Terminal.cs |
Exposes font queries and resets DECSACE. |
src/XTerm.NET/InputHandler.StoredModes.cs |
Adds DECSACE hard-reset support. |
src/XTerm.NET/InputHandler.Rectangles.cs |
Implements rectangular attribute changes. |
src/XTerm.NET/InputHandler.Osc.cs |
Handles OSC 50 font queries. |
src/XTerm.NET/InputHandler.Csi.cs |
Adds reports and checksum trimming. |
src/XTerm.NET/InputHandler.cs |
Dispatches new CSI/OSC commands and DA3. |
src/XTerm.NET/Events/TerminalEvents.cs |
Defines font-query event arguments. |
src/XTerm.NET/Common/OscCommand.cs |
Registers OSC 50. |
src/XTerm.NET/Common/CsiCommand.cs |
Defines new CSI commands. |
src/XTerm.NET/Common/CommandExtensions.cs |
Maps new sequence identifiers. |
src/XTerm.NET.Tests/RectangleOpsTests.cs |
Tests attribute-area behavior. |
src/XTerm.NET.Tests/OscSequenceTests.cs |
Tests OSC 50 handling. |
src/XTerm.NET.Tests/InputHandlerTests.cs |
Tests DA3 behavior. |
src/XTerm.NET.Tests/DeviceReportTests.cs |
Tests new device reports. |
src/XTerm.NET.Tests/Common/CsiCommandExtensionsTests.cs |
Updates command mapping expectations. |
src/XTerm.NET.Tests/ChecksumReportTests.cs |
Tests blank trimming rules. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
JohnCampionJr
added a commit
that referenced
this pull request
Sep 1, 2026
…le family (#152) Addresses the three findings on #151. A rendition change is not a text write, and DECCARA and DECRARA were making one. Both went through SetCell, which is the text-write path: it splits the Sixel and Kitty placements at the column being written, on the reasonable assumption that a cell being written is a cell whose character is changing. For these two it never is -- they set attributes and leave every character alone -- so bolding a region that happened to contain a picture punched a hole in it. The cells that change now go back through the indexer, which stores the cell and invalidates the render cache without touching the placements. FillCells still uses SetCell, deliberately: DECFRA and DECERA do replace characters, and a picture under them is being overwritten. The parameter list is read once instead of per cell. It was re-walked for every cell in the area, asking a question whose answer cannot vary across it -- a full-screen request re-read the whole list a parameter at a time, once per cell. It now folds into one operation per attribute before the walk starts, composing rather than appending so that one pass stays faithful to the list's order: a later parameter overrides an earlier one, and a toggle applied to a pending toggle cancels it, exactly as xterm's per-cell XOR does when the same bit is named twice. That is also what makes the no-op case free, which is the other half of the placement fix. A request naming nothing this implements -- CSI 1;1;1;10;31 $ r, a colour -- or a DECRARA whose toggles cancel now returns before a cell is touched. Writing a cell back unchanged is still writing it as far as the placements are concerned, so a control that did nothing at all was carving up images. The rectangle family is VT400 and was ungated. xterm gates every one of them at vtXX_level >= 4 -- DECCRA, DECERA, DECFRA, DECSERA, DECCARA, DECRARA and DECRQCRA -- and this terminal's own primary DA already says the same thing by advertising attribute 28, rectangular editing, only from level 64. Acting on the controls at a level where the DA reply denies them is the terminal contradicting itself, and a program that lowered the level with DECSCL asked to be treated as older hardware. esctest already assumes this gate: it asserts VT level 4 before it reads a single cell back through DECRQCRA. DECSACE is the one exception, and it is xterm's asymmetry rather than an oversight on this side: its handler has no level test where every neighbour has one. Storing which extent a program would prefer changes nothing by itself, since the two controls that read it are gated, so there is nothing to refuse. DECRQTSR is gated with them. The control is VT320 vintage, but the capability the primary DA offers for it -- attribute 17, terminal state interrogation -- is advertised only from level 64, and declining a request the DA reply has already said the terminal does not take is the same contradiction one report over. It was answering at every level. The placement test was checked against the bug: with SetCell put back it fails, which is how the FillCells regression above was caught before it shipped rather than after. 2209 passed, 0 failed. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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 the five remaining open issues. Four are queries that answered nothing; the fifth is an answer that was wrong by a fixed amount. Full suite green: 2175 passed, 0 failed (+26 tests).
#123 — DA3 goes unanswered
CSI = cwas not in the identifier table, so it never reached the handler that was already shaped for it. It now repliesDCS ! | 00000000 ST— xterm's DECRPTUI, zeros for the site code and serial number, because there is no unit to identify. VT400 and up as in xterm: the control arrived with the VT420, and a program that dropped the level with DECSCL is entitled to a VT200's silence. A non-zero parameter is still ignored, since that is another terminal's reply arriving on our input.#128 — DECRQCRA counts untouched cells as spaces
The issue's direction was right and its mechanism was not, which is the part worth reading. Checked against
xtermCheckRectin xterm'sscreen.c: xterm skips cells it has never drawn, but a line in this buffer is born full of drawn spaces, so "skip the undrawn" would have changed nothing at all. What does the work is trimming: a run of blanks at the end of a row is dropped rather than summed, so a blank is worth0x20only when something further along its own row follows it.That is the whole of the reported discrepancy. vttest computes the two rows it checks with the blanks worth nothing, and this emulator came back exactly 95 spaces high.
The first cell counted is exempt and adds whatever it holds. Not decoration: esctest reads content back one cell at a time and expects a written space as
0x20, so trimming unconditionally would answer zero for every space on screen. vttest builds its expectation with the same exception, in the same place — which is why both tools agree once the rule has it.One judgement call, recorded on the method. xterm's code and xterm's documentation disagree here.
xtermCheckRectonly ever accumulates its held-back run undercsNOTRIM— the flag meaning do not trim at all, under which the trimmed total is then discarded — so its default mode drops interior blanks too. The two readings agree on every screen either tool checks; this follows the documented one. Happy to switch to the literal xterm behaviour if you would rather match the implementation than the spec.Attributes still contribute nothing, where xterm weights six of them into the cell value (bold
0x80, blink0x40, inverse0x20, underline0x10, invisible0x8, protected0x4). esctest compares a cell's checksum to the bare codepoint of the character it expects, so a weight per attribute bit would fail every assertion on styled text. That divergence predates this change and is now documented rather than fixed.#139 — three VT320 reports
Implemented as the issue triaged them.
CSI rows ; cols ; 1 ; 1 ; 1 " wDCS 1 ! u A STDCS 0 $ s STThe window is the page, so DECRQDE's corner is
1;1and there is one page — the DEC spelling of whatCSI 18 thas always answered in the dtterm dialect, and the only one of the two forms that stayed quiet. Both it and DECRQUPSS are VT300 and up, as in xterm.DECRQUPSS reports ISO Latin-1: the choice UPSS offers is DEC Supplemental or Latin-1, and a UTF-8 terminal reaches Latin-1, which is the conclusion xterm reaches for the same reason. DECAUPSS is not implemented, so the default is the only value this can report — still better than silence.
DECRQTSR is the refusal the issue argued for rather than a payload nothing can consume: DECTSR serialises state for DECRSTS to restore, there is no DECRSTS, and a terminal that cannot restore state has no business claiming it can report it. Refusing is not saying nothing — DECRQSS already declines with
DCS 0 $ r ST, and this is that shape one final character over.Psabsent or zero asks for nothing and gets nothing, per the VT510 manual.#142 — DECCARA and DECRARA missing, DECSACE inert
Both implemented, and DECSACE now has its only consumer:
2is the rectangle the coordinates describe, anything else the stream running from the top-left position to the bottom-right one across whole intervening lines. Only DEC's set is touched —0,1,4,5,7,8and the resets22,24,25,27,28, with the resets ignored under DECRARA because reversing an attribute already says both directions. Parameter0covers xterm'sSGR_MASK(bold, underline, blink, inverse) and deliberately not invisible.Two things fell out of giving DECSACE teeth, both worth a look:
ReallyResetand nowhere else. While the setting was inert a stale value was harmless; now it turns the next program's stream into a box.#145 — the OSC 50 font query
Asks the host through a new
FontQueryRequestedevent, on the OSC 52 model the issue pointed at: the emulator has no font, whatever draws its cells does.The part worth having is what happens when the host declines. xterm answers a font query it cannot satisfy with a nameless
OSC 50— the reply with its semicolon and name left off — and vttest reads exactly that, skipping replies that carry no name. So a host that does not answer, or is not subscribed, still produces a reply rather than leaving the client blocked. An indexed query (?#2,?+1) is always nameless since there is no font menu to index, and the host is not asked. Setting a font is left to the host and reported as unrecognised, so a listener onOscReceivedcan act on it without having to work out whether the terminal already did.Notes
No hot-path changes — everything lands on query and dispatch paths or in new methods — so no bench run. The five subjects share files but are independent; happy to split into separate PRs if you would rather review them apart.
🤖 Generated with Claude Code