-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add the ability to quit all terminal instances #11143
Conversation
- Warn the user before they do so to give a chance to cancel - Percolate a QuitAll event up to the monarch who then directs each peasant to clsoe. - Leave a window layout saving sized hole to add that feature on top
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.
Yep, this is how I'd do it!
@@ -469,4 +469,7 @@ | |||
<data name="MinimizeToTrayCommandKey" xml:space="preserve"> | |||
<value>Minimize current window to tray</value> | |||
</data> | |||
<data name="QuitCommandKey" xml:space="preserve"> |
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.
these are tabs, but I'm gonna be real, I'm the one who usually ends up reformatting this file in much worse ways
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.
Just a few questions before signing off
// - <none> | ||
winrt::fire_and_forget WindowManager::RequestQuitAll() | ||
{ | ||
auto strongThis{ get_strong() }; |
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.
What's the strongThis
for?
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 think I just copied from the above RequestHideTrayIcon
and I assumed it was there for an important reason. It can probably be removed?
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.
Ah, found something on it: #10938 (comment)
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.
As always, thanks for doing this! 😊
man ronpaul.gif is really getting a lot of milage this release |
Ah! Don't forget to update the docs repo! |
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
}; | ||
|
||
_forEachPeasant(callback, onError); |
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.
Just curious, so if all windows get told to quit
at roughly the same time, including the monarch, would it pose any issues if the monarch was the first to quit while all other peasants are in the middle of quitting (maybe if some peasants had more tabs and took longer to quit)?
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 not sure. Since ultimately each of the quit
actions is asynchronous (somewhere in the chain is a fire_and_forget method) it is hard to say. I imagine there might be some fighting for who becomes the monarch but I think that shouldn't matter since the other thread will continue quitting even while that happens. I haven't attempted to prove that behavior, so take what I just said with a grain of salt.
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.
As a test I opened ~100 windows and initiated quit
. All of the windows closed, but there remained a "runtime broker" process which exited a few seconds afterwards. I don't know what the "runtime broker" is or if I should be concerned that it hung around on its own for a little bit.
Edit: doesn't seem like the runtime broker always gets closed immediately, or at least it can stick around for a little while if there were a lot of windows.
Edit Edit: Maybe a bug, but it doesn't seem like it really matters and it is unrelated anyways, when opening a bunch of windows sometimes the process is named DesktopWindowXamlSource
.
Edit Edit Edit: Opening a whole bunch of windows quickly can cause other problems. I managed to get memory corruption? Looks like the monarch died and a new one wasn't made, since quit/rename window/etc all fail to do anything.
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.
@leonMSFT With further testing, I was able to sometimes get it to close every window except one, that presumably became the new monarch, so I'll need to figure out how to fix that.
Hello @zadjii-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 (
|
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References #11083 #11143 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments While testing the save/quit features a number of issues were found that were caused by poor synchronization on the monarch, resulting in various unexpected crashes. Because this uses std collections, and I didn't see any builtin winrt multithreaded containers I went with the somewhat heavy-handed mutex approach. e.g. - #11083 (comment) - #11083 (comment) - #11143 This also makes it so that on quit peasants don't try to become the monarch, and the monarch closes their peasant last to prevent elections from happening. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed Create many windows (hold down ctrl-shift-n) then use the quit action from peasants/the monarch to make sure everything closes properly.
🎉 Handy links: |
Add the ability to quit all terminal instances. Doing this separately from the window layout saving ones to lessen the number of 1k+ line monsters I make y'all review.
References
#11083
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed