[vscode] Support EnvironmentVariableCollection description #12696 #12838

jfaltermeier · 2023-08-17T09:20:23Z

What it does

The first commit adds the new description field to EnvironmentVariableCollection . This will be enough to be compatible with VSCode 1.80.

Further commits add ui that shows the descriptions to the user.
VSCode does it like this: https://code.visualstudio.com/updates/v1_80#_environmentvariablecollectiondescription
As far as I know we did not have a similar ui yet, so I've started to implement one. The results looks like this atm:

This is an enhanced preview for the terminal widget that shows the descriptions on hover, similar to VSCode. I've also included PID and Command Line like in VSCode.

Fixes #12696

Contributed on behalf of STMicroelectronics

Signed-off-by: Johannes Faltermeier jfaltermeier@eclipsesource.com

How to test

I've prepared three extensions that set either an undefined description, a string description, or a markdown description.

Sources:
env-description-sources.zip

Extensions:
env-description-extensions.zip

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

* add new field to EnvironmentVariableCollection and implement interface Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

* show information in UI Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

tsmaeder · 2023-08-23T08:21:01Z

@jfaltermeier it's not clear to me how I can test this: (having no familiarity with EnvVar collections. Do I just open a terminal?

* fix toolbar item visibility Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

jfaltermeier · 2023-08-23T09:14:50Z

@jfaltermeier it's not clear to me how I can test this: (having no familiarity with EnvVar collections. Do I just open a terminal?

@tsmaeder VSCode Extension may change the env variables in a new terminal. E.g. the builtin git extension will set GIT_ASKPASS:

echo $GIT_ASKPASS
/usr/share/code/resources/app/extensions/git/dist/askpass.sh

This functionality was existing in Theia already. So I haven't tested this part much.
The added VSCode API allows to describe the changes made to the environment to the users.

In the three test extensions I attached I am changing FOO, BAR, and FOOBAR.

So you may start with some default values and look at them in the terminal before installing the extensions.

# in terminal used for launching theia:
export FOO=hello
export BAR=world
export FOOBAR=123

# yarn electron start
# open terminal in started Theia and run

echo $FOO
echo $BAR
echo $FOOBAR

Then install the extensions, close and reopen a terminal. This should now show changed values:

$ echo $FOO
echo $BAR
echo $FOOBAR
markdown
string
undefined

The new toolbar item shows the description strings on hover, one being a markdown string, one a regular string, and one being a missing undefined description.

Please note that I pushed one additional commit, because I noticed to toolbar item was showing in too many locations.

tsmaeder · 2023-08-23T12:03:16Z

I don't think I like the toolbar button as a UI here: in VS Code, I can press the button and it produces a drop-down menu. Toolbars are for actions, not pure hover targets: as a user, I would think something is broken if I have a toolbar button with no function.
In VS Code, as soon as you have more than one terminal, you get a "tab bar" with an entry for each open terminal. Since we open a new tab for each terminal, I think showing the descriptions as a hover on the tab would be more logical. I think with the "enhancecd preview" support for tabs we have, this should be possible. WDYT?

tsmaeder · 2023-08-23T08:52:37Z

packages/plugin-ext/src/common/plugin-api-rpc.ts

@@ -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,


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

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.

I've went with $setEnvironmentVariableCollection(persistent: boolean, collection: SerializableExtensionEnvironmentVariableCollection): void in the updated PR. Please let me know what you think.

tsmaeder · 2023-08-23T12:04:26Z

packages/plugin-ext/src/plugin/terminal-ext.ts

+            this.descriptionToDTO(collection.description));
+    }
+
+    private descriptionToDTO(value: string | theia.MarkdownString | undefined): string | MarkdownStringDTO | undefined {


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

Moved to converter as fromMarkdownOrString.

tsmaeder · 2023-08-23T12:07:19Z

packages/terminal/src/browser/base/terminal-widget.ts

@@ -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>>;


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

Went with envVarCollectionDescriptionsByExtension

tsmaeder · 2023-08-23T12:08:48Z

packages/terminal/src/browser/terminal-frontend-contribution.ts

@@ -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));


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

