From 6ed67dd61ebbd218892b3c47008cac33da0ac899 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Thu, 28 Oct 2021 09:23:08 +0200 Subject: [PATCH] editors - return `undefined` from `openEditor` when operation cancelled (fix #134786) --- .../src/singlefolder-tests/window.test.ts | 23 +++++++ .../browser/parts/editor/editorGroupView.ts | 17 +++-- .../browser/parts/editor/editorPanes.ts | 66 +++++++++++-------- .../editor/test/browser/editorService.test.ts | 32 +++++++++ 4 files changed, 105 insertions(+), 33 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/window.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/window.test.ts index b89ee7b3593..7c62aae9436 100644 --- 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 index 53179f98fb8..b446ee591ad 100644 --- 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; 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 index ccb86f9477d..e5abcd3d767 100644 --- 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,12 +32,12 @@ 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 @@ -45,6 +45,14 @@ export interface IOpenEditorResult { * 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 { // 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 { + private async doSetInput(editorPane: EditorPane, operation: IOperation, editor: EditorInput, options: IEditorOptions | undefined, context: IEditorOpenContext): Promise { // 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); + 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(); - } - } - - 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 index d406b6c4699..5445b7ff1d2 100644 --- 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());