Preprocess and convert C1 controls to their 7 bit equivalent #7340
Conversation
…75489/terminal into feature/init-dcs-support
ghost
added
Area-VT
| { | ||
| return wch == L'\x90'; | ||
| return wch - L'\x40'; |
There was a problem hiding this comment.
This seems correct but I can't find valid reference for this.
There was a problem hiding this comment.
Yeah, I believe this is correct. Quoting the DEC ST 070 manual:
expansion escape sequence (Fe) - 2-character escape sequence in which the final character is in columns 4 and 5, and is used to extend the 7-bit code table by being the row equivalent to the corresponding C1 set of controls in columns 8 and 9 of an 8-bit code.
There was a problem hiding this comment.
(Should we prefer -0x40 or &~0x40 here?)
There was a problem hiding this comment.
I choose L'\x40' cuz I want it have the same type as wch. Not that it mean anything, but hey, no implicit conversion.
| // Arguments: | ||
| // - wch - Character that triggered the event | ||
| // Return Value: | ||
| // - <none> | ||
| void StateMachine::_EventDcsIgnore(const wchar_t wch) noexcept | ||
| void StateMachine::_EventDcsIgnore() noexcept |
There was a problem hiding this comment.
It’s a little weird to have just one case in this, isn’t it. But this is actually the correct way to do this.
There was a problem hiding this comment.
Well this looks good to me. I'd love to make sure we get @j4james to sign off on this too, if he's willing.
There was a problem hiding this comment.
This looks good to me. I've run a few basic tests with it, and the C1 controls are now working as expected, and the APC/PM/SOS sequences are being appropriately ignored. I'm a little concerned about the input side of things, because I'm not how to test that, but I think the C1 mapping should still be OK for that.
|
Thanks @j4james . I literally couldn't write any of these PRs without you . You know originally I just want those dazzling Sixel images. But who knows VT sequences are actually full of fun. Be prepared for my following unknown numbers of PRs that's VT related 😄 . |
There was a problem hiding this comment.
Okay - I finally read over this, and I think I understand how this improves our state machine. I've got to trust Mike and James here, as well. Thanks @skyline75489!
|
@msftbot merge this in 1 minute |
ghost
added
the
AutoMerge
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
ghost
merged commit 
## Summary of the Pull Request This fixes a typo in the `HyperlinkIdConsistency` unit test which was causing that test to fail. It was mistakenly using a `/` instead of `\` for the string terminator sequences. ## References The test initially worked because of a bug in the state machine parser, but that bug was recently fixed in PR #7340. ## PR Checklist * [x] Closes #7654 * [x] CLA signed. * [x] Tests passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. ## Validation Steps Performed I've run the test again and it now passes.
|
🎉 Handy links: |
ghost
mentioned this pull request
|
This fix was just released as part of Windows in Insider Build 20236! |
This PR introduces a mechanism via which DCS data strings can be passed through directly to the dispatch method that will be handling them, so the data can be processed as it is received, rather than being buffered in the state machine. This also simplifies the way string termination is handled, so it now more closely matches the behaviour of the original DEC terminals. * Initial support for DCS sequences was introduced in PR #6328. * Handling of DCS (and other) C1 controls was added in PR #7340. * This is a prerequisite for Sixel (#448) and Soft Font (#9164) support. The way this now works, a `DCS` sequence is dispatched as soon as the final character of the `VTID` is received. Based on that ID, the `OutputStateMachineEngine` should forward the call to the corresponding dispatch method, and its the responsibility of that method to return an appropriate handler function for the sequence. From then on, the `StateMachine` will pass on all of the remaining bytes in the data string to the handler function. When a data string is terminated (with `CAN`, `SUB`, or `ESC`), the `StateMachine` will pass on one final `ESC` character to let the handler know that the sequence is finished. The handler can also end a sequence prematurely by returning false, and then all remaining data bytes will be ignored. Note that once a `DCS` sequence has been dispatched, it's not possible to abort the data string. Both `CAN` and `SUB` are considered valid forms of termination, and an `ESC` doesn't necessarily have to be followed by a `\` for the string terminator. This is because the data string is typically processed as it's received. For example, when outputting a Sixel image, you wouldn't erase the parts that had already been displayed if the data string is terminated early. With this new way of handling the string termination, I was also able to simplify some of the `StateMachine` processing, and get rid of a few states that are no longer necessary. These changes don't apply to the `OSC` sequences, though, since we're more likely to want to match the XTerm behavior for those cases (which requires a valid `ST` control for the sequence to be accepted). ## Validation Steps Performed For the unit tests, I've had to make a few changes to some of the `OutputEngineTests` to account for the updated `StateMachine` processing. I've also added a new `StateMachineTest` to confirm that the data strings are correctly passed through to the string handler under all forms of termination. To test whether the framework is actually usable, I've been working on DRCS Soft Font support branched off of this PR, and haven't encountered any problems. To test the throughput speed, I also hacked together a basic Sixel parser, and that seemed to perform reasonably well. Closes #7316
C1 control characters are now first converted to their 7 bit equivalent.
This allows us to unify the logic of C1 and C0 escape handling. This
also adds support for SOS/PM/APC string.
beforehand. This adds support for various C1 characters, including
IND(8/4), NEL(8/5), HTS(8/8), RI(8/13), SS2(8/14), SS3(8/15),
OSC(9/13), etc.
handling of OSC, DCS and SOS/PM/APC. This fixes an issue where OSC
action is successfully dispatched even when terminated with non-ST
character. Introduced by Add initial support for VT DCS sequences #6328, the DCS PassThrough is spared from
this issue. This PR puts them together and add test cases for them.
References:
https://vt100.net/docs/vt510-rm/chapter4.html
https://vt100.net/emu/dec_ansi_parser
Closes #7032
Closes #7317