Sync `collapsed` and `jupyter.outputs_hidden` metadata in the cell model

This makes sure that these redundant pieces of metadata always agree.

See https://github.com/jupyter/nbformat/issues/137

packages/cells/src/model.ts

@@ -19,7 +19,8 @@ import {
   IObservableJSON,
   IModelDB,
   IObservableValue,
-  ObservableValue
+  ObservableValue,
+  IObservableMap
 } from '@jupyterlab/observables';
 
 import { IOutputAreaModel, OutputAreaModel } from '@jupyterlab/outputarea';
@@ -493,14 +494,35 @@ export class CodeCellModel extends CellModel implements ICodeCellModel {
       this
     );
 
-    // We prefer output collapse status to be in the `collapse` metadata field
-    // rather than the `jupyter.outputs_hidden` field for backwards
-    // compatibility. We only do this conversion at construction, so See https://github.com/jupyter/nbformat/issues/137.
+    // We keep `collapsed` and `jupyter.outputs_hidden` metadata in sync, since
+    // they are redundant in nbformat 4.4. See
+    // https://github.com/jupyter/nbformat/issues/137
+    this.metadata.changed.connect(
+      Private.collapseChanged,
+      this
+    );
 
-    // We only do this conversion at construction so that we can understand
-    // nbformat 4.4. We don't do the conversion at runtime so the behavior is
-    // not confusing.
-    this._convertCollapsed();
+    // Sync `collapsed` and `jupyter.outputs_hidden` for the first time, giving
+    // preference to `collapsed`.
+    if (this.metadata.has('collapsed')) {
+      let collapsed = this.metadata.get('collapsed');
+      Private.collapseChanged(this.metadata, {
+        type: 'change',
+        key: 'collapsed',
+        oldValue: collapsed,
+        newValue: collapsed
+      });
+    } else if (this.metadata.has('jupyter')) {
+      let jupyter = this.metadata.get('jupyter') as JSONObject;
+      if (jupyter.hasOwnProperty('outputs_hidden')) {
+        Private.collapseChanged(this.metadata, {
+          type: 'change',
+          key: 'jupyter',
+          oldValue: jupyter,
+          newValue: jupyter
+        });
+      }
+    }
   }
 
   /**
@@ -570,34 +592,6 @@ export class CodeCellModel extends CellModel implements ICodeCellModel {
     });
   }
 
-  /**
-   * Consolidate `jupyter.outputs_hidden` metadata and `collapsed` metadata
-   *
-   * #### Notes
-   * We transfer `jupyter.outputs_hidden` metadata to the `collapsed` metadata
-   * field, with the `collapsed` metadata field taking precedence. See
-   * https://github.com/jupyter/nbformat/issues/137
-   */
-  private _convertCollapsed() {
-    const jupyter = this.metadata.get('jupyter') as JSONObject;
-    if (jupyter === undefined || !jupyter.hasOwnProperty('outputs_hidden')) {
-      return;
-    }
-
-    const { outputs_hidden, ...other } = jupyter;
-
-    // collapsed takes precedence over jupyter.outputs_hidden if it is defined
-    if (outputs_hidden === true && !this.metadata.has('collapsed')) {
-      this.metadata.set('collapsed', outputs_hidden);
-    }
-
-    if (Object.keys(other).length === 0) {
-      this.metadata.delete('jupyter');
-    } else {
-      this.metadata.set('jupyter', other);
-    }
-  }
-
   /**
    * Handle a change to the execution count.
    */
@@ -657,3 +651,33 @@ export namespace CodeCellModel {
    */
   export const defaultContentFactory = new ContentFactory();
 }
