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

Shut down all controls under a tab before we remove it from the list #4337

Merged
1 commit merged into from
Jan 23, 2020

Conversation

DHowett-MSFT
Copy link
Contributor

This commit introduces a new recursive pane shutdown that will give all
controls under a tab a chance to clean up their state before beign
detached from the UI. It also reorders the call to LastTabClosed() so
that the application does not exit before the final connections are
terminated.

It also teaches TSFInputControl how to shut down to avoid a dramatic
platform bug.

Fixes #4159.
Fixes #4336.

PR Checklist

  • CLA signed
  • I've discussed this with core contributors already.

Validation Steps Performed

Validated through manual terminal teardown within and without the debugger, given a crazy number of panes and tabs.

This commit introduces a new recursive pane shutdown that will give all
controls under a tab a chance to clean up their state before beign
detached from the UI. It also reorders the call to LastTabClosed() so
that the application does not exit before the final connections are
terminated.

It also teaches TSFInputControl how to shut down to avoid a dramatic
platform bug.

Fixes #4159.
Fixes #4336.
@DHowett-MSFT DHowett-MSFT added the Severity-Blocking We won't ship a release like this! No-siree. label Jan 22, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v0.9 milestone Jan 22, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally I approve of this, but maybe I'm a little confused now between the difference between Shutdown and Close on the Pane. Maybe add a comment to Shutdown indicating that this method will result in Pane::Close getting called (if that's correct)

@DHowett-MSFT
Copy link
Contributor Author

It will not result in Pane::Close being called -- the pane remains in the UI tree until it's otherwise removed, but all of its controls and connections go inert.

@zadjii-msft
Copy link
Member

It will not result in Pane::Close being called -- the pane remains in the UI tree until it's otherwise removed, but all of its controls and connections go inert.

So if the control cleanly closes though, like the pre-closeOnExit:graceful change, then Close gets called, yea?

@DHowett-MSFT
Copy link
Contributor Author

Yes. If the control is closed and removed from the UI hierarchy (because its leaf pane has been resorbed by its parent), its Close method will be called as usual.

@DHowett-MSFT
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zadjii-msft
Copy link
Member

plz

/azp run

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 23, 2020
@ghost
Copy link

ghost commented Jan 23, 2020

Hello @zadjii-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.

@zadjii-msft
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost ghost merged commit 82f302b into master Jan 23, 2020
@ghost ghost deleted the dev/duhowett/i_ll_show_you_blocking branch January 23, 2020 22:12
DHowett-MSFT pushed a commit that referenced this pull request Jan 24, 2020
…4337)

This commit introduces a new recursive pane shutdown that will give all
controls under a tab a chance to clean up their state before beign
detached from the UI. It also reorders the call to LastTabClosed() so
that the application does not exit before the final connections are
terminated.

It also teaches TSFInputControl how to shut down to avoid a dramatic
platform bug.

Fixes #4159.
Fixes #4336.

## PR Checklist
* [x] CLA signed
* [x] I've discussed this with core contributors already.

## Validation Steps Performed
Validated through manual terminal teardown within and without the debugger, given a crazy number of panes and tabs.

(cherry picked from commit 82f302b)
@ghost
Copy link

ghost commented Feb 13, 2020

🎉Windows Terminal Preview v0.9.433.0 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 Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
4 participants