Add support for breakpoint dependencies #166202

gayanper · 2022-11-12T19:43:53Z

This change introduce a new breakpoint configuration to configure another breakpoint as a dependency

The breakpoint use the same icon as conditional breakpoint.

Until the dependent breakpoint is hit, the configured breakpoint is skipped from the client side. More information #152428

connor4312 · 2023-12-09T01:15:31Z

Thanks for the PR, and sorry it took so long to get to. This is something I personally would find useful and would like to get in :) however, I think we may want something a little different than this PR:

in this PR, all breakpoints are sent to the DA and the client just continue()s if a breakpoint is hit when their precondition trigger hasn't been met. This would be pretty slow if the 'triggeree' is inside a hot loop. Instead, it'd be preferable to only set the triggeree once paused on the breakpoint that triggers it.
it doesn't seem like there's UI updates to indicate pending/triggered state, so we'd want to do some work to add that

This month is housekeep for us, but this feature is on my radar for the next few iterations. I'll leave the PR open for now -- if you're still interested in doing work on this, we would welcome the contribution. Otherwise, I may or may not end up building on it to implement the feature, depending on where things stand.

Thanks again for your contribution!

gayanper · 2023-12-09T07:48:30Z

Thanks for the PR, and sorry it took so long to get to. This is something I personally would find useful and would like to get in :) however, I think we may want something a little different than this PR:

in this PR, all breakpoints are sent to the DA and the client just continue()s if a breakpoint is hit when their precondition trigger hasn't been met. This would be pretty slow if the 'triggeree' is inside a hot loop. Instead, it'd be preferable to only set the triggeree once paused on the breakpoint that triggers it.

it doesn't seem like there's UI updates to indicate pending/triggered state, so we'd want to do some work to add that

This month is housekeep for us, but this feature is on my radar for the next few iterations. I'll leave the PR open for now -- if you're still interested in doing work on this, we would welcome the contribution. Otherwise, I may or may not end up building on it to implement the feature, depending on where things stand.

Thanks again for your contribution!

Thanks for the feedback @connor4312 . I will start working on the first point which I do agree with you. For the second suggestion on the UI part, Are you thinking of a new icon ?

connor4312 · 2023-12-09T19:08:08Z

Yea, I think they should show in an "unverified" state until triggered, maybe reuse the issue-draft codicon?

gayanper · 2023-12-17T08:07:39Z

@connor4312 I did the changes you suggested, please check and let me know what you think. Also I still in doubt of the namings I used for this new feature. Using waitFor is not something I'm happy about, if you do have a better suggestion I would like to change to it.

gayanper · 2023-12-17T08:10:27Z

Yea, I think they should show in an "unverified" state until triggered, maybe reuse the issue-draft codicon?

Yes the issue-draft looks good as well. Let's change to it when we finalize the logic behind the behavior. So now I have made dependent breakpoints unverified until they become active.

connor4312 · 2023-12-18T15:18:28Z

Some initial notes using the UI 🙂

We'd want a context menu option here to set a trigger point. Perhaps disabled unless there's >1 breakpoint
If the breakpoint path is in a workspace folder, we can probably show it more nicely here by omitting the folder name.
We probably want some icon here to show that the breakpoint is waiting to be set rather than being a normal univerified breakpoint
It seems like disabled dependent breakpoints are still hit, we should check if a BP is enabled before setting it
We probably want something to happen, perhaps a confirmation box similar to when conditional breakpoints are removed, if a breakpoint with dependent breakpoints is removed.
We should probably have a "close" or "ok" button in the breakpoint widget. Right now the only way to close the widget if there aren't other breakpoints is by focusing the editor and pressing escape which is a little annoying.

connor4312

Some comments, excited to see this happening :)

src/vs/workbench/contrib/debug/browser/breakpointWidget.ts

src/vs/workbench/contrib/debug/browser/debugSession.ts

Co-authored-by: Connor Peet <connor@peet.io>

…nt breakpoints

connor4312 · 2023-12-20T19:17:42Z

I pushed a few changes. I think the last pending things for me are those in the Breakpoints view to reflect state better...

- change feature name to triggeredBy - add close button - add actions - add confirmation on toggle

gayanper · 2023-12-20T20:12:19Z

I pushed a few changes. I think the last pending things for me are those in the Breakpoints view to reflect state better...

Ops, I did few changes during last few days and just pushed them, I think I might have overwritten your changes, sorry, I should have checked the MR before force pushing into my branch.

But please feel free to push your changes over mine, I can then have a look at the remaining. Only thing that was pending in my changes was the path and button label and icon to be decided.

connor4312 · 2023-12-20T20:19:36Z

No worries, I still have them locally, will push

connor4312 · 2023-12-20T20:37:00Z

Re-applied, and I fixed up some of the styling on the widget too

gayanper · 2023-12-21T08:34:51Z

