Skip to content

Commit

Permalink
correct delegation for working copies (#228887)
Browse files Browse the repository at this point in the history
* include view and notebook type in the WC ID

* resolve models with alternate viewtype

* update test, and new test
  • Loading branch information
amunger committed Sep 19, 2024
1 parent faf7a5c commit c27e262
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}
}

Expand Down
12 changes: 8 additions & 4 deletions src/vs/workbench/contrib/notebook/common/notebookCommon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ export interface INotebookLoadOptions {
export type NotebookEditorModelCreationOptions = {
limits?: IFileReadLimits;
scratchpad?: boolean;
viewType?: string;
};

export interface IResolvedNotebookEditorModel extends INotebookEditorModel {
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,16 @@ class NotebookModelReferenceCollection extends ReferenceCollection<Promise<IReso
return this._dirtyStates.get(resource) ?? false;
}

protected async createReferencedObject(key: string, viewType: string, hasAssociatedFilePath: boolean, limits?: IFileReadLimits, isScratchpad?: boolean): Promise<IResolvedNotebookEditorModel> {
protected async createReferencedObject(key: string, notebookType: string, hasAssociatedFilePath: boolean, limits?: IFileReadLimits, isScratchpad?: boolean, viewType?: string): Promise<IResolvedNotebookEditorModel> {
// 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 = <IFileWorkingCopyManager<NotebookFileWorkingCopyModel, NotebookFileWorkingCopyModel>><any>this._instantiationService.createInstance(
FileWorkingCopyManager,
workingCopyTypeId,
Expand All @@ -80,8 +80,8 @@ class NotebookModelReferenceCollection extends ReferenceCollection<Promise<IReso
this._workingCopyManagers.set(workingCopyTypeId, workingCopyManager);
}

const isScratchpadView = isScratchpad || (viewType === 'interactive' && this._configurationService.getValue<boolean>(NotebookSetting.InteractiveWindowPromptToSave) !== true);
const model = this._instantiationService.createInstance(SimpleNotebookEditorModel, uri, hasAssociatedFilePath, viewType, workingCopyManager, isScratchpadView);
const isScratchpadView = isScratchpad || (notebookType === 'interactive' && this._configurationService.getValue<boolean>(NotebookSetting.InteractiveWindowPromptToSave) !== true);
const model = this._instantiationService.createInstance(SimpleNotebookEditorModel, uri, hasAssociatedFilePath, notebookType, workingCopyManager, isScratchpadView);
const result = await model.load({ limits });


Expand All @@ -99,7 +99,7 @@ class NotebookModelReferenceCollection extends ReferenceCollection<Promise<IReso
// isDirty -> 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;
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
16 changes: 10 additions & 6 deletions src/vs/workbench/contrib/replNotebook/browser/repl.contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -124,7 +124,7 @@ export class ReplDocumentContribution extends Disposable implements IWorkbenchCo
{
createUntitledEditorInput: async ({ resource, options }) => {
const scratchpad = this.configurationService.getValue<boolean>(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
Expand All @@ -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 {
Expand All @@ -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);
}
}
Expand Down

0 comments on commit c27e262

Please sign in to comment.