Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce vk() and sc() key chord specifiers #10666

Merged
15 commits merged into from
Jul 20, 2021
Merged

Introduce vk() and sc() key chord specifiers #10666

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0bb6e34
Introduce vk() and sc() key chord specifiers
lhecker Jul 15, 2021
561d40a
Improve til::from_wchars
lhecker Jul 15, 2021
7297b41
Address reviewer comments
lhecker Jul 15, 2021
b8d6f68
Fix unit tests
lhecker Jul 15, 2021
9fb176b
Fix formatting
lhecker Jul 15, 2021
877acab
Fix spelling
lhecker Jul 15, 2021
f901604
Fix audit checks
lhecker Jul 15, 2021
098aff1
Update profiles.schema.json
lhecker Jul 15, 2021
bb6e64d
Fix x86 build
lhecker Jul 15, 2021
253072f
Improve documentation and til::from_wchars
lhecker Jul 16, 2021
c3cc1fc
Add some basic KeyChordSerialization unit tests
lhecker Jul 19, 2021
a38e415
Remove unnecessary comment
lhecker Jul 19, 2021
9c54d77
Merge remote-tracking branch 'origin/main' into dev/lhecker/7539-key-…
lhecker Jul 19, 2021
d1bef1e
Fix build error after merging main
lhecker Jul 19, 2021
a8b29ed
Address reviewer comments
lhecker Jul 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/actions/spelling/allow/apis.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ IObject
iosfwd
IPackage
IPeasant
isspace
IStorage
istream
IStringable
Expand Down Expand Up @@ -133,6 +134,7 @@ STDCPP
STDMETHOD
strchr
streambuf
strtoul
Stubless
Subheader
Subpage
Expand Down
8 changes: 4 additions & 4 deletions doc/cascadia/profiles.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
"title": "Microsoft's Windows Terminal Settings Profile Schema",
"definitions": {
"KeyChordSegment": {
"pattern": "^(?<modifier>(?<mod1>ctrl|alt|shift|win)(?:\\+(?<mod2>ctrl|alt|shift|win)(?<!\\k<mod1>))?(?:\\+(?<mod3>ctrl|alt|shift|win)(?<!\\k<mod1>|\\k<mod2>))?(?:\\+(?<mod4>ctrl|alt|shift|win)(?<!\\k<mod1>|\\k<mod2>|\\k<mod3>))?\\+)?(?<key>[^\\s+]|app|menu|backspace|tab|enter|esc|escape|space|pgup|pageup|pgdn|pagedown|end|home|left|up|right|down|insert|delete|(?<!shift.+)(?:numpad_?[0-9]|numpad_(?:period|decimal))|numpad_(?:multiply|plus|add|minus|subtract|divide)|f[1-9]|f1[0-9]|f2[0-4]|plus)$",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an actually correct regex for all the things this accepts, I won't be shocked, but I don't think it's valuable to try and optimize this more past this point IMO.

Copy link
Member Author

@lhecker lhecker Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I made it less correct than it was, because the previous regexp was really really hard to understand. 😅

"pattern": "^(?:(?:ctrl|alt|shift|win)\\+)*(?:app|backspace|comma|delete|down|end|enter|esc|escape|home|insert|left|menu|minus|pagedown|pageup|period|pgdn|pgup|plus|right|space|tab|up|f(?:1\\d?|2[0-4]?|[3-9])|numpad\\d|numpad_(?:\\d|add|decimal|divide|minus|multiply|period|plus|subtract)|(?:vk|sc)\\((?:[1-9]|1?\\d{2}|2[0-4]\\d|25[0-5])\\)|[^\\s+])(?:\\+(?:ctrl|alt|shift|win))*$",
"type": "string",
"description": "The string should fit the format \"[ctrl+][alt+][shift+][win+]<keyName>\", where each modifier is optional, separated by + symbols, and keyName is either one of the names listed in the table below, or any single key character. The string should be written in full lowercase.\napp, menu\tMENU key\nbackspace\tBACKSPACE key\ntab\tTAB key\nenter\tENTER key\nesc, escape\tESC key\nspace\tSPACEBAR\npgup, pageup\tPAGE UP key\npgdn, pagedown\tPAGE DOWN key\nend\tEND key\nhome\tHOME key\nleft\tLEFT ARROW key\nup\tUP ARROW key\nright\tRIGHT ARROW key\ndown\tDOWN ARROW key\ninsert\tINS key\ndelete\tDEL key\nnumpad_0-numpad_9, numpad0-numpad9\tNumeric keypad keys 0 to 9. Can't be combined with the shift modifier.\nnumpad_multiply\tNumeric keypad MULTIPLY key (*)\nnumpad_plus, numpad_add\tNumeric keypad ADD key (+)\nnumpad_minus, numpad_subtract\tNumeric keypad SUBTRACT key (-)\nnumpad_period, numpad_decimal\tNumeric keypad DECIMAL key (.). Can't be combined with the shift modifier.\nnumpad_divide\tNumeric keypad DIVIDE key (/)\nf1-f24\tF1 to F24 function keys\nplus\tADD key (+)"
"description": "The string should fit the format \"[ctrl+][alt+][shift+][win+]<KeyName>\", where each modifier is optional. KeyName is either any single key character, an explicit virtual key or scan code in the form vk(nnn) and sc(nnn) respectively, or one of the special names listed at https://docs.microsoft.com/en-us/windows/terminal/customize-settings/actions#accepted-modifiers-and-keys"
},
"Color": {
"default": "#",
"pattern": "^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$",
"pattern": "^#[A-Fa-f0-9]{3}(?:[A-Fa-f0-9]{3})?$",
"type": "string",
"format": "color"
},
Expand Down Expand Up @@ -596,7 +596,7 @@
"defaultsFile",
"allFiles",
"settingsUI"

]
}
}
Expand Down
18 changes: 9 additions & 9 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1971,9 +1971,9 @@ namespace SettingsModelLocalTests
auto settings = implementation::CascadiaSettings::FromJson(settingsObject);

VERIFY_ARE_EQUAL(3u, settings->_globals->_actionMap->_KeyMap.size());
VERIFY_IS_NULL(settings->_globals->_actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('a') }));
VERIFY_IS_NULL(settings->_globals->_actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('b') }));
VERIFY_IS_NULL(settings->_globals->_actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('c') }));
VERIFY_IS_NULL(settings->_globals->_actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('A'), 0 }));
VERIFY_IS_NULL(settings->_globals->_actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('B'), 0 }));
VERIFY_IS_NULL(settings->_globals->_actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('C'), 0 }));
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

for (const auto& warning : settings->_globals->_keybindingsWarnings)
{
Expand Down Expand Up @@ -2124,7 +2124,7 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(1u, nameMap.Size());

{
KeyChord kc{ true, false, false, static_cast<int32_t>('A') };
KeyChord kc{ true, false, false, false, static_cast<int32_t>('A'), 0 };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
Expand All @@ -2141,7 +2141,7 @@ namespace SettingsModelLocalTests
Log::Comment(L"Note that we're skipping ctrl+B, since that doesn't have `keys` set.");

{
KeyChord kc{ true, false, false, static_cast<int32_t>('C') };
KeyChord kc{ true, false, false, false, static_cast<int32_t>('C'), 0 };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
Expand All @@ -2155,7 +2155,7 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('D') };
KeyChord kc{ true, false, false, false, static_cast<int32_t>('D'), 0 };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
Expand All @@ -2169,7 +2169,7 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('E') };
KeyChord kc{ true, false, false, false, static_cast<int32_t>('E'), 0 };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
Expand All @@ -2183,7 +2183,7 @@ namespace SettingsModelLocalTests
VERIFY_IS_TRUE(realArgs.TerminalArgs().Profile().empty());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('F') };
KeyChord kc{ true, false, false, false, static_cast<int32_t>('F'), 0 };
auto actionAndArgs = ::TestUtils::GetActionAndArgs(*actionMap, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
Expand Down Expand Up @@ -2841,7 +2841,7 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(0u, settings->_warnings.Size());
VERIFY_ARE_EQUAL(1u, nameMap.Size());

const KeyChord expectedKeyChord{ true, false, true, static_cast<int>('W') };
const KeyChord expectedKeyChord{ true, false, true, false, static_cast<int>('W'), 0 };
{
// Verify NameMap returns correct value
const auto& cmd{ nameMap.TryLookup(L"foo") };
Expand Down
Loading