I've added the description to SerializableExtensionEnvironmentVariableCollection now.

tsmaeder · 2023-08-23T12:20:38Z

packages/terminal/src/browser/terminal-info-toolbar-item.tsx

+        );
+    }
+
+    protected async onMouseEnter(


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

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.

Code removed because of usage of enhanced preview

tsmaeder · 2023-08-23T12:23:15Z

packages/terminal/src/browser/terminal-info-toolbar-item.tsx

+    ): Promise<void> {
+        const currentTarget = event.currentTarget;
+        if (currentTerminal instanceof TerminalWidget) {
+            const extensions = await currentTerminal.contributingExtensions;


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?

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

tsmaeder · 2023-08-23T12:25:42Z

packages/terminal/src/browser/terminal-info-toolbar-item.tsx

+    }
+
+    protected async onMouseLeave(toDispose: DisposableCollection): Promise<void> {
+        toDispose.dispose();


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

tsmaeder · 2023-08-23T12:27:19Z

packages/terminal/src/common/base-terminal-protocol.ts

@@ -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>>;


envVarCollectionDescriptionsByExtension?

jfaltermeier · 2023-08-23T13:07:56Z

I don't think I like the toolbar button as a UI here: in VS Code, I can press the button and it produces a drop-down menu. Toolbars are for actions, not pure hover targets: as a user, I would think something is broken if I have a toolbar button with no function. In VS Code, as soon as you have more than one terminal, you get a "tab bar" with an entry for each open terminal. Since we open a new tab for each terminal, I think showing the descriptions as a hover on the tab would be more logical. I think with the "enhancecd preview" support for tabs we have, this should be possible. WDYT?

Thanks for the review, I will have a look at the comments.

Regarding the Preview:
I had a look at the enhanced preview initially and imo the problem is that it just uses the widget title's label and caption, which are both strings:

theia/packages/core/src/browser/shell/tab-bars.ts

Line 500 in 628d6d2

protected renderEnhancedPreview = (title: Title<Widget>) => {

The env var collection's description may be a markdownstring however. Is there a way to transform this to a regular string while keeping the formatting?

We could probably try the visualPreview, but this seems hacky as we are showing something different that the visual preview

tsmaeder · 2023-08-23T14:53:40Z

Why not just do the same thing as for the visual preview? Test whether the Widget supports rendering an "enhanced" title hover and if not fall back to the current code? We could then implement the protocol for the TerminalWidget.

* add description to SerializableExtensionEnvironmentVariableCollection and use as DTO in $setEnvironmentVariableCollection * add fromMarkdownOrString method to converter * review comments Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

* allow widgets to customize the enhanced preview node Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

* implement enhanced preview for terminal widget * remove terminal info toolbar item Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

jfaltermeier · 2023-08-24T08:25:44Z

I've addressed the comments in some follow up commits. The enhanced preview looks like this atm:

tsmaeder

Just one more question.

packages/core/src/browser/widgets/enhanced-preview-widget.ts

[vscode] Support EnvironmentVariableCollection description #12696

f45ef07

* add new field to EnvironmentVariableCollection and implement interface Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

jfaltermeier force-pushed the jf/EnvironmentVariableCollection.description branch from 60625a5 to 085c0e9 Compare August 17, 2023 09:26

jfaltermeier changed the title ~~[vscode] Support EnvironmentVariableCollection description #12696~~ [WIP] [vscode] Support EnvironmentVariableCollection description #12696 Aug 17, 2023

jfaltermeier force-pushed the jf/EnvironmentVariableCollection.description branch from 085c0e9 to f8adc0a Compare August 17, 2023 10:13

[vscode] Support EnvironmentVariableCollection description #12696

10df2a8

* show information in UI Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

jfaltermeier force-pushed the jf/EnvironmentVariableCollection.description branch from f8adc0a to 10df2a8 Compare August 17, 2023 10:17

jfaltermeier changed the title ~~[WIP] [vscode] Support EnvironmentVariableCollection description #12696~~ [vscode] Support EnvironmentVariableCollection description #12696 Aug 17, 2023

jfaltermeier marked this pull request as ready for review August 17, 2023 10:48

jfaltermeier requested a review from tsmaeder August 17, 2023 10:48

vince-fugnitto added the vscode issues related to VSCode compatibility label Aug 18, 2023

[vscode] Support EnvironmentVariableCollection description #12696

c27c87d

* fix toolbar item visibility Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

tsmaeder reviewed Aug 23, 2023

View reviewed changes

jfaltermeier added 3 commits August 23, 2023 17:58

[vscode] Introduce EnhancedPreviewWidget interface #12696

81de14d

* allow widgets to customize the enhanced preview node Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

[vscode] Support EnvironmentVariableCollection description #12696

bab13b9

* implement enhanced preview for terminal widget * remove terminal info toolbar item Contributed on behalf of STMicroelectronics Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>

jfaltermeier force-pushed the jf/EnvironmentVariableCollection.description branch from 1c9b127 to bab13b9 Compare August 24, 2023 07:48

jfaltermeier requested a review from tsmaeder August 24, 2023 08:24

tsmaeder reviewed Aug 24, 2023

View reviewed changes

packages/core/src/browser/widgets/enhanced-preview-widget.ts Show resolved Hide resolved

tsmaeder approved these changes Aug 25, 2023

View reviewed changes

jfaltermeier merged commit 5720724 into master Aug 25, 2023

jfaltermeier deleted the jf/EnvironmentVariableCollection.description branch August 25, 2023 10:19

github-actions bot added this to the 1.41.0 milestone Aug 25, 2023

jfaltermeier mentioned this pull request Aug 28, 2023

Bump API version to 1.80.0 #12866

Merged

1 task

magnologan mentioned this pull request Nov 27, 2023

[Snyk] Fix for 7 vulnerabilities magnologan/theia#32

Open

This was referenced Nov 28, 2023

[Snyk] Fix for 7 vulnerabilities magnologan/theia#35

Open

[Snyk] Fix for 7 vulnerabilities magnologan/theia#42

Open

[Snyk] Fix for 7 vulnerabilities magnologan/theia#44

Open

[Snyk] Fix for 7 vulnerabilities magnologan/theia#49

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vscode] Support EnvironmentVariableCollection description #12696 #12838

[vscode] Support EnvironmentVariableCollection description #12696 #12838

jfaltermeier commented Aug 17, 2023 •

edited

Loading

tsmaeder commented Aug 23, 2023

jfaltermeier commented Aug 23, 2023 •

edited

Loading

tsmaeder commented Aug 23, 2023

tsmaeder Aug 23, 2023

jfaltermeier Aug 23, 2023 •

edited

Loading

jfaltermeier Aug 24, 2023

tsmaeder Aug 23, 2023

jfaltermeier Aug 24, 2023

tsmaeder Aug 23, 2023

jfaltermeier Aug 24, 2023

tsmaeder Aug 23, 2023

jfaltermeier Aug 24, 2023

tsmaeder Aug 23, 2023

jfaltermeier Aug 23, 2023

jfaltermeier Aug 24, 2023

tsmaeder Aug 23, 2023

jfaltermeier Aug 23, 2023

tsmaeder Aug 23, 2023

tsmaeder Aug 23, 2023

jfaltermeier commented Aug 23, 2023

tsmaeder commented Aug 23, 2023

jfaltermeier commented Aug 24, 2023

tsmaeder left a comment

[vscode] Support EnvironmentVariableCollection description #12696 #12838

[vscode] Support EnvironmentVariableCollection description #12696 #12838

Conversation

jfaltermeier commented Aug 17, 2023 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

tsmaeder commented Aug 23, 2023

jfaltermeier commented Aug 23, 2023 • edited Loading

tsmaeder commented Aug 23, 2023

Choose a reason for hiding this comment

jfaltermeier Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfaltermeier commented Aug 23, 2023

tsmaeder commented Aug 23, 2023

jfaltermeier commented Aug 24, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

jfaltermeier commented Aug 17, 2023 •

edited

Loading

jfaltermeier commented Aug 23, 2023 •

edited

Loading

jfaltermeier Aug 23, 2023 •

edited

Loading