Add support for DECSCNM in Windows Terminal #6809
Conversation
…he terminal dispatcher.
ghost
added
Area-VT
|
Note that the refactoring in this PR isn't strictly necessary to get the |
|
You’re a mind reader with these refactors! Just from a change atomicity standpoint I’d like to have the refactor go in as a separate PR, but I’m not glued to that idea and the team may overrule me :) |
No problem. I'm happy to do that. Just let me know one way or the other. |
|
Let’s just split it up, it’s probably easier than waiting for a review cycle. 😄 thanks for being flexible |
j4james
force-pushed
the
feature-terminal-decscnm
branch
from
7d637b8
to
7ea7901
Compare
…atus into account.
j4james
force-pushed
the
feature-terminal-decscnm
branch
from
7ea7901
to
cf16325
Compare
|
FYI, I've just noticed that this PR exposes a bug in the "run of spaces" optimisation introduced in PR #4877. We're assuming that we don't need to send through foreground color changes on a run of spaces, because there's no text to be seen. That made perfect sense at the time, but that won't be true when the reversed screen mode is active. When that's the case, any spaces will be rendered with the foreground rather than background color, so it's essential that those foreground colors are actually passed through. I'm not sure there's any way to keep that optimisation and also have the |
|
Is it possible to retain the optimization's fidelity by making conhost AND terminal aware of DECSCNM by doing the passthrough after we update conhost's state? |
That only helps if you also force a repaint of the entire screen. And even then it's not technically correct, because DECSCNM is like a palette setting - the client itself could potentially enable it without conhost knowing anything about it. That's probably unlikely, and usage of DECSCNM in general is fairly rare, so this is really an unfortunate edge case. It'd be horrible to lose a good optimisation just to make DECSCNM work. But I don't have any ideas other than leave it broken and hope nobody notices. |
|
That explanation makes sense. Thanks 😄 |
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! 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 (
|
ghost
merged commit 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.
|
🎉 Handy links: |
ghost
mentioned this pull request
Summary of the Pull Request
This PR adds full support for the
DECSCNMreverse screen mode in the Windows Terminal to align with the implementation in conhost.References
DECSCNMwas in PR Add support for the DECSCNM screen mode #3817.PR Checklist
Detailed Description of the Pull Request / Additional comments
The
AdaptDispatch::SetScreenModenow checks if it's in conpty mode and simply returns false to force a pass-through of the mode change. And theTerminalDispatchnow has its ownSetScreenModeimplementation 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
GetForegroundColorandGetBackgroundColormethods of the terminal'sIRenderDataimplementation to check the reversed state, and switch the colors being calculated, the same way theLookupForegroundColorandLookupBackgroundColormethods work in the conhostSettingsclass.Validation Steps Performed
I've manually tested the
DECSCNMfunctionality for Windows Terminal in Vttest, and also with some of my own test scripts.