Re-applied, and I fixed up some of the styling on the widget too

Thanks, and I started to add icons and I will use a mix of default breakpoint icons and issue-draft to see how it comes out. Then we can decide if we want new icons or if we can change the icons in the code to something else.

gayanper · 2023-12-21T11:55:58Z

@connor4312 I tried to see if we can improve the icons than how it look like now. But I cannot see a better way than using

conditional breakpoint icon while not debugging
show unverified icon until the trigger point is hit
show the conidinal breakpoint icon when the breakpoint is sent to DA and verified

If we use these icons, we will be able to expand this feature later into other type of breakpoints easily.

I also tried to treat this pending state as a separate breakpoint state like the unsupported breakpoint state by using issue-draft icon, But I didn't find how to get the issue-draft icon in the same size as other debug icons in stylesheets.

I would like to get some hints over here.

…ching

- Referencing the breakpoint location got tricky when breakpoints resolve to different locations than they were initially set at, and also as breakpoints change when files change. Instead, use the breakpoint UUID. - Newly-enabled breakpoints were sent for a URI when hit, but in DAP sending breakpoints for a URI replaces all the breakpoints in it, so this was problematic. Instead track on the breakpoint whether it's been triggered for a session.

connor4312 · 2024-01-06T00:33:20Z

The icons look good! I pushed a few more changes from my testing with it. There were some more involved changes I made in my last commit after struggling with some edge cases the way we were doing things. But I think this is about good to merge. @roblourens can you give this a spin as well?

roblourens · 2024-01-08T18:19:30Z

If we are going with "Add Wait For Breakpoint" as the name of the feature, should it be "Add Wait-For Breakpoint"? I wouldn't mind "Add Triggerpoint" personally

roblourens · 2024-01-08T18:25:41Z

I think that hiding the breakpoint widget after I pick a breakpoint in the dropdown is odd, if the OK button is there I should have to click it

gayanper · 2024-01-08T18:27:37Z

I think that hiding the breakpoint widget after I pick a breakpoint in the dropdown is odd, if the OK button is there I should have to click it

I can have a look at it now

gayanper · 2024-01-08T18:32:00Z

I think that hiding the breakpoint widget after I pick a breakpoint in the dropdown is odd, if the OK button is there I should have to click it

I can have a look at it now

In this case, should add a cancel button as well ? Or should we let cancellation be handled by escape in editor ?

src/vs/workbench/contrib/debug/browser/debugSession.ts

roblourens · 2024-01-08T18:38:51Z

In this case, should add a cancel button as well ? Or should we let cancellation be handled by escape in editor ?

Cancel by escape is fine, same as for other types of breakpoints.

src/vs/workbench/contrib/debug/browser/debugSession.ts

connor4312 · 2024-01-10T00:56:20Z

I think this is good to go, thank you for all your work on this @gayanper! 👏

The final vocabulary for this is "triggered breakpoint" and "triggering breakpoint" The command name to create a new breakpoint is Debug: Add Triggered Breakpoint.... I kept "Wait for Breakpoint" in the dropdown of the breakpoint widget, since to me that's describing what the input for the breakpoint is, and "wait for breakpoint... foo.js: 12" reads very nicely. It's also a good hint for users who see that new breakpoint type but don't immediately know what a "Triggered Breakpoint" is.

gayanper · 2024-01-10T06:08:52Z

Thanks a lot @connor4312 for the support to get this feature in. 👍

roblourens · 2024-01-10T18:50:55Z

Thanks for the contribution, this is cool! Re: the name "triggered breakpoint", it doesn't quite roll off the tongue, I will see if I get used to it.

gayanper mentioned this pull request Nov 12, 2022

Add support for trigger points #159948

Closed

vscodenpa assigned roblourens Nov 12, 2022

gayanper force-pushed the wait_for_breakpoint branch from 4f30831 to cf759e5 Compare November 13, 2022 11:15

connor4312 mentioned this pull request Oct 4, 2023

Protocol extension points microsoft/debug-adapter-protocol#435

Closed

connor4312 self-assigned this Dec 9, 2023

roblourens removed their assignment Dec 15, 2023

gayanper force-pushed the wait_for_breakpoint branch from 62f7ecc to 6c4ef9f Compare December 17, 2023 07:55

connor4312 reviewed Dec 18, 2023

View reviewed changes

gayanper and others added 3 commits December 18, 2023 18:58

Add support for breakpoint dependencies

4ba8b88

update the PR according to feedback

8c05225

Update src/vs/workbench/contrib/debug/browser/breakpointWidget.ts

1275188

Co-authored-by: Connor Peet <connor@peet.io>

gayanper force-pushed the wait_for_breakpoint branch from 0ddcf25 to 2f5c1bd Compare December 18, 2023 17:58

change breakpoint lookup at current frame and exclude disable depende…

37070e9

…nt breakpoints

