-
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
Don't explode when we tear out the last tab of the window #15259
Don't explode when we tear out the last tab of the window #15259
Conversation
If you were really fast, and closed one window, and then tried to drag the only tab out of the last remaining window, the Terminal could explode. It'd attempt to restore the previous window state, and explode. Easy way to stop this (also, be more robust): just don't attempt to restore windows during tear-out. That's obvious. This is a part of #14957
@@ -220,7 +220,8 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window | |||
// seemed to reorder bits of init so much that everything broke. So | |||
// we'll leave it here. | |||
const auto numPeasants = _windowManager.GetNumberOfPeasants(); | |||
if (numPeasants == 1) | |||
// Don't attempt to session restore if we're just making a window for tear-out | |||
if (!startedForContent && numPeasants == 1) |
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.
BTW I have to mention that this code is kind of... weird? I'm honestly not sure how to describe this feeling. Basically, I'm confused why session restore related code runs inside _HandleCommandlineArgs
why it's related to numPeasants
. And do we ever have more than 1 peasant nowadays? Not really asking, I'm just a lil puzzled about this code. I suppose that's how it feels like when reading AtlasEngine code for you all... 😄
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.
It's super weird that's for sure. All of session restore sorta is. It probably could live in a helper just after _HandleCommandlineArgs
. Might just make a PR for that for s&g's
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.
we may want to take a todo to really fine-toothed-comb this.
…lode-when-last-tears-out
@@ -220,7 +220,8 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window | |||
// seemed to reorder bits of init so much that everything broke. So | |||
// we'll leave it here. | |||
const auto numPeasants = _windowManager.GetNumberOfPeasants(); | |||
if (numPeasants == 1) | |||
// Don't attempt to session restore if we're just making a window for tear-out | |||
if (!startedForContent && numPeasants == 1) |
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.
we may want to take a todo to really fine-toothed-comb this.
If you were really fast, and closed one window, and then tried to drag the only tab out of the last remaining window, the Terminal could explode. It'd attempt to restore the previous window state, and explode.
Easy way to stop this (also, be more robust): just don't attempt to restore windows during tear-out. That's obvious.
This is a part of #14957