-
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
Only attempt to focus if there is a control to focus #11040
Conversation
@zadjii-msft I know you assigned it to yourself, but I'm sure you're busy being a team lead instead of dealing with my bugs. |
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.
This is a nice targeted solution. I somewhat wish we didn't have to check for an active control everywhere, but we'll almost certainly refactor before too long.
Thank you!
@msftbot merge this in 7 minutes and make sure @zadjii-msft signs off |
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
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.
Lol I actually have this in the branch I'm working on today too 😋
@msftbot merge this in 1 minute |
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
Only focus if there is a control to focus (which may be null if e.g. the focused tab is being destroyed) Closes #11037 ## Additional comments I tried to remove the _activePane = nullptr in `TerminalTab::DetachPane` but that actually completely broke being able to focus the control at all making the tab completely unusable. Focus does seem to transfer just fine here with this change. ## Validation Steps Performed Used the command execution to move panes to and from existing panes, including new tabs and destroying tabs.
🎉 Handy links: |
🎉 Handy links: |
Only focus if there is a control to focus (which may be null if e.g. the focused tab is being destroyed)
Closes #11037
Additional comments
I tried to remove the _activePane = nullptr in
TerminalTab::DetachPane
but that actually completely broke being able to focus the control at all making the tab completely unusable. Focus does seem to transfer just fine here with this change.Validation Steps Performed
Used the command execution to move panes to and from existing panes, including new tabs and destroying tabs.