Investigate ESC D defined as IND vs CUB #976
Labels
Area-VT
Virtual Terminal sequence support
Help Wanted
We encourage anyone to jump in on these.
Issue-Feature
Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Product-Conhost
For issues in the Console codebase
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Comments
zadjii-msft
added
Product-Conhost
ghost
added
the
Needs-Triage
|
We may need to support the sequence that switches us between |
DHowett-MSFT
added
Issue-Feature
|
I've been tinkering with this for a while now (mostly the VT52 side of things), and I've got to the point where I think I have a reasonable approach to splitting out those commands and adding in the rest of the VT52 functionality. What I have so far is good enough to pass at least the basic VT52 tests in Vttest. But before I go any further, I want to check whether anyone else is planning to work on this, or if it's OK for me to continue? If you give me the go ahead, I'll try and put together a proper feature spec detailing the approach I'm taking, and you can let me know if you think I'm doing anything fundamentally wrong. Once the VT52 stuff is out of the way, the IND command should be reasonably straightforward. But if anyone else wants to take that on in the meantime, the two issues are largely orthogonal. You can just drop the VT52 cursor movements from |
|
In VT100+, DECANM enters VT52 mode, but then in the VT52 command set there's a further ANSI mode, entered with
update: never mind -- lol, I was going around in circles. ANSI = vt100+. So in effect, |
5 tasks
|
I realise I haven't been given the go-ahead to work on this, but nobody said not to either, so I've started a draft spec outlining how I think this could be implemented (i.e. splitting the VT52 commands from the VT100 implementation). If anyone doesn't want me to continue with this, feel free to tell me to stop - I won't mind at all. |
This was referenced Oct 14, 2019
5 tasks
ghost
pushed a commit
that referenced
this issue
Dec 24, 2019
…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.
ghost
pushed a commit
that referenced
this issue
Jan 15, 2020
## Summary of the Pull Request This adds support for the `FF` (form feed) and `VT` (vertical tab) [control characters](https://vt100.net/docs/vt510-rm/chapter4.html#T4-1), as well as the [`NEL` (Next Line)](https://vt100.net/docs/vt510-rm/NEL.html) and [`IND` (Index)](https://vt100.net/docs/vt510-rm/IND.html) escape sequences. ## References #976 discusses the conflict between VT100 Index sequence and the VT52 cursor back sequence. ## PR Checklist * [x] Closes #3189 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] 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: #3189 ## Detailed Description of the Pull Request / Additional comments I've added a `LineFeed` method to the `ITermDispatch` interface, with an enum parameter specifying the required line feed type (i.e. with carriage return, without carriage return, or dependent on the [`LNM` mode](https://vt100.net/docs/vt510-rm/LNM.html)). The output state machine can then call that method to handle the various line feed control characters (parsed in the `ActionExecute` method), as well the `NEL` and `IND` escape sequences (parsed in the `ActionEscDispatch` method). The `AdaptDispatch` implementation of `LineFeed` then forwards the call to a new `PrivateLineFeed` method in the `ConGetSet` interface, which simply takes a bool parameter specifying whether a carriage return is required or not. In the case of mode-dependent line feeds, the `AdaptDispatch` implementation determines whether the return is necessary or not, based on the existing _AutoReturnOnNewLine_ setting (which I'm obtaining via another new `PrivateGetLineFeedMode` method). Ultimately we'll want to support changing the mode via the [`LNM` escape sequence](https://vt100.net/docs/vt510-rm/LNM.html), 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 `PrivateLineFeed` implementation, that is just a simplified version of how the line feed would previously have been executed in the `WriteCharsLegacy` function. This includes setting the cursor to "On" (with `Cursor::SetIsOn`), potentially clearing the wrap property of the line being left (with `CharRow::SetWrapForced` false), and then setting the new position using `AdjustCursorPosition` with the _fKeepCursorVisible_ parameter set to false. I'm unsure whether the `SetIsOn` call 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::LineFeed` method in the `TerminalDispatch` class. There is currently no need to support mode-specific line feeds here, so this simply forwards a `\n` or `\r\n` to the `Execute` method, which is ultimately handled by the `Terminal::_WriteBuffer` implementation. ## 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 `PrivateLineFeed` API correctly. And finally I added some screen buffer tests that check the actual results of the `NEL` and `IND` sequences, which covers both forms of the `PrivateLineFeed` API (i.e. with and without a carriage return). I've also run the _Test of cursor movements_ in the [Vttest](https://invisible-island.net/vttest/) utility, and confirmed that screens 1, 2, and 5 are now working correctly. The first two depend on `NEL` and `IND` being supported, and screen 5 requires the `VT` control character.
ghost
pushed a commit
that referenced
this issue
Jun 1, 2020
## Summary of the Pull Request This PR adds support for the core VT52 commands, and implements the `DECANM` private mode sequence, which switches the terminal between ANSI mode and VT52-compatible mode. ## References PR #2017 defined the initial specification for VT52 support. PR #4044 removed the original VT52 cursor ops that conflicted with VT100 sequences. ## PR Checklist * [x] Closes #976 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] 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: #2017 ## Detailed Description of the Pull Request / Additional comments Most of the work involves updates to the parsing state machine, which behaves differently in VT52 mode. `CSI`, `OSC`, and `SS3` sequences are not applicable, and there is one special-case escape sequence (_Direct Cursor Address_), which requires an additional state to handle parameters that come _after_ the final character. Once the parsing is handled though, it's mostly just a matter of dispatching the commands to existing methods in the `ITermDispatch` interface. Only one new method was required in the interface to handle the _Identify_ command. The only real new functionality is in the `TerminalInput` class, which needs to generate different escape sequences for certain keys in VT52 mode. This does not yet support _all_ of the VT52 key sequences, because the VT100 support is itself not yet complete. But the basics are in place, and I think the rest is best left for a follow-up issue, and potentially a refactor of the `TerminalInput` class. I should point out that the original spec called for a new _Graphic Mode_ character set, but I've since discovered that the VT terminals that _emulate_ VT52 just use the existing VT100 _Special Graphics_ set, so that is really what we should be doing too. We can always consider adding the VT52 graphic set as a option later, if there is demand for strict VT52 compatibility. ## Validation Steps Performed I've added state machine and adapter tests to confirm that the `DECANM` mode changing sequences are correctly dispatched and forwarded to the `ConGetSet` handler. I've also added state machine tests that confirm the VT52 escape sequences are dispatched correctly when the ANSI mode is reset. For fuzzing support, I've extended the VT command fuzzer to generate the different kinds of VT52 sequences, as well as mode change sequences to switch between the ANSI and VT52 modes. In terms of manual testing, I've confirmed that the _Test of VT52 mode_ in Vttest now works as expected.
ghost
added
the
Resolution-Fix-Committed
|
🎉This issue was addressed in #4789, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Right now in our documentation,
ESC Dis defined as the VT sequence CUB. However, in the vt-100 spec,ESC Dshould be IND.In fact, I'm not sure that any of what we have on
ESC A,ESC B,ESC C,ESC D(source) is correct. I vaguely recall something aboutANSI.SYSfor those sequences, but I can't be sure.We should investigate if those sequences are being handled correctly, and if they aren't, make sure they are fixed.
And update the documentation with whatever we find.
The text was updated successfully, but these errors were encountered: