-
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
Allow overriding tab switcher mode on command level #9507
Conversation
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
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.
wow that was simpler than I expected. I like that this is unified with the existing tab switcher mode. If we ever do figure out a way for UI-less MRU switching, then we could presumably add it both to the global enum values and the action enum values at the same time.
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.
qqs from discussion
@@ -234,16 +234,13 @@ namespace Microsoft.Terminal.Settings.Model | |||
NewTerminalArgs TerminalArgs { get; }; | |||
}; | |||
|
|||
unsealed runtimeclass SwitchToAdjacentTabArgs : IActionArgs | |||
[default_interface] runtimeclass PrevTabArgs : IActionArgs |
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 just remembered... if I recall correctly (@zadjii-msft can keep me honest!) there's a way to have one argument type apply to two different actions -- part of why we made the action interface take arguments separately. Since these are just the same action, we might want to keep the root node (SwitchToAdjacentTabArgs) and delete the leaves. If we can!
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.
Oh, but they're separate for the purposes of generating the command name. Aaargh
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 may file followup work to make ActionAndArgs::GenerateName more flexible.
// If the args class supports appending to an existing command name, do that instead
if (auto appendName{ _Args.try_as<ICanAppendCommandName>() }) {
auto otherName = GeneratedActionNames.find(_Action);
return appendName.AppendName(otherName);
}
one of these days
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 can open a follow up for this 😊
Any additional blockers here?
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.
No blockers! It would be very cool if we consolidated all the action types that only differ based on name 😄 in the followup of course
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.
Thanks!
Hello @DHowett! 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 (
|
🎉 Handy links: |
Summary of the Pull Request
Currently, when the MRU is enabled we lose the keybinding allowing us to
go forward/backward (aka right/left in LTR) in the tab view.
To fix that, this PR introduces "tabSwitcherMode" optional parameter to
the prevTab / nextTab commands.
If it is not provided the global setting will be used.
So if you want to go to adjacent tabs, even if MRU is enabled on the
system level you can use:
or even
if you don't want tab switcher to show up
PR Checklist