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

Get window size on more Windows terminals. #6010

Merged
merged 1 commit into from
Sep 12, 2018
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 11, 2018

This is an alternate approach to determining the window size that works on more Windows terminals.

Terminals with accurate width detection: Normal Windows console, cmder, ConEmu, VS Code, Hyper, etc.

mintty-based terminals will always be 60 characters wide. Cygwin in a command console is ok, but
running under x-windows will also always be 60.

Tested on Windows 8 and Windows 10.

Closes #5124.

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Nice!

To confirm, this is bypassing GetStdHandle which means it may run the risk of not doing the right thing when output is piped to a file (as CONOUT I guess is always set?). Have you confirmed that no progress bars show up in that situation? (or should we just prefer GetStdHandle to CONOUT if it works?)

Additionally, this means that if an msys terminal is resized it could produce some garbled output, right? (resized below 79 cols that is). I guess there's no way for us to actually learn the size of the console?

@ehuss
Copy link
Contributor Author

ehuss commented Sep 11, 2018

When Cargo's stderr is connected to a pipe or redirected, atty::is() is false, so this code path doesn't get executed in that case. I tested in a variety of shells, and it seemed to always work correctly when redirected.

If you mean when just stdout is piped (not stderr), then you get a progress bar, but that's true on all platforms.

I initially did both GetStdHandle and CONOUT, but I couldn't find a situation where that improved anything. I could switch it back if you think it would be safer.

You are correct, on mintty terminals you will get weird wrapping when it is less than 79. AFAIK, there is no way to get the correct information. As I mentioned in the issue, it is plumbed inside the guts of cygwin, and there's no sane way to get to it from a non-cygwin binary. I'm unsure how much of an issue that would be. It would be interesting to know how common different shell/terminal combinations are. Unfortunately I fear that "Git Bash" (which is mintty) from "Git for Windows" is probably pretty common, so it is somewhat risky. If its any consolation, git's progress bar wraps funny when the screen is too small (<~65).

@alexcrichton
Copy link
Member

Ah ok cool, makes sense! Given that we check atty::is anyway, that means that in theory if you set a new console for Cargo as GetStdHandle (but one that still looks like a "tty") then Cargo could get wrong information as it opens the original console. That seems vanishingly rare and not something we should worry about.

For MSYS though perhaps we could clamp to 60 or so columns as a conservative estimate? There's not really any real need to use the maximal terminal width other than it can look better

@ehuss
Copy link
Contributor Author

ehuss commented Sep 11, 2018

For MSYS though perhaps we could clamp to 60 or so columns as a conservative estimate?

That sounds like a good idea, i'll try to update this to do that soon.

This is an alternate approach to determining the window size that works on more Windows terminals.

Terminals with accurate width detection: Normal Windows console, cmder, ConEmu, VS Code, Hyper, etc.

mintty-based terminals will always be 60 characters wide. Cygwin in a command console is ok, but
running under x-windows will also always be 60.

Tested on Windows 8 and Windows 10.

Closes rust-lang#5124.
@ehuss
Copy link
Contributor Author

ehuss commented Sep 12, 2018

Updated with a different approach. I was reluctant to copy all the code from atty to detect msys terminals specifically, since it doesn't buy a whole lot.

@alexcrichton
Copy link
Member

@bors: r+

Looks good to me!

@bors
Copy link
Collaborator

bors commented Sep 12, 2018

📌 Commit 5b139d1 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Sep 12, 2018

⌛ Testing commit 5b139d1 with merge c514b94...

bors added a commit that referenced this pull request Sep 12, 2018
Get window size on more Windows terminals.

This is an alternate approach to determining the window size that works on more Windows terminals.

Terminals with accurate width detection: Normal Windows console, cmder, ConEmu, VS Code, Hyper, etc.

mintty-based terminals will always be 60 characters wide. Cygwin in a command console is ok, but
running under x-windows will also always be 60.

Tested on Windows 8 and Windows 10.

Closes #5124.
@bors
Copy link
Collaborator

bors commented Sep 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c514b94 to master...

@bors bors merged commit 5b139d1 into rust-lang:master Sep 12, 2018
@ehuss ehuss added this to the 1.31.0 milestone Feb 6, 2022
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.

Cargo can't determine the terminal width on MSYS terminals
4 participants