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

11503 terminal profile api 2 #60

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tsmaeder
Copy link
Collaborator

What it does

Adds support for terminal profiles.

Fixes #11503

  • 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 ST Microelectronics

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

Reminder for reviewers

@sdirix sdirix requested a review from tortmayr January 4, 2023 08:05
Copy link

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Terminal profiles are definitely a nice addition to Theia, thanks!

In general the change looks good to me and seems to work well for the most part. (Tested on Linux & Windows)

However, there seems to be a problem with persisting the setting for the default profile.
If I change the default terminal profile, everything works as expected during the active session (i.e. new terminals spawn with the new profile), When I close and reopen Theia the setting is lost and the system default for the terminal profile is used again. The corresponding setting (terminal.integrated.defaultProfileXX) is set but it seems to be ignored.

Secondly, I noticed that the cmd Profile is not working on Window when using node 14. For me it just spawns empty new terminal windows which I cannot interact with.
With node 16 everything works as expected.

The default Profile for Linux is also calledCMD which is a bit confusing.

Compared to VS Code I noticed that some nice-to-have features are missing. They don't necessarily have to be part of this initial contribution and can also be tackled with a follow-up PR:

  • No system default profiles.
    Depending on the underlying OS VS Code creates default profiles for the installed terminals e.g. "CMD, PowerShell & Gitbash" for Windows or "Bash, Zash and Tmux" for Linux. This would be really usefull for Theia as well

  • In VS Code the settings UI element for setting the default profile is a selection box rather than a plain text box. Maybe we should implement it in the same way in Theia?

packages/core/i18n/nls.de.json Outdated Show resolved Hide resolved
packages/core/i18n/nls.de.json Outdated Show resolved Hide resolved
packages/core/i18n/nls.de.json Outdated Show resolved Hide resolved
packages/plugin-ext/src/common/plugin-protocol.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-profile-service.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-profile-service.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/shell-terminal-profile.ts Outdated Show resolved Hide resolved
@tsmaeder
Copy link
Collaborator Author

tsmaeder commented Jan 9, 2023

However, there seems to be a problem with persisting the setting for the default profile. If I change the default terminal profile, everything works as expected during the active session (i.e. new terminals spawn with the new profile), When I close and reopen Theia the setting is lost and the system default for the terminal profile is used again. The corresponding setting (terminal.integrated.defaultProfileXX) is set but it seems to be ignored.

that's odd. I'll have a look.

Secondly, I noticed that the cmd Profile is not working on Window when using node 14. For me it just spawns empty new terminal windows which I cannot interact with. With node 16 everything works as expected.

Good catch, strange that this would make a difference 🤷‍♂️

The default Profile for Linux is also calledCMD which is a bit confusing.

I guess that's a copy/paste error in the preferences

Compared to VS Code I noticed that some nice-to-have features are missing. They don't necessarily have to be part of this initial contribution and can also be tackled with a follow-up PR:

* No system default profiles.
  Depending on the underlying OS VS Code creates default profiles for the installed terminals e.g. "CMD, PowerShell & Gitbash" for Windows or "Bash, Zash and Tmux" for Linux. This would be really usefull for Theia as well

Yes, I considered that out of scope, given that the issue I'm fixing is to implement the VS Code profiles API.

* In VS Code the settings UI element for setting the default profile is a selection box rather than a plain text box. Maybe we should implement it in the same way in Theia?

Which UI element do you mean? The one when you do F1-Profile: Select Default Profile? How is that different from what we have?

@tortmayr
Copy link

I can confirm that the default profile is now correclty loaded on startup and that the OS default Profile for Linux is now named "SHELL".

Yes, I considered that out of scope, given that the issue I'm fixing is to implement the VS Code profiles API.

Agreed, definitely out of scope for this issue

Which UI element do you mean? The one when you do F1-Profile: Select Default Profile? How is that different from what we have?

I was talking about the text box in the Preferences UI for setting the default profile (terminal.integrated.defaultProfile.xxx):
Screenshot from 2023-01-10 14-34-48

@tsmaeder
Copy link
Collaborator Author

@tortmayr I just built and ran the browser version with nodejs v14.21.2. Running the 'cmd' profile works just fine for me. Do you maybe have a 'cmd' profile defined in the settings which might override the built-in?

@tsmaeder tsmaeder force-pushed the 11503_terminal_profile_api_2 branch from 324e982 to be25212 Compare January 12, 2023 15:27
@tsmaeder
Copy link
Collaborator Author

Which UI element do you mean? The one when you do F1-Profile: Select Default Profile? How is that different from what we have?

I was talking about the text box in the Preferences UI for setting the default profile (terminal.integrated.defaultProfile.xxx):

Ah, OK. T.b.h, I would defer that to a future task: I don't think it's that important, since there is an alternative via the terminal menu (which I would expect most people to use), and I'd would rather focus on making sure we get the PR merged for the community release, as this would allow us to raise the support API version.

@tortmayr tortmayr self-requested a review January 12, 2023 16:58
Copy link

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks Thomas. Change looks good to me now. 👍🏼

I also can no longer reproduce the node 14 issues.

@tsmaeder tsmaeder force-pushed the 11503_terminal_profile_api_2 branch from be25212 to b6af1d8 Compare January 13, 2023 06:54
- 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]>
@tsmaeder tsmaeder force-pushed the 11503_terminal_profile_api_2 branch from b6af1d8 to eadd849 Compare January 17, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] Support TerminalProfileProvider
2 participants