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

Handle the case where no default terminal profile is set no startup #12191

Merged
merged 2 commits into from
Feb 21, 2023

Conversation

tsmaeder
Copy link
Contributor

What it does

We are handling the contributes.terminal extension point later than we are initializing terminal profiles from the preferences. So the default profile in the preferences might not be present yet in the terminal profile registry when we read the preferences. So we try to set the default whenever a new profile is added. In this endeavor, we were not handling the case that no default profile is set in the preferences.

Fixes #12119

How to test

  1. Make sure you have not default profile setting in the Theia preferences
  2. Make sure you have the default extensions downloaded
  3. Start the electron version of Theia
  4. Observe: you do not get the exception from the issue
  5. Now set a default profile to "Javascript Debug Terminal"
  6. Restart Theia
  7. Observe: the default terminal is correctly set.

Review checklist

Reminder for reviewers

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
@msujew msujew self-requested a review February 17, 2023 13:52
@msujew msujew added the terminal issues related to the terminal label Feb 17, 2023
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me. I can't reproduce the issue anymore 👍

this.profileService.onAdded(id => {
// extension contributions get read after this point: need to set the default profile if necessary
let defaultProfileId;
let defaultProfileId = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Minor: This gets implicitly typed as any

Suggested change
let defaultProfileId = undefined;
let defaultProfileId: string | undefined;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange: it seems trivial to infer the type here.

Copy link
Member

@msujew msujew Feb 17, 2023

Choose a reason for hiding this comment

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

TypeScript doesn't seem to do it anyway ¯\_(ツ)_/¯

Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder tsmaeder merged commit 798366f into eclipse-theia:master Feb 21, 2023
@vince-fugnitto vince-fugnitto added this to the 1.35.0 milestone Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terminal: error thrown when attempting to set the default profile to ''
3 participants