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 crash and rendering where large content is pasted to console on non-Windows #691

Closed
wants to merge 2 commits into from

Conversation

SteveL-MSFT
Copy link
Member

Several issues:

  1. When large content is pasted to the console window on Windows, the buffer scrolls and PSReadLine now has a negative value for the cursor. Exception is raised when trying to set the cursor location to a negative y value. Fix is to check for this and set y to 0 (top).
  2. When the buffer scrolls on paste, the re-rendering gets confused as it didn't track the scrolling appropriately. Fix is to skip drawing content that is off screen.
  3. If the last line drawn to the screen is at the bottom of the screen buffer and the width of the output is exactly the window width, scroll doesn't happen until the next character is drawn; however, PSReadLine expected it to scroll so it's off by one. Fix here is to catch this and reset position back one line.

Fix #663

@lzybkr
Copy link
Member

lzybkr commented May 30, 2018

I think the changes are fine, I'll try them out for a bit before merging.

Are you also going to fix MenuComplete? See this comment.

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented May 30, 2018

@lzybkr that's a separate issue (able to repro with a large directory listing with my changes). I'll look at it in between other work as a separate PR.

@lzybkr
Copy link
Member

lzybkr commented Jun 4, 2018

I'm on the fence with this fix. I'm not sure _initialY should be allowed to be negative, I do see other code that will misbehave under those conditions, e.g. ClearScreen.

Rendering is also still broken if you scroll up, e.g. press Home twice, you won't see the first line.

I think I've known about this general problem for so long that I'd forgotten about it, so it seems to come up rarely.

Is it now more important in some scenario I wasn't aware of? If not, I might be OK with the exception until the scenario is fixed completely. Of course at that point, we almost have a real text editor.

@SteveL-MSFT
Copy link
Member Author

@lzybkr it comes up easily on non-Windows when pasting a large script

@lzybkr
Copy link
Member

lzybkr commented Jun 5, 2018

It happens easily on Windows as well, but you need a small window.

For my education and to better understand an appropriate fix - when are small console windows normally hit on non-Windows? I know that with no gui the window is small, but where else?

@SteveL-MSFT
Copy link
Member Author

The default terminal size on Ubuntu is 80x24.

@lzybkr
Copy link
Member

lzybkr commented Jun 5, 2018

Maybe it's reasonable to disallow a command line that exceeds the buffer height, at least until you can actually edit it properly.

@SteveL-MSFT
Copy link
Member Author

@lzybkr I think we should take this fix as even if you can't edit the buffer, it does show the syntax highlighting correctly making it easier to read. If you think we should explicitly disallow editing, I think we should have that as a separate issue. This PR does fix the current issue of an unhandled exception.

@lzybkr
Copy link
Member

lzybkr commented Jun 26, 2018

I'm still on the fence about allowing _initialY to be negative. Before this PR, there was no code that expected that, this change basically takes the position that it is an expected condition, and we need to carefully review all other uses.

@SteveL-MSFT
Copy link
Member Author

There aren't too many references to _initialY in the code. The menu code relies on _initialY and is probably the related crash for tab-complete for a large result set which I'll look into after we resolve this one. The alternative requires using a different variable to track how far off the screen we are.

@lzybkr
Copy link
Member

lzybkr commented Jun 26, 2018

If we decide a negative _initialY is reasonable, the field declaration and where we change it should have comments saying as much and why.

@lzybkr
Copy link
Member

lzybkr commented Jun 26, 2018

It would be good to add a test as well. The current console proxy does use a fixed buffer, but it might need some improvements to handle this scenario.

@daxian-dbw
Copy link
Member

#979 has been merged as a more general fix to the ArgumentOutOfRangeException issue, so close this PR.

@daxian-dbw daxian-dbw closed this Sep 12, 2019
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.

Exception on non-Windows w/ command line w/ more lines than visible console buffer
3 participants