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

Windows console fixes #12064

Merged
merged 1 commit into from Apr 6, 2015

Conversation

Projects
None yet
9 participants
@brendandixon
Contributor

brendandixon commented Apr 3, 2015

Corrected integer size passed to Windows
Corrected DisableEcho / SetRawTerminal to not modify state
Cleaned up and made routines more idiomatic
Corrected raw mode state bits
Removed duplicate IsTerminal
Corrected off-by-one error
Minor idiomatic change

Signed-off-by: Brendan Dixon brendand@microsoft.com

Brendan Dixon
Windows console fixes
Corrected integer size passed to Windows
Corrected DisableEcho / SetRawTerminal to not modify state
Cleaned up and made routines more idiomatic
Corrected raw mode state bits
Removed duplicate IsTerminal
Corrected off-by-one error
Minor idiomatic change

Signed-off-by: Brendan Dixon <brendand@microsoft.com>
@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Apr 3, 2015

@icecrime @tiborvass @jfrazelle Folks meet @brendandixon, our new ANSI emulation maintainer.

This change reduces panics on terminal-attached sessions a great deal (for example I haven't got one today 😄) as far as I can tell and fixes several other minor displaying problems. I really appreciate if you folks can review and try out.

I think we should take this for 1.6. (I know, this is short notice but this observably improves the quality).

@jessfraz jessfraz added this to the 1.6.0 milestone Apr 3, 2015

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Apr 3, 2015

does this fix the panics?

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Apr 3, 2015

@jfrazelle as far as I can tell, yeah "many of them", however we do not have a systematic way to test and we've seen that not all repro steps causes panics in everybody's computer. I remember @icecrime/@tiborvass were observing certain panics with repro steps. That's why I'd love if you guys try it out.

As I said, this visibly reduces panics on terminal attach.

@icecrime

This comment has been minimized.

Contributor

icecrime commented Apr 3, 2015

My hat off to you guys: it does fix the reproducible panics I encountered. 😮 LGTM 👍

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Apr 3, 2015

Hey @unclejack, since you're the buffer master, would you mind taking a peek at the ANSI code (that this PR modifies) sometime? There are bunch of weird things it's doing :(

@brendandixon

This comment has been minimized.

Contributor

brendandixon commented Apr 3, 2015

I will address them. It's going to take a couple/ three days.

On Apr 3, 2015, at 3:59 PM, Tibor Vass notifications@github.com wrote:

Hey @unclejack, since you're the buffer master, would you mind taking a peek at the ANSI code (that this PR modifies) sometime? There are bunch of weird things it's doing :(


Reply to this email directly or view it on GitHub.

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Apr 4, 2015

I can attest that this does not make things worse LGTM

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Apr 4, 2015

also thanks so much for the hustle

@brendandixon

This comment has been minimized.

Contributor

brendandixon commented Apr 4, 2015

Well, that sets a high bar. :)

Brendan
423.922.8798
brendandixon@me.com

On Apr 3, 2015, at 5:57 PM, Jessie Frazelle notifications@github.com wrote:

I can attest that this does not make things worse LGTM


Reply to this email directly or view it on GitHub.

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Apr 4, 2015

Also @stevvooe fixed similar bufio bug in pull.

inFd = file.Fd()
isTerminalIn = IsTerminal(inFd)
}
isTerminalIn = IsConsole(inFd)

This comment has been minimized.

@gossmanster

gossmanster Apr 5, 2015

This line highlights a nit I have with renaming the Terminal stuff in _windows to Console...we just end up with a mix at the two levels of abstraction that makes the code no clearer...arguably more confusing.

This comment has been minimized.

@brendandixon

brendandixon Apr 5, 2015

Contributor

Actually, the reason I renamed them is that the concepts are sufficiently separate to avoid mixed assumptions. Consoles in Windows are only somewhat like Terminals (TTYs) in Unix / Linux. Further, since the code is part of the abstraction layer for Windows, it made most sense to use Windows-centric terms at that level and buried within the Windows files.

I'm not married to these ideas, but they motivated the name.

This comment has been minimized.

@gossmanster
@@ -211,14 +217,14 @@ func getStdHandle(stdhandle int) uintptr {
return uintptr(handle)
}
func StdStreams() (stdIn io.ReadCloser, stdOut io.Writer, stdErr io.Writer) {
func WinConsoleStreams() (stdIn io.ReadCloser, stdOut, stdErr io.Writer) {

This comment has been minimized.

@gossmanster

gossmanster Apr 5, 2015

Total nit, but Win prefix is gratuitous in a _windows file.

This comment has been minimized.

@brendandixon

brendandixon Apr 5, 2015

Contributor

In hindsight, I agree. It had been ill-named "StdStreams" which was wholly incorrect. ConsoleStreams would be better and less verbose. Good call.

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Apr 6, 2015

LGTM

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Apr 6, 2015

I should note that pkg/term/term_windows.go has wrong build-tag, it should be followed by empty line :) But it's unrelated to this change.

@brendandixon

This comment has been minimized.

Contributor

brendandixon commented Apr 6, 2015

We plan other changes to clean-up, simplify, and improve the code. I'll merge these suggestions with those changes.

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Apr 6, 2015

LGTM

jessfraz pushed a commit that referenced this pull request Apr 6, 2015

@jessfraz jessfraz merged commit 37a1658 into moby:master Apr 6, 2015

2 checks passed

janky Jenkins build Docker-PRs 4969 has succeeded
Details
windows Jenkins build Windows-PRs 1928 has succeeded
Details
return winconsole.SetConsoleMode(fd, state.mode)
mode := state.mode
mode &^= winconsole.ENABLE_ECHO_INPUT
mode |= winconsole.ENABLE_PROCESSED_INPUT | winconsole.ENABLE_LINE_INPUT

This comment has been minimized.

@calavera

calavera Apr 6, 2015

Contributor

these options don't mutate state.mode anymore, is this correct @brendandixon, @ahmetalpbalkan? I wonder if this can be a possible cause of the term resizing regression that we just detected.

This comment has been minimized.

@brendandixon

brendandixon Apr 7, 2015

Contributor

The code has had window resize issues. These changes did not introduce them nor expose them. The intention here is to honor the "contract" of DisableEcho as implemented by other terminal code.

@brendandixon brendandixon deleted the brendandixon:win-cli/rawfix branch Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment