From c27e26211a109a0a6ce486fe4d7157eb1d0480a3 Mon Sep 17 00:00:00 2001 From: Aaron Munger Date: Thu, 19 Sep 2024 11:25:11 -0700 Subject: [PATCH] correct delegation for working copies (#228887) * include view and notebook type in the WC ID * resolve models with alternate viewtype * update test, and new test --- .../browser/interactive.contribution.ts | 2 +- .../notebook/browser/notebook.contribution.ts | 10 +++++++--- .../contrib/notebook/common/notebookCommon.ts | 12 ++++++++---- .../notebook/common/notebookEditorInput.ts | 2 +- .../notebookEditorModelResolverServiceImpl.ts | 14 +++++++------- .../notebook/test/browser/notebookCommon.test.ts | 13 ++++++++++--- .../replNotebook/browser/repl.contribution.ts | 16 ++++++++++------ 7 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/vs/workbench/contrib/interactive/browser/interactive.contribution.ts b/src/vs/workbench/contrib/interactive/browser/interactive.contribution.ts index 54ac71a022268..7d208551f3868 100644 --- a/src/vs/workbench/contrib/interactive/browser/interactive.contribution.ts +++ b/src/vs/workbench/contrib/interactive/browser/interactive.contribution.ts @@ -265,7 +265,7 @@ class InteractiveWindowWorkingCopyEditorHandler extends Disposable implements IW } private _getViewType(workingCopy: IWorkingCopyIdentifier): string | undefined { - return NotebookWorkingCopyTypeIdentifier.parse(workingCopy.typeId); + return NotebookWorkingCopyTypeIdentifier.parse(workingCopy.typeId)?.viewType; } } diff --git a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts index 8f397ac477703..c35c62624f7ec 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts @@ -785,7 +785,7 @@ class SimpleNotebookWorkingCopyEditorHandler extends Disposable implements IWork private handlesSync(workingCopy: IWorkingCopyIdentifier): string /* viewType */ | undefined { const viewType = this._getViewType(workingCopy); - if (!viewType || viewType === 'interactive' || extname(workingCopy.resource) === '.replNotebook') { + if (!viewType || viewType === 'interactive') { return undefined; } @@ -810,8 +810,12 @@ class SimpleNotebookWorkingCopyEditorHandler extends Disposable implements IWork this._register(this._workingCopyEditorService.registerHandler(this)); } - private _getViewType(workingCopy: IWorkingCopyIdentifier): string | undefined { - return NotebookWorkingCopyTypeIdentifier.parse(workingCopy.typeId); + private _getViewType(workingCopy: IWorkingCopyIdentifier) { + const notebookType = NotebookWorkingCopyTypeIdentifier.parse(workingCopy.typeId); + if (notebookType && notebookType.viewType === notebookType.notebookType) { + return notebookType?.viewType; + } + return undefined; } } diff --git a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts index ec4d418f7b6fd..4d8506b9adfea 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts @@ -814,6 +814,7 @@ export interface INotebookLoadOptions { export type NotebookEditorModelCreationOptions = { limits?: IFileReadLimits; scratchpad?: boolean; + viewType?: string; }; export interface IResolvedNotebookEditorModel extends INotebookEditorModel { @@ -1033,13 +1034,16 @@ export class NotebookWorkingCopyTypeIdentifier { private static _prefix = 'notebook/'; - static create(viewType: string): string { - return `${NotebookWorkingCopyTypeIdentifier._prefix}${viewType}`; + static create(notebookType: string, viewType?: string): string { + return `${NotebookWorkingCopyTypeIdentifier._prefix}${notebookType}/${viewType ?? notebookType}`; } - static parse(candidate: string): string | undefined { + static parse(candidate: string): { notebookType: string; viewType: string } | undefined { if (candidate.startsWith(NotebookWorkingCopyTypeIdentifier._prefix)) { - return candidate.substring(NotebookWorkingCopyTypeIdentifier._prefix.length); + const split = candidate.substring(NotebookWorkingCopyTypeIdentifier._prefix.length).split('/'); + if (split.length === 2) { + return { notebookType: split[0], viewType: split[1] }; + } } return undefined; } diff --git a/src/vs/workbench/contrib/notebook/common/notebookEditorInput.ts b/src/vs/workbench/contrib/notebook/common/notebookEditorInput.ts index 2e92193c67016..fef2c7c253b41 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookEditorInput.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookEditorInput.ts @@ -293,7 +293,7 @@ export class NotebookEditorInput extends AbstractResourceEditorInput { if (!this.editorModelReference) { const scratchpad = this.capabilities & EditorInputCapabilities.Scratchpad ? true : false; - const ref = await this._notebookModelResolverService.resolve(this.resource, this.viewType, { limits: this.ensureLimits(_options), scratchpad }); + const ref = await this._notebookModelResolverService.resolve(this.resource, this.viewType, { limits: this.ensureLimits(_options), scratchpad, viewType: this.editorId }); if (this.editorModelReference) { // Re-entrant, double resolve happened. Dispose the addition references and proceed // with the truth. diff --git a/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts b/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts index 953809a9c7441..410339077b319 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts @@ -61,16 +61,16 @@ class NotebookModelReferenceCollection extends ReferenceCollection { + protected async createReferencedObject(key: string, notebookType: string, hasAssociatedFilePath: boolean, limits?: IFileReadLimits, isScratchpad?: boolean, viewType?: string): Promise { // Untrack as being disposed this.modelsToDispose.delete(key); const uri = URI.parse(key); - const workingCopyTypeId = NotebookWorkingCopyTypeIdentifier.create(viewType); + const workingCopyTypeId = NotebookWorkingCopyTypeIdentifier.create(notebookType, viewType); let workingCopyManager = this._workingCopyManagers.get(workingCopyTypeId); if (!workingCopyManager) { - const factory = new NotebookFileWorkingCopyModelFactory(viewType, this._notebookService, this._configurationService, this._telemetryService, this._notebookLoggingService); + const factory = new NotebookFileWorkingCopyModelFactory(notebookType, this._notebookService, this._configurationService, this._telemetryService, this._notebookLoggingService); workingCopyManager = >this._instantiationService.createInstance( FileWorkingCopyManager, workingCopyTypeId, @@ -80,8 +80,8 @@ class NotebookModelReferenceCollection extends ReferenceCollection(NotebookSetting.InteractiveWindowPromptToSave) !== true); - const model = this._instantiationService.createInstance(SimpleNotebookEditorModel, uri, hasAssociatedFilePath, viewType, workingCopyManager, isScratchpadView); + const isScratchpadView = isScratchpad || (notebookType === 'interactive' && this._configurationService.getValue(NotebookSetting.InteractiveWindowPromptToSave) !== true); + const model = this._instantiationService.createInstance(SimpleNotebookEditorModel, uri, hasAssociatedFilePath, notebookType, workingCopyManager, isScratchpadView); const result = await model.load({ limits }); @@ -99,7 +99,7 @@ class NotebookModelReferenceCollection extends ReferenceCollection add reference // !isDirty -> free reference if (isDirty && !onDirtyAutoReference) { - onDirtyAutoReference = this.acquire(key, viewType); + onDirtyAutoReference = this.acquire(key, notebookType); } else if (onDirtyAutoReference) { onDirtyAutoReference.dispose(); onDirtyAutoReference = undefined; @@ -257,7 +257,7 @@ export class NotebookModelResolverServiceImpl implements INotebookEditorModelRes const validated = await this.validateResourceViewType(resource, viewType); - const reference = this._data.acquire(validated.resource.toString(), validated.viewType, hasAssociatedFilePath, options?.limits, options?.scratchpad); + const reference = this._data.acquire(validated.resource.toString(), validated.viewType, hasAssociatedFilePath, options?.limits, options?.scratchpad, options?.viewType); try { const model = await reference.object; return { diff --git a/src/vs/workbench/contrib/notebook/test/browser/notebookCommon.test.ts b/src/vs/workbench/contrib/notebook/test/browser/notebookCommon.test.ts index ac38349dfeabe..a5c1637df4c95 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/notebookCommon.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/notebookCommon.test.ts @@ -410,10 +410,17 @@ suite('CellRange', function () { suite('NotebookWorkingCopyTypeIdentifier', function () { ensureNoDisposablesAreLeakedInTestSuite(); - test('works', function () { + test('supports notebook type only', function () { const viewType = 'testViewType'; - const type = NotebookWorkingCopyTypeIdentifier.create('testViewType'); - assert.strictEqual(NotebookWorkingCopyTypeIdentifier.parse(type), viewType); + const type = NotebookWorkingCopyTypeIdentifier.create(viewType); + assert.deepEqual(NotebookWorkingCopyTypeIdentifier.parse(type), { notebookType: viewType, viewType }); + assert.strictEqual(NotebookWorkingCopyTypeIdentifier.parse('something'), undefined); + }); + + test('supports different viewtype', function () { + const notebookType = { notebookType: 'testNotebookType', viewType: 'testViewType' }; + const type = NotebookWorkingCopyTypeIdentifier.create(notebookType.notebookType, notebookType.viewType); + assert.deepEqual(NotebookWorkingCopyTypeIdentifier.parse(type), notebookType); assert.strictEqual(NotebookWorkingCopyTypeIdentifier.parse('something'), undefined); }); }); diff --git a/src/vs/workbench/contrib/replNotebook/browser/repl.contribution.ts b/src/vs/workbench/contrib/replNotebook/browser/repl.contribution.ts index f525f11860286..677bfe99d6148 100644 --- a/src/vs/workbench/contrib/replNotebook/browser/repl.contribution.ts +++ b/src/vs/workbench/contrib/replNotebook/browser/repl.contribution.ts @@ -21,7 +21,7 @@ import { IWorkbenchContribution, registerWorkbenchContribution2, WorkbenchPhase import { IExtensionService } from '../../../services/extensions/common/extensions.js'; import { IWorkingCopyIdentifier } from '../../../services/workingCopy/common/workingCopy.js'; import { IWorkingCopyEditorHandler, IWorkingCopyEditorService } from '../../../services/workingCopy/common/workingCopyEditorService.js'; -import { extname, isEqual } from '../../../../base/common/resources.js'; +import { isEqual } from '../../../../base/common/resources.js'; import { INotebookService } from '../../notebook/common/notebookService.js'; import { IEditorResolverService, RegisteredEditorPriority } from '../../../services/editor/common/editorResolverService.js'; import { INotebookEditorModelResolverService } from '../../notebook/common/notebookEditorModelResolverService.js'; @@ -124,7 +124,7 @@ export class ReplDocumentContribution extends Disposable implements IWorkbenchCo { createUntitledEditorInput: async ({ resource, options }) => { const scratchpad = this.configurationService.getValue(NotebookSetting.InteractiveWindowPromptToSave) !== true; - const ref = await this.notebookEditorModelResolverService.resolve({ untitledResource: resource }, 'jupyter-notebook', { scratchpad }); + const ref = await this.notebookEditorModelResolverService.resolve({ untitledResource: resource }, 'jupyter-notebook', { scratchpad, viewType: 'repl' }); // untitled notebooks are disposed when they get saved. we should not hold a reference // to such a disposed notebook and therefore dispose the reference as well @@ -151,16 +151,20 @@ class ReplWindowWorkingCopyEditorHandler extends Disposable implements IWorkbenc @IInstantiationService private readonly instantiationService: IInstantiationService, @IWorkingCopyEditorService private readonly workingCopyEditorService: IWorkingCopyEditorService, @IExtensionService private readonly extensionService: IExtensionService, + @INotebookService private readonly notebookService: INotebookService ) { super(); this._installHandler(); } - handles(workingCopy: IWorkingCopyIdentifier): boolean { - const viewType = this._getViewType(workingCopy); - return !!viewType && viewType === 'jupyter-notebook' && extname(workingCopy.resource) === '.replNotebook'; + async handles(workingCopy: IWorkingCopyIdentifier) { + const notebookType = this._getNotebookType(workingCopy); + if (!notebookType) { + return false; + } + return !!notebookType && notebookType.viewType === 'repl' && await this.notebookService.canResolve(notebookType.notebookType); } isOpen(workingCopy: IWorkingCopyIdentifier, editor: EditorInput): boolean { @@ -181,7 +185,7 @@ class ReplWindowWorkingCopyEditorHandler extends Disposable implements IWorkbenc this._register(this.workingCopyEditorService.registerHandler(this)); } - private _getViewType(workingCopy: IWorkingCopyIdentifier): string | undefined { + private _getNotebookType(workingCopy: IWorkingCopyIdentifier) { return NotebookWorkingCopyTypeIdentifier.parse(workingCopy.typeId); } }