Skip to content

Commit 449da3b

Browse files
lramos15bpasero
lramos15
and
bpasero
authored
Lighter weight matches (microsoft#159554)
* Implement a lighter weight matches when no override present * Ensure diff still passes without override * Address PR feedback Co-authored-by: Benjamin Pasero <[email protected]>
1 parent 99a7f26 commit 449da3b

File tree

6 files changed

+99
-34
lines changed

6 files changed

+99
-34
lines changed

‎src/vs/workbench/common/editor/editorInput.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,9 @@ export abstract class EditorInput extends AbstractEditorInput {
262262
// Untyped inputs: go into properties
263263
const otherInputEditorId = otherInput.options?.override;
264264

265-
if (this.editorId === undefined) {
266-
return false; // untyped inputs can only match for editors that have adopted `editorId`
267-
}
268-
269-
if (this.editorId !== otherInputEditorId) {
270-
return false; // untyped input uses another `editorId`
265+
// If the overrides are both defined and don't match that means they're separate inputs
266+
if (this.editorId !== otherInputEditorId && otherInputEditorId !== undefined && this.editorId !== undefined) {
267+
return false;
271268
}
272269

273270
return isEqual(this.resource, EditorResourceAccessor.getCanonicalUri(otherInput));

‎src/vs/workbench/contrib/mergeEditor/browser/mergeEditorInput.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ export class MergeEditorInput extends AbstractTextResourceEditorInput implements
178178
&& isEqual(this.result, otherInput.result);
179179
}
180180
if (isResourceMergeEditorInput(otherInput)) {
181-
return this.editorId === otherInput.options?.override
181+
return (this.editorId === otherInput.options?.override || otherInput.options?.override === undefined)
182182
&& isEqual(this.base, otherInput.base.resource)
183183
&& isEqual(this.input1.uri, otherInput.input1.resource)
184184
&& isEqual(this.input2.uri, otherInput.input2.resource)

‎src/vs/workbench/contrib/notebook/common/notebookDiffEditorInput.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ export class NotebookDiffEditorInput extends DiffEditorInput {
118118
return this.modified.matches(otherInput.modified)
119119
&& this.original.matches(otherInput.original)
120120
&& this.editorId !== undefined
121-
&& this.editorId === otherInput.options?.override;
121+
&& (this.editorId === otherInput.options?.override || otherInput.options?.override === undefined);
122122
}
123123

124124
return false;

‎src/vs/workbench/services/editor/common/editorGroupFinder.ts

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,10 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { isEqual } from 'vs/base/common/resources';
76
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
87
import { EditorActivation } from 'vs/platform/editor/common/editor';
98
import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation';
10-
import { EditorResourceAccessor, EditorInputWithOptions, isEditorInputWithOptions, IUntypedEditorInput, isEditorInput, EditorInputCapabilities, isResourceDiffEditorInput } from 'vs/workbench/common/editor';
11-
import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput';
9+
import { EditorInputWithOptions, isEditorInputWithOptions, IUntypedEditorInput, isEditorInput, EditorInputCapabilities } from 'vs/workbench/common/editor';
1210
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
1311
import { IEditorGroup, GroupsOrder, preferredSideBySideGroupDirection, IEditorGroupsService } from 'vs/workbench/services/editor/common/editorGroupsService';
1412
import { PreferredGroup, SIDE_GROUP } from 'vs/workbench/services/editor/common/editorService';
@@ -183,35 +181,15 @@ function isActive(group: IEditorGroup, editor: EditorInput | IUntypedEditorInput
183181
return false;
184182
}
185183

186-
return matchesEditor(group.activeEditor, editor);
184+
return group.activeEditor.matches(editor);
187185
}
188186

189187
function isOpened(group: IEditorGroup, editor: EditorInput | IUntypedEditorInput): boolean {
190188
for (const typedEditor of group.editors) {
191-
if (matchesEditor(typedEditor, editor)) {
189+
if (typedEditor.matches(editor)) {
192190
return true;
193191
}
194192
}
195193

196194
return false;
197195
}
198-
199-
function matchesEditor(typedEditor: EditorInput, editor: EditorInput | IUntypedEditorInput): boolean {
200-
if (typedEditor.matches(editor)) {
201-
return true;
202-
}
203-
204-
// Note: intentionally doing a "weak" check on the resource
205-
// because `EditorInput.matches` will not work for untyped
206-
// editors that have no `override` defined.
207-
//
208-
// TODO@lramos15 https://github.com/microsoft/vscode/issues/131619
209-
if (typedEditor instanceof DiffEditorInput && isResourceDiffEditorInput(editor)) {
210-
return matchesEditor(typedEditor.primary, editor.modified) && matchesEditor(typedEditor.secondary, editor.original);
211-
}
212-
if (typedEditor.resource) {
213-
return isEqual(typedEditor.resource, EditorResourceAccessor.getCanonicalUri(editor));
214-
}
215-
216-
return false;
217-
}

‎src/vs/workbench/test/browser/parts/editor/editorInput.test.ts

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { URI } from 'vs/base/common/uri';
1010
import { IResourceEditorInput, ITextResourceEditorInput } from 'vs/platform/editor/common/editor';
1111
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
1212
import { DEFAULT_EDITOR_ASSOCIATION, IResourceDiffEditorInput, IResourceMergeEditorInput, IResourceSideBySideEditorInput, isEditorInput, isResourceDiffEditorInput, isResourceEditorInput, isResourceMergeEditorInput, isResourceSideBySideEditorInput, isUntitledResourceEditorInput, IUntitledTextResourceEditorInput } from 'vs/workbench/common/editor';
13+
import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput';
1314
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
1415
import { TextResourceEditorInput } from 'vs/workbench/common/editor/textResourceEditorInput';
1516
import { FileEditorInput } from 'vs/workbench/contrib/files/browser/editors/fileEditorInput';
@@ -31,10 +32,45 @@ suite('EditorInput', () => {
3132
const untypedResourceDiffEditorInput: IResourceDiffEditorInput = { original: untypedResourceEditorInput, modified: untypedResourceEditorInput, options: { override: DEFAULT_EDITOR_ASSOCIATION.id } };
3233
const untypedResourceMergeEditorInput: IResourceMergeEditorInput = { base: untypedResourceEditorInput, input1: untypedResourceEditorInput, input2: untypedResourceEditorInput, result: untypedResourceEditorInput, options: { override: DEFAULT_EDITOR_ASSOCIATION.id } };
3334

35+
// Function to easily remove the overrides from the untyped inputs
36+
const stripOverrides = () => {
37+
if (
38+
!untypedResourceEditorInput.options ||
39+
!untypedTextResourceEditorInput.options ||
40+
!untypedUntitledResourceEditorinput.options ||
41+
!untypedResourceDiffEditorInput.options ||
42+
!untypedResourceMergeEditorInput.options
43+
) {
44+
throw new Error('Malformed options on untyped inputs');
45+
}
46+
// Some of the tests mutate the overrides so we want to reset them on each test
47+
untypedResourceEditorInput.options.override = undefined;
48+
untypedTextResourceEditorInput.options.override = undefined;
49+
untypedUntitledResourceEditorinput.options.override = undefined;
50+
untypedResourceDiffEditorInput.options.override = undefined;
51+
untypedResourceMergeEditorInput.options.override = undefined;
52+
};
53+
3454
setup(() => {
3555
disposables = new DisposableStore();
3656
instantiationService = workbenchInstantiationService(undefined, disposables);
3757
accessor = instantiationService.createInstance(TestServiceAccessor);
58+
59+
if (
60+
!untypedResourceEditorInput.options ||
61+
!untypedTextResourceEditorInput.options ||
62+
!untypedUntitledResourceEditorinput.options ||
63+
!untypedResourceDiffEditorInput.options ||
64+
!untypedResourceMergeEditorInput.options
65+
) {
66+
throw new Error('Malformed options on untyped inputs');
67+
}
68+
// Some of the tests mutate the overrides so we want to reset them on each test
69+
untypedResourceEditorInput.options.override = DEFAULT_EDITOR_ASSOCIATION.id;
70+
untypedTextResourceEditorInput.options.override = DEFAULT_EDITOR_ASSOCIATION.id;
71+
untypedUntitledResourceEditorinput.options.override = DEFAULT_EDITOR_ASSOCIATION.id;
72+
untypedResourceDiffEditorInput.options.override = DEFAULT_EDITOR_ASSOCIATION.id;
73+
untypedResourceMergeEditorInput.options.override = DEFAULT_EDITOR_ASSOCIATION.id;
3874
});
3975

4076
teardown(() => {
@@ -116,6 +152,16 @@ suite('EditorInput', () => {
116152
assert.ok(!fileEditorInput.matches(untypedResourceDiffEditorInput));
117153
assert.ok(!fileEditorInput.matches(untypedResourceMergeEditorInput));
118154

155+
// Now we remove the override on the untyped to ensure that FileEditorInput supports lightweight resource matching
156+
stripOverrides();
157+
158+
assert.ok(fileEditorInput.matches(untypedResourceEditorInput));
159+
assert.ok(fileEditorInput.matches(untypedTextResourceEditorInput));
160+
assert.ok(!fileEditorInput.matches(untypedResourceSideBySideEditorInput));
161+
assert.ok(!fileEditorInput.matches(untypedUntitledResourceEditorinput));
162+
assert.ok(!fileEditorInput.matches(untypedResourceDiffEditorInput));
163+
assert.ok(!fileEditorInput.matches(untypedResourceMergeEditorInput));
164+
119165
fileEditorInput.dispose();
120166
});
121167

@@ -130,6 +176,15 @@ suite('EditorInput', () => {
130176
assert.ok(!mergeEditorInput.matches(untypedResourceDiffEditorInput));
131177
assert.ok(mergeEditorInput.matches(untypedResourceMergeEditorInput));
132178

179+
stripOverrides();
180+
181+
assert.ok(!mergeEditorInput.matches(untypedResourceEditorInput));
182+
assert.ok(!mergeEditorInput.matches(untypedTextResourceEditorInput));
183+
assert.ok(!mergeEditorInput.matches(untypedResourceSideBySideEditorInput));
184+
assert.ok(!mergeEditorInput.matches(untypedUntitledResourceEditorinput));
185+
assert.ok(!mergeEditorInput.matches(untypedResourceDiffEditorInput));
186+
assert.ok(mergeEditorInput.matches(untypedResourceMergeEditorInput));
187+
133188
mergeEditorInput.dispose();
134189
});
135190

@@ -144,6 +199,41 @@ suite('EditorInput', () => {
144199
assert.ok(!untitledTextEditorInput.matches(untypedResourceDiffEditorInput));
145200
assert.ok(!untitledTextEditorInput.matches(untypedResourceMergeEditorInput));
146201

202+
stripOverrides();
203+
204+
assert.ok(!untitledTextEditorInput.matches(untypedResourceEditorInput));
205+
assert.ok(!untitledTextEditorInput.matches(untypedTextResourceEditorInput));
206+
assert.ok(!untitledTextEditorInput.matches(untypedResourceSideBySideEditorInput));
207+
assert.ok(untitledTextEditorInput.matches(untypedUntitledResourceEditorinput));
208+
assert.ok(!untitledTextEditorInput.matches(untypedResourceDiffEditorInput));
209+
assert.ok(!untitledTextEditorInput.matches(untypedResourceMergeEditorInput));
210+
147211
untitledTextEditorInput.dispose();
148212
});
213+
214+
test('Untyped inputs properly match DiffEditorInput', () => {
215+
const fileEditorInput1 = instantiationService.createInstance(FileEditorInput, testResource, undefined, undefined, undefined, undefined, undefined, undefined);
216+
const fileEditorInput2 = instantiationService.createInstance(FileEditorInput, testResource, undefined, undefined, undefined, undefined, undefined, undefined);
217+
const diffEditorInput: DiffEditorInput = instantiationService.createInstance(DiffEditorInput, undefined, undefined, fileEditorInput1, fileEditorInput2, false);
218+
219+
assert.ok(!diffEditorInput.matches(untypedResourceEditorInput));
220+
assert.ok(!diffEditorInput.matches(untypedTextResourceEditorInput));
221+
assert.ok(!diffEditorInput.matches(untypedResourceSideBySideEditorInput));
222+
assert.ok(!diffEditorInput.matches(untypedUntitledResourceEditorinput));
223+
assert.ok(diffEditorInput.matches(untypedResourceDiffEditorInput));
224+
assert.ok(!diffEditorInput.matches(untypedResourceMergeEditorInput));
225+
226+
stripOverrides();
227+
228+
assert.ok(!diffEditorInput.matches(untypedResourceEditorInput));
229+
assert.ok(!diffEditorInput.matches(untypedTextResourceEditorInput));
230+
assert.ok(!diffEditorInput.matches(untypedResourceSideBySideEditorInput));
231+
assert.ok(!diffEditorInput.matches(untypedUntitledResourceEditorinput));
232+
assert.ok(diffEditorInput.matches(untypedResourceDiffEditorInput));
233+
assert.ok(!diffEditorInput.matches(untypedResourceMergeEditorInput));
234+
235+
diffEditorInput.dispose();
236+
fileEditorInput1.dispose();
237+
fileEditorInput2.dispose();
238+
});
149239
});

‎src/vs/workbench/test/browser/workbenchTestServices.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1603,7 +1603,7 @@ export class TestFileEditorInput extends EditorInput implements IFileEditorInput
16031603
if (other instanceof EditorInput) {
16041604
return !!(other?.resource && this.resource.toString() === other.resource.toString() && other instanceof TestFileEditorInput && other.typeId === this.typeId);
16051605
}
1606-
return isEqual(this.resource, other.resource) && this.editorId === other.options?.override;
1606+
return isEqual(this.resource, other.resource) && (this.editorId === other.options?.override || other.options?.override === undefined);
16071607
}
16081608
setPreferredResource(resource: URI): void { }
16091609
async setEncoding(encoding: string) { }

0 commit comments

Comments
 (0)