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

Fix non-Windows terminals assumed to always support 256-color #81

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Oct 10, 2022

Backports cli/cli#6356

Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Interesting that govulncheck found an vulnerability that dependabot didn't. The vulnerability wasn't introduced in this PR so it is probably okay to merge, and we can fix the vulnerability in a follow up PR.

@heaths
Copy link
Contributor

heaths commented Oct 12, 2022

Wouldn't this completely break alternate screen buffers on *nix then? There are other ways of detecting whether VT sequences are supported:

See cli/cli#6356 (comment) for more information.

@heaths
Copy link
Contributor

heaths commented Oct 13, 2022

I submitted cli/cli#6435 to restore behavior without impacting this PR, since the change was only to restore the original behavior without assumptions of VT support this change inferred. This PR would actually be a good fix, but long-term support for various color or other VT support might be good to add to go-gh. See cli/cli#6435 (comment) for possibilities.

@mislav mislav merged commit 1209c4a into trunk Oct 13, 2022
@mislav mislav deleted the enable-virtual-terminal-processing-fix branch October 13, 2022 15:25
@mislav
Copy link
Contributor Author

mislav commented Oct 13, 2022

@heaths I don't see how this can break alternate screen buffers on *nix. There is still an issue with that in the CLI repo, I agree, but we will resolve that in that repo.

@heaths
Copy link
Contributor

heaths commented Oct 13, 2022

Agreed. Please see my latest comment.

@heaths
Copy link
Contributor

heaths commented Oct 13, 2022

To clarify my comment about VT support, I was suggesting maybe something in this module could define, but perhaps that would best be in one of @mattn's many term modules, if it isn't already (I couldn't find one).

@mattn
Copy link

mattn commented Oct 13, 2022

Can I help you? If you can explain how to reproduce the problem, I can fix that.

@heaths
Copy link
Contributor

heaths commented Oct 13, 2022

Mainly, with all the terminal modules you have, was wondering if any of them might be a good place to put better VT support detection into. See cli/cli#6435 (comment) for some thoughts. We've also been wrestling with some of these ideas for Windows Terminal as well. If you think one of your modules might be appropriate, maybe you or I could open an issue and discuss more there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants