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

Jump to bottom

Use Monaco when creating files in DebugConfigurationManager #9467

Merged
merged 1 commit into from May 24, 2021
Merged

Use Monaco when creating files in DebugConfigurationManager #9467

merged 1 commit into from May 24, 2021

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented May 11, 2021 •
edited

What it does

This PR fixes #9310 by using Monaco to manipulate the file contents rather than having the FileService make the change on the backend only.

How to test

  1. Open a workspace with no launch.json files.
  2. Create an empty launch.json file in some root and have it open.
  3. Open the debug widget.
  4. Open the dropdown to select configurations and select Add Configuration.
  5. The file you created should be populated with the default content of a launch.json file.
  6. Save the file.
  7. You should see no error message.
  8. It should work regardless of whether the file you create is in a .vscode or .theia folder.

The behavior may differ depending on whether you've previously had a launch.json in the other folder during the same session.

Review checklist

Reminder for reviewers

Signed-off-by: Colin Grant colin.grant@ericsson.com

@colin-grant-work colin-grant-work force-pushed the bugfix/add-config-with-open-file branch from 97a415c to b815e02 Compare May 11, 2021 17:11
@colin-grant-work colin-grant-work changed the title Use Monaco when creating files in Debug/TaskConfigurationManager Use Monaco when creating files in DebugConfigurationManager May 12, 2021
@vince-fugnitto vince-fugnitto added bug bugs found in the application debug issues that related to debug functionality labels May 12, 2021
vince-fugnitto
vince-fugnitto approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirm that the changes successfully fix #9310 👍

packages/debug/src/browser/debug-configuration-manager.ts Outdated
@@ -252,28 +252,28 @@ export class DebugConfigurationManager {
return this.models.values().next().value;
}

protected async doOpen(model: DebugConfigurationModel): Promise<EditorWidget> {
protected async doOpen(model: DebugConfigurationModel): Promise<EditorWidget | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

Should we document this breakage to be safe?
A downstream extension may previously expect EditorWidget when calling doOpen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, likely best to be safe. I'll push a changelog entry

@colin-grant-work colin-grant-work force-pushed the bugfix/add-config-with-open-file branch from b815e02 to d785eb0 Compare May 13, 2021 18:10
@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, as we discussed offline, I've modified the code so that it handles any case in which a launch.json does not contain at least {configurations: []}. Rather than changing the signatures of the methods, I've thrown an Error in case the PreferenceService returns no URI. As I note in the code, that should never happen here, since the inputs are known. I have added a breaking change notice about swapping out the FileService for MonacoTextModelService, however.

vince-fugnitto
vince-fugnitto approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the changes work well 👍

  • verified that it properly creates the launch.json when it does not exist.
  • verified that it properly populates the launch.json when it does not contain a configurations object.
  • verified that it properly adds new configurations (through add configuration) for an existing configurations object.
  • works for both .vscode and .theia.

@colin-grant-work
Use Monaco when creating files in DebugConfigurationManager
f229c68 
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work force-pushed the bugfix/add-config-with-open-file branch from d785eb0 to f229c68 Compare May 24, 2021 14:51
@colin-grant-work colin-grant-work merged commit 38f176a into eclipse-theia:master May 24, 2021
@vince-fugnitto vince-fugnitto added this to the 1.14.0 milestone May 27, 2021
@colin-grant-work colin-grant-work deleted the bugfix/add-config-with-open-file branch April 14, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

Assignees
No one assigned
Labels
bug bugs found in the application debug issues that related to debug functionality
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.

Fail to add debug configuration when an empty launch.json is opened
2 participants
@colin-grant-work @vince-fugnitto