Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to environment collection workspace API #182238

Merged
merged 21 commits into from
May 18, 2023
Merged

Updates to environment collection workspace API #182238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f9a7029
approach 1
May 11, 2023
b90dd82
approach 2 part 1
May 11, 2023
44ccada
part 2
May 11, 2023
df10286
approach 2
May 12, 2023
69ce8fc
approach 3
May 12, 2023
436a801
Make scope mandatory so we dont miss to pass it
May 12, 2023
a2b0106
Remove redundant
May 12, 2023
16a8093
More
May 12, 2023
76f8b2c
Try to fix compile errors
May 12, 2023
4a0d85d
Code review
May 12, 2023
0a521c6
More
May 12, 2023
0185d27
Add `IDefaultEnvironmentVariableCollection`
May 12, 2023
3a4c2b1
Comment out the proposal to fix compile errors
May 18, 2023
88799ca
Temporarily finalize `getScopedEnvironmentVariableCollection`
May 18, 2023
affca3b
Fix error
May 18, 2023
0874a6a
Do not use spread
May 18, 2023
5749e3d
Revert "Fix error"
May 18, 2023
616fa57
Revert "Temporarily finalize `getScopedEnvironmentVariableCollection`"
May 18, 2023
db774ad
Simplify
May 18, 2023
64ee37a
Change how we get variable map
May 18, 2023
c2df3b7
Undo unnecessary cast
May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 105 additions & 33 deletions src/vs/workbench/api/common/extHostTerminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ export interface IExtHostTerminalService extends ExtHostTerminalServiceShape, ID
registerLinkProvider(provider: vscode.TerminalLinkProvider): vscode.Disposable;
registerProfileProvider(extension: IExtensionDescription, id: string, provider: vscode.TerminalProfileProvider): vscode.Disposable;
registerTerminalQuickFixProvider(id: string, extensionId: string, provider: vscode.TerminalQuickFixProvider): vscode.Disposable;
getEnvironmentVariableCollection(extension: IExtensionDescription, persistent?: boolean): vscode.EnvironmentVariableCollection;
getEnvironmentVariableCollection(extension: IExtensionDescription): IEnvironmentVariableCollection;
}

type IEnvironmentVariableCollection = vscode.EnvironmentVariableCollection & { getScopedEnvironmentVariableCollection(scope: vscode.EnvironmentVariableScope | undefined): vscode.EnvironmentVariableCollection };
export interface ITerminalInternalOptions {
isFeatureTerminal?: boolean;
useShellEnvironment?: boolean;
Expand Down Expand Up @@ -823,13 +823,13 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
return index;
}

public getEnvironmentVariableCollection(extension: IExtensionDescription): vscode.EnvironmentVariableCollection {
public getEnvironmentVariableCollection(extension: IExtensionDescription): IEnvironmentVariableCollection {
let collection = this._environmentVariableCollections.get(extension.identifier.value);
if (!collection) {
collection = new EnvironmentVariableCollection();
this._setEnvironmentVariableCollection(extension.identifier.value, collection);
}
return collection;
return collection.getScopedEnvironmentVariableCollection(undefined);
}

private _syncEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
Expand Down Expand Up @@ -867,8 +867,9 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
}
}

