From 06bb703b9eb6209aa6e42a6a5b5896ff6c70dc2d Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Fri, 27 Dec 2019 09:26:29 +0100 Subject: [PATCH] Editor limit: can end up opening a disposed editor if limit is 1 (fixes #87447) --- .../browser/parts/editor/editorGroupView.ts | 11 ++++- .../editor/test/browser/editorService.test.ts | 49 ++++++++++++++----- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/vs/workbench/browser/parts/editor/editorGroupView.ts b/src/vs/workbench/browser/parts/editor/editorGroupView.ts index 4bce5420b38..2e06af3dc2e 100644 --- a/src/vs/workbench/browser/parts/editor/editorGroupView.ts +++ b/src/vs/workbench/browser/parts/editor/editorGroupView.ts @@ -821,6 +821,13 @@ export class EditorGroupView extends Themable implements IEditorGroupView { private doOpenEditor(editor: EditorInput, options?: EditorOptions): Promise { + // Guard against invalid inputs. Disposed inputs + // should never open because they emit no events + // e.g. to indicate dirty changes. + if (editor.isDisposed()) { + return Promise.resolve(undefined); + } + // Determine options const openEditorOptions: IEditorOpenOptions = { index: options ? options.index : undefined, @@ -1492,10 +1499,10 @@ export class EditorGroupView extends Themable implements IEditorGroupView { }); // Handle inactive first - inactiveReplacements.forEach(({ editor, replacement, options }) => { + inactiveReplacements.forEach(async ({ editor, replacement, options }) => { // Open inactive editor - this.doOpenEditor(replacement, options); + await this.doOpenEditor(replacement, options); // Close replaced inactive editor unless they match if (!editor.matches(replacement)) { 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 dccd154b670..b36462a5491 100644 --- a/src/vs/workbench/services/editor/test/browser/editorService.test.ts +++ b/src/vs/workbench/services/editor/test/browser/editorService.test.ts @@ -131,8 +131,8 @@ suite('EditorService', () => { const service: EditorServiceImpl = testInstantiationService.createInstance(EditorService); - const input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-basics')); - const otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-basics')); + let input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-basics')); + let otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-basics')); let activeEditorChangeEventCounter = 0; const activeEditorChangeListener = service.onDidActiveEditorChange(() => { @@ -180,7 +180,14 @@ suite('EditorService', () => { assert.equal(visibleEditorChangeEventCounter, 2); assert.ok(input.gotDisposed); - // Open again 2 inputs + // Open again 2 inputs (disposed editors are ignored!) + await service.openEditor(input, { pinned: true }); + assert.equal(0, service.count); + + // Open again 2 inputs (recreate because disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-basics')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-basics')); + await service.openEditor(input, { pinned: true }); editor = await service.openEditor(otherInput, { pinned: true }); @@ -511,8 +518,8 @@ suite('EditorService', () => { const service: EditorServiceImpl = testInstantiationService.createInstance(EditorService); - const input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); - const otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); + let input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + let otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); let activeEditorChangeEventFired = false; const activeEditorChangeListener = service.onDidActiveEditorChange(() => { @@ -563,7 +570,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 2.) open, open same (forced open) + // 2.) open, open same (forced open) (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -574,7 +583,9 @@ suite('EditorService', () => { await closeEditorAndWaitForNextToOpen(group, input); - // 3.) open, open inactive, close + // 3.) open, open inactive, close (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -587,7 +598,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 4.) open, open inactive, close inactive + // 4.) open, open inactive, close inactive (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -604,7 +617,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 5.) add group, remove group + // 5.) add group, remove group (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -625,7 +640,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 6.) open editor in inactive group + // 6.) open editor in inactive group (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -646,7 +663,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 7.) activate group + // 7.) activate group (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -671,7 +690,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 8.) move editor + // 8.) move editor (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); @@ -688,7 +709,9 @@ suite('EditorService', () => { assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true); - // 9.) close editor in inactive group + // 9.) close editor in inactive group (recreate inputs that got disposed) + input = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource-active')); + otherInput = testInstantiationService.createInstance(TestEditorInput, URI.parse('my://resource2-active')); editor = await service.openEditor(input, { pinned: true }); assertActiveEditorChangedEvent(true); assertVisibleEditorsChangedEvent(true);