Support for terminal profiles. #12066

tsmaeder · 2023-01-13T06:57:54Z

What it does

Adds support for terminal profiles.

UI and service to manage terminal profiles
Handle profiles in preferences according to VS Code schema
API and contribution markup for contributing profiles and activation event handling

contributed on behalf of STMicroelectronics

How to test

This PR adds two new items under the "Terminal" menu: "New Terminal...", which allows to select a profile and use it to open a terminal and "Choose Default Profile", which selects the profile to be used when selecting "New Terminal" from the menu.
Some profiles are added by default ("cmd" by default on windows), but they can also be contributed by plugins or defined in the preferences.
Here are two extensions that can be used to test contributed terminals:

plugins.zip

One contributes a shell terminal profile ("JavaScript Debug Terminal") and the other a pseudo-pty terminal ("My Extension Terminal").

Adding profiles via terminals is described here:

https://code.visualstudio.com/docs/terminal/profiles

Note that terminal "source"s have not been implemented. Theia will accept the syntax, but the resulting profiles will not work.

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

vince-fugnitto

It seems that there are conflicts, do you mind rebasing?

packages/core/i18n/nls.de.json

vince-fugnitto · 2023-01-13T16:14:36Z

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

+    export const PROFILE_NEW = Command.toLocalizedCommand({
+        id: 'terminal:new:profile',
+        category: TERMINAL_CATEGORY,
+        label: 'Create New Integrated Terminal from a Profile'
+    });
+    export const PROFILE_DEFAULT = Command.toLocalizedCommand({
+        id: 'terminal:profile:default',
+        category: TERMINAL_CATEGORY,
+        label: 'Choose the default Terminal Profile'
+    });


These commands do not seem to be present in VS Code, and are not added to the terminal main-menu.
I do see the command 'Create New Terminal (With Profile)' however.

terminal-frontend-contribution.ts L 653-668?

I mean, these commands do not exist in VS Code, are they necessary?
Instead we should probably implement Create New Terminal (With Profile) which is present in VS Code and likely achieves what we are attempting here.

Yes, the UI (in particular choosing the default profile) is very different from what VS Code does. As for the "Create New Terminal (With Profile)", I would argue that my solution is superior: the "..." at the end of "New Profile" is a well established UI pattern indicating that the user will face a dialog to do some configuration or selection before running the action.
Remember that "do the same as VS Code" is not a strategic goal of the project anymore.

However in this case it adds entries in the terminal menu that downstream applications might not be expecting since in general we do follow the default implementation of VS Code. Moreover, plugins may attempt to trigger the workbench.action.terminal.newWithProfile command but it does not exist.

If we want to keep the commands that you've added on top of VS Code I'd maybe suggest we move them out of the terminal menu.

However in this case it adds entries in the terminal menu that downstream applications might not be expecting

Having the same menu structure as VS Code is not a promise I think we are making to our adopters. If downstream apps rely on that, we have a problem. Our UI with terminals and profiles is very different (no tabbed terminals, for example) and I feel it's a bit of a kludge in VS Code. It would be wrong to strive for visual compatibility in this respect, IMO.

Moreover, plugins may attempt to trigger the workbench.action.terminal.newWithProfile command but it does not exist.

That's the question I'm asking: is this command id VS Code published API we should support? I don't think it is.

These are, though: https://code.visualstudio.com/api/references/commands

That's the question I'm asking: is this command id VS Code published API we should support? I don't think it is.

I believe it is the main reason that command ids follow VS Code, since they can be ultimately executed from plugins.
In the case that we do not match their id, we have a mapping performed in the plugin system, for example:

theia/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

Lines 373 to 375 in d383fe2

commands.registerCommand({ id: 'workbench.action.files.saveAll', }, {

execute: () => this.shell.saveAll()

});

I understand you might be referring to builtin commands, but the page also describes "common commands" and such commands are also accessible by plugins.

