Notebook Diff Viewer does not reflect true state of ipynb (JSON contents)

[DonJayamanne]

• Create an ipynb file (with one Python cell)
• Save this
• Verify the entry exists in time line
• Change the language of the cell to javascript
• Save this
• Verify we have an entry Update Cell Language
• Click that item

Bug

• On the left we need {custom: metadata: vscode: languageId: "javascript"...
• On the right there's no such entry in metadata

This is incorrect. The ipynb has the above metadata.
What seems to be happening is

• When the language changes,
• We serialize the ipynb and JSon contains the metadata
• However the model is not updated with corresponding changes to the ipynb file

I.e. model change triggered => ipynb serialize to JSON => Json contains new metadtaa not in model => Missing step is to update the model again

If we implement the last step, we could always end up in an infinite loop,
Perhaps we should first update the model with metadata when ever we detect chagnes in the model before the save event.
E.g. as part of pre-Save (onDidChangeNotebookDocument &/or onWillSaveNotebookDocument)
This way, model will always be accurate and upto date when ipynb is serialized.

[DonJayamanne]

Easier way to explain the bug

• Create an ipynb file (with one Python cell)
• Save this
• Change the language of the cell to javascript
• Save this
• The cell metadata will not contain the new metadata that exists in the ipynb JSON file. (model is not in sync with ipynb json)

[DonJayamanne]

Suggested Solution

• Perform model updates when we're about to save (or when we detect changes to model)
• When saving, ensure we save the metadata as is, without generating any new content
  I.e. JSON.stringify the metadata.

[DonJayamanne]

@rebornix /cc

[rebornix]

Considering that a serialize operation should not have side effects (ideally), enforcing a "save participant" from ipynb extension makes very good sense to me. We can leverage onWillSaveNotebookDocument which support additional async operations, like how we handle attachment clearnup

vscode/extensions/ipynb/src/notebookAttachmentCleaner.ts

Lines 83 to 108 in 9f25e63

	this._delayer.dispose();
	if (e.notebook.getCells().length === 0) {
	return;
	}
	const notebookEdits: vscode.NotebookEdit[] = [];
	for (const cell of e.notebook.getCells()) {
	if (cell.kind !== vscode.NotebookCellKind.Markup) {
	continue;
	}
	const metadataEdit = this.cleanNotebookAttachments({
	notebook: e.notebook,
	cell: cell,
	document: cell.document
	});
	if (metadataEdit) {
	notebookEdits.push(metadataEdit);
	}
	}
	if (!notebookEdits.length) {
	return;
	}
	const workspaceEdit = new vscode.WorkspaceEdit();
	workspaceEdit.set(e.notebook.uri, notebookEdits);
	e.waitUntil(Promise.resolve(workspaceEdit));

[rebornix]

For such critical metadata change, we might want to handle both auto save and manual save as we want the json and model to be always in sync.

[DonJayamanne]

Another simple repro step

• Create a notebook, and save this
• Add a new cell
• The latest entry in timeline will be Insert Cell
• However there's a difference, one has metadata: {} and the other (current model state) does not have any metadata

Again, model does not contain metadta (as this is a new cell) and ipynb JSON contains metadata when serialized.

[DonJayamanne]

Internal Notes

Here's the work flow

• Add a cell
• A change event is triggered, undo redo stack is updated
  • Last undo redo is Insert Cell
• User hits save button
• Working copy starts to handle the save operation
• Save participants start
  • We add a save participant and update the model via a work space edit
  • Now a new change event is triggered
  • This results in a whole new undo redo stack
  • The latest (last) undo redo item is Update Cell Metadata
• Save participants done
• Working copy keeps track of the last undo redo as the entry for Time line

Problems

• Time line does not include Insert Cell, time line shows only Update Cell Metadata
• I guess this is correct,
  However from a users perspective, the labels are never going to be meaningful as we'd always say Update Cell Metadata.