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.

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

Work on easy-configuring of the console execute shortcut. #4054

Merged
merged 3 commits into from May 15, 2018

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 27, 2018

Fixes #3643.

This makes the keyboard stroke to execute a console cell more easily configurable. It allows easy toggling/setting of Enter vs Shift-Enter to execute. Other shortcuts may still be set using the Settings Editor if the user desires.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 28, 2018

@afshin If you are concerned about the amount of time searching the keyboard shortcuts during isToggled, it would be reasonably straightforward to cache the results of that calculation, and invalidate the cache if the keyboard shortcuts change.

@ian-r-rose ian-r-rose force-pushed the console_execute_settings branch from 62249fe to e824418 Mar 1, 2018
@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

I thought that with #4070 we were able to keep the same structure that currently exists because the line break behavior is correct. Doesn't that obviate the need for checking which shortcut is set?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 3, 2018

No, this is mostly orthogonal: it allows people to choose whether they want to use Enter or Shift+Enter to execute code.

The fix from #4070 for line breaks works for both keybindings here.

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

Right, sorry, I hadn't re-read the code. This seems like a nice feature. I'm a little uncomfortable with the fact that you're writing a setting to another extension's key.

Do you think maybe it would be better to update the shortcuts extension to have a public method for saving/overwriting a binding? This seems like behavior other extensions may want even after we create a nice shortcut editor UI, so maybe we should offer it programmatically.

What do you think?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 3, 2018

Yes, I agree that it is not ideal to write to the key of a different extension.

We could do what you suggest. Another option would be for the shortcuts-extension to listen to commands.keyBindingChanged(), and handle changes in the keybindings there. Then, when other extensions mess with keybindings they will automatically be serialized to the settings system.

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

@ian-r-rose: That's a reasonable solution as long as we make sure the shortcuts extension handles the case where it's the one adding the binding and ignores those!

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 3, 2018

I think we can put a change guard somewhere to make that work.

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

Yeah, absolutely. I was just pointing it out as a thing to look out for if that's how you want to implement it. I think listening to the keyBindingChanged changed in the shortcuts extension is a more robust solution than what I suggested. Do you want to do that in this PR?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 3, 2018

Sure, I'll do that here.

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

Awesome.
👷

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 3, 2018

Hmm, this is actually a bit trickier than I had hoped. I am not sure how to remove an existing keybinding in an extension without a handle on the DisposableDelegate that belongs to the shortcuts extension. Any ideas around that?

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

I haven't tested this!

But something like this logic should do it, I think.

diff --git a/packages/shortcuts-extension/src/index.ts b/packages/shortcuts-extension/src/index.ts
index f4819054d..0f933f533 100644
--- a/packages/shortcuts-extension/src/index.ts
+++ b/packages/shortcuts-extension/src/index.ts
@@ -14,7 +14,7 @@ import {
 } from '@phosphor/commands';
 
 import {
-  JSONObject, JSONValue
+  JSONExt, JSONObject, JSONValue
 } from '@phosphor/coreutils';
 
 import {
@@ -57,7 +57,25 @@ const plugin: JupyterLabPlugin<void> = {
     const { commands } = app;
 
     settingReqistry.load(plugin.id).then(settings => {
+      // Load the shortcuts on initial load.
       Private.loadShortcuts(commands, settings.composite);
+
+      // Save newly modified key bindings to settings.
+      commands.keyBindingChanged.connect((sender: any, change: CommandRegistry.IKeyBindingChangedArgs) => {
+        const { args, command, keys, selector } = change.binding;
+        const binding = {
+          command, selector,
+          args: args as JSONObject,
+          keys: keys as Array<string>
+        };
+        const key = Object.keys(settings.composite).filter(key => {
+          return JSONExt.deepEqual(binding, settings.composite[key]);
+        })[0] || command;
+
+        settingReqistry.set(plugin.id, key, binding);
+      });
+
+      // Reload the shortcuts if settings change.
       settings.changed.connect(() => {
         Private.loadShortcuts(commands, settings.composite);
       });

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

Oh wait, I once again misunderstood your question. You're saying you can't remove an extant binding. Right.

It might be the case that offering a save method in the shortcuts extension is the way to go after all.

@afshin
Copy link
Member

@afshin afshin commented Mar 3, 2018

And possibly also listening to the keyBindingChanged as above in case somebody adds a binding without using that save method.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 3, 2018

Right, only the owner of the keybinding seems to be able to remove it. I suppose we should go with some sort of save functionality.

@ian-r-rose ian-r-rose changed the title Work on easy-configuring of the console execute shortcut. [WIP] Work on easy-configuring of the console execute shortcut. Mar 5, 2018
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Mar 13, 2018

Taking another look at this: I think we need a better story for best practices when it comes to keyboard shortcuts. Currently the shortcuts-extension owns the serialization of the shortcuts for all the core plugins. This is nice as they are all in one place, but it has a few holes in it:

  • What if a plugin wants to do some additional, or more coordinated changes to shortcuts, as in this case? Should it write to the setting registry and let the shortcuts-extension plugin update commands.keyBinding? Or should it update commands.keyBinding and let the shortcuts-extension do the serialization?
  • There is currently no good way for an extension to remove a keybinding. commands.addKeyBinding returns an opaque disposable, which is stored in a private namespace in the shortcuts-extension. The only way to do it at the moment is to write to the setting registry of shortcuts-extension and let it pick up the changes.
  • We could think about exposing a Token in the shortcuts-extension with something like save and remove on it. However, this is kind of messy, and it feels like it would be the third place where similar functionality is implemented (commands.keyBindings, the setting registry, and the shortcuts-extension).
  • If extensions should add new keybindings to the commands object, should we serialize those by default? Or should they be considered in-memory bindings?

Anyways, food for thought. I am not sure what the best way forwards is.

const enterToExecute = 'console:enter-to-execute';

export
const shiftEnterToExecute = 'consol:shift-enter-to-execute';
Copy link
Member

@saulshanabrook saulshanabrook Mar 14, 2018

consol maybe should be console?

Copy link
Member Author

@ian-r-rose ian-r-rose Mar 14, 2018

Haha, right you are.

@ian-r-rose ian-r-rose force-pushed the console_execute_settings branch from e824418 to 33b20d6 Mar 14, 2018
@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 14, 2018

According to @ian-r-rose this needs a rebase and then review/merge.

@ian-r-rose ian-r-rose force-pushed the console_execute_settings branch from 33b20d6 to ce6ab99 May 14, 2018
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 14, 2018

@afshin This is rebased and ready for a look.

@afshin afshin changed the title [WIP] Work on easy-configuring of the console execute shortcut. Work on easy-configuring of the console execute shortcut. May 15, 2018
afshin
afshin approved these changes May 15, 2018
Copy link
Member

@afshin afshin left a comment

Thanks, @ian-r-rose!

@afshin afshin merged commit 659b053 into jupyterlab:master May 15, 2018
2 checks passed
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.