Skip to content

Commit

Permalink
Re #179224. Stop disposing dirty editor model if serializer is gone. (#…
Browse files Browse the repository at this point in the history 
…179410)
  • Loading branch information
@rebornix
rebornix committed Apr 6, 2023
1 parent 7ba867b commit eafe8f0
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 71 deletions.
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
Expand Down
32 changes: 21 additions & 11 deletions src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -109,8 +109,7 @@ export class SimpleNotebookEditorModel extends EditorModel implements INotebookE
}

async load(options?: INotebookLoadOptions): Promise<IResolvedNotebookEditorModel> {

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 });
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -202,9 +201,10 @@ export class NotebookFileWorkingCopyModel extends Disposable implements IStoredF
}

async snapshot(token: CancellationToken): Promise<VSBufferReadableStream> {
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: [],
};

Expand All @@ -218,28 +218,38 @@ 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();
}
return bufferToStream(bytes);
}

async update(stream: VSBufferReadableStream, token: CancellationToken): Promise<void> {
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<INotebookSerializer> {
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() {
Expand Down Expand Up @@ -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);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ class NotebookModelReferenceCollection extends ReferenceCollection<Promise<IReso

this._disposables.add(_notebookService.onWillRemoveViewType(viewType => {
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));
}
}));
}

Expand Down
144 changes: 86 additions & 58 deletions src/vs/workbench/contrib/notebook/test/browser/notebookEditorModel.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -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',
Expand All @@ -43,41 +44,45 @@ suite('NotebookFileWorkingCopyModel', function () {
let callCount = 0;
const model = new NotebookFileWorkingCopyModel(
notebook,
new class extends mock<INotebookSerializer>() {
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<INotebookSerializer>() {
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);
}

{ // NOT transient output
let callCount = 0;
const model = new NotebookFileWorkingCopyModel(
notebook,
new class extends mock<INotebookSerializer>() {
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<INotebookSerializer>() {
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',
Expand All @@ -91,41 +96,45 @@ suite('NotebookFileWorkingCopyModel', function () {
let callCount = 0;
const model = new NotebookFileWorkingCopyModel(
notebook,
new class extends mock<INotebookSerializer>() {
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<INotebookSerializer>() {
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);
}

{ // NOT transient
let callCount = 0;
const model = new NotebookFileWorkingCopyModel(
notebook,
new class extends mock<INotebookSerializer>() {
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<INotebookSerializer>() {
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',
Expand All @@ -139,37 +148,56 @@ suite('NotebookFileWorkingCopyModel', function () {
let callCount = 0;
const model = new NotebookFileWorkingCopyModel(
notebook,
new class extends mock<INotebookSerializer>() {
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<INotebookSerializer>() {
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);
}

{ // NOT transient
let callCount = 0;
const model = new NotebookFileWorkingCopyModel(
notebook,
new class extends mock<INotebookSerializer>() {
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<INotebookSerializer>() {
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<INotebookService>() {
override async withNotebookDataProvider(viewType: string): Promise<SimpleNotebookProviderInfo> {
return new SimpleNotebookProviderInfo(
notebook.viewType,
notebookSerializer,
{
id: new ExtensionIdentifier('test'),
location: undefined
}
);
}
};
}

0 comments on commit eafe8f0

Please sign in to comment.