class EnvironmentVariableCollection implements vscode.EnvironmentVariableCollection {
class EnvironmentVariableCollection {
karrtikr marked this conversation as resolved.
Show resolved Hide resolved
readonly map: Map<string, IEnvironmentVariableMutator> = new Map();
private readonly scopedCollections: Map<string, ScopedEnvironmentVariableCollection> = new Map();
readonly descriptionMap: Map<string, IEnvironmentDescriptionMutator> = new Map();
private _persistent: boolean = true;

Expand All @@ -887,23 +888,30 @@ class EnvironmentVariableCollection implements vscode.EnvironmentVariableCollect
this.map = new Map(serialized);
}

get size(): number {
return this.map.size;
getScopedEnvironmentVariableCollection(scope: vscode.EnvironmentVariableScope | undefined): IEnvironmentVariableCollection {
const scopedCollectionKey = this.getScopeKey(scope);
let scopedCollection = this.scopedCollections.get(scopedCollectionKey);
if (!scopedCollection) {
scopedCollection = new ScopedEnvironmentVariableCollection(this, scope);
this.scopedCollections.set(scopedCollectionKey, scopedCollection);
scopedCollection.onDidChangeCollection(() => this._onDidChangeCollection.fire());
}
return scopedCollection;
}

replace(variable: string, value: string, scope?: vscode.EnvironmentVariableScope): void {
replace(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing you just haven't got around to removing the scope??

Copy link
Contributor Author

@karrtikr karrtikr May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me clarify my design, where I have no plans of removing scope internally.

EnvironmentVariableCollection class is used only created once for each extension and then is passed around, that carries information for all the scopes. As scope is not present in constructor, it is present in each method in the class as a parameter; We should probably rename it to ExtensionOwnedEnvVarCollection.

The external vscode.EnvironmentVariableCollection is different from this, it does not have scope as a parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably rename it to ExtensionOwnedEnvVarCollection

Little confusing as they are by definition owned by an extension

Copy link
Contributor Author

@karrtikr karrtikr May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can go something like the following if it's already clear that they're for a specific extension:

  • MergedExtensionEnvVarCollection
  • GlobalEnvironmentVariableCollection
  • ExtensionSpecificEnvironmentVariableCollection

Basically we want to indicate that it is a unified environment variable collection carrying information for all scopes, for a specific extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExtensionOwnedEnvVarCollection seemed appropriate as it is being used exactly that way in here:

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

Line 18 in 9b59b96

private readonly map: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();

IExtensionOwnedEnvironmentVariableMutator(s) came from ExtensionOwnedEnvVarCollection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IExtensionOwnedEnvironmentVariableMutator is just a mutator object but with the extension id attached. The environment variable collection is extension owned, but the mutator objects are owned by the collection and usually lack this property.

UnifiedEnvironmentVariableCollection is my favorite, but I'm not sure we should be renaming it as I believe it's only not-unified in the extension API. Adding "unified" throughout all usages wouldn't add much

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like MergedExtensionEnvVarCollection because that's what it is, but I agree it probably wouldn't add much. I'll add comments clarifying where it is defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's confusing though, merged environment variable collection is already a thing and the merging there means there is only a single value per mutator type + variable.

Copy link
Contributor Author

@karrtikr karrtikr May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove the "Merge" part, ExtensionEnvironmentVariableCollection. I'll add comments regardless clarifying.

this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Replace, scope });
}

append(variable: string, value: string, scope?: vscode.EnvironmentVariableScope): void {
append(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void {
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Append, scope });
}

prepend(variable: string, value: string, scope?: vscode.EnvironmentVariableScope): void {
prepend(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void {
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Prepend, scope });
}

private _setIfDiffers(variable: string, mutator: vscode.EnvironmentVariableMutator): void {
private _setIfDiffers(variable: string, mutator: vscode.EnvironmentVariableMutator & { scope: vscode.EnvironmentVariableScope | undefined }): void {
if (!mutator.scope) {
delete (mutator as any).scope; // Convenient for tests
}
Expand All @@ -917,44 +925,42 @@ class EnvironmentVariableCollection implements vscode.EnvironmentVariableCollect
}
}

get(variable: string, scope?: vscode.EnvironmentVariableScope): vscode.EnvironmentVariableMutator | undefined {
get(variable: string, scope: vscode.EnvironmentVariableScope | undefined): vscode.EnvironmentVariableMutator | undefined {
const key = this.getKey(variable, scope);
const value = this.map.get(key);
return value ? convertMutator(value) : undefined;
}

private getKey(variable: string, scope: vscode.EnvironmentVariableScope | undefined) {
const workspaceKey = this.getWorkspaceKey(scope?.workspaceFolder);
return workspaceKey ? `${variable}:::${workspaceKey}` : variable;
const scopeKey = this.getScopeKey(scope);
return scopeKey.length ? `${variable}:::${scopeKey}` : variable;
}

private getWorkspaceKey(workspaceFolder: vscode.WorkspaceFolder | undefined): string | undefined {
return workspaceFolder ? workspaceFolder.uri.toString() : undefined;
private getScopeKey(scope: vscode.EnvironmentVariableScope | undefined): string {
return this.getWorkspaceKey(scope?.workspaceFolder) ?? '';
}

forEach(callback: (variable: string, mutator: vscode.EnvironmentVariableMutator, collection: vscode.EnvironmentVariableCollection) => any, thisArg?: any): void {
this.map.forEach((value, _) => callback.call(thisArg, value.variable, convertMutator(value), this));
private getWorkspaceKey(workspaceFolder: vscode.WorkspaceFolder | undefined): string | undefined {
return workspaceFolder ? workspaceFolder.uri.toString() : undefined;
}

[Symbol.iterator](): IterableIterator<[variable: string, mutator: vscode.EnvironmentVariableMutator]> {
const map: Map<string, vscode.EnvironmentVariableMutator> = new Map();
this.map.forEach((mutator, _key) => {
if (mutator.scope) {
// Scoped mutators are not supported via this iterator, as it returns variable as the key which is supposed to be unique.
return;
public getVariableMap(scope: vscode.EnvironmentVariableScope | undefined): Map<string, IEnvironmentVariableMutator> {
const map = new Map<string, IEnvironmentVariableMutator>();
for (const [key, value] of this.map) {
if (this.getScopeKey(value.scope) === this.getScopeKey(scope)) {
map.set(key, value);
}
map.set(mutator.variable, convertMutator(mutator));
});
return map.entries();
}
return map;
}

delete(variable: string, scope?: vscode.EnvironmentVariableScope): void {
delete(variable: string, scope: vscode.EnvironmentVariableScope | undefined): void {
const key = this.getKey(variable, scope);
this.map.delete(key);
this._onDidChangeCollection.fire();
}

clear(scope?: vscode.EnvironmentVariableScope): void {
clear(scope: vscode.EnvironmentVariableScope | undefined): void {
if (scope?.workspaceFolder) {
for (const [key, mutator] of this.map) {
if (mutator.scope?.workspaceFolder?.index === scope.workspaceFolder.index) {
Expand All @@ -969,8 +975,8 @@ class EnvironmentVariableCollection implements vscode.EnvironmentVariableCollect
this._onDidChangeCollection.fire();
}

setDescription(description: string | vscode.MarkdownString | undefined, scope?: vscode.EnvironmentVariableScope): void {
const key = this.getKey('', scope);
setDescription(description: string | vscode.MarkdownString | undefined, scope: vscode.EnvironmentVariableScope | undefined): void {
const key = this.getScopeKey(scope);
const current = this.descriptionMap.get(key);
if (!current || current.description !== description) {
let descriptionStr: string | undefined;
Expand All @@ -986,12 +992,78 @@ class EnvironmentVariableCollection implements vscode.EnvironmentVariableCollect
}
}

private clearDescription(scope?: vscode.EnvironmentVariableScope): void {
const key = this.getKey('', scope);
public getDescription(scope: vscode.EnvironmentVariableScope | undefined): string | vscode.MarkdownString | undefined {
const key = this.getScopeKey(scope);
return this.descriptionMap.get(key)?.description;
}

private clearDescription(scope: vscode.EnvironmentVariableScope | undefined): void {
const key = this.getScopeKey(scope);
this.descriptionMap.delete(key);
}
}

class ScopedEnvironmentVariableCollection implements vscode.EnvironmentVariableCollection, IEnvironmentVariableCollection {
public get persistent(): boolean { return this.collection.persistent; }
public set persistent(value: boolean) {
this.collection.persistent = value;
}

protected readonly _onDidChangeCollection = new Emitter<void>();
get onDidChangeCollection(): Event<void> { return this._onDidChangeCollection && this._onDidChangeCollection.event; }

constructor(
private readonly collection: EnvironmentVariableCollection,
private readonly scope: vscode.EnvironmentVariableScope | undefined
) {
}

getScopedEnvironmentVariableCollection() {
return this.collection.getScopedEnvironmentVariableCollection(this.scope);
}

replace(variable: string, value: string): void {
this.collection.replace(variable, value, this.scope);
}

append(variable: string, value: string): void {
this.collection.append(variable, value, this.scope);
}

prepend(variable: string, value: string): void {
this.collection.prepend(variable, value, this.scope);
}

get(variable: string): vscode.EnvironmentVariableMutator | undefined {
return this.collection.get(variable, this.scope);
}

forEach(callback: (variable: string, mutator: vscode.EnvironmentVariableMutator, collection: vscode.EnvironmentVariableCollection) => any, thisArg?: any): void {
this.collection.getVariableMap(this.scope).forEach((value, variable) => callback.call(thisArg, variable, convertMutator(value), this), this.scope);
}

[Symbol.iterator](): IterableIterator<[variable: string, mutator: vscode.EnvironmentVariableMutator]> {
return this.collection.getVariableMap(this.scope).entries();
}

delete(variable: string): void {
this.collection.delete(variable, this.scope);
this._onDidChangeCollection.fire(undefined);
}

clear(): void {
this.collection.clear(this.scope);
}

set description(description: string | vscode.MarkdownString | undefined) {
this.collection.setDescription(description, this.scope);
}

get description(): string | vscode.MarkdownString | undefined {
return this.collection.getDescription(this.scope);
}
}

export class WorkerExtHostTerminalService extends BaseExtHostTerminalService {
constructor(
@IExtHostRpcService extHostRpc: IExtHostRpcService
Expand Down
43 changes: 20 additions & 23 deletions src/vscode-dts/vscode.proposed.envCollectionWorkspace.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,30 @@

declare module 'vscode' {

// https://github.com/microsoft/vscode/issues/171173
// https://github.com/microsoft/vscode/issues/182069

export interface EnvironmentVariableMutator {
readonly type: EnvironmentVariableMutatorType;
readonly value: string;
readonly scope: EnvironmentVariableScope | undefined;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
/**
* Sets a description for the environment variable collection, this will be used to describe the changes in the UI.
* @param description A description for the environment variable collection.
* @param scope Specific scope to which this description applies to.
*/
setDescription(description: string | MarkdownString | undefined, scope?: EnvironmentVariableScope): void;
replace(variable: string, value: string, scope?: EnvironmentVariableScope): void;
append(variable: string, value: string, scope?: EnvironmentVariableScope): void;
prepend(variable: string, value: string, scope?: EnvironmentVariableScope): void;
get(variable: string, scope?: EnvironmentVariableScope): EnvironmentVariableMutator | undefined;
delete(variable: string, scope?: EnvironmentVariableScope): void;
clear(scope?: EnvironmentVariableScope): void;
}
// export interface ExtensionContext {
// /**
// * Gets the extension's environment variable collection for this workspace, enabling changes
// * to be applied to terminal environment variables.
// *
// * @param scope The scope to which the environment variable collection applies to.
// */
// readonly environmentVariableCollection: EnvironmentVariableCollection & { getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection };
// }

export type EnvironmentVariableScope = {
/**
* The workspace folder to which this collection applies to. If unspecified, collection applies to all workspace folders.
*/
* Any specific workspace folder to get collection for. If unspecified, collection applicable to all workspace folders is returned.
*/
workspaceFolder?: WorkspaceFolder;
};

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
/**
* A description for the environment variable collection, this will be used to describe the
* changes in the UI.
*/
description: string | MarkdownString | undefined;
}
}