Refactor VT control sequence identification #7304
Conversation
…s and reorder alphabetically.
…acters of an escape sequence.
…acter sets identifiers.
…he escape dispatchers.
…he VT52 dispatchers.
…he CSI dispatchers.
ghost
added
Area-VT
j4james
force-pushed
the
refactor-vt-identification
branch
from
508bd22
to
c885d44
Compare
|
I'm only leaving this as a draft because I'm waiting on PR #6328 which is likely to conflict with this. Other than that I think it's ready to review. I was a bit unsure about where to put the Also I'm not sure why the x86 build is failing. I can't actually see an error being reported anyway, and it seems to build locally (given enough cleans, reboots, and retries), so I don't know if there's just a build bot issue. |
|
I kicked the x86 build. That's very unusual. |
Fine by me. That's as close as we'll get to "consolidated", and it is involved in Dispatch afterall.
That's kind of you! I'm fine with this one coming out of draft and I can throw a block on the head commit so the bot (the bot bot or a personbot) doesn't merge it. I still haven't figured out how my team uses draft PRs, so I think the best way to get more eyes on this is to mark it "ready for review" at any rate. 😄 |
There was a problem hiding this comment.
Overall I think this looks fine to me. Do we want to merge this one or #6328 first? It looks like the two might end up conflicting a bit. /cc @DHowett, @skyline75489
|
|
||
| VTID Finalize(const wchar_t finalChar) noexcept | ||
| { | ||
| return _idAccumulator + (static_cast<uint64_t>(finalChar) << _idShift); |
There was a problem hiding this comment.
Are we worried at all about using a wchar_t here as opposed to only valid char values?
There was a problem hiding this comment.
Yeah, there are actually some cases when this needs to be a full wchar_t in the input state machine - pressing something lik Alt+ € on a european keyboard will end up being encoded as ESC € so the final char in that case is going to be more than a byte. But there are no intermediates involved in that situation, so we just interpret the final id as a wchar_t when processing it. It's not as clean a solution as I'd like, but it's workable.
|
@zadjii-msft one step ahead of ya ;) check the build statuses |
|
as a heads-up, #7319 will conflict here as well due to the introduction of |
OK thanks. I'll wait for that to merge. |
|
Alright - because the merge status is on the latest commit, pushing an update should clear it & now that we're past all the build hurdles I'm not worried about the CI failing 😄 |
|
This is way better. Thanks for the vision and implementing it, @j4james. |
|
@msftbot merge this in 5 minutes |
|
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
added
the
AutoMerge
ghost
merged commit
|
🎉 Handy links: |
ghost
mentioned this pull request
This PR changes the way VT control sequences are identified and
dispatched, to be more efficient and easier to extend. Instead of
parsing the intermediate characters into a vector, and then having to
identify a sequence using both that vector and the final char, we now
use just a single
uint64_tvalue as the identifier.The way the identifier is constructed is by taking the private parameter
prefix, each of the intermediate characters, and then the final
character, and shifting them into a 64-bit integer one byte at a time,
in reverse order. For example, the
DECTLTCcontrol has a privateparameter prefix of
?, one intermediate of', and a final characterof
s. The ASCII values of those characters are0x3F,0x27, and0x73respectively, and reversing them gets you 0x73273F, so that wouldthen be the identifier for the control.
The reason for storing them in reverse order, is because sometimes we
need to look at the first intermediate to determine the operation, and
treat the rest of the sequence as a kind of sub-identifier (the
character set designation sequences are one example of this). When in
reverse order, this can easily be achieved by masking off the low byte
to get the first intermediate, and then shifting the value right by 8
bits to get a new identifier with the rest of the sequence.
With 64 bits we have enough space for a private prefix, six
intermediates, and the final char, which is way more than we should ever
need (the DEC STD 070 specification recommends supporting at least
three intermediates, but in practice we're unlikely to see more than
two).
With this new way of identifying controls, it should now be possible for
every action code to be unique (for the most part). So I've also used
this PR to clean up the action codes a bit, splitting the codes for the
escape sequences from the control sequences, and sorting them into
alphabetical order (which also does a reasonable job of clustering
associated controls).
Validation Steps Performed
I think the existing unit tests should be good enough to confirm that
all sequences are still being dispatched correctly. However, I've also
manually tested a number of sequences to make sure they were still
working as expected, in particular those that used intermediates, since
they were the most affected by the dispatch code refactoring.
Since these changes also affected the input state machine, I've done
some manual testing of the conpty keyboard handling (both with and
without the new Win32 input mode enabled) to make sure the keyboard VT
sequences were processed correctly. I've also manually tested the
various VT mouse modes in Vttest to confirm that they were still working
correctly too.
Closes #7276