Skip to content

Commit

Permalink
editors - fix side by side editor matching logic (#132651)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bpasero
bpasero committed Sep 9, 2021
1 parent aa93eef commit 844aab2
Show file tree
Hide file tree
Showing 7 changed files with 159 additions and 91 deletions.
14 changes: 12 additions & 2 deletions src/vs/workbench/browser/parts/editor/editor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { GroupIdentifier, IWorkbenchEditorConfiguration, IEditorInput, IEditorIdentifier, IEditorCloseEvent, IEditorPartOptions, IEditorPartOptionsChangeEvent } from 'vs/workbench/common/editor';
import { GroupIdentifier, IWorkbenchEditorConfiguration, IEditorInput, IEditorIdentifier, IEditorCloseEvent, IEditorPartOptions, IEditorPartOptionsChangeEvent, SideBySideEditor } from 'vs/workbench/common/editor';
import { IEditorGroup, GroupDirection, IAddGroupOptions, IMergeGroupOptions, GroupsOrder, GroupsArrangement } from 'vs/workbench/services/editor/common/editorGroupsService';
import { IDisposable } from 'vs/base/common/lifecycle';
import { Dimension } from 'vs/base/browser/dom';
Expand Down Expand Up @@ -184,7 +184,17 @@ export interface IInternalEditorTitleControlOptions {
skipTitleUpdate?: boolean;
}

export interface IInternalEditorOpenOptions extends IInternalEditorTitleControlOptions { }
export interface IInternalEditorOpenOptions extends IInternalEditorTitleControlOptions {

/**
* Whether to consider a side by side editor as matching
* when figuring out if the editor to open is already
* opened or not. By default, side by side editors will
* not be considered as matching, even if the editor is
* opened in one of the sides.
*/
supportSideBySide?: SideBySideEditor.ANY | SideBySideEditor.BOTH;
}

export interface IInternalEditorCloseOptions extends IInternalEditorTitleControlOptions {

Expand Down
36 changes: 25 additions & 11 deletions src/vs/workbench/browser/parts/editor/editorGroupView.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
*--------------------------------------------------------------------------------------------*/

import 'vs/css!./media/editorgroupview';
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 { EditorGroupModel, IEditorOpenOptions, EditorCloseEvent, ISerializedEditorGroupModel, isSerializedEditorGroupModel } 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, SideBySideEditor } from 'vs/workbench/common/editor';
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput';
import { Event, Emitter, Relay } from 'vs/base/common/event';
Expand Down Expand Up @@ -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: SideBySideMatchingStrategy.ANY_SIDE // include any side of an opened side by side editor
strictEquals: true, // only if this input is not shared across editor groups
supportSideBySide: SideBySideEditor.ANY // include any side of an opened side by side editor
})) {
return false;
}
Expand Down Expand Up @@ -948,7 +948,12 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
//#region openEditor()

async openEditor(editor: EditorInput, options?: IEditorOptions): Promise<IEditorPane | undefined> {
return this.doOpenEditor(editor, options);
return this.doOpenEditor(editor, options, {
// 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.
supportSideBySide: SideBySideEditor.BOTH
});
}

private async doOpenEditor(editor: EditorInput, options?: IEditorOptions, internalOptions?: IInternalEditorOpenOptions): Promise<IEditorPane | undefined> {
Expand All @@ -968,7 +973,8 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
index: options ? options.index : undefined,
pinned: options?.sticky || !this.accessor.partOptions.enablePreview || editor.isDirty() || (options?.pinned ?? typeof options?.index === 'number' /* unless specified, prefer to pin when opening with index */) || (typeof options?.index === 'number' && this.model.isSticky(options.index)),
sticky: options?.sticky || (typeof options?.index === 'number' && this.model.isSticky(options.index)),
active: this.count === 0 || !options || !options.inactive
active: this.count === 0 || !options || !options.inactive,
supportSideBySide: internalOptions?.supportSideBySide
};

if (options?.sticky && typeof options?.index === 'number' && !this.model.isSticky(options.index)) {
Expand Down Expand Up @@ -1202,7 +1208,14 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
return null;
}

await this.openEditor(firstEditor.editor, firstEditor.options);
const openEditorsOptions: IInternalEditorOpenOptions = {
// 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.
supportSideBySide: SideBySideEditor.BOTH
};

await this.doOpenEditor(firstEditor.editor, firstEditor.options, openEditorsOptions);

// Open the other ones inactive
const inactiveEditors = editorsToOpen.slice(1);
Expand All @@ -1214,6 +1227,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
pinned: true,
index: startingIndex + index
}, {
...openEditorsOptions,
// optimization: update the title control later
// https://github.com/microsoft/vscode/issues/130634
skipTitleUpdate: true
Expand Down Expand Up @@ -1471,7 +1485,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
ignoreError: internalOptions?.fromError
};

