Dispatch more C0 control characters from the VT state machine #4171
Conversation
j4james
force-pushed
the
move-vt-control-chars
branch
from
6f4056f to
92c75c3
Compare
|
Note that I haven't made any changes to |
There was a problem hiding this comment.
Thanks for doing this, this looks like a pretty clean PR. I'll get right on #3628 ASAP.
zadjii-msft
added
Area-VT
ghost
requested review from
|
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…the CR control character.
…to handle the BEL control character.
j4james
force-pushed
the
move-vt-control-chars
branch
from
92c75c3 to
07e2cd5
Compare
ghost
removed
the
AutoMerge
|
I just did a rebase to fix the merge conflicts, and now the buildbot refuses to run the unit tests. 😟 |
|
Sorry I hit merge to try to get the build to work and now it's worse. :( My apologies, @j4james. I'm not sure I can fix up your branch for you. |
No problem. I was expecting a merge conflict at some point, either here or in #3628 depending on which merged first. But I thought I'd have time to resolve the conflicts before anyone noticed. Wasn't expecting them both to merge so soon. :) Sorry for the stress. It looks like it's all good now though. |
## Summary of the Pull Request This PR removes all of the VT-specific functionality from the `WriteCharsLegacy` function that dealt with control characters, since those controls are now handled in the state machine when in VT mode. It also removes most of the control character handling from the `Terminal::_WriteBuffer` method for the same reason. ## References This is a followup to PR #4171 ## PR Checklist * [x] Closes #3971 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Requires documentation to be updated * [x] 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. Issue number where discussion took place: #780 (comment) ## Detailed Description of the Pull Request / Additional comments There are four changes to the `WriteCharsLegacy` implementation: 1. The `TAB` character had special case handling in VT mode which is now no longer required. This fixes a bug in the Python REPL editor (when run from a cmd shell in Windows Terminal), which would prevent you tabbing past the end of the line. It also fixes #3971. 2. Following on from point 1, the `WC_NONDESTRUCTIVE_TAB` flag could also now be removed. It only ever applied in VT mode, in which case the `TAB` character isn't handled in `WriteCharsLegacy`, so there isn't a need for a non-destructive version. 3. There used to be special case handling for a `BS` character at the beginning of the line when in VT mode, and that is also no longer required. This fixes an edge-case bug which would prevent a glyph being output for code point 8 when `ENABLE_PROCESSED_OUTPUT` was disabled. 4. There was quite a lot of special case handling for control characters in the "end-of-line wrap" implementation, which is no longer required. This fixes a bug which would prevent "low ASCII" characters from wrapping when output at the end of a line. Then in the `Terminal::_WriteBuffer` implementation, I've simply removed all control character handling, except for `LF`. The Terminal is always in VT mode, so the control characters are always handled by the state machine. The exception for the `LF` character is simply because it doesn't have a proper implementation yet, so it still passes the character through to `_WriteBuffer`. That will get cleaned up eventually, but I thought that could wait for a later PR. Finally, with the removal of the VT mode handling in `WriteCharsLegacy`, there was no longer a need for the `SCREEN_INFORMATION::InVTMode` method to be publicly accessible. That has now been made private. ## Validation Steps Performed I've only tested manually, making sure the conhost and Windows Terminal still basically work, and confirming that the above-mentioned bugs are fixed by these changes.
|
🎉 Handy links: |
|
🎉 Once again, thanks for the contribution! This pull request was included in a set of conhost changes that was just |
Summary of the Pull Request
This PR moves the handling of the
BEL,BS,TAB, andCRcontrols characters into the state machine (when in VT mode), instead of forwarding them on to the default string writer, which would otherwise have to parse them out all over again.This doesn't cover all the control characters, but
ESC,SUB, andCANare already an integral part of theStateMachineitself;NULis filtered out by theOutputStateMachineEngine; andLF,FF, andVTare due to be implemented as part of PR #3271.Once all of these controls are handled at the state machine level, we can strip out all the VT-specific code from the
WriteCharsLegacyfunction, which should simplify it considerably. This would also let us simplify theTerminal::_WriteBufferimplementation, and the planned replacement stream writer for issue #780.References
#3271, #780, #3628, #4046
PR Checklist
Detailed Description of the Pull Request / Additional comments
On the conhost side, the implementation is handled as follows:
BScontrol is dispatched to the existingCursorBackwardmethod, with a distance of 1.TABcontrol is dispatched to the existingForwardTabmethod, with a tab count of 1.CRcontrol required a new dispatch method, but the implementation was a simple call to the new_CursorMovePositionmethod from PR Improve the VT cursor movement implementation #3628.BELcontrol also required a new dispatch method, as well as an additional private API in theConGetSetinterface. But that's mostly boilerplate code - ultimately it just calls theSendNotifyBeepmethod.On the Windows Terminal side, not all dispatch methods are implemented.
CursorBackwardimplementation, soBSworks OK.ForwardTabimplementation, butTABisn't currently required by the conpty protocol.CarriageReturndispatch method, but that was a simple call toTerminal::SetCursorPosition.WarningBellmethod I've left unimplemented, because that functionality wasn't previously supported anyway, and there's an existing issue for that (Terminal doesn’t support audible bell (BEL, 0x7) #4046).Validation Steps Performed
I've added a state machine test to confirm that the updated control characters are now forwarded to the appropriate dispatch handlers. But since the actual implementation is mostly relying on existing functionality, I'm assuming that code is already adequately tested elsewhere. That said, I have also run various manual tests of my own, and confirmed that everything still worked as well as before.