From bc48965ed5e6a8638b7f0e7a6990220f909bbae3 Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 31 Aug 2026 20:48:13 -0400 Subject: [PATCH 1/2] 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 --- src/XTerm.NET.Tests/VtTestBehaviourTests.cs | 48 +++++++++- src/XTerm.NET/Buffer/TerminalBuffer.cs | 13 ++- src/XTerm.NET/InputHandler.Csi.cs | 6 +- src/XTerm.NET/InputHandler.Print.cs | 97 +++++++++++++++------ src/XTerm.NET/InputHandler.cs | 16 +++- 5 files changed, 143 insertions(+), 37 deletions(-) diff --git a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs index eda0ca2..77bff0d 100644 --- a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs +++ b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs @@ -20,6 +20,8 @@ namespace XTerm.Tests; public class VtTestBehaviourTests { private const string Esc = "\u001b"; + private const string ShiftOut = "\u000e"; // SO -- invoke G1 into GL + private const string ShiftIn = "\u000f"; // SI -- back to G0 private static Terminal Sized(int cols = 40, int rows = 6) => new(new TerminalOptions { Cols = cols, Rows = rows }); @@ -289,12 +291,15 @@ public void Special_graphics_maps_the_control_pictures_too() [Fact] public void The_96_character_set_designators_are_a_separate_space() { + // Both halves designate G1 and invoke it with SO, so they differ by ONE thing: the + // space the designator came from. Designating G0 for one and G1 for the other left + // the comparison carrying a second difference it was not trying to test. var uk = Sized(30, 3); - uk.Write($"{Esc}(A#@["); + uk.Write($"{Esc})A{ShiftOut}#@[{ShiftIn}"); Assert.Equal("£@[", uk.GetLine(0)); var latin1 = Sized(30, 3); - latin1.Write($"{Esc}-A#@["); + latin1.Write($"{Esc}-A{ShiftOut}#@[{ShiftIn}"); Assert.Equal("#@[", latin1.GetLine(0)); } @@ -389,4 +394,43 @@ public void A_national_set_answers_to_both_of_its_designators() Assert.Equal("£à°", terminal.GetLine(0)); } } + + /// + /// DECRC puts back the DESIGNATION, so a later DECNRCM re-resolves what was restored. + /// + /// + /// DECSC saved the table each G-set had resolved to rather than what it was designated + /// as, so the identifier behind a restored slot stayed as whatever had been designated AFTER + /// the save. The screen was right and the state behind it was not, which is why this needs a + /// mode change to show at all: the next DECNRCM re-resolves the restored slot into the wrong + /// set, arbitrarily far from the DECRC that caused it. + /// + /// Both spaces, because they fail differently. The 94-set pair loses line drawing to a + /// national set -- ESC ( 0, DECSC, ESC ( R, DECRC draws borders until the mode moves and then + /// draws letters. The 96-set pair is the identifier collision again: Latin-1 restored, then + /// re-resolved as the United Kingdom set. + /// + [Fact] + public void DECRC_restores_what_was_designated_not_what_it_resolved_to() + { + var graphics = Sized(30, 3); + graphics.Write($"{Esc})0{Esc}7{Esc})R{Esc}8"); // graphics, DECSC, French, DECRC + graphics.Write($"{Esc}[?42h"); // DECNRCM, which re-resolves + graphics.Write($"{ShiftOut}qqq{ShiftIn}"); + Assert.Equal("───", graphics.GetLine(0)); + + var latin1 = Sized(30, 3); + latin1.Write($"{Esc}-A{Esc}7{Esc})A{Esc}8"); // Latin-1, DECSC, UK, DECRC + latin1.Write($"{Esc}[?42h"); + latin1.Write($"{ShiftOut}#@[{ShiftIn}"); + Assert.Equal("#@[", latin1.GetLine(0)); + + // 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)); + } } diff --git a/src/XTerm.NET/Buffer/TerminalBuffer.cs b/src/XTerm.NET/Buffer/TerminalBuffer.cs index 5532852..9fc1aa0 100644 --- a/src/XTerm.NET/Buffer/TerminalBuffer.cs +++ b/src/XTerm.NET/Buffer/TerminalBuffer.cs @@ -136,10 +136,17 @@ public class SavedCursor /// and reused after: DECSC is not rare -- a full-screen program saves and restores the /// cursor on every redraw -- and copying a dictionary allocated once per save. There are /// exactly four G-slots and the enum numbers them from zero, so the save is four - /// reference writes. Null means DECSC has not run on this screen, which is what tells - /// DECRC to leave the designations alone. + /// writes. Null means DECSC has not run on this screen, which is what tells DECRC to + /// leave the designations alone. + /// + /// The DESIGNATION each slot held, not the table it had resolved to. The two differ once + /// a mode moves under them: DECNRCM re-resolves every designation, so a slot restored as + /// a table carries whatever identifier was designated AFTER the save, and the next + /// DECNRCM re-resolves the restored slot into that instead. The identifier carries the + /// space it came from for the same reason -- 'A' is the United Kingdom set in the 94-set + /// space and ISO Latin-1 in the 96-set one. /// - public Dictionary?[]? Designations { get; set; } + public (string Id, bool NinetySix)[]? Designations { get; set; } public SavedCursor() { diff --git a/src/XTerm.NET/InputHandler.Csi.cs b/src/XTerm.NET/InputHandler.Csi.cs index 0d8592a..e1e2203 100644 --- a/src/XTerm.NET/InputHandler.Csi.cs +++ b/src/XTerm.NET/InputHandler.Csi.cs @@ -1609,9 +1609,9 @@ private void SaveCursor() // which set is selected, so saving _currentCharset alone restored a pointer to a table // the program had since replaced: a TUI that saved the cursor mid-border finished the box // in letters. - var designations = _buffer.SavedCursorState.Designations ??= new Dictionary?[4]; + var designations = _buffer.SavedCursorState.Designations ??= new (string, bool)[4]; for (var slot = 0; slot < designations.Length; slot++) - designations[slot] = _charsets.GetValueOrDefault((CharsetMode)slot); + designations[slot] = DesignationOf((CharsetMode)slot); _buffer.SavedCursorState.OriginMode = _terminal.OriginMode; _buffer.SavedCursorState.PendingWrap = _buffer.PendingWrap; } @@ -1632,7 +1632,7 @@ private void RestoreCursor() if (designations is not null) { for (var slot = 0; slot < designations.Length; slot++) - _charsets[(CharsetMode)slot] = designations[slot]; + RestoreDesignation((CharsetMode)slot, designations[slot]); } _currentCharset = _buffer.SavedCursorState.Charset; diff --git a/src/XTerm.NET/InputHandler.Print.cs b/src/XTerm.NET/InputHandler.Print.cs index 0a5ce96..4abe1b6 100644 --- a/src/XTerm.NET/InputHandler.Print.cs +++ b/src/XTerm.NET/InputHandler.Print.cs @@ -709,17 +709,8 @@ private bool TryPairRegionalIndicator(string data, int cellX) return true; } - private void SetCharset(CharsetMode mode, string charsetId) - { - // The ID is kept, not just the table it resolves to. A national set means one thing - // with DECNRCM set and ASCII without it, so the designation has to outlive the - // resolution -- a program designating French and then enabling NRC mode expects - // French, and it never designates again. - _charsetIds[mode] = charsetId; - _ninetySixSets.Remove(mode); - _charsets[mode] = Charsets.GetCharset(charsetId, _terminal.NationalReplacementCharsets); - RefreshActiveCharset(); - } + private void SetCharset(CharsetMode mode, string charsetId) => + Designate(mode, charsetId, ninetySix: false); /// /// Designates a 96-character set: ESC - Ps (G1), ESC . Ps (G2), @@ -731,14 +722,40 @@ private void SetCharset(CharsetMode mode, string charsetId) /// 96-set space and the United Kingdom set in the 94-set one. Anything else is left as /// ASCII rather than guessed at. /// - private void SetNinetySixCharset(CharsetMode mode, string charsetId) + private void SetNinetySixCharset(CharsetMode mode, string charsetId) => + Designate(mode, charsetId, ninetySix: true); + + /// Records a designation and resolves it, for both spaces. + /// + /// The ID is kept, not just the table it resolves to. A national set means one thing with + /// DECNRCM set and ASCII without it, so the designation has to outlive the resolution -- a + /// program designating French and then enabling NRC mode expects French, and it never + /// designates again. + /// + private void Designate(CharsetMode mode, string charsetId, bool ninetySix) { _charsetIds[mode] = charsetId; - _ninetySixSets.Add(mode); - _charsets[mode] = Charsets.ASCII; + if (ninetySix) + _ninetySixSets.Add(mode); + else + _ninetySixSets.Remove(mode); + + _charsets[mode] = Resolve(charsetId, ninetySix); RefreshActiveCharset(); } + /// What a designation means RIGHT NOW, given the mode state. + /// + /// One answer for the three callers that need it -- designation, DECRC and DECNRCM -- because + /// the question is the same one and they got different answers while each resolved separately. + /// The space is half the question: 'A' is ISO Latin-1 after ESC - and the United Kingdom set + /// after ESC (, so an identifier without the space it came from cannot be resolved at all. + /// + private Dictionary? Resolve(string charsetId, bool ninetySix) => + ninetySix + ? Charsets.ASCII + : Charsets.GetCharset(charsetId, _terminal.NationalReplacementCharsets); + /// Re-resolves every designation, for when DECNRCM changes under them. /// /// Through the space each was designated in. 'A' is ISO Latin-1 after ESC - and the @@ -749,16 +766,40 @@ private void SetNinetySixCharset(CharsetMode mode, string charsetId) /// internal void RefreshDesignatedCharsets() { - foreach (var mode in _charsetIds.Keys.ToList()) - { - _charsets[mode] = _ninetySixSets.Contains(mode) - ? Charsets.ASCII - : Charsets.GetCharset(_charsetIds[mode], _terminal.NationalReplacementCharsets); - } + foreach (var mode in GSets) + _charsets[mode] = Resolve(_charsetIds[mode], _ninetySixSets.Contains(mode)); RefreshActiveCharset(); } + /// The designation a G-set is holding, for DECSC to save. + internal (string Id, bool NinetySix) DesignationOf(CharsetMode mode) => + (_charsetIds[mode], _ninetySixSets.Contains(mode)); + + /// + /// Puts a saved designation back, for DECRC, resolving it against the mode state as it is NOW. + /// + /// + /// The DESIGNATION is what DECSC saves, not the table it had resolved to. Saving the table + /// restores the right glyphs and leaves the identifier behind it stale, so the next DECNRCM + /// re-resolves the restored slot from whatever was designated AFTER the save -- and a program + /// doing ESC ( 0, DECSC, ESC ( R, DECRC gets its line drawing back and then loses it again the + /// first time the mode moves, arbitrarily far from the DECRC that caused it. + /// + /// Resolving at restore time rather than replaying the saved table is also the right answer + /// when DECNRCM moved BETWEEN the save and the restore. + /// + internal void RestoreDesignation(CharsetMode mode, (string Id, bool NinetySix) designation) + { + _charsetIds[mode] = designation.Id; + if (designation.NinetySix) + _ninetySixSets.Add(mode); + else + _ninetySixSets.Remove(mode); + + _charsets[mode] = Resolve(designation.Id, designation.NinetySix); + } + /// /// Shift Out - Select G1 character set (SO, 0x0E). /// @@ -812,16 +853,22 @@ public void InvokeSingleShift(CharsetMode mode) /// public void ResetCharsets() { - _charsetIds.Clear(); _ninetySixSets.Clear(); - _charsets[CharsetMode.G0] = Charsets.ASCII; - _charsets[CharsetMode.G1] = Charsets.ASCII; - _charsets[CharsetMode.G2] = Charsets.ASCII; - _charsets[CharsetMode.G3] = Charsets.ASCII; + foreach (var mode in GSets) + { + // Seeded rather than cleared: US ASCII IS the designation every slot starts with, and + // saying so is what keeps every walk over the four total. + _charsetIds[mode] = UsAsciiId; + _charsets[mode] = Charsets.ASCII; + } + _currentCharset = CharsetMode.G0; RefreshActiveCharset(); } + /// The identifier US ASCII is designated by, which is where every G-set starts. + private const string UsAsciiId = "B"; + /// /// Prints the payload of an OSC 66 whose metadata could not be parsed, as ordinary text. /// diff --git a/src/XTerm.NET/InputHandler.cs b/src/XTerm.NET/InputHandler.cs index 086cf73..12bda2c 100644 --- a/src/XTerm.NET/InputHandler.cs +++ b/src/XTerm.NET/InputHandler.cs @@ -41,12 +41,20 @@ public partial class InputHandler /// What each G-set was DESIGNATED as, by its escape identifier. /// - /// Kept alongside the resolved tables because a designation outlives its resolution: a + /// Kept alongside the resolved tables because a designation outlives its resolution: a /// national set resolves to ASCII while DECNRCM is reset and to itself once it is set, and - /// the program that designated it does not designate again when the mode changes. + /// the program that designated it does not designate again when the mode changes. + /// + /// All four slots are always present, seeded to US ASCII, so a designation is a value + /// rather than a value-or-absent. DECSC, DECRC and DECNRCM each walk every slot, and "never + /// designated" and "designated B" mean the same thing to all three. /// private readonly Dictionary _charsetIds = new(); + /// The four G-sets, for the walks that touch all of them. + private static readonly CharsetMode[] GSets = + [CharsetMode.G0, CharsetMode.G1, CharsetMode.G2, CharsetMode.G3]; + /// Which G-sets were designated as 96-character sets. /// /// The identifier alone does not say: 'A' is the UK set in one space and ISO Latin-1 in the @@ -117,8 +125,8 @@ public InputHandler(Terminal terminal) { CharsetMode.G3, Charsets.ASCII } }; - _currentCharset = CharsetMode.G0; // G0 is active by default - RefreshActiveCharset(); + // And the designations behind them, which ResetCharsets seeds alongside the tables. + ResetCharsets(); } /// From e8206dc0c1f8a5129c14a7c7e87b2b0271eb6457 Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 31 Aug 2026 20:58:23 -0400 Subject: [PATCH 2/2] 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 --- src/XTerm.NET.Tests/VtTestBehaviourTests.cs | 16 ++++++++++++++++ src/XTerm.NET/Buffer/TerminalBuffer.cs | 8 +++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs index 77bff0d..3bbea05 100644 --- a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs +++ b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs @@ -432,5 +432,21 @@ public void DECRC_restores_what_was_designated_not_what_it_resolved_to() immediate.Write($"{Esc})0{Esc}7{Esc})R{Esc}8"); immediate.Write($"{ShiftOut}qqq{ShiftIn}"); Assert.Equal("───", immediate.GetLine(0)); + + // DECNRCM moving BETWEEN the save and the restore, both directions. This is the half the + // doc comment claims and the three cases above do not reach: they move the mode after the + // DECRC, so replaying a saved TABLE would satisfy them. Here the table saved and the table + // wanted are different, and only re-resolving the designation produces the second one. + var modeOnAfterSave = Sized(30, 3); + modeOnAfterSave.Write($"{Esc})R{Esc}7"); // French designated with NRC OFF + modeOnAfterSave.Write($"{Esc}[?42h{Esc}8"); // NRC on, then restore + modeOnAfterSave.Write($"{ShiftOut}@{ShiftIn}"); + Assert.Equal("à", modeOnAfterSave.GetLine(0)); // French, not the ASCII it saved + + var modeOffAfterSave = Sized(30, 3); + modeOffAfterSave.Write($"{Esc}[?42h{Esc})R{Esc}7"); // French designated with NRC ON + modeOffAfterSave.Write($"{Esc}[?42l{Esc}8"); // NRC off, then restore + modeOffAfterSave.Write($"{ShiftOut}@{ShiftIn}"); + Assert.Equal("@", modeOffAfterSave.GetLine(0)); // ASCII, not the French it saved } } diff --git a/src/XTerm.NET/Buffer/TerminalBuffer.cs b/src/XTerm.NET/Buffer/TerminalBuffer.cs index 9fc1aa0..e59abdf 100644 --- a/src/XTerm.NET/Buffer/TerminalBuffer.cs +++ b/src/XTerm.NET/Buffer/TerminalBuffer.cs @@ -145,8 +145,14 @@ public class SavedCursor /// DECNRCM re-resolves the restored slot into that instead. The identifier carries the /// space it came from for the same reason -- 'A' is the United Kingdom set in the 94-set /// space and ISO Latin-1 in the 96-set one. + /// + /// Internal, unlike the rest of this class: it is DECSC scratch that a consumer cannot do + /// anything with, and it was public only by having been written that way. Narrowing it now + /// costs nothing -- the property arrived in #93, after v1.2, so no released package has + /// ever carried it -- and it stops the shape of an internal detail being an API question + /// every time DECSC learns something new. /// - public (string Id, bool NinetySix)[]? Designations { get; set; } + internal (string Id, bool NinetySix)[]? Designations { get; set; } public SavedCursor() {