this.openEditor(nextActiveEditor, options);
this.doOpenEditor(nextActiveEditor, options);
}

// Otherwise we are empty, so clear from editor control and send event
Expand Down Expand Up @@ -1598,7 +1612,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
else {

// Switch to editor that we want to handle and confirm to save/revert
await this.openEditor(editor);
await this.doOpenEditor(editor);

// Let editor handle confirmation if implemented
if (typeof editor.confirm === 'function') {
Expand Down Expand Up @@ -1821,7 +1835,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
for (const { editor, replacement, forceReplaceDirty, options } of inactiveReplacements) {

// Open inactive editor
await this.openEditor(replacement, options);
await this.doOpenEditor(replacement, options);

// Close replaced inactive editor unless they match
if (!editor.matches(replacement)) {
Expand All @@ -1842,7 +1856,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
if (activeReplacement) {

// Open replacement as active editor
const openEditorResult = this.openEditor(activeReplacement.replacement, activeReplacement.options);
const openEditorResult = this.doOpenEditor(activeReplacement.replacement, activeReplacement.options);

// Close replaced active editor unless they match
if (!activeReplacement.editor.matches(activeReplacement.replacement)) {
Expand Down
11 changes: 8 additions & 3 deletions src/vs/workbench/common/editor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -952,7 +952,8 @@ export interface IEditorPartOptionsChangeEvent {
export enum SideBySideEditor {
PRIMARY = 1,
SECONDARY = 2,
BOTH = 3
BOTH = 3,
ANY = 4
}

export interface IEditorResourceAccessorOptions {
Expand Down Expand Up @@ -990,7 +991,7 @@ class EditorResourceAccessorImpl {
* such, the original URI and the canonical URI can be different.
*/
getOriginalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null): URI | undefined;
getOriginalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options: IEditorResourceAccessorOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY }): URI | undefined;
getOriginalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options: IEditorResourceAccessorOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY | SideBySideEditor.ANY }): URI | undefined;
getOriginalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options: IEditorResourceAccessorOptions & { supportSideBySide: SideBySideEditor.BOTH }): URI | { primary?: URI, secondary?: URI } | undefined;
getOriginalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options?: IEditorResourceAccessorOptions): URI | { primary?: URI, secondary?: URI } | undefined {
if (!editor) {
Expand All @@ -1006,6 +1007,8 @@ class EditorResourceAccessorImpl {
primary: this.getOriginalUri(primary, { filterByScheme: options.filterByScheme }),
secondary: this.getOriginalUri(secondary, { filterByScheme: options.filterByScheme })
};
} else if (options?.supportSideBySide === SideBySideEditor.ANY) {
return this.getOriginalUri(primary, { filterByScheme: options.filterByScheme }) ?? this.getOriginalUri(secondary, { filterByScheme: options.filterByScheme });
}

editor = options.supportSideBySide === SideBySideEditor.PRIMARY ? primary : secondary;
Expand Down Expand Up @@ -1051,7 +1054,7 @@ class EditorResourceAccessorImpl {
* such, the original URI and the canonical URI can be different.
*/
getCanonicalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null): URI | undefined;
getCanonicalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options: IEditorResourceAccessorOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY }): URI | undefined;
getCanonicalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options: IEditorResourceAccessorOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY | SideBySideEditor.ANY }): URI | undefined;
getCanonicalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options: IEditorResourceAccessorOptions & { supportSideBySide: SideBySideEditor.BOTH }): URI | { primary?: URI, secondary?: URI } | undefined;
getCanonicalUri(editor: IEditorInput | IUntypedEditorInput | undefined | null, options?: IEditorResourceAccessorOptions): URI | { primary?: URI, secondary?: URI } | undefined {
if (!editor) {
Expand All @@ -1067,6 +1070,8 @@ class EditorResourceAccessorImpl {
primary: this.getCanonicalUri(primary, { filterByScheme: options.filterByScheme }),
secondary: this.getCanonicalUri(secondary, { filterByScheme: options.filterByScheme })
};
} else if (options?.supportSideBySide === SideBySideEditor.ANY) {
return this.getCanonicalUri(primary, { filterByScheme: options.filterByScheme }) ?? this.getCanonicalUri(secondary, { filterByScheme: options.filterByScheme });
}

editor = options.supportSideBySide === SideBySideEditor.PRIMARY ? primary : secondary;
Expand Down
63 changes: 27 additions & 36 deletions src/vs/workbench/common/editor/editorGroupModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { Event, Emitter } from 'vs/base/common/event';
import { IEditorFactoryRegistry, IEditorIdentifier, IEditorCloseEvent, GroupIdentifier, IEditorInput, EditorsOrder, EditorExtensions, IUntypedEditorInput } from 'vs/workbench/common/editor';
import { IEditorFactoryRegistry, IEditorIdentifier, IEditorCloseEvent, GroupIdentifier, IEditorInput, EditorsOrder, EditorExtensions, IUntypedEditorInput, SideBySideEditor } from 'vs/workbench/common/editor';
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
Expand Down Expand Up @@ -34,6 +34,7 @@ export interface IEditorOpenOptions {
sticky?: boolean;
active?: boolean;
readonly index?: number;
readonly supportSideBySide?: SideBySideEditor.ANY | SideBySideEditor.BOTH;
}

export interface IEditorOpenResult {
Expand Down Expand Up @@ -61,28 +62,14 @@ 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.
* Whether to consider a side by side editor as matching.
* By default, side by side editors will not be considered
* as matching, even if the editor is opened in one of the sides.
*/
supportSideBySide?: SideBySideMatchingStrategy;
supportSideBySide?: SideBySideEditor.ANY | SideBySideEditor.BOTH;

/**
* Only consider an editor to match when the
Expand Down Expand Up @@ -220,13 +207,7 @@ 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, {
// 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
});
const existingEditorAndIndex = this.findEditor(candidate, options);

// New editor
if (!existingEditorAndIndex) {
Expand Down Expand Up @@ -725,17 +706,27 @@ export class EditorGroupModel extends Disposable {
}

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, options)) {
return i;
let index = -1;

if (candidate) {
for (let i = 0; i < editors.length; i++) {
const editor = editors[i];

if (this.matches(editor, candidate, options)) {
// If we are to support side by side matching, it is possible that
// a better direct match is found later. As such, we continue finding
// a matching editor and prefer that match over the side by side one.
if (options?.supportSideBySide && editor instanceof SideBySideEditorInput && !(candidate instanceof SideBySideEditorInput)) {
index = i;
} else {
index = i;
break;
}
}
}
}

return -1;
return index;
}

private findEditor(candidate: EditorInput | null, options?: IMatchOptions): [EditorInput, number /* index */] | undefined {
Expand Down Expand Up @@ -764,12 +755,12 @@ export class EditorGroupModel extends Disposable {

if (options?.supportSideBySide && editor instanceof SideBySideEditorInput && !(candidate instanceof SideBySideEditorInput)) {
switch (options.supportSideBySide) {
case SideBySideMatchingStrategy.ANY_SIDE:
case SideBySideEditor.ANY:
if (this.matches(editor.primary, candidate, options) || this.matches(editor.secondary, candidate, options)) {
return true;
}
break;
case SideBySideMatchingStrategy.BOTH_SIDES:
case SideBySideEditor.BOTH:
if (this.matches(editor.primary, candidate, options) && this.matches(editor.secondary, candidate, options)) {
return true;
}
Expand Down
24 changes: 23 additions & 1 deletion src/vs/workbench/services/editor/test/browser/editorGroupsService.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { MockScopableContextKeyService } from 'vs/platform/keybinding/test/commo
import { ConfirmResult } from 'vs/platform/dialogs/common/dialogs';
import { TestConfigurationService } from 'vs/platform/configuration/test/common/testConfigurationService';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput';

suite('EditorGroupsService', () => {

Expand All @@ -23,7 +24,7 @@ suite('EditorGroupsService', () => {
const disposables = new DisposableStore();

setup(() => {
disposables.add(registerTestEditor(TEST_EDITOR_ID, [new SyncDescriptor(TestFileEditorInput)], TEST_EDITOR_INPUT_ID));
disposables.add(registerTestEditor(TEST_EDITOR_ID, [new SyncDescriptor(TestFileEditorInput), new SyncDescriptor(SideBySideEditorInput)], TEST_EDITOR_INPUT_ID));
});

teardown(() => {
Expand Down Expand Up @@ -1143,6 +1144,27 @@ suite('EditorGroupsService', () => {
assert.strictEqual(group.getEditorByIndex(4), input8);
});

test('replaceEditors - should be able to replace when side by side editor is involved with same input side by side', async () => {
const [part] = await createPart();
const group = part.activeGroup;
assert.strictEqual(group.isEmpty, true);

const input = new TestFileEditorInput(URI.file('foo/bar'), TEST_EDITOR_INPUT_ID);
const sideBySideInput = new SideBySideEditorInput(undefined, undefined, input, input);

await group.openEditor(input);
assert.strictEqual(group.count, 1);
assert.strictEqual(group.getEditorByIndex(0), input);

await group.replaceEditors([{ editor: input, replacement: sideBySideInput }]);
assert.strictEqual(group.count, 1);
assert.strictEqual(group.getEditorByIndex(0), sideBySideInput);

await group.replaceEditors([{ editor: sideBySideInput, replacement: input }]);
assert.strictEqual(group.count, 1);
assert.strictEqual(group.getEditorByIndex(0), input);
});

test('find editors', async () => {
const [part] = await createPart();
const group = part.activeGroup;
Expand Down
Loading

0 comments on commit 844aab2

Please sign in to comment.