microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.66k stars 8.33k forks source link

Documentation: Accepted Key Chord Serialization Values #3856

Closed carlos-zamora closed 4 years ago

carlos-zamora commented 4 years ago

Could you please add a short description for the key chord definitions as well? It's not too clear what a user is supposed to write for all the special keys (e.g. there's numpad_5, there's esc and not escape, etc.) or whether we are supposed to separate parts with + or -. Also, the JSON schema helps a bit but this information should really be documented!

The parsing code can be found in: src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

Here's my take on the doc for all the special keys, adapted from the code with descriptions sourced from the dev center:

The string should fit the format "[ctrl+][alt+][shift+]<keyName>", where each modifier is optional, separated by + symbols, and keyName is either one of the names listed in the table below, or is one of 0-9a-zA-Z. The string should be written in full lowercase.

keyName String Description
backspace BACKSPACE key
tab TAB key
enter ENTER key
esc ESC key
space SPACEBAR
pgup PAGE UP key
pgdn PAGE DOWN key
end END key
home HOME key
left LEFT ARROW key
up UP ARROW key
right RIGHT ARROW key
down DOWN ARROW key
insert INS key
delete DEL key
numpad_0 to numpad_9 Numeric keypad keys (0 to 9)
numpad_multiply Multiply key
numpad_plus Add key
numpad_minus Subtract key
numpad_period Decimal key
numpad_divide Divide key
f1 to f24 F1-F24 keys
plus For any country/region, the '+' key

Note: The last plus key description seemed weird. From my testing, it's the =/+ key as usual. But the code name for it is VK_OEM_PLUS and shares the same description as other OEM keys. They are apparently called OEM because they provide compatibility between different keyboard layouts. In this case, it's probably here to prevent issues with keybindings such as ctrl++.

Originally posted by @nahiyan8 in https://github.com/microsoft/terminal/pull/3809#issuecomment-560961405

davidwin commented 4 years ago

Right now the JSON schema doesn't enumerate the allowed chords (which I have a suggestion and solution for in #2967). Now, a regexp can't help with showing the possible alternatives very clearly, like with an enum, but the description field could list all the allowed values.

Hmm, that issue has just been sitting there, is this when I file a PR for it?

davidwin commented 4 years ago

Just to try out the idea, I transferred the table above to the description field to see what it looks like, combined with an updated schema that does a more correct validation. This is the resulting popup in VSCode: image Unfortunately, \t doesn't align columns, and the font is variable width, so the "table" looks kind of messy. A question that arose is that keyName is documented to be either one of the items in the table or one of 0-9a-zA-Z. This isn't entirely true, since nearly any key on the keyboard seems to work, and the regex will accept any single character but whitespace or +. This is probably too wide, but what is the actual limitation here? The code in KeyChordSerialization.cpp uses VkKeyScanW, which is very much "black box". Anyway, I used the vague term "any single key character", whatever that means.

This is the mod I made to the keys property in the schema:

{
  "description": "Defines the key combinations used to call the command.",
  "items": {
    "pattern": "^(?<modifier>(ctrl|alt|shift)(?:\\+(ctrl|alt|shift)(?<!\\2))?(?:\\+(ctrl|alt|shift)(?<!\\2|\\3))?\\+)?(?<key>[^\\s+]|backspace|tab|enter|esc|space|pgup|pgdn|end|home|left|up|right|down|insert|delete|(?<!shift.+)numpad_(?:[0-9]|period)|numpad_(?:multiply|plus|minus|divide)|f[1-9]|f1[0-9]|f2[0-4]|plus)$",
    "type": "string",
    "description": "The string should fit the format \"[ctrl+][alt+][shift+]<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.\nbackspace\tBACKSPACE key\ntab\tTAB key\nenter\tENTER key\nesc\tESC key\nspace\tSPACEBAR\npgup\tPAGE UP key\npgdn\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 to numpad_9\tNumeric keypad keys (0 to 9). Can't be combined with the shift modifier.\nnumpad_multiply\tMultiply key\nnumpad_plus\tAdd key\nnumpad_minus\tSubtract key\nnumpad_period\tDecimal key. Can't be combined with the shift modified.\nnumpad_divide\tDivide key\nf1 to f24\tF1-F24 keys\nplus\tFor any country/region, the '+' key"
  },
  "minItems": 1,
  "type": "array"
}

No, this doesn't exactly play to the strengths of either JSON or whitespace sensitive regex, but it gets the job done. :)

carlos-zamora commented 4 years ago

Right now the JSON schema doesn't enumerate the allowed chords (which I have a suggestion and solution for in #2967). Now, a regexp can't help with showing the possible alternatives very clearly, like with an enum, but the description field could list all the allowed values.

Hmm, that issue has just been sitting there, is this when I file a PR for it?

Yeah. #2967 isn't scheduled for this month's milestone. Feel free to submit a PR. Updating that issue right now with the help-wanted tag (and doing the same with this one).

carlos-zamora commented 4 years ago

Personally, I think that looks waaaaaay better than seeing a long/complicated regex. I think @zadjii-msft can speak to VkKeyScanW in KeyChordSerialization.cpp