-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom default colors are problematic for legacy apps #5952
Comments
Man, I hate this issue. We wrote about it in the blog post that announced the "experimental terminal features" tab (which was intended to let users make conhost more like a terminal emulator), and... actually surprisingly, nobody complained about it. I wonder if that's because nobody used it? This is ugly, and we did have those heuristics to determine whether the application was simply setting what it thought were the defaults. Incidentally, the treatment of "matches default colors" as "wants default" is the root cause of #293. |
I did, at one point, try to make life horrible for applications. (hax branch - no warranty - no disclaimers) There was one unused legacy attribute flag on the API surface. I got to wondering, "What if?" |
On a scale of 1-horrible, how bad was that? I was considering something similar where we use that bit as "the fg and/or bg is default colored, not indexed", and then if you queried the current attributes and the FG (or BG) byte matched the FG (or BG) byte of Though, if an app just wants to set |
It was a little horrible. PowerShell doesn't exactly round-trip them, for example: it recomposes them out of what it thinks they meant. :| |
My idea was somewhat simpler than that. I was just going to suggest that a legacy background index of 0 maps to the default background, and a legacy foreground index of 7 maps to the default foreground (where 0 and 7 are whatever the legacy Screen Background and Screen Text values are set to). Legacy apps would still just see their usual range of 16 colors, but when they use the "default" colors (typically white on black), those colors would be stored as I think this is actual crucial for PowerShell, which seems to mix the VT default background and legacy black indiscriminately. If we don't do something to address that, it's going to look a mess once #2661 is fixed. This is how it currently appears in my 2661 branch: It's possible that's a result of something I've done wrong, but I'm fairly certain it's just this black/default mismatch that's the problem. |
OK, so I knocked together a POC of the solution I was proposing, and it's definitely an improvement, but there are still a few weird color artifacts in PowerShell. Here's what it looks like in my current branch (which includes the conpty narrowing fix).
On the plus side, though, there are definitely things that are improved by this patch. You can see the difference it has made by comparing it to a PowerShell session in conhost, with the Use Separate Background option set to a shade of blue, but with the Screen Background and Screen Text still set to the default black and white (which is more or less what you're getting in a conpty session). The The reason for the latter issue is because most PowerShell commands seem to use legacy APIs to set their colors. So restoring the default attributes typically means setting the colors to white on black. And without my fix in place, that won't translate back to the real default colors. Bottom line: I think what I'm proposing is a reasonable solution. The results in PowerShell aren't perfect, but that is really PowerShell's problem. A pure legacy app would probably fare much better than their current legacy-vt hybrid approach. That said, maybe these are issues that have already been fixed - I haven't tried testing with the latest PS version. |
I'm comfortable with us piloting the workaround that 0/7 are "default" when they come through the legacy API, yeah. It seems like we could safely(?) extend that to "active buffer defaults = default", but I'm not sure the risk there. Unfortunately, PS6 and PS7 are not significantly better in this regard and we may need to gate #6506 on a fix for this. :/ |
I'd love to be able to ignore the active buffer default (assuming we're talking about
I figured that might be necessary. |
That old powershell shortcut is, unfortunately, checked into the Windows source tree and isn't going anywhere. Now, if people enable the weird woo-woo experimental settings on that LNK file and stuff goes wrong, I legit don't care. Terminal and other PTY hosts are unimpacted. I'm more concerned about applications that set the default attributes programmatically and then try to print in those attributes. This could be an as-yet-unexploded minefield |
I don't think that's possible, at least as far I could tell. Those attributes are initialized at startup from the |
So, hmm. I was going to remark on the split between I stand corrected! There's no API that sets the "default" attributes -- we just have really well and truly mixed up what the word "default" means in SCREEN_INFORMATION. 😄 |
That's correct. The naming is extremely confusing. I've had to make copious notes tracking what these different attributes mean and where they're used. That's the only way I can make sense of the code. |
I had what I thought was an inspirational idea. What if we allow profiles to override the default legacy attributes, so PowerShell could set its default legacy background index to 1 (i.e. blue). That's essentially what make the original PowerShell work the way it does. That turned out to be more complicated than I thought, but I eventually got it working in my POC, and this was the result: On the plus side, However, if you tweak the But that's a whole lot of special case tweaks that all need to be in alignment for this to look right. Anyone trying to use a different color scheme is going to find things looking a mess again. Only now you're going to get bug reports about DarkBlue being rendered as black (or whatever the default background is). In short, I'd say this isn't worth the effort, but I thought I should at least present my results here in case anything thinks this approach is worth pursuing. |
One other thing I should add regarding the above test. I was a little concerned about the fact that the dir error was displayed with the default background color, since I was actually expecting that to be black. But it turns that is just a difference between powershell.exe and pwsh.exe - the former displays the error with a black background while the latter uses the default background color (technically the "startup" color). Not that it makes much of a difference - it's just good to know that PR #6506 is actually doing the right thing here, and it is possible to get it exactly matching the conhost behaviour, assuming you're willing to jump through enough hoops. |
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
🎉This issue was addressed in #6698, which has now been successfully released as Handy links: |
Environment
Windows build number: Version 10.0.18362.719
Windows Terminal version (if applicable): commit b46d393
Steps to reproduce
opencon
, to get a cmd shell.cls
.Expected behavior
I'd expect the screen to be cleared with the same colors used to render the prompt. This is what it looks like in my existing Windows cmd shell.
Actual behavior
The screen is cleared with a cyan background, while the prompt is rendered with a red background.
The problem here is the active background color has
ColorType::IsDefault
, but the console APIs only deal in legacy attributes, which can't represent a "default" color. So whencls
looks up the active background color, the legacy API returns the Screen Background index, which is cyan. And that's the color thatcls
then uses to fill the screen. The prompt in the meantime is just using the active background color, which is a genuine default color that maps to red.The reason why this used to work, is the
FillConsoleOutputAttribute
API had a hack (since removed in PR #3100) that magically converted a legacy color that matched the legacy version of the active color into that equivalentTextColor
value. However, that only applied when VT mode was enabled, so it wouldn't work for most legacy apps, and also relied on the active color just happening to match the color you wanted to fill with, which is by no means guaranteed.So one option is we could add that hack back (or something similar), but I don't think that's the right solution. Other than the limitations mentioned already, it's only solving part of the problem. I know from working on issue #2661 that this is likely to effect us in other ways once conpty is passing through the full attributes correctly (the fact that conpty maps black to default is actually shielding us from these issues).
I have an idea for how we might address this, but I wanted to think it through some more before writing up the details. In the meantime, though, I thought it best to get this issue filed so you're at least aware of the problem.
The text was updated successfully, but these errors were encountered: