Save the designation at DECSC, not the table it resolved to - #149
Conversation
Recovered from tomlm#146, which was closed unmerged when the branch it was stacked on went away with tomlm#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 tomlm#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>
87a811b to
bc48965
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The public SavedCursor.Designations type change breaks NuGet consumers, and one documented mode-transition case remains untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Corrects DECSC/DECRC charset restoration by preserving designation identifiers and their 94/96-set namespace.
Changes:
- Centralizes charset designation resolution.
- Saves and restores designation metadata instead of resolved tables.
- Adds regression coverage and improves charset-space test symmetry.
File summaries
| File | Description |
|---|---|
InputHandler.Print.cs |
Centralizes designation and restoration logic. |
InputHandler.Csi.cs |
Saves/restores charset designations. |
InputHandler.cs |
Initializes all G-set designations. |
Buffer/TerminalBuffer.cs |
Stores saved designation tuples. |
VtTestBehaviourTests.cs |
Adds DECRC regressions and test cleanup. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// space and ISO Latin-1 in the 96-set one. | ||
| /// </summary> | ||
| public Dictionary<char, string>?[]? Designations { get; set; } | ||
| public (string Id, bool NinetySix)[]? Designations { get; set; } |
There was a problem hiding this comment.
Narrowed to internal in e8206dc, though not quite for the reason given.
The type change is real; the break is not. Designations 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 this property, and there is no consumer to break either at source or at binary level.
That makes the answer easier rather than harder. It is DECSC scratch that a consumer cannot do anything useful with, and it was public only by having been written that way. Internal now, so the shape of an internal detail stops being an API question every time DECSC learns to save something more. Nothing outside the assembly referenced it — only the two call sites in InputHandler.Csi.cs — and the full solution builds, demos included.
Keeping the old contract alongside a new private member, the other option you offer, would mean maintaining a field that nothing reads.
| // And the restore itself, which was never the broken half: without the mode change both | ||
| // of the above already came back right, and a test that stopped there would pass on the | ||
| // defect. | ||
| var immediate = Sized(30, 3); | ||
| immediate.Write($"{Esc})0{Esc}7{Esc})R{Esc}8"); | ||
| immediate.Write($"{ShiftOut}qqq{ShiftIn}"); | ||
| Assert.Equal("───", immediate.GetLine(0)); |
There was a problem hiding this comment.
Right, and this was the sharper of the two. Covered in e8206dc.
All three existing cases move DECNRCM after the DECRC, so replaying a saved table satisfies every one of them. The contract the doc comment actually claims — that a restore resolves the designation against the mode as it is at restore time — had no test at all.
Both directions now, because they fail oppositely:
French designated, NRC off, DECSC, NRC on, DECRC -> à
French designated, NRC on, DECSC, NRC off, DECRC -> @
Checked against the pre-fix code rather than assumed: it gives @ and à respectively — each the table that was saved, rather than what the designation means at the moment it is put back. So neither case would have passed on the defect, in either direction.
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>
Recovers the half of #146 that was lost. That PR was stacked on
vttest-alignment, so closing that branch with #141 closed #146 unmerged. Its 96-set half landed independently in1f08d48; this half did not, and still reproduces on6d32504:What is wrong
DECSC saved the table each G-set had resolved to. So the screen after a DECRC was right and the state behind it was not: the identifier stayed as whatever had been designated after the save, and the next DECNRCM re-resolved the restored slot into that.
That is why it needs a mode change to show at all, and why it is unpleasant to chase — a TUI that saves the cursor mid-border gets its border back and loses it again the next time anything touches DECNRCM, arbitrarily far from the DECRC that caused it.
The first case is older than the 96-set work; it arrived with the identifiers themselves in #141. The second is the identifier collision one save-and-restore later:
Ais ISO Latin-1 in the 96-set space and the United Kingdom set in the 94-set one, and a saved table records neither.The fix
DECSC 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 save and the restore.
One
Resolveanswers "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 came to disagree.Every G-set is seeded to
Brather than left absent, so a designation is a value rather than a value-or-absent and the three walks over the four slots are total.Adapted to the structures on
main(_charsetIds+_ninetySixSets) rather than re-importing #146's competing record refactor, so this is a small change against what actually merged.Also, from Copilot's review on #146
The_96_character_set_designators_are_a_separate_spacedesignated 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 removes the file's last two literal control bytes (0x0E/0x0Fon line 297, the ones the sweep in1f08d48missed) in favour of namedShiftOut/ShiftInconstants.That test passes either way; it is a test-quality fix, not a guard for this one.
Verification
DECRC_restores_what_was_designated_not_what_it_resolved_tocovers both spaces, because they fail differently, and the restore without a mode change — the half that was never broken, and where a test that stopped would pass on the defect.Checked against the defect rather than assumed: with the four production files reverted the new test fails
Expected "───" / Actual "qqq", and passes with them.2150 passed, 1 skipped, 0 failed. The skip is DA3 (#123).
Still open from the #141 review, not in here
A pending single shift survives RIS and SO.
ESC * 0,ESC N, RIS,qstill prints the line-drawing dash on6d32504—ShiftInclears_singleShiftPending,ShiftOut,LockingShiftandResetCharsetsdo not. Left out to keep this change to one thing; happy to send it separately.🤖 Generated with Claude Code