From fb60d95290c7c1225596543d93053d64169ae021 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Wed, 8 Sep 2021 08:18:15 +0200 Subject: [PATCH] editors - reuse existing side by side editor when identical input opened (#36700) --- .../browser/parts/editor/editorGroupView.ts | 12 +- .../common/editor/editorGroupModel.ts | 77 ++++++++--- .../parts/editor/editorGroupModel.test.ts | 122 +++++++++++++++--- 3 files changed, 169 insertions(+), 42 deletions(-) diff --git a/src/vs/workbench/browser/parts/editor/editorGroupView.ts b/src/vs/workbench/browser/parts/editor/editorGroupView.ts index d82a138ea3d56..3ef28989cceb7 100644 --- a/src/vs/workbench/browser/parts/editor/editorGroupView.ts +++ b/src/vs/workbench/browser/parts/editor/editorGroupView.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import 'vs/css!./media/editorgroupview'; -import { EditorGroupModel, IEditorOpenOptions, EditorCloseEvent, ISerializedEditorGroupModel, isSerializedEditorGroupModel } from 'vs/workbench/common/editor/editorGroupModel'; +import { EditorGroupModel, IEditorOpenOptions, EditorCloseEvent, ISerializedEditorGroupModel, isSerializedEditorGroupModel, SideBySideMatchingStrategy } from 'vs/workbench/common/editor/editorGroupModel'; import { GroupIdentifier, CloseDirection, IEditorCloseEvent, ActiveEditorDirtyContext, IEditorPane, EditorGroupEditorsCountContext, SaveReason, IEditorPartOptionsChangeEvent, EditorsOrder, IVisibleEditorPane, ActiveEditorStickyContext, ActiveEditorPinnedContext, EditorResourceAccessor, IEditorMoveEvent, EditorInputCapabilities, IEditorOpenEvent, IUntypedEditorInput, DEFAULT_EDITOR_ASSOCIATION, ActiveEditorGroupLockedContext, IEditorInput } from 'vs/workbench/common/editor'; import { EditorInput } from 'vs/workbench/common/editor/editorInput'; import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput'; @@ -613,8 +613,8 @@ export class EditorGroupView extends Themable implements IEditorGroupView { private canDispose(editor: IEditorInput): boolean { for (const groupView of this.accessor.groups) { if (groupView instanceof EditorGroupView && groupView.model.contains(editor, { - strictEquals: true, // only if this input is not shared across editor groups - supportSideBySide: true // include side by side editor primary & secondary + strictEquals: true, // only if this input is not shared across editor groups + supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE // include any side of an opened side by side editor })) { return false; } @@ -1031,19 +1031,19 @@ export class EditorGroupView extends Themable implements IEditorGroupView { let lock = false; // By `typeId` - if (this.accessor.partOptions.experimentalAutoLockGroups?.has(editor.typeId)) { + if (this.accessor.partOptions.experimentalAutoLockGroups?.has(openedEditor.typeId)) { lock = true; } // By `editorId` - else if (editor.editorId && this.accessor.partOptions.experimentalAutoLockGroups?.has(editor.editorId)) { + else if (openedEditor.editorId && this.accessor.partOptions.experimentalAutoLockGroups?.has(openedEditor.editorId)) { lock = true; } // By `viewType` (TODO@bpasero remove this hack once editors have adopted `editorId`) // See https://github.com/microsoft/vscode/issues/131692 else { - const editorViewType = (editor as { viewType?: string }).viewType; + const editorViewType = (openedEditor as { viewType?: string }).viewType; if (editorViewType && this.accessor.partOptions.experimentalAutoLockGroups?.has(editorViewType)) { lock = true; } diff --git a/src/vs/workbench/common/editor/editorGroupModel.ts b/src/vs/workbench/common/editor/editorGroupModel.ts index 649bce16cc60a..e420635016e32 100644 --- a/src/vs/workbench/common/editor/editorGroupModel.ts +++ b/src/vs/workbench/common/editor/editorGroupModel.ts @@ -61,6 +61,37 @@ export function isSerializedEditorGroupModel(group?: unknown): group is ISeriali return !!(candidate && typeof candidate === 'object' && Array.isArray(candidate.editors) && Array.isArray(candidate.mru)); } +export enum SideBySideMatchingStrategy { + + /** + * Consider an editor to match a side by side + * editor if any of the two sides match. + */ + ANY_SIDE = 1, + + /** + * Consider an editor to match a side by side + * editor if both sides match. + */ + BOTH_SIDES +} + +export interface IMatchOptions { + + /** + * Whether to support side by side editors when + * considering a match. + */ + supportSideBySide?: SideBySideMatchingStrategy; + + /** + * Only consider an editor to match when the + * `candidate === editor` but not when + * `candidate.matches(editor)`. + */ + strictEquals?: boolean; +} + export class EditorGroupModel extends Disposable { private static IDS = 0; @@ -189,7 +220,13 @@ export class EditorGroupModel extends Disposable { const makePinned = options?.pinned || options?.sticky; const makeActive = options?.active || !this.activeEditor || (!makePinned && this.matches(this.preview, this.activeEditor)); - const existingEditorAndIndex = this.findEditor(candidate); + const existingEditorAndIndex = this.findEditor(candidate, { + // Allow to match on a side-by-side editor when same + // editor is opened on both sides. In that case we + // do not want to open a new editor but reuse that one. + // For: https://github.com/microsoft/vscode/issues/36700 + supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES + }); // New editor if (!existingEditorAndIndex) { @@ -687,13 +724,13 @@ export class EditorGroupModel extends Disposable { } } - indexOf(candidate: IEditorInput | null, editors = this.editors): number { + indexOf(candidate: IEditorInput | null, editors = this.editors, options?: IMatchOptions): number { if (!candidate) { return -1; } for (let i = 0; i < editors.length; i++) { - if (this.matches(editors[i], candidate)) { + if (this.matches(editors[i], candidate, options)) { return i; } } @@ -701,8 +738,8 @@ export class EditorGroupModel extends Disposable { return -1; } - private findEditor(candidate: EditorInput | null): [EditorInput, number /* index */] | undefined { - const index = this.indexOf(candidate, this.editors); + private findEditor(candidate: EditorInput | null, options?: IMatchOptions): [EditorInput, number /* index */] | undefined { + const index = this.indexOf(candidate, this.editors, options); if (index === -1) { return undefined; } @@ -710,31 +747,41 @@ export class EditorGroupModel extends Disposable { return [this.editors[index], index]; } - contains(candidate: EditorInput | IUntypedEditorInput, options?: { supportSideBySide?: boolean, strictEquals?: boolean }): boolean { + contains(candidate: EditorInput | IUntypedEditorInput, options?: IMatchOptions): boolean { for (const editor of this.editors) { - if (this.matches(editor, candidate, options?.strictEquals)) { + if (this.matches(editor, candidate, options)) { return true; } - - if (options?.supportSideBySide && editor instanceof SideBySideEditorInput) { - if (this.matches(editor.primary, candidate, options?.strictEquals) || this.matches(editor.secondary, candidate, options?.strictEquals)) { - return true; - } - } } return false; } - private matches(editor: IEditorInput | null, candidate: IEditorInput | IUntypedEditorInput | null, strictEquals?: boolean): boolean { + private matches(editor: IEditorInput | null, candidate: IEditorInput | IUntypedEditorInput | null, options?: IMatchOptions): boolean { if (!editor || !candidate) { return false; } - if (strictEquals) { + if (options?.strictEquals) { return editor === candidate; } + if (options?.supportSideBySide && editor instanceof SideBySideEditorInput && !(candidate instanceof SideBySideEditorInput)) { + switch (options.supportSideBySide) { + case SideBySideMatchingStrategy.ANY_SIDE: + if (this.matches(editor.primary, candidate, options) || this.matches(editor.secondary, candidate, options)) { + return true; + } + break; + case SideBySideMatchingStrategy.BOTH_SIDES: + if (this.matches(editor.primary, candidate, options) && this.matches(editor.secondary, candidate, options)) { + return true; + } + break; + } + + } + return editor.matches(candidate); } diff --git a/src/vs/workbench/test/browser/parts/editor/editorGroupModel.test.ts b/src/vs/workbench/test/browser/parts/editor/editorGroupModel.test.ts index e4511889ba271..fb4d67aeef604 100644 --- a/src/vs/workbench/test/browser/parts/editor/editorGroupModel.test.ts +++ b/src/vs/workbench/test/browser/parts/editor/editorGroupModel.test.ts @@ -4,8 +4,8 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; -import { EditorGroupModel, ISerializedEditorGroupModel, EditorCloseEvent } from 'vs/workbench/common/editor/editorGroupModel'; -import { EditorExtensions, IEditorFactoryRegistry, IFileEditorInput, IEditorSerializer, CloseDirection, EditorsOrder, IResourceDiffEditorInput } from 'vs/workbench/common/editor'; +import { EditorGroupModel, ISerializedEditorGroupModel, EditorCloseEvent, SideBySideMatchingStrategy } from 'vs/workbench/common/editor/editorGroupModel'; +import { EditorExtensions, IEditorFactoryRegistry, IFileEditorInput, IEditorSerializer, CloseDirection, EditorsOrder, IResourceDiffEditorInput, IResourceSideBySideEditorInput } from 'vs/workbench/common/editor'; import { URI } from 'vs/base/common/uri'; import { TestLifecycleService, workbenchInstantiationService } from 'vs/workbench/test/browser/workbenchTestServices'; import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService'; @@ -23,6 +23,8 @@ import { IStorageService } from 'vs/platform/storage/common/storage'; import { DisposableStore } from 'vs/base/common/lifecycle'; import { TestContextService, TestStorageService } from 'vs/workbench/test/common/workbenchTestServices'; import { EditorInput } from 'vs/workbench/common/editor/editorInput'; +import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput'; +import { isEqual } from 'vs/base/common/resources'; suite('EditorGroupModel', () => { @@ -180,7 +182,12 @@ suite('EditorGroupModel', () => { if (super.matches(other)) { return true; } - return other && this.id === other.id && other instanceof TestFileEditorInput; + + if (other instanceof TestFileEditorInput) { + return isEqual(other.resource, this.resource); + } + + return false; } } @@ -305,6 +312,32 @@ suite('EditorGroupModel', () => { assert.ok(!group.isActive(untypedNonActiveInput)); }); + test('openEditor - prefers existing side by side editor if same', () => { + const group = createEditorGroupModel(); + const input1 = new TestFileEditorInput('testInput', URI.file('fake1')); + const input2 = new TestFileEditorInput('testInput', URI.file('fake2')); + + const sideBySideInputSame = new SideBySideEditorInput(undefined, undefined, input1, input1); + const sideBySideInputDifferent = new SideBySideEditorInput(undefined, undefined, input1, input2); + + let res = group.openEditor(sideBySideInputSame, { pinned: true, active: true }); + assert.strictEqual(res.editor, sideBySideInputSame); + assert.strictEqual(res.isNew, true); + + res = group.openEditor(input1, { pinned: true, active: true }); + assert.strictEqual(res.editor, sideBySideInputSame); + assert.strictEqual(res.isNew, false); + + group.closeEditor(sideBySideInputSame); + res = group.openEditor(sideBySideInputDifferent, { pinned: true, active: true }); + assert.strictEqual(res.editor, sideBySideInputDifferent); + assert.strictEqual(res.isNew, true); + + res = group.openEditor(input1, { pinned: true, active: true }); + assert.strictEqual(res.editor, input1); + assert.strictEqual(res.isNew, true); + }); + test('contains() - untyped', function () { const group = createEditorGroupModel(); const instantiationService = workbenchInstantiationService(); @@ -327,14 +360,28 @@ suite('EditorGroupModel', () => { modified: untypedInput1 }; + const sideBySideInputSame = new SideBySideEditorInput('name', undefined, input1, input1); + const sideBySideInputDifferent = new SideBySideEditorInput('name', undefined, input1, input2); + + const untypedSideBySideInputSame: IResourceSideBySideEditorInput = { + primary: untypedInput1, + secondary: untypedInput1 + }; + const untypedSideBySideInputDifferent: IResourceSideBySideEditorInput = { + primary: untypedInput2, + secondary: untypedInput1 + }; + group.openEditor(input1, { pinned: true, active: true }); assert.strictEqual(group.contains(untypedInput1), true); assert.strictEqual(group.contains(untypedInput1, { strictEquals: true }), false); - assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), true); assert.strictEqual(group.contains(untypedInput2), false); assert.strictEqual(group.contains(untypedInput2, { strictEquals: true }), false); - assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: true }), false); + assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), false); + assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), false); assert.strictEqual(group.contains(untypedDiffInput1), false); assert.strictEqual(group.contains(untypedDiffInput2), false); @@ -362,7 +409,8 @@ suite('EditorGroupModel', () => { group.closeEditor(input1); assert.strictEqual(group.contains(untypedInput1), false); - assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), false); assert.strictEqual(group.contains(untypedInput2), true); assert.strictEqual(group.contains(untypedDiffInput1), true); assert.strictEqual(group.contains(untypedDiffInput2), true); @@ -370,29 +418,43 @@ suite('EditorGroupModel', () => { group.closeEditor(input2); assert.strictEqual(group.contains(untypedInput1), false); - assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(untypedInput2), false); - assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(untypedDiffInput1), true); assert.strictEqual(group.contains(untypedDiffInput2), true); group.closeEditor(diffInput1); assert.strictEqual(group.contains(untypedInput1), false); - assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(untypedInput2), false); - assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(untypedDiffInput1), false); assert.strictEqual(group.contains(untypedDiffInput2), true); group.closeEditor(diffInput2); assert.strictEqual(group.contains(untypedInput1), false); - assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: true }), false); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), false); assert.strictEqual(group.contains(untypedInput2), false); - assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: true }), false); + assert.strictEqual(group.contains(untypedInput2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), false); assert.strictEqual(group.contains(untypedDiffInput1), false); assert.strictEqual(group.contains(untypedDiffInput2), false); + + assert.strictEqual(group.count, 0); + group.openEditor(sideBySideInputSame, { pinned: true, active: true }); + assert.strictEqual(group.contains(untypedSideBySideInputSame), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), true); + + group.closeEditor(sideBySideInputSame); + + assert.strictEqual(group.count, 0); + group.openEditor(sideBySideInputDifferent, { pinned: true, active: true }); + assert.strictEqual(group.contains(untypedSideBySideInputDifferent), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); + assert.strictEqual(group.contains(untypedInput1, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), false); }); test('contains()', () => { @@ -405,14 +467,17 @@ suite('EditorGroupModel', () => { const diffInput1 = instantiationService.createInstance(DiffEditorInput, 'name', 'description', input1, input2, undefined); const diffInput2 = instantiationService.createInstance(DiffEditorInput, 'name', 'description', input2, input1, undefined); + const sideBySideInputSame = new SideBySideEditorInput('name', undefined, input1, input1); + const sideBySideInputDifferent = new SideBySideEditorInput('name', undefined, input1, input2); + group.openEditor(input1, { pinned: true, active: true }); assert.strictEqual(group.contains(input1), true); assert.strictEqual(group.contains(input1, { strictEquals: true }), true); - assert.strictEqual(group.contains(input1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(input2), false); assert.strictEqual(group.contains(input2, { strictEquals: true }), false); - assert.strictEqual(group.contains(input2, { supportSideBySide: true }), false); + assert.strictEqual(group.contains(input2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), false); assert.strictEqual(group.contains(diffInput1), false); assert.strictEqual(group.contains(diffInput2), false); @@ -440,7 +505,7 @@ suite('EditorGroupModel', () => { group.closeEditor(input1); assert.strictEqual(group.contains(input1), false); - assert.strictEqual(group.contains(input1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(input2), true); assert.strictEqual(group.contains(diffInput1), true); assert.strictEqual(group.contains(diffInput2), true); @@ -448,27 +513,27 @@ suite('EditorGroupModel', () => { group.closeEditor(input2); assert.strictEqual(group.contains(input1), false); - assert.strictEqual(group.contains(input1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(input2), false); - assert.strictEqual(group.contains(input2, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(input2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(diffInput1), true); assert.strictEqual(group.contains(diffInput2), true); group.closeEditor(diffInput1); assert.strictEqual(group.contains(input1), false); - assert.strictEqual(group.contains(input1, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(input2), false); - assert.strictEqual(group.contains(input2, { supportSideBySide: true }), true); + assert.strictEqual(group.contains(input2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); assert.strictEqual(group.contains(diffInput1), false); assert.strictEqual(group.contains(diffInput2), true); group.closeEditor(diffInput2); assert.strictEqual(group.contains(input1), false); - assert.strictEqual(group.contains(input1, { supportSideBySide: true }), false); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), false); assert.strictEqual(group.contains(input2), false); - assert.strictEqual(group.contains(input2, { supportSideBySide: true }), false); + assert.strictEqual(group.contains(input2, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), false); assert.strictEqual(group.contains(diffInput1), false); assert.strictEqual(group.contains(diffInput2), false); @@ -483,6 +548,21 @@ suite('EditorGroupModel', () => { group.closeEditor(input3); assert.strictEqual(group.contains(input3), false); + + assert.strictEqual(group.count, 0); + group.openEditor(sideBySideInputSame, { pinned: true, active: true }); + + assert.strictEqual(group.contains(sideBySideInputSame), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), true); + + group.closeEditor(sideBySideInputSame); + + assert.strictEqual(group.count, 0); + group.openEditor(sideBySideInputDifferent, { pinned: true, active: true }); + assert.strictEqual(group.contains(sideBySideInputDifferent), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.ANY_SIDE }), true); + assert.strictEqual(group.contains(input1, { supportSideBySide: SideBySideMatchingStrategy.BOTH_SIDES }), false); }); test('group serialization', function () {