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

Refactor keyboard shortcuts #5470

Merged
merged 96 commits into from Dec 6, 2018
Merged

Conversation

@afshin
Copy link
Member

@afshin afshin commented Oct 10, 2018

Fixes #5298

IDataConnector changes

  • Updates IDataConnector and DataConnector to add a list(filter?: V): Promise<{ ids: V[]; values: T[] }> where T is the same resolution type as what fetch() resolves and V is the same type as the id passed into fetch but may have different content (this is conventionally an ID or filter string).
  • Changes StateDB#fetchNamespace() to StateDB#list() to match IDataConnector update.
  • This requires a major version bump in @jupyterlab/coreutils and because this change touches @jupyterlab/services exports, its major version needs to increment as well. Between these two libraries, it's likely that all of the @jupyterlab/... packages will need a major version bump.
  • Makes the setting manager compatible with the IDataConnector that the setting registry needs to function, obviating a standalone connector class.
  • Makes the state database accept a generic type argument (that must extend ReadonlyJSONValue) in order to make it a viable stand-in for many data connectors (e.g., for testing)

Setting registry changes

  • Adds a standalone file plugin-schema.json that is the (version 1.0.0) schema against which a JupyterLab plugin's schema will be validated.
  • Adds three new JupyterLab schema plugin attributes:
    • jupyter.lab.shortcuts is a root-level array of shortcut entities (as defined in plugin-schema.json)
    • jupyter.lab.setting-deprecated is a boolean that indicates whether a particular schema is deprecated.
    • jupyter.lab.setting-transform is a boolean that indicates whether a setting transformation function needs to be applied to an ISettings object before the SettingRegistry#load(plugin: string) promise resolves.
  • Adds SettingRegistry#transform whose signature is:
     transform(plugin: string, transforms: {[phase in Phase]: Transform});
    where a transform function is defined as:
    type Transform = (plugin: IPlugin) => IPlugin | Promise<IPlugin>;
    and IPlugin.Phase is a union of the phases 'compose' and 'fetch'.

Setting editor changes

  • The setting editor does not display a schema if its jupyter.lab.setting-deprecated attribute is set to true
  • Updates alt-text to show plugin version.
    Before:
    tooltip-before
    After:
    tooltip-after
  • Changes default plugin icon in the setting editor to a cog unless explicitly specified.
@afshin afshin added this to the 1.0 milestone Oct 10, 2018
@afshin afshin self-assigned this Oct 10, 2018
@afshin afshin changed the title [wip] Add keyboard shortcuts to extension JSON schema definition. [wip] Refactor keyboard shortcuts Oct 10, 2018
@afshin afshin force-pushed the keyboard-shortcuts branch 2 times, most recently from eb7765d to f64c4b1 Oct 11, 2018
@afshin afshin force-pushed the keyboard-shortcuts branch 3 times, most recently from ba7251e to 395ad0b Oct 15, 2018
@afshin afshin force-pushed the keyboard-shortcuts branch 2 times, most recently from adea386 to 2e3fb72 Nov 1, 2018
@afshin afshin force-pushed the keyboard-shortcuts branch 4 times, most recently from 8196072 to 5ac78ad Nov 13, 2018
@afshin afshin force-pushed the keyboard-shortcuts branch 3 times, most recently from 697e9f4 to ce3e127 Nov 14, 2018
}

const json = await response.json();
const values: ISettingRegistry.IPlugin[] = (json || {})['settings'] || [];
Copy link
Contributor

@jasongrout jasongrout Dec 4, 2018

ISettingRegistry.IPlugin has a data attribute, but these values do not. The data attribute doesn't get set until https://github.com/jupyterlab/jupyterlab/pull/5470/files#diff-1926d857572cffe43160eec8a8e82828R189

I think we should either (a) not call these data values ISettingRegistry.IPlugin until that .data attribute is set, or (b) make the .data attribute optional in ISettingRegistry.IPlugin.

Copy link
Member Author

@afshin afshin Dec 5, 2018

I think making the data optional would place too many burdens on consumers. So maybe we could do a variant of (a) and simply create a minimally empty data attribute right here when they are fetched.

