From 0d3112ef35163387cdd0653247f46b9a1c274019 Mon Sep 17 00:00:00 2001 From: Chris LaFreniere <40371649+chlafreniere@users.noreply.github.com> Date: Fri, 2 Apr 2021 14:49:52 -0700 Subject: [PATCH] Adding Rendered Notebook Diff Option (#14860) * First attempt nb diff preview * First attempt nb diff preview * Simplify everything * Interim scroll one way * Double scroll * Add setting * Add tests * Add comment * Fix tests * first round PR comments * Ensure scrollable portion has scrollbar in diff * Fix sqllint errors, register events * Fix scrolling, readonly --- .../common/editorReplacerContribution.ts | 10 +- .../editorReplacerContribution.test.ts | 35 ++++++ .../browser/models/diffNotebookInput.ts | 111 ++++++++++++++++++ .../browser/models/nodebookInputFactory.ts | 16 ++- .../notebook/browser/models/notebookInput.ts | 11 ++ .../notebook/browser/notebook.contribution.ts | 7 +- .../notebook/browser/notebookEditor.ts | 1 + .../query/browser/queryInputFactory.ts | 5 +- .../notebook/browser/models/notebookModel.ts | 2 +- 9 files changed, 186 insertions(+), 12 deletions(-) create mode 100644 src/sql/workbench/contrib/notebook/browser/models/diffNotebookInput.ts diff --git a/src/sql/workbench/contrib/editorReplacement/common/editorReplacerContribution.ts b/src/sql/workbench/contrib/editorReplacement/common/editorReplacerContribution.ts index aad0c6bda4..b92df3b67d 100644 --- a/src/sql/workbench/contrib/editorReplacement/common/editorReplacerContribution.ts +++ b/src/sql/workbench/contrib/editorReplacement/common/editorReplacerContribution.ts @@ -18,6 +18,7 @@ import { ILanguageAssociationRegistry, Extensions as LanguageAssociationExtensio import { UntitledTextEditorInput } from 'vs/workbench/services/untitled/common/untitledTextEditorInput'; import { isThenable } from 'vs/base/common/async'; import { withNullAsUndefined } from 'vs/base/common/types'; +import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput'; const languageAssociationRegistry = Registry.as(LanguageAssociationExtensions.LanguageAssociations); @@ -41,7 +42,7 @@ export class EditorReplacementContribution implements IWorkbenchContribution { // return undefined; // } - if (!(editor instanceof FileEditorInput) && !(editor instanceof UntitledTextEditorInput)) { + if (!(editor instanceof FileEditorInput) && !(editor instanceof UntitledTextEditorInput) && !(editor instanceof DiffEditorInput)) { return undefined; } @@ -57,13 +58,14 @@ export class EditorReplacementContribution implements IWorkbenchContribution { } if (!language) { - // just use the extension - language = path.extname(editor.resource.toString()).slice(1); // remove the . + // Attempt to use extension or extension of modified input (if in diff editor) + // remove the . + language = editor instanceof DiffEditorInput ? path.extname(editor.modifiedInput.resource.toString()).slice(1) : path.extname(editor.resource.toString()).slice(1); } if (!language) { const defaultInputCreator = languageAssociationRegistry.defaultAssociation; - if (defaultInputCreator) { + if (defaultInputCreator && !(editor instanceof DiffEditorInput)) { editor.setMode(defaultInputCreator[0]); const newInput = defaultInputCreator[1].convertInput(editor); if (newInput) { diff --git a/src/sql/workbench/contrib/editorReplacement/test/browser/editorReplacerContribution.test.ts b/src/sql/workbench/contrib/editorReplacement/test/browser/editorReplacerContribution.test.ts index b6f8fbe47b..031347f900 100644 --- a/src/sql/workbench/contrib/editorReplacement/test/browser/editorReplacerContribution.test.ts +++ b/src/sql/workbench/contrib/editorReplacement/test/browser/editorReplacerContribution.test.ts @@ -32,6 +32,10 @@ import { IUntitledTextEditorService } from 'vs/workbench/services/untitled/commo import { IQueryEditorService } from 'sql/workbench/services/queryEditor/common/queryEditorService'; import { TestQueryEditorService } from 'sql/workbench/services/queryEditor/test/common/testQueryEditorService'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; +import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput'; +import { DiffNotebookInput } from 'sql/workbench/contrib/notebook/browser/models/diffNotebookInput'; +import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; +import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService'; const languageAssociations = Registry.as(LanguageAssociationExtensions.LanguageAssociations); @@ -47,6 +51,9 @@ suite('Editor Replacer Contribution', () => { const editorService = new MockEditorService(instantiationService); instantiationService.stub(IEditorService, editorService); instantiationService.stub(IQueryEditorService, instantiationService.createInstance(TestQueryEditorService)); + const configService = new TestConfigurationService(); + configService.setUserConfiguration('notebook', { showRenderedNotebookInDiffEditor: true }); + instantiationService.stub(IConfigurationService, configService); instantiationService.invokeFunction(accessor => { languageAssociations.start(accessor); }); @@ -126,6 +133,34 @@ suite('Editor Replacer Contribution', () => { contrib.dispose(); }); + test('does replace notebook file diff input using input extension ipynb', async () => { + const instantiationService = workbenchInstantiationService(); + const editorService = new MockEditorService(instantiationService); + instantiationService.stub(IEditorService, editorService); + const contrib = instantiationService.createInstance(EditorReplacementContribution); + const input = instantiationService.createInstance(FileEditorInput, URI.file('/test/file.ipynb'), undefined, undefined, undefined, undefined, undefined); + const input2 = instantiationService.createInstance(FileEditorInput, URI.file('/test/file2.ipynb'), undefined, undefined, undefined, undefined, undefined); + const diffInput = instantiationService.createInstance(DiffEditorInput, undefined, undefined, input, input2, undefined); + const response = editorService.fireOpenEditor(diffInput, undefined, undefined as IEditorGroup, OpenEditorContext.NEW_EDITOR); + const newinput = await response.override; // our test service returns this so we are fine to cast this + assert(newinput instanceof DiffNotebookInput); + contrib.dispose(); + }); + + test('does not replace sql file diff input using input extension sql', async () => { + const instantiationService = workbenchInstantiationService(); + const editorService = new MockEditorService(instantiationService); + instantiationService.stub(IEditorService, editorService); + const contrib = instantiationService.createInstance(EditorReplacementContribution); + const input = instantiationService.createInstance(FileEditorInput, URI.file('/test/file.sql'), undefined, undefined, undefined, undefined, undefined); + const input2 = instantiationService.createInstance(FileEditorInput, URI.file('/test/file2.sql'), undefined, undefined, undefined, undefined, undefined); + const diffInput = instantiationService.createInstance(DiffEditorInput, undefined, undefined, input, input2, undefined); + const response = editorService.fireOpenEditor(diffInput, undefined, undefined as IEditorGroup, OpenEditorContext.NEW_EDITOR); + const newinput = await response.override; + assert(newinput instanceof DiffEditorInput); + contrib.dispose(); + }); + test('does replace file input using default mode', async function () { const instantiationService = workbenchInstantiationService(); const editorService = new MockEditorService(instantiationService); diff --git a/src/sql/workbench/contrib/notebook/browser/models/diffNotebookInput.ts b/src/sql/workbench/contrib/notebook/browser/models/diffNotebookInput.ts new file mode 100644 index 0000000000..f03fe35a27 --- /dev/null +++ b/src/sql/workbench/contrib/notebook/browser/models/diffNotebookInput.ts @@ -0,0 +1,111 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the Source EULA. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { FileEditorInput } from 'vs/workbench/contrib/files/common/editors/fileEditorInput'; +import { SideBySideEditorInput } from 'vs/workbench/common/editor'; +import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput'; +import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; +import { FileNotebookInput } from 'sql/workbench/contrib/notebook/browser/models/fileNotebookInput'; +import { INotebookService } from 'sql/workbench/services/notebook/browser/notebookService'; +import { Deferred } from 'sql/base/common/promise'; +import { ILogService } from 'vs/platform/log/common/log'; + +export class DiffNotebookInput extends SideBySideEditorInput { + public static ID: string = 'workbench.editorinputs.DiffNotebookInput'; + private _notebookService: INotebookService; + private _logService: ILogService; + + constructor( + title: string, + diffInput: DiffEditorInput, + @IInstantiationService instantiationService: IInstantiationService, + @INotebookService notebookService: INotebookService, + @ILogService logService: ILogService + ) { + let originalInput = instantiationService.createInstance(FileNotebookInput, diffInput.primary.getName(), diffInput.primary.resource, diffInput.originalInput as FileEditorInput); + let modifiedInput = instantiationService.createInstance(FileNotebookInput, diffInput.secondary.getName(), diffInput.secondary.resource, diffInput.modifiedInput as FileEditorInput); + super(title, diffInput.getTitle(), modifiedInput, originalInput); + this._notebookService = notebookService; + this._logService = logService; + this.setupScrollListeners(originalInput, modifiedInput); + } + + public getTypeId(): string { + return DiffNotebookInput.ID; + } + + /** + * Setup scroll listeners so that both the original and modified editors scroll together + * @param originalInput original notebook input + * @param modifiedInput modified notebook input + */ + private setupScrollListeners(originalInput: FileNotebookInput, modifiedInput: FileNotebookInput): void { + Promise.all([originalInput.containerResolved, modifiedInput.containerResolved]).then(() => { + + // Setting container height to 100% ensures that scrollbars will be added when in diff mode + originalInput.container.parentElement.style.height = '100%'; + modifiedInput.container.parentElement.style.height = '100%'; + + // Keep track of when original and modified notebooks are shown + const originalNotebookEditorShown: Deferred = new Deferred(); + const modifiedNotebookEditorShown: Deferred = new Deferred(); + + // Possible for notebooks to have been shown already, so check this case + if (this._notebookService.findNotebookEditor(originalInput.notebookUri)) { + originalNotebookEditorShown.resolve(); + } else if (this._notebookService.findNotebookEditor(modifiedInput.notebookUri)) { + modifiedNotebookEditorShown.resolve(); + } + + // If not already shown, listen for add events + this._register(this._notebookService.onNotebookEditorAdd((e) => { + if (e.notebookParams.input === originalInput) { + originalNotebookEditorShown.resolve(); + } else if (e.notebookParams.input === modifiedInput) { + modifiedNotebookEditorShown.resolve(); + } + })); + + // Once both are shown, look for scrollable DIV. Add scroll listeners here + Promise.all([originalNotebookEditorShown.promise, modifiedNotebookEditorShown.promise]).then(() => { + const originalScrollableNode = originalInput.container?.querySelector('.scrollable'); + const modifiedScrollableNode = modifiedInput.container?.querySelector('.scrollable'); + + if (originalScrollableNode && modifiedScrollableNode) { + + // origTarget/modTarget track the scrollTop for the other editor. + // This ensures that events are getting fired while a scroll is ongoing, which can + // result in scrolling speed that seems much slower than expected + let origTarget: number | undefined; + let modTarget: number | undefined; + originalScrollableNode.addEventListener('scroll', () => { + if (origTarget !== undefined) { + if (origTarget === originalScrollableNode.scrollTop) { + origTarget = undefined; + } + return; + } + modTarget = originalScrollableNode.scrollTop; + modifiedScrollableNode.scroll({ top: originalScrollableNode.scrollTop }); + }); + modifiedScrollableNode.addEventListener('scroll', () => { + if (modTarget !== undefined) { + if (modTarget === modifiedScrollableNode.scrollTop) { + modTarget = undefined; + } + return; + } + origTarget = modifiedScrollableNode.scrollTop; + originalScrollableNode.scroll({ top: modifiedScrollableNode.scrollTop }); + }); + } + }).catch(error => { + this._logService.error(`Issue encountered waiting for original and modified notebook editors to be shown in notebook diff input: ${error}`); + }); + }).catch(error => { + this._logService.error(`Issue encountered waiting for original and modified notebook containers to exist in notebook diff input: ${error}`); + }); + } +} diff --git a/src/sql/workbench/contrib/notebook/browser/models/nodebookInputFactory.ts b/src/sql/workbench/contrib/notebook/browser/models/nodebookInputFactory.ts index 513c19af55..7461201baf 100644 --- a/src/sql/workbench/contrib/notebook/browser/models/nodebookInputFactory.ts +++ b/src/sql/workbench/contrib/notebook/browser/models/nodebookInputFactory.ts @@ -13,22 +13,30 @@ import { FileEditorInput } from 'vs/workbench/contrib/files/common/editors/fileE import { UntitledTextEditorInput } from 'vs/workbench/services/untitled/common/untitledTextEditorInput'; import { ILanguageAssociation } from 'sql/workbench/services/languageAssociation/common/languageAssociation'; import { NotebookInput } from 'sql/workbench/contrib/notebook/browser/models/notebookInput'; +import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput'; +import { DiffNotebookInput } from 'sql/workbench/contrib/notebook/browser/models/diffNotebookInput'; +import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; const editorInputFactoryRegistry = Registry.as(EditorInputExtensions.EditorInputFactories); export class NotebookEditorInputAssociation implements ILanguageAssociation { static readonly languages = ['notebook', 'ipynb']; - constructor(@IInstantiationService private readonly instantiationService: IInstantiationService) { } + constructor(@IInstantiationService private readonly instantiationService: IInstantiationService, @IConfigurationService private readonly configurationService: IConfigurationService) { } - convertInput(activeEditor: IEditorInput): NotebookInput { + convertInput(activeEditor: IEditorInput): NotebookInput | DiffNotebookInput { if (activeEditor instanceof FileEditorInput) { return this.instantiationService.createInstance(FileNotebookInput, activeEditor.getName(), activeEditor.resource, activeEditor); } else if (activeEditor instanceof UntitledTextEditorInput) { return this.instantiationService.createInstance(UntitledNotebookInput, activeEditor.getName(), activeEditor.resource, activeEditor); - } else { - return undefined; + } else if (activeEditor instanceof DiffEditorInput) { + // Only show rendered notebook in diff editor if setting is true (otherwise, defaults back to text diff) + // Note: intentionally not listening to config changes here, given that inputs won't be converted dynamically if the setting is changed + if (this.configurationService.getValue('notebook.showRenderedNotebookInDiffEditor') === true) { + return this.instantiationService.createInstance(DiffNotebookInput, activeEditor.getName(), activeEditor); + } } + return undefined; } createBase(activeEditor: NotebookInput): IEditorInput { diff --git a/src/sql/workbench/contrib/notebook/browser/models/notebookInput.ts b/src/sql/workbench/contrib/notebook/browser/models/notebookInput.ts index fedd335173..d854508b9c 100644 --- a/src/sql/workbench/contrib/notebook/browser/models/notebookInput.ts +++ b/src/sql/workbench/contrib/notebook/browser/models/notebookInput.ts @@ -118,6 +118,11 @@ export class NotebookEditorModel extends EditorModel { } public updateModel(contentChange?: NotebookContentChange, type?: NotebookChangeType): void { + // If text editor model is readonly, exit early as no changes need to occur on the model + // Note: this follows what happens in fileCommands where update/save logic is skipped for readonly text editor models + if (this.textEditorModel?.isReadonly()) { + return; + } if (type === NotebookChangeType.KernelChanged && this._isFirstKernelChange) { this._isFirstKernelChange = false; return; @@ -222,6 +227,7 @@ export abstract class NotebookInput extends EditorInput { private _notebookEditorOpenedTimestamp: number; private _modelResolveInProgress: boolean = false; private _modelResolved: Deferred = new Deferred(); + private _containerResolved: Deferred = new Deferred(); private _notebookFindModel: NotebookFindModel; @@ -456,12 +462,17 @@ export abstract class NotebookInput extends EditorInput { set container(container: HTMLElement) { this._disposeContainer(); this._parentContainer = container; + this._containerResolved.resolve(); } get container(): HTMLElement { return this._parentContainer; } + get containerResolved(): Promise { + return this._containerResolved.promise; + } + /** * An editor that is dirty will be asked to be saved once it closes. */ diff --git a/src/sql/workbench/contrib/notebook/browser/notebook.contribution.ts b/src/sql/workbench/contrib/notebook/browser/notebook.contribution.ts index ccbf798929..b268bb4383 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebook.contribution.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebook.contribution.ts @@ -289,7 +289,12 @@ configurationRegistry.registerConfiguration({ 'default': 1.5, 'minimum': 1, 'description': localize('notebook.markdownPreviewLineHeight', "Controls the line height used in the notebook markdown preview. This number is relative to the font size.") - } + }, + 'notebook.showRenderedNotebookInDiffEditor': { + 'type': 'boolean', + 'default': false, + 'description': localize('notebook.showRenderedNotebookinDiffEditor', "(Preview) Show rendered notebook in diff editor.") + }, } }); diff --git a/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts b/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts index ecce0db4de..188115eb86 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebookEditor.ts @@ -210,6 +210,7 @@ export class NotebookEditor extends EditorPane implements IFindNotebookControlle } } + private async setFindInput(parentElement: HTMLElement): Promise { parentElement.appendChild(this._overlay); await this.setNotebookModel(); diff --git a/src/sql/workbench/contrib/query/browser/queryInputFactory.ts b/src/sql/workbench/contrib/query/browser/queryInputFactory.ts index 275282d631..3359a1892b 100644 --- a/src/sql/workbench/contrib/query/browser/queryInputFactory.ts +++ b/src/sql/workbench/contrib/query/browser/queryInputFactory.ts @@ -38,6 +38,9 @@ export class QueryEditorLanguageAssociation implements ILanguageAssociation { @IQueryEditorService private readonly queryEditorService: IQueryEditorService) { } async convertInput(activeEditor: IEditorInput): Promise { + if (!(activeEditor instanceof FileEditorInput) && !(activeEditor instanceof UntitledTextEditorInput)) { + return undefined; + } const queryResultsInput = this.instantiationService.createInstance(QueryResultsInput, activeEditor.resource.toString(true)); let queryEditorInput: QueryEditorInput; if (activeEditor instanceof FileEditorInput) { @@ -45,8 +48,6 @@ export class QueryEditorLanguageAssociation implements ILanguageAssociation { } else if (activeEditor instanceof UntitledTextEditorInput) { const content = (await activeEditor.resolve()).textEditorModel.getValue(); queryEditorInput = await this.queryEditorService.newSqlEditor({ resource: this.editorService.isOpen(activeEditor) ? activeEditor.resource : undefined, open: false, initalContent: content }) as UntitledQueryEditorInput; - } else { - return undefined; } const profile = getCurrentGlobalConnection(this.objectExplorerService, this.connectionManagementService, this.editorService); diff --git a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts index 904773718e..bc47e097d0 100644 --- a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts +++ b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts @@ -434,7 +434,7 @@ export class NotebookModel extends Disposable implements INotebookModel { }); } // Only add new parameter cell if notebookUri Parameters are found - if (notebookUriParams) { + if (notebookUriParams && this.notebookUri?.scheme !== 'git') { this.addUriParameterCell(notebookUriParams, hasParameterCell, parameterCellIndex, hasInjectedCell); } }