Skip to content
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

Remove VT color quirk for PowerShell #17666

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Aug 5, 2024

Roughly 4 years ago we gave Windows Terminal the ability to
differentiate between black/white and the default colors.
One of the victims was PowerShell and most importantly PSReadLine,
which emit SRG 37 & 40 when what they really want is 38 & 48.
We fixed this on our side by adding a shim.

Since the addition of VT passthrough in #17510 we now intentionally
lost the ability to translate VT sequences from one thing to another.
This meant we also lost the ability to do this shim and as such
this PR removes it. Luckily Windows 11 now ships PSReadLine 2.0.0,
which contains a proper fix for this.

Unfortunately, this is not the case for Windows 10, which ships
PSReadLine 2.0.0-beta2. Users affected by this will have to install
a newer version of PSReadLine or use the default black/white theme.

See 1bf4c08

Closes #13037

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. labels Aug 5, 2024
@lhecker lhecker merged commit dfb5233 into main Aug 6, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/remove-pwsh-color-quirk branch August 6, 2024 12:23
DHowett pushed a commit that referenced this pull request Aug 8, 2024
The only reason we had the `SetTextAttributes` method in `ITerminalApi`
was to allow for conhost to remap the default color attributes when the
VT PowerShell quirk was active. Since that quirk has now been removed,
there's no need for this API anymore.

## References and Relevant Issues

The PowerShell quirk was removed in PR #17666.

## Validation Steps Performed

I've had to update all the attribute tests in adapterTest to manually
check the expected attributes, since those checks were previously being
handled in a `SetTextAttributes` mock which no longer exists.

I've also performed some manual tests of the VT attribute operations to
double check that they're still working as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news.
Projects
None yet
3 participants