Fix automatic syncing with metadata.

This also exposed an issue with the initializeState. A class can’t call initializeState in the constructor since a subclass might need to have its constructor finish before the initializeState is called. Therefore, whatever constructs the cell should call the initializeState.

examples/cell/src/index.ts

@@ -54,7 +54,7 @@ function main(): void {
   const cellWidget = new CodeCell({
     rendermime,
     model: new CodeCellModel({})
-  });
+  }).initializeState();
 
   // Handle the mimeType for the current kernel.
   session.kernelChanged.connect(() => {

packages/cells/src/widget.ts

@@ -199,16 +199,25 @@ export class Cell extends Widget {
         }
       );
     }
+
+    // Initialize state
+    model.metadata.changed.connect(
+      this.onMetadataChanged,
+      this
+    );
   }
 
   /**
-   * Modify some state for initialization.
+   * Initialize view state from model.
    *
-   * Should be called at the end of the subclasses's constructor.
+   * #### Notes
+   * Should be called after construction. For convenience, returns this, so it
+   * can be chained in the construction, like `new Foo().initializeState();`
    */
-  protected initializeState() {
-    this.revertCollapseState();
-    this._readOnly = this.model.metadata.get('editable') === false;
+  initializeState(): this {
+    this.loadCollapseState();
+    this.loadEditableState();
+    return this;
   }
 
   /**
@@ -448,6 +457,29 @@ export class Cell extends Widget {
     }
   }
 
+  /**
+   * Handle changes in the metadata.
+   */
+  protected onMetadataChanged(
+    model: IObservableMap<JSONValue>,
+    args: IObservableMap.IChangedArgs<JSONValue>
+  ): void {
+    switch (args.key) {
+      case 'jupyter':
+        if (this.syncCollapse) {
+          this.loadCollapseState();
+        }
+        break;
+      case 'editable':
+        if (this.syncEditable) {
+          this.loadEditableState();
+        }
+        break;
+      default:
+        break;
+    }
+  }
+
   private _readOnly = false;
   private _model: ICellModel = null;
   private _inputHidden = false;
@@ -455,6 +487,7 @@ export class Cell extends Widget {
   private _inputWrapper: Widget = null;
   private _inputPlaceholder: InputPlaceholder = null;
   private _syncCollapse = false;
+  private _syncEditable = false;
 }
 
 /**
@@ -640,17 +673,10 @@ export class CodeCell extends Cell {
     this._outputPlaceholder = new OutputPlaceholder(() => {
       this.outputHidden = !this.outputHidden;
     });
-
-    // Modify state
-    this.initializeState();
     model.stateChanged.connect(
       this.onStateChanged,
       this
     );
-    model.metadata.changed.connect(
-      this.onMetadataChanged,
-      this
-    );
   }
 
   /**
@@ -659,16 +685,18 @@ export class CodeCell extends Cell {
   readonly model: ICodeCellModel;
 
   /**
-   * Modify some state for initialization.
+   * Initialize view state from model.
    *
-   * Should be called at the end of the subclasses's constructor.
+   * #### Notes
+   * Should be called after construction. For convenience, returns this, so it
+   * can be chained in the construction, like `new Foo().initializeState();`
    */
-  protected initializeState() {
+  initializeState(): this {
     super.initializeState();
-    this.revertCollapseState();
-    this.revertScrolledState;
+    this.loadScrolledState();
 
     this.setPrompt(`${this.model.executionCount || ''}`);
+    return this;
   }
 
   /**
@@ -862,17 +890,20 @@ export class CodeCell extends Cell {
     args: IObservableMap.IChangedArgs<JSONValue>
   ): void {
     switch (args.key) {
-      case 'collapsed':
       case 'scrolled':
-      case 'jupyter':
-        this.update();
+        if (this.syncScrolled) {
+          this.loadScrolledState();
+        }
         break;
-      case 'editable':
-        this.readOnly = !args.newValue;
+      case 'collapsed':
+        if (this.syncCollapse) {
+          this.loadCollapseState();
+        }
         break;
       default:
         break;
     }
+    super.onMetadataChanged(model, args);
   }
 
   /**
@@ -994,8 +1025,6 @@ export class MarkdownCell extends Cell {
       this._ready.resolve(void 0);
     });
     this.renderInput(this._renderer);
-
-    super.initializeState();
   }
 
   /**
@@ -1134,7 +1163,6 @@ export class RawCell extends Cell {
   constructor(options: Cell.IOptions) {
     super(options);
     this.addClass(RAW_CELL_CLASS);
-    super.initializeState();
   }
 
   /**

packages/console/src/widget.ts

@@ -244,7 +244,7 @@ export class CodeConsole extends Widget {
     let banner = (this._banner = new RawCell({
       model,
       contentFactory: this.contentFactory
-    }));
+    })).initializeState();
     banner.addClass(BANNER_CLASS);
     banner.readOnly = true;
     this._content.addWidget(banner);
@@ -889,7 +889,8 @@ export namespace CodeConsole {
       if (!options.contentFactory) {
         options.contentFactory = this;
       }
-      return new CodeCell(options);
+      const cell = new CodeCell(options).initializeState();
+      return cell;
     }
 
     /**
@@ -903,7 +904,7 @@ export namespace CodeConsole {
       if (!options.contentFactory) {
         options.contentFactory = this;
       }
-      return new RawCell(options);
+      return new RawCell(options).initializeState();
     }
   }
 

packages/notebook/src/widget.ts

@@ -755,7 +755,8 @@ export namespace StaticNotebook {
       if (!options.contentFactory) {
         options.contentFactory = this;
       }
-      return new CodeCell(options);
+      const cell = new CodeCell(options).initializeState();
+      return cell;
     }
 
     /**
@@ -772,7 +773,7 @@ export namespace StaticNotebook {
       if (!options.contentFactory) {
         options.contentFactory = this;
       }
-      return new MarkdownCell(options);
+      return new MarkdownCell(options).initializeState();
     }
 
     /**
@@ -786,7 +787,7 @@ export namespace StaticNotebook {
       if (!options.contentFactory) {
         options.contentFactory = this;
       }
-      return new RawCell(options);
+      return new RawCell(options).initializeState();
     }
   }
 

tests/test-cells/src/widget.spec.ts

@@ -334,12 +334,14 @@ describe('cells/widget', () => {
     describe('#constructor()', () => {
       it('should create a code cell widget', () => {
         const widget = new CodeCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         expect(widget).to.be.an.instanceof(CodeCell);
       });
 
       it('should accept a custom contentFactory', () => {
         const contentFactory = NBTestUtils.createCodeCellFactory();
         const widget = new CodeCell({ model, contentFactory, rendermime });
+        widget.initializeState();
         expect(widget).to.be.an.instanceof(CodeCell);
       });
     });
@@ -347,6 +349,7 @@ describe('cells/widget', () => {
     describe('#outputArea', () => {
       it('should be the output area used by the cell', () => {
         const widget = new CodeCell({ model, rendermime });
+        widget.initializeState();
         expect(widget.outputArea).to.be.an.instanceof(OutputArea);
       });
     });
@@ -355,21 +358,25 @@ describe('cells/widget', () => {
       it('should initialize from the model', () => {
         const collapsedModel = new CodeCellModel({});
         let widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputHidden).to.equal(false);
 
         collapsedModel.metadata.set('collapsed', true);
         collapsedModel.metadata.set('jupyter', { outputs_hidden: false });
         widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputHidden).to.equal(true);
 
         collapsedModel.metadata.set('collapsed', false);
         collapsedModel.metadata.set('jupyter', { outputs_hidden: true });
         widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputHidden).to.equal(true);
       });
 
       it('should be the view state of the output being collapsed', () => {
         const widget = new CodeCell({ model, rendermime });
+        widget.initializeState();
         expect(widget.outputHidden).to.equal(false);
         widget.outputHidden = true;
         expect(widget.outputHidden).to.equal(true);
@@ -380,18 +387,22 @@ describe('cells/widget', () => {
       it('should initialize from the model', () => {
         const collapsedModel = new CodeCellModel({});
         let widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputsScrolled).to.equal(false);
 
         collapsedModel.metadata.set('scrolled', false);
         widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputsScrolled).to.equal(false);
 
         collapsedModel.metadata.set('scrolled', 'auto');
         widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputsScrolled).to.equal(false);
 
         collapsedModel.metadata.set('scrolled', true);
         widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
         expect(widget.outputsScrolled).to.equal(true);
       });
     });
@@ -399,12 +410,14 @@ describe('cells/widget', () => {
     describe('#dispose()', () => {
       it('should dispose of the resources held by the widget', () => {
         const widget = new CodeCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         widget.dispose();
         expect(widget.isDisposed).to.equal(true);
       });
 
       it('should be safe to call multiple times', () => {
         const widget = new CodeCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         widget.dispose();
         widget.dispose();
         expect(widget.isDisposed).to.equal(true);
@@ -413,7 +426,7 @@ describe('cells/widget', () => {
 
     describe('#onUpdateRequest()', () => {
       it('should update the widget', () => {
-        const widget = new LogCodeCell();
+        const widget = new LogCodeCell().initializeState();
         expect(widget.methods).to.not.contain('onUpdateRequest');
         MessageLoop.sendMessage(widget, Widget.Msg.UpdateRequest);
         expect(widget.methods).to.contain('onUpdateRequest');
@@ -423,7 +436,7 @@ describe('cells/widget', () => {
     describe('#onMetadataChanged()', () => {
       it('should fire when model metadata changes', () => {
         const method = 'onMetadataChanged';
-        const widget = new LogCodeCell();
+        const widget = new LogCodeCell().initializeState();
         expect(widget.methods).to.not.contain(method);
         widget.model.metadata.set('foo', 1);
         expect(widget.methods).to.contain(method);
@@ -445,6 +458,7 @@ describe('cells/widget', () => {
 
       it('should fulfill a promise if there is no code to execute', async () => {
         const widget = new CodeCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         try {
           await CodeCell.execute(widget, session);
         } catch (error) {
@@ -454,6 +468,7 @@ describe('cells/widget', () => {
 
       it('should fulfill a promise if there is code to execute', async () => {
         const widget = new CodeCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         let originalCount: number;
         widget.model.value.text = 'foo';
         originalCount = widget.model.executionCount;
@@ -471,16 +486,19 @@ describe('cells/widget', () => {
     describe('#constructor()', () => {
       it('should create a markdown cell widget', () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         expect(widget).to.be.an.instanceof(MarkdownCell);
       });
 
       it('should accept a custom contentFactory', () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         expect(widget).to.be.an.instanceof(MarkdownCell);
       });
 
       it('should set the default mimetype to text/x-ipythongfm', () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         expect(widget.model.mimeType).to.equal('text/x-ipythongfm');
       });
     });
@@ -488,6 +506,7 @@ describe('cells/widget', () => {
     describe('#rendered', () => {
       it('should default to true', async () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         Widget.attach(widget, document.body);
         expect(widget.rendered).to.equal(true);
         await framePromise();
@@ -496,6 +515,7 @@ describe('cells/widget', () => {
 
       it('should unrender the widget', async () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         Widget.attach(widget, document.body);
         widget.rendered = false;
         await framePromise();
@@ -507,12 +527,14 @@ describe('cells/widget', () => {
     describe('#dispose()', () => {
       it('should dispose of the resources held by the widget', () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         widget.dispose();
         expect(widget.isDisposed).to.equal(true);
       });
 
       it('should be safe to call multiple times', () => {
         const widget = new MarkdownCell({ model, rendermime, contentFactory });
+        widget.initializeState();
         widget.dispose();
         widget.dispose();
         expect(widget.isDisposed).to.equal(true);
@@ -525,7 +547,7 @@ describe('cells/widget', () => {
           model,
           rendermime,
           contentFactory
-        });
+        }).initializeState();
         expect(widget.methods).to.not.contain('onUpdateRequest');
         MessageLoop.sendMessage(widget, Widget.Msg.UpdateRequest);
         expect(widget.methods).to.contain('onUpdateRequest');
@@ -539,7 +561,7 @@ describe('cells/widget', () => {
     describe('#constructor()', () => {
       it('should create a raw cell widget', () => {
         const model = new RawCellModel({});
-        const widget = new RawCell({ model, contentFactory });
+        const widget = new RawCell({ model, contentFactory }).initializeState();
         expect(widget).to.be.an.instanceof(RawCell);
       });
     });

tests/test-console/src/foreign.spec.ts

@@ -34,7 +34,11 @@ class TestParent extends Panel implements ForeignHandler.IReceiver {
   createCodeCell(): CodeCell {
     const contentFactory = NBTestUtils.createCodeCellFactory();
     const model = new CodeCellModel({});
-    const cell = new CodeCell({ model, rendermime, contentFactory });
+    const cell = new CodeCell({
+      model,
+      rendermime,
+      contentFactory
+    }).initializeState();
     return cell;
   }
 

tests/test-console/src/widget.spec.ts

@@ -153,7 +153,11 @@ describe('console/widget', () => {
       it('should add a code cell to the content widget', () => {
         const contentFactory = NBTestUtils.createCodeCellFactory();
         const model = new CodeCellModel({});
-        const cell = new CodeCell({ model, contentFactory, rendermime });
+        const cell = new CodeCell({
+          model,
+          contentFactory,
+          rendermime
+        }).initializeState();
         Widget.attach(widget, document.body);
         expect(widget.cells.length).to.equal(0);
         widget.addCell(cell);