## 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: https://github.com/microsoft/terminal/issues/780#issuecomment-570287435
## 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.
## Summary of the Pull Request
This adds support for the [`DECSCNM`](https://vt100.net/docs/vt510-rm/DECSCNM.html) private mode escape sequence, which toggles the display between normal and reverse screen modes. When reversed, the background and foreground colors are switched. Tested manually, with [Vttest](https://invisible-island.net/vttest/), and with some new unit tests.
## References
This also fixes issue #72 for the most part, although if you toggle the mode too fast, there is no discernible flash.
## PR Checklist
* [x] Closes#3773
* [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
* [ ] 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: #xxx
## Detailed Description of the Pull Request / Additional comments
I've implemented this as a new flag in the `Settings` class, along with updates to the `LookupForegroundColor` and `LookupBackgroundColor` methods, to switch the returned foreground and background colors when that flag is set.
It also required a new private API in the `ConGetSet` interface to toggle the setting. And that API is then called from the `AdaptDispatch` class when the screen mode escape sequence is received.
The last thing needed was to add a step to the `HardReset` method, to reset the mode back to normal, which is one of the `RIS` requirements.
Note that this does currently work in the Windows Terminal, but once #2661 is implemented that may no longer be the case. It might become necessary to let the mode change sequences pass through conpty, and handle the color reversing on the client side.
## Validation Steps Performed
I've added a state machine test to make sure the escape sequence is dispatched correctly, and a screen buffer test to confirm that the mode change does alter the interpretation of colors as expected.
I've also confirmed that the various "light background" tests in Vttest now display correctly, and that the `tput flash` command (in a bash shell) does actually cause the screen to flash.
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
## Summary of the Pull Request
Originally there were 3 different methods for implementing VT cursor movement, and between them they still couldn't handle some of the operations correctly. This PR unifies those operations into a single method that can handle every type of cursor movement, and which fixes some of the issues with the existing implementations. In particular it fixes the `CNL` and `CPL` operations, so they're now correctly constrained by the `DECSTBM` margins.
## References
If this PR is accepted, the method added here should make it trivial to implement the `VPR` and `HPR` commands in issue #3428.
## PR Checklist
* [x] Closes#2926
* [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
* [ ] 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: #xxx
## Detailed Description of the Pull Request / Additional comments
The new [`AdaptDispatch::_CursorMovePosition`](d6c4f35cf6/src/terminal/adapter/adaptDispatch.cpp (L169)) method is based on the proposal I made in issue #3428 for the `VPR` and `HPR` comands. It takes three arguments: a row offset (which can be absolute or relative), a column offset (ditto), and a flag specifying whether the position should be constrained by the `DECSTBM` margins.
To make the code more readable, I've implemented the offsets using [a `struct` with some `constexpr` helper functions for the construction](d6c4f35cf6/src/terminal/adapter/adaptDispatch.hpp (L116-L125)). This lets you specify the parameters with expressions like `Offset::Absolute(col)` or `Offset::Forward(distance)` which I think makes the calling code a little easier to understand.
While implementing this new method, I noticed a couple of issues in the existing movement implementations which I thought would be good to fix at the same time.
1. When cursor movement is constrained horizontally, it should be constrained by the buffer width, and not the horizontal viewport boundaries. This is an issue I've previously corrected in other parts of the codebase, and I think the cursor movement was one of the last areas where it was still a problem.
2. A number of the commands had range and overflow checks for their parameters that were either unnecessary (testing for a condition that could never occur) or incorrect (if an operation overflows, the correct behavior is to clamp it, and not just fail). The new implementation handles legitimate overflows correctly, but doesn't check for impossible ranges.
Because of the change of behavior in point 1, I also had to update the implementations of [the `DECSC` and `CPR` commands](9cf7a9b577) to account for the column offset now being relative to the buffer and not the viewport, otherwise those operations would no longer work correctly.
## Validation Steps Performed
Because of the two changes in behavior mentioned above, there were a number of adapter tests that stopped working and needed to be updated. First off there were those that expected the column offset to be relative to the left viewport position and constrained by the viewport width. These now had to be updated to [use the full buffer width](49887a3589) as the allowed horizontal extent.
Then there were all the overflow and out-of-range tests that were testing conditions that could never occur in practice, or where the expected behavior that was tested was actually incorrect. I did spend some time trying to see if there was value in updating these tests somehow, but in the end I decided it was best to just [drop them](6e80d0de19) altogether.
For the `CNL` and `CPL` operations, there didn't appear to be any existing tests, so I added some [new screen buffer tests](d6c4f35cf6) to check that those operations now work correctly, both with and without margins.
## 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.
## Summary of the Pull Request
Refactors parsing/adapting libraries and consumers to use safer and/or more consistent mechanisms for passing information.
## PR Checklist
* [x] I work here
* [x] Tests still pass
* [x] Am a core contributor.
## Detailed Description of the Pull Request / Additional comments
This is in support of hopefully turning audit mode on to more projects. If I turned it on, it would immediately complain about certain classes of issues like pointer and size, pointer math, etc. The changes in this refactoring will eliminate those off the top.
Additionally, this has caught a bunch of comments all over the VT classes that weren't updated to match the parameters lists.
Additionally, this has caught a handful of member variables on classes that were completely unused (and now gone).
Additionally, I'm killing almost all hungarian and shortening variable names. I'm only really leaving 'p' for pointers.
Additionally, this is vaguely in support of a future where we can have "infinite scrollback" in that I'm moving things to size_t across the board. I know it's a bit of a memory cost, but all the casting and moving between types is error prone and unfun to save a couple bytes.
## Validation Steps Performed
- [x] build it
- [x] run all the tests
- [x] everyone looked real hard at it
## Summary of the Pull Request
Operations that erase areas of the screen are typically meant to do so using the current color attributes, but with the rendition attributes reset (what we refer to as meta attributes). This also includes scroll operations that have to clear the area of the screen that has scrolled into view. The only exception is the _Erase Scrollback_ operation, which needs to reset the buffer with the default attributes. This PR updates all of these cases to apply the correct attributes when scrolling and erasing.
## PR Checklist
* [x] Closes#2553
* [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
* [ ] I've not really discussed this with core contributors. I'm ready to accept this work might be rejected in favor of a different grand plan.
## Detailed Description of the Pull Request / Additional comments
My initial plan was to use a special case legacy attribute value to indicate the "standard erase attribute" which could safely be passed through the legacy APIs. But this wouldn't cover the cases that required default attributes to be used. And then with the changes in PR #2668 and #2987, it became clear that our requirements could be better achieved with a couple of new private APIs that wouldn't have to depend on legacy attribute hacks at all.
To that end, I've added the `PrivateFillRegion` and `PrivateScrollRegion` APIs to the `ConGetSet` interface. These are just thin wrappers around the existing `SCREEN_INFORMATION::Write` method and the `ScrollRegion` function respectively, but with a simple boolean parameter to choose between filling with default attributes or the standard erase attributes (i.e the current colors but with meta attributes reset).
With those new APIs in place, I could then update most scroll operations to use `PrivateScrollRegion`, and most erase operations to use `PrivateFillRegion`.
The functions affected by scrolling included:
* `DoSrvPrivateReverseLineFeed` (the RI command)
* `DoSrvPrivateModifyLinesImpl` (the IL and DL commands)
* `AdaptDispatch::_InsertDeleteHelper` (the ICH and DCH commands)
* `AdaptDispatch::_ScrollMovement` (the SU and SD commands)
The functions affected by erasing included:
* `AdaptDispatch::_EraseSingleLineHelper` (the EL command, and most ED variants)
* `AdaptDispatch::EraseCharacters` (the ECH command)
While updating these erase methods, I noticed that both of them also required boundary fixes similar to those in PR #2505 (i.e. the horizontal extent of the erase operation should apply to the full width of the buffer, and not just the current viewport width), so I've addressed that at the same time.
In addition to the changes above, there were also a few special cases, the first being the line feed handling, which required updating in a number of places to use the correct erase attributes:
* `SCREEN_INFORMATION::InitializeCursorRowAttributes` - this is used to initialise the rows that pan into view when the viewport is moved down the buffer.
* `TextBuffer::IncrementCircularBuffer` - this occurs when we scroll passed the very end of the buffer, and a recycled row now needs to be reinitialised.
* `AdjustCursorPosition` - when within margin boundaries, this relies on a couple of direct calls to `ScrollRegion` which needed to be passed the correct fill attributes.
The second special case was the full screen erase sequence (`ESC 2 J`), which is handled separately from the other ED sequences. This required updating the `SCREEN_INFORMATION::VtEraseAll` method to use the standard erase attributes, and also required changes to the horizontal extent of the filled area, since it should have been clearing the full buffer width (the same issue as the other erase operations mentioned above).
Finally, there was the `AdaptDispatch::_EraseScrollback` method, which uses both scroll and fill operations, which could now be handled by the new `PrivateScrollRegion` and `PrivateFillRegion` APIs. But in this case we needed to fill with the default attributes rather than the standard erase attributes. And again this implementation needed some changes to make sure the full width of the active area was retained after the erase, similar to the horizontal boundary issues with the other erase operations.
Once all these changes were made, there were a few areas of the code that could then be simplified quite a bit. The `FillConsoleOutputCharacterW`, `FillConsoleOutputAttribute`, and `ScrollConsoleScreenBufferW` were no longer needed in the `ConGetSet` interface, so all of that code could now be removed. The `_EraseSingleLineDistanceHelper` and `_EraseAreaHelper` methods in the `AdaptDispatch` class were also no longer required and could be removed.
Then there were the hacks to handle legacy default colors in the `FillConsoleOutputAttributeImpl` and `ScrollConsoleScreenBufferWImpl` implementations. Since those hacks were only needed for VT operations, and the VT code no longer calls those methods, there was no longer a need to retain that behaviour (in fact there are probably some edge cases where that behaviour might have been considered a bug when reached via the public console APIs).
## Validation Steps Performed
For most of the scrolling operations there were already existing tests in place, and those could easily be extended to check that the meta attributes were correctly reset when filling the revealed lines of the scrolling region.
In the screen buffer tests, I made updates of that sort to the `ScrollOperations` method (handling SU, SD, IL, DL, and RI), the `InsertChars` and `DeleteChars` methods (ICH and DCH), and the `VtNewlinePastViewport` method (LF). I also added a new `VtNewlinePastEndOfBuffer` test to check the case where the line feed causes the viewport to pan past the end of the buffer.
The erase operations, however, were being covered by adapter tests, and those aren't really suited for this kind of functionality (the same sort of issue came up in PR #2505). As a result I've had to reimplement those tests as screen buffer tests.
Most of the erase operations are covered by the `EraseTests` method, except the for the scrollback erase which has a dedicated `EraseScrollbackTests` method. I've also had to replace the `HardReset` adapter test, but that was already mostly covered by the `HardResetBuffer` screen buffer test, which I've now extended slightly (it could do with some more checks, but I think that can wait for a future PR when we're fixing other RIS issues).
The current DECSC implementation only saves the cursor position and
origin mode. This PR improves that functionality with additional support
for saving the SGR attributes, as well as the active character set.
It also fixes the way the saved state interacts with the alt buffer
(private mode 1049), triggering a save when switching to the alt buffer,
and a restore when switching back, and tracking the alt buffer state
independently from the main state.
In order to properly save and restore the SGR attributes, we first
needed to add a pair of APIs in the `ConGetSet` interface which could
round-trip the attributes with full 32-bit colors (the existing methods
only work with legacy attributes).
I then added a struct in the `AdaptDispatch` implementation to make it
easier to manage all of the parameters that needed to be saved. This
includes the cursor position and origin mode that we were already
tracking, and now also the SGR text attributes and the active character
set (via the `TermOutput` class).
Two instances of this structure are required, since changes made to the
saved state in the alt buffer need to be tracked separately from changes
in the main buffer. I've added a boolean property that specifies whether
we're in the alt buffer or not, and use that to decide which of the two
instances we're working with.
I also needed to explicitly trigger a save when switching to the alt
buffer, and a restore when switching back, since we weren't already
doing that (the existing implementation gave the impression that the
state was saved, because each buffer has its own cursor position, but
that doesn't properly match the XTerm behaviour).
For the state tracking itself, we've now got two additional properties -
the SGR attributes, which we obtain via the new private API, and the
active character set, which we get from a local `AdaptDispatch` field.
I'm saving the whole `TermOutput` class for the character set, since I'm
hoping that will make it automatically supports future enhancements.
When restoring the cursor position, there is also now a fix to handle
the relative origin mode correctly. If the margins are changed between
the position being saved and restored, it's possible for the cursor to
end up outside of the new margins, which would be illegal. So there is
now an additional step that clamps the Y coordinate within the margin
boundaries if the origin mode is relative.
# Validation
I've added a couple of screen buffer tests which check that the various
parameters are saved and restored as expected, as well as checking that
the Y coordinate is clamped appropriately when the relative origin mode
is set.
I've also tested manually with vttest and confirmed that the
_SAVE/RESTORE CURSOR_ test (the last page of the _Test of screen
features_)) is now working a lot better than it used to.
Closes#148.
## Summary of the Pull Request
Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does **NOT** add support for those styles to conhost or the terminal.
We will store these "Extended Text Attributes" in a `TextAttribute`. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a `bool` to a single bit in these flags.
Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however.
## References
See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1.
## PR Checklist
* [x] Closes#2554
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
<hr>
* store text with extended attributes too
* Plumb attributes through all the renderers
* parse extended attrs, though we're not renderering them right
* Render these states correctly
* Add a very extensive test
* Cleanup for PR
* a block of PR feedback
* add 512 test cases
* Fix the build
* Fix @carlos-zamora's suggestions
* @miniksa's PR feedback
* Removed using namespace directive from header files and put these in cpp files where they are used
* Fixed tabbing issues by replacing them with spaces.
Also regrouped the using directives.
* Update src/host/exemain.cpp
Co-Authored-By: Mike Griese <migrie@microsoft.com>
* Update src/interactivity/win32/find.cpp
Co-Authored-By: Mike Griese <migrie@microsoft.com>
* Support OSC to set default background and foreground colors
* Update the Terminal theme when the background changes
* Fix whitespace per code-review
* Add Documentation Comments
Also fix a few outdated comments and whitespace
* Update Telemetry codes per code review
* Add Unit Tests for OSC ForegroundColor and BackgroundColor
* Add a couple additional test cases
* Minor doc and whitespace change per PR review
* Update comment help per code review
* Add another OSC 10 & 11 test case, improve output
* Comments and syntax cleanup per code reviews
James Holderness
Michael Niksa
Mike Griese
adiviness
MelulekiDube
Joel Bennett
Dustin Howett