+
+namespace Private {
+  export function collapseChanged(
+    metadata: IObservableJSON,
+    args: IObservableMap.IChangedArgs<JSONValue>
+  ) {
+    if (args.key === 'collapsed') {
+      const jupyter = (metadata.get('jupyter') || {}) as JSONObject;
+      const { outputs_hidden, ...newJupyter } = jupyter;
+
+      if (outputs_hidden !== args.newValue) {
+        if (args.newValue !== undefined) {
+          newJupyter['outputs_hidden'] = args.newValue;
+        }
+        if (Object.keys(newJupyter).length === 0) {
+          metadata.delete('jupyter');
+        } else {
+          metadata.set('jupyter', newJupyter);
+        }
+      }
+    } else if (args.key === 'jupyter') {
+      const jupyter = (args.newValue || {}) as JSONObject;
+      if (jupyter.hasOwnProperty('outputs_hidden')) {
+        metadata.set('collapsed', jupyter.outputs_hidden);
+      } else {
+        metadata.delete('collapsed');
+      }
+    }
+  }
+}

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

@@ -296,48 +296,41 @@ describe('cells/model', () => {
         expect(called).to.equal(true);
       });
 
-      it('should consolidate collapsed and jupyter.outputs_hidden metadata on construction', () => {
+      it('should sync collapsed and jupyter.outputs_hidden metadata on construction', () => {
         let model: CodeCellModel;
         let jupyter: JSONObject | undefined;
 
+        // Setting `collapsed` works
         model = new CodeCellModel({
           cell: { cell_type: 'code', source: '', metadata: { collapsed: true } }
         });
         expect(model.metadata.get('collapsed')).to.be.true;
+        jupyter = model.metadata.get('jupyter') as JSONObject;
+        expect(jupyter.outputs_hidden).to.be.true;
 
-        // collapsed takes precedence
+        // Setting `jupyter.outputs_hidden` works
         model = new CodeCellModel({
           cell: {
             cell_type: 'code',
             source: '',
-            metadata: { collapsed: true, jupyter: { outputs_hidden: false } }
+            metadata: { jupyter: { outputs_hidden: true } }
           }
         });
         expect(model.metadata.get('collapsed')).to.be.true;
-        expect(model.metadata.get('jupyter')).to.be.undefined;
-
-        // default values are not set
-        model = new CodeCellModel({
-          cell: {
-            cell_type: 'code',
-            source: '',
-            metadata: { jupyter: { outputs_hidden: false } }
-          }
-        });
-        expect(model.metadata.get('collapsed')).to.be.undefined;
-        expect(model.metadata.get('jupyter')).to.be.undefined;
+        jupyter = model.metadata.get('jupyter') as JSONObject;
+        expect(jupyter.outputs_hidden).to.be.true;
 
-        // other jupyter values are not disturbed
+        // `collapsed` takes precedence
         model = new CodeCellModel({
           cell: {
             cell_type: 'code',
             source: '',
-            metadata: { jupyter: { outputs_hidden: true, other: true } }
+            metadata: { collapsed: false, jupyter: { outputs_hidden: true } }
           }
         });
-        expect(model.metadata.get('collapsed')).to.be.true;
+        expect(model.metadata.get('collapsed')).to.be.false;
         jupyter = model.metadata.get('jupyter') as JSONObject;
-        expect(jupyter.outputs_hidden).to.be.undefined;
+        expect(jupyter.outputs_hidden).to.be.false;
       });
     });
 
@@ -448,35 +441,70 @@ describe('cells/model', () => {
     });
 
     describe('.metadata', () => {
-      it('should not consolidate collapsed and jupyter.outputs_hidden metadata when changed', () => {
+      it('should sync collapsed and jupyter.outputs_hidden metadata when changed', () => {
         const metadata = new CodeCellModel({}).metadata;
 
         expect(metadata.get('collapsed')).to.be.undefined;
         expect(metadata.get('jupyter')).to.be.undefined;
 
-        // collapsed takes precedence
+        // Setting collapsed sets jupyter.outputs_hidden
         metadata.set('collapsed', true);
-        metadata.set('jupyter', { outputs_hidden: false });
         expect(metadata.get('collapsed')).to.be.true;
+        expect(metadata.get('jupyter')).to.deep.equal({
+          outputs_hidden: true
+        });
+
+        metadata.set('collapsed', false);
+        expect(metadata.get('collapsed')).to.be.false;
         expect(metadata.get('jupyter')).to.deep.equal({
           outputs_hidden: false
         });
 
-        // default values are not set
         metadata.delete('collapsed');
-        metadata.set('jupyter', { outputs_hidden: false });
         expect(metadata.get('collapsed')).to.be.undefined;
+        expect(metadata.get('jupyter')).to.be.undefined;
+
+        // Setting jupyter.outputs_hidden sets collapsed
+        metadata.set('jupyter', { outputs_hidden: true });
+        expect(metadata.get('collapsed')).to.be.true;
+        expect(metadata.get('jupyter')).to.deep.equal({
+          outputs_hidden: true
+        });
+
+        metadata.set('jupyter', { outputs_hidden: false });
+        expect(metadata.get('collapsed')).to.be.false;
         expect(metadata.get('jupyter')).to.deep.equal({
           outputs_hidden: false
         });
 
-        // other jupyter values are not disturbed
-        metadata.delete('collapsed');
+        metadata.delete('jupyter');
+        expect(metadata.get('collapsed')).to.be.undefined;
+        expect(metadata.get('jupyter')).to.be.undefined;
+
+        // Deleting jupyter.outputs_hidden preserves other jupyter fields
         metadata.set('jupyter', { outputs_hidden: true, other: true });
+        expect(metadata.get('collapsed')).to.be.true;
+        expect(metadata.get('jupyter')).to.deep.equal({
+          outputs_hidden: true,
+          other: true
+        });
+        metadata.set('jupyter', { other: true });
         expect(metadata.get('collapsed')).to.be.undefined;
         expect(metadata.get('jupyter')).to.deep.equal({
-          other: true,
-          outputs_hidden: false
+          other: true
+        });
+
+        // Deleting collapsed preserves other jupyter fields
+        metadata.set('jupyter', { outputs_hidden: true, other: true });
+        expect(metadata.get('collapsed')).to.be.true;
+        expect(metadata.get('jupyter')).to.deep.equal({
+          outputs_hidden: true,
+          other: true
+        });
+        metadata.delete('collapsed');
+        expect(metadata.get('collapsed')).to.be.undefined;
+        expect(metadata.get('jupyter')).to.deep.equal({
+          other: true
         });
       });
     });

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

@@ -628,23 +628,6 @@ describe('cells/widget', () => {
         widget.loadCollapseState();
         expect(widget.outputHidden).to.equal(false);
       });
-
-      it('should defer to `collapsed` metadata key as sole source of truth, ignoring the `jupyter.outputs_hidden` metadata', () => {
-        const collapsedModel = new CodeCellModel({});
-        let widget = new CodeCell({ model: collapsedModel, rendermime });
-
-        // We only pay attention to `collapsed` metadata, even when
-        // `jupyter.outputs_hidden` is defined.
-        collapsedModel.metadata.set('collapsed', false);
-        collapsedModel.metadata.set('jupyter', { outputs_hidden: true });
-        widget.loadCollapseState();
-        expect(widget.outputHidden).to.equal(false);
-
-        collapsedModel.metadata.set('collapsed', true);
-        collapsedModel.metadata.set('jupyter', { outputs_hidden: false });
-        widget.loadCollapseState();
-        expect(widget.outputHidden).to.equal(true);
-      });
     });
 
     describe('#saveCollapseState()', () => {
@@ -669,24 +652,6 @@ describe('cells/widget', () => {
         widget.saveCollapseState();
         expect(model.metadata.get('collapsed')).to.equal(undefined);
       });
-
-      it('should not write to the model `jupyter.outputs_hidden` metadata', () => {
-        const model = new CodeCellModel({});
-        let widget = new CodeCell({ model, rendermime });
-        widget.initializeState();
-        expect(widget.syncCollapse).to.equal(false);
-        expect(widget.outputHidden).to.equal(false);
-
-        widget.outputHidden = true;
-        widget.saveCollapseState();
-        // We do not write to the jupyter.outputs_hidden key
-        expect(model.metadata.get('jupyter')).to.be.undefined;
-
-        model.metadata.set('jupyter', { outputs_hidden: true });
-        widget.outputHidden = false;
-        widget.saveCollapseState();
-        expect(model.metadata.get('jupyter')).to.be.undefined;
-      });
     });
 
     describe('#syncCollapse', () => {