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

Fix invisible file path text in PowerShell #557

Merged
merged 2 commits into from
Jul 17, 2017

Conversation

latkin
Copy link
Contributor

@latkin latkin commented Jul 16, 2017

Fix for #342

Root cause is that the standard Windows PowerShell launch shortcut sets the console background color to "dark magenta", then re-defines "dark magenta" as RGB(1, 36, 86) aka "powershell noble blue." Thus if a console app writes output with "dark magenta" foreground color, it will be invisible in a default PowerShell console.

Fix is to do a simple, Windows-only check for a "dark magenta" background at startup, and in this case change path output style to "intense magenta," which will render fine.

Result:

image

Potential concerns:

  • Arg parsing now has to know about Windows console. That's not great but it was simplest path forward for me as a rust n00b.
  • This will add "intense" styling to path output even when user has manually overridden the color themselves. Additional path:style:nointense will be needed to counteract this change.
    • Seems not terribly burdensome. By definition this will only affect users who are already comfortable overriding color settings.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@latkin Thanks for doing this!

I'm not sure if this is the path I necessarily want to go down. In particular, what do you think about just changing the default colors on Windows and removing the conditional check altogether? That way, we don't interfere with any specific user settings but we avoid the "text is invisible" problem by default.

Tangentially, I did a lot of work to avoid making wincolor a direct dependency on ripgrep in an effort to hide that layer of abstraction. I'll bow to the alter of pragmatism if necessary, but I think I'd rather see if we can get away with just changing the defaults for now.

@@ -97,12 +97,24 @@ impl Console {
///
/// If there was a problem setting attributes on the console, then an error
/// is returned.
pub fn bg(&mut self, intense: Intense, color: Color) -> io::Result<()> {
pub fn set_bg(&mut self, intense: Intense, color: Color) -> io::Result<()> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not rename these methods, since that would be a breaking change.

@latkin latkin force-pushed the latkin-powershell-color-fix branch from 1dccd43 to ca67858 Compare July 17, 2017 20:20
@latkin
Copy link
Contributor Author

latkin commented Jul 17, 2017

@BurntSushi No problem, I understand your concerns. If you are ok with simply changing the Windows defaults, then that's even easier :-) I went with Cyan for now.

PR udpated. New behavior:

image

@latkin latkin force-pushed the latkin-powershell-color-fix branch from ca67858 to 097d4d8 Compare July 17, 2017 20:31
@BurntSushi BurntSushi merged commit 354a5ca into BurntSushi:master Jul 17, 2017
@BurntSushi
Copy link
Owner

Fantastic, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants