Skip to content

/moby

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

Swap width/height in GetWinsize and monitorTtySize #12127

Merged
merged 1 commit into from Apr 6, 2015
+5 −5

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
  1.6.0
5 participants
@ahmetb @jessfraz @icecrime @jhowardmsft @GordonTheTurtle
@ahmetb
Copy link
Contributor

commented Apr 6, 2015

Fixing code lost in translation

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
Emergency cc: @icecrime @jfrazelle @calavera @brendandixon
@ahmetb
Swap width/height in GetWinsize and monitorTtySize 
Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
Loading status checks…
6e44246

@GordonTheTurtle GordonTheTurtle added the status/0-triage label Apr 6, 2015

@jessfraz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

you rule
@jessfraz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

omg its beautiful LGTM

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

@jessfraz jessfraz added status/2-code-review and removed status/0-triage labels Apr 6, 2015

@jessfraz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

ping @icecrime @tiborvass
@icecrime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

Testing!
@jessfraz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

I tried ls, vim , top and now im trying nyancat in conemu
@jessfraz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

btw i put the binary here if you wanna try http://jesss.s3.amazonaws.com/tmp/docker-1.5.0-dev.exe
@jessfraz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

https://s3.j3ss.co/screenshots/2015-04-06-15-09-38_2560x2520.png

docker run --rm -it supertest2014/nyan
pkg/term/term_windows.go
Width: uint16(info.Window.Bottom - info.Window.Top + 1),
Height: uint16(info.Window.Right - info.Window.Left + 1),
Width: uint16(info.Window.Right - info.Window.Left + 1),
Height: uint16(info.Window.Bottom - info.Window.Top + 1),

This comment has been minimized.

Sign in to view
Copy link
@icecrime

icecrime Apr 6, 2015

Contributor

I think those + 1 might be wrong: if I run an interactive session and sleep on the a key, it wraps one character "too late", and gives me empty lines. Behavior is correct without the +1.

This comment has been minimized.

Sign in to view
Copy link
@ahmetb

ahmetb Apr 6, 2015

Author Contributor

cc: @brendandixon @jhowardmsft about this off by one

This comment has been minimized.

Sign in to view
Copy link
@jhowardmsft

jhowardmsft Apr 7, 2015

Contributor

That wrapping should be solved by microsoft@7c83ec1 which @brendandixon or @ahmetalpbalkan where going to open a PR for.
@icecrime

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 6, 2015

Question regarding the + 1 offset, otherwise LGTM 👍

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

Jessie Frazelle
Merge pull request #12127 from ahmetalpbalkan/win-cli/termsize-fix 
Swap width/height in GetWinsize and monitorTtySize
9ede3f9

@jessfraz jessfraz merged commit 9ede3f9 into moby:master Apr 6, 2015
2 checks passed

2 checks passed

janky Jenkins build Docker-PRs 5109 has succeeded
Details
windows Jenkins build Windows-PRs 2074 has succeeded
Details

@ahmetb ahmetb deleted the ahmetb:win-cli/termsize-fix branch Apr 6, 2015

@icecrime icecrime referenced this pull request Apr 7, 2015

Closed

Windows CLI - Off by one terminal size #12147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.