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

Hide the commandline on a resize to prevent a crash when snapping the window #5620

Merged
3 commits merged into from
Apr 29, 2020

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 28, 2020

Hide any commandline (cooked read) we have before we begin a resize, and
show it again after the resize.

References

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

  • ran tests
  • checked resizing, snapping in conhost with a cooked read
  • checked resizing, snapping in the Terminal with a cooked read

@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty labels Apr 28, 2020
Comment on lines +592 to +600
// 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();
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Member

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.

@ghost ghost added Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Severity-Crash Crashes are real bad news. labels Apr 29, 2020
@DHowett-MSFT DHowett-MSFT added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Severity-Crash Crashes are real bad news. labels Apr 29, 2020
@ghost
Copy link

ghost commented Apr 29, 2020

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 10fa310 into master Apr 29, 2020
@ghost ghost deleted the dev/migrie/b/1856-for-real branch April 29, 2020 23:48
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snapping to smaller screen with a COOKED_READ crashes conpty
4 participants