Register additional editor overrides when adding new notebook file types (#17708)

* Also standardized file extension contributions to always start with a period, and to always do lower case string comparisons for file extensions.
This commit is contained in:
Cory Rivera
2021-11-19 09:14:41 -08:00
committed by GitHub
parent 09666bc4e8
commit 8e04d3992a
8 changed files with 64 additions and 50 deletions

View File

@@ -53,7 +53,7 @@ import { ImageMimeTypes, TextCellEditModes } from 'sql/workbench/services/notebo
import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
import { NotebookInput } from 'sql/workbench/contrib/notebook/browser/models/notebookInput';
import { INotebookModel } from 'sql/workbench/services/notebook/browser/models/modelInterfaces';
import { IExecuteManager } from 'sql/workbench/services/notebook/browser/notebookService';
import { DEFAULT_NOTEBOOK_FILETYPE, IExecuteManager, SQL_NOTEBOOK_PROVIDER } from 'sql/workbench/services/notebook/browser/notebookService';
import { NotebookExplorerViewletViewsContribution } from 'sql/workbench/contrib/notebook/browser/notebookExplorer/notebookExplorerViewlet';
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
import { ContributedEditorPriority, IEditorOverrideService } from 'vs/workbench/services/editor/common/editorOverrideService';
@@ -62,7 +62,8 @@ import { IModeService } from 'vs/editor/common/services/modeService';
import { ILogService } from 'vs/platform/log/common/log';
import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput';
import { useNewMarkdownRendererKey } from 'sql/workbench/contrib/notebook/common/notebookCommon';
import { JUPYTER_PROVIDER_ID } from 'sql/workbench/common/constants';
import { JUPYTER_PROVIDER_ID, NotebookLanguage } from 'sql/workbench/common/constants';
import { INotebookProviderRegistry, NotebookProviderRegistryId } from 'sql/workbench/services/notebook/common/notebookRegistry';
Registry.as<IEditorInputFactoryRegistry>(EditorExtensions.EditorInputFactories)
.registerEditorInputSerializer(FileNotebookInput.ID, FileNoteBookEditorInputSerializer);
@@ -687,10 +688,12 @@ configurationRegistry.registerConfiguration({
});
const languageAssociationRegistry = Registry.as<ILanguageAssociationRegistry>(LanguageAssociationExtensions.LanguageAssociations);
const notebookRegistry = Registry.as<INotebookProviderRegistry>(NotebookProviderRegistryId);
export class NotebookEditorOverrideContribution extends Disposable implements IWorkbenchContribution {
private _registeredOverrides = new DisposableStore();
private _newFileExtensions: string[] = [];
constructor(
@ILogService private _logService: ILogService,
@@ -705,47 +708,59 @@ export class NotebookEditorOverrideContribution extends Disposable implements IW
this._modeService.onLanguagesMaybeChanged(() => {
this.registerEditorOverrides();
});
notebookRegistry.onNewDescriptionRegistration(({ id, registration }) => {
if (id !== JUPYTER_PROVIDER_ID && id !== SQL_NOTEBOOK_PROVIDER && registration?.fileExtensions?.length > 0) {
let extensions = registration.fileExtensions
.filter(ext => ext?.length > 0 && ext.toLowerCase() !== DEFAULT_NOTEBOOK_FILETYPE);
this._newFileExtensions = this._newFileExtensions.concat(extensions);
this.registerEditorOverrides();
}
});
}
private registerEditorOverrides(): void {
this._registeredOverrides.clear();
// List of language IDs to associate the query editor for. These are case sensitive.
NotebookEditorInputAssociation.languages.map(lang => {
let allExtensions: string[] = [];
// List of built-in language IDs to associate the query editor for. These are case sensitive.
NotebookEditorInputAssociation.languages.forEach(lang => {
const langExtensions = this._modeService.getExtensions(lang);
if (langExtensions.length === 0) {
return;
}
// Create the selector from the list of all the language extensions we want to associate with the
// notebook editor (filtering out any languages which didn't have any extensions registered yet)
const selector = `*{${langExtensions.join(',')}}`;
this._registeredOverrides.add(this._editorOverrideService.registerEditor(
selector,
{
id: NotebookEditor.ID,
label: NotebookEditor.LABEL,
describes: (currentEditor) => currentEditor instanceof FileNotebookInput,
priority: ContributedEditorPriority.builtin
},
{},
(resource, options, group) => {
const fileInput = this._editorService.createEditorInput({
resource: resource
}) as FileEditorInput;
// Try to convert the input, falling back to just a plain file input if we're unable to
const newInput = this.tryConvertInput(fileInput, lang) ?? fileInput;
return { editor: newInput, options: options, group: group };
},
(diffEditorInput, options, group) => {
// Try to convert the input, falling back to the original input if we're unable to
const newInput = this.tryConvertInput(diffEditorInput, lang) ?? diffEditorInput;
return { editor: newInput, options: options, group: group };
}
));
allExtensions = allExtensions.concat(langExtensions);
});
// Add newly registered file extensions
allExtensions = allExtensions.concat(this._newFileExtensions);
// Create the selector from the list of all the language extensions we want to associate with the
// notebook editor
const selector = `*{${allExtensions.join(',')}}`;
this._registeredOverrides.add(this._editorOverrideService.registerEditor(
selector,
{
id: NotebookEditor.ID,
label: NotebookEditor.LABEL,
describes: (currentEditor) => currentEditor instanceof FileNotebookInput,
priority: ContributedEditorPriority.builtin
},
{},
(resource, options, group) => {
const fileInput = this._editorService.createEditorInput({
resource: resource
}) as FileEditorInput;
// Try to convert the input, falling back to just a plain file input if we're unable to
const newInput = this.tryConvertInput(fileInput) ?? fileInput;
return { editor: newInput, options: options, group: group };
},
(diffEditorInput, options, group) => {
// Try to convert the input, falling back to the original input if we're unable to
const newInput = this.tryConvertInput(diffEditorInput) ?? diffEditorInput;
return { editor: newInput, options: options, group: group };
}
));
}
private tryConvertInput(input: IEditorInput, lang: string): IEditorInput | undefined {
const langAssociation = languageAssociationRegistry.getAssociationForLanguage(lang);
private tryConvertInput(input: IEditorInput): IEditorInput | undefined {
const langAssociation = languageAssociationRegistry.getAssociationForLanguage(NotebookLanguage.Ipynb);
const notebookEditorInput = langAssociation?.syncConvertInput?.(input);
if (!notebookEditorInput) {
this._logService.warn('Unable to create input for overriding editor ', input instanceof DiffEditorInput ? `${input.primary.resource.toString()} <-> ${input.secondary.resource.toString()}` : input.resource.toString());

View File

@@ -221,17 +221,17 @@ suite.skip('NotebookService:', function (): void {
assert.strictEqual(notebookService.languageMagics.length, 0, 'No language magics should exist after creation');
assert.strictEqual(notebookService.listNotebookEditors().length, 0, 'No notebook editors should be listed');
assert.strictEqual(notebookService.getMimeRegistry().mimeTypes.length, 15, 'MIME Types need to have appropriate tests when added or removed');
assert.deepStrictEqual(notebookService.getProvidersForFileType('ipynb'), ['sql'], 'sql provider should be registered for ipynb extension');
assert.deepStrictEqual(notebookService.getProvidersForFileType('.ipynb'), ['sql'], 'sql provider should be registered for ipynb extension');
assert.strictEqual(notebookService.getStandardKernelsForProvider('sql').length, 1, 'SQL kernel should be provided by default');
assert.strictEqual(notebookService.getStandardKernelsForProvider('otherProvider'), undefined, 'Other provider should not have kernels since it has not been added as a provider');
assert.deepStrictEqual(notebookService.getSupportedFileExtensions(), ['IPYNB'], 'IPYNB file extension should be supported by default');
assert.deepStrictEqual(notebookService.getSupportedFileExtensions(), ['.ipynb'], 'IPYNB file extension should be supported by default');
await notebookService.registrationComplete;
assert.ok(notebookService.isRegistrationComplete, `notebookService.isRegistrationComplete should be true once its registrationComplete promise is resolved`);
});
test('Validate another provider added successfully', async function (): Promise<void> {
await notebookService.registrationComplete;
assert.deepStrictEqual(notebookService.getProvidersForFileType('ipynb'), ['sql'], 'sql provider should be registered for ipynb extension');
assert.deepStrictEqual(notebookService.getProvidersForFileType('.ipynb'), ['sql'], 'sql provider should be registered for ipynb extension');
const otherProviderRegistration: ProviderDescriptionRegistration = {
fileExtensions: ['ipynb'],
@@ -246,8 +246,8 @@ suite.skip('NotebookService:', function (): void {
const notebookRegistry = Registry.as<INotebookProviderRegistry>(NotebookProviderRegistryId);
notebookRegistry.registerProviderDescription(otherProviderRegistration);
assert.deepStrictEqual(notebookService.getProvidersForFileType('ipynb'), ['sql', 'otherProvider'], 'otherProvider should also be registered for ipynb extension');
assert.deepStrictEqual(notebookService.getSupportedFileExtensions(), ['IPYNB'], 'Only IPYNB should be registered as supported file extension');
assert.deepStrictEqual(notebookService.getProvidersForFileType('.ipynb'), ['sql', 'otherProvider'], 'otherProvider should also be registered for ipynb extension');
assert.deepStrictEqual(notebookService.getSupportedFileExtensions(), ['.ipynb'], 'Only IPYNB should be registered as supported file extension');
assert.strictEqual(notebookService.getStandardKernelsForProvider('otherProvider').length, 1, 'otherProvider kernel info could not be found');
assert.deepStrictEqual(notebookService.getStandardKernelsForProvider('otherProvider')[0], otherProviderRegistration.standardKernels[0], 'otherProviderRegistration standard kernels does not match');
});

View File

@@ -21,8 +21,7 @@ export function getProvidersForFileName(fileName: string, notebookService: INote
let fileExt = path.extname(fileName);
let providers: string[];
// First try to get provider for actual file type
if (fileExt && fileExt.startsWith('.')) {
fileExt = fileExt.slice(1, fileExt.length);
if (fileExt) {
providers = notebookService.getProvidersForFileType(fileExt);
}
// Fallback to provider for default file type (assume this is a global handler)

View File

@@ -26,7 +26,7 @@ export const SERVICE_ID = 'sqlNotebookService';
export const INotebookService = createDecorator<INotebookService>(SERVICE_ID);
export const DEFAULT_NOTEBOOK_PROVIDER = 'builtin';
export const DEFAULT_NOTEBOOK_FILETYPE = 'IPYNB';
export const DEFAULT_NOTEBOOK_FILETYPE = '.ipynb';
export const SQL_NOTEBOOK_PROVIDER = 'sql';
export const OVERRIDE_EDITOR_THEMING_SETTING = 'notebook.overrideEditorTheming';

View File

@@ -311,7 +311,7 @@ export class NotebookService extends Disposable implements INotebookService {
if (!this._serializationProviders.has(p.id)) {
// Only add a new provider descriptor if the provider
// supports file extensions beyond the default ipynb
let addNewProvider = extensions.some(ext => ext?.length > 0 && ext.toUpperCase() !== DEFAULT_NOTEBOOK_FILETYPE);
let addNewProvider = extensions.some(ext => ext?.length > 0 && ext.toLowerCase() !== DEFAULT_NOTEBOOK_FILETYPE);
if (addNewProvider) {
this._serializationProviders.set(p.id, new SerializationProviderDescriptor(p.id));
}
@@ -378,12 +378,12 @@ export class NotebookService extends Disposable implements INotebookService {
}
private addFileProvider(fileType: string, provider: ProviderDescriptionRegistration) {
let providers = this._fileToProviderDescriptions.get(fileType.toUpperCase());
let providers = this._fileToProviderDescriptions.get(fileType.toLowerCase());
if (!providers) {
providers = [];
}
providers.push(provider);
this._fileToProviderDescriptions.set(fileType.toUpperCase(), providers);
this._fileToProviderDescriptions.set(fileType.toLowerCase(), providers);
}
// Standard kernels are contributed where a list of kernels are defined that can be shown
@@ -411,7 +411,7 @@ export class NotebookService extends Disposable implements INotebookService {
}
getProvidersForFileType(fileType: string): string[] | undefined {
let providers = this._fileToProviderDescriptions.get(fileType.toUpperCase());
let providers = this._fileToProviderDescriptions.get(fileType.toLowerCase());
return providers?.map(provider => provider.provider);
}