support multi-root for 'Add Configuration' #8066

elaihau · 2020-06-20T19:58:47Z

If a user is using a multi-root workspace, the command "Debug: Add Configuration" adds a configuration directly in the first workspace root instead of prompting users to select the correct root. With this change the command prompts users with a quick-pick which allows them to select their desired workspace root.
resolves [debug] support multi-root for 'Add Configuration' #5166

Signed-off-by: Liang Huang [email protected]

How to test

in a workspace that has only one root folder, the command "Debug: Add Configuration" should work the way it used ot
in a multi root workspace, we should see a prompt that asks users to select which root folder the configuration should be added to.

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

tsmaeder · 2020-06-22T11:52:20Z

packages/debug/src/browser/debug-configuration-manager.ts

+        let model: DebugConfigurationModel | undefined;
+        if (this.models.size > 1) {
+            model = await this.quickPick.show(Array.from(this.models.entries()).map(entry => {
+                const [rootFolder, modelValue] = entry;


It would be nice if the list of roots would be ordered like what the user sees in the explorer: I guess if we iterate over the root folders array and 'get' from the map that would be the case. Iteration order over a map is not defined.

Thank you for the feedback !
With my most recent update, I am using workspaceService.roots to ensure the order of the workspace folders.

tsmaeder · 2020-06-22T11:54:47Z

packages/debug/src/browser/debug-configuration-manager.ts

+                    value: modelValue
+                };
+            }), {
+                placeholder: 'Select for which root folder to add the configuration to'


I think "workspace folder" would be the VS Code terminology here: I suggest:

Select the workspace folder to add the configuration to

vince-fugnitto

@elaihau I had issues with adding configurations when I had multiple roots and my roots were simple folders without .theia or .vscode present. In these cases, when selecting a root with the quick-pick, the launch.json was not created in their respective root and instead the workspace file was opened instead, is this the expected behavior?

{
   "folders": [
      {
         "path": "file:///Users/vincentfugnitto/Desktop/a"
      },
      {
         "path": "file:///Users/vincentfugnitto/workspace/theia"
      }
   ],
"settings": {
    "launch": {}
}
}

vince-fugnitto · 2020-06-22T23:33:07Z

packages/debug/src/browser/debug-configuration-manager.ts

+            model = await this.quickPick.show(rootUris.map(rootUri => {
+                const modelValue = this.models.get(rootUri);
+                return {
+                    label: this.labelProvider.getName(new URI(rootUri)),


I think we can align with the 'new terminal' and also display the path to the workspace (helps when there are multiple roots with the same name):

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

Lines 568 to 571 in c7182b5

return this.quickPick.show(roots.map(

({ uri }) => ({ label: this.labelProvider.getName(new URI(uri)), description: this.labelProvider.getLongName(new URI(uri)), value: uri })

), { placeholder: 'Select current working directory for new terminal' });

}

Your feedback should have been resolved in the lastest patch.
I tested the multi root workspace

that has more than one folders

that has only one folder, and

with & without the .theia or .vscode folders

Thank you !

tsmaeder · 2020-06-23T08:12:24Z

I found a problem with the following scenario:

Create a multiroot-workspace looking like this:

Third
  .vscode
    launch.json
Second
  untitled.txt

Where launch.json contains:

{
  // Use IntelliSense to learn about possible attributes.
  // Hover to view descriptions of existing attributes.
  "version": "0.2.0",
  "configurations": []
}

Run "Debug: Add Configuration"
Observe: the content of Third/.vscode/launch.json is replaced with:

{}

tsmaeder · 2020-06-23T08:14:59Z

packages/debug/src/browser/debug-configuration-manager.ts

-        const { model } = this;
+        let model: DebugConfigurationModel | undefined;
+        if (this.models.size > 1) {
+            const rootUris = (await this.workspaceService.roots).map(r => r.uri);


I don't this first map is needed: why not just use r.uri inside the quick pick?

tsmaeder · 2020-06-23T08:19:03Z

packages/debug/src/browser/debug-configuration-manager.ts

@@ -254,21 +274,28 @@ export class DebugConfigurationManager {

    protected async doOpen(model: DebugConfigurationModel): Promise<EditorWidget> {
        let uri = model.uri;
-        if (!uri) {
+        const configFileUri = this.getFolderConfigurationUri(model);


I think this sequence is ignoring the case where there is a launch.json but it's in the .vscode folder instead of .theia

tsmaeder · 2020-06-23T08:45:25Z

packages/debug/src/browser/debug-configuration-manager.ts

@@ -254,21 +274,28 @@ export class DebugConfigurationManager {

    protected async doOpen(model: DebugConfigurationModel): Promise<EditorWidget> {
        let uri = model.uri;
-        if (!uri) {
+        const configFileUri = this.getFolderConfigurationUri(model);
+        const fileStat = await this.filesystem.getFileStat(configFileUri.toString());


This line throws an error when the the parent folder of the file does not exist. Maybe exists does the trick? But the documentation is really not very precise on this.

- If a user is using a multi-root workspace, the command "Debug: Add Configuration" adds a configuration directly in the first workspace root instead of prompting users to select the correct root. With this change the command prompts users with a quick-pick which allows them to select their desired workspace root. - resolves #5166 Signed-off-by: Liang Huang <[email protected]>

elaihau · 2020-06-25T02:38:22Z

Thank you for your time @tsmaeder !
You are right. I totally forgot the scenarios where .vscode/launch.json is used by Theia.
The bugs you found should have been fixed in my lastest patch. Could you please take a look when you get the chance? Thanks !

tsmaeder

I'm very good at finding subtle problems, I guess 🤷 Otherwise, looks good.

tsmaeder · 2020-06-26T09:21:21Z

packages/debug/src/browser/debug-configuration-manager.ts

+            console.log(thismodel);
+        }
+        let model: DebugConfigurationModel | undefined;
+        if (this.models.size > 1) {


I think this check is not reliable: I was able to construct a case where I'm not asked which folder I would like to configure:

Open an unrelated folder

Select "Open Workspace" and open a folder looking like this:

Third .vscode launch.json

Add a second folder to the workspace looking like this (don't save to a file)

Second untitled.txt

Hit F1->Debug: Add Configuration

Observe: I'm not asked which workspace folder to use: the config goes to Third

I've been able to trace the problem to the fact that the listener for preference changes is never called when a new workspace root is added: theia/packages/debug/src/browser/debug-configuration-manager.ts#88

I am not really sure what the right fix is, though: should we listen to workspace folder changes or should the preferences system send a change?

@akosyakov I'd like your opinion on that one.

It sounds fine to listen to workspace root changes, since they are input to updateModels function and seems to be easy fix.

But that we don't fire preference events when roots are changed bother me too. We should check what VS Code extensions will expect. Could you file an issue to investigate?

akosyakov · 2020-06-29T08:35:08Z

packages/debug/src/browser/debug-configuration-manager.ts

+        const paths = this.preferenceConfigurations.getPaths();
+        for (const configPath of paths) {
+            const configUri = new URI(model.workspaceFolderUri).resolve(`${configPath}/launch.json`);
+            if (await this.filesystem.exists(configUri.toString())) {


why do we need it? if something exist already we should not override it at all, previous fallback was fine to me

doCreate method should assume that it is called when nothing exists

The previous version of the PR did not work in the cases where a launch.json existed, but was under .vscode instead of .theia

that sounds like a bug in the preference service, DebugConfigurationModel.uri used to point to a config from which data was loaded or nothing

akosyakov · 2020-06-29T08:36:42Z

packages/debug/src/browser/debug-configuration-manager.ts

@@ -254,21 +277,27 @@ export class DebugConfigurationManager {

    protected async doOpen(model: DebugConfigurationModel): Promise<EditorWidget> {
        let uri = model.uri;
-        if (!uri) {
+        const configFileUri = await this.getFolderConfigurationUri(model);
+        if (!await this.filesystem.exists(configFileUri.toString())) {


why previous check was not good? if uri is missing, that already means that model does not exist.

akosyakov · 2020-06-29T08:37:10Z

packages/debug/src/browser/debug-configuration-manager.ts

            uri = await this.doCreate(model);
        }
-        return this.editorManager.open(uri, {
+        return this.editorManager.open(uri!, {


! why is it needed?, if branch should ensure that uri exists

i agree with you while the linter doesn't.

without ! it gives me this error

Argument of type 'URI | undefined' is not assignable to parameter of type 'URI'. Type 'undefined' is not assignable to type 'URI'.ts(2345)

and

'Exit with failure status (1): concurrently -n compile,lint -c blue,green "theiaext compile" "theiaext lint"\nerror Command failed with exit code 1.\n',

We should sort out other comments, I don't think that so many changes to doCreate function are required. It seems there are regressions with PreferenceService.

marcdumais-work · 2020-07-06T14:48:32Z

Liang's contract is over and he has moved-on from Theia.

@tsmaeder I have not followed closely this PR - how close DYT it is to be merged? Is there someone on your side who could take-over and address remaining comments?

tsmaeder · 2020-07-06T14:54:03Z

@marcdumais there is a corner case when adding a workspace folder that needs to be addressed, should be pretty straight-forward, though.

akosyakov requested review from RomanNikitenko and vince-fugnitto June 22, 2020 06:25

akosyakov added the tasks issues related to the task system label Jun 22, 2020

tsmaeder reviewed Jun 22, 2020

View reviewed changes

elaihau force-pushed the Liang/addConfig branch 2 times, most recently from bb20598 to 7aacc6f Compare June 22, 2020 22:42

vince-fugnitto reviewed Jun 22, 2020

View reviewed changes

vince-fugnitto added debug issues that related to debug functionality multi-root issues related to multi-root support labels Jun 22, 2020

elaihau force-pushed the Liang/addConfig branch from 7aacc6f to ea4120e Compare June 23, 2020 01:35

tsmaeder requested changes Jun 23, 2020

View reviewed changes

elaihau force-pushed the Liang/addConfig branch from ea4120e to edd5f17 Compare June 25, 2020 02:36

tsmaeder requested changes Jun 26, 2020

View reviewed changes

akosyakov reviewed Jun 29, 2020

View reviewed changes

JonasHelming assigned vince-fugnitto Feb 25, 2022

JonasHelming added the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label Feb 25, 2022

vince-fugnitto mentioned this pull request Mar 11, 2022

debug: add multi-root support for add configurations #10875

Merged

1 task

vince-fugnitto closed this in #10875 Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support multi-root for 'Add Configuration' #8066

support multi-root for 'Add Configuration' #8066

elaihau commented Jun 20, 2020

tsmaeder Jun 22, 2020

elaihau Jun 22, 2020

tsmaeder Jun 22, 2020

elaihau Jun 22, 2020

vince-fugnitto left a comment

vince-fugnitto Jun 22, 2020

elaihau Jun 23, 2020 •

edited

Loading

tsmaeder commented Jun 23, 2020

tsmaeder Jun 23, 2020

tsmaeder Jun 23, 2020

tsmaeder Jun 23, 2020

elaihau commented Jun 25, 2020

tsmaeder left a comment

tsmaeder Jun 26, 2020

tsmaeder Jun 26, 2020

akosyakov Jun 29, 2020

akosyakov Jun 29, 2020 •

edited

Loading

tsmaeder Jun 29, 2020

akosyakov Jun 29, 2020

akosyakov Jun 29, 2020

akosyakov Jun 29, 2020

elaihau Jun 29, 2020

akosyakov Jun 30, 2020

marcdumais-work commented Jul 6, 2020

tsmaeder commented Jul 6, 2020

	return this.quickPick.show(roots.map(
	({ uri }) => ({ label: this.labelProvider.getName(new URI(uri)), description: this.labelProvider.getLongName(new URI(uri)), value: uri })
	), { placeholder: 'Select current working directory for new terminal' });
	}

support multi-root for 'Add Configuration' #8066

support multi-root for 'Add Configuration' #8066

Conversation

elaihau commented Jun 20, 2020

How to test

Review checklist

Reminder for reviewers

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

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elaihau Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

tsmaeder commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elaihau commented Jun 25, 2020

tsmaeder 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

akosyakov Jun 29, 2020 • 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

marcdumais-work commented Jul 6, 2020

tsmaeder commented Jul 6, 2020

elaihau Jun 23, 2020 •

edited

Loading

akosyakov Jun 29, 2020 •

edited

Loading