lib: support dumb terminals by wlodzislav · Pull Request #26261 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation59 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

wlodzislav

Add checks for TERM='dumb' in console and repl.

Add support for dumb terminals in readline. When TERM='dumb':

Checklist

@nodejs-github-bot nodejs-github-bot added console

Issues and PRs related to the console subsystem.

readline

Issues and PRs related to the built-in readline module.

repl

Issues and PRs related to the REPL subsystem.

labels

Feb 22, 2019

@silverwind

@BridgeAR do we need this? Maybe these cases can be covered via hasColor from #26247.

@wlodzislav

It is different from hasColor. Dumb terminals doesn't support any ANSI escape codes(not only color codes).

According to infocmp dumb dumb terminals support only escape and return. So TERM=dumb means that tty couldn't process any escape codes.

readline uses escape codes to redraw full line every keypress it it's run in tty, regardless of colors.

@wlodzislav

It is annoying when you run node repl or any cmd with console.log/dir inside GUI versions of VIM or emacs. It makes running node directly from them unusable.

BridgeAR

@BridgeAR

@silverwind as pointed out by @wlodzislav this is indeed required to really support dumb terminals. The hasColors function is still useful in this context (otherwise we'd have to change more places throughout the code base).

BridgeAR

@BridgeAR BridgeAR added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Feb 22, 2019

@silverwind

this is indeed required to really support dumb terminals

Then I would suggest adding something like tty.supportsEscapes(). I think its result could be statically determined at startup. Possibly the same for the color methods. Thought that comes with the drawback that users can no longer change the environment vars it uses at runtime.

Having separate write methods _ttyWriteDumb seems like nonsense.

@wlodzislav

Having separate write methods _ttyWriteDumb seems like nonsense.

I originally thought to use _normalWrite, but it doesn't do the job - it ignores all incoming keys.

Dumb terminals couldn't process escape codes, but they could send any keys(like C-c).

And using _ttyWrite means that all branches will be wrapped in if(dumb) check to ignore all incoming keys that trigger commands that work with the cursor(basically all of them).

@BridgeAR

@silverwind

I would suggest adding something like tty.supportsEscapes()

But what does that function do internally? I guess it would just return process.env.TERM !== 'dumb'.

Having separate write methods _ttyWriteDumb seems like nonsense.

We could theoretically just skip mostly everything and add more code to _ttyWrite() but that just increases the complexity of an already complex function. Separating the logic seems therefore right to me.

@wlodzislav

Adding only typeof check makes parallel/test-repl-editor.js fail. When _ttyWrite is called with s='' there is an unnescessary call to this._writeToOutput(s) without non-empty check.

@wlodzislav wlodzislav changed the titleFix: Nodejs STILL sends ANSI escape sequences to dumb terminals. #26187 Fix: Nodejs STILL sends ANSI escape sequences to dumb terminals

Feb 22, 2019

@silverwind

But what does that function do internally? I guess it would just return process.env.TERM !== 'dumb'.

Oh, I was under the assumption that is ecompasses more than just that, but I guess if it is that simple, it might make sense to not move it to a method.

@silverwind

Separating the logic seems therefore right to me.

On second thought, it probably makes sense, if we can avoid too much code duplication.

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Mar 3, 2019

@BridgeAR

PR-URL: nodejs#26264 Refs: nodejs#26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Mar 3, 2019

@BridgeAR

  1. Using process.env.TERM = 'dumb' should never return any colors.
  2. process.env.TERM = 'terminator' supports 24 bit colors.
  3. Add support for process.env.TERM = 'rxvt-unicode-24bit'
  4. Hyper does not support true colors anymore. It should fall back to the xterm settings in regular cases.
  5. process.env.COLORTERM = 'truecolor' should return 24 bit colors.

PR-URL: nodejs#26264 Refs: nodejs#26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com

BridgeAR added a commit that referenced this pull request

Mar 4, 2019

@BridgeAR

PR-URL: #26264 Refs: #26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com

BridgeAR added a commit that referenced this pull request

Mar 4, 2019

@BridgeAR

  1. Using process.env.TERM = 'dumb' should never return any colors.
  2. process.env.TERM = 'terminator' supports 24 bit colors.
  3. Add support for process.env.TERM = 'rxvt-unicode-24bit'
  4. Hyper does not support true colors anymore. It should fall back to the xterm settings in regular cases.
  5. process.env.COLORTERM = 'truecolor' should return 24 bit colors.

PR-URL: #26264 Refs: #26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com

@BridgeAR

Ping @wlodzislav this needs a rebase and I landed my upstream fix to improve the color detection. So now it should be possible to address #26261 (comment) and #26261 (comment).

This is already in a great state and to me it seems it only needs a little bit more polishing.

@wlodzislav

@wlodzislav

When TERM=dumb and .isTTY=true don't use ANSI escape codes and ignore all keys, except 'escape', 'return' and 'ctrl-c'.

Fixes: nodejs#26187

@wlodzislav

@wlodzislav

@wlodzislav

@BridgeAR

@Fishrock123

Is there a way to override color depth to ensure colors are outputted even if the terminal suggests it does not output color?

@BridgeAR

@Fishrock123 see #26485.

This PR is somewhat independent of that though. A dumb terminal does not only not support colors, it is not capable of handling control sequences either.

silverwind

@BridgeAR

@Trott @nodejs/tsc is the TSC fine with landing this as semver-minor instead of semver-major?

@rvagg

I'm not super keen on landing as a minor, because it's not. I don't feel strongly enough to argue it out but my default would be to stick with semver-major. A hypothetical situation is that someone is depending on parsing output in a particular way and they happen to have dumb set but don't know it, then suddenly the output is dramatically changed with an minor upgrade. I don't know the last time I saw dumb, though, but it's certainly plausable.

@BridgeAR

@rvagg in that case this PR only requires two LGs from the @nodejs/tsc.

Trott

@BridgeAR

@nodejs/tsc PTAL. This PR has a couple LGs but it misses one more TSC LG because it's semver-major.

Fishrock123

Choose a reason for hiding this comment

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

The idea as a major seems fine to me - I'd like one code clarification though.

options.useColors = self.terminal;
options.useColors = self.terminal && (
typeof self.outputStream.getColorDepth === 'function' ?
self.outputStream.getColorDepth() > 2 : true);

Choose a reason for hiding this comment

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

Shouldn't this be > 1?

Choose a reason for hiding this comment

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

Good catch while it won't have any actual impact (the return values are either 1, 4, 8 or 24).

Choose a reason for hiding this comment

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

I got that check from lib/internal/console/constructor.js:256-258, should I change the check there too in this PR?

Choose a reason for hiding this comment

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

@wlodzislav I think it’s okay to leave that for a separate PR, but here it would be good to replace it with > 1 before this is being merged.

Choose a reason for hiding this comment

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

I fixed this while landing.

@BridgeAR

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Mar 21, 2019

@wlodzislav @BridgeAR

PR-URL: nodejs#26261 Fixes: nodejs#26187 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Mar 21, 2019

@wlodzislav @BridgeAR

When TERM=dumb and .isTTY=true don't use ANSI escape codes and ignore all keys, except 'escape', 'return' and 'ctrl-c'.

PR-URL: nodejs#26261 Fixes: nodejs#26187 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Mar 21, 2019

@wlodzislav @BridgeAR

PR-URL: nodejs#26261 Fixes: nodejs#26187 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@silverwind silverwind silverwind approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@mcollina mcollina Awaiting requested review from mcollina

@mhdawson mhdawson Awaiting requested review from mhdawson

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

console

Issues and PRs related to the console subsystem.

readline

Issues and PRs related to the built-in readline module.

repl

Issues and PRs related to the REPL subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.