Add support for the DECSCNM screen mode #3817
Conversation
There was a problem hiding this comment.
- We should file a follow-up to "passthrough DECSCNM and implement support in Windows Terminal"
- If someone emits enables reverse mode, then switches to the alt/main buffer, is the other buffer reversed too? (That's what'll happen here, but I'm not sure if that's how other terminals work)
- Great work as always 😄
That's the way XTerm works - other terminals vary wildly, but that's typical for anything to do with the alt buffer. For XTerm, the reverse screen mode is kind of like a theme for people that prefer a light background (which was the original VT use case too - it could be configured from the setup screen), so it makes sense that they wouldn't reset that mode when switching buffers. |
zadjii-msft
added
Area-VT
j4james
force-pushed
the
feature-decscnm
branch
from
a47026d
to
cf629f6
Compare
|
I don't have any qualms with the implementation. It looks just fine to me. I'll sign after we finish discussion below. But what I'm looking at here is how this works relative to the existing inversion flag It's probably inappropriate to use that flag and apply it uniformly throughout the buffer for the sake of toggling, so an additional flag is almost certainly warranted. But I have two questions:
|
No, it applies to all colors, so red on blue would become blue on red. At the time a cell is rendered, if For cells that already have the
That's exactly right. It's purely a render-time effect. |
… rendering the screen.
…een setting and trigger a redraw of the screen.
…spatched correctly.
…rpretation of the colors.
j4james
force-pushed
the
feature-decscnm
branch
from
cf629f6
to
259c3ed
Compare
ghost
requested review from
miniksa,
carlos-zamora,
DHowett-MSFT and
leonMSFT
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
miniksa
added
AutoMerge
|
Hello @miniksa! 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 (
|
|
🎉 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 PR adds full support for the `DECSCNM` reverse screen mode in the Windows Terminal to align with the implementation in conhost. ## References * The conhost implementation of `DECSCNM` was in PR #3817. * WT originally inherited that functionality via the colors being passed through, but that behaviour was lost in PR #6506. ## PR Checklist * [x] Closes #6622 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema 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: #6622 ## Detailed Description of the Pull Request / Additional comments The `AdaptDispatch::SetScreenMode` now checks if it's in conpty mode and simply returns false to force a pass-through of the mode change. And the `TerminalDispatch` now has its own `SetScreenMode` implementation that tracks any changes to the reversed state, and triggers a redraw in the renderer. To make the renderer work, we just needed to update the `GetForegroundColor` and `GetBackgroundColor` methods of the terminal's `IRenderData` implementation to check the reversed state, and switch the colors being calculated, the same way the `LookupForegroundColor` and `LookupBackgroundColor` methods work in the conhost `Settings` class. ## Validation Steps Performed I've manually tested the `DECSCNM` functionality for Windows Terminal in Vttest, and also with some of my own test scripts.
This is a refactoring of the renderer color calculations to simplify the implementation, and to make it easier to support additional color-altering rendition attributes in the future (e.g. _faint_ and _conceal_). ## References * This is a followup to PRs #3817 and #6809, which introduced additional complexity in the color calculations, and which suggested the need for refactoring. ## Detailed Description of the Pull Request / Additional comments When we added support for `DECSCNM`, that required the foreground and background color lookup methods to be able to return the opposite of what was requested when the reversed mode was set. That made those methods unnecessarily complicated, and I thought we could simplify them considerably just by combining the calculations into a single method that derived both colors at the same time. And since both conhost and Windows Terminal needed to perform the same calculations, it also made sense to move that functionality into the `TextAttribute` class, where it could easily be shared. In general this way of doing things is a bit more efficient. However, it does result in some unnecessary work when only one of the colors is required, as is the case for the gridline painter. So to make that less of an issue, I've reordered the gridline code a bit so it at least avoids looking up the colors when no gridlines are needed. ## Validation Steps Performed Because of the API changes, quite a lot of the unit tests had to be updated. For example instead of verifying colors with two separate calls to `LookupForegroundColor` and `LookupBackgroundColor`, that's now achieved with a single `LookupAttributeColors` call, comparing against a pair of values. The specifics of the tests haven't changed though, and they're all still working as expected. I've also manually confirmed that the various color sequences and rendition attributes are rendering correctly with the new refactoring.
Summary of the Pull Request
This adds support for the
DECSCNMprivate 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, 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
Detailed Description of the Pull Request / Additional comments
I've implemented this as a new flag in the
Settingsclass, along with updates to theLookupForegroundColorandLookupBackgroundColormethods, to switch the returned foreground and background colors when that flag is set.It also required a new private API in the
ConGetSetinterface to toggle the setting. And that API is then called from theAdaptDispatchclass when the screen mode escape sequence is received.The last thing needed was to add a step to the
HardResetmethod, to reset the mode back to normal, which is one of theRISrequirements.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 flashcommand (in a bash shell) does actually cause the screen to flash.