Skip to content

Commit 9d5a45a

Browse files
jriekenjrieken
committed
nuke NotebookEditorModelManager, add ref-counting notebook resolver service, don't cache or re-create notebook editor inputs, cache notebook widgets by "tab id" (uri and group), extract move spy'ing into separate open override handler
1 parent ac4e67b commit 9d5a45a

7 files changed

Lines changed: 130 additions & 299 deletions

File tree

‎src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts‎

Lines changed: 63 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,8 @@
66
import { coalesce, distinct } from 'vs/base/common/arrays';
77
import { Schemas } from 'vs/base/common/network';
88
import { IDisposable, Disposable } from 'vs/base/common/lifecycle';
9-
import { ResourceMap } from 'vs/base/common/map';
109
import { parse } from 'vs/base/common/marshalling';
11-
import { basename, isEqual } from 'vs/base/common/resources';
10+
import { isEqual } from 'vs/base/common/resources';
1211
import { assertType } from 'vs/base/common/types';
1312
import { URI } from 'vs/base/common/uri';
1413
import { ITextModel, ITextBufferFactory, DefaultEndOfLine, ITextBuffer } from 'vs/editor/common/model';
@@ -17,7 +16,7 @@ import { IModeService } from 'vs/editor/common/services/modeService';
1716
import { ITextModelContentProvider, ITextModelService } from 'vs/editor/common/services/resolverService';
1817
import * as nls from 'vs/nls';
1918
import { Extensions, IConfigurationRegistry } from 'vs/platform/configuration/common/configurationRegistry';
20-
import { IEditorOptions, ITextEditorOptions } from 'vs/platform/editor/common/editor';
19+
import { IEditorOptions, ITextEditorOptions, IResourceEditorInput } from 'vs/platform/editor/common/editor';
2120
import { SyncDescriptor } from 'vs/platform/instantiation/common/descriptors';
2221
import { registerSingleton } from 'vs/platform/instantiation/common/extensions';
2322
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
@@ -83,7 +82,6 @@ class NotebookEditorFactory implements IEditorInputFactory {
8382
resource: input.resource,
8483
name: input.name,
8584
viewType: input.viewType,
86-
group: input.group
8785
});
8886
}
8987
deserialize(instantiationService: IInstantiationService, raw: string) {
@@ -97,13 +95,7 @@ class NotebookEditorFactory implements IEditorInputFactory {
9795
return undefined;
9896
}
9997

100-
// if we have two editors open with the same resource (in different editor groups), we should then create two different
101-
// editor inputs, instead of `getOrCreate`.
10298
const input = NotebookEditorInput.create(instantiationService, resource, name, viewType);
103-
if (typeof data.group === 'number') {
104-
input.updateGroup(data.group);
105-
}
106-
10799
return input;
108100
}
109101

@@ -147,7 +139,6 @@ function getFirstNotebookInfo(notebookService: INotebookService, uri: URI): Note
147139
}
148140

149141
export class NotebookContribution extends Disposable implements IWorkbenchContribution {
150-
private _resourceMapping = new ResourceMap<NotebookEditorInput>();
151142

152143
constructor(
153144
@IEditorService private readonly editorService: IEditorService,
@@ -193,7 +184,27 @@ export class NotebookContribution extends Disposable implements IWorkbenchContri
193184
};
194185
});
195186
},
196-
open: (editor, options, group, context) => this.onEditorOpening(editor, options, group, context)
187+
open: (editor, options, group) => {
188+
return this.onEditorOpening2(editor, options, group);
189+
}
190+
}));
191+
192+
// HACK
193+
// we use the open override to spy on tab movements because that's the only
194+
// way to do that...
195+
this._register(this.editorService.overrideOpenEditor({
196+
open: (input, _options, group, context) => {
197+
if (input instanceof NotebookEditorInput && context === OpenEditorContext.MOVE_EDITOR) {
198+
// when moving a notebook editor we release it from its current tab and we
199+
// "place" it into its future slot so that the editor can pick it up from there
200+
const widgetRef = NotebookRegistry.getNotebookEditorWidget(input.resource, this.editorGroupsService.activeGroup);
201+
if (widgetRef) {
202+
NotebookRegistry.releaseNotebookEditorWidget(input.resource, this.editorGroupsService.activeGroup);
203+
NotebookRegistry.claimNotebookEditorWidget(input.resource, group, widgetRef);
204+
}
205+
}
206+
return undefined;
207+
}
197208
}));
198209

199210
this._register(this.editorService.onDidVisibleEditorsChange(() => {
@@ -215,22 +226,6 @@ export class NotebookContribution extends Disposable implements IWorkbenchContri
215226
this.notebookService.updateActiveNotebookEditor(null);
216227
}
217228
}));
218-
219-
this._register(this.editorService.onDidCloseEditor(({ editor }) => {
220-
if (!(editor instanceof NotebookEditorInput)) {
221-
return;
222-
}
223-
224-
if (!this.editorService.editors.some(other => (
225-
other.resource === editor.resource
226-
&& other instanceof NotebookEditorInput
227-
&& other.viewType === editor.viewType
228-
))) {
229-
editor.clearTextModel();
230-
}
231-
232-
editor.dispose();
233-
}));
234229
}
235230

236231
getUserAssociatedEditors(resource: URI) {
@@ -252,54 +247,38 @@ export class NotebookContribution extends Disposable implements IWorkbenchContri
252247
return this.notebookService.getContributedNotebookProviders(resource);
253248
}
254249

255-
private onEditorOpening(originalInput: IEditorInput, options: IEditorOptions | ITextEditorOptions | undefined, group: IEditorGroup, context: OpenEditorContext): IOpenEditorOverride | undefined {
250+
private onEditorOpening2(originalInput: IEditorInput, options: IEditorOptions | ITextEditorOptions | undefined, group: IEditorGroup): IOpenEditorOverride | undefined {
251+
256252
const id = typeof options?.override === 'string' ? options.override : undefined;
257253
if (id === undefined && originalInput.isUntitled()) {
258-
return;
254+
return undefined;
259255
}
260256

261-
if (originalInput instanceof NotebookEditorInput) {
262-
if ((originalInput.group === group.id || originalInput.group === undefined) && (originalInput.viewType === id || typeof id !== 'string')) {
263-
// No need to do anything
264-
originalInput.updateGroup(group.id);
265-
return {
266-
override: this.editorService.openEditor(originalInput, new NotebookEditorOptions(options || {}).with({ override: false }), group)
267-
};
268-
} else {
269-
// Create a copy of the input.
270-
// Unlike normal editor inputs, we do not want to share custom editor inputs
271-
// between multiple editors / groups.
272-
const copiedInput = NotebookEditorInput.create(this.instantiationService, originalInput.resource, originalInput.name, originalInput.viewType);
273-
copiedInput.updateGroup(group.id);
274-
275-
if (context === OpenEditorContext.MOVE_EDITOR) {
276-
// transfer ownership of editor widget
277-
const widgetRef = NotebookRegistry.getNotebookEditorWidget(originalInput.resource, this.editorGroupsService.activeGroup);
278-
if (widgetRef) {
279-
NotebookRegistry.releaseNotebookEditorWidget(originalInput.resource, this.editorGroupsService.activeGroup);
280-
NotebookRegistry.claimNotebookEditorWidget(originalInput.resource, group, widgetRef);
281-
}
282-
}
283-
284-
return {
285-
override: this.editorService.openEditor(copiedInput, new NotebookEditorOptions(options || {}).with({ override: false }), group)
286-
};
287-
}
257+
if (!originalInput.resource) {
258+
return undefined;
288259
}
289260

290-
let resource = originalInput.resource;
291-
if (!resource) {
261+
if (originalInput instanceof NotebookEditorInput) {
292262
return undefined;
293263
}
294264

265+
let notebookUri: URI = originalInput.resource;
266+
let cellOptions: IResourceEditorInput | undefined;
267+
268+
const data = CellUri.parse(originalInput.resource);
269+
if (data) {
270+
notebookUri = data.notebook;
271+
cellOptions = { resource: originalInput.resource, options };
272+
}
273+
295274
if (id === undefined) {
296-
const existingEditors = group.editors.filter(editor => editor.resource && isEqual(editor.resource, resource) && !(editor instanceof NotebookEditorInput));
275+
const existingEditors = group.editors.filter(editor => editor.resource && isEqual(editor.resource, notebookUri) && !(editor instanceof NotebookEditorInput));
297276

298277
if (existingEditors.length) {
299278
return undefined;
300279
}
301280

302-
const userAssociatedEditors = this.getUserAssociatedEditors(resource);
281+
const userAssociatedEditors = this.getUserAssociatedEditors(notebookUri);
303282
const notebookEditor = userAssociatedEditors.filter(association => this.notebookService.getContributedNotebookProvider(association.viewType));
304283

305284
if (userAssociatedEditors.length && !notebookEditor.length) {
@@ -310,46 +289,18 @@ export class NotebookContribution extends Disposable implements IWorkbenchContri
310289
// user might pick a notebook editor
311290

312291
const associatedEditors = distinct([
313-
...this.getUserAssociatedNotebookEditors(resource),
314-
...this.getContributedEditors(resource)
292+
...this.getUserAssociatedNotebookEditors(notebookUri),
293+
...this.getContributedEditors(notebookUri)
315294
], editor => editor.id).filter(editor => editor.priority === NotebookEditorPriority.default);
316295

317296
if (!associatedEditors.length) {
318297
// there is no notebook editor contribution which is enabled by default
319-
return;
320-
}
321-
322-
} else {
323-
const existingEditors = group.editors.filter(editor => editor.resource && isEqual(editor.resource, resource) && (editor instanceof NotebookEditorInput) && editor.viewType === id);
324-
325-
if (existingEditors.length) {
326-
// switch to this cell
327-
return { override: this.editorService.openEditor(existingEditors[0], new NotebookEditorOptions(options || {}).with({ override: false }), group) };
328-
}
329-
}
330-
331-
let info: NotebookProviderInfo | undefined;
332-
const data = CellUri.parse(resource);
333-
if (data) {
334-
const infos = this.getContributedEditors(data.notebook);
335-
336-
if (infos.length) {
337-
const info = id === undefined ? infos[0] : (infos.find(info => info.id === id) || infos[0]);
338-
// cell-uri -> open (container) notebook
339-
const name = basename(data.notebook);
340-
let input = this._resourceMapping.get(data.notebook);
341-
if (!input || input.isDisposed()) {
342-
input = NotebookEditorInput.create(this.instantiationService, data.notebook, name, info.id);
343-
this._resourceMapping.set(data.notebook, input);
344-
}
345-
346-
input.updateGroup(group.id);
347-
return { override: this.editorService.openEditor(input, new NotebookEditorOptions({ ...options, forceReload: true, cellOptions: { resource, options } }), group) };
298+
return undefined;
348299
}
349300
}
350301

351-
const infos = this.notebookService.getContributedNotebookProviders(resource);
352-
info = id === undefined ? infos[0] : infos.find(info => info.id === id);
302+
const infos = this.notebookService.getContributedNotebookProviders(notebookUri);
303+
let info = infos.find(info => !id || info.id === id);
353304

354305
if (!info && id !== undefined) {
355306
info = this.notebookService.getContributedNotebookProvider(id);
@@ -359,20 +310,19 @@ export class NotebookContribution extends Disposable implements IWorkbenchContri
359310
return undefined;
360311
}
361312

362-
const input = NotebookEditorInput.create(this.instantiationService, resource, originalInput.getName(), info.id);
363-
input.updateGroup(group.id);
364-
this._resourceMapping.set(resource, input);
365313

366314
/**
367315
* Scenario: we are reopening a file editor input which is pinned, we should open in a new editor tab.
368316
*/
369317
let index = undefined;
370-
if (group.activeEditor === originalInput && isEqual(originalInput.resource, resource)) {
318+
if (group.activeEditor === originalInput && isEqual(originalInput.resource, notebookUri)) {
371319
const originalEditorIndex = group.getIndexOfEditor(originalInput);
372320
index = group.isPinned(originalInput) ? originalEditorIndex + 1 : originalEditorIndex;
373321
}
374322

375-
return { override: this.editorService.openEditor(input, new NotebookEditorOptions(options || {}).with({ override: false, index }), group) };
323+
const notebookInput = NotebookEditorInput.create(this.instantiationService, notebookUri, originalInput.getName(), info.id);
324+
const notebookOptions = new NotebookEditorOptions({ ...options, cellOptions, override: false, index });
325+
return { override: this.editorService.openEditor(notebookInput, notebookOptions, group) };
376326
}
377327
}
378328

@@ -385,6 +335,7 @@ class CellContentProvider implements ITextModelContentProvider {
385335
@IModelService private readonly _modelService: IModelService,
386336
@IModeService private readonly _modeService: IModeService,
387337
@INotebookService private readonly _notebookService: INotebookService,
338+
@INotebookEditorModelResolverService private readonly _notebookModelResolverService: INotebookEditorModelResolverService,
388339
) {
389340
this._registration = textModelService.registerTextModelContentProvider(CellUri.scheme, this);
390341
}
@@ -408,12 +359,10 @@ class CellContentProvider implements ITextModelContentProvider {
408359
return null;
409360
}
410361

411-
const editorModel = await this._notebookService.modelManager.resolve(data.notebook, info.id);
412-
if (!editorModel) {
413-
return null;
414-
}
362+
const ref = await this._notebookModelResolverService.resolve(data.notebook, info.id);
363+
let result: ITextModel | null = null;
415364

416-
for (let cell of editorModel.notebook.cells) {
365+
for (let cell of ref.object.notebook.cells) {
417366
if (cell.uri.toString() === resource.toString()) {
418367
const bufferFactory: ITextBufferFactory = {
419368
create: (defaultEOL) => {
@@ -426,15 +375,23 @@ class CellContentProvider implements ITextModelContentProvider {
426375
}
427376
};
428377
const language = cell.cellKind === CellKind.Markdown ? this._modeService.create('markdown') : (cell.language ? this._modeService.create(cell.language) : this._modeService.createByFilepathOrFirstLine(resource, cell.textBuffer.getLineContent(1)));
429-
return this._modelService.createModel(
378+
result = this._modelService.createModel(
430379
bufferFactory,
431380
language,
432381
resource
433382
);
383+
break;
434384
}
435385
}
436386

437-
return null;
387+
if (result) {
388+
const once = result.onWillDispose(() => {
389+
once.dispose();
390+
ref.dispose();
391+
});
392+
}
393+
394+
return result;
438395
}
439396
}
440397

‎src/vs/workbench/contrib/notebook/browser/notebookEditor.ts‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,9 @@ export class NotebookEditor extends BaseEditor {
4343
@IStorageService storageService: IStorageService,
4444
@IEditorService private readonly editorService: IEditorService,
4545
@IEditorGroupsService private readonly editorGroupService: IEditorGroupsService,
46-
@INotificationService private readonly notificationService: INotificationService) {
46+
@INotificationService private readonly notificationService: INotificationService
47+
) {
4748
super(NotebookEditor.ID, telemetryService, themeService, storageService);
48-
49-
// this._widget = this.instantiationService.createInstance(NotebookEditorWidget);
5049
this.editorMemento = this.getEditorMemento<INotebookEditorViewState>(editorGroupService, NOTEBOOK_EDITOR_VIEW_STATE_PREFERENCE_KEY);
5150
}
5251

@@ -144,17 +143,16 @@ export class NotebookEditor extends BaseEditor {
144143

145144
await super.setInput(input, options, token);
146145

146+
147147
// input attached
148148
Event.once(input.onDispose)(() => {
149-
// make sure the editor widget is removed from the view
150-
const existingEditorWidgetForInput = NotebookRegistry.getNotebookEditorWidget(input.resource, group);
151-
if (existingEditorWidgetForInput) {
152-
// the editor widget is only referenced by the editor input
153-
// clear its state
154-
existingEditorWidgetForInput.onWillHide();
155-
existingEditorWidgetForInput.getDomNode().remove();
156-
existingEditorWidgetForInput.dispose();
157-
NotebookRegistry.releaseNotebookEditorWidget(input.resource, group);
149+
// todo@jrieken
150+
// there is no more input for a resource and that is used as signal to clean up
151+
// all widget that might still be around
152+
for (let widget of NotebookRegistry.releaseAllNotebookEditorWidgets(input.resource)) {
153+
widget.onWillHide();
154+
widget.getDomNode().remove();
155+
widget.dispose();
158156
}
159157
});
160158

0 commit comments

Comments
 (0)