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

util: use magenta;bold #14359

Closed
wants to merge 2 commits into from
Closed

util: use magenta;bold #14359

wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 19, 2017

  • Default Powershell on Windows setting remap
    ANSI magenta to the color of the background

Microsoft will not/can not change the palette remapping on all installed instances of Windows, but we can use magenta;bold instead of regular magenta 🤷‍♂️

Fixes: #14243

image

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

repl,util

* Default Powershell on Windows setting remap
  ANSI magenta to the color of the background
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jul 19, 2017
@refack refack added the repl Issues and PRs related to the REPL subsystem. label Jul 19, 2017
@refack
Copy link
Contributor Author

refack commented Jul 19, 2017

Apparently this is not tested IMHO it should.

@mscdex
Copy link
Contributor

mscdex commented Jul 19, 2017

Doesn't this change affect non-Windows too? This color will now stand out like a sore thumb compared to the others since none of the others are bolded? I'm not sure that is a good idea...

@gibfahn
Copy link
Member

gibfahn commented Jul 19, 2017

Microsoft will not/can not change the palette remapping on all installed instances of Windows

Have Microsoft said they're not willing to change the default anywhere?

@refack
Copy link
Contributor Author

refack commented Jul 19, 2017

Doesn't this change affect non-Windows too? This color will now stand out like a sore thumb compared to the others since none of the others are bolded? I'm not sure that is a good idea...

I'm not sure either... Looking for feedback...

@refack
Copy link
Contributor Author

refack commented Jul 19, 2017

Have Microsoft said they're not willing to change the default anywhere?

This wierdsnes was intentional PowerShell/PowerShell#4266 (comment), and is not directly part of Powershell, but is part of the deatult user setting.
I did not hear anything about future plans explicitly, but all current installations won't be changed.

@targos
Copy link
Member

targos commented Jul 19, 2017

On Fedora.
Before:
image

After:
image

@refack
Copy link
Contributor Author

refack commented Jul 19, 2017

On default windows console:

Before

image

After

image
I'd say 3/5 on the sore thumb scale (also the default ANSI 35 (128,0,128) seems very dark)

@joyeecheung
Copy link
Member

joyeecheung commented Jul 19, 2017

In the default terminal of MacOS,

before:
screen shot 2017-07-20 at 1 51 19 am

after:
screen shot 2017-07-20 at 1 51 31 am

both look good enough since the default background is white...

@joyeecheung
Copy link
Member

joyeecheung commented Jul 19, 2017

BTW this does stand out a bit if you put this alongside other types, but then only dates use magenta and the ISO format already stands out itself..in the end I guess most people would not care that much as long as the colors are different from each other...

screen shot 2017-07-20 at 1 59 06 am

@gibfahn
Copy link
Member

gibfahn commented Jul 20, 2017

I think I'm -1 on doing this, especially for all platforms. On some terminals the difference between normal and bold is pretty pronounced, and bold is normally used to highlight things.

Is there a way to just do it for Windows?

I also somewhat agree with @bnoordhuis's point in #14243 (comment)

It's the responsibility of the console to display that as purple / magenta.

so I'd like to be sure that Microsoft have 0 intention of fixing this (!!!) before we do this.

@silverwind
Copy link
Contributor

silverwind commented Jul 20, 2017

Also would like to see this only applied on Windows, boldening just one color would be inconsistent on other platforms (And I personally dislike the bold style in Mintty so much that I'm maintaining a fork of it that disables bold styles).

@refack
Copy link
Contributor Author

refack commented Jul 20, 2017

@nodejs/platform-windows @Fishrock123 @addaleax Do we actually want this?

tl;dr

On windows the default shortcut that launches the Powershell console, remaps ANSI color 35 (magenta) and uses it as the background.
BTW: That happens only when PS is run by double clicking the shortcut. Explicitly calling powershell.exe doesn't have this problem.

@refack
Copy link
Contributor Author

refack commented Jul 20, 2017

@refack
Copy link
Contributor Author

refack commented Jul 20, 2017

Made it so magenta;bold is only used on Windows, which made me realize this will also solve the issue:

require('util').inspect.styles.date = 'yellow' // Or whatever

@silverwind
Copy link
Contributor

BTW, this will still affect alternate terminals (hyper, mintty, console2, ...) on Windows. Maybe a better fix would be to detect PS, maybe through a environment variable?

// Magenta has problems with the deafult configuration of Powershell
// Ref: https://github.com/nodejs/node/issues/14243
if (process.platform === 'win32')
inspect.styles.date = 'magenta;bold';
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work for anything the user logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are logs a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm saying a proper "fix" would be to intercept any write to stdout and replace the non-bold sequence with the bold one.

Copy link
Contributor

Choose a reason for hiding this comment

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

OTOH, such an rewrite of colors can be considered breaking. Probably better to not go that route and instead make sure we don't print that magenta, and maybe that modules like chalk workaround it until PS fixes it.

@refack
Copy link
Contributor Author

refack commented Jul 20, 2017

I don't see anything obvious to gate on:
https://www.diffchecker.com/2NcaNnts

@silverwind
Copy link
Contributor

silverwind commented Jul 20, 2017

Hmm yeah, probably can't distinguish PS from cmd by environment alone, thought it might not be too bad to go off PSModulePath. That one shouldn't be defined in Git Bash or Cygwin at least, while still affecting things like Hyper.

Overall, I'm strongly feeling that this is better fixed on PowerShell's side, or at least users should be instructed to change the default colors.

@refack
Copy link
Contributor Author

refack commented Jul 21, 2017

Hmm yeah, probably can't distinguish PS from cmd by environment alone, thought it might not be too bad to go off PSModulePath. That one shouldn't be defined in Git Bash or Cygwin at least, while still affecting things like Hyper.

FWIW I found a tiny change; process.env.PathExt.include('.cpl') is auto added in PS...
image
https://msdn.microsoft.com/en-us/powershell/reference/5.1/microsoft.powershell.management/show-controlpanelitem

@BridgeAR
Copy link
Member

Ping @refack

@@ -242,6 +243,10 @@ inspect.styles = Object.assign(Object.create(null), {
// "name": intentionally not styling
'regexp': 'red'
});
// Magenta has problems with the deafult configuration of Powershell
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo deafult

@silverwind
Copy link
Contributor

So I see two routes forward here:

  1. Add all bold colors to util.inspect.colors and then change the default for dates to the bold variant. This color change should be done on all platforms to stay consistent.

  2. Wait for Microsoft to fix their stuff. I'm not even sure it's worth documenting the issue as it applies to all CLI programs that print non-bold magenta. I'd much prefer this solution.

@refack
Copy link
Contributor Author

refack commented Aug 31, 2017

Closing in favor of manual util.inspect.styles.date = 'bold' workaround.

@refack refack closed this Aug 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Foreground Magenta is not shown well in powershell(windows)
10 participants