Copy link
Contributor

@jasongrout jasongrout Dec 5, 2018

@afshin and I discussed this more at the dev meeting. I think we have a difference of opinion about how things should be structured, but neither of us feel super strongly one way or the other.

(a) One way of thinking about it is that an IPlugin has a populated data attribute with the user settings and the composed settings. Thus the things returned from the connector are not IPlugins, but rather something more raw. The fetch transform takes in these raw values and transforms them into other raw things. I'm not sure when the compose data is calculated, but perhaps it is before the compose transform, and the compose transform takes and returns an IPlugin.

(b) Another way to think about it is that it is all dealing with an IPlugin, but an IPlugin may not have the data attribute populated until after the bootstrapping process is run. In that thinking, the fetch and compose transforms both operate on an IPlugin, but the IPlugin is in different stages of construction.

At this point, the easiest is to go with @afshin's new commit, which follows the (b) philosophy. I'm willing to try it out and see how it works in practice.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 5, 2018

Here's a third option: Have a 3-way setting in the console extension for "run shortcut", that is "Enter", "Shift Enter", and "No shortcut provided". Use that setting to add a class to the prompt DOM. Supply both keyboard shortcuts in the extension, but only have at most one of them active by which class is added to the prompt. This is a way for the setting to just affect the defaults (not modifying the user settings), but not have any transform magic going on.

In the JLab dev meeting, we decided to go with this approach, but call the classes something more semantically meaningful, like "notebook-mode" or "terminal-mode" or something.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 5, 2018

I added a few commits to address issues that I saw: c926535, bd51c11, ed8d864

There is still a default global “Run” shortcut initially tied to Shift Enter that is still active even when the console interaction mode is “none”. Perhaps we shouldn’t have a “none” option?
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 5, 2018

I added an initial implementation of the console run shortcut setting. I called it more generally "interaction mode", and provided a "none" option. However, there still is a global "Run" shortcut tied to Shift Enter, so with no keyboard shortcuts, Shift Enter still works.

So perhaps we should just have "Notebook" and "Terminal" settings, without a "None".

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 5, 2018

So perhaps we should just have "Notebook" and "Terminal" settings, without a "None".

I just added another commit removing the "none" option.

populate(canonical);
// If some other plugin changed, clear the shortcut cache and reload
// myself (repopulating the keyboard shortcuts)
if (plugin !== shortcuts.id) {
Copy link
Member Author

@afshin afshin Dec 6, 2018

The premise I was assuming was that defaults of a plugin won't change even if its user preferences change. Do you think that might not be the case? This will cause keyboard shortcuts to reload after every trivial user preference update and I think that might be overkill.

Copy link
Member Author

@afshin afshin Dec 6, 2018

Specifically, once a plugin has been loaded (and been transformed), I don't think its jupyter.lab.shortcuts will ever change again unless the author has written a Transform that is non-deterministically modifying the jupyter.lab.shortcuts, which is possible but seems both implausible and like bad behavior to me.

Copy link
Contributor

@jasongrout jasongrout Dec 6, 2018

That's (a) exactly what I was doing in my play example, and (b) totally possible in a transform, and indeed exactly what the shortcut extension does itself (dynamically change its keyboard shortcuts). If we have this assumption that no one else but the shortcuts extension does this, we should definitely document it somewhere prominent where people who author plugin settings will see it.

Copy link
Contributor

@jasongrout jasongrout Dec 6, 2018

On the other hand, isn't calculating the keyboard shortcuts relatively quick? I imagined that it wouldn't add too much overhead to make the behavior technically correct.

@afshin
Copy link
Member Author

@afshin afshin commented Dec 6, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

Disposing all shortcuts and adding g them again seems heavy to me.

Technically, we only have a single plugin that might have changed, so if we kept enough information around, we could just rerun our filtering without asking every plugin for its shortcuts again.

@afshin
Copy link
Member Author

@afshin afshin commented Dec 6, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

With this instrumentation:

diff --git a/packages/shortcuts-extension/src/index.ts b/packages/shortcuts-extension/src/index.ts
index 761151abc..8bc95c28b 100644
--- a/packages/shortcuts-extension/src/index.ts
+++ b/packages/shortcuts-extension/src/index.ts
@@ -165,7 +165,11 @@ const shortcuts: JupyterLabPlugin<void> = {
       // myself (repopulating the keyboard shortcuts)
       if (plugin !== shortcuts.id) {
         canonical = null;
-        registry.reload(shortcuts.id);
+        let start = performance.now();
+        registry.reload(shortcuts.id).then(() => {
+          let diff = performance.now() - start;
+          console.log(`Time to reload shortcuts: ${diff}`);
+        });
       }
     });

