From d3ffc565eae2d186486924ab74963e13dfa9e0cc Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 23 May 2023 17:35:54 +0000 Subject: [PATCH] Correctly show descriptions for each scope --- .../src/singlefolder-tests/terminal.test.ts | 44 +++++++++++++++---- .../terminal/common/environmentVariable.ts | 3 +- .../common/environmentVariableCollection.ts | 15 ++++++- .../api/common/extHostTerminalService.ts | 26 +++++------ .../browser/environmentVariableInfo.ts | 42 ++++++++++-------- .../environmentVariableCollection.test.ts | 2 + 6 files changed, 89 insertions(+), 43 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index e902bcd04a7cc..869db49105915 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/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: {} } ]); }); }); diff --git a/src/vs/platform/terminal/common/environmentVariable.ts b/src/vs/platform/terminal/common/environmentVariable.ts index 8ff5a08abc495..3798d1672c6f2 100644 --- a/src/vs/platform/terminal/common/environmentVariable.ts +++ b/src/vs/platform/terminal/common/environmentVariable.ts @@ -74,7 +74,8 @@ export interface IMergedEnvironmentVariableCollection { getVariableMap(scope: EnvironmentVariableScope | undefined): Map; /** * 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; /** diff --git a/src/vs/platform/terminal/common/environmentVariableCollection.ts b/src/vs/platform/terminal/common/environmentVariableCollection.ts index 67b0e2a534beb..2d50644a58f93 100644 --- a/src/vs/platform/terminal/common/environmentVariableCollection.ts +++ b/src/vs/platform/terminal/common/environmentVariableCollection.ts @@ -151,7 +151,7 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa getDescriptionMap(scope: EnvironmentVariableScope | undefined): Map { const result = new Map(); 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 diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 78a1d4a52919f..d20f3a89f6726 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/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 } = {}; - protected _environmentVariableCollections: Map = new Map(); + protected _environmentVariableCollections: Map = 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 = new Map(); private readonly scopedCollections: Map = new Map(); readonly descriptionMap: Map = 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 { 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; diff --git a/src/vs/workbench/contrib/terminal/browser/environmentVariableInfo.ts b/src/vs/workbench/contrib/terminal/browser/environmentVariableInfo.ts index 242e6c16ffdec..0b9638e8e6b81 100644 --- a/src/vs/workbench/contrib/terminal/browser/environmentVariableInfo.ts +++ b/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 { + 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, diff: IterableIterator): void { for (const mutators of diff) { for (const mutator of mutators) { diff --git a/src/vs/workbench/contrib/terminal/test/common/environmentVariableCollection.test.ts b/src/vs/workbench/contrib/terminal/test/common/environmentVariableCollection.test.ts index cffe7c45081c1..cb047ca042bb0 100644 --- a/src/vs/workbench/contrib/terminal/test/common/environmentVariableCollection.test.ts +++ b/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'], ]); });