editors - return undefined from openEditor when operation cancell…

…ed (fix #134786)
microsoft · bpasero · Nov 1, 2021 · Oct 28, 2021 · Oct 28, 2021 · Oct 28, 2021
commit 6ed67dd61ebbd218892b3c47008cac33da0ac899
diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/window.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/window.test.ts
@@ -190,6 +190,29 @@ suite('vscode API - window', () => {
 		}
 	});
 
+	test.only('editor, opening multiple at the same time #134786', async () => {
+		const fileA = await createRandomFile();
+		const fileB = await createRandomFile();
+		const fileC = await createRandomFile();
+
+		const testFiles = [fileA, fileB, fileC];
+		const result = await Promise.all(testFiles.map(async testFile => {
+			try {
+				const doc = await workspace.openTextDocument(testFile);
+				const editor = await window.showTextDocument(doc);
+
+				return editor.document.uri;
+			} catch (error) {
+				return undefined;
+			}
+		}));
+
+		assert.strictEqual(result.length, 3);
+		assert.strictEqual(result[0], undefined);
+		assert.strictEqual(result[1], undefined);
+		assert.strictEqual(result[2]?.toString(), fileC.toString());
+	});
+
 	test('default column when opening a file', async () => {
 		const [docA, docB, docC] = await Promise.all([
 			workspace.openTextDocument(await createRandomFile()),

diff --git a/src/vs/workbench/browser/parts/editor/editorGroupView.ts b/src/vs/workbench/browser/parts/editor/editorGroupView.ts
@@ -1059,26 +1059,31 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
 		let openEditorPromise: Promise<IEditorPane | undefined>;
 		if (context.active) {
 			openEditorPromise = (async () => {
-				const result = await this.editorPane.openEditor(editor, options, { newInGroup: context.isNew });
+				const { pane, changed, cancelled, error } = await this.editorPane.openEditor(editor, options, { newInGroup: context.isNew });
+
+				// Return early if the operation was cancelled by another operation
+				if (cancelled) {
+					return undefined;
+				}
 
 				// Editor change event
-				if (result.editorChanged) {
+				if (changed) {
 					this._onDidGroupChange.fire({ kind: GroupChangeKind.EDITOR_ACTIVE, editor });
 				}
 
 				// Handle errors but do not bubble them up
-				if (result.error) {
-					await this.doHandleOpenEditorError(result.error, editor, options);
+				if (error) {
+					await this.doHandleOpenEditorError(error, editor, options);
 				}
 
 				// Without an editor pane, recover by closing the active editor
 				// (if the input is still the active one)
-				if (!result.editorPane && this.activeEditor === editor) {
+				if (!pane && this.activeEditor === editor) {
 					const focusNext = !options || !options.preserveFocus;
 					this.doCloseEditor(editor, focusNext, { fromError: true });
 				}
 
-				return result.editorPane;
+				return pane;
 			})();
 		} else {
 			openEditorPromise = Promise.resolve(undefined); // inactive: return undefined as result to signal this

diff --git a/src/vs/workbench/browser/parts/editor/editorPanes.ts b/src/vs/workbench/browser/parts/editor/editorPanes.ts
@@ -12,7 +12,7 @@ import { IEditorPaneRegistry, IEditorPaneDescriptor } from 'vs/workbench/browser
 import { IWorkbenchLayoutService } from 'vs/workbench/services/layout/browser/layoutService';
 import { EditorPane } from 'vs/workbench/browser/parts/editor/editorPane';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
-import { IEditorProgressService, LongRunningOperation } from 'vs/platform/progress/common/progress';
+import { IEditorProgressService, IOperation, LongRunningOperation } from 'vs/platform/progress/common/progress';
 import { IEditorGroupView, DEFAULT_EDITOR_MIN_DIMENSIONS, DEFAULT_EDITOR_MAX_DIMENSIONS } from 'vs/workbench/browser/parts/editor/editor';
 import { Emitter } from 'vs/base/common/event';
 import { assertIsDefined } from 'vs/base/common/types';
@@ -32,19 +32,27 @@ export interface IOpenEditorResult {
 	 * open the editor and in cases where no placeholder is being
 	 * used.
 	 */
-	readonly editorPane?: EditorPane;
+	readonly pane?: EditorPane;
 
 	/**
 	 * Whether the editor changed as a result of opening.
 	 */
-	readonly editorChanged?: boolean;
+	readonly changed?: boolean;
 
 	/**
 	 * This property is set when an editor fails to restore and
 	 * is shown with a generic place holder. It allows callers
 	 * to still present the error to the user in that case.
 	 */
 	readonly error?: Error;
+
+	/**
+	 * This property indicates whether the open editor operation was
+	 * cancelled or not. The operation may have been cancelled
+	 * in case another editor open operation was triggered right
+	 * after cancelling this one out.
+	 */
+	readonly cancelled?: boolean;
 }
 
 export class EditorPanes extends Disposable {
@@ -136,11 +144,32 @@ export class EditorPanes extends Disposable {
 	private async doOpenEditor(descriptor: IEditorPaneDescriptor, editor: EditorInput, options: IEditorOptions | undefined, context: IEditorOpenContext = Object.create(null)): Promise<IOpenEditorResult> {
 
 		// Editor pane
-		const editorPane = this.doShowEditorPane(descriptor);
+		const pane = this.doShowEditorPane(descriptor);
+
+		// Show progress while setting input after a certain timeout.
+		// If the workbench is opening be more relaxed about progress
+		// showing by increasing the delay a little bit to reduce flicker.
+		const operation = this.editorOperation.start(this.layoutService.isRestored() ? 800 : 3200);
 
 		// Apply input to pane
-		const editorChanged = await this.doSetInput(editorPane, editor, options, context);
-		return { editorPane, editorChanged };
+		let changed: boolean;
+		let cancelled: boolean;
+		try {
+			changed = await this.doSetInput(pane, operation, editor, options, context);
+			cancelled = !operation.isCurrent();
+		} finally {
+			operation.stop();
+		}
+
+		// Focus unless cancelled
+		if (!cancelled) {
+			const focus = !options || !options.preserveFocus;
+			if (focus) {
+				pane.focus();
+			}
+		}
+
+		return { pane, changed, cancelled };
 	}
 
 	private getEditorPaneDescriptor(editor: EditorInput): IEditorPaneDescriptor {
@@ -234,12 +263,12 @@ export class EditorPanes extends Disposable {
 		this._onDidChangeSizeConstraints.fire(undefined);
 	}
 
-	private async doSetInput(editorPane: EditorPane, editor: EditorInput, options: IEditorOptions | undefined, context: IEditorOpenContext): Promise<boolean> {
+	private async doSetInput(editorPane: EditorPane, operation: IOperation, editor: EditorInput, options: IEditorOptions | undefined, context: IEditorOpenContext): Promise<boolean> {
 
 		// If the input did not change, return early and only apply the options
 		// unless the options instruct us to force open it even if it is the same
 		const forceReload = options?.forceReload;
-		const inputMatches = editorPane.input && editorPane.input.matches(editor);
+		const inputMatches = editorPane.input?.matches(editor);
 		if (inputMatches && !forceReload) {
 
 			// Forward options
@@ -254,27 +283,10 @@ export class EditorPanes extends Disposable {
 			return false;
 		}
 
-		// Show progress while setting input after a certain timeout. If the workbench is opening
-		// be more relaxed about progress showing by increasing the delay a little bit to reduce flicker.
-		const operation = this.editorOperation.start(this.layoutService.isRestored() ? 800 : 3200);
-
 		// Call into editor pane
-		const editorWillChange = !inputMatches;
-		try {
-			await editorPane.setInput(editor, options, context, operation.token);
-
-			// Focus (unless prevented or another operation is running)
-			if (operation.isCurrent()) {
-				const focus = !options || !options.preserveFocus;
-				if (focus) {
-					editorPane.focus();
-				}
-			}
+		await editorPane.setInput(editor, options, context, operation.token);
 
-			return editorWillChange;
-		} finally {
-			operation.stop();
-		}
+		return !inputMatches;
 	}
 
 	private doHideActiveEditorPane(): void {

diff --git a/src/vs/workbench/services/editor/test/browser/editorService.test.ts b/src/vs/workbench/services/editor/test/browser/editorService.test.ts
@@ -164,6 +164,38 @@ suite('EditorService', () => {
 		didCloseEditorListener.dispose();
 	});
 
+	test('openEditor() - multiple calls are cancelled and indicated as such', async () => {
+		const [, service] = await createEditorService();
+
+		let input = new TestFileEditorInput(URI.parse('my://resource-basics'), TEST_EDITOR_INPUT_ID);
+		let otherInput = new TestFileEditorInput(URI.parse('my://resource2-basics'), TEST_EDITOR_INPUT_ID);
+
+		let activeEditorChangeEventCounter = 0;
+		const activeEditorChangeListener = service.onDidActiveEditorChange(() => {
+			activeEditorChangeEventCounter++;
+		});
+
+		let visibleEditorChangeEventCounter = 0;
+		const visibleEditorChangeListener = service.onDidVisibleEditorsChange(() => {
+			visibleEditorChangeEventCounter++;
+		});
+
+		const editorP1 = service.openEditor(input, { pinned: true });
+		const editorP2 = service.openEditor(otherInput, { pinned: true });
+
+		const editor1 = await editorP1;
+		assert.strictEqual(editor1, undefined);
+
+		const editor2 = await editorP2;
+		assert.strictEqual(editor2?.input, otherInput);
+
+		assert.strictEqual(activeEditorChangeEventCounter, 1);
+		assert.strictEqual(visibleEditorChangeEventCounter, 1);
+
+		activeEditorChangeListener.dispose();
+		visibleEditorChangeListener.dispose();
+	});
+
 	test('openEditor() - locked groups', async () => {
 		disposables.add(registerTestFileEditor());