Skip to content

Commit

Permalink
Merge pull request #182914 from microsoft/kartik/terminalupdates
Browse files Browse the repository at this point in the history 
Correctly show descriptions for each scope
  • Loading branch information
Kartik Raj authored May 23, 2023
2 parents 75b90a6 + d3ffc56 commit 046a314
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 43 deletions.
44 changes: 35 additions & 9 deletions extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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: {} }
]);
});
});
Expand Down
3 changes: 2 additions & 1 deletion src/vs/platform/terminal/common/environmentVariable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
/**
Expand Down
15 changes: 13 additions & 2 deletions src/vs/platform/terminal/common/environmentVariableCollection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
26 changes: 13 additions & 13 deletions src/vs/workbench/api/common/extHostTerminalService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
}
Expand All @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
42 changes: 24 additions & 18 deletions src/vs/workbench/contrib/terminal/browser/environmentVariableInfo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions src/vs/workbench/contrib/terminal/test/common/environmentVariableCollection.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
]);
});
Expand Down

0 comments on commit 046a314

Please sign in to comment.