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 mosh #27843

Closed
wants to merge 5 commits into from
Closed

tty: add color support for mosh #27843

wants to merge 5 commits into from

Conversation

aditya1906
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label May 23, 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.

Would you be wo kind and verify process.env.TERM in the Node.js REPL while using mosh and post the output here? AFAIK the value is set to xterm-256?

@@ -53,6 +53,7 @@ const TERM_ENVS = {
'konsole': COLORS_16,
'kterm': COLORS_16,
'mlterm': COLORS_16,
'mosh': COLORS_256,
Copy link
Member

Choose a reason for hiding this comment

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

It seems like mosh has true color support?

Copy link
Contributor Author

@aditya1906 aditya1906 May 25, 2019

Choose a reason for hiding this comment

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

Hi @BridgeAR Mosh has xterm-256 support. It doesn't have truecolor support. I took a look in mosh repository and found that it has xterm-256 support and not truecolor. So shouldn't it be COLORS_256? Or what is the value for xterm-256? Please let me know if I'm mistaken.

Copy link
Member

Choose a reason for hiding this comment

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

mosh gained truecolor support a few years ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @devsnek yes I read that it is now supporting truecolor. So is my commit worth merging to the master branch? Or should it be COLORS_16?

Copy link
Member

Choose a reason for hiding this comment

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

you should change it to COLORS_16m

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have corrected the mistake

@aditya1906
Copy link
Contributor Author

Hi I made the corrections for mosh. Is it good enough?

@BridgeAR
Copy link
Member

@aditya1906 would you be so kind and make a screenshot of the corresponding environment variable entry? That way it's easier to verify that it's set as expected :)

@aditya1906
Copy link
Contributor Author

Hi @BridgeAR
Screenshot from 2019-05-29 20-21-06
In my case it shows xterm-256color.
But in order to get truecolor support we will need to install other dependencies with mosh.
Here is the process.

@addaleax
Copy link
Member

addaleax commented Jun 2, 2019

Fwiw, the commit message here should start with tty:.

@aditya1906 aditya1906 changed the title lib: add color support for mosh tty: add color support for mosh Jun 2, 2019
@aditya1906
Copy link
Contributor Author

What do you think about this?

@mbj36
Copy link
Contributor

mbj36 commented Jun 5, 2019

@aditya1906 you should change the commit message rather than the PR title.

https://help.github.com/en/articles/changing-a-commit-message this will help you learn how to change commit message.

@addaleax
Copy link
Member

addaleax commented Jun 5, 2019

@mbj36 I think in this case it’s fine – we’ll squash the commits anyway when merging. My message was more supposed to inform the person who merges this than @aditya1906 (although of course it’s great if the commit messages here are changed as well).

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2019
@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Sep 16, 2019
PR-URL: #27843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member

Trott commented Sep 16, 2019

Landed in 70abb4f

@Trott Trott closed this Sep 16, 2019
targos pushed a commit that referenced this pull request Sep 20, 2019
PR-URL: #27843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
PR-URL: #27843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants