Mac-only keyboard shortcut

[jasongrout]

I'd like to create a keybinding that only works on macs. Intuitively, I might imagine something like

           {
               "command": "notebook:create-console",
               "selector": ".jp-Notebook:focus",
               "macKeys": ["Cmd Shift ."]
           }

but I think that won't work in the current code (we assume keys is an array throughout the code, and this would make it undefined on non-Mac platforms). I could do this:

           {
               "command": "notebook:create-console",
               "selector": ".jp-Notebook:focus",
               "keys": [],
               "macKeys": ["Cmd Shift ."]
           }

but that assumes that the matching function at

phosphor/packages/commands/src/index.ts

Lines 1356 to 1369 in 7a87d35

	function matchSequence(bindKeys: ReadonlyArray<string>, userKeys: ReadonlyArray<string>): SequenceMatch {
	if (bindKeys.length < userKeys.length) {
	return SequenceMatch.None;
	}
	for (let i = 0, n = userKeys.length; i < n; ++i) {
	if (bindKeys[i] !== userKeys[i]) {
	return SequenceMatch.None;
	}
	}
	if (bindKeys.length > userKeys.length) {
	return SequenceMatch.Partial;
	}
	return SequenceMatch.Exact;
	}

is never called with userKeys being an empty list. I think that's valid - are we guaranteed that?

Note that I can't do

           {
               "command": "notebook:create-console",
               "selector": ".jp-Notebook:focus",
               "keys": ["Cmd Shift ."]
           }

because this would be translated to "Shift ." on non-mac platforms, which is not intuitive to me. I think the current behavior of ignoring Cmd on non-mac platforms should be changed to invalidating a keybinding if it contains Cmd on a non-mac platform.

[sccolbert]

The platform-specific keys are chosen during the normalization process:

phosphor/packages/commands/src/index.ts

Line 1323 in 7a87d35

keys = options.macKeys || options.keys;

So your second example should work. Note that the keybinding keys become the bindKeys argument in this function. The userKeys are the keys currently pressed by the user. So userKeys will always have .length >= 1 and bindKeys will have .length == 0 for non-Mac platforms:

phosphor/packages/commands/src/index.ts

Line 1357 in 7a87d35

if (bindKeys.length < userKeys.length) {

[sccolbert]

(we assume keys is an array throughout the code, and this would make it undefined on non-Mac platforms)

No, it would make it a compile error if you have strictNullChecks turned on like you should ;-)

[jasongrout]

No, it would make it a compile error if you have strictNullChecks turned on like you should ;-)

Touché

[jasongrout]

So your second example should work.

I realized it's our fault in jlab that it won't work - we insist that keybindings have at least one item in them in our json schema. I've put in a PR fixing this: jupyterlab/jupyterlab#7335

[jasongrout]

I think the current behavior of ignoring Cmd on non-mac platforms should be changed to invalidating a keybinding if it contains Cmd on a non-mac platform.

Chris, I think my questions are answered except the above one. What do you think about invalidating the entire keybinding if it contains Cmd on a non-mac platform. Otherwise you end up having bindings on non-mac platforms that you wouldn't didn't expect, or at least I wouldn't expect.

[vidartf]

No, it would make it a compile error if you have strictNullChecks turned on like you should ;-)

@sccolbert That's a WIP for 2.0. Should we wait for #417, or go ahead with jupyterlab/jupyterlab#7002 ?

[sccolbert]

@vidartf Wait for #417. I didn't realize you were blocked on me. I'll take care of it.

[sccolbert]

@jasongrout By invalidate do you mean "ignore the entire keybinding" or "throw an exception"?

[jasongrout]

@jasongrout By invalidate do you mean "ignore the entire keybinding" or "throw an exception"?

Ignore is probably most convenient to the end-user for me. Then the person registering the binding doesn't have to check to see if they are on a mac. I mean, clearly the keybinding won't be triggered on windows, so they won't even notice it's not there.

[sccolbert]

Ignoring it sounds reasonable. Technically its a behavioral change, not an API change. Could probably be considered a bugfix.