Improve the propagation of color attributes over ConPTY #6506
Conversation
ghost
added
Area-Rendering
|
Ah Christmas came early this year |
There was a problem hiding this comment.
(21/34 files in; I know it's a draft, so I'm only doing preliminaries!)
We'll need to update the WDDM and BGFX render engines in src/interactivity/onecore. Unfortunately, they build as part of Windows but do not build as part of OpenConsole.sln. This bites us every so often when we need to make IRenderEngine changes 😄
Trivia: the WDDM renderer was copy/pasted to become the Dx renderer! I wonder if there's a world where we re-unify them. Michael almost certainly wants that.
| if (textAttributes.IsReverseVideo() != _lastTextAttributes.IsReverseVideo()) | ||
| { | ||
| RETURN_IF_FAILED(_SetReverseVideo(textAttributes.IsReverseVideo())); | ||
| _lastTextAttributes.SetReverseVideo(textAttributes.IsReverseVideo()); |
There was a problem hiding this comment.
This is one of those rare cases where I might prefer a macro. This opinion is not likely held by the team (but I bet it could be socialized). What do you think? It's mechanical substring replacement, admittedly less debuggable, but repetition breeds potential for errors 😄
#define UPDATE_STATE(stateName) \
if(textAttributes.Is##stateName##() != _lastTextAttributes.Is##stateName##()) { \
R_I_F(_Set##stateName##(...
UPDATE_STATE(CrossedOut)
UPDATE_STATE(Invisible)
#undef ...There was a problem hiding this comment.
Personally I'm not a fan of macros, since I find it makes the code harder to understand. Like there's no indication what properties are being referenced or updated in that UPDATE_STATE call. But if you guys prefer that sort of thing, I don't mind changing it - I can see the argument for minimizing the repetition.
However, I should add that in the longer term I would have proposed rewriting much of that code anyway, to bundle the SGR sequences (e.g. instead of sending \e[m\e[37m\e[1m\e[7m, we should really be sending something like \e[0;37;1;7m). And in that case, the code would look completely different, and could well be one line per attribute anyway.
There was a problem hiding this comment.
I'm not terribly bothered by it. If in the long term we'll evolve towards attribute bundling, I'm down for leaving this expanded to make it painful/obvious.
There was a problem hiding this comment.
I vote no macro and for James's longer term plan of sending one SGR with semicolons that makes this mostly obsolete.
OK, I'll try and update those. It looks like it should be a simple change, since both just seem to want the legacy attributes. Is there an easy way for me to build that code, at least enough to confirm that my changes are compiling?
Well what's there now looks nothing like the Dx renderer, so if you want re-unify them, that would require somebody that knows what they're doing, which is definitely not me. 😀 |
Unfortunately, no. I'll handle any breaks that end up on the Windows side -- so don't gate too hard on it. Visual verification is enough for those two until we try to re-ingest the changes.
Oh man, I wouldn't ask you to do that. Especially given that that project doesn't build in public 😄 We've considered shipping some shim headers that would make those renderers compile -- ONLY compile -- just for basic validation. |
j4james
force-pushed
the
fix-conpty-narrowing
branch
from
52f3164
to
6ce6bf2
Compare
… the attribute color format.
…t the order attributes are applied.
|
I'm switching my signoff to a sign but adding a rejected build status; that'll more correctly represent what's going on here. 😄 |
|
FYI, I've got a potential PR for #5952 which I'll probably commit tonight, but I won't be able to write up the PR details until tomorrow evening or the weekend. I'm not sure you'll like it, but it's at least an option to consider. |
ghost
removed
the
Needs-Author-Feedback
Essentially what this does is map the default legacy foreground and background attributes (typically white on black) to the `IsDefault` color type in the `TextColor` class. As a result, we can now initialize the buffer for "legacy" shells (like PowerShell and cmd.exe) with default colors, instead of white on black. This fixes the startup rendering in conpty clients, which expect an initial default background color. It also makes these colors update appropriately when the default palette values change. One complication in getting this to work, is that the console permits users to change which color indices are designated as defaults, so we can't assume they'll always be white on black. This means that the legacy-to-`TextAttribute` conversion will need access to those default values. Unfortunately the defaults are stored in the conhost `Settings` class (the `_wFillAttribute` field), which isn't easily accessible to all the code that needs to construct a `TextAttribute` from a legacy value. The `OutputCellIterator` is particularly problematic, because some iterator types need to generate a new `TextAttribute` on every iteration. So after trying a couple of different approaches, I decided that the least worst option would be to add a pair of static properties for the legacy defaults in the `TextAttribute` class itself, then refresh those values from the `Settings` class whenever the defaults changed (this only happens on startup, or when the conhost _Properties_ dialog is edited). And once the `TextAttribute` class had access to those defaults, it was fairly easy to adapt the constructor to handle the conversion of default values to the `IsDefault` color type. I could also then simplify the `TextAttribute::GetLegacyAttributes` method which does the reverse mapping, and which previously required the default values to be passed in as a parameter VALIDATION I had to make one small change to the `TestRoundtripExhaustive` unit test which assumed that all legacy attributes would convert to legacy color types, which is no longer the case, but otherwise all the existing tests passed as is. I added a new unit test verifying that the default legacy attributes correctly mapped to default color types, and the default color types were mapped back to the correct legacy attributes. I've manually confirmed that this fixed the issue raised in #5952, namely that the conhost screen is cleared with the correct default colors, and also that it is correctly refreshed when changing the palette from the properties dialog. And I've combined this PR with #6506, and confirmed that the PowerShell and the cmd shell renderings in Windows Terminal are at least improved, if not always perfect. This is a prerequisite for PR #6506 Closes #5952
DHowett
changed the title
This is essentially a rewrite of the `TerminalDispatch::SetGraphicsRendition` method, bringing it into closer alignment with the `AdaptDispatch` implementation, simplifying the `ITerminalApi` interface, and making the code easier to extend. It adds support for a number of attributes which weren't previously implemented. REFERENCES * This is a mirror of the `AdaptDispatch` refactoring in PR #5758. * The closer alignment with `AdaptDispatch` is a small step towards solving issue #3849. * The newly supported attributes should help a little with issues #5461 (italics) and #6205 (strike-through). DETAILS I've literally copied and pasted the `SetGraphicsRendition` implementation from `AdaptDispatch` into `TerminalDispatch`, with only few minor changes: * The `SetTextAttribute` and `GetTextAttribute` calls are slightly different in the `TerminalDispatch` version, since they don't return a pointless `success` value, and in the case of the getter, the `TextAttribute` is returned directly instead of by reference. Ultimately I'd like to move the `AdaptDispatch` code towards that way of doing things too, but I'd like to deal with that later as part of a wider refactoring of the `ConGetSet` interface. * The `SetIndexedForeground256` and `SetIndexedBackground256` calls required the color indices to be remapped in the `AdaptDispatch` implementation, because the conhost color table is in a different order to the XTerm standard. `TerminalDispatch` doesn't have that problem, so doesn't require the mapping. * The index color constants used in the 16-color `SetIndexedForeground` and `SetIndexedBackground` calls are also slightly different for the same reason. VALIDATION I cherry-picked this code on top of the #6506 and #6698 PRs, since that's only way to really get the different color formats passed-through to the terminal. I then ran a bunch of manual tests with various color coverage scripts that I have, and confirmed that all the different color formats were being rendered as expected. Closes #6725
There is going to be a very long tail of applications that will explicitly request VT SGR 40 when what they really want is to SetConsoleTextAttribute() with a black background. Instead of making those applications look bad (and therefore making us look bad, because we're releasing this as an update to something that "looks good" already), we're introducing this compatibility hack. Before the color reckoning in #6698 + #6506, *every* color was subject to being spontaneously and erroneously turned into the default color. Now, only the 16-color palette value that matches the active console background color will be destroyed. This is not intended to be a long-term solution. This comment will be discovered in forty years(*) time and people will laugh at my hubris. Removal, or final remediation, will be tracked by #6807. *it doesn't matter when you're reading this, it will always be 40 years from now.
## 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.
There is going to be a very long tail of applications that will explicitly request VT SGR 40/37 when what they really want is to SetConsoleTextAttribute() with a black background/white foreground. Instead of making those applications look bad (and therefore making us look bad, because we're releasing this as an update to something that "looks good" already), we're introducing this compatibility quirk. Before the color reckoning in #6698 + #6506, *every* color was subject to being spontaneously and erroneously turned into the default color. Now, only the 16-color palette value that matches the active console background/foreground color will be destroyed, and only when received from specific applications. Removal will be tracked by #6807. Michael and I discussed what layer this quirk really belonged in. I originally believed it would be sufficient to detect a background color that matched the legacy default background, but @j4james provided an example of where that wouldn't work out (powershell setting the foreground color to white/gray). In addition, it was too heavyhanded: it re-broke black backgrounds for every application. Michael thought that it should live in the server, as a small VT parser that righted the wrongs coming directly out of the application. On further investigation, however, I realized that we'd need to push more information up into the server (so that it could make the decision about which VT was wrong and which was right) than should be strictly necessary. The host knows which colors are right and wrong, and it gets final say in what ends up in the buffer. Because of that, I chose to push the quirk state down through WriteConsole to DoWriteConsole and toggle state on the SCREEN_INFORMATION that indicates whether the colors coming out of the application are to be distrusted. This quirk _only applies to pwsh.exe and powershell.exe._ NOTE: This doesn't work for PowerShell the .NET Global tool, because it is run as an assembly through dotnet.exe. I have no opinion on how to fix this, or whether it is worth fixing. VALIDATION ---------- I configured my terminals to have an incredibly garish color scheme to show exactly what's going to happen as a result of this. The _default terminal background_ is purple or red, and the foreground green. I've printed out a heap of test colors to see how black interacts with them. Pull request #6810 contains the images generated from this test. The only color lines that change are the ones where black as a background or white as a foreground is selected out of the 16-color palette explicitly. Reverse video still works fine (because black is in the foreground!), and it's even possible to represent "black on default" and reverse it into "default on black", despite the black in question having been `40`. Fixes #6767.
|
🎉 Handy links: |
Summary of the Pull Request
This PR reimplements the VT rendering engines to do a better job of preserving the original color types when propagating attributes over ConPTY. For the 16-color renderers it provides better support for default colors and improves the efficiency of the color narrowing conversions. It also fixes problems with the ordering of character renditions that could result in attributes being dropped.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Originally the base renderer would calculate the RGB color values and legacy/extended attributes up front, passing that data on to the active engine's
UpdateDrawingBrushesmethod. With this new implementation, the renderer now just passes through the originalTextAttributealong with anIRenderDatainterface, and leaves it to the engines to extract the information they need.The GDI and DirectX engines now have to lookup the RGB colors themselves (via simple
IRenderDatacalls), but have no need for the other attributes. The VT engines extract the information that they need from theTextAttribute, instead of having to reverse engineer it fromCOLORREFs.The process for the 256-color Xterm engine starts with a check for default colors. If both foreground and background are default, it outputs a SGR 0 reset, and clears the
_lastTextAttributecompletely to make sure any reset state is reapplied. With that out the way, the foreground and background are updated (if changed) in one of 4 ways. They can either be a default value (SGR 39 and 49), a 16-color index (using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value (both using SGR 38 and 48 sequences).Then once the colors are accounted for, there is a separate step that handles the character rendition attributes (bold, italics, underline, etc.) This step must come after the color sequences, in case a SGR reset is required, which would otherwise have cleared any character rendition attributes if it came last (which is what happened in the original implementation).
The process for the 16-color engines is a little different. The target client in this case (Windows telnet) is incapable of setting default colors individually, so we need to output an SGR 0 reset if either color has changed to default. With that out the way, we use the
TextColor::GetLegacyIndexmethod to obtain an approximate 16-color index for each color, and apply the bold attribute by brightening the foreground index (setting bit 8) if the color type permits that.However, since Windows telnet only supports the 8 basic ANSI colors, the best we can do for bright colors is to output an SGR 1 attribute to get a bright foreground. There is nothing we can do about a bright background, so after that we just have to drop the high bit from the colors. If the resulting index values have changed from what they were before, we then output ANSI 8-color SGR sequences to update them.
As with the 256-color engine, there is also a final step to handle the character rendition attributes. But in this case, the only supported attributes are underline and reversed video.
Since the VT engines no longer depend on the active color table and default color values, there was quite a lot of code that could now be removed. This included the
IDefaultColorProviderinterface and implementations, theFind(Nearest)TableIndexfunctions, and also the associated HLS conversion and difference calculations.Validation Steps Performed
Other than simple API parameter changes, the majority of updates required in the unit tests were to correct assumptions about the way the colors should be rendered, which were the source of the narrowing bugs this PR was trying to fix. Like passing white on black to the
UpdateDrawingBrushesAPI, and expecting it to output the defaultSGR 0sequence, or passing an RGB color and expecting an indexed SGR sequence.In addition to that, I've added some VT renderer tests to make sure the rendition attributes (bold, underline, etc) are correctly retained when a default color update causes an
SGR 0sequence to be generated (the source of bug #3076). And I've extended the VT renderer color tests (both 256-color and 16-color) to make sure we're covering all of the different color types (default, RGB, and both forms of indexed colors).I've also tried to manually verify that all of the test cases in the linked bug reports (and their associated duplicates) are now fixed when this PR is applied.