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

Support debugging process from the process explorer #63953

Merged
merged 3 commits into from
Nov 30, 2018

Conversation

RMacfarlane
Copy link
Contributor

Adds a 'Debug' context menu item for processes that can be attached to in the process explorer. #63147

The issue service that communicates with the process explorer window lives in the main process, and the debug service is only in the renderer process. To start debugging, the issue service sends a vscode:runAction message to the renderer with the workbench debug start action id. To pass the debug config, I tweaked the vscode:runAction listener to pass through arguments to executeCommand. I also had to make a change to the workbench command handler to pass the argument, let me know if this looks reasonable.

@RMacfarlane RMacfarlane self-assigned this Nov 28, 2018
@bpasero bpasero self-assigned this Nov 29, 2018
@bpasero bpasero added this to the November 2018 milestone Nov 29, 2018
@bpasero bpasero removed their assignment Nov 29, 2018
@@ -133,7 +133,11 @@ export class StartAction extends AbstractDebugAction {
this.toDispose.push(this.contextService.onDidChangeWorkbenchState(() => this.updateEnablement()));
}

public run(): Thenable<any> {
public run(config?: string | IConfig): Thenable<any> {
Copy link
Member

@bpasero bpasero Nov 29, 2018

Choose a reason for hiding this comment

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

@isidorn fyi on this change in the PR impacting debug action

Copy link
Contributor

Choose a reason for hiding this comment

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

@RMacfarlane not the nicest. But fine for me. Why do you have string | IConfig, when you always pass an IConfig as far as I understand?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please add a comment for the use case here. So I do not accidently delete this code later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it to be only IConfig and added a note

@@ -110,7 +110,11 @@ Registry.add(Extensions.WorkbenchActions, new class implements IWorkbenchActionR

const from = args && args.from || 'keybinding';

return Promise.resolve(actionInstance.run(undefined, { from })).then(() => {
if (args) {
Copy link
Member

@bpasero bpasero Nov 29, 2018

Choose a reason for hiding this comment

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

@RMacfarlane the change looks good to me except for this piece that scares me. Are we sure we are not sending arbitrary data to all actions that get executed that might expect something else here? There is only one action where we need this for (Start Debug) but for any action that is triggered we now pass the args on. This has the potential of breaking badly if an action is not ready for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was the code I wasn't sure about. Since only Start Debug needs the args passed on, I reverted this change and added separate code for registering that command

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@RMacfarlane yeah better, you seem to have a weird strictNull check change in this PR though.

@RMacfarlane RMacfarlane merged commit 86052e3 into master Nov 30, 2018
@RMacfarlane RMacfarlane deleted the rmacfarlane/debug-process branch May 9, 2019 18:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants