Allow different exception breakpoints from multiple debuggers to be shown at once#158355

nisargjhaveri · 2022-08-17T09:56:33Z

Instead of replacing all exception breakpoints, add new exception breakpoints when a new debug sessions starts.

This way, the existing exception breakpoints stay as is. If no active debug session supports an existing exception breakpoint, it is shown as disabled and can be removed manually. A currently supported exception breakpoint cannot be removed manually as long as the session is active.

This gives more control to the user and saves the breakpoint preferences across different debug sessions in the workspace.

roblourens · 2022-08-17T23:09:30Z

Instead of replacing all exception breakpoints, add new exception breakpoints when a new debug sessions starts.

I don't think I like this approach. I would rather simply see the available exception breakpoint filters for the selected session. I think that mixing filters from different debuggers will be confusing. Also it seems that this PR allows for manually deleting exception breakpoint filters, and I don't understand that.

nisargjhaveri · 2022-08-18T04:01:00Z

Also it seems that this PR allows for manually deleting exception breakpoint filters, and I don't understand that.

With these changes, we're always adding new exception breakpoints, the old once (from other debuggers) are never automatically deleted. For that, I've added an option to delete exception breakpoints only if they're not from the current running debuggers.

I don't think I like this approach. I would rather simply see the available exception breakpoint filters for the selected session. I think that mixing filters from different debuggers will be confusing.

Sure. Can you please suggest how should we approach this? I'm new and I could not figure out how we'll support storing preferences from previous runs, if we simply pull the available exception filters from the selected debugger. This seemed important as otherwise we'll always fallback to the defaults set by the adapter on starting the new session. Suggestions?

roblourens · 2022-08-18T16:31:06Z

We should just show the exception filters for whichever debug session is active. It could be that a different model is loaded into the tree, or maybe a tree filter is helpful.

nisargjhaveri · 2022-08-27T17:46:23Z

@roblourens I've updated the PR. Please have a look?

I've moved the exception breakpoints ownership to DebugSession. The exception breakpoints now can have per session enablement and other properties.

The model (and view) reflects exception breakpoints of the last focused session, it updates on "did focus" event. The enablement is restored/duplicated from model when a new debug session starts.

roblourens · 2022-08-30T18:38:35Z

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

 			this.initialized = true;
 			this._onDidChangeState.fire();
-			this.debugService.setExceptionBreakpoints((this.raw && this.raw.capabilities.exceptionBreakpointFilters) || []);
+			this.exceptionBreakpoints = this.debugService.createExceptionBreakpoints((this.raw && this.raw.capabilities.exceptionBreakpointFilters) || []);


I don't think it's necessary to call all the way into the DebugModel here to create real ExceptionBreakpoint objects. I think it's ok for the DebugSession to expose raw exceptionBreakpointFilters and we can create real objects when that session is selected.

Seems that many things exposed by the DebugSession are raw DebugProtocol types

The alternative would be to keep a map from session to exception breakpoints in model or somewhere else, if we ultimately want different breakpoint configuration per session, no?

Theoretically, in this case, session owns the breakpoints, so made sense to keep it in session.

Model has the info required for creating the real ExceptionBreakpoint objects, i.e. values from last/current session to be used as default.

Agreed that the current flow is not intuitive. Suggestions?

I see that the enablement state of the exception breakpoint is on that object, so those need to live in some map somewhere. I think that putting them on the session is not right, because if I enable "caught exceptions" for one node debug session, that should be enabled for all node debug sessions. So you could have a map from debug adapter type to exception filters for that type.

But if I am debugging an electron app, you have node and chrome configs, those DAs contribute the same breakpoint filters, and if I enable "caught exceptions" for node, I want it to also be enabled for chrome. We could keep a set of all exception breakpoint filters each with the debug types that they are enabled for. I think we should actually be working towards something like that.

ExceptionBreakpointsFilter has an ID. If an unrelated DA contributes an exception filter with the same ID but a different label, should it be treated the same as the node/chrome filter? Or should we look at all fields of ExceptionBreakpointsFilter to decide whether it's a match?

I see that the enablement state of the exception breakpoint is on that object, so those need to live in some map somewhere. I think that putting them on the session is not right, because if I enable "caught exceptions" for one node debug session, that should be enabled for all node debug sessions. So you could have a map from debug adapter type to exception filters for that type.

Having exception breakpoints in session, including enablement also fixes the scenario mentioned in #147096. But to be fair, if we want enablement different for every session, it could apply to other breakpoints as well, depending on the particular multi-session scenario. Though with this restoring earlier preferences doesn't work so well. Primarily because we'll anyway have to choose which session to store if multiple session from the same DA are active. To store different preferences for different DAs would also be challenging this case.

But if I am debugging an electron app, you have node and chrome configs, those DAs contribute the same breakpoint filters, and if I enable "caught exceptions" for node, I want it to also be enabled for chrome. We could keep a set of all exception breakpoint filters each with the debug types that they are enabled for. I think we should actually be working towards something like that.

ExceptionBreakpointsFilter has an ID. If an unrelated DA contributes an exception filter with the same ID but a different label, should it be treated the same as the node/chrome filter? Or should we look at all fields of ExceptionBreakpointsFilter to decide whether it's a match?

What if we keep the exception breakpoints separate for each DA always, even if the id and name are same? We could show the breakpoints from all sessions, along with the DA name it is coming from to differentiate those. Which would also reduce confusion since the breakpoint pane won't change as per the active session. The user gets the choice to select exception breakpoints per DA. Since the two sessions are using different DAs, it is also likely that the significance and usage of the exception breakpoints can also be different, even if the id/name are same.

The original changes I made was somewhat similar to this, where the user sees all the exception breakpoints at once (though it did not differentiate per DA as such, it only looked at the id). The original changes also gave better experience when it comes to saving and restoring the breakpoints across multiple DAs, with a twist that if some EB from an inactive DA that are no longer needs to be saved, they needed to be manually deleted by the user. I feel something like that would give a much better experience when working with multiple debuggers at once. I can try this approach again if it makes sense.

If we want to share EB state across sessions, session can no longer own it, which is the case with the current changes.

Here is what I'm thinking, though, cannot figure out a couple of things.

Model can keep a list of all exception breakpoints across currently active DAs. When a new session starts, we add exception breakpoints for it to the model if not already exists with same id/name. Model can then expose a method to get exception breakpoints for a particular session for the UI to display.

When do we remove the old exception breakpoints from the model in this case?

We could remove breakpoints from the model when the session ends, if no other active session supports it. If we do this, we regress the current behaviour of saving the enablement for exception breakpoints across restarts of a session.

We "could" never remove it. In that case the enablement is remembered even for multi-debugger cases as well, which is great. But the model would bloat over time in theory.

Btw, finding a design that can save enablement even for multi-debugger scenarios would help a lot. It is annoying that the exception breakpoints would fallback to default every time I start debugging with a multi-debugger setup.

Suggestions?

What you are describing is pretty much what I have in mind. I think that just looking at the ID is probably ok (worth taking a quick look at popular DA extensions) and saving the selection in workspace state forever is also fine.

Just looking at the id won't be sufficient since the breakpoints can have different support for conditions along with different names and descriptions.

For example, Java and Nodejs debugger has an overlapping id, but with different condition support and different descriptions.

I've updated the PR with changes, have a look?

For example, Java and Nodejs debugger has an overlapping id, but with different condition support and different descriptions.

Interesting, what is that ID?

nodejs provides 'uncaught' and 'all', Java provides 'uncaught' and 'caught'. 'uncaught' is the conflicting one for these two.

Since the name are generic, I'd guess more debuggers would have similar conflicting names.

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

src/vs/workbench/contrib/debug/common/debugModel.ts

roblourens · 2022-08-30T18:50:38Z

A couple comments but this is basically what I had in mind, thank you for the PR!

- Keep track of all exception breakpoints, instead of the ones from last started session - Update breakpoints view when focused debugger changes to show the correct set of exception breakpoints - Store all exception breakpoints in model for restoring enablement later - Only send the correct set of breakpoints for each debug adapter when updated

nisargjhaveri · 2022-09-29T05:06:44Z

Updated the PR.

Some behaviour differences with this:

The exception breakpoints shows up when the session is done attaching, compared to when the session was started earlier.
When no session is active, no exception breakpoints are visible. Earlier, we kept showing the once from the last session.

