search

PowerShell / PSReadLine

A bash inspired readline implementation for PowerShell
BSD 2-Clause "Simplified" License
3.75k stars 296 forks source link

Incorrect color handling / color handling improvements #1110

Open jhoneill opened 5 years ago

jhoneill commented 5 years ago

This is both a bug and a feature request, which could be better as single issues. It is connected to https://github.com/PowerShell/PowerShell/issues/10811 First the bug. Currently the code assumes default colors are in force in the console. A simple example is $host.ui.RawUI.foregroundColor = "cyan" changes the color of the output text but
get-psreadlineOption changes it back to the terminal default. This is because after setting the color with an escape sequence there is reset to console defaults \x1b[0m. To fix this in cmdlets.cs, inside formatcolor() line 1059 "\x1b[0m"; should be the escape sequence for host.ui.rawui.foregroundcolor

Additionally the error color should be the one set in host.private data, In the same file in PSConsoleReadLineOptions line 79 DefaultErrorColor = ConsoleColor.Red; should set the value to host.privateData.errorforegroundColor if it is a console color, and only if it is not set ConsoleColor.Red.

and in resetcolors () line 427 fg = console.ForegroundColor -- should set the value to host.ui.rawui.foregroundcolor if it is a console color otherwise -1

and in resetcolors () line 442 bg = console.backgroundColor -- should set the value to host.ui.rawui.backgroundcolor if it is a console color otherwise -1

Now the improvements. The thinking

  1. The colors in psreadline are a de-facto theme. If the the user has accessibility issues which require specific colours or if the have strong views about colors they want to see they will almost certainly set the values for PS readline, and also the values in host.privatedata for error,verbose,debug colors etc.
  2. In https://github.com/PowerShell/PowerShell/issues/10811 I suggest trying to get anything which uses color to heed the "color theme" / user choices, and provide a common mechanism to set colors rather than hard coding colors which seem like a good idea to the author and have them going to output when they should not.

Much of the work for this has already been done in psreadline Two enhancements are needed. first in cmdlets.cs after the public read/write properties for the psreadline colors. (after about line 404) Insert read only properties for formatAccentColor, verboseColor, warningColor, debugcolor, to return values from Host.PrivateData and for foregroundColor & backgroundColor to return from host.ui.rawui. Rationale PSConsoleReadLine.GetOptions() then returns all theme colors in one place. Second VTColorUtils should have an addition static method which takes a string to apply formatting to and an enum whose members are the theme colors, reverse, and underline, and applies that formatting to the string and ends with set-to-foreground-color / underline-off / reverse-off. Ideally this would check a preference variable and host.ui.SupportsVirtualTerminal and return the text unformatted where appropriate. Rationale instead of encoding "\x1b[32m"+<some value>+" \x1b[0m" either in a format.ps1xml file or in compiled code the formatting is [vtColourUtils]::ApplyFormat(<someValue>,formatAccentColor) Which can be promoted as good practice. I understand these are not "read line" things but as the owner of VT Color utils, in my view it would be silly to do them anywhere else.

lzybkr commented 5 years ago

I believe that \x1b[0m is necessary because it is common to have a terminal with foreground and background colors that are not a valid ConsoleColor. It might not be an issue on non-Windows where -1 is the value returned from Console.ForegroundColor and Console.BackgroundColor (and hence the default values in $host.UI.RawUI, but it would definitely be an issue with the new Windows Terminal.

Actually that's not entirely true, XTPUSHSGR/XTPOPSGR is precisely what should be used here, but I don't think there is critical mass in support yet that we can use those.

But I have to ask - is it really a good idea to have PowerShell specific terminal settings? $host.UI.RawUI is in my opinion a legacy api with good intentions, but not especially relevant anymore. If a user is configuring their terminal purely in PowerShell - other applications won't honor those settings.

jhoneill commented 5 years ago

I believe that \x1b[0m is necessary because it is common to have a terminal with foreground and background colors that are not a valid ConsoleColor.

Ah ... if I set the background RGB of 64,64,0 - a sort of muddy brown $host.ui.rawui reports it as yellow (Dark yellow) and the console seems to redefine yellow as that combination. If I set host.ui.rawui.backgroundcolor to blue and back to yellow I get mud. But if I but if I set the background as with \x1b[43m I get 128,128,0 "true yellow".

Well ... that's a real pain. Because you have to choose between either honouring a temporary change made by setting $host.ui.rawui but breaking custom colours OR, honouring custom colours and breaking the temporary change. Which is more of a problem ? I've never used custom colours myself and so I assume no-one else does, anywhere, ever :-) So I automatically wonder if you're right about it being common, there's little evidence either way, and the lack of people protesting that they set the foreground colour with $host.ui.rawui and the syntax colouring breaks it suggests I might be worrying about the situation which doesn't occur very often.

But I have to ask - is it really a good idea to have PowerShell specific terminal settings? $host.UI.RawUI is in my opinion a legacy api with good intentions, but not especially relevant anymore. If a user is configuring their terminal purely in PowerShell - other applications won't honor those settings.

I see $host.UI.RawUI as a way to set something for a session, or part of a session. The more I fiddle with it, the more things I find can reset it and ANSI escape sequences are the latest in a line of those (VS Code is pretty poor at respecting it too). There doesn't seem to be a way to set the different terminals' foreground/background colours via a script so setting it via $host has superficial appeal...

Did you have any thoughts on the enhancements ?

lzybkr commented 5 years ago

It seems Windows Terminal has chosen a background color for PowerShell that is not used in normal color table, see here

I also recall seeing the same in a default Ubuntu desktop - but it's been awhile so I might be mistaken.

As the use of \x1b[0m seems to be the defacto standard in command line tools, I think it's a mistake to do something else - otherwise PowerShell just ends up acting slightly weird and most users won't understand why.

As for themes - I think that maybe belongs in PowerShell rather than PSReadLine. There may be some interest in making PSReadLine agnostic to PowerShell so that it can be used in other tools.

jhoneill commented 5 years ago

Yes, it does, but PowerShell sees it as "Black" - changing via $host.ui.rawui works - changes color and hides and background image. Change back to black restores the chosen color and image. You can set the same non-standard colors in the old console host image

I think PowerShell is unusual in having the ability to change foreground/background via a variable. If respecting that variable goes down the weird path, that's another reason to leave things as they are.

Themes. I was thinking (and still think) that the VTColorUtils was the central piece of this, but I've also found that the markdown renderer has it's own VT classes. I think there's a strong case for having one set of colour utilities and sending everybody to use the same ones. Maybe, picking up the themes from markdown, and from readline, and from the different streams in host private data is going to be asking too much.
But on the flip side if there is no good mechanism for applying a "theme color" in a way that respects the user saying "don't use any color codes" or "I can't distinguish 'light black' from 'dark white' so use magenta for one of them", then I foresee code authors picking bad colors (most of the time they are unhelpful - obvious exceptions being identifying error/warning/verbose, Syntax highlighting , rendering markdown, and highlighting matches in Select-string), and always sending the escape codes so piping to an external program or redirecting to a file gets dirty output. If authors had a good way ... but perhaps it's too late to get the genie back in the bottle.