Skip to content

Commit

Permalink
Sync collapsed and jupyter.outputs_hidden metadata in the cell model
Browse files Browse the repository at this point in the history 
This makes sure that these redundant pieces of metadata always agree.

See jupyter/nbformat#137
  • Loading branch information
@jasongrout
jasongrout committed Apr 1, 2019
1 parent 80ddce4 commit b6d71f0
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 99 deletions.
96 changes: 60 additions & 36 deletions packages/cells/src/model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import {
IObservableJSON,
IModelDB,
IObservableValue,
ObservableValue
ObservableValue,
IObservableMap
} from '@jupyterlab/observables';

import { IOutputAreaModel, OutputAreaModel } from '@jupyterlab/outputarea';
Expand Down Expand Up @@ -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
});
}
}
}

/**
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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');
}
}
}
}
85 changes: 57 additions & 28 deletions tests/test-cells/src/model.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});

Expand Down Expand Up @@ -448,35 +441,71 @@ 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
});

// 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,
outputs_hidden: false
other: true
});
});
});
Expand Down
35 changes: 0 additions & 35 deletions tests/test-cells/src/widget.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()', () => {
Expand All @@ -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', () => {
Expand Down

0 comments on commit b6d71f0

Please sign in to comment.