-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Hide the commandline on a resize to prevent a crash when snapping the window #5620
Conversation
// GH#1856 - make sure to hide the commandline _before_ we execute | ||
// the resize, and the re-display it after the resize. If we leave | ||
// it displayed, we'll crash during the resize when we try to figure | ||
// out if the bounds of the old commandline fit within the new | ||
// window (it might not). | ||
CommandLine& commandLine = CommandLine::Instance(); | ||
commandLine.Hide(FALSE); | ||
context.SetViewportSize(&NewSize); | ||
commandLine.Show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have an impact on any traditional console interactive scenarios? I'm a bit worried about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh -- resizing goes through here before because it's coming off the pty's resize handle? Does a user-initiated window resize on conhost run through ApiRoutines too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty OK with this because the console host used to do this a ton in the past. It always hid the command line display before manipulating anything and then restored it when it was done. It's kinda sad that we have to do it again, but if it prevents a crash, I'm not that bothered by it.
Hello @DHowett-MSFT! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
Hide any commandline (cooked read) we have before we begin a resize, and
show it again after the resize.
References
COOKED_READ_DATA
ctor takes astring_view
forwchar_t
data? #5618 while I was working on this.PR Checklist
Detailed Description of the Pull Request / Additional comments
Basically, during a resize, we try to restore the viewport position
correctly, and part of that checks where the current commandline ends.
However, when we do that, the commandline's current state still
reflects the old buffer size, so resizing to be smaller can cause us
to throw an exception, when we find that the commandline doesn't fit in
the new viewport cleanly.
By hiding it, then redrawing it, we avoid this problem entirely. We
don't need to perform the check on the old commandline contents (since
they'll be empty), and we'll redraw it just fine for the new buffer size
Validation Steps Performed