From 778478c42ed5705abdb7c65afea8127b14bfb5ba Mon Sep 17 00:00:00 2001 From: rebornix Date: Thu, 6 Apr 2023 11:51:15 -0700 Subject: [PATCH] Re #179224. Stop disposing dirty editor model if serializer is gone. --- .../notebook/browser/notebook.contribution.ts | 2 +- .../notebook/common/notebookEditorModel.ts | 32 ++-- .../notebookEditorModelResolverServiceImpl.ts | 4 +- .../test/browser/notebookEditorModel.test.ts | 144 +++++++++++------- 4 files changed, 111 insertions(+), 71 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts index 02a40f9eefcb4..cc1d705a463fd 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts @@ -568,7 +568,7 @@ class NotebookEditorManager implements IWorkbenchContribution { // CLOSE notebook editor for models that have no more serializer const listener = notebookService.onWillRemoveViewType(e => { for (const group of editorGroups.groups) { - const staleInputs = group.editors.filter(input => input instanceof NotebookEditorInput && input.viewType === e); + const staleInputs = group.editors.filter(input => input instanceof NotebookEditorInput && input.viewType === e && !input.isDirty()); group.closeEditors(staleInputs); } }); diff --git a/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts b/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts index 98ff3ddd2b866..9745a324fcde5 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts @@ -90,7 +90,7 @@ export class SimpleNotebookEditorModel extends EditorModel implements INotebookE isReadonly(): boolean { if (SimpleNotebookEditorModel._isStoredFileWorkingCopy(this._workingCopy)) { - return this._workingCopy.isReadonly(); + return this._workingCopy?.isReadonly(); } else if (this._fileService.hasCapability(this.resource, FileSystemProviderCapabilities.Readonly)) { return true; } else { @@ -109,8 +109,7 @@ export class SimpleNotebookEditorModel extends EditorModel implements INotebookE } async load(options?: INotebookLoadOptions): Promise { - - if (!this._workingCopy) { + if (!this._workingCopy || !this._workingCopy.model) { if (this.resource.scheme === Schemas.untitled) { if (this._hasAssociatedFilePath) { this._workingCopy = await this._workingCopyManager.resolve({ associatedResource: this.resource }); @@ -168,7 +167,7 @@ export class NotebookFileWorkingCopyModel extends Disposable implements IStoredF constructor( private readonly _notebookModel: NotebookTextModel, - private readonly _notebookSerializer: INotebookSerializer + private readonly _notebookService: INotebookService ) { super(); @@ -202,9 +201,10 @@ export class NotebookFileWorkingCopyModel extends Disposable implements IStoredF } async snapshot(token: CancellationToken): Promise { + const serializer = await this.getNotebookSerializer(); const data: NotebookData = { - metadata: filter(this._notebookModel.metadata, key => !this._notebookSerializer.options.transientDocumentMetadata[key]), + metadata: filter(this._notebookModel.metadata, key => !serializer.options.transientDocumentMetadata[key]), cells: [], }; @@ -218,13 +218,13 @@ export class NotebookFileWorkingCopyModel extends Disposable implements IStoredF internalMetadata: cell.internalMetadata }; - cellData.outputs = !this._notebookSerializer.options.transientOutputs ? cell.outputs : []; - cellData.metadata = filter(cell.metadata, key => !this._notebookSerializer.options.transientCellMetadata[key]); + cellData.outputs = !serializer.options.transientOutputs ? cell.outputs : []; + cellData.metadata = filter(cell.metadata, key => !serializer.options.transientCellMetadata[key]); data.cells.push(cellData); } - const bytes = await this._notebookSerializer.notebookToData(data); + const bytes = await serializer.notebookToData(data); if (token.isCancellationRequested) { throw new CancellationError(); } @@ -232,14 +232,24 @@ export class NotebookFileWorkingCopyModel extends Disposable implements IStoredF } async update(stream: VSBufferReadableStream, token: CancellationToken): Promise { + const serializer = await this.getNotebookSerializer(); const bytes = await streamToBuffer(stream); - const data = await this._notebookSerializer.dataToNotebook(bytes); + const data = await serializer.dataToNotebook(bytes); if (token.isCancellationRequested) { throw new CancellationError(); } - this._notebookModel.reset(data.cells, data.metadata, this._notebookSerializer.options); + this._notebookModel.reset(data.cells, data.metadata, serializer.options); + } + + async getNotebookSerializer(): Promise { + const info = await this._notebookService.withNotebookDataProvider(this.notebookModel.viewType); + if (!(info instanceof SimpleNotebookProviderInfo)) { + throw new Error('CANNOT open file notebook with this provider'); + } + + return info.serializer; } get versionId() { @@ -279,7 +289,7 @@ export class NotebookFileWorkingCopyModelFactory implements IStoredFileWorkingCo } const notebookModel = this._notebookService.createNotebookTextModel(info.viewType, resource, data, info.serializer.options); - return new NotebookFileWorkingCopyModel(notebookModel, info.serializer); + return new NotebookFileWorkingCopyModel(notebookModel, this._notebookService); } } diff --git a/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts b/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts index 41baa1c8266de..96fac50d85414 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts @@ -44,7 +44,9 @@ class NotebookModelReferenceCollection extends ReferenceCollection { const manager = this._workingCopyManagers.get(NotebookWorkingCopyTypeIdentifier.create(viewType)); - manager?.destroy().catch(err => _logService.error(err)); + if (!manager?.workingCopies.find(w => w.isDirty())) { + manager?.destroy().catch(err => _logService.error(err)); + } })); } diff --git a/src/vs/workbench/contrib/notebook/test/browser/notebookEditorModel.test.ts b/src/vs/workbench/contrib/notebook/test/browser/notebookEditorModel.test.ts index df3d90c02417a..1dca935b41026 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/notebookEditorModel.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/notebookEditorModel.test.ts @@ -10,11 +10,12 @@ import { DisposableStore } from 'vs/base/common/lifecycle'; import { Mimes } from 'vs/base/common/mime'; import { URI } from 'vs/base/common/uri'; import { mock } from 'vs/base/test/common/mock'; +import { ExtensionIdentifier } from 'vs/platform/extensions/common/extensions'; import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock'; import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookTextModel'; import { CellKind, NotebookData, TransientOptions } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { NotebookFileWorkingCopyModel } from 'vs/workbench/contrib/notebook/common/notebookEditorModel'; -import { INotebookSerializer } from 'vs/workbench/contrib/notebook/common/notebookService'; +import { INotebookSerializer, INotebookService, SimpleNotebookProviderInfo } from 'vs/workbench/contrib/notebook/common/notebookService'; import { setupInstantiationService } from 'vs/workbench/contrib/notebook/test/browser/testNotebookEditor'; suite('NotebookFileWorkingCopyModel', function () { @@ -29,7 +30,7 @@ suite('NotebookFileWorkingCopyModel', function () { suiteTeardown(() => disposables.dispose()); - test('no transient output is send to serializer', function () { + test('no transient output is send to serializer', async function () { const notebook = instantiationService.createInstance(NotebookTextModel, 'notebook', @@ -43,18 +44,20 @@ suite('NotebookFileWorkingCopyModel', function () { let callCount = 0; const model = new NotebookFileWorkingCopyModel( notebook, - new class extends mock() { - override options: TransientOptions = { transientOutputs: true, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; - override async notebookToData(notebook: NotebookData) { - callCount += 1; - assert.strictEqual(notebook.cells.length, 1); - assert.strictEqual(notebook.cells[0].outputs.length, 0); - return VSBuffer.fromString(''); + mockNotebookService(notebook, + new class extends mock() { + override options: TransientOptions = { transientOutputs: true, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; + override async notebookToData(notebook: NotebookData) { + callCount += 1; + assert.strictEqual(notebook.cells.length, 1); + assert.strictEqual(notebook.cells[0].outputs.length, 0); + return VSBuffer.fromString(''); + } } - } + ) ); - model.snapshot(CancellationToken.None); + await model.snapshot(CancellationToken.None); assert.strictEqual(callCount, 1); } @@ -62,22 +65,24 @@ suite('NotebookFileWorkingCopyModel', function () { let callCount = 0; const model = new NotebookFileWorkingCopyModel( notebook, - new class extends mock() { - override options: TransientOptions = { transientOutputs: false, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; - override async notebookToData(notebook: NotebookData) { - callCount += 1; - assert.strictEqual(notebook.cells.length, 1); - assert.strictEqual(notebook.cells[0].outputs.length, 1); - return VSBuffer.fromString(''); + mockNotebookService(notebook, + new class extends mock() { + override options: TransientOptions = { transientOutputs: false, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; + override async notebookToData(notebook: NotebookData) { + callCount += 1; + assert.strictEqual(notebook.cells.length, 1); + assert.strictEqual(notebook.cells[0].outputs.length, 1); + return VSBuffer.fromString(''); + } } - } + ) ); - model.snapshot(CancellationToken.None); + await model.snapshot(CancellationToken.None); assert.strictEqual(callCount, 1); } }); - test('no transient metadata is send to serializer', function () { + test('no transient metadata is send to serializer', async function () { const notebook = instantiationService.createInstance(NotebookTextModel, 'notebook', @@ -91,18 +96,20 @@ suite('NotebookFileWorkingCopyModel', function () { let callCount = 0; const model = new NotebookFileWorkingCopyModel( notebook, - new class extends mock() { - override options: TransientOptions = { transientOutputs: true, transientCellMetadata: {}, transientDocumentMetadata: { bar: true }, cellContentMetadata: {} }; - override async notebookToData(notebook: NotebookData) { - callCount += 1; - assert.strictEqual(notebook.metadata.foo, 123); - assert.strictEqual(notebook.metadata.bar, undefined); - return VSBuffer.fromString(''); + mockNotebookService(notebook, + new class extends mock() { + override options: TransientOptions = { transientOutputs: true, transientCellMetadata: {}, transientDocumentMetadata: { bar: true }, cellContentMetadata: {} }; + override async notebookToData(notebook: NotebookData) { + callCount += 1; + assert.strictEqual(notebook.metadata.foo, 123); + assert.strictEqual(notebook.metadata.bar, undefined); + return VSBuffer.fromString(''); + } } - } + ) ); - model.snapshot(CancellationToken.None); + await model.snapshot(CancellationToken.None); assert.strictEqual(callCount, 1); } @@ -110,22 +117,24 @@ suite('NotebookFileWorkingCopyModel', function () { let callCount = 0; const model = new NotebookFileWorkingCopyModel( notebook, - new class extends mock() { - override options: TransientOptions = { transientOutputs: false, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; - override async notebookToData(notebook: NotebookData) { - callCount += 1; - assert.strictEqual(notebook.metadata.foo, 123); - assert.strictEqual(notebook.metadata.bar, 456); - return VSBuffer.fromString(''); + mockNotebookService(notebook, + new class extends mock() { + override options: TransientOptions = { transientOutputs: false, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; + override async notebookToData(notebook: NotebookData) { + callCount += 1; + assert.strictEqual(notebook.metadata.foo, 123); + assert.strictEqual(notebook.metadata.bar, 456); + return VSBuffer.fromString(''); + } } - } + ) ); - model.snapshot(CancellationToken.None); + await model.snapshot(CancellationToken.None); assert.strictEqual(callCount, 1); } }); - test('no transient cell metadata is send to serializer', function () { + test('no transient cell metadata is send to serializer', async function () { const notebook = instantiationService.createInstance(NotebookTextModel, 'notebook', @@ -139,18 +148,20 @@ suite('NotebookFileWorkingCopyModel', function () { let callCount = 0; const model = new NotebookFileWorkingCopyModel( notebook, - new class extends mock() { - override options: TransientOptions = { transientOutputs: true, transientDocumentMetadata: {}, transientCellMetadata: { bar: true }, cellContentMetadata: {} }; - override async notebookToData(notebook: NotebookData) { - callCount += 1; - assert.strictEqual(notebook.cells[0].metadata!.foo, 123); - assert.strictEqual(notebook.cells[0].metadata!.bar, undefined); - return VSBuffer.fromString(''); + mockNotebookService(notebook, + new class extends mock() { + override options: TransientOptions = { transientOutputs: true, transientDocumentMetadata: {}, transientCellMetadata: { bar: true }, cellContentMetadata: {} }; + override async notebookToData(notebook: NotebookData) { + callCount += 1; + assert.strictEqual(notebook.cells[0].metadata!.foo, 123); + assert.strictEqual(notebook.cells[0].metadata!.bar, undefined); + return VSBuffer.fromString(''); + } } - } + ) ); - model.snapshot(CancellationToken.None); + await model.snapshot(CancellationToken.None); assert.strictEqual(callCount, 1); } @@ -158,18 +169,35 @@ suite('NotebookFileWorkingCopyModel', function () { let callCount = 0; const model = new NotebookFileWorkingCopyModel( notebook, - new class extends mock() { - override options: TransientOptions = { transientOutputs: false, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; - override async notebookToData(notebook: NotebookData) { - callCount += 1; - assert.strictEqual(notebook.cells[0].metadata!.foo, 123); - assert.strictEqual(notebook.cells[0].metadata!.bar, 456); - return VSBuffer.fromString(''); + mockNotebookService(notebook, + new class extends mock() { + override options: TransientOptions = { transientOutputs: false, transientCellMetadata: {}, transientDocumentMetadata: {}, cellContentMetadata: {} }; + override async notebookToData(notebook: NotebookData) { + callCount += 1; + assert.strictEqual(notebook.cells[0].metadata!.foo, 123); + assert.strictEqual(notebook.cells[0].metadata!.bar, 456); + return VSBuffer.fromString(''); + } } - } + ) ); - model.snapshot(CancellationToken.None); + await model.snapshot(CancellationToken.None); assert.strictEqual(callCount, 1); } }); }); + +function mockNotebookService(notebook: NotebookTextModel, notebookSerializer: INotebookSerializer) { + return new class extends mock() { + override async withNotebookDataProvider(viewType: string): Promise { + return new SimpleNotebookProviderInfo( + notebook.viewType, + notebookSerializer, + { + id: new ExtensionIdentifier('test'), + location: undefined + } + ); + } + }; +}