Merge pull request #2752 from jasongrout/displayview

Delete display_model and display_view
jupyter-widgets · Feb 11, 2020 · fa3c706 · fa3c706
2 parents 8fbc773 + 6d297ea
commit fa3c706
Show file tree

Hide file tree

Showing 18 changed files with 105 additions and 258 deletions.
diff --git a/examples/web1/index.js b/examples/web1/index.js
@@ -31,7 +31,7 @@ document.addEventListener('DOMContentLoaded', function(event) {
       }, console.error.bind(console))
       .then(function(view) {
         console.log(widgetType + ' view created');
-        manager.display_view(null, view);
+        manager.display_view(view);
         return view;
       }, console.error.bind(console));
   }

diff --git a/examples/web1/manager.js b/examples/web1/manager.js
@@ -46,13 +46,10 @@ class WidgetManager extends ManagerBase {
     });
   }
 
-  display_view(msg, view, options) {
+  display_view(view) {
     var that = this;
-    return Promise.resolve(view).then(function(view) {
+    return Promise.resolve(view).then(view => {
       LuminoWidget.attach(view.pWidget, that.el);
-      view.on('remove', function() {
-        console.log('View removed', view);
-      });
       return view;
     });
   }

diff --git a/examples/web2/index.js b/examples/web2/index.js
@@ -21,12 +21,14 @@ document.addEventListener('DOMContentLoaded', function(event) {
   var state = require('./widget_state.json');
   var widgetarea = document.getElementsByClassName('widgetarea')[0];
   var manager = new WidgetManager(widgetarea);
-  manager.set_state(state).then(function(models) {
-    manager.display_model(
-      undefined,
-      models.find(function(element) {
-        return element.model_id == '8621699ecc804983a612f09b7dfe806b';
-      })
-    );
-  });
+  manager
+    .set_state(state)
+    .then(models =>
+      manager.create_view(
+        models.find(
+          element => element.model_id == '8621699ecc804983a612f09b7dfe806b'
+        )
+      )
+    )
+    .then(view => manager.display_view(view));
 });
diff --git a/examples/web2/manager.js b/examples/web2/manager.js
@@ -46,13 +46,10 @@ class WidgetManager extends ManagerBase {
     });
   }
 
-  display_view(msg, view, options) {
+  display_view(view) {
     var that = this;
     return Promise.resolve(view).then(function(view) {
       LuminoWidget.attach(view.pWidget, that.el);
-      view.on('remove', function() {
-        console.log('View removed', view);
-      });
       return view;
     });
   }

diff --git a/examples/web3/src/index.ts b/examples/web3/src/index.ts
@@ -3,6 +3,7 @@ import 'codemirror/lib/codemirror.css';
 import 'codemirror/mode/python/python';
 import 'font-awesome/css/font-awesome.css';
 import { WidgetManager } from './manager';
+import * as lmWidget from '@lumino/widgets';
 
 import {
   KernelManager,
@@ -48,22 +49,21 @@ document.addEventListener('DOMContentLoaded', async function(event) {
   const widgetarea = document.getElementsByClassName(
     'widgetarea'
   )[0] as HTMLElement;
-  const manager = new WidgetManager(kernel, widgetarea);
+  const manager = new WidgetManager(kernel);
 
   // Run backend code to create the widgets.  You could also create the
   // widgets in the frontend, like the other widget examples demonstrate.
   const execution = kernel.requestExecute({ code: code });
-  execution.onIOPub = (msg): void => {
+  execution.onIOPub = async (msg): Promise<void> => {
     // If we have a display message, display the widget.
     if (KernelMessage.isDisplayDataMsg(msg)) {
       const widgetData: any =
         msg.content.data['application/vnd.jupyter.widget-view+json'];
       if (widgetData !== undefined && widgetData.version_major === 2) {
-        const model = manager.get_model(widgetData.model_id);
+        const model = await manager.get_model(widgetData.model_id);
         if (model !== undefined) {
-          model.then(model => {
-            manager.display_model(msg, model);
-          });
+          const view = await manager.create_view(model);
+          lmWidget.Widget.attach(view.pWidget, widgetarea);
         }
       }
     }

diff --git a/examples/web3/src/manager.ts b/examples/web3/src/manager.ts
@@ -1,40 +1,22 @@
 import * as base from '@jupyter-widgets/base';
-import * as pWidget from '@lumino/widgets';
 
 import { Kernel } from '@jupyterlab/services';
 
 import { HTMLManager } from '@jupyter-widgets/html-manager';
 
 import './widgets.css';
-import { DOMWidgetView } from '@jupyter-widgets/base';
 
 export class WidgetManager extends HTMLManager {
-  constructor(kernel: Kernel.IKernelConnection, el: HTMLElement) {
+  constructor(kernel: Kernel.IKernelConnection) {
     super();
     this.kernel = kernel;
-    this.el = el;
 
     kernel.registerCommTarget(this.comm_target_name, async (comm, msg) => {
       const oldComm = new base.shims.services.Comm(comm);
       await this.handle_comm_open(oldComm, msg);
     });
   }
 
-  display_view(
-    msg: any,
-    view: DOMWidgetView,
-    options: any
-  ): Promise<HTMLElement> {
-    return Promise.resolve(view).then(view => {
-      pWidget.Widget.attach(view.pWidget, this.el);
-      view.on('remove', function() {
-        console.log('view removed', view);
-      });
-      // We will resolve this lie in another PR.
-      return view as any;
-    });
-  }
-
   /**
    * Create a comm.
    */
@@ -61,5 +43,4 @@ export class WidgetManager extends HTMLManager {
   }
 
   kernel: Kernel.IKernelConnection;
-  el: HTMLElement;
 }
diff --git a/packages/base-manager/src/manager-base.ts b/packages/base-manager/src/manager-base.ts
@@ -55,33 +55,7 @@ export interface IBase64Buffers extends PartialJSONObject {
 /**
  * Manager abstract base class
  */
-export abstract class ManagerBase<T> implements IWidgetManager {
-  /**
-   * Display a DOMWidgetView for a particular model.
-   */
-  display_model(
-    msg: services.KernelMessage.IMessage | null,
-    model: DOMWidgetModel,
-    options: any = {}
-  ): Promise<T> {
-    return this.create_view(model, options)
-      .then(view => this.display_view(msg, view, options))
-      .catch(reject('Could not create view', true));
-  }
-
-  /**
-   * Display a DOMWidget view.
-   *
-   * #### Notes
-   * This must be implemented by a subclass. The implementation must trigger the view's displayed
-   * event after the view is on the page: `view.trigger('displayed')`
-   */
-  abstract display_view(
-    msg: services.KernelMessage.IMessage | null,
-    view: DOMWidgetView,
-    options: any
-  ): Promise<T>;
-
+export abstract class ManagerBase implements IWidgetManager {
   /**
    * Modifies view options. Generally overloaded in custom widget manager
    * implementations.
@@ -93,8 +67,12 @@ export abstract class ManagerBase<T> implements IWidgetManager {
   /**
    * Creates a promise for a view of a given model
    *
+   * #### Notes
+   * The implementation must trigger the Lumino 'after-attach' and 'after-show' events when appropriate, which in turn will trigger the view's 'displayed' events.
+   *
    * Make sure the view creation is not out of order with
    * any state updates.
+   *
    */
   create_view<VT extends DOMWidgetView = DOMWidgetView>(
     model: DOMWidgetModel,
@@ -108,38 +86,38 @@ export abstract class ManagerBase<T> implements IWidgetManager {
     model: WidgetModel,
     options = {}
   ): Promise<VT> {
-    const viewPromise = (model.state_change = model.state_change.then(() => {
-      return this.loadClass(
-        model.get('_view_name'),
-        model.get('_view_module'),
-        model.get('_view_module_version')
-      )
-        .then((ViewType: typeof WidgetView) => {
-          const view: WidgetView = new ViewType({
+    const id = uuid();
+    const viewPromise = (model.state_change = model.state_change.then(
+      async () => {
+        try {
+          const ViewType = (await this.loadClass(
+            model.get('_view_name'),
+            model.get('_view_module'),
+            model.get('_view_module_version')
+          )) as typeof WidgetView;
+          const view = new ViewType({
             model: model,
             options: this.setViewOptions(options)
           });
           view.listenTo(model, 'destroy', view.remove);
-          return Promise.resolve(view.render()).then(() => {
-            return view;
+          await view.render();
+
+          // This presumes the view is added to the list of model views below
+          view.once('remove', () => {
+            delete model.views[id];
           });
-        })
-        .catch(
-          reject('Could not create a view for model id ' + model.model_id, true)
-        );
-    }));
-    const id = uuid();
+
+          return view;
+        } catch (e) {
+          console.error(
+            `Could not create a view for model id ${model.model_id}`
+          );
+          throw e;
+        }
+      }
+    ));
     model.views[id] = viewPromise;
-    viewPromise.then(view => {
-      view.once(
-        'remove',
-        () => {
-          delete view.model.views[id];
-        },
-        this
-      );
-    });
-    return model.state_change;
+    return viewPromise;
   }
 
   /**

diff --git a/packages/base-manager/test/src/dummy-manager.ts b/packages/base-manager/test/src/dummy-manager.ts
@@ -2,7 +2,6 @@
 // Distributed under the terms of the Modified BSD License.
 
 import * as widgets from '@jupyter-widgets/base';
-import * as services from '@jupyterlab/services';
 import * as Backbone from 'backbone';
 
 import { ManagerBase } from '../../lib';
@@ -139,26 +138,12 @@ const testWidgets = {
   BinaryWidgetView
 };
 
-export class DummyManager extends ManagerBase<HTMLElement> {
+export class DummyManager extends ManagerBase {
   constructor() {
     super();
     this.el = window.document.createElement('div');
   }
 
-  display_view(
-    msg: services.KernelMessage.IMessage,
-    view: Backbone.View<Backbone.Model>,
-    options: any
-  ): Promise<HTMLElement> {
-    // TODO: make this a spy
-    // TODO: return an html element
-    return Promise.resolve(view).then(view => {
-      this.el.appendChild(view.el);
-      view.on('remove', () => console.log('view removed', view));
-      return view.el;
-    });
-  }
-
   protected loadClass(
     className: string,
     moduleName: string,

diff --git a/packages/base-manager/test/src/manager_test.ts b/packages/base-manager/test/src/manager_test.ts
@@ -33,26 +33,6 @@ describe('ManagerBase', function() {
     });
   });
 
-  describe('display_model', function() {
-    it('exists', function() {
-      expect(this.managerBase.display_model).to.not.be.undefined;
-    });
-    it('calls create_view and display_view', async function() {
-      const manager = this.managerBase;
-      sinon.spy(manager, 'create_view');
-      sinon.spy(manager, 'display_view');
-      const msg = { msg: true };
-      const viewOptions = { viewOptions: true };
-      const model = await manager.new_model(this.modelOptions);
-      await manager.display_model(msg, model, viewOptions);
-      expect(manager.create_view.calledWith(model, viewOptions)).to.be.true;
-      expect(manager.display_view.calledWith(msg)).to.be.true;
-      expect(manager.display_view.getCall(0).args[2]).to.deep.equal(
-        viewOptions
-      );
-    });
-  });
-
   describe('setViewOptions', function() {
     it('returns an object', function() {
       expect(this.managerBase.setViewOptions()).to.deep.equal({});

diff --git a/packages/controls/test/src/dummy-manager.ts b/packages/controls/test/src/dummy-manager.ts
@@ -2,7 +2,6 @@
 // Distributed under the terms of the Modified BSD License.
 
 import * as widgets from '../../lib';
-import * as services from '@jupyterlab/services';
 import * as Backbone from 'backbone';
 import * as base from '@jupyter-widgets/base';
 import { WidgetModel, WidgetView } from '@jupyter-widgets/base';
@@ -83,26 +82,12 @@ class TestWidgetView extends base.WidgetView {
 
 const testWidgets = { TestWidget, TestWidgetView };
 
-export class DummyManager extends ManagerBase<HTMLElement> {
+export class DummyManager extends ManagerBase {
   constructor() {
     super();
     this.el = window.document.createElement('div');
   }
 
-  display_view(
-    msg: services.KernelMessage.IMessage,
-    view: Backbone.View<Backbone.Model>,
-    options: any
-  ): Promise<HTMLElement> {
-    // TODO: make this a spy
-    // TODO: return an html element
-    return Promise.resolve(view).then(view => {
-      this.el.appendChild(view.el);
-      view.on('remove', () => console.log('view removed', view));
-      return view.el;
-    });
-  }
-
   protected loadClass(
     className: string,
     moduleName: string,

diff --git a/packages/html-manager/src/htmlmanager.ts b/packages/html-manager/src/htmlmanager.ts
@@ -13,9 +13,9 @@ import {
 } from '@jupyterlab/rendermime';
 
 import { WidgetRenderer, WIDGET_MIMETYPE } from './output_renderers';
-import { WidgetModel, WidgetView } from '@jupyter-widgets/base';
+import { WidgetModel, WidgetView, DOMWidgetView } from '@jupyter-widgets/base';
 
-export class HTMLManager extends ManagerBase<HTMLElement> {
+export class HTMLManager extends ManagerBase {
   constructor(options?: {
     loader?: (moduleName: string, moduleVersion: string) => Promise<any>;
   }) {
@@ -37,18 +37,11 @@ export class HTMLManager extends ManagerBase<HTMLElement> {
    * Display the specified view. Element where the view is displayed
    * is specified in the `options.el` argument.
    */
-  display_view(
-    msg: any,
-    view: any,
-    options: { el: HTMLElement }
-  ): Promise<HTMLElement> {
-    return Promise.resolve(view).then(view => {
-      LuminoWidget.Widget.attach(view.pWidget, options.el);
-      view.on('remove', () => {
-        console.log('View removed', view);
-      });
-      return view;
-    });
+  async display_view(
+    view: Promise<DOMWidgetView> | DOMWidgetView,
+    el: HTMLElement
+  ): Promise<void> {
+    LuminoWidget.Widget.attach((await view).pWidget, el);
   }
 
   /**