nisargjhaveri · 2022-09-29T05:09:09Z

src/vs/workbench/contrib/debug/common/debugModel.ts

 		return this.exceptionBreakpoints;
 	}

+	getExceptionBreakpointsForSession(sessionId?: string): IExceptionBreakpoint[] {


I've changes some usages of getExceptionBreakpoints with the new getExceptionBreakpointsForSession when appropriate.

The usage in logDebugSessionStart in debugTelemetry.ts, I'm not sure about what is the preferred one to use.

roblourens · 2022-09-29T18:15:18Z

When no session is active, no exception breakpoints are visible. Earlier, we kept showing the once from the last session.

I actually think it should continue to show the breakpoints from the last session. I think that the most common usecase is rerunning the same debug type, and changing these exception breakpoints before you start debugging is a common task.

…t active

nisargjhaveri · 2022-10-02T13:35:18Z

I actually think it should continue to show the breakpoints from the last session. I think that the most common usecase is rerunning the same debug type, and changing these exception breakpoints before you start debugging is a common task.

Agreed. I've pushed one more change to fix this.

nisargjhaveri · 2022-10-28T13:11:48Z

@roblourens is anything pending for this one? It'd be great if you could have a look once at the latest changes.

roblourens · 2022-10-28T16:54:51Z

Sorry, I didn't have time to work on it this month but I'll try to take a look again after our release next week.

roblourens

This works great, thanks for tackling it. Last request- could you think about testing for the new behavior? A couple tests in breakpoints.test.ts would probably be sufficient.

src/vs/workbench/contrib/debug/common/debugModel.ts

nisargjhaveri · 2022-11-13T10:41:33Z

@roblourens I've updated the review and added more tests for this as well, which should cover most scenarios.

nisargjhaveri mentioned this pull request Aug 17, 2022

Breakpoints pane should update when debugger has switched #126608

Closed

vscodenpa assigned roblourens Aug 17, 2022

nisargjhaveri force-pushed the nisargjh/issue-126608 branch from 9051e62 to 5a728ed Compare August 27, 2022 17:42

roblourens reviewed Aug 30, 2022

View reviewed changes

src/vs/workbench/contrib/debug/common/debugModel.ts Outdated Show resolved Hide resolved

nisargjhaveri force-pushed the nisargjh/issue-126608 branch from c695441 to 6ebbd15 Compare September 29, 2022 05:01

nisargjhaveri commented Sep 29, 2022

View reviewed changes

Show last session's exception breakpoints even when the session is no…

2e8f92e

…t active

roblourens added this to the October 2022 milestone Oct 3, 2022

roblourens modified the milestones: October 2022, November 2022 Oct 24, 2022

Merge branch 'main' into pr/nisargjhaveri/158355

88870fc

roblourens enabled auto-merge (squash) November 12, 2022 23:08

roblourens disabled auto-merge November 12, 2022 23:08

roblourens reviewed Nov 12, 2022

View reviewed changes

src/vs/workbench/contrib/debug/common/debugModel.ts Show resolved Hide resolved

Add more tests for exception breakpoints in multiple sessions

3dbbb1b

tsdoc commetns

4e9605c

lramos15 approved these changes Nov 17, 2022

View reviewed changes

roblourens approved these changes Nov 17, 2022

View reviewed changes

roblourens merged commit 8f2c8e3 into microsoft:main Nov 17, 2022

github-actions bot locked and limited conversation to collaborators Jan 2, 2023

Conversation

nisargjhaveri commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roblourens commented Aug 17, 2022

Uh oh!

nisargjhaveri commented Aug 18, 2022

Uh oh!

roblourens commented Aug 18, 2022

Uh oh!

nisargjhaveri commented Aug 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

roblourens commented Aug 30, 2022

Uh oh!

nisargjhaveri commented Sep 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roblourens commented Sep 29, 2022

Uh oh!

nisargjhaveri commented Oct 2, 2022

Uh oh!

nisargjhaveri commented Oct 28, 2022

Uh oh!

roblourens commented Oct 28, 2022

Uh oh!

roblourens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nisargjhaveri commented Nov 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nisargjhaveri commented Aug 17, 2022 •

edited

Loading