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

Add "breakpoint created/deleted" notifications. #8642

Closed
vadimcn opened this issue Jul 1, 2016 · 12 comments
Closed

Add "breakpoint created/deleted" notifications. #8642

vadimcn opened this issue Jul 1, 2016 · 12 comments
Assignees
Labels
api bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded

Comments

@vadimcn
Copy link
Contributor

vadimcn commented Jul 1, 2016

Some debuggers have much more flexible ways of setting breakpoints than VSCode currently supports (e.g. set breakpoints by regex). Would be nice if breakpoints created via commands in debugger console were reflected in the UI.

I'd suggest adding a new notification message to the protocol, which adapter can send when a breakpoint is created/deleted. It should include a "cookie" that needs to be passed back to the adapter in order to re-create the breakpoint.

@bpasero bpasero added feature-request Request for new features or functionality api labels Jul 2, 2016
@weinand
Copy link
Contributor

weinand commented Jul 5, 2016

@vadimcn the protocol has already a BreakpointEvent for this. Please see: https://github.com/Microsoft/vscode-debugadapter-node/blob/master/protocol/src/debugProtocol.ts#L135

@isidorn are you already tracking this event?

@isidorn
Copy link
Contributor

isidorn commented Jul 5, 2016

@weinand yes here

@weinand weinand closed this as completed Jul 5, 2016
@vadimcn
Copy link
Contributor Author

vadimcn commented Jul 7, 2016

@weinand: But does VSCode support creation/deleteion? Sending a BreakpointEvent with reason = 'new' doesn't seem to have any effect.
Your comment from February says "VS Code now listens on breakpoint events with reason update and updates the verification state and location". If I read the relevant code, correctly, it indeed only deals with updates of existing breakpoints.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jul 7, 2016

Also, please note that currently the Breakpoint does not provide a way to preserve custom information (such as condition, hit count, etc) across debugging sessions.

@isidorn
Copy link
Contributor

isidorn commented Jul 7, 2016

@vadimcn you are right, there is no code for dealing with Breakpoint event = 'new' or 'deleted' (@weinand do we want to call this 'deleted'?). Thus reopening this

@isidorn isidorn reopened this Jul 7, 2016
@isidorn isidorn assigned isidorn and unassigned weinand Jul 7, 2016
@isidorn isidorn added this to the July 2016 milestone Jul 7, 2016
@isidorn
Copy link
Contributor

isidorn commented Jul 8, 2016

Assigning first to @weinand since this needs a couple of more additions to the protocol:

  • How do we call an event when a breakpoint got deleted?
  • Do we add a possibility that a adapter can also add / remove function breakpoints - if yes we need a way to distinguish those events from adding/removing regular breakpoints
  • As mentioned by @vadimcn currently it is not possible for the debug adapter to send a breakpoint with a condition to vscode, so we might need to introduce the condition to the Breakpoint interface in the debug protocol

Once that is done we just need to handle those events here for which a pull request is welcome @vadimcn.

@isidorn isidorn removed this from the July 2016 milestone Jul 8, 2016
@isidorn isidorn assigned weinand and unassigned isidorn Jul 8, 2016
@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Apr 6, 2017
@daviwil
Copy link
Contributor

daviwil commented May 17, 2017

Hey guys, the PowerShell extension is really in need of this behavior now since we're able to interactively debug PowerShell scripts using our new Integrated Console feature. I've started trying to implement the add/delete behavior for BreakpointEvent in debugService so that I can send you a PR for it to go into the release at the beginning of June. Once I get something basic working I'll send a WIP PR so we can discuss the implementation.

I also noticed the lack of support for the debugger sending condition and function breakpoints to the client. Is that something we could potentially add to the protocol soon? Any way I could help with that?

@isidorn
Copy link
Contributor

isidorn commented Sep 19, 2017

After discussing with @weinand we decided to introduce 3 different breakpoint events
"new", "changed" and "removed". @weinand will update the debug protocol and will probably also cover the case that the adapter can update the condition of a breakpoint.

For compatibiliy reason if a breakpoint event has no reason (which is required) I will handle it as a "changed" event.
@roblourens please update the node2 adapter to always send the appropriate reason for breakpoint events(currently it is sending always "new")

isidorn added a commit that referenced this issue Sep 19, 2017
@isidorn
Copy link
Contributor

isidorn commented Sep 19, 2017

Now vscode will react to breakpoint events.
Please note that a "new" BreakpointEvent requires that a Source is passed. In the future we can consider if adapters can add new function breakpoints with this "new" event.

Unassigning myself and assigning to @weinand to update the protocol and @roblourens to adopt this in node-debug2 adapter.
Even though condition is not specified in the protocl for BreakpointEvent yet vscode should proparly react to this.

@daviwil please try it out and let me know how it goes

@isidorn isidorn assigned roblourens and unassigned isidorn Sep 19, 2017
@weinand weinand added the verification-needed Verification of issue is requested label Sep 26, 2017
@weinand weinand assigned isidorn and unassigned weinand and roblourens Sep 26, 2017
@weinand weinand added the verified Verification succeeded label Sep 26, 2017
@weinand
Copy link
Contributor

weinand commented Sep 26, 2017

For verifying this feature, I've added two REPL command to mock-debug:

  • new 12 adds a new breakpoint in line 12 of the source file,
  • del 12 deletes a breakpoint in line 12 of the source file.

I found these issues:

  • for a newly created breakpoint the "verified" attribute is ignored
  • I could not delete a breakpoint

And maybe unrelated:
Breakpoints section is empty #35179

@weinand weinand added verification-found Issue verification failed bug Issue identified by VS Code Team member as probable bug and removed verified Verification succeeded labels Sep 26, 2017
@isidorn isidorn reopened this Sep 27, 2017
@isidorn
Copy link
Contributor

isidorn commented Sep 27, 2017

@weinand thanks for verifying and for nicely updating the mock debug so this is easy to try out.
I have pushed a fix for both issues specified here, as for the section being empty I will tackle that in that seperate issue.

@weinand weinand added verified Verification succeeded and removed verification-found Issue verification failed labels Sep 28, 2017
@weinand
Copy link
Contributor

weinand commented Sep 28, 2017

Now its working fine.

@isidorn isidorn added the release-notes Release notes issues label Sep 29, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants