Correctly show descriptions for each scope

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

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { deepStrictEqual, doesNotThrow, equal, strictEqual, throws } from 'assert';
-import { ConfigurationTarget, Disposable, env, EnvironmentVariableMutator, EnvironmentVariableMutatorType, EventEmitter, ExtensionContext, extensions, ExtensionTerminalOptions, Pseudoterminal, Terminal, TerminalDimensions, TerminalExitReason, TerminalOptions, TerminalState, UIKind, window, workspace } from 'vscode';
+import { ConfigurationTarget, Disposable, env, EnvironmentVariableCollection, EnvironmentVariableMutator, EnvironmentVariableMutatorType, EnvironmentVariableScope, EventEmitter, ExtensionContext, extensions, ExtensionTerminalOptions, Pseudoterminal, Terminal, TerminalDimensions, TerminalExitReason, TerminalOptions, TerminalState, UIKind, Uri, window, workspace } from 'vscode';
 import { assertNoRpc, poll } from '../utils';
 
 // Disable terminal tests:
@@ -848,19 +848,45 @@ import { assertNoRpc, poll } from '../utils';
 				collection.replace('A', '~a2~');
 				collection.append('B', '~b2~');
 				collection.prepend('C', '~c2~');
-
 				// Verify get
-				deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined, options: {} });
-				deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined, options: {} });
-				deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined, options: {} });
-
+				deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} });
+				deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, options: {} });
+				deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} });
 				// Verify forEach
 				const entries: [string, EnvironmentVariableMutator][] = [];
 				collection.forEach((v, m) => entries.push([v, m]));
 				deepStrictEqual(entries, [
-					['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined, options: {} }],
-					['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined, options: {} }],
-					['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined, options: {} }]
+					['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} }],
+					['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, options: {} }],
+					['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} }]
+				]);
+			});
+
+			test('get and forEach should work (scope)', () => {
+				// TODO: Remove cast once `envCollectionWorkspace` API is finalized.
+				const collection = extensionContext.environmentVariableCollection as (EnvironmentVariableCollection & { getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection });
+				disposables.push({ dispose: () => collection.clear() });
+				const scope = { workspaceFolder: { uri: Uri.file('workspace1'), name: 'workspace1', index: 0 } };
+				const scopedCollection = collection.getScopedEnvironmentVariableCollection(scope);
+				scopedCollection.replace('A', 'scoped~a2~');
+				scopedCollection.append('B', 'scoped~b2~');
+				scopedCollection.prepend('C', 'scoped~c2~');
+				collection.replace('A', '~a2~');
+				collection.append('B', '~b2~');
+				collection.prepend('C', '~c2~');
+				// Verify get for scope
+				const expectedScopedCollection = collection.getScopedEnvironmentVariableCollection(scope);
+				deepStrictEqual(expectedScopedCollection.get('A'), { value: 'scoped~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} });
+				deepStrictEqual(expectedScopedCollection.get('B'), { value: 'scoped~b2~', type: EnvironmentVariableMutatorType.Append, options: {} });
+				deepStrictEqual(expectedScopedCollection.get('C'), { value: 'scoped~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} });
+
+				// Verify forEach
+				const entries: [string, EnvironmentVariableMutator][] = [];
+				expectedScopedCollection.forEach((v, m) => entries.push([v, m]));
+				deepStrictEqual(entries.map(v => v[1]), [
+					{ value: 'scoped~a2~', type: EnvironmentVariableMutatorType.Replace, options: {} },
+					{ value: 'scoped~b2~', type: EnvironmentVariableMutatorType.Append, options: {} },
+					{ value: 'scoped~c2~', type: EnvironmentVariableMutatorType.Prepend, options: {} }
 				]);
 			});
 		});

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

@@ -74,7 +74,8 @@ export interface IMergedEnvironmentVariableCollection {
 	getVariableMap(scope: EnvironmentVariableScope | undefined): Map<string, IExtensionOwnedEnvironmentVariableMutator[]>;
 	/**
 	 * Gets the description map for a given scope.
-	 * @param scope The scope to get the description map for. If undefined, the global scope is used.
+	 * @param scope The scope to get the description map for. If undefined, description map for the
+	 * global scope is returned.
 	 */
 	getDescriptionMap(scope: EnvironmentVariableScope | undefined): Map<string, string | undefined>;
 	/**

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

@@ -151,7 +151,7 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
 	getDescriptionMap(scope: EnvironmentVariableScope | undefined): Map<string, string | undefined> {
 		const result = new Map<string, string | undefined>();
 		this.descriptionMap.forEach((mutators, _key) => {
-			const filteredMutators = mutators.filter(m => filterScope(m, scope));
+			const filteredMutators = mutators.filter(m => filterScope(m, scope, true));
 			if (filteredMutators.length > 0) {
 				// There should be exactly one description per extension per scope.
 				result.set(filteredMutators[0].extensionIdentifier, filteredMutators[0].description);
@@ -190,11 +190,22 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
 	}
 }
 
+/**
+ * Returns whether a mutator matches with the scope provided.
+ * @param mutator Mutator to filter
+ * @param scope Scope to be used for querying
+ * @param strictFilter If true, mutators with global scope is not returned when querying for workspace scope.
+ * i.e whether mutator scope should always exactly match with query scope.
+ */
 function filterScope(
 	mutator: IExtensionOwnedEnvironmentVariableMutator | IExtensionOwnedEnvironmentDescriptionMutator,
-	scope: EnvironmentVariableScope | undefined
+	scope: EnvironmentVariableScope | undefined,
+	strictFilter = false
 ): boolean {
 	if (!mutator.scope) {
+		if (strictFilter) {
+			return scope === mutator.scope;
+		}
 		return true;
 	}
 	// If a mutator is scoped to a workspace folder, only apply it if the workspace

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

@@ -356,7 +356,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
 	protected _terminalProcessDisposables: { [id: number]: IDisposable } = {};
 	protected _extensionTerminalAwaitingStart: { [id: number]: { initialDimensions: ITerminalDimensionsDto | undefined } | undefined } = {};
 	protected _getTerminalPromises: { [id: number]: Promise<ExtHostTerminal | undefined> } = {};
-	protected _environmentVariableCollections: Map<string, EnvironmentVariableCollection> = new Map();
+	protected _environmentVariableCollections: Map<string, UnifiedEnvironmentVariableCollection> = new Map();
 	private _defaultProfile: ITerminalProfile | undefined;
 	private _defaultAutomationProfile: ITerminalProfile | undefined;
 
@@ -827,13 +827,13 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
 	public getEnvironmentVariableCollection(extension: IExtensionDescription): IEnvironmentVariableCollection {
 		let collection = this._environmentVariableCollections.get(extension.identifier.value);
 		if (!collection) {
-			collection = new EnvironmentVariableCollection(extension);
+			collection = new UnifiedEnvironmentVariableCollection(extension);
 			this._setEnvironmentVariableCollection(extension.identifier.value, collection);
 		}
 		return collection.getScopedEnvironmentVariableCollection(undefined);
 	}
 
-	private _syncEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
+	private _syncEnvironmentVariableCollection(extensionIdentifier: string, collection: UnifiedEnvironmentVariableCollection): void {
 		const serialized = serializeEnvironmentVariableCollection(collection.map);
 		const serializedDescription = serializeEnvironmentDescriptionMap(collection.descriptionMap);
 		this._proxy.$setEnvironmentVariableCollection(extensionIdentifier, collection.persistent, serialized.length === 0 ? undefined : serialized, serializedDescription);
@@ -842,7 +842,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
 	public $initEnvironmentVariableCollections(collections: [string, ISerializableEnvironmentVariableCollection][]): void {
 		collections.forEach(entry => {
 			const extensionIdentifier = entry[0];
-			const collection = new EnvironmentVariableCollection(undefined, entry[1]);
+			const collection = new UnifiedEnvironmentVariableCollection(undefined, entry[1]);
 			this._setEnvironmentVariableCollection(extensionIdentifier, collection);
 		});
 	}
@@ -856,7 +856,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
 		}
 	}
 
-	private _setEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
+	private _setEnvironmentVariableCollection(extensionIdentifier: string, collection: UnifiedEnvironmentVariableCollection): void {
 		this._environmentVariableCollections.set(extensionIdentifier, collection);
 		collection.onDidChangeCollection(() => {
 			// 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
 	}
 }
 
-class EnvironmentVariableCollection {
+/**
+ * Unified environment variable collection carrying information for all scopes, for a specific extension.
+ */
+class UnifiedEnvironmentVariableCollection {
 	readonly map: Map<string, IEnvironmentVariableMutator> = new Map();
 	private readonly scopedCollections: Map<string, ScopedEnvironmentVariableCollection> = new Map();
 	readonly descriptionMap: Map<string, IEnvironmentDescriptionMutator> = new Map();
@@ -930,9 +933,6 @@ class EnvironmentVariableCollection {
 		if (mutator.options && mutator.options.applyAtProcessCreation === false && !mutator.options.applyAtShellIntegration) {
 			throw new Error('EnvironmentVariableMutatorOptions must apply at either process creation or shell integration');
 		}
-		if (!mutator.scope) {
-			delete (mutator as any).scope; // Convenient for tests
-		}
 		const key = this.getKey(variable, mutator.scope);
 		const current = this.map.get(key);
 		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
 	get onDidChangeCollection(): Event<void> { return this._onDidChangeCollection && this._onDidChangeCollection.event; }
 
 	constructor(
-		private readonly collection: EnvironmentVariableCollection,
+		private readonly collection: UnifiedEnvironmentVariableCollection,
 		private readonly scope: vscode.EnvironmentVariableScope | undefined
 	) {
 	}
 
-	getScopedEnvironmentVariableCollection() {
-		return this.collection.getScopedEnvironmentVariableCollection(this.scope);
+	getScopedEnvironmentVariableCollection(scope: vscode.EnvironmentVariableScope | undefined) {
+		return this.collection.getScopedEnvironmentVariableCollection(scope);
 	}
 
 	replace(variable: string, value: string, options?: vscode.EnvironmentVariableMutatorOptions | undefined): void {
@@ -1120,7 +1120,7 @@ function asTerminalColor(color?: vscode.ThemeColor): ThemeColor | undefined {
 
 function convertMutator(mutator: IEnvironmentVariableMutator): vscode.EnvironmentVariableMutator {
 	const newMutator = { ...mutator };
-	newMutator.scope = newMutator.scope ?? undefined;
+	delete newMutator.scope;
 	newMutator.options = newMutator.options ?? undefined;
 	delete (newMutator as any).variable;
 	return newMutator as vscode.EnvironmentVariableMutator;

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

@@ -33,15 +33,7 @@ export class EnvironmentVariableInfoStale implements IEnvironmentVariableInfo {
 		addExtensionIdentifiers(extSet, this._diff.changed.values());
 
 		let message = localize('extensionEnvironmentContributionInfoStale', "The following extensions want to relaunch the terminal to contribute to its environment:");
-		message += '\n';
-		const descriptionMap = this._collection.getDescriptionMap(scope);
-		for (const ext of extSet) {
-			message += `\n- \`${getExtensionName(ext, this._extensionService)}\``;
-			const description = descriptionMap.get(ext);
-			if (description) {
-				message += `: ${description}`;
-			}
-		}
+		message += getMergedDescription(this._collection, scope, this._extensionService, extSet);
 		return message;
 	}
 
@@ -79,15 +71,7 @@ export class EnvironmentVariableInfoChangesActive implements IEnvironmentVariabl
 		addExtensionIdentifiers(extSet, this._collection.getVariableMap(scope).values());
 
 		let message = localize('extensionEnvironmentContributionInfoActive', "The following extensions have contributed to this terminal's environment:");
-		message += '\n';
-		const descriptionMap = this._collection.getDescriptionMap(scope);
-		for (const ext of extSet) {
-			message += `\n- \`${getExtensionName(ext, this._extensionService)}\``;
-			const description = descriptionMap.get(ext);
-			if (description) {
-				message += `: ${description}`;
-			}
-		}
+		message += getMergedDescription(this._collection, scope, this._extensionService, extSet);
 		return message;
 	}
 
@@ -109,6 +93,28 @@ export class EnvironmentVariableInfoChangesActive implements IEnvironmentVariabl
 	}
 }
 
+function getMergedDescription(collection: IMergedEnvironmentVariableCollection, scope: EnvironmentVariableScope | undefined, extensionService: IExtensionService, extSet: Set<string>): string {
+	const message = ['\n'];
+	const globalDescriptions = collection.getDescriptionMap(undefined);
+	const workspaceDescriptions = collection.getDescriptionMap(scope);
+	for (const ext of extSet) {
+		const globalDescription = globalDescriptions.get(ext);
+		if (globalDescription) {
+			message.push(`\n- \`${getExtensionName(ext, extensionService)}\``);
+			message.push(`: ${globalDescription}`);
+		}
+		const workspaceDescription = workspaceDescriptions.get(ext);
+		if (workspaceDescription) {
+			message.push(`\n- \`${getExtensionName(ext, extensionService)} (${localize('ScopedEnvironmentContributionInfo', 'workspace')})\``);
+			message.push(`: ${workspaceDescription}`);
+		}
+		if (!globalDescription && !workspaceDescription) {
+			message.push(`\n- \`${getExtensionName(ext, extensionService)}\``);
+		}
+	}
+	return message.join('');
+}
+
 function addExtensionIdentifiers(extSet: Set<string>, diff: IterableIterator<IExtensionOwnedEnvironmentVariableMutator[]>): void {
 	for (const mutators of diff) {
 		for (const mutator of mutators) {

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

@@ -181,6 +181,8 @@ suite('EnvironmentVariable - MergedEnvironmentVariableCollection', () => {
 			]));
 			deepStrictEqual([...merged.getDescriptionMap(scope1).entries()], [
 				['ext1', 'ext1 scope1 description'],
+			]);
+			deepStrictEqual([...merged.getDescriptionMap(undefined).entries()], [
 				['ext2', 'ext2 global description'],
 			]);
 		});