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

tty: improve tty color detection #19730

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 1, 2018

This moves the getColorDepth function to the new internal/tty file. This is important to properly give attributions to @sindresorhus.

I also improved the detection by adding new values that should be checked. I am currently playing around with ncurses has_colors function. It does seem to work fine, I am just not sure where I should expose the function and how this should work on e.g. Windows. Somewhat similar: tput colors. Adding that would allow to detect simple color support. A couple of the terminals that support colors actually support more colors than currently returned, but I did not yet find a good way to always detect the version (as I have to install the terminal etc...). If anyone has a idea how to detect color support without checking environment variables: please let me know. printf and read can be used in combination, I am just not sure how to make use of that information for Node.js.

This definitely needs a backport to 9.x.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This adds attributions for the getColorDepth function as it got
inspired by https://github.com/chalk/supports-color and more sources.
This adds support for a couple more terminals.
@BridgeAR BridgeAR requested review from MylesBorins and a team April 1, 2018 17:39
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tty Issues and PRs related to the tty subsystem. labels Apr 1, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 1, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Seems fine to me, if @sindresorhus is okay with it too.

Also, fyi: http://no-color.org/

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 1, 2018

@addaleax I actually know that site, I just did not come up with the idea to use the information properly. So thanks a lot 👍

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2018
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 4, 2018

Landed in 1b71522, ceaeee0.

@BridgeAR BridgeAR closed this Apr 4, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 4, 2018
This adds attributions for the getColorDepth function as it got
inspired by https://github.com/chalk/supports-color and more sources.

PR-URL: nodejs#19730
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 4, 2018
This adds support for a couple more terminals.

PR-URL: nodejs#19730
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2018
targos pushed a commit that referenced this pull request Apr 6, 2018
This adds attributions for the getColorDepth function as it got
inspired by https://github.com/chalk/supports-color and more sources.

PR-URL: #19730
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Apr 6, 2018
This adds support for a couple more terminals.

PR-URL: #19730
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR deleted the improve-tty-color-detection branch April 1, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants