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

supports-color breaks coloring #683

Closed
NaNraptor opened this issue Feb 7, 2019 · 12 comments
Closed

supports-color breaks coloring #683

NaNraptor opened this issue Feb 7, 2019 · 12 comments
Labels
bug This issue identifies a malfunction

Comments

@NaNraptor
Copy link

NaNraptor commented Feb 7, 2019

This line over here defines an ANSI extended color set for bright color which doesn't display correctly: (by incorrectly I mean it's just bold white color)

https://github.com/visionmedia/debug/blob/5c7c61dc0df0db4eb5de25707d8cd1b9be1add4f/src/node.js#L169

It shouldn't(?) have a traling ;1 after ${colorCode}. So the line should like:

const prefix = ` ${colorCode}m${name} \u001B[0m`;

This fixes the bug (for me :) )

EDIT: This supports my observation 256 colors

Also from the article:

Note that the bright versions of the background colors do not change the background, but rather make the foreground text brighter. This is unintuitive but that's just the way it works.

and the ;1 option only applies to \u001b[44 ie background colors, while the \u001b[38 option for foreground colors does not have such thing

@Qix-
Copy link
Member

Qix- commented Feb 7, 2019

  • 1m with 38;...m works just fine, in theory.
  • which emulator are you using?
  • screenshot please.

@NaNraptor
Copy link
Author

NaNraptor commented Feb 7, 2019

Sorry for the late reply, here are some screenshots.

Here I'm using debug v4.1.1 installed with npm install debug.
The result is the same when using git bash(with cmd console) and powershell:
debug_old

What I found strange is that the +NNNms has the correct color, while the namespace didn't, I narrowed it down to that line I pointed to in my initial comment.

Below I'm using my fork as the version installed with npm install https://github.com/NaNraptor/debug
Here is the result when I use the version from my fork:
debug_my

@Qix-
Copy link
Member

Qix- commented Feb 7, 2019

Could you do me a favor and change this line:

https://github.com/visionmedia/debug/blob/5c7c61dc0df0db4eb5de25707d8cd1b9be1add4f/src/node.js#L168

To this:

-const colorCode = '\u001B[3' + (c < 8 ? c : '8;5;' + c);
+const colorCode = '\\e[3' + (c < 8 ? c : '8;5;' + c);

Re-run and post the results here?

@Qix- Qix- added the awaiting-response This issue or pull request is awaiting a user's response label Feb 7, 2019
@NaNraptor
Copy link
Author

NaNraptor commented Feb 7, 2019

Yes of course.

First run is when ;1 is not in the code, the second run is with it:

debug_test

@Qix-
Copy link
Member

Qix- commented Feb 8, 2019

Those look fine, they should be working. This is a bug in CMD.exe.

I'll open a ticket with Microsoft later.

@Qix- Qix- added bug This issue identifies a malfunction and removed awaiting-response This issue or pull request is awaiting a user's response labels Feb 8, 2019
@NaNraptor
Copy link
Author

You are in fact correct, I tested it on a mac and it works just fine

@Qix-
Copy link
Member

Qix- commented Feb 8, 2019

Bold has always been quite broken on Windows. Here's an open issue that probably has to do with this bug.

microsoft/terminal#109

For posterity, here's a bug where bold-off (21m) was completely missing that I filed in 2017.

microsoft/WSL#2174

So, it's an ongoing process.


cc @zadjii-msft is this a new problem that I should file, or is this the same as microsoft/terminal#109?

@zadjii-msft
Copy link

So, microsoft/terminal#109 is mostly just tracking the rendering of bolded text, which is still somewhere down the backlog and unsupported.

However, the bug here is that combining 256-color values (38;5;210m for ex) and boldness (1m) doesn't work correctly. I don't know when for a fact that was fixed, but I do believe it's fixed in Insider's now, and will be generally available with the next Windows update after 1809:

image

@Qix-
Copy link
Member

Qix- commented Feb 8, 2019

Wonderful, thanks for the quick response as always @zadjii-msft :)

I'm going to close this as fixed. Thanks for the report @NaNraptor, good find.

@webuniverseio
Copy link

@qix, what if I make a PR that removes boldness based on environment variable? I'm working in a corporate environment and very likely that windows updates that will bring this feature will take a while :(. And it will depend on each of my coworkers to update their machine 😢. Thanks

@Qix-
Copy link
Member

Qix- commented May 4, 2019

@webuniverseio I wouldn't approve that, unfortunately. I don't want to introduce more environment variables when we're working to remove them.

You can, however, do FORCE_COLOR=0 as an environment variable

@webuniverseio
Copy link

@Qix- Thank you so much for your response. Unfortunately I'm trying to archive the opposite - get different colors with supports-color. I guess for now I have to use a workaround.

I have a different proposal - what if everything will be in normal font when color is used? Even on windows 1809 I don't see bold text & even if was bold, not sure how much value would it add. I guess it can be useful when there is no color & perhaps it really helps to visually separate logs on other OS...

Please let me know what you think. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue identifies a malfunction
Development

No branches or pull requests

4 participants