-
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
Teach tab to focus terminal after rename #9162
Conversation
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.
Love it! Awesome work!
if (const auto control{ tab->GetActiveTerminalControl() }) | ||
{ | ||
control.Focus(FocusState::Programmatic); | ||
} |
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.
Nit: should we just do tab->Focus(FocusState::Programmatic)
to keep it consistent with the rest of the file?
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.
Love it! Thanks! It is not just consistency: I can use TabBase with it, meaning that it will also make SUI tab to well-behave (if it becomes active upon dismiss of the renamer).
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.
You know, we've got a mess of focus issues, but I don't think this makes it worse. I think I was hoping that there was some sort of holistic "whenever the tab gets focus, for any reason, toss the focus back to the content" solution to our focus issues. This is more of a specific solution to an individual case of focus being weird. It might just be easier to solve these issues on-off as they come up.
In the future, we might want to pull the TabRenamerDeactivated
lambda into it's own method in TerminalPage, so that other scenarios might be able to leverage this as well.
Hello @carlos-zamora! 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 (
|
I think the CI is stuck. Can someone kick it? 😊 |
## Summary of the Pull Request After rename ends (either by enter or escape) the rename box gets collapsed and the focus moves to the next tab stop in the TabView (e.g., new tab button). To overcome this: * Added RenameEnded event to TabHeaderControl * Forwarded it as RenamerDeactivated from TerminalTab * Registered in the TerminalPage to focus the active control upon RenamerDeactivated if focus didn't move to tab menu. This means, no matter how you close the renamer, the current terminal gains the focus. ## PR Checklist * [x] Closes #9160 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. (cherry picked from commit 00d1dc9)
🎉 Handy links: |
🎉 Handy links: |
Summary of the Pull Request
After rename ends (either by enter or escape) the rename box
gets collapsed and the focus moves to the next tab stop
in the TabView (e.g., new tab button).
To overcome this:
RenamerDeactivated if focus didn't move to tab menu.
This means, no matter how you close the renamer,
the current terminal gains the focus.
PR Checklist