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

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<ILanguageAssociationRegistry>(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) {

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<ILanguageAssociationRegistry>(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);

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<void> = new Deferred<void>();
+			const modifiedNotebookEditorShown: Deferred<void> = new Deferred<void>();
+
+			// 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}`);
+		});
+	}
+}

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<IEditorInputFactoryRegistry>(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 {

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<void> = new Deferred<void>();
+	private _containerResolved: Deferred<void> = new Deferred<void>();
 
 	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<void> {
+		return this._containerResolved.promise;
+	}
+
 	/**
 	 * An editor that is dirty will be asked to be saved once it closes.
 	 */

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.")
+		},
 	}
 });
 

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<void> {
 		parentElement.appendChild(this._overlay);
 		await this.setNotebookModel();

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<QueryEditorInput | undefined> {
+		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);

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);
 				}
 			}