Support all the newline operations #3189
Comments
j4james
added
the
Issue-Feature
ghost
added
Needs-Triage
|
This'll be important as we nail down #780. Thanks! |
DHowett-MSFT
added
Area-VT
Yes! I was just thinking about this. If you're in VT mode, the control characters should all already be parsed out from the regular text, so it seems really pointless to then forward them on to the If we got the It would also mean the |
|
@DHowett-MSFT Is it OK if I start putting together a PR for this? Or is it likely to conflict with the work that is already being done for #780? |
|
That makes sense. And I fully understand how busy you guys are, so please don't feel any pressure to deal with my issues. If I'm stepping on any toes with my PRs, or there's anything you don't like, don't hesitate to reject them. I'm on holiday at the moment, so I may be going a bit overboard with the PR submissions. 😀 |
## 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
added
Resolution-Fix-Committed
|
🎉This issue was addressed in #3271, which has now been successfully released as Handy links: |
Description of the new feature/enhancement
VT terminals support at least five newline controls that I'm aware of. There are the three control characters:
LF(linefeed),VT(vertical tab), andFF(form feed). And then there are also two escape sequences:NEL(Next Line) andIND(Index).The three control characters are simply aliases for the same operation. When the New Line Mode is reset, they perform a linefeed by itself. When the New Line Mode is set, they perform a carriage return as well as the linefeed.
The
NELandINDsequences are not affected by the mode, though. TheINDescape sequence always performs a linefeed by itself, with no carriage return. And theNELescape sequence always performs a carriage return and linefeed together.It may not seem as if there's much value in supporting all of these sequences, but they are required to pass the Vttest Test of cursor movements, so it would be nice to have them supported in the Windows console.
Proposed technical implementation details (optional)
I'd ignore the New Line Mode to start with, since that's not essential, and introduces a whole lot more complexity. And that means we could actually implement everything in the
OutputStateMachineEngine, simply with calls to theITermDispatch::Executemethod, with eitherLF, orCRandLF, depending on what was needed.That said, I think it would be nicer to add a dedicated
LineFeedmethod to theITermDispatchinterface, probably with an enum parameter specifying the required type (i.e. with carriage return, without carriage return, or mode-dependent). Then we can always add a mode condition in there at a later stage if we want to support the New Line Mode.It may even be worth adding a dedicated method in the
ConGetSetinterface for implementing the newline functionality directly, instead of going throughWriteCharsLegacy. This requires a little more work, but has the advantage that it would not be influenced by theDISABLE_NEWLINE_AUTO_RETURNflag (which can produce the wrong behaviour when not set).I should also mention there is an additional complication with implementing the
INDsequence, which is issue #976. My preference would be to drop those cursor movement operations for now, in favor of gettingINDworking. And then we can always add them back when a proper VT52 mode is implemented (which is something I still have on the back burner). But it seems pointless keeping them without the rest of the VT52 support, and without the user explicitly requesting VT52 mode.The text was updated successfully, but these errors were encountered: