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

Use alternate screen buffer unconditionally #6435

Closed
wants to merge 1 commit into from

Conversation

heaths
Copy link
Contributor

@heaths heaths commented Oct 13, 2022

Fixes regression from #6356. Previous assumption before #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.

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.
@heaths heaths requested a review from a team as a code owner October 13, 2022 15:05
@heaths heaths requested review from vilmibm and removed request for a team October 13, 2022 15:05
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Oct 13, 2022
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Oct 13, 2022
@heaths
Copy link
Contributor Author

heaths commented Oct 13, 2022

@mislav @samcoe this effectively restore the old assumption before alternate screen buffers that VT100 cursor movements and screen clearing was supported, while remaining separate from color support: https://github.com/cli/cli/pull/5681/files#diff-4ca7c158ba1609a9968f3f5bd80a5138fd242ed3b07882e12774ff0ac3188fcfL215-L223

At the very least, it's no worse than before.

A longer-term solution might be to, on Windows, check if virtual terminal processing is enabled, and on other platforms:

  1. For 256- or truecolor detection, you can write a sequence like '\e[48:2:1:2:3m\eP$qm\e\\' (for truecolor) to the console and read it back to see if the color matches: https://github.com/termstandard/colors#querying-the-terminal
  2. For cursor movement, alternate screen buffers, etc., if you wanted to lump them together (should be fairly safe) you could try moving the cursor and querying for the current position, then reset it.

All this would, of course, only be done if stdout is TTY.

@mislav
Copy link
Contributor

mislav commented Oct 13, 2022

@heaths Good catch and thank you! But, I was already starting to address this in https://github.com/cli/cli/pull/6421/files#diff-f27e472205191c606d87c322264c60b4e47e333d45866f76f5a822e35b22a3baR8

@mislav mislav closed this Oct 13, 2022
The GitHub CLI automation moved this from Needs review 🤔 to Done 💤 Oct 13, 2022
@heaths heaths deleted the separate-from-color-support branch October 13, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

None yet

3 participants