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: add color support for iterm2 #27723

Closed
wants to merge 2 commits into from

Conversation

dlionz
Copy link

@dlionz dlionz commented May 15, 2019

Added iterm2 to the tty.js file for terminal color support #27609

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

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label May 15, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

@dlionz thanks for your contribution! This does not seem to be correct though. The terminal supports "TRUECOLOR", so 16 million colors, not only 256. Are you also certain that process.env.TERM is set to 'iterm2'? And are there any other terminal related environment variables set?

@BridgeAR
Copy link
Member

@dlionz I removed the test output from your description above.

@dlionz
Copy link
Author

dlionz commented May 16, 2019

@BridgeAR Ah I see, sorry I geuss I didnt understand before.
this is my output when I console.log(process.env.TERM): xterm-256color
so then does my change need to look like this

const TERM_ENVS = {
//code
'xterm-256color': COLORS_16m
}

thanks again

@devsnek
Copy link
Member

devsnek commented May 16, 2019

don't forget about xterm-256color-italic, tmux-256color, and tmux-256color-italic (all of these iterm2 configs support 16m)

@BridgeAR
Copy link
Member

We already have a check for /^xterm-256/. The main problem with that environment variable is that it's used by multiple terminals and some do not support true color. Is there anything to clearly identify that the terminal is indeed iterm2?

@dlionz
Copy link
Author

dlionz commented May 16, 2019

@BridgeAR I see on the process.env object this property TERM_PROGRAM: 'iTerm.app but reviewing further down the page I see a hook for this already. So I suppose I don't have anything to contribute as this time. Or later today I can go find new some terminals to try.

switch (env.TERM_PROGRAM) {
    case 'iTerm.app':
      if (!env.TERM_PROGRAM_VERSION ||
        /^[0-2]\./.test(env.TERM_PROGRAM_VERSION)) {
        return COLORS_256;
      }
      return COLORS_16m;
    case 'HyperTerm':
    case 'MacTerm':
      return COLORS_16m;
    case 'Apple_Terminal':
      return COLORS_256;
  }

@devsnek
Copy link
Member

devsnek commented May 16, 2019

taking a deeper look, iterm reports COLORTERM, and process.stdout.getColorDepth() returns 24.

@dlionz
Copy link
Author

dlionz commented May 17, 2019

Closing this pr as I didn't realize my update was already implemented. Thank you for your time

@dlionz dlionz closed this May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants