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.

Sign up for GitHub

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

Keep current configuration when executing a dynamic configuration #10917

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Keep current configuration when executing a dynamic configuration #10917

merged 1 commit into from
Mar 30, 2022

Conversation

alvsan09
Copy link
Contributor

@alvsan09 alvsan09 commented Mar 23, 2022 •
edited by vince-fugnitto
Loading

What it does

Starting a session from a dynamic debug configuration left it selected as current in the DebugConfigurationManager, however this selection was not reflected in the debug view since dynamic configurations are not found under the launch.json file.

Therefore before this change, starting a debug session by clicking the start button under the debug view may start a previously run dynamic configuration instead of the one reflected in the GUI.

debug_started_wrong_config

This change fixes the above problem by saving the current configuration before the start of a dynamic debug session and
restores it right after the session starts.

How to test

  • Having two .ts program files e.g. A and B
    • Define a corresponding launch entry (under the launch.json) for each of them (the configurations are arbitrary as long as you have two file programs that are ready for debugging).
    • Start B, make sure the debug session starts properly
    • Start A, make sure the debug session starts properly
    • Select the Editor for B
    • Using the command palette call the command `debug ', NOTE: The last space is important to trigger the command
    • Select the entry dynamic configuration entry: 'Run Current File'
    • Make sure a debug session is started for B and then stop the session
    • The debug view shall still indicate A as the currently selected configuration
    • From the debug view click the start button and make sure that A is started (and not B as before the fix)

debug_started_wrong_config_fix

Review checklist

Reminder for reviewers

@alvsan09 alvsan09 requested review from msujew and removed request for msujew March 23, 2022 17:07
@vince-fugnitto vince-fugnitto added the debug issues that related to debug functionality label Mar 23, 2022
@colin-grant-work
Copy link
Contributor

@alvsan09, could you add some detail to what you mean by this?

define a corresponding launch entry for each of them.

What should the launch entry look like?

@alvsan09
Copy link
Contributor Author

@colin-grant-work for the question.

What should the launch entry look like?

The configurations are not really special, as long as you have two file programs ready to be debugged from an entry in your launch.json, but just in case here goes an example:
launch.json

I have updated the instructions to point to this gist file as well !

colin-grant-work
colin-grant-work reviewed Mar 25, 2022
View reviewed changes
packages/debug/src/browser/debug-prefix-configuration.ts Outdated
Comment on lines 158 to 160
protected async runConfiguration(configuration: DebugSessionOptions): Promise<void> {
this.debugConfigurationManager.current = { ...configuration };
this.commandRegistry.executeCommand(DebugCommands.START.id);
await this.commandRegistry.executeCommand(DebugCommands.START.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a more direct strategy might be to modify this code. The DebugCommands.Start command accepts an optional argument DebugSessionOptions to be run, and falls back to debugConfigurationManager.current if none is supplied. Given that, I believe that passing configuration as a second argument to executeCommand should successfully run the configuration without modifying the current field of the debugConfigurationManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch Colin!
Since there are two different behaviours when exectuing 'dynamic' vs 'non dynamic' configurations,
I created a new function for 'dynamic', I could inline this call rather than creating a new function although I think it's clearer this way.
Comments are very welcome !

@alvsan09 alvsan09 force-pushed the asl/restore_debug_config branch from 99ff9d4 to 2a9102c Compare March 28, 2022 13:32
colin-grant-work
colin-grant-work approved these changes Mar 28, 2022
View reviewed changes
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. Running a dynamic configuration no longer affects the selection in the debug widget, but does successfully run the config. 👍

@alvsan09 alvsan09 force-pushed the asl/restore_debug_config branch from 2a9102c to 23e3590 Compare March 28, 2022 14:40
@alvsan09
Copy link
Contributor Author

Re-based to latest master

@alvsan09
Keep current configuration when executing a dynamic config.
433f6eb 
Starting a session from a dynamic debug configuration left it
selected as `current` in the `DebugConfigurationManager`, however
this selection was not reflected in the debug view as the dynamic
configurations are not found under the `launch.json` file.

Therefore before this change, starting a debug session clicking the
`start` button under the debug view may start a previously run
dynamic configuration instead of the one reflected in the GUI.

This change fixes the above by leaving the current configuration,
and specify the configuration to use in an existing optonal argument.
@alvsan09 alvsan09 force-pushed the asl/restore_debug_config branch from 23e3590 to 433f6eb Compare March 28, 2022 23:07
@alvsan09 alvsan09 changed the title Restore configuration after start of dynamic debug configuration Keep current configuration when executing a dynamic configuration Mar 28, 2022
vince-fugnitto
vince-fugnitto approved these changes Mar 30, 2022
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.

The changes look good to me 👍
I confirmed that both the debug-view, and statusbar reflect the last proper debug configuration executed.

@alvsan09 alvsan09 merged commit b3d4103 into master Mar 30, 2022
@alvsan09 alvsan09 deleted the asl/restore_debug_config branch March 30, 2022 18:27
@github-actions github-actions bot added this to the 1.24.0 milestone Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-grant-work colin-grant-work colin-grant-work approved these changes

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

Assignees
No one assigned
Labels
debug issues that related to debug functionality
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
3 participants
@alvsan09 @colin-grant-work @vince-fugnitto