Cursor motion, margins and tab stops as xterm does them - #93
Conversation
Perf comparison3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
Worth a look — over the floor, under this run's gate, so not failed:
Re-run on a quieter machine, or with more |
There was a problem hiding this comment.
🟡 Changes recommended
Tab-stop behavior is incomplete/inconsistent because CBT (CSI Z) and TBC (CSI g) do not yet use the new real tab-stop set, so programs cannot reliably clear or backward-tab to custom stops.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR is part of the ongoing audit to align XTerm.NET’s behavior with xterm (and reference VT500 parsing behavior), focusing on cursor motion within margins/scroll regions, tab-stop correctness, and hard bounds for hostile/untrusted input so the terminal stays responsive.
Changes:
- Align cursor motion/editing operations with xterm semantics (scroll-region-aware CUU/CUD/CNL/CPL; correct “phantom column” behavior; DECAWM-off overwrite behavior; proper scroll-region parameter defaults; more accurate DECSC/DECRC state).
- Implement “real” tab stops (HTS/TBC) and unify tab motion behavior around them; add HPA/HPR/VPR mappings.
- Add parser/bounds hardening: correct CAN/SUB cancellation, C1 ST OSC termination, DEL handling, CSI param saturation, OSC payload cap, cluster/table caps; bound PNG inflate and Kitty payload accumulation; add regression tests.
File summaries
| File | Description |
|---|---|
| src/XTerm.NET/Terminal.cs | Adds tab-stop storage + helpers; integrates tab-stop reset into ctor/reset/resize; HT now uses tab-stop set; negative resize guard. |
| src/XTerm.NET/Parser/EscapeSequenceParser.cs | VT500-state-machine fixes (CAN/SUB cancellation, ESC clear action, ST/DEL behavior) plus parameter saturation and OSC payload cap/reset hygiene. |
| src/XTerm.NET/InputHandler.cs | Cursor/margin correctness fixes, DECAWM-off overwrite semantics, editing ops settling from phantom column, charset designation save/restore, HTS hook, Kitty/OSC/cluster bounds. |
| src/XTerm.NET/Graphics/PngDecoder.cs | Bounds zlib inflation to expected size to avoid decompression bombs. |
| src/XTerm.NET/Common/CommandExtensions.cs | Maps ECMA-48 HPA/HPR/VPR spellings to existing cursor motions. |
| src/XTerm.NET/Common/ClusterTable.cs | Caps distinct interned clusters to prevent unbounded process-lifetime growth. |
| src/XTerm.NET/Buffer/TerminalBuffer.cs | Extends saved cursor state (origin mode, pending-wrap) and adds controlled pending-wrap restore API. |
| src/XTerm.NET.Tests/Parser/ParserConformanceTests.cs | New tests covering VT500-diagram conformance cases and regressions. |
| src/XTerm.NET.Tests/HostileInputBoundsTests.cs | New tests asserting bounds for hostile but protocol-legal inputs (clusters, scroll counts, OSC, params, PNG inflate, resize). |
| src/XTerm.NET.Tests/CursorAndMarginTests.cs | New tests pinning xterm-compatible cursor/margin/tab-stop behavior. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for (var i = 0; i < count; i++) | ||
| _buffer.SetCursor(_terminal.NextTabStop(_buffer.X), _buffer.Y); | ||
| } | ||
|
|
||
| private void CursorBackwardTab(Params parameters) |
| _buffer.SetCursor(Math.Min(nextTabStop, _terminal.Cols - 1), _buffer.Y); | ||
| } | ||
| for (var i = 0; i < count; i++) | ||
| _buffer.SetCursor(_terminal.NextTabStop(_buffer.X), _buffer.Y); |
JohnCampionJr
left a comment
JohnCampionJr
left a comment
There was a problem hiding this comment.
Two cursor/tab-state issues should be addressed before merge.
| } | ||
|
|
||
| /// <summary>Clears the stop at <paramref name="column"/>, or every stop. TBC.</summary> | ||
| internal void ClearTabStop(int column, bool all) |
There was a problem hiding this comment.
These new tab-stop operations are not actually used by TBC or CBT: TabClear in InputHandler still clears nothing, and CursorBackwardTab still uses TabStopWidth arithmetic. Thus CSI 3 g leaves all default stops active, and backward tabs ignore HTS-created stops. The new test does not expose this because its custom column-4 stop merely precedes the unchanged default column-8 stop. Please wire both handlers to ClearTabStop and PreviousTabStop and test that removed defaults are absent.
| _currentCharset = _buffer.SavedCursorState.Charset; | ||
| RefreshActiveCharset(); | ||
| _terminal.OriginMode = _buffer.SavedCursorState.OriginMode; | ||
| _buffer.SetPendingWrap(_buffer.SavedCursorState.PendingWrap); |
There was a problem hiding this comment.
This flag alone cannot restore a pending-wrap cursor. The saved position may have X == Cols, but the preceding SetCursor clamps it to Cols - 1; the next printed character then overwrites the final cell instead of wrapping. Please restore the raw coordinate when PendingWrap is saved, and add a full-line DECSC/DECRC/print regression test.
3785d3b to
57b96f4
Compare
Perf comparison — this change, against its base3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
Perf comparison — cumulative, everything since 2.0.0-rc0023 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
|
57b96f4 to
1875002
Compare
|
Rebased onto the current #92, and both findings fixed. You were both right, and the second one is the more interesting failure. TBC and CBT were never wired to the stop set. This PR gave They were written and lost rather than never written, which is worth recording: the patch script that made those two edits asserted on a third, failed, and never reached its write — so all three were discarded together. Re-running only the third compiled, the suite passed, and nothing pointed at the hole. And the test would not have caught it, exactly as @JohnCampionJr said. The custom stop at column 4 merely preceded the untouched default at 8, so a tab landed on 4 whether or not TBC worked. The replacement clears every stop and asks whether a default went with them, which only passes if TBC does something. DECRC could not restore a pending wrap. That position is Three regression tests, each verified to fail with the fix reverted and pass with it — the property the original tab test lacked. Suite green: 1923. |
1875002 to
15b1a6d
Compare
|
Reviewed this PR's own hot path, as asked. Two touches, both minor, one of which was an allocation worth removing. Per-run: the ASCII fast-path gate gained Per-DECSC, and now gone: Everything else here is per-sequence rather than per-character: the margin limits in Perf 3×3 against main, measured with nothing else running: scroll-ascii +0.4%, sgr-churn +1.1%, truecolor +0.9%, alt-redraw +0.6%, unicode +1.9%, flood +0.4% — every corpus inside noise (±2–3%) and well under gate. Suite green: 1923. Rebased onto main with #92 merged; |
There was a problem hiding this comment.
🟡 Changes recommended
The new PendingWrap-based logic and saved charset designation storage need corrections to avoid incorrect cursor/edit behavior and cross-buffer DECSC/DECRC state leakage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
src/XTerm.NET/InputHandler.cs:4497
- CursorBackward uses PendingWrap alone to decide whether to subtract 1 from the starting column, but PendingWrap can be stale when X is within the margins (only X>WrapLimit is the phantom-column case). This makes CUB move one extra column left after ordinary printing that didn't reach the right margin.
This issue also appears in the following locations of the same file:
- line 4837
- line 6529
}
src/XTerm.NET/InputHandler.cs:4838
- SettleForEditing settles the cursor whenever PendingWrap is true, but PendingWrap can be true even when the cursor is not in the phantom column. This would make ICH/DCH/ECH operate one column to the left in common cases; gate settling on the cursor actually being past the wrap limit.
/// <summary>
src/XTerm.NET/InputHandler.cs:6527
- SaveCursor overwrites the saved charset designations globally; with per-buffer DECSC/DECRC semantics this should save the snapshot keyed by the current buffer instance.
/// the cursor mid-border finished the box in letters.
src/XTerm.NET/InputHandler.cs:6548
- RestoreCursor should restore the charset designations snapshot that was saved for the current buffer; otherwise DECRC can apply a snapshot saved on the other screen.
_buffer.SavedCursorState.PendingWrap = _buffer.PendingWrap;
}
private void RestoreCursor()
{
src/XTerm.NET/InputHandler.cs:6529
- SaveCursor persists PendingWrap directly, but PendingWrap is only meaningful as "wrap is pending" when the cursor is actually in the phantom column (X > WrapLimit). Saving it unconditionally can cause DECRC to restore a phantom-wrap state that never existed.
private void SaveCursor()
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| private CharsetMode _currentCharset; | ||
|
|
||
| /// <summary>What each G-slot pointed at when DECSC last ran. See <see cref="SaveCursor"/>.</summary> | ||
| private Dictionary<CharsetMode, Dictionary<char, string>?>? _savedCharsetDesignations; |
Ten divergences, each reproduced before fixing: - CUU/CUD/CNL/CPL ignored the scrolling region. CUF/CUB had the in/out rule already -- a cursor starting inside stops at the margin, one starting outside at the screen edge -- and the row motions now share it, so a full-screen editor's status line stops scrolling with the text. - CUB counted from the phantom column a filled line leaves the cursor in, landing one right of where every other terminal puts it. - DECAWM off DISCARDED characters past the margin; the VT100, xterm and xterm.js all keep overwriting the last column. The ASCII run path bails to Print for this rather than learning it twice, which costs nothing while wrapping is on -- the default. - CSI 0;0r clamped the region to one row. An explicit 0 means the default, exactly as a missing parameter does, and the parser seeds parameters with 0 so the bare CSI ;r form arrived the same way. - IL/DL left the cursor column alone instead of moving to the left margin, so an editor inserting a line then writing to it started from wherever the cursor happened to sit. - DECSC/DECRC saved position and SGR only. They now carry the charset DESIGNATIONS (ESC ( 0 changes what G0 means, so saving which G-set was active restored a pointer to a table the program had replaced), origin mode, and the pending-wrap flag. SavedCursor.Charset had sat unassigned, which was the tell. - ICH/DCH/ECH tested the phantom column against their right-margin guard and returned, so an editor that filled a line and inserted a character saw nothing happen. - Tab stops were arithmetic, so HTS could not create one and TBC cleared nothing -- its own comment admitted it. There is a real stop set now; �[3g �H works, and C0 HT stops hardcoding 8 while CHT honoured the option, which had the two tab motions disagreeing on one screen. - HPA, HPR and VPR were unimplemented and silently ignored, though xterm's terminfo lists hpa and programs emit it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both reviewers caught the same thing and both were right: this PR gave Terminal a real tab-stop set and then left two of its four consumers using the old arithmetic. HTS could add a stop, HT and CHT honoured it, but TBC cleared nothing and CBT ignored it -- so CSI 3 g left every default standing and backward tab disagreed with forward tab on the same screen. They were written and lost rather than never written. The patch script that made them asserted on a third edit, failed, and never reached its write, so all three were discarded together; re-running only the third compiled and passed, and nothing pointed at the hole. The tests here would not have caught it either -- as noted in review, the custom stop at column 4 merely preceded the untouched default at 8, so a tab landed on 4 whether or not TBC worked. The new one clears every stop and asks whether a DEFAULT went with them. Also from review: DECRC could not restore a cursor that was pending a wrap. That position is X == Cols, one past the last column, and SetCursor clamps it to Cols - 1 -- so the restore put the cursor ON the last cell and the next character overwrote it instead of wrapping. The flag set afterwards could not undo that; the coordinate was already gone. Restored rawly when the saved cursor carried the flag. Three tests, each verified to fail without its fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DECSC is not rare -- a full-screen program saves and restores the cursor on every redraw -- and copying the designation map allocated a Dictionary per save. There are exactly four G-slots and the enum numbers them from zero, so a fixed array indexes them directly and the save becomes four reference writes into storage allocated once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
state already lives The designations were held in an array on the input handler while every other thing DECSC saves -- position, attributes, selected G-set, origin mode, pending wrap -- sits in the buffer's own SavedCursorState. DECSC is per-screen, so that one field was shared between the normal and alternate buffers: a full-screen program saving and restoring on its redraws overwrote what the shell had saved, and after the program exited the shell's DECRC brought back the program's designations. A shell that had selected line drawing got ASCII. Putting the array in SavedCursor gives it the same lifetime as the rest, and the array being null is a better "DECSC has not run here" than the separate bool it replaces -- one less thing with its own places to be cleared and forgotten. The regression test drives the actual sequence: designate line drawing on the normal screen, DECSC, enter 1049, designate ASCII, save and restore there, leave 1049, DECRC, then print. It fails on the shared field.
f795c09 to
9d732db
Compare
|
Rebased onto main (post-#97), 1934 tests green. Copilot's per-screen finding is right, and it was mine. The designations were the only thing DECSC saves that did not live on the buffer — position, attributes, selected G-set, origin mode and pending wrap are all in The shell's line drawing comes back as ASCII. It is in The test drives that exact sequence and fails against the previous commit. The three earlier comments on this PR were addressed in the push before this one, and still stand after the rebase: CBT routes through |
SetCursorRaw set PendingWrap = true on EVERY raw advance -- after every printed character -- documented as harmless because "only the boundary column reads it". #93 quietly broke that contract by adding two readers that consult the flag anywhere: SettleForEditing (ICH/DCH/ECH) and the CUB phantom-column adjustment. From then on, any CUB or DCH issued immediately after printing acted one column LEFT of the cursor. On screen that was the asciiquarium bug: duplicated fragments trailing left-moving sprites (CUB off by one leaves the old cells unerased) and characters eaten from right-moving ones (DCH deletes one cell early). It looked rate- and machine-dependent, but it never was -- it needed a print immediately followed by a relative-back or edit in the stream, which curses emits as a cursor-motion optimisation, so it tracked what the animation happened to draw. A 16-byte repro settles it: ESC[5;1H AB ESC[1P eats the B. The fix makes the flag state a fact instead of trusting its readers: PendingWrap = x > _scrollRight, true exactly at the one-past-the-margin position only a fill can produce. DECRC's explicit restore still runs through SetPendingWrap afterwards, so the saved ambiguity survives. Tests: the three repros (DCH after print, CUB after print, a stale flag carried across a cursor address), plus two guards proving the fix does not regress what the flag is FOR -- autowrap at the last column, and SettleForEditing's fill-then-insert case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two findings, both worth acting on, one for a different reason than the one given. The API break is real as a type change and cannot break anyone: the property arrived in tomlm#93 on 2026-08-29, and the latest release is v1.2 from 2026-08-26, so no published package has ever carried it. That makes the answer easier rather than harder -- it is DECSC scratch a consumer cannot do anything with, so it is internal now, and the shape of an internal detail stops being an API question every time DECSC learns something more. Nothing outside the assembly referenced it; the solution builds, demos included. The untested case was the sharper find. The three cases in the test move DECNRCM AFTER the DECRC, so replaying a saved table would satisfy all three -- the very contract the doc comment claims, that a restore resolves against the mode as it is at restore time, had no test at all. Both directions now, because they fail oppositely: French designated with NRC off, DECSC, NRC on, DECRC -> a-grave French designated with NRC on, DECSC, NRC off, DECRC -> @ On the pre-fix code those give @ and a-grave respectively -- each the table that was saved rather than what the designation means at the moment it is put back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Save the designation at DECSC, not the table it resolved to Recovered from #146, which was closed unmerged when the branch it was stacked on went away with #141. The 96-set half of that PR landed independently in 1f08d48; this half did not, and still reproduces on 6d32504. DECSC saved the TABLE each G-set had resolved to. The screen after a DECRC was therefore right and the state behind it was not: the identifier stayed as whatever had been designated after the save, so the next DECNRCM re-resolved the restored slot into that instead. ESC ( 0, DECSC, ESC ( R, DECRC line drawing back, correct ...then DECNRCM letters -- re-resolved as French ESC - A, DECSC, ESC ) A, DECRC Latin-1 back, correct ...then DECNRCM a pound sign -- the UK set The first is older than the 96-set work; it arrived with the identifiers themselves. The second is the identifier collision again, one save and restore later: A is ISO Latin-1 in the 96-set space and the United Kingdom set in the 94-set one, and the saved table records neither. DECSC now saves the designation with the space it came from, and DECRC resolves it against the mode state as it is then -- which is also the right answer when DECNRCM moved between the two. One Resolve answers "what does this designation mean now" for all three callers that ask: the designation path, DECRC, and the DECNRCM refresh. They resolved separately before, which is how two of them disagreed. Every G-set is seeded to B rather than left absent, so a designation is a value rather than a value-or-absent and the walks over the four -- the save, the restore, the DECNRCM refresh -- are all total. Also from Copilot's review on #146: the separate-space test designated G0 for its UK half and G1 for its Latin-1 half, so the two differed by two things rather than one. Both designate G1 and invoke it with SO now, which also takes the last two literal control bytes out of the file. The new test asserts the restore WITHOUT a mode change as well. That is the part that was never broken, and a test that stopped there would pass on the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address Copilot's review: narrow the property, cover the mode change Two findings, both worth acting on, one for a different reason than the one given. The API break is real as a type change and cannot break anyone: the property arrived in #93 on 2026-08-29, and the latest release is v1.2 from 2026-08-26, so no published package has ever carried it. That makes the answer easier rather than harder -- it is DECSC scratch a consumer cannot do anything with, so it is internal now, and the shape of an internal detail stops being an API question every time DECSC learns something more. Nothing outside the assembly referenced it; the solution builds, demos included. The untested case was the sharper find. The three cases in the test move DECNRCM AFTER the DECRC, so replaying a saved table would satisfy all three -- the very contract the doc comment claims, that a restore resolves against the mode as it is at restore time, had no test at all. Both directions now, because they fail oppositely: French designated with NRC off, DECSC, NRC on, DECRC -> a-grave French designated with NRC on, DECSC, NRC off, DECRC -> @ On the pre-fix code those give @ and a-grave respectively -- each the table that was saved rather than what the designation means at the moment it is put back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Fourth of eight PRs from the whole-codebase audit. Stacked on #92 (which is stacked on #89) — merge those first.
Ten divergences from xterm's cursor and margin behavior. Each was reproduced against the built library before being fixed.
CUU/CUD/CNL/CPLignored the scrolling regionCUF/CUBalready had the in/out rule (start inside → stop at the margin, start outside → screen edge); the row motions now share it, so a full-screen editor's status line stops scrolling with the textCUBcounted from the phantom columnDECAWMoff discarded characters past the marginCSI 0;0rclamped the region to one row0means the default exactly as a missing parameter does, and the parser seeds parameters with0, so the bareCSI ;rform arrived the same wayIL/DLleft the cursor column aloneDECSC/DECRCsaved position and SGR onlySavedCursor.Charsethad sat unassigned, which was the tell — and saving which G-set is active is not enough, becauseESC ( 0changes what G0 meansICH/DCH/ECHno-opped from a full lineHTScould not create one andTBCcleared nothing — its own comment admitted it. There is a real stop set now, sotabs 4worksHThardcoded 8 columnsCHT/CBThonouredOptions.TabStopWidth, so the two tab motions disagreed on one screenHPA,HPR,VPRunimplementedhpaand programs emit itBehavior changes worth a deliberate look
DECAWMoff now overwrites rather than discards. Correct per every reference, but it changes what a program with wrapping disabled puts on screen.HTnow honoursTabStopWidth. A host that set the option while relying on tabs staying at 8 will see different columns.Public API
None. The tab-stop methods on
Terminalareinternal.Validation
12 new tests, each naming the program behavior at stake. Suite green: 1911. Perf 3×3 vs main, measured together with #92 beneath it: every corpus inside noise (unicode +2.6% ±3%, alt-redraw +0.1% ±3%, flood −3.2% ±6%).
🤖 Generated with Claude Code