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

Does setBreakpoints trigger multiple breakpoint events? #434

Open
benmcmorran opened this issue Sep 19, 2023 · 6 comments
Open

Does setBreakpoints trigger multiple breakpoint events? #434

benmcmorran opened this issue Sep 19, 2023 · 6 comments
Assignees
Labels
clarification Protocol clarification
Milestone

Comments

@benmcmorran
Copy link
Member

The breakpoint event says that it "indicates that some information about a breakpoint has changed." Does this only include changes that were internally triggered by the debug adapter, or also changes were triggered externally, like by a setBreakpoints request? For example, if the IDE sends a setBreakpoints request with 5 elements in arguments.breakpoints, should the IDE expect to immediately receive 5 separate breakpoint events from the debug adapter in addition to the setBreakpoints response?

@tromey
Copy link

tromey commented Sep 22, 2023

FWIW, the DAP implementation in gdb emitted events in this situation, and we ended up removing them because some clients were not expecting them and would display breakpoints twice.

@pointhex
Copy link

I'm relying on them actually. But if it is not supposed to work like that I will change my logic.

@connor4312
Copy link
Member

connor4312 commented Aug 7, 2024

No. I suggest the following change to clarify this:

interface BreakpointEvent extends Event {
  event: 'breakpoint';

  body: {
    /**
     * The reason for the event.
     * - `changed` indiciates that a breakpoint previously sent to the client
     *   has been updated.
     * - `new` and `removed` indicate that a breakpoint has been added or
     *   removed by the adapter or client action outside of a `setBreakpoints`
     *   request. The client should update its breakpoint list accordingly.
     * 
     * Values: 'changed', 'new', 'removed', etc.
     */
    reason: 'changed' | 'new' | 'removed' | string;

    /**
     * The `id` attribute is used to find the target breakpoint, the other
     * attributes are used as the new values.
     */
    breakpoint: Breakpoint;
  };
}

@connor4312 connor4312 added this to the August 2024 milestone Aug 7, 2024
@connor4312
Copy link
Member

fyi @roblourens

@roblourens
Copy link
Member

roblourens commented Aug 9, 2024

Any reason to break out 'changed' in here?

     * - `changed` indiciates that a breakpoint previously sent to the client
     *   has been updated.
     * - `new` and `removed` indicate that a breakpoint has been added or
     *   removed by the adapter or client action outside of a `setBreakpoints`
     *   request. The client should update its breakpoint list accordingly.

The qualifier by the adapter or client action outside of a setBreakpoints request. The client should update its breakpoint list accordingly. could just apply to 'changed' too right?

@connor4312
Copy link
Member

True, my thinking was it'd be useful for clients to explicitly call out that it's creating/destroying client-side breakpoints, inverting the normal flow of things, since that isn't a common flow implementors would necessarily think about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Protocol clarification
Projects
None yet
Development

No branches or pull requests

5 participants