Make output hidden state solely controlled by the `collapsed` metadata.

We *set* the `jupyter.outputs_hidden` metadata for completeness, but we only pay attention to the top-level `collapsed` metadata field.

We might remove even setting the jupyter.outputs_hidden metadata depending on the resolution of https://github.com/jupyter/nbformat/issues/137

packages/cells/src/widget.ts

@@ -762,58 +762,59 @@ export class CodeCell extends Cell {
    * Save view collapse state to model
    */
   saveCollapseState() {
-    const metadata = this.model.metadata;
-    const jupyter = { ...(metadata.get('jupyter') as any) };
-    const collapsed = this.model.metadata.get('collapsed');
+    // Because collapse state for a code cell involves two different pieces of
+    // metadata, we block reacting to changes in metadata until we have fully
+    // committed our changes.
+    this._savingMetadata = true;
 
-    // Check to see that the two fields are already set appropriately.
-    if (
-      (this.outputHidden &&
-        jupyter.outputs_hidden === true &&
-        collapsed === true) ||
-      (!this.outputHidden &&
-        jupyter.outputs_hidden === undefined &&
-        collapsed === undefined)
-    ) {
-      // State is already what we want for output collapse, so just call the
-      // super method.
+    try {
       super.saveCollapseState();
-      return;
-    }
 
-    // There are a number of metadata changes below, so ignore any metadata
-    // changes in our syncing until we reach the last change.
-    this._ignoreMetadataChanges = true;
-    super.saveCollapseState();
+      const metadata = this.model.metadata;
+      const collapsed = this.model.metadata.get('collapsed');
+      const jupyter = { ...(metadata.get('jupyter') as any) };
+
+      // Check to see that the two fields are already set appropriately.
+      if (
+        (this.outputHidden &&
+          jupyter.outputs_hidden === true &&
+          collapsed === true) ||
+        (!this.outputHidden &&
+          jupyter.outputs_hidden === undefined &&
+          collapsed === undefined)
+      ) {
+        return;
+      }
 
-    if (this.outputHidden) {
-      // set both metadata keys
-      // https://github.com/jupyterlab/jupyterlab/pull/3981#issuecomment-391139167
-      metadata.set('collapsed', true);
-      jupyter.outputs_hidden = true;
-    } else {
-      metadata.delete('collapsed');
-      delete jupyter.outputs_hidden;
-    }
+      if (this.outputHidden) {
+        // set both metadata keys
+        // https://github.com/jupyterlab/jupyterlab/pull/3981#issuecomment-391139167
+        metadata.set('collapsed', true);
+        jupyter.outputs_hidden = true;
+      } else {
+        metadata.delete('collapsed');
+        delete jupyter.outputs_hidden;
+      }
 
-    // Now we make our last metadata change, so unblock listening to metadata
-    // changes.
-    this._ignoreMetadataChanges = false;
-    if (Object.keys(jupyter).length === 0) {
-      metadata.delete('jupyter');
-    } else {
-      metadata.set('jupyter', jupyter);
+      if (Object.keys(jupyter).length === 0) {
+        metadata.delete('jupyter');
+      } else {
+        metadata.set('jupyter', jupyter);
+      }
+    } finally {
+      this._savingMetadata = false;
     }
   }
 
   /**
    * Revert view collapse state from model.
+   *
+   * We consider the `collapsed` metadata key as the source of truth for outputs
+   * being hidden.
    */
   loadCollapseState() {
     super.loadCollapseState();
-    const jupyter = (this.model.metadata.get('jupyter') as any) || {};
-    const collapsed = this.model.metadata.get('collapsed');
-    this.outputHidden = !!jupyter.outputs_hidden || !!collapsed;
+    this.outputHidden = !!this.model.metadata.get('collapsed');
   }
 
   /**
@@ -957,7 +958,7 @@ export class CodeCell extends Cell {
     model: IObservableMap<JSONValue>,
     args: IObservableMap.IChangedArgs<JSONValue>
   ): void {
-    if (this._ignoreMetadataChanges) {
+    if (this._savingMetadata) {
       // We are in middle of a metadata transaction, so don't react to it.
       return;
     }
@@ -993,7 +994,7 @@ export class CodeCell extends Cell {
   private _outputPlaceholder: OutputPlaceholder = null;
   private _output: OutputArea = null;
   private _syncScrolled = false;
-  private _ignoreMetadataChanges = false;
+  private _savingMetadata = false;
 }
 
 /**

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

@@ -500,15 +500,32 @@ describe('cells/widget', () => {
         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);
+      });
+
+      it('should defer to `collapsed` metadata key as sole source of truth', () => {
+        const collapsedModel = new CodeCellModel({});
+
+        // No collapsed metadata set -> collapsed is false
+        collapsedModel.metadata.set('jupyter', { outputs_hidden: true });
+        let widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
+        expect(widget.outputHidden).to.equal(false);
 
+        // 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 = 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);
       });
 
@@ -618,18 +635,37 @@ describe('cells/widget', () => {
         const model = new CodeCellModel({});
         let widget = new CodeCell({ model, rendermime });
         widget.initializeState();
+        widget.loadCollapseState();
         expect(widget.outputHidden).to.equal(false);
 
-        model.metadata.set('jupyter', { outputs_hidden: true });
+        model.metadata.set('collapsed', true);
         widget.loadCollapseState();
         expect(widget.outputHidden).to.equal(true);
 
-        model.metadata.set('jupyter', { outputs_hidden: false });
+        model.metadata.set('collapsed', false);
         widget.loadCollapseState();
         expect(widget.outputHidden).to.equal(false);
+      });
 
-        // Either one of the existing attributes is enough to set the value.
-        model.metadata.set('collapsed', true);
+      it('should defer to `collapsed` metadata key as sole source of truth', () => {
+        const collapsedModel = new CodeCellModel({});
+
+        // No collapsed metadata set -> collapsed is false
+        collapsedModel.metadata.set('jupyter', { outputs_hidden: true });
+        let widget = new CodeCell({ model: collapsedModel, rendermime });
+        widget.initializeState();
+        widget.loadCollapseState();
+        expect(widget.outputHidden).to.equal(false);
+
+        // 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);
       });

tests/test-notebook/src/actions.spec.ts

@@ -1400,5 +1400,41 @@ describe('@jupyterlab/notebook', () => {
         await promise;
       });
     });
+
+    describe('#clearCollapseScrollState()', () => {
+      it.only('should clear the collapse and scroll metadata on all cells', () => {
+        const markmd = widget.widgets[1].model.metadata;
+        const codemd = widget.widgets[2].model.metadata;
+
+        markmd.set('jupyter', {
+          source_hidden: true,
+          othervalue: true
+        });
+        codemd.set('collapsed', true);
+        codemd.set('jupyter', {
+          source_hidden: true,
+          outputs_hidden: true
+        });
+        NotebookActions.clearCollapseScrollState(widget);
+        expect(markmd.get('jupyter')).to.deep.equal({ othervalue: true });
+        console.log(codemd.get('jupyter'));
+        expect(codemd.get('jupyter')).to.be.undefined;
+        expect(codemd.get('collapsed')).to.be.undefined;
+      });
+
+      it('should preserve the widget mode', () => {
+        NotebookActions.clearCollapseScrollState(widget);
+        expect(widget.mode).to.equal('command');
+        widget.mode = 'edit';
+        NotebookActions.clearCollapseScrollState(widget);
+        expect(widget.mode).to.equal('edit');
+      });
+
+      it('should be a no-op if there is no model', () => {
+        widget.model = null;
+        NotebookActions.clearCollapseScrollState(widget);
+        expect(widget.activeCellIndex).to.equal(-1);
+      });
+    });
   });
 });