Pass _luminoEvent argument when executing commands via keybinding

[andrewfulton9]

Necessary for jupyterlab issue #15046 .

relevant discussion captured in #570.

Simply appends an _luminoEvent argument to args in _executeKeyBinding.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[krassowski]

What would be a good place(s) to document this new argument?

[andrewfulton9]

That's a good question. It looks like the Lumino docs aren't much more than the API reference, but the function that is directly changed here is private and isn't in the docs. I could add something to the docstring for IKeyBindingOptions.args and maybe ICommandOptions.isEnabled and ICommandOptions.execute referencing the added argument in the case that the command is called from a keybinding. How does that sound?

[fcollonval]

Thanks @andrewfulton9

To move further in the direction of passing the event information.

Should we set isKeybinding with the binding.keys instead of a boolean?

[krassowski]

Great question. For context we discussed exposing a subset of the event itself for menus, for an unrelated reason: jupyterlab/jupyterlab#14173 (comment)

I imagine we could go in the direction of having a ILuminoEvent in which is (a) fully JSON-serializable, (b) includes context specific to Lumino applications (higher level concepts as type: "menu" or type: "keybinding").

[andrewfulton9]

@fcollonval, do you mean something like let newArgs = { ...args, isKeybinding: binding.keys };? That would then allow for more customization in downstream actions. Am I understanding that correctly?

[krassowski]

I was personally thinking:

newArgs = { ...args, event: { type: "keybinding", keys: binding.keys } }

or if we want to "hide" it:

newArgs = { ...args, _luminoEvent: { type: "keybinding", keys: binding.keys } }

or something in between. Thoughts?

[andrewfulton9]

That seems reasonable to me. Would it always be type and keys for the _luminoEvent? Would you just keep keys blank if the event type was menu?

[andrewfulton9]

It looks like ReadonlyPartialJSONObject is readonly [key: string], so I don't think I can do that without changing ReadonlyPartialJsonObject or updating CommandRegistry.execute to include another args type

[krassowski]

I do not see a problem here. Can you expand? In general the proposed event would be a union of types following a base interface for shared fields like "type". Such an interface/type w should indeed have read-only fields. It should conform to read-only JSON type.

[andrewfulton9]

I may be misunderstanding an error. when I try to access newArgs._luminoEvent?.type I am getting this error:

Property 'type' does not exist on type 'string | number | boolean | ReadonlyPartialJSONObject | ReadonlyPartialJSONArray'.
  Property 'type' does not exist on type 'string'.ts(2339)

Any idea why it thinks _luminoEvent is a string?

[krassowski]

You will need to add a cast (with type guard). It should work since I've if the shower values is ReadonlyPartialJSONObject.

[fcollonval]

Thanks @andrewfulton9 it looks really nice. I like the idea of prefixing by _ added arguments.

Would you mind extending the following test to check it receives the new attribute with the correct value:

lumino/packages/commands/tests/src/index.spec.ts

Line 634 in 42b30f7

it('should add key bindings to the registry', () => {

[andrewfulton9]

Thanks @andrewfulton9 it looks really nice. I like the idea of prefixing by _ added arguments.

Would you mind extending the following test to check it receives the new attribute with the correct value:

lumino/packages/commands/tests/src/index.spec.ts

Line 634 in 42b30f7

it('should add key bindings to the registry', () => {

Thanks! I just added the test.

[fcollonval]

Thanks a lot @andrewfulton9

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️