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 color output for non-256 color terminals #6356

Merged
merged 1 commit into from Sep 27, 2022
Merged

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Sep 27, 2022

The function enableVirtualTerminalProcessing must return an error if virtual terminal processing (only applicable on Windows) was not enabled, otherwise we assume that the terminal supports both 256-color and truecolor (since that is true of Windows terminals with virtual terminal processing enabled).

We have been erroneously assuming that all non-Windows terminals are 256-color and sending escape sequences to those that cannot interpret them. This led to some parts of Survey prompts being invisible on old terminals.

Regressed in #5681

The function enableVirtualTerminalProcessing must return an error if
virtual terminal processing (only applicable on Windows) was not
enabled, otherwise we assume that the terminal supports both 256-color
and truecolor.

We have been erroneously assuming that all non-Windows terminals are
256-color and sending escape sequences to those that cannot intepret
them. This led to some parts of Survey prompts being invisible on old
terminals.

Regressed in be4b392
@mislav mislav requested a review from a team as a code owner September 27, 2022 17:54
@mislav mislav requested review from vilmibm and removed request for a team September 27, 2022 17:54
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Sep 27, 2022
The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Sep 27, 2022
@mislav mislav merged commit 0b2348a into trunk Sep 27, 2022
@mislav mislav deleted the fix-non-256-color branch September 27, 2022 18:05
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Sep 27, 2022
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Oct 4, 2022
@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:

EDIT: Yes, this would now print pages of information to *nix as it did before with Windows: https://github.com/cli/cli/pull/5681/files#diff-887a9d62c9cb63f7a0eab6a95bf42464d5bcc5ffb4df181dcb00748b9ac24da7R410

The original assumption was that Windows was late to the party for decent VT sequence support, but was otherwise prevalent in *nix terminals for many, many years. Were there particular terminals where this didn't work that we could test in? Relying on either tput (which some other modules - even some of mattn's - use) or sending a VT sequence and reading the result should be safe.

@heaths
Copy link
Contributor

heaths commented Oct 12, 2022

Thinking about this more, it was already assumed before with the old implementation that VT100 sequences were supported for --watch, so maybe a better fix overall is keep what you have but where it's used to decide if alt screen bufs are supported, do a check on windows && enableVirtualTerminalProcessing, or assume. It would be no worse than before since VT100 sequences were being used even to clear the screen and set the cursor position. It seems very unlikely given how long these VT sequences have been around that one set is supported but not another. @mislav @samcoe thoughts?

I could write up a PR, but would love to test out on whatever *nix terminals this failed to make sure. At the very least, it should be no worse than before.

@heaths
Copy link
Contributor

heaths commented Oct 13, 2022

Yes, the previous code before my change assumed VT support: https://github.com/cli/cli/pull/5681/files#diff-4ca7c158ba1609a9968f3f5bd80a5138fd242ed3b07882e12774ff0ac3188fcfL215-L223. I've got a PR incoming to effectively restore that assumption and separate logic from color support.

heaths added a commit to heaths/cli that referenced this pull request Oct 13, 2022
Fixes regression from cli#6356. Previous assumption before cli#5681 was that VT cursor movement and screen clearing was supported, so this effectively restores that assumption while remaining separate from color support and virtual terminal processing support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

None yet

5 participants