Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Re #179224. Stop disposing dirty editor model if serializer is gone. #179410

Merged
merged 1 commit into from
Apr 6, 2023
Merged

Re #179224. Stop disposing dirty editor model if serializer is gone. #179410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
}
);
}
};
}