I changed settings 62 times and got these values:

> pandas.DataFrame([13,33,46,10,66,21,36,13,22,27,15,34,30,78,35,10,52,30,44,22,10,26,34,23,14,8 ,8 ,24,16,24,12,30,8 ,34,26,21,23,31,9 ,21,18,14,30,19,8 
>    ...: ,17,22,8 ,42,30,29,8 ,13,18,16,29,8 ,14,9 ,28,7 ,19]).describe()                                                                                        
 
count  62.000000
mean   23.306452
std    13.994834
min     7.000000
25%    13.000000
50%    21.500000
75%    30.000000
max    78.000000

I think it's probably okay to do the technically correct thing? What do you think?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

An issue with this technically correct way of doing things is if some other plugin has the same slot to this signal, you get an infinite cycle of plugin reloadings. Though if that other plugin actually is modifying its keyboard shortcuts that infinite cycle is actually the "correct" thing to do.

@afshin
Copy link
Member Author

@afshin afshin commented Dec 6, 2018

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Dec 6, 2018

Get out of here @afshin!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

I'll push something in a bit. Thanks again @afshin, and have a good break!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

I think 21eaecd addresses the performance and looping concerns. Now, when a plugin change is detected, we actually examine the keyboard shortcuts, and if they didn't change, we don't do anything. If the keyboard shortcuts did change, we reload all keyboard shortcuts (which still takes far less than 100ms for me).

I tested with this fetch transform:

diff --git a/packages/console-extension/schema/tracker.json b/packages/console-extension/schema/tracker.json
index 36479c075..eb8af3223 100644
--- a/packages/console-extension/schema/tracker.json
+++ b/packages/console-extension/schema/tracker.json
@@ -28,6 +28,7 @@
       "selector": ".jp-CodeConsole[data-jp-interaction-mode='terminal'] .jp-CodeConsole-promptCell"
     }
   ],
+  "jupyter.lab.transform": true,
   "properties": {
     "interactionMode": {
       "title": "Interaction mode",
diff --git a/packages/console-extension/src/index.ts b/packages/console-extension/src/index.ts
index b3a0ad911..c8978d8a2 100644
--- a/packages/console-extension/src/index.ts
+++ b/packages/console-extension/src/index.ts
@@ -152,6 +152,18 @@ async function activateConsole(
     when: manager.ready
   });
 
+  let times = 1;
+  // Transform the plugin object to return different schema than the default.
+  settingRegistry.transform('@jupyterlab/console-extension:tracker', {
+    fetch: plugin => {
+      for (let i = 0; i < times; i++) {
+        plugin.schema['jupyter.lab.shortcuts'][0].keys.push('S');
+      }
+      times += 1;
+      return plugin;
+    }
+  });
+
   // Add a launcher item if the launcher is available.
   if (launcher) {
     manager.ready.then(() => {

And indeed, when the console settings were changed, the fetch transform effectively changed the keyboard shortcut, and the shortcut extension correctly noticed it and rebuilt the system keyboard shortcuts.

In the future, the “interactionMode” being “notebook” vs “terminal” might affect more than just the run shortcut. In that case, we’d probably want to make the menu item more generic. However, for now, it’s clearer to just note the keyboard shortcut effect.
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

@ian-r-rose - I'm good with this being merged now. Do you want to take it for a spin, and/or review my few commits here at the end?

@jasongrout jasongrout merged commit 7e44e84 into jupyterlab:master Dec 6, 2018
3 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 6, 2018

Woohoo! Thanks again @afshin!

@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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants