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

Allow debug session to share repl with its parent; fixes #62419 #80673

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@dgozman
Copy link
Contributor

commented Sep 10, 2019

Introduced DebugSessionOptions parameter to debug.startDebugging to
control this behavior.

@weinand, @isidorn Could you please take a look? Changing API following this suggestion.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@dgozman great work, I really like this.

For the API I have created this api proposal #80718. After discussions with @weinand we would prefer if instead of adding a new argument to the startDebugging method, we would instead add the parentSession to the option bag. Due to backwards compatibility we would need to make the last argument parentSessionOrOptions: DebugSession | DebugSessionOptions
I will bring this up in our API sync next monday and then we can have this finalized.

As for the code changes I will comment directly in the code.

Thanks!

src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/workbench/api/node/extHostDebugService.ts Outdated Show resolved Hide resolved
@dgozman dgozman force-pushed the dgozman:fix-62419 branch from 89a4826 to fa52bee Sep 12, 2019
Copy link
Contributor Author

left a comment

@isidorn Please take another look. I changed the API as you suggested.

Also, this PR includes some fixes for FocusSessionActionViewItem around synchronizing selection with focused session. Both instances filter the sessions list, so it's not 1 to 1 mapping with the call stack pane.

src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.d.ts Outdated Show resolved Hide resolved
src/vs/vscode.d.ts Outdated Show resolved Hide resolved
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

After bringing this up in the API sync. We decided to have it in the vscode.proposed.d.ts for one milestone and then after one milestone we would merge it into vscode.d.ts. That way we would give time for other extension authors to provide feedback and we do not rush anything. @dgozman can you please move it to proposed?

Another feedback was that the DebugSessionOptions should have all the arguments, not just the last two. A similar approach is done by the Terminal here
So it keeps the old method which passes the arguments one after the other, and there is a new method which just takes an option bag.
@weinand @dgozman @jrieken let me know what you think about this

Apart from that can you please tackle the conflict issues in this PR @dgozman

@dgozman dgozman force-pushed the dgozman:fix-62419 branch 2 times, most recently from a4ca4b4 to 318c183 Sep 16, 2019
@dgozman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@isidorn I updated the PR and put everything to vscode.proposed.d.ts, giving it some time sounds good.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@dgozman thanks a lot for moving it to vscode.proposed.d.ts and for fixing conflicts.
However using the option bag was just a proposal. After discussing it this morning with @weinand we decided to go with your previous approach, to keep the first arguments out of the option bag since they are more important than the rest and in practice should not be optional. This way we also specify to the user what is important from the arguments more easily. Using an option bag could make the user easily forget to pass the WorkspaceFolder resulting in variables not being substituted in launch.json for example.

I appologize for the misunderstanding. Can you just go back to your previous approach, that we use
startDebugging(folder: WorkspaceFolder | undefined, nameOrConfiguration: string | DebugConfiguration, parentSessionOrOption?: DebugSession | DebugSessionOptions)

Once we have that I can merge this in.
Thanks a lot!

Introduced DebugSessionOptions passed to debug.startDebugging which
encapsulated DebugConsoleMode to control this behavior.
@dgozman dgozman force-pushed the dgozman:fix-62419 branch from 318c183 to e4d4b43 Sep 18, 2019
@dgozman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@isidorn I reverted back to parentSessionOrOptions. It seems that just putting new signature in vscode.proposed.d.ts works, so this should be it.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Looks great, mering in.You missed one spot in extHost.protocol.ts, though I will fix that on master (via a9c67d8).
I have also created this item to not forget to move this out of proposed next milestone #81170

Thanks a lot for this PR 🍻 ☀️

@isidorn isidorn merged commit c4a6fc1 into microsoft:master Sep 19, 2019
2 checks passed
2 checks passed
VS Code #20190918.49 succeeded
Details
license/cla All CLA requirements met.
Details
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

I verified it works nicely end to end.
And just to add that I like that we went with the enum DebugConsoleMode because some extension might have an use case that it wants new sessions debug console merged even though there is no parent / child relationship. So in theory we could add a new option to the DebugConsoleMode MergeWithExistingSession` in case the need arrises.

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.