Having a look at the broader picture here: all the other terminal-related commands do not have the same command ids as in VS Code (e.g. terminal:new instead of workbench.action.terminal.new. So having just the two new one's aligned seems odd. In this PR, the command ids are consistent with the rest of the terminal commands. If we want to align the command ids with VS Code, it should be done in a separate PR that covers all terminal-related command ids.

I've opened an issue: #12084

packages/terminal/src/browser/terminal-preferences.ts

msujew

I noticed a few issues when the frontend OS isn't the same as the backend OS. The preferences don't match up well.

packages/terminal/src/browser/terminal-preferences.ts

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

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

- UI and service to manage terminal profiles - Handle profiles in preferences according to VS Code schema - API and contribution markup for contributing profiles and activation event handling contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>

Signed-off-by: Thomas Mäder <[email protected]>

msujew

The changes look good to me. I would wait to merge until @vince-fugnitto has given his approval as well 👍

vince-fugnitto

The changes work well for me when I verified the functionality, I only have a minor comment regarding the new menu item.

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

Signed-off-by: Thomas Mäder <[email protected]>

vince-fugnitto

The latest updates look good to me 👍
I confirmed that the functionality works well and as expected, and the new API is marked as supported.

colin-grant-work · 2023-02-17T22:08:17Z

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

+            throw new Error('There are not profiles registered');
+        }
+        if (profile instanceof ShellTerminalProfile) {
+            profile = profile.modify({ rootURI: cwd });


@tsmaeder, I believe this should have just been modify({cwd}). rootURI is not a recognized field when these options are eventually passed into the TerminalWidgetFactory. As a consequence, running the new terminal command in a multi-root workspace ignores the user's selection of root and just goes with the first root no matter what. Alternatively, it should be converted to cwd when the widget is created.

colin-grant-work · 2023-02-17T22:17:04Z

packages/terminal/src/browser/shell-terminal-profile.ts

+    modify(options: IShellTerminalServerOptions): TerminalProfile {
+        return new ShellTerminalProfile(this.terminalService, { ...this.options, ...options });
+    }


This operation is mostly bogus, because IShellTerminalServerOptions (the function arg) and TerminalWidgetOptions (the constructor argument type) have very little in common.

tsmaeder · 2023-02-20T08:46:15Z

I think this is a leftover from prototype I did where I separated the code paths for shell terminals and other terminals (Pseudoterminal).

tsmaeder requested review from vince-fugnitto and msujew January 13, 2023 06:58

vince-fugnitto reviewed Jan 13, 2023

View reviewed changes

vince-fugnitto added terminal issues related to the terminal vscode issues related to VSCode compatibility labels Jan 13, 2023

msujew reviewed Jan 16, 2023

View reviewed changes

tsmaeder added 2 commits January 17, 2023 10:58

Addressed first batch of comments

eadd849

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder force-pushed the 11503_terminal_profile_api_2 branch from b6af1d8 to eadd849 Compare January 17, 2023 10:00

Use single string constants for i18n default strings

7293a08

Signed-off-by: Thomas Mäder <[email protected]>

msujew approved these changes Jan 18, 2023

View reviewed changes

vince-fugnitto reviewed Jan 18, 2023

View reviewed changes

packages/terminal/src/browser/terminal-frontend-contribution.ts Outdated Show resolved Hide resolved

Renamed profile:new

221976a

Signed-off-by: Thomas Mäder <[email protected]>

vince-fugnitto approved these changes Jan 19, 2023

View reviewed changes

tsmaeder merged commit 945a97b into eclipse-theia:master Jan 19, 2023

paul-marechal added this to the 1.34.0 milestone Jan 24, 2023

vince-fugnitto mentioned this pull request Jan 27, 2023

terminal: error thrown when attempting to set the default profile to '' #12119

Closed

tsmaeder mentioned this pull request Feb 1, 2023

Align terminal command ids with VS Code. Fixes #12084 #12134

Merged

1 task

colin-grant-work reviewed Feb 17, 2023

View reviewed changes

colin-grant-work mentioned this pull request Feb 17, 2023

ShellTerminalProfile modify method broken #12197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for terminal profiles. #12066

Support for terminal profiles. #12066

tsmaeder commented Jan 13, 2023

vince-fugnitto left a comment

vince-fugnitto Jan 13, 2023

tsmaeder Jan 16, 2023

vince-fugnitto Jan 16, 2023

tsmaeder Jan 17, 2023

vince-fugnitto Jan 17, 2023

tsmaeder Jan 17, 2023 •

edited

Loading

tsmaeder Jan 17, 2023

vince-fugnitto Jan 17, 2023

tsmaeder Jan 18, 2023

tsmaeder Jan 18, 2023

msujew left a comment

msujew left a comment

vince-fugnitto left a comment

vince-fugnitto left a comment

colin-grant-work Feb 17, 2023 •

edited

Loading

colin-grant-work Feb 17, 2023

tsmaeder commented Feb 20, 2023

	commands.registerCommand({ id: 'workbench.action.files.saveAll', }, {
	execute: () => this.shell.saveAll()
	});

Support for terminal profiles. #12066

Support for terminal profiles. #12066

Conversation

tsmaeder commented Jan 13, 2023

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a 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

tsmaeder Jan 17, 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

msujew left a comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Feb 20, 2023

tsmaeder Jan 17, 2023 •

edited

Loading

colin-grant-work Feb 17, 2023 •

edited

Loading