Spend a single shift on the next graphic character, and nothing else - #150
Merged
JohnCampionJr merged 2 commits intoSep 1, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Supplementary Unicode graphic characters still fail to consume a pending single shift.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates single-shift handling so pending shifts resolve current G-set designations and reset correctly.
Changes:
- Stores the pending G-set rather than its resolved table.
- Preserves shifts across locking shifts but clears them on reset.
- Adds regression coverage for reset, designation, and consumption behavior.
File summaries
| File | Description |
|---|---|
InputHandler.Print.cs |
Revises shift resolution and reset behavior. |
InputHandler.cs |
Replaces shift table state with a G-set identifier. |
VtTestBehaviourTests.cs |
Adds single-shift behavioral tests. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- 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
force-pushed
the
fix/single-shift-lifetime
branch
2 times, most recently
from
September 1, 2026 01:10
5c0d158 to
9f71e80
Compare
From the review on tomlm#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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
JohnCampionJr
force-pushed
the
fix/single-shift-lifetime
branch
from
September 1, 2026 01:11
9f71e80 to
cb055d0
Compare
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.
From the review on #141, and still reproducing on
6d32504.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.
Why RIS was the sharp one
The pending state held the table G2 had resolved to.
ResetCharsetsput 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. RIS is exactly how someone recovers from a program that died mid-sequence, and a shift left dangling by one is the state most likely to be sitting there.What changed
The pending state is the G-set, not a table, resolved when the character arrives. That fixes a third case for the same reason:
SS2 invokes G2, and what G2 holds is a question with an answer at print time. A designation between the shift and the character it shifts belongs to that character.
SI no longer cancels
The VT510 manual scopes a single shift to "the next graphic character". A locking shift is not one — so of the four entry points, the three that left a pending shift standing were right and the one that cancelled it was wrong.
InvokeSingleShift's own remarks already said consumption is what spends it;ShiftInwas the line that disagreed.Nothing in the suite depended on the old behaviour.
Verification
The test carries the three cases 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".
Checked against the defect rather than assumed: with the production files reverted it fails on the RIS case (
Expected "q" / Actual "─"), and a probe onmainfails the SI and designate-after-shift cases too.2150 passed, 1 skipped, 0 failed. The skip is DA3 (#123).
Relationship to #149
Both are based on
main— neither is stacked, after what happened to #146. They do touchResetCharsetsand both append a test toVtTestBehaviourTests.cs, so whichever lands second needs a small rebase. Merge in either order.🤖 Generated with Claude Code