gayanper force-pushed the wait_for_breakpoint branch from 2f5c1bd to 37070e9 Compare December 19, 2023 19:58

feedback changes

8b30e7b

- change feature name to triggeredBy - add close button - add actions - add confirmation on toggle

gayanper force-pushed the wait_for_breakpoint branch from d60e245 to 8b30e7b Compare December 20, 2023 20:08

re-apply changes with css fixups

203dd25

add icon support for better diff look of pending breakpoints

05e23a3

connor4312 added 2 commits January 5, 2024 15:35

unlink dependent breakpoints when parent is removed, fixed column mat…

cce8983

…ching

Capitalize labels

0d85618

roblourens reviewed Jan 8, 2024

View reviewed changes

src/vs/workbench/contrib/debug/browser/debugSession.ts Outdated Show resolved Hide resolved

gayanper added 2 commits January 8, 2024 19:44

Only update breakpoint when OK is clicked.

7eea545

Make sure dependent breakpoints updates are sent to DA before continue

217d2ac

connor4312 reviewed Jan 8, 2024

View reviewed changes

src/vs/workbench/contrib/debug/browser/debugSession.ts Outdated Show resolved Hide resolved

connor4312 added 2 commits January 9, 2024 16:32

Merge remote-tracking branch 'origin/main' into pr/gayanper/166202-1

c116ed3

wait to resume until breakpoint is set, align vocabulary

a308dde

connor4312 previously approved these changes Jan 10, 2024

View reviewed changes

connor4312 enabled auto-merge January 10, 2024 00:52

vscodenpa added this to the December / January 2024 milestone Jan 10, 2024

one more place

207197a

connor4312 dismissed their stale review via 207197a January 10, 2024 00:59

connor4312 approved these changes Jan 10, 2024

View reviewed changes

joaomoreno approved these changes Jan 10, 2024

View reviewed changes

connor4312 merged commit ea77026 into microsoft:main Jan 10, 2024
6 checks passed

gayanper deleted the wait_for_breakpoint branch January 10, 2024 18:38

connor4312 mentioned this pull request Jan 19, 2024

Add support for breakpoint dependencies (trigger points) #152428

Closed

gjsjohnmurray mentioned this pull request Feb 8, 2024

wait for breakpoint loading .... #204699

Closed

microsoft locked and limited conversation to collaborators Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for breakpoint dependencies #166202

Add support for breakpoint dependencies #166202

gayanper commented Nov 12, 2022 •

edited

Loading

connor4312 commented Dec 9, 2023

gayanper commented Dec 9, 2023

connor4312 commented Dec 9, 2023

gayanper commented Dec 17, 2023

gayanper commented Dec 17, 2023 •

edited

Loading

connor4312 commented Dec 18, 2023 •

edited

Loading

connor4312 left a comment

connor4312 commented Dec 20, 2023

gayanper commented Dec 20, 2023

connor4312 commented Dec 20, 2023

connor4312 commented Dec 20, 2023

gayanper commented Dec 21, 2023

gayanper commented Dec 21, 2023

connor4312 commented Jan 6, 2024

roblourens commented Jan 8, 2024 •

edited

Loading

roblourens commented Jan 8, 2024

gayanper commented Jan 8, 2024

gayanper commented Jan 8, 2024

roblourens commented Jan 8, 2024

connor4312 commented Jan 10, 2024 •

edited

Loading

gayanper commented Jan 10, 2024

roblourens commented Jan 10, 2024

Add support for breakpoint dependencies #166202

Add support for breakpoint dependencies #166202

Conversation

gayanper commented Nov 12, 2022 • edited Loading

connor4312 commented Dec 9, 2023

gayanper commented Dec 9, 2023

connor4312 commented Dec 9, 2023

gayanper commented Dec 17, 2023

gayanper commented Dec 17, 2023 • edited Loading

connor4312 commented Dec 18, 2023 • edited Loading

connor4312 left a comment

Choose a reason for hiding this comment

connor4312 commented Dec 20, 2023

gayanper commented Dec 20, 2023

connor4312 commented Dec 20, 2023

connor4312 commented Dec 20, 2023

gayanper commented Dec 21, 2023

gayanper commented Dec 21, 2023

connor4312 commented Jan 6, 2024

roblourens commented Jan 8, 2024 • edited Loading

roblourens commented Jan 8, 2024

gayanper commented Jan 8, 2024

gayanper commented Jan 8, 2024

roblourens commented Jan 8, 2024

connor4312 commented Jan 10, 2024 • edited Loading

gayanper commented Jan 10, 2024

roblourens commented Jan 10, 2024

gayanper commented Nov 12, 2022 •

edited

Loading

gayanper commented Dec 17, 2023 •

edited

Loading

connor4312 commented Dec 18, 2023 •

edited

Loading

roblourens commented Jan 8, 2024 •

edited

Loading

connor4312 commented Jan 10, 2024 •

edited

Loading