Skip to content

Commit 046a314

Browse files
author
Kartik Raj
authored
Merge pull request microsoft#182914 from microsoft/kartik/terminalupdates
Correctly show descriptions for each scope
2 parents 75b90a6 + d3ffc56 commit 046a314

File tree

6 files changed

+89
-43
lines changed

6 files changed

+89
-43
lines changed

‎extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { deepStrictEqual, doesNotThrow, equal, strictEqual, throws } from 'assert';
7-
import { ConfigurationTarget, Disposable, env, EnvironmentVariableMutator, EnvironmentVariableMutatorType, EventEmitter, ExtensionContext, extensions, ExtensionTerminalOptions, Pseudoterminal, Terminal, TerminalDimensions, TerminalExitReason, TerminalOptions, TerminalState, UIKind, window, workspace } from 'vscode';
7+
import { ConfigurationTarget, Disposable, env, EnvironmentVariableCollection, EnvironmentVariableMutator, EnvironmentVariableMutatorType, EnvironmentVariableScope, EventEmitter, ExtensionContext, extensions, ExtensionTerminalOptions, Pseudoterminal, Terminal, TerminalDimensions, TerminalExitReason, TerminalOptions, TerminalState, UIKind, Uri, window, workspace } from 'vscode';
88
import { assertNoRpc, poll } from '../utils';
99

1010
// Disable terminal tests:
@@ -848,19 +848,45 @@ import { assertNoRpc, poll } from '../utils';
848848
collection.replace('A', '~a2~');
849849
collection.append('B', '~b2~');
850850
collection.prepend('C', '~c2~');
851-
852851
// Verify get
853-
deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined, options: {} });
854-
deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined, options: {} });
855-
deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined, options: {} });
856-
852+
deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} });
853+
deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, options: {} });
854+
deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} });
857855
// Verify forEach
858856
const entries: [string, EnvironmentVariableMutator][] = [];
859857
collection.forEach((v, m) => entries.push([v, m]));
860858
deepStrictEqual(entries, [
861-
['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined, options: {} }],
862-
['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined, options: {} }],
863-
['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined, options: {} }]
859+
['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} }],
860+
['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, options: {} }],
861+
['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} }]
862+
]);
863+
});
864+
865+
test('get and forEach should work (scope)', () => {
866+
// TODO: Remove cast once `envCollectionWorkspace` API is finalized.
867+
const collection = extensionContext.environmentVariableCollection as (EnvironmentVariableCollection & { getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection });
868+
disposables.push({ dispose: () => collection.clear() });
869+
const scope = { workspaceFolder: { uri: Uri.file('workspace1'), name: 'workspace1', index: 0 } };
870+
const scopedCollection = collection.getScopedEnvironmentVariableCollection(scope);
871+
scopedCollection.replace('A', 'scoped~a2~');
872+
scopedCollection.append('B', 'scoped~b2~');
873+
scopedCollection.prepend('C', 'scoped~c2~');
874+
collection.replace('A', '~a2~');
875+
collection.append('B', '~b2~');
876+
collection.prepend('C', '~c2~');
877+
// Verify get for scope
878+
const expectedScopedCollection = collection.getScopedEnvironmentVariableCollection(scope);
879+
deepStrictEqual(expectedScopedCollection.get('A'), { value: 'scoped~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} });
880+
deepStrictEqual(expectedScopedCollection.get('B'), { value: 'scoped~b2~', type: EnvironmentVariableMutatorType.Append, options: {} });
881+
deepStrictEqual(expectedScopedCollection.get('C'), { value: 'scoped~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} });
882+
883+
// Verify forEach
884+
const entries: [string, EnvironmentVariableMutator][] = [];
885+
expectedScopedCollection.forEach((v, m) => entries.push([v, m]));
886+
deepStrictEqual(entries.map(v => v[1]), [
887+
{ value: 'scoped~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} },
888+
{ value: 'scoped~b2~', type: EnvironmentVariableMutatorType.Append, options: {} },
889+
{ value: 'scoped~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} }
864890
]);
865891
});
866892
});

‎src/vs/platform/terminal/common/environmentVariable.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ export interface IMergedEnvironmentVariableCollection {
7474
getVariableMap(scope: EnvironmentVariableScope | undefined): Map<string, IExtensionOwnedEnvironmentVariableMutator[]>;
7575
/**
7676
* Gets the description map for a given scope.
77-
* @param scope The scope to get the description map for. If undefined, the global scope is used.
77+
* @param scope The scope to get the description map for. If undefined, description map for the
78+
* global scope is returned.
7879
*/
7980
getDescriptionMap(scope: EnvironmentVariableScope | undefined): Map<string, string | undefined>;
8081
/**

‎src/vs/platform/terminal/common/environmentVariableCollection.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
151151
getDescriptionMap(scope: EnvironmentVariableScope | undefined): Map<string, string | undefined> {
152152
const result = new Map<string, string | undefined>();
153153
this.descriptionMap.forEach((mutators, _key) => {
154-
const filteredMutators = mutators.filter(m => filterScope(m, scope));
154+
const filteredMutators = mutators.filter(m => filterScope(m, scope, true));
155155
if (filteredMutators.length > 0) {
156156
// There should be exactly one description per extension per scope.
157157
result.set(filteredMutators[0].extensionIdentifier, filteredMutators[0].description);
@@ -190,11 +190,22 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
190190
}
191191
}
192192

193+
/**
194+
* Returns whether a mutator matches with the scope provided.
195+
* @param mutator Mutator to filter
196+
* @param scope Scope to be used for querying
197+
* @param strictFilter If true, mutators with global scope is not returned when querying for workspace scope.
198+
* i.e whether mutator scope should always exactly match with query scope.
199+
*/
193200
function filterScope(
194201
mutator: IExtensionOwnedEnvironmentVariableMutator | IExtensionOwnedEnvironmentDescriptionMutator,
195-
scope: EnvironmentVariableScope | undefined
202+
scope: EnvironmentVariableScope | undefined,
203+
strictFilter = false
196204
): boolean {
197205
if (!mutator.scope) {
206+
if (strictFilter) {
207+
return scope === mutator.scope;
208+
}
198209
return true;
199210
}
200211
// If a mutator is scoped to a workspace folder, only apply it if the workspace

‎src/vs/workbench/api/common/extHostTerminalService.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
356356
protected _terminalProcessDisposables: { [id: number]: IDisposable } = {};
357357
protected _extensionTerminalAwaitingStart: { [id: number]: { initialDimensions: ITerminalDimensionsDto | undefined } | undefined } = {};
358358
protected _getTerminalPromises: { [id: number]: Promise<ExtHostTerminal | undefined> } = {};
359-
protected _environmentVariableCollections: Map<string, EnvironmentVariableCollection> = new Map();
359+
protected _environmentVariableCollections: Map<string, UnifiedEnvironmentVariableCollection> = new Map();
360360
private _defaultProfile: ITerminalProfile | undefined;
361361
private _defaultAutomationProfile: ITerminalProfile | undefined;
362362

@@ -827,13 +827,13 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
827827
public getEnvironmentVariableCollection(extension: IExtensionDescription): IEnvironmentVariableCollection {
828828
let collection = this._environmentVariableCollections.get(extension.identifier.value);
829829
if (!collection) {
830-
collection = new EnvironmentVariableCollection(extension);
830+
collection = new UnifiedEnvironmentVariableCollection(extension);
831831
this._setEnvironmentVariableCollection(extension.identifier.value, collection);
832832
}
833833
return collection.getScopedEnvironmentVariableCollection(undefined);
834834
}
835835

836-
private _syncEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
836+
private _syncEnvironmentVariableCollection(extensionIdentifier: string, collection: UnifiedEnvironmentVariableCollection): void {
837837
const serialized = serializeEnvironmentVariableCollection(collection.map);
838838
const serializedDescription = serializeEnvironmentDescriptionMap(collection.descriptionMap);
839839
this._proxy.$setEnvironmentVariableCollection(extensionIdentifier, collection.persistent, serialized.length === 0 ? undefined : serialized, serializedDescription);
@@ -842,7 +842,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
842842
public $initEnvironmentVariableCollections(collections: [string, ISerializableEnvironmentVariableCollection][]): void {
843843
collections.forEach(entry => {
844844
const extensionIdentifier = entry[0];
845-
const collection = new EnvironmentVariableCollection(undefined, entry[1]);
845+
const collection = new UnifiedEnvironmentVariableCollection(undefined, entry[1]);
846846
this._setEnvironmentVariableCollection(extensionIdentifier, collection);
847847
});
848848
}
@@ -856,7 +856,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
856856
}
857857
}
858858

859-
private _setEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
859+
private _setEnvironmentVariableCollection(extensionIdentifier: string, collection: UnifiedEnvironmentVariableCollection): void {
860860
this._environmentVariableCollections.set(extensionIdentifier, collection);
861861
collection.onDidChangeCollection(() => {
862862
// When any collection value changes send this immediately, this is done to ensure
@@ -868,7 +868,10 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
868868
}
869869
}
870870

871-
class EnvironmentVariableCollection {
871+
/**
872+
* Unified environment variable collection carrying information for all scopes, for a specific extension.
873+
*/
874+
class UnifiedEnvironmentVariableCollection {
872875
readonly map: Map<string, IEnvironmentVariableMutator> = new Map();
873876
private readonly scopedCollections: Map<string, ScopedEnvironmentVariableCollection> = new Map();
874877
readonly descriptionMap: Map<string, IEnvironmentDescriptionMutator> = new Map();
@@ -930,9 +933,6 @@ class EnvironmentVariableCollection {
930933
if (mutator.options && mutator.options.applyAtProcessCreation === false && !mutator.options.applyAtShellIntegration) {
931934
throw new Error('EnvironmentVariableMutatorOptions must apply at either process creation or shell integration');
932935
}
933-
if (!mutator.scope) {
934-
delete (mutator as any).scope; // Convenient for tests
935-
}
936936
const key = this.getKey(variable, mutator.scope);
937937
const current = this.map.get(key);
938938
if (!current || current.value !== mutator.value || current.type !== mutator.type || current.scope?.workspaceFolder?.index !== mutator.scope?.workspaceFolder?.index) {
@@ -1032,13 +1032,13 @@ class ScopedEnvironmentVariableCollection implements vscode.EnvironmentVariableC
10321032
get onDidChangeCollection(): Event<void> { return this._onDidChangeCollection && this._onDidChangeCollection.event; }
10331033

10341034
constructor(
1035-
private readonly collection: EnvironmentVariableCollection,
1035+
private readonly collection: UnifiedEnvironmentVariableCollection,
10361036
private readonly scope: vscode.EnvironmentVariableScope | undefined
10371037
) {
10381038
}
10391039

1040-
getScopedEnvironmentVariableCollection() {
1041-
return this.collection.getScopedEnvironmentVariableCollection(this.scope);
1040+
getScopedEnvironmentVariableCollection(scope: vscode.EnvironmentVariableScope | undefined) {
1041+
return this.collection.getScopedEnvironmentVariableCollection(scope);
10421042
}
10431043

10441044
replace(variable: string, value: string, options?: vscode.EnvironmentVariableMutatorOptions | undefined): void {
@@ -1120,7 +1120,7 @@ function asTerminalColor(color?: vscode.ThemeColor): ThemeColor | undefined {
11201120

11211121
function convertMutator(mutator: IEnvironmentVariableMutator): vscode.EnvironmentVariableMutator {
11221122
const newMutator = { ...mutator };
1123-
newMutator.scope = newMutator.scope ?? undefined;
1123+
delete newMutator.scope;
11241124
newMutator.options = newMutator.options ?? undefined;
11251125
delete (newMutator as any).variable;
11261126
return newMutator as vscode.EnvironmentVariableMutator;

‎src/vs/workbench/contrib/terminal/browser/environmentVariableInfo.ts

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,7 @@ export class EnvironmentVariableInfoStale implements IEnvironmentVariableInfo {
3333
addExtensionIdentifiers(extSet, this._diff.changed.values());
3434

3535
let message = localize('extensionEnvironmentContributionInfoStale', "The following extensions want to relaunch the terminal to contribute to its environment:");
36-
message += '\n';
37-
const descriptionMap = this._collection.getDescriptionMap(scope);
38-
for (const ext of extSet) {
39-
message += `\n- \`${getExtensionName(ext, this._extensionService)}\``;
40-
const description = descriptionMap.get(ext);
41-
if (description) {
42-
message += `: ${description}`;
43-
}
44-
}
36+
message += getMergedDescription(this._collection, scope, this._extensionService, extSet);
4537
return message;
4638
}
4739

@@ -79,15 +71,7 @@ export class EnvironmentVariableInfoChangesActive implements IEnvironmentVariabl
7971
addExtensionIdentifiers(extSet, this._collection.getVariableMap(scope).values());
8072

8173
let message = localize('extensionEnvironmentContributionInfoActive', "The following extensions have contributed to this terminal's environment:");
82-
message += '\n';
83-
const descriptionMap = this._collection.getDescriptionMap(scope);
84-
for (const ext of extSet) {
85-
message += `\n- \`${getExtensionName(ext, this._extensionService)}\``;
86-
const description = descriptionMap.get(ext);
87-
if (description) {
88-
message += `: ${description}`;
89-
}
90-
}
74+
message += getMergedDescription(this._collection, scope, this._extensionService, extSet);
9175
return message;
9276
}
9377

@@ -109,6 +93,28 @@ export class EnvironmentVariableInfoChangesActive implements IEnvironmentVariabl
10993
}
11094
}
11195

96+
function getMergedDescription(collection: IMergedEnvironmentVariableCollection, scope: EnvironmentVariableScope | undefined, extensionService: IExtensionService, extSet: Set<string>): string {
97+
const message = ['\n'];
98+
const globalDescriptions = collection.getDescriptionMap(undefined);
99+
const workspaceDescriptions = collection.getDescriptionMap(scope);
100+
for (const ext of extSet) {
101+
const globalDescription = globalDescriptions.get(ext);
102+
if (globalDescription) {
103+
message.push(`\n- \`${getExtensionName(ext, extensionService)}\``);
104+
message.push(`: ${globalDescription}`);
105+
}
106+
const workspaceDescription = workspaceDescriptions.get(ext);
107+
if (workspaceDescription) {
108+
message.push(`\n- \`${getExtensionName(ext, extensionService)} (${localize('ScopedEnvironmentContributionInfo', 'workspace')})\``);
109+
message.push(`: ${workspaceDescription}`);
110+
}
111+
if (!globalDescription && !workspaceDescription) {
112+
message.push(`\n- \`${getExtensionName(ext, extensionService)}\``);
113+
}
114+
}
115+
return message.join('');
116+
}
117+
112118
function addExtensionIdentifiers(extSet: Set<string>, diff: IterableIterator<IExtensionOwnedEnvironmentVariableMutator[]>): void {
113119
for (const mutators of diff) {
114120
for (const mutator of mutators) {

‎src/vs/workbench/contrib/terminal/test/common/environmentVariableCollection.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,8 @@ suite('EnvironmentVariable - MergedEnvironmentVariableCollection', () => {
181181
]));
182182
deepStrictEqual([...merged.getDescriptionMap(scope1).entries()], [
183183
['ext1', 'ext1 scope1 description'],
184+
]);
185+
deepStrictEqual([...merged.getDescriptionMap(undefined).entries()], [
184186
['ext2', 'ext2 global description'],
185187
]);
186188
});

0 commit comments

Comments
 (0)