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

implemented start debugger on restart debugger if debugger isn't started #47219

Merged

Conversation

@ivorhuang
Copy link
Contributor

commented Apr 5, 2018

My friend (@ylingene) and I worked on #24425. We're unsure if the condition in RestartAction::isEnabled is satisfactory for preventing StartAction::isEnabled from being called when the debugger has been started already.

@msftclas

This comment has been minimized.

Copy link

commented Apr 5, 2018

CLA assistant check
All CLA requirements met.

@weinand weinand added this to the April 2018 milestone Apr 5, 2018

@@ -215,10 +215,21 @@ export class RestartAction extends AbstractDebugAction {
static LABEL = nls.localize('restartDebug', "Restart");
static RECONNECT_LABEL = nls.localize('reconnectDebug', "Reconnect");

constructor(id: string, label: string, @IDebugService debugService: IDebugService, @IKeybindingService keybindingService: IKeybindingService) {
private startAction: StartAction;

This comment has been minimized.

Copy link
@isidorn

isidorn Apr 13, 2018

Contributor

You should not create an instance of the startAction, but simply call into debugService to start the session, instead of callin action.run()

@@ -240,6 +251,9 @@ export class RestartAction extends AbstractDebugAction {
}

protected isEnabled(state: State): boolean {
if (!this.debugService.getViewModel().focusedProcess) {

This comment has been minimized.

Copy link
@isidorn

isidorn Apr 13, 2018

Contributor

Instead of looking at the focusedProcess look at the state as we do below. Feels cleaner to me

This comment has been minimized.

Copy link
@ivorhuang

ivorhuang Apr 13, 2018

Author Contributor

super.isEnabled(state) returns true - I don't believe we want to return StartAction.isEnabled when legitimately restarting the debugger and instead only return StartAction.isEnabled when the debugger is not running. Thoughts?

This comment has been minimized.

Copy link
@ivorhuang

ivorhuang Apr 14, 2018

Author Contributor

After some more thought, it should be okay for RestartAction.isEnabled to always return true if StartAction.isEnabled is true because RestartAction.run() will always be a valid call in that case. As long as the proper run() function is called then it shouldn't matter if RestartAction.isEnabled returns true.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

Thanks for your PR. I have added some comments directly in the code.
Also did you test this out if it works as expected?

@ivorhuang

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2018

Test plan:

  1. Click restart debugging from menu without debugger started (keyboard shortcut works too)
    debug_1

  2. Debugger has started
    debug_2

  3. Ensure debugger restarts when selecting "Restart Debugger" rather than starting a new debugger (we also checked that the program does not reach line 245 on restart)
    3

ivorhuang added some commits Apr 14, 2018

@ivorhuang ivorhuang closed this Apr 15, 2018

@ivorhuang ivorhuang reopened this Apr 15, 2018

constructor(id: string, label: string,
@IDebugService debugService: IDebugService,
@IKeybindingService keybindingService: IKeybindingService,
@IWorkspaceContextService private contextService?: IWorkspaceContextService,

This comment has been minimized.

Copy link
@isidorn

isidorn Apr 16, 2018

Contributor

Why are these arguments optional?

This comment has been minimized.

Copy link
@ivorhuang

ivorhuang Apr 16, 2018

Author Contributor

The bottom two arguments are there because we need those two services for starting a debugger. They're passed by what seems to be reflection in src\vs\platform\instantiation\common\instantiationService.ts:120 (this is how StartAction is called also). This the black magic that the other contributor mentioned in #39060.

However, existing calls to RestartAction, such as at src\vs\workbench\parts\debug\browser\debugActionsWidget.ts:253 and src\vs\workbench\parts\debug\electron-browser\callStackView.ts:259 do not pass an IWorkspaceContextService or an IHistoryService, which is why we made them optional. Let us know your thoughts on this.

const configurationManager = this.debugService.getConfigurationManager();
let launch = configurationManager.selectedConfiguration.launch;
if (!launch) {
const rootUri = this.historyService.getLastActiveWorkspaceRoot();

This comment has been minimized.

Copy link
@isidorn

isidorn Apr 16, 2018

Contributor

All of this code is now duplicated. This is not good.
Either create a helper function which will be used inside both RestarAction and StartAction, or RestartAction should create a StartAction as you previously have done it and I wrongly corrected you

This comment has been minimized.

Copy link
@isidorn

isidorn Apr 18, 2018

Contributor

Can you please adress this?

This comment has been minimized.

Copy link
@ivorhuang

ivorhuang Apr 18, 2018

Author Contributor

Will do - I've been busy the past couple of days.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

Thanks for your changes. I have commented in the code directly.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

@ivorhuang this looks good, thanks for your PR. Merging it in 🍻

@isidorn isidorn merged commit 4b74bb0 into microsoft:master Apr 19, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@ivorhuang ivorhuang deleted the ivorhuang:restart-starts-debugger-if-not-started branch Jul 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.