From 0df38cad56403e362e8a7f9cba917fdb896c78e7 Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 31 Aug 2026 20:53:52 -0400 Subject: [PATCH 1/2] Spend a single shift on the next graphic character, and nothing else From the review on #141. Four ways in and three answers: SI cancelled a pending single shift, SO and the locking shifts left it standing, and RIS could not reach it at all. ESC * 0, SS2, RIS, then 'q' a line-drawing dash, on a terminal that had just been reset to ASCII RIS is the sharpest of the three because the pending state held the TABLE G2 had resolved to. ResetCharsets put the tables back and the shift went on pointing at the one it had captured, so the reset was undone by a character arriving after it. The pending state is now the G-SET rather than a table, resolved when the character arrives. That fixes a third case the same way: a designation between the shift and the character it shifts belongs to that character, because SS2 invokes G2 and what G2 holds is a question with an answer at print time. ESC N, ESC * 0, then 'q' printed a letter; the shift had captured G2 before it was designated SI no longer cancels. The VT510 manual scopes a single shift to "the next graphic character", and a locking shift is not one -- so the three that left it standing were right and the one that did not was wrong. Nothing in the suite depended on the old behaviour. The test carries the three that already worked -- SO, LS2, and a shift spending itself on one character rather than two -- alongside the three that did not, because a test of only the broken cases cannot tell "missing" from "misrouted". Verified against the defect: with the production files reverted the test fails on the RIS case, and the earlier probe fails on the SI and designate-after-shift cases too. Co-Authored-By: Claude Opus 5 --- src/XTerm.NET.Tests/VtTestBehaviourTests.cs | 45 +++++++++++++++++++++ src/XTerm.NET/InputHandler.Print.cs | 40 +++++++++++------- src/XTerm.NET/InputHandler.cs | 20 +++++---- 3 files changed, 82 insertions(+), 23 deletions(-) diff --git a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs index 3bbea05..013722c 100644 --- a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs +++ b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs @@ -449,4 +449,49 @@ public void DECRC_restores_what_was_designated_not_what_it_resolved_to() modeOffAfterSave.Write($"{ShiftOut}@{ShiftIn}"); Assert.Equal("@", modeOffAfterSave.GetLine(0)); // ASCII, not the French it saved } + + /// + /// A single shift is spent by the next graphic character, and by nothing else short of a reset. + /// + /// + /// Four ways in and three answers: SI cancelled a pending shift, SO and the locking + /// shifts did not, and RIS could not reach it at all -- the pending state held the TABLE G2 + /// had resolved to, so a reset put the tables back and the shift went on pointing at the old + /// one. ESC * 0, SS2, RIS, 'q' printed a line-drawing dash on a terminal that had just been + /// reset to ASCII. + /// + /// The VT510 manual scopes a single shift to "the next graphic character". A locking + /// shift is not one, and neither is a designation -- so a designation between the shift and + /// the character it shifts belongs to that character, which is the last case here. + /// + [Fact] + public void A_single_shift_is_spent_by_the_next_graphic_character_and_nothing_else() + { + const string so = "\u000e"; + const string si = "\u000f"; + + // RIS reaches it. Line drawing in G2, shift pending, reset -- the 'q' is a letter again. + var reset = Sized(20, 3); + reset.Write($"{Esc}*0{Esc}N{Esc}c" + "q"); + Assert.Equal("q", reset.GetLine(0)); + + // The three locking shifts leave it standing, and now agree with each other. + foreach (var shift in new[] { si, so, $"{Esc}n" }) + { + var terminal = Sized(20, 3); + terminal.Write($"{Esc}*0{Esc}N{shift}q"); + Assert.Equal("─", terminal.GetLine(0)); + } + + // A designation after the shift counts: SS2 invokes G2, and what G2 holds is a question + // with an answer at print time rather than at shift time. + var late = Sized(20, 3); + late.Write($"{Esc}N{Esc}*0" + "q"); + Assert.Equal("─", late.GetLine(0)); + + // And it really is spent, on one character and not two. + var once = Sized(20, 3); + once.Write($"{Esc}*0{Esc}N" + "qq"); + Assert.Equal("─q", once.GetLine(0)); + } } diff --git a/src/XTerm.NET/InputHandler.Print.cs b/src/XTerm.NET/InputHandler.Print.cs index 4abe1b6..6897cec 100644 --- a/src/XTerm.NET/InputHandler.Print.cs +++ b/src/XTerm.NET/InputHandler.Print.cs @@ -392,12 +392,12 @@ public void Print(string data) var translatedData = data; if (data.Length == 1) { - // A single shift outranks GL for this character and is spent doing it. - if (_singleShiftPending) + // A single shift outranks GL for this character and is spent doing it. Resolved + // here rather than when the shift arrived, so a designation in between counts. + if (_singleShift is { } shifted) { - translatedData = Charsets.TranslateChar(data[0], _singleShiftCharset); - _singleShiftCharset = null; - _singleShiftPending = false; + translatedData = Charsets.TranslateChar(data[0], _charsets.GetValueOrDefault(shifted)); + _singleShift = null; } else { @@ -812,11 +812,14 @@ public void ShiftOut() /// /// Shift In - Select G0 character set (SI, 0x0F). /// + /// + /// A pending single shift survives this, as it survives SO and the locking shifts. SI used + /// to cancel one and the other three did not, which is three answers to one question: a + /// single shift is spent by the next GRAPHIC character, and a locking shift is not one. + /// public void ShiftIn() { _currentCharset = CharsetMode.G0; - _singleShiftCharset = null; - _singleShiftPending = false; RefreshActiveCharset(); } @@ -838,21 +841,28 @@ public void LockingShift(CharsetMode mode) /// SS2 (ESC N) and SS3 (ESC O) - invoke G2 or G3 for the NEXT character only. /// /// - /// The single shift is held pending rather than swapped in, so it expires by being consumed - /// instead of by something remembering to put the old set back. A shift with no character - /// after it simply never fires. + /// The single shift is held pending rather than swapped in, so it expires by being + /// consumed instead of by something remembering to put the old set back. A shift with no + /// character after it simply never fires. + /// + /// Consumption is the ONLY thing that spends it, short of a reset. The VT510 manual + /// says a single shift maps its G-set "for the next graphic character", and neither a + /// locking shift nor a designation is one. /// - public void InvokeSingleShift(CharsetMode mode) - { - _singleShiftCharset = _charsets.GetValueOrDefault(mode); - _singleShiftPending = true; - } + public void InvokeSingleShift(CharsetMode mode) => _singleShift = mode; /// /// Resets charset state to defaults. /// public void ResetCharsets() { + // A pending single shift is charset state, and RIS is exactly how someone recovers from + // a program that died between the shift and the character it was going to shift. + // + // _charsetIds is no longer cleared here: #149 made the loop below SEED every slot with the + // US ASCII designation rather than leave it absent, so clearing first would only empty a + // dictionary that is about to have all four entries written. + _singleShift = null; _ninetySixSets.Clear(); foreach (var mode in GSets) { diff --git a/src/XTerm.NET/InputHandler.cs b/src/XTerm.NET/InputHandler.cs index 103ccc7..e70fad4 100644 --- a/src/XTerm.NET/InputHandler.cs +++ b/src/XTerm.NET/InputHandler.cs @@ -63,17 +63,21 @@ public partial class InputHandler private readonly HashSet _ninetySixSets = new(); /// - /// The set a SINGLE shift has invoked for the next printed character, or null. + /// The G-set a SINGLE shift has invoked for the next printed character, or null. /// /// - /// Separate from because it outranks it for exactly one + /// Separate from because it outranks it for exactly one /// character and then stops: SS2 and SS3 shift the character that follows and nothing /// after it. Holding it as pending state rather than swapping the active set is what makes - /// "and then stops" automatic instead of something the print path has to remember to undo. + /// "and then stops" automatic instead of something the print path has to remember to undo. + /// + /// The G-SET, not the table it resolved to when the shift arrived. A designation + /// between the shift and the character it shifts belongs to that character -- SS2 invokes + /// G2, and what G2 holds is a question with an answer at print time. Holding the table also + /// meant a reset could not reach it: RIS put the tables back and the pending shift went on + /// pointing at the one it had captured. /// - private Dictionary? _singleShiftCharset; - - private bool _singleShiftPending; + private CharsetMode? _singleShift; private CharsetMode _currentCharset; // Variation selector and combining character constants @@ -188,7 +192,7 @@ internal void PrintAsciiRun(ReadOnlySpan data) // path can only stop. Rare enough to hand to Print rather than teach twice -- the default // is on, so nothing in normal output takes this branch. if (!UseRunPrinting || _terminal.InsertMode || _activeCharset is not null - || _singleShiftPending + || _singleShift is not null || _buffer.HasMultiRowSizedRuns || !_terminal.Options.Wraparound) { foreach (var b in data) @@ -296,7 +300,7 @@ internal void PrintAsciiRun(string data, int start, int count) // path can only stop. Rare enough to hand to Print rather than teach twice -- the default // is on, so nothing in normal output takes this branch. if (!UseRunPrinting || _terminal.InsertMode || _activeCharset is not null - || _singleShiftPending + || _singleShift is not null || _buffer.HasMultiRowSizedRuns || !_terminal.Options.Wraparound) { for (var k = 0; k < count; k++) From cb055d0b49555cc35263dbc2936fbc000275fc0f Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 31 Aug 2026 21:08:46 -0400 Subject: [PATCH 2/2] Spend the shift on a supplementary character too From Copilot's review. The invariant this branch sets out -- a single shift is spent by the next graphic character -- held only for characters that fit in one UTF-16 code unit. A supplementary character reaches Print as two, and the clear that spends the shift sat inside the single-code-unit branch. That does not skip the shift, it MOVES it. ESC * 0, SS2, an emoji, then 'q' drew the q as a box-drawing glyph on a terminal whose G2 the program had finished with -- the same shape as the RIS case this branch already fixes, reached by a different route. The emoji itself stays untranslated: a 94-character set has no entry outside the BMP, and TranslateChar takes a single code unit. Spending the shift and translating through it are separate things, and only the first applies here. The clear goes AFTER the length test rather than before it. The review suggested consuming the shift ahead of the graphic-character branches, which would put a null check in front of the test every ordinary character takes; CLAUDE.md opens with that. This branch runs for a surrogate pair only, and only to clear a field. Co-Authored-By: Claude Fable 5 --- src/XTerm.NET.Tests/VtTestBehaviourTests.cs | 12 ++++++++++++ src/XTerm.NET/InputHandler.Print.cs | 14 ++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs index 013722c..f28afb5 100644 --- a/src/XTerm.NET.Tests/VtTestBehaviourTests.cs +++ b/src/XTerm.NET.Tests/VtTestBehaviourTests.cs @@ -493,5 +493,17 @@ public void A_single_shift_is_spent_by_the_next_graphic_character_and_nothing_el var once = Sized(20, 3); once.Write($"{Esc}*0{Esc}N" + "qq"); Assert.Equal("─q", once.GetLine(0)); + + // A SUPPLEMENTARY character spends it too. It reaches Print as two UTF-16 code units, + // and the clear that spends the shift used to sit inside the single-code-unit branch -- + // so the shift survived the emoji and landed on whatever came next. That does not skip + // the shift, it MOVES it: the q below drew as a box-drawing glyph on a terminal whose G2 + // the program had finished with, the same class of bug as the RIS case above. + // + // The emoji itself is untranslated, because a 94-character set has no entry outside the + // BMP -- spending the shift and translating through it are different things. + var supplementary = Sized(20, 3); + supplementary.Write($"{Esc}*0{Esc}N" + "\U0001F600q"); + Assert.Equal("\U0001F600q", supplementary.GetLine(0)); } } diff --git a/src/XTerm.NET/InputHandler.Print.cs b/src/XTerm.NET/InputHandler.Print.cs index 6897cec..00c6fd6 100644 --- a/src/XTerm.NET/InputHandler.Print.cs +++ b/src/XTerm.NET/InputHandler.Print.cs @@ -404,6 +404,20 @@ public void Print(string data) translatedData = Charsets.TranslateChar(data[0], _activeCharset); } } + else if (_singleShift is not null) + { + // A supplementary character is a graphic character too, and spends the shift like any + // other -- it just has nothing to spend it ON, because a 94- or 96-character set has no + // entry outside the BMP and TranslateChar takes a single code unit. Leaving the shift + // standing through it does not skip the shift, it MOVES it: the character after the + // emoji gets translated instead, so `SS2 q` drew q as a box-drawing glyph on a + // terminal whose G2 the program had finished with. + // + // Second, after the length test rather than before it, because that is the branch every + // ordinary character takes -- see CLAUDE.md's first section. This one runs only for a + // surrogate pair, and only to clear a field. + _singleShift = null; + } var width = GetStringCellWidth(translatedData);