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

Introduce DebugSession#workspaceFolder #11090

Merged
merged 3 commits into from
May 12, 2022

Conversation

jfaltermeier
Copy link
Contributor

@jfaltermeier jfaltermeier commented Apr 29, 2022

What it does

Fixes #10023

  • Adds the missing workspaceFolderUri to the DebugSession API
  • Adjust code to create the session including workspaceFolderUri
  • The existing DebugSessionManager was already honoring the workspace folder as far as I could see (-> e.g. when executing pre launch tasks:
    const taskRun = await this.runTask(options.workspaceFolderUri, resolved.configuration.preLaunchTask, true);
    ) so only the session object creation itself was adjusted
  • Adds the workspacefolder to the DebugSession's UI label, in case we have a multi-root workspace (and multiple running debug sessions, as otherwise the ui node is hidden)

VS-Code UI (esm-i was the set workspace-folder. This is not shown in the Top, because this is a launch without a set ws folder):
ksnip_20220426-092056

Theia UI (workspace folder was called java):
ksnip_20220503-101753

Contributed on behalf of STMicroelectronics

Signed-off-by: Johannes Faltermeier jfaltermeier@eclipsesource.com

How to test

I've created a small VSCode extension that tracks the creation of debug sessions and logs the workspace folder of a session:
https://github.com/jfaltermeier/vscode-debug-playground
https://github.com/jfaltermeier/vscode-debug-playground/blob/main/src/extension.ts
You may get this test extension from https://github.com/jfaltermeier/vscode-debug-playground/releases/tag/0.0.1

  • start Theia Browser Example with above extension
  • debug any application (I was testing with a java application)

This should create a log message similar to this one:

root INFO [hosted-plugin: 67589] ES : DebugAdapterTracker: DebugSession created with workspaceFolder: file:///home/johannes/git/java-sample

Without the changes, the worspaceFolder was undefined all the time

root INFO [hosted-plugin: 70803] ES : DebugAdapterTracker: DebugSession created with workspaceFolder: undef
  • For testing the UI label, create a multi root workspace and start multiple debug sessions that stop at a breakpoint.
  • The session label will show the wsfolder in brackets

Review checklist

Reminder for reviewers

@jfaltermeier jfaltermeier force-pushed the 10023-debug-wsFolder branch 3 times, most recently from 61a3a79 to d476ea3 Compare May 3, 2022 08:14
@jfaltermeier jfaltermeier marked this pull request as ready for review May 3, 2022 08:51
@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility debug issues that related to debug functionality labels May 3, 2022
@colin-grant-work colin-grant-work self-requested a review May 5, 2022 14:32
packages/debug/src/browser/debug-session.tsx Outdated Show resolved Hide resolved
@@ -214,7 +214,7 @@ export class DebugSessionManager {
}
}

const sessionId = await this.debug.createDebugSession(resolved.configuration);
const sessionId = await this.debug.createDebugSession(resolved.configuration, options.workspaceFolderUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not appear that the implementation of the createDebugSession method on the DebugServiceImpl has been updated to handle the workspaceFolderUri. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. No, this was an oversight, since this value is not needed there.
In the plain Theia API the ws folder is taken from the DebugSessionOptions and we only need it for vs code compatibility.
I've added the parameter as _workspaceFolderUri?

@jfaltermeier jfaltermeier force-pushed the 10023-debug-wsFolder branch 2 times, most recently from c50970f to 08baedf Compare May 9, 2022 11:12
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

My fault, but in my suggested snippet, I omitted a space between this.configuration.name and (${suffixes.join(' - ')}). Apart from that minor UI issue, this looks good to me, so if you fix that, I'll approve.

Comment on lines 752 to 763
const showWSFolderInLabel = this.workspaceService.isMultiRootWorkspaceOpened && this.options.workspaceFolderUri;
if (showWSFolderInLabel) {
const wsFolder = this.labelProvider.getName(new URI(this.options.workspaceFolderUri));
if (InternalDebugSessionOptions.is(this.options) && this.options.id) {
return this.configuration.name + ' (' + (this.options.id + 1) + ' - ' + wsFolder + ')';
}
return this.configuration.name + ' (' + wsFolder + ')';
} else {
if (InternalDebugSessionOptions.is(this.options) && this.options.id) {
return this.configuration.name + ' (' + (this.options.id + 1) + ')';
}
return this.configuration.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const showWSFolderInLabel = this.workspaceService.isMultiRootWorkspaceOpened && this.options.workspaceFolderUri;
if (showWSFolderInLabel) {
const wsFolder = this.labelProvider.getName(new URI(this.options.workspaceFolderUri));
if (InternalDebugSessionOptions.is(this.options) && this.options.id) {
return this.configuration.name + ' (' + (this.options.id + 1) + ' - ' + wsFolder + ')';
}
return this.configuration.name + ' (' + wsFolder + ')';
} else {
if (InternalDebugSessionOptions.is(this.options) && this.options.id) {
return this.configuration.name + ' (' + (this.options.id + 1) + ')';
}
return this.configuration.name;
const suffixes = [];
if (InternalDebugSessionOptions.is(this.options) && this.options.id !== undefined) {
suffixes.push(String(this.options.id + 1));
}
if (this.workspaceService.isMultiRootWorkspaceOpened && this.options.workspaceFolderUri) {
suffixes.push(this.labelProvider.getName(new URI(this.options.workspaceFolderUri)));
}
return suffixes.length === 0 ? this.configuration.name : this.configuration.name + `(${suffixes.join(' - ')})`;

The WorkspaceService fields about multi-root workspaces are a bit counterintuitive. The code above will show the workspace root if the workspace is defined in a file, even if there is only a single root. I think that's fine behavior, but if we want to display the root only when there are actually multiple roots in the workspace, then this.workspaceService.tryGetRoots().length > 1 is the most informative check.

* add to model and adjust session creation

Contributed on behalf of STMicroelectronics

Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>
* Adapt DebugSessions Tree Model

Contributed on behalf of STMicroelectronics

Signed-off-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>
also:
* add missing parameter to DebugServiceImpl
* remove unenecessary addition from Theia DebugConfiguration

Co-authored-by: colin-grant-work <colin.grant@est.tech>

Contributed on behalf of STMicroelectronics
@jfaltermeier
Copy link
Contributor Author

I've added the space between configuration name and the joined suffixes and rebased on current master.
I haven't changed the logic for checking whether we have a multiroot-workspace. I also think that it is fine to display the suffix in a multi-root environment with just one root.

Copy link
Contributor

@colin-grant-work colin-grant-work 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. 👍

@jfaltermeier jfaltermeier merged commit f249b3e into eclipse-theia:master May 12, 2022
@vince-fugnitto vince-fugnitto added this to the 1.26.0 milestone May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce DebugSession#workspaceFolder
3 participants