-
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
Use WinRT VirtualKeyModifiers instead of a custom enum #10603
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.
This PR removes the need for ConvertVKModifiers
. Nice. 👍
If you want to you could write
using VirtualKeyModifiers = winrt::Windows::System::VirtualKeyModifiers;
here and there, to shorten the code lines, but I'm fine with merging as it is.
So, our initial design for the terminal settings model was intended to avoid a dependency on the Windows.UI namespace-- it is a big namespace that we may not want consumers to depend on. @zadjii-msft and @carlos-zamora may have opinions. |
(At the same time, thanks for the contribution! It's good to have you here :)) |
@DHowett Isn't |
Windows.UI is used in several places in the terminal settings model. For example, in |
Shows what I get for triaging pull requests on my phone and not reading their bodies. Thanks all! This looks fine to me, but I'd prefer @zadjii-msft sign off. @msftbot make sure @zadjii-msft signs off |
@msftbot 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.
Just a minor suggestion.
I'll probably run into a few conflicts on my end with adding a key chord listener to the actions page in SUI, but that's my problem, not yours haha.
Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
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: |
Replaces
KeyModifiers
with the pretty much equivalentVirtualKeyModifiers
enum in winrt.After doing this I noticed #10593 which changes the KeyChords a lot, but
it seems these PRs are still compatible
The issue also mentions replacing Vkey with
Windows::System::VirtualKey
, but I chose not to because that enum onlyincludes a subset of the keys terminal supports here (no VK_OEM_* keys)
Validation Steps Performed
Changed key bind in config, and confirmed it still works after
restarting terminal
Closes #877