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.

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

[vscode] Support EnvironmentVariableCollection description #12696 #12838

Merged
merged 6 commits into from
Aug 25, 2023
3 changes: 2 additions & 1 deletion packages/plugin-ext/src/common/plugin-api-rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,8 @@ export interface TerminalServiceMain {
*/
$disposeByTerminalId(id: number, waitOnExit?: boolean | string): void;

$setEnvironmentVariableCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection | undefined): void;
$setEnvironmentVariableCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

The descriptions seems part of the SerializableEnvironmentCollection to me. Why not change the typing to be a proper object?

Copy link
Contributor Author

@jfaltermeier jfaltermeier Aug 23, 2023

Choose a reason for hiding this comment

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

I am not sure if I understand this, because there is no SerializableEnvironmentCollection. Are you referring to SerializableEnvironmentVariableCollection? This type is also used used in SerializableExtensionEnvironmentVariableCollection, which is actually persisted using the storageService. So I think changing this type/interface would break reading existing data.

But I guess we could add the description to SerializableExtensionEnvironmentVariableCollection and use $setEnvironmentVariableCollection(persistent: boolean, collection: SerializableExtensionEnvironmentVariableCollection): void; as a signature, if this does not count as an API break.

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've went with $setEnvironmentVariableCollection(persistent: boolean, collection: SerializableExtensionEnvironmentVariableCollection): void in the updated PR. Please let me know what you think.

description: string | MarkdownString | undefined): void;

/**
* Set the terminal widget name.
Expand Down
7 changes: 4 additions & 3 deletions packages/plugin-ext/src/main/browser/terminal-main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { interfaces } from '@theia/core/shared/inversify';
import { ApplicationShell, WidgetOpenerOptions } from '@theia/core/lib/browser';
import { TerminalEditorLocationOptions, TerminalOptions } from '@theia/plugin';
import { MarkdownString, TerminalEditorLocationOptions, TerminalOptions } from '@theia/plugin';
import { TerminalLocation, TerminalWidget } from '@theia/terminal/lib/browser/base/terminal-widget';
import { TerminalService } from '@theia/terminal/lib/browser/base/terminal-service';
import { TerminalServiceMain, TerminalServiceExt, MAIN_RPC_CONTEXT } from '../../common/plugin-api-rpc';
Expand Down Expand Up @@ -75,9 +75,10 @@ export class TerminalServiceMainImpl implements TerminalServiceMain, TerminalLin
return this.extProxy.$startProfile(id, CancellationToken.None);
}

$setEnvironmentVariableCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection | undefined): void {
$setEnvironmentVariableCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection | undefined,
description: string | MarkdownString | undefined): void {
if (collection) {
this.shellTerminalServer.setCollection(extensionIdentifier, persistent, collection);
this.shellTerminalServer.setCollection(extensionIdentifier, persistent, collection, description);
} else {
this.shellTerminalServer.deleteCollection(extensionIdentifier);
}
Expand Down
22 changes: 21 additions & 1 deletion packages/plugin-ext/src/plugin/terminal-ext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ import { RPCProtocol } from '../common/rpc-protocol';
import { Event, Emitter } from '@theia/core/lib/common/event';
import { Deferred } from '@theia/core/lib/common/promise-util';
import * as theia from '@theia/plugin';
import * as Converter from './type-converters';
import { Disposable, EnvironmentVariableMutatorType, TerminalExitReason, ThemeIcon } from './types-impl';
import { SerializableEnvironmentVariableCollection } from '@theia/terminal/lib/common/base-terminal-protocol';
import { ProvidedTerminalLink } from '../common/plugin-api-rpc-model';
import { ThemeIcon as MonacoThemeIcon } from '@theia/monaco-editor-core/esm/vs/platform/theme/common/themeService';
import { MarkdownString as MarkdownStringDTO } from '@theia/core/lib/common/markdown-rendering';

export function getIconUris(iconPath: theia.TerminalOptions['iconPath']): { id: string } | undefined {
if (ThemeIcon.is(iconPath)) {
Expand Down Expand Up @@ -313,7 +315,18 @@ export class TerminalServiceExtImpl implements TerminalServiceExt {

private syncEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
const serialized = [...collection.map.entries()];
this.proxy.$setEnvironmentVariableCollection(extensionIdentifier, collection.persistent, serialized.length === 0 ? undefined : serialized);
this.proxy.$setEnvironmentVariableCollection(extensionIdentifier, collection.persistent, serialized.length === 0 ? undefined : serialized,
this.descriptionToDTO(collection.description));
}

private descriptionToDTO(value: string | theia.MarkdownString | undefined): string | MarkdownStringDTO | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to move this to the Converter? Are there other locations that do the same conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to converter as fromMarkdownOrString.

if (value === undefined) {
return undefined;
} else if (typeof value === 'string') {
return value;
} else {
return Converter.fromMarkdown(value);
}
}

private setEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void {
Expand All @@ -339,8 +352,15 @@ export class TerminalServiceExtImpl implements TerminalServiceExt {

export class EnvironmentVariableCollection implements theia.EnvironmentVariableCollection {
readonly map: Map<string, theia.EnvironmentVariableMutator> = new Map();
private _description?: string | theia.MarkdownString;
private _persistent: boolean = true;

public get description(): string | theia.MarkdownString | undefined { return this._description; }
public set description(value: string | theia.MarkdownString | undefined) {
this._description = value;
this.onDidChangeCollectionEmitter.fire();
}

public get persistent(): boolean { return this._persistent; }
public set persistent(value: boolean) {
this._persistent = value;
Expand Down
6 changes: 6 additions & 0 deletions packages/plugin/src/theia.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3585,6 +3585,12 @@ export module '@theia/plugin' {
* A collection of mutations that an extension can apply to a process environment.
*/
export interface EnvironmentVariableCollection {

/**
* A description for the environment variable collection, this will be used to describe the changes in the UI.
*/
description: string | MarkdownString | undefined;

/**
* Whether the collection should be cached for the workspace and applied to the terminal
* across window reloads. When true the collection will be active immediately such when the
Expand Down
4 changes: 4 additions & 0 deletions packages/terminal/src/browser/base/terminal-widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { CommandLineOptions } from '@theia/process/lib/common/shell-command-buil
import { TerminalSearchWidget } from '../search/terminal-search-widget';
import { TerminalProcessInfo, TerminalExitReason } from '../../common/base-terminal-protocol';
import URI from '@theia/core/lib/common/uri';
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering/markdown-string';

export interface TerminalDimensions {
cols: number;
Expand Down Expand Up @@ -58,6 +59,9 @@ export abstract class TerminalWidget extends BaseWidget {
*/
abstract processInfo: Promise<TerminalProcessInfo>;

/** The extensions contributing to the environment of this terminal */
abstract contributingExtensions: Promise<Map<string, string | MarkdownString | undefined>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name is quite non-descriptive here: what is being held in this map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with envVarCollectionDescriptionsByExtension


/** Terminal kind that indicates whether a terminal is created by a user or by some extension for a user */
abstract readonly kind: 'user' | string;

Expand Down
18 changes: 16 additions & 2 deletions packages/terminal/src/browser/terminal-frontend-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
import {
ApplicationShell, KeybindingContribution, KeyCode, Key, WidgetManager, PreferenceService,
KeybindingRegistry, LabelProvider, WidgetOpenerOptions, StorageService, QuickInputService,
codicon, CommonCommands, FrontendApplicationContribution, OnWillStopAction, Dialog, ConfirmDialog, FrontendApplication, PreferenceScope, Widget
codicon, CommonCommands, FrontendApplicationContribution, OnWillStopAction, Dialog, ConfirmDialog, FrontendApplication, PreferenceScope, Widget, HoverService
} from '@theia/core/lib/browser';
import { TabBarToolbarContribution, TabBarToolbarRegistry } from '@theia/core/lib/browser/shell/tab-bar-toolbar';
import { TERMINAL_WIDGET_FACTORY_ID, TerminalWidgetFactoryOptions, TerminalWidgetImpl } from './terminal-widget-impl';
Expand Down Expand Up @@ -60,6 +60,8 @@ import { nls } from '@theia/core/lib/common/nls';
import { Profiles, TerminalPreferences } from './terminal-preferences';
import { ShellTerminalProfile } from './shell-terminal-profile';
import { VariableResolverService } from '@theia/variable-resolver/lib/browser';
import { TerminalInfoToolbarItem } from './terminal-info-toolbar-item';
import { MarkdownRenderer, MarkdownRendererFactory } from '@theia/core/lib/browser/markdown-rendering/markdown-renderer';

export namespace TerminalMenus {
export const TERMINAL = [...MAIN_MENU_BAR, '7_terminal'];
Expand Down Expand Up @@ -216,6 +218,17 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu
@inject(TerminalPreferences)
protected terminalPreferences: TerminalPreferences;

@inject(HoverService)
protected readonly hoverService: HoverService;

@inject(MarkdownRendererFactory) protected readonly markdownRendererFactory: MarkdownRendererFactory;

protected _markdownRenderer: MarkdownRenderer | undefined;
protected get markdownRenderer(): MarkdownRenderer {
this._markdownRenderer ||= this.markdownRendererFactory();
return this._markdownRenderer;
}

protected mergePreferencesPromise: Promise<void> = Promise.resolve();

protected readonly onDidCreateTerminalEmitter = new Emitter<TerminalWidget>();
Expand Down Expand Up @@ -250,7 +263,7 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu
this.storageService.getData<string>(ENVIRONMENT_VARIABLE_COLLECTIONS_KEY).then(data => {
if (data) {
const collectionsJson: SerializableExtensionEnvironmentVariableCollection[] = JSON.parse(data);
collectionsJson.forEach(c => this.shellTerminalServer.setCollection(c.extensionIdentifier, true, c.collection));
collectionsJson.forEach(c => this.shellTerminalServer.setCollection(c.extensionIdentifier, true, c.collection, undefined));
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make the description part of the env var collection, we can serialize it at the same time, right?

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've added the description to SerializableExtensionEnvironmentVariableCollection now.

}
});
});
Expand Down Expand Up @@ -731,6 +744,7 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu
}

registerToolbarItems(toolbar: TabBarToolbarRegistry): void {
toolbar.registerItem(new TerminalInfoToolbarItem(this.hoverService, this.markdownRenderer));
toolbar.registerItem({
id: TerminalCommands.SPLIT.id,
command: TerminalCommands.SPLIT.id,
Expand Down
113 changes: 113 additions & 0 deletions packages/terminal/src/browser/terminal-info-toolbar-item.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// *****************************************************************************
// Copyright (C) 2023 STMicroelectronics and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// http://www.eclipse.org/legal/epl-2.0.
//
// This Source Code may also be made available under the following Secondary
// Licenses when the conditions for such availability set forth in the Eclipse
// Public License v. 2.0 are satisfied: GNU General Public License, version 2
// with the GNU Classpath Exception which is available at
// https://www.gnu.org/software/classpath/license.html.
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
// *****************************************************************************
import { ReactTabBarToolbarItem } from '@theia/core/lib/browser/shell/tab-bar-toolbar';
import { ReactNode } from '@theia/core/shared/react';
import React = require('@theia/core/shared/react');
import { HoverService, Widget } from '@theia/core/lib/browser';
import { TerminalWidget } from './base/terminal-widget';
import { MarkdownRenderer } from '@theia/core/lib/browser/markdown-rendering/markdown-renderer';
import { MarkdownStringImpl } from '@theia/core/lib/common/markdown-rendering/markdown-string';
import { DisposableCollection } from '@theia/core';

export class TerminalInfoToolbarItem implements ReactTabBarToolbarItem {
readonly id = 'terminal:info';

constructor(
protected readonly hoverService: HoverService,
protected readonly markdownRenderer: MarkdownRenderer
) {}

isVisible(widget?: Widget): boolean {
return widget instanceof TerminalWidget;
}

render(widget?: Widget): ReactNode {
const toDispose = new DisposableCollection();
return (
<div
id={this.id}
className='codicon codicon-terminal-bash action-label'
onMouseEnter={e => this.onMouseEnter(e, toDispose, widget)}
onMouseLeave={e => this.onMouseLeave(toDispose)}
></div>
);
}

protected async onMouseEnter(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems odd that we have to implement our own "show hover on mouse-over" support. Dont' we have this somewhere else already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I am aware of. I think the reusable functionality is all in the HoverService. However we have to call hoverService.requestHover from somewhere. Here we have drawn the div over which the hover will appear ourself and the onMouseEnter only creates the custom hover ui and then calls the Hover Service. I don't think we could omit much of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code removed because of usage of enhanced preview

event: React.MouseEvent<HTMLElement, MouseEvent>, toDispose: DisposableCollection, currentTerminal?: Widget
): Promise<void> {
const currentTarget = event.currentTarget;
if (currentTerminal instanceof TerminalWidget) {
const extensions = await currentTerminal.contributingExtensions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make more sense to let the TerminalWidget render (and cache) a Markdown string that we can just render with the markdow renderer instead of manipulating divs ourselves here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When adapting the enhanced preview this definitely makes sense. So I will change this.

const processId = await currentTerminal.processId;
const processInfo = await currentTerminal.processInfo;

const mainDiv = document.createElement('div');

const pid = document.createElement('div');
pid.textContent = 'Process ID: ' + processId;
mainDiv.appendChild(pid);

const commandLine = document.createElement('div');
commandLine.textContent =
'Command line: ' +
processInfo.executable +
' ' +
processInfo.arguments.join(' ');
mainDiv.appendChild(commandLine);

mainDiv.appendChild(document.createElement('hr'));

const header = document.createElement('div');
header.textContent =
'The following extensions have contributed to this terminal\'s environment:';
mainDiv.appendChild(header);

const list = document.createElement('ul');
mainDiv.appendChild(list);

extensions.forEach((value, key) => {
const item = document.createElement('li');
let markdown;
if (value === undefined) {
markdown = new MarkdownStringImpl('');
markdown.appendText(key);
} else if (typeof value === 'string') {
markdown = new MarkdownStringImpl('');
markdown.appendText(key + ': ' + value);
} else {
markdown = new MarkdownStringImpl('', value);
markdown.appendText(key + ': ');
markdown.appendMarkdown(value.value);
}
const result = this.markdownRenderer.render(markdown);
toDispose.push(result);
item.appendChild(result.element);
list.appendChild(item);
});

this.hoverService.requestHover({
content: mainDiv,
target: currentTarget,
position: 'right',
});
}
}

protected async onMouseLeave(toDispose: DisposableCollection): Promise<void> {
toDispose.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

toDispose is too generic. How about hoverResources.dispose()

}
}
8 changes: 8 additions & 0 deletions packages/terminal/src/browser/terminal-widget-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { Key } from '@theia/core/lib/browser/keys';
import { nls } from '@theia/core/lib/common/nls';
import { TerminalMenus } from './terminal-frontend-contribution';
import debounce = require('p-debounce');
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering/markdown-string';

export const TERMINAL_WIDGET_FACTORY_ID = 'terminal';

Expand Down Expand Up @@ -422,6 +423,13 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
return this.shellTerminalServer.getProcessInfo(this.terminalId);
}

get contributingExtensions(): Promise<Map<string, string | MarkdownString | undefined>> {
if (!IBaseTerminalServer.validateId(this.terminalId)) {
return Promise.reject(new Error('terminal is not started'));
}
return this.shellTerminalServer.getContributingExtensions(this.terminalId);
}

get terminalId(): number {
return this._terminalId;
}
Expand Down
5 changes: 4 additions & 1 deletion packages/terminal/src/common/base-terminal-protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import { RpcServer } from '@theia/core/lib/common/messaging/proxy-factory';
import { Disposable } from '@theia/core';
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering/markdown-string';

export interface TerminalProcessInfo {
executable: string
Expand All @@ -28,6 +29,7 @@ export interface IBaseTerminalServer extends RpcServer<IBaseTerminalClient> {
create(IBaseTerminalServerOptions: object): Promise<number>;
getProcessId(id: number): Promise<number>;
getProcessInfo(id: number): Promise<TerminalProcessInfo>;
getContributingExtensions(id: number): Promise<Map<string, string | MarkdownString | undefined>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

envVarCollectionDescriptionsByExtension?

getCwdURI(id: number): Promise<string>;
resize(id: number, cols: number, rows: number): Promise<void>;
attach(id: number): Promise<number>;
Expand All @@ -48,7 +50,7 @@ export interface IBaseTerminalServer extends RpcServer<IBaseTerminalClient> {
/**
* Sets an extension's environment variable collection.
*/
setCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection): void;
setCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection, description: string | MarkdownString | undefined): void;
/**
* Deletes an extension's environment variable collection.
*/
Expand Down Expand Up @@ -154,6 +156,7 @@ export interface EnvironmentVariableCollection {

export interface EnvironmentVariableCollectionWithPersistence extends EnvironmentVariableCollection {
readonly persistent: boolean;
readonly description: string | MarkdownString | undefined;
}

export enum EnvironmentVariableMutatorType {
Expand Down
17 changes: 15 additions & 2 deletions packages/terminal/src/node/base-terminal-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
} from '../common/base-terminal-protocol';
import { TerminalProcess, ProcessManager, TaskTerminalProcess } from '@theia/process/lib/node';
import { ShellProcess } from './shell-process';
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering/markdown-string';

@injectable()
export abstract class BaseTerminalServer implements IBaseTerminalServer {
Expand Down Expand Up @@ -100,6 +101,18 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer {
};
}

async getContributingExtensions(id: number): Promise<Map<string, string | MarkdownString | undefined>> {
const terminal = this.processManager.get(id);
if (!(terminal instanceof TerminalProcess)) {
throw new Error(`terminal "${id}" does not exist`);
}
const result = new Map<string, string | MarkdownString | undefined>();
this.collections.forEach((value, key) => {
result.set(key, value.description);
});
return result;
}

async getCwdURI(id: number): Promise<string> {
const terminal = this.processManager.get(id);
if (!(terminal instanceof TerminalProcess)) {
Expand Down Expand Up @@ -176,8 +189,8 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer {
*--------------------------------------------------------------------------------------------*/
// some code copied and modified from https://github.com/microsoft/vscode/blob/1.49.0/src/vs/workbench/contrib/terminal/common/environmentVariableService.ts

setCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection): void {
const translatedCollection = { persistent, map: new Map<string, EnvironmentVariableMutator>(collection) };
setCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection, description: string | MarkdownString | undefined): void {
const translatedCollection = { persistent, description, map: new Map<string, EnvironmentVariableMutator>(collection) };
this.collections.set(extensionIdentifier, translatedCollection);
this.updateCollections();
}
Expand Down