Refactor setting transformation.

packages/coreutils/src/settingregistry.ts

@@ -150,10 +150,15 @@ export namespace ISettingRegistry {
    */
   export namespace IPlugin {
     /**
-     * A function that transforms a plugin object before it is loaded into a
+     * A function that transforms a plugin object before it is consumed by the
      * setting registry.
      */
-    export type Transform = (plugin: IPlugin) => IPlugin | Promise<IPlugin>;
+    export type Transform = (plugin: IPlugin) => IPlugin;
+
+    /**
+     * The phases during which a transformation may be applied to a plugin.
+     */
+    export type Phase = 'compose' | 'fetch';
   }
 
   /**
@@ -363,20 +368,14 @@ export namespace ISettingRegistry {
   }
 
   /**
-   * A namespace for setting object functionality.
+   * An interface describing a JupyterLab keyboard shortcut.
    */
-  export namespace ISettings {
+  export interface IShortcut extends JSONObject {
     /**
-     * A function that transforms a settings object before returning it from a
-     * setting registry.
+     * The optional arguments passed into the shortcut's command.
      */
-    export type Transform = (plugin: IPlugin) => ISettings | Promise<ISettings>;
-  }
+    args?: JSONObject;
 
-  /**
-   * An interface describing a JupyterLab keyboard shortcut.
-   */
-  export interface IShortcut extends JSONObject {
     /**
      * The shortcut category for rendering and organization of shortcuts.
      */
@@ -637,11 +636,11 @@ export class SettingRegistry {
     await this._ready;
 
     const plugins = this.plugins;
+    const registry = this;
 
     // If the plugin exists, resolve.
     if (plugin in plugins) {
-      const settings = await this._transform('settings', plugins[plugin]);
-      return settings as ISettingRegistry.ISettings;
+      return new Settings({ plugin: plugins[plugin], registry });
     }
 
     // If the plugin needs to be loaded from the data connector, fetch.
@@ -660,15 +659,14 @@ export class SettingRegistry {
     // Wait for data preload before normal allowing normal operation.
     await this._ready;
 
-    const plugins = this.plugins;
     const fetched = await this.connector.fetch(plugin);
-    const transformed = await this._transform('plugin', fetched);
+    const plugins = this.plugins;
+    const registry = this;
 
-    this._load(transformed as ISettingRegistry.IPlugin);
+    await this._load(await this._transform('fetch', fetched));
     this._pluginChanged.emit(plugin);
 
-    const settings = await this._transform('settings', plugins[plugin]);
-    return settings as ISettingRegistry.ISettings;
+    return new Settings({ plugin: plugins[plugin], registry });
   }
 
   /**
@@ -741,25 +739,34 @@ export class SettingRegistry {
    * @param transforms - The transform functions applied to the plugin.
    *
    * @returns A disposable that removes the setting transform from the registry.
+   *
+   * #### Notes
+   * - `compose` transformations: The registry automatically overwrites a
+   * plugin's default values with user overrides, but a plugin may instead wish
+   * to merge values. This behavior can be accomplished in a `compose`
+   * transformation.
+   * - `plugin` transformations: The registry uses the plugin data that is
+   * fetched from its connector. If a plugin wants to override, e.g. to update
+   * its schema with dynamic defaults, a `plugin` transformation can be applied.
+   * - `settings` transformations: The registry creates a settings object to
+   * return to clients loading a plugin. If a custom settings object is
+   * necessary, a `settings` transformation can be applied.
    */
   transform(
     plugin: string,
     transforms: {
-      plugin?: ISettingRegistry.IPlugin.Transform;
-      settings?: ISettingRegistry.ISettings.Transform;
+      [phase in ISettingRegistry.IPlugin.Phase]: ISettingRegistry.IPlugin.Transform
     }
   ): IDisposable {
-    const registry = this;
     const transformers = this._transformers;
 
     if (plugin in transformers) {
-      throw new Error(`${plugin} aleady has a transformer.`);
+      throw new Error(`${plugin} already has a transformer.`);
     }
 
     transformers[plugin] = {
-      plugin: transforms.plugin || (plugin => plugin),
-      settings:
-        transforms.settings || (plugin => new Settings({ plugin, registry }))
+      fetch: transforms.fetch || (plugin => plugin),
+      compose: transforms.compose || (plugin => plugin)
     };
 
     return new DisposableDelegate(() => {
@@ -794,16 +801,13 @@ export class SettingRegistry {
 
   /**
    * Load a plugin into the registry.
-   *
-   * #### Notes
-   * A
    */
-  private _load(data: ISettingRegistry.IPlugin): void {
+  private async _load(data: ISettingRegistry.IPlugin): Promise<void> {
     const plugin = data.id;
 
     // Validate and preload the item.
     try {
-      this._validate(data);
+      await this._validate(data);
     } catch (errors) {
       const output = [`Validating ${plugin} failed:`];
 
@@ -826,9 +830,7 @@ export class SettingRegistry {
     plugins.forEach(async plugin => {
       try {
         // Apply a transformation to the plugin if necessary.
-        const transformed = await this._transform('plugin', plugin);
-
-        this._load(transformed as ISettingRegistry.IPlugin);
+        await this._load(await this._transform('fetch', plugin));
       } catch (errors) {
         /* Ignore preload errors. */
         console.log('Ignored setting registry preload errors.', errors);
@@ -847,16 +849,13 @@ export class SettingRegistry {
     }
 
     try {
-      this._validate(plugins[plugin]);
+      await this._validate(plugins[plugin]);
     } catch (errors) {
       console.warn(`${plugin} validation errors:`, errors);
       throw new Error(`${plugin} failed to validate; check console.`);
     }
 
-    // Apply a transformation to the plugin if necessary.
-    const transformed = await this._transform('plugin', plugins[plugin]);
-
-    this._load(transformed as ISettingRegistry.IPlugin);
+    await this._load(plugins[plugin]);
     await this.connector.save(plugin, plugins[plugin].raw);
     this._pluginChanged.emit(plugin);
   }
@@ -865,22 +864,19 @@ export class SettingRegistry {
    * Transform the plugin or settings if necessary.
    */
   private async _transform(
-    transformation: 'plugin' | 'settings',
+    phase: ISettingRegistry.IPlugin.Phase,
     plugin: ISettingRegistry.IPlugin,
     started = new Date().getTime()
-  ): Promise<ISettingRegistry.IPlugin | ISettingRegistry.ISettings> {
+  ): Promise<ISettingRegistry.IPlugin> {
     const elapsed = new Date().getTime() - started;
-    const registry = this;
     const transformers = this._transformers;
 
     if (!plugin.schema['jupyter.lab.setting-transform']) {
-      return transformation === 'settings'
-        ? new Settings({ plugin, registry })
-        : plugin;
+      return plugin;
     }
 
     if (plugin.id in transformers) {
-      return transformers[plugin.id][transformation].call(null, plugin);
+      return transformers[plugin.id][phase].call(null, plugin);
     }
 
     // If the timeout has not been exceeded, stall and try again.
@@ -890,16 +886,16 @@ export class SettingRegistry {
           resolve();
         }, TRANSFORM_TIMEOUT / 20);
       });
-      return this._transform(transformation, plugin, started);
+      return this._transform(phase, plugin, started);
     }
 
-    throw new Error(`#_transformSettings(${plugin.id}) timed out`);
+    throw new Error(`Transforming ${plugin.id} timed out.`);
   }
 
   /**
    * Validate and preload a plugin, compose the `composite` data.
    */
-  private _validate(plugin: ISettingRegistry.IPlugin): void {
+  private async _validate(plugin: ISettingRegistry.IPlugin): Promise<void> {
     // Validate the user data and create the composite data.
     const errors = this.validator.validateData(plugin);
 
@@ -907,16 +903,15 @@ export class SettingRegistry {
       throw errors;
     }
 
-    // Set the local copy.
-    this.plugins[plugin.id] = plugin;
+    // Apply a transformation if necessary and set the local copy.
+    this.plugins[plugin.id] = await this._transform('compose', plugin);
   }
 
   private _pluginChanged = new Signal<this, string>(this);
   private _ready: Promise<void>;
   private _transformers: {
     [plugin: string]: {
-      plugin: ISettingRegistry.IPlugin.Transform;
-      settings: ISettingRegistry.ISettings.Transform;
+      [phase in ISettingRegistry.IPlugin.Phase]: ISettingRegistry.IPlugin.Transform
     };
   } = Object.create(null);
 }

packages/shortcuts-extension/schema/shortcuts.json

@@ -8,6 +8,8 @@
   "additionalProperties": false,
   "properties": {
     "shortcuts": {
+      "title": "Keyboard Shortcuts",
+      "description": "The list of keyboard shortcuts.",
       "items": { "$ref": "#/definitions/shortcut" },
       "type": "array",
       "default": []

packages/shortcuts-extension/src/index.ts

@@ -8,7 +8,7 @@ import { ISettingRegistry } from '@jupyterlab/coreutils';
 import { CommandRegistry } from '@phosphor/commands';
 
 import {
-  JSONValue,
+  JSONExt,
   ReadonlyJSONObject,
   ReadonlyJSONValue
 } from '@phosphor/coreutils';
@@ -72,22 +72,76 @@ const shortcuts: JupyterLabPlugin<void> = {
   requires: [ISettingRegistry],
   activate: async (app: JupyterLab, registry: ISettingRegistry) => {
     const { commands } = app;
+    let canonical: ISettingRegistry.ISchema;
+
+    let loaded: { [name: string]: null } = {};
+    function populate(schema: ISettingRegistry.ISchema) {
+      console.log('populating...');
+      loaded = {};
+      schema.properties.shortcuts.default = Object.keys(registry.plugins)
+        .map(plugin => {
+          loaded[plugin] = null;
+          return registry.plugins[plugin];
+        })
+        .reduce(
+          (acc, val) => acc.concat(val.schema['jupyter.lab.shortcuts'] || []),
+          []
+        )
+        .sort((a, b) => a.command.localeCompare(b.command));
+    }
 
-    // Transform the settings object to return different annotated defaults
-    // calculated from all the keyboard shortcuts in the registry instead of
-    // using the default values from this plugin's schema.
-    registry.transform(shortcuts.id, {
-      plugin: plugin => {
-        const transformed = new Private.ShortcutsPlugin({ plugin, registry });
+    registry.pluginChanged.connect((sender, plugin) => {
+      if (!(plugin in loaded)) {
+        populate(canonical);
+      }
+    });
 
-        console.log('Transforming ... returning plugin:', transformed);
-        return transformed;
+    // Transform the plugin object to return different schema than the default.
+    registry.transform(shortcuts.id, {
+      compose: plugin => {
+        // Only override the canonical schema the first time.
+        if (!canonical) {
+          canonical = JSONExt.deepCopy(plugin.schema);
+          populate(canonical);
+        }
+
+        const defaults = canonical.properties.shortcuts.default;
+        const user = {
+          shortcuts: ((plugin.data && plugin.data.user) || {}).shortcuts || []
+        };
+        const composite = {
+          shortcuts: Private.merge(
+            defaults,
+            user.shortcuts as ISettingRegistry.IShortcut[]
+          )
+        };
+
+        plugin.data = { composite, user };
+
+        return plugin;
+      },
+      fetch: plugin => {
+        // Only override the canonical schema the first time.
+        if (!canonical) {
+          canonical = JSONExt.deepCopy(plugin.schema);
+          populate(canonical);
+        }
+
+        return {
+          data: plugin.data,
+          id: plugin.id,
+          raw: plugin.raw,
+          schema: canonical,
+          version: plugin.version
+        };
       }
     });
 
     try {
       const settings = await registry.load(shortcuts.id);
 
+      console.log('settings.composite', settings.composite);
+
       Private.loadShortcuts(commands, settings.composite);
       settings.changed.connect(() => {
         Private.loadShortcuts(commands, settings.composite);
@@ -115,53 +169,6 @@ namespace Private {
    */
   let disposables: IDisposable;
 
-  /**
-   * A wrapper for this plugin's settings object to override what the setting
-   * registry returns to client that load this plugin.
-   */
-  export class ShortcutsPlugin implements ISettingRegistry.IPlugin {
-    constructor({
-      plugin,
-      registry
-    }: {
-      plugin: ISettingRegistry.IPlugin;
-      registry: ISettingRegistry;
-    }) {
-      const { id, data, raw, schema, version } = plugin;
-
-      this.id = id;
-      this.data = data;
-      this.raw = raw;
-      this.schema = schema;
-      this.version = version;
-
-      const shortcuts = Object.keys(registry.plugins)
-        .map(plugin => registry.plugins[plugin])
-        .slice()
-        .sort((a, b) => {
-          return (a.schema.title || a.id).localeCompare(b.schema.title || b.id);
-        })
-        .reduce(
-          (acc, val) => acc.concat(val.schema['jupyter.lab.shortcuts'] || []),
-          []
-        );
-
-      schema.properties['shortcuts'].default = shortcuts;
-    }
-
-    id: string;
-
-    data: ISettingRegistry.ISettingBundle;
-
-    raw: string;
-
-    schema: ISettingRegistry.ISchema;
-
-    version: string;
-
-    [name: string]: JSONValue;
-  }
-
   /**
    * Load the keyboard shortcuts from settings.
    */
@@ -183,6 +190,13 @@ namespace Private {
     }, new DisposableSet());
   }
 
+  export function merge(
+    defaults: ISettingRegistry.IShortcut[],
+    user: ISettingRegistry.IShortcut[]
+  ): ISettingRegistry.IShortcut[] {
+    return [];
+  }
+
   /**
    * Normalize potential keyboard shortcut options.
    */