Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upPreprocess and convert C1 controls to their 7 bit equivalent #7340
Conversation
| { | ||
| return wch == L'\x90'; | ||
| return wch - L'\x40'; |
This comment has been minimized.
This comment has been minimized.
skyline75489
Aug 19, 2020
Author
Member
This seems correct but I can't find valid reference for this.
This comment has been minimized.
This comment has been minimized.
j4james
Aug 19, 2020
Contributor
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
skyline75489
Aug 20, 2020
Author
Member
I choose L'\x40' cuz I want it have the same type as wch. Not that it mean anything, but hey, no implicit conversion.
| // - True if it is. False if it isn't. | ||
| const bool StateMachine::_IsVariableLengthStringState() const noexcept | ||
| { | ||
| return _state == VTStates::OscString || _state == VTStates::DcsPassThrough || _state == VTStates::SosPmApcString; |
This comment has been minimized.
This comment has been minimized.
skyline75489
Aug 20, 2020
Author
Member
From the graph here https://vt100.net/emu/dec_ansi_parser, these are the only three cases. Not sure if there's more.
| // Arguments: | ||
| // - wch - Character that triggered the event | ||
| // Return Value: | ||
| // - <none> | ||
| void StateMachine::_EventDcsIgnore(const wchar_t wch) noexcept | ||
| void StateMachine::_EventDcsIgnore() noexcept |
This comment has been minimized.
This comment has been minimized.
skyline75489
Aug 23, 2020
Author
Member
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.
|
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. |
|
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 |
|
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 |
|
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". |
msftbot
bot
merged commit f91b53d
into
microsoft:master
## 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: |
skyline75489 commentedAug 19, 2020
•
edited by DHowett
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 #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