Add support for all the line feed control sequences #3271
Conversation
zadjii-msft
added
Product-Conhost
There was a problem hiding this comment.
I would sort of prefer losing the cursor move sequences in a separate commit before the new ones are introduced, but I don't think I'm going to lose that much sleep over it if it goes all in as one.
I only really had one particular comment here that I'm reserving sign off until I hear back from James.
As to the history of why the cursor sequences were listed... I think I originally built some sort of compatibility with ANSI.sys and then went into real VT and it just got left behind. The original TH2 implementation of the parser was a bastardization of ANSI.sys, VT52, VT100, and Xterm that all mixed in my head when I was scrambling to make something work in the first place. So correcting it now with the idea that we'll have a VT52/VT100 switch correctly "Soon(TM)" is fine with me.
j4james
force-pushed
the
feature-linefeed-controls
branch
from
abca53c
to
726722e
Compare
I've now cherry-picked the removal of the cursor movement ops into a separate PR (#4044), if you want to deal with that first. |
…T100 IND control. (#4044) ## Summary of the Pull Request This removes support for the the VT52 cursor movement operations, in preparation for PR #3271, since the cursor back operation conflicts with the VT100 [`IND`](https://vt100.net/docs/vt510-rm/IND.html) sequence, which we're planning to add. Eventually these ops will be brought back as part of a proper VT52 implementation, when appropriately activated by the [`DECANM`](https://vt100.net/docs/vt510-rm/DECANM.html) mode. ## References #976 #3271 ## PR Checklist * [ ] Closes #xxx * [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: #3271 ## Detailed Description of the Pull Request / Additional comments The operations were removed from the `OutputStateMachineEngine`, and their associated test cases were removed from `StateMachineExternalTest`. There is no real loss of functionality here, since these sequences were never valid as implemented. ## Validation Steps Performed I've just tested manually to confirm that the sequences no longer work.
j4james
force-pushed
the
feature-linefeed-controls
branch
from
726722e
to
b60af11
Compare
ghost
requested review from
There was a problem hiding this comment.
I love all the actual tests you've added here. Sorry though that merging #4005 created a ton of merge conflicts on this PR :/
No worries. I was expecting that. I'll try get it merged over the weekend. |
…e different linefeed/newline variants required by VT mode.
…h the WriteCharsLegacy function.
j4james
force-pushed
the
feature-linefeed-controls
branch
from
b60af11
to
db477bf
Compare
|
@DHowett-MSFT Do we want to bring this in for 0.8? I'm inclined to hold it for 0.9 personally - just rather not have a change like this with as little runway as we have left. We might be able to tell the bot to merge this next week... |
|
Let's hold on this one. I wonder if the bot can do that. |
|
@msftbot merge this in 1 week |
|
Hello @DHowett-MSFT! I think you told me that you want to delay the approval for a certain amount of time, but I am not confident that I have understood you correctly. Please try rephrasing your instruction to me. |
|
@msftbot merge this in 168 hours |
ghost
added
the
AutoMerge
|
Hello @DHowett-MSFT! 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 merge this in 1 minute |
|
Hello @zadjii-msft! 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". |
This commit moves the handling of the `BEL`, `BS`, `TAB`, and `CR` controls 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`, and `CAN` are already an integral part of the `StateMachine` itself; `NUL` is filtered out by the `OutputStateMachineEngine`; and `LF`, `FF`, and `VT` are 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 `WriteCharsLegacy` function, which should simplify it considerably. This would also let us simplify the `Terminal::_WriteBuffer` implementation, and the planned replacement stream writer for issue #780. On the conhost side, the implementation is handled as follows: * The `BS` control is dispatched to the existing `CursorBackward` method, with a distance of 1. * The `TAB` control is dispatched to the existing `ForwardTab` method, with a tab count of 1. * The `CR` control required a new dispatch method, but the implementation was a simple call to the new `_CursorMovePosition` method from PR #3628. * The `BEL` control also required a new dispatch method, as well as an additional private API in the `ConGetSet` interface. But that's mostly boilerplate code - ultimately it just calls the `SendNotifyBeep` method. On the Windows Terminal side, not all dispatch methods are implemented. * There is an existing `CursorBackward` implementation, so `BS` works OK. * There isn't a `ForwardTab` implementation, but `TAB` isn't currently required by the conpty protocol. * I had to implement the `CarriageReturn` dispatch method, but that was a simple call to `Terminal::SetCursorPosition`. * The `WarningBell` method I've left unimplemented, because that functionality wasn't previously supported anyway, and there's an existing issue for that (#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. References #3271 References #780 References #3628 References #4046
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 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 adds support for the
FF(form feed) andVT(vertical tab) control characters, as well as theNEL(Next Line) andIND(Index) escape sequences.References
#976 discusses the conflict between VT100 Index sequence and the VT52 cursor back sequence.
PR Checklist
Detailed Description of the Pull Request / Additional comments
I've added a
LineFeedmethod to theITermDispatchinterface, with an enum parameter specifying the required line feed type (i.e. with carriage return, without carriage return, or dependent on theLNMmode). The output state machine can then call that method to handle the various line feed control characters (parsed in theActionExecutemethod), as well theNELandINDescape sequences (parsed in theActionEscDispatchmethod).The
AdaptDispatchimplementation ofLineFeedthen forwards the call to a newPrivateLineFeedmethod in theConGetSetinterface, which simply takes a bool parameter specifying whether a carriage return is required or not. In the case of mode-dependent line feeds, theAdaptDispatchimplementation determines whether the return is necessary or not, based on the existing AutoReturnOnNewLine setting (which I'm obtaining via another newPrivateGetLineFeedModemethod).Ultimately we'll want to support changing the mode via the
LNMescape sequence, but there's no urgent need for that now. And using the existing AutoReturnOnNewLine setting as a substitute for the mode gives us backwards compatible behaviour, since that will be true for the Windows shells (which expect a linefeed to also generate a carriage return), and false in a WSL bash shell (which won't want the carriage return by default).As for the actual
PrivateLineFeedimplementation, that is just a simplified version of how the line feed would previously have been executed in theWriteCharsLegacyfunction. This includes setting the cursor to "On" (withCursor::SetIsOn), potentially clearing the wrap property of the line being left (withCharRow::SetWrapForcedfalse), and then setting the new position usingAdjustCursorPositionwith the fKeepCursorVisible parameter set to false.I'm unsure whether the
SetIsOncall is really necessary, and I think the way the forced wrap is handled needs a rethink in general, but for now this should at least be compatible with the existing behaviour.Finally, in order to make this all work in the Windows Terminal app, I also had to add a basic implementation of the
ITermDispatch::LineFeedmethod in theTerminalDispatchclass. There is currently no need to support mode-specific line feeds here, so this simply forwards a\nor\r\nto theExecutemethod, which is ultimately handled by theTerminal::_WriteBufferimplementation.Validation Steps Performed
I've added output engine tests which confirm that the various control characters and escape sequences trigger the dispatch method correctly. Then I've added adapter tests which confirm the various dispatch options trigger the
PrivateLineFeedAPI correctly. And finally I added some screen buffer tests that check the actual results of theNELandINDsequences, which covers both forms of thePrivateLineFeedAPI (i.e. with and without a carriage return).I've also run the Test of cursor movements in the Vttest utility, and confirmed that screens 1, 2, and 5 are now working correctly. The first two depend on
NELandINDbeing supported, and screen 5 requires theVTcontrol character.