Add condition editing UI to breakpoint filters

[weinand]

With microsoft/debug-adapter-protocol#137 "condition" support has arrived in the Debug Adapter Protocol.

This feature requests asks for adding condition editing UI for breakpoint filters.

Implementation sketch:

• if DA has capability supportsExceptionFilterOptions it will use the new protocol
• the exceptionBreakpointFilters capability can now return a supportsCondition property which indicates that VS Code should enable a "condition" edit UI for the exception filter.
• when calling the setExceptionBreakpoints request the set of exceptions must be passed via the filterOptions property instead of the filters property. The filterOptions property takes an array of pairs consisting of the breakpoint filter ID and an optional condition. If none of the exception filters has a condition, the filters property can be used.

Proposal for condition editing UI:

The simplest "condition" edit UI would be to provide a context menu action that opens an editable text box on top the exception filters name (identical to the function breakpoints, see green arrow below). If an exception filter has a condition, it could be rendered at the end of the exception's name in a dimmed style (similar to the path style that we use for regular breakpoints; see red arrow):

/cc @connor4312

[isidorn]

@weinand this proposal makes sense to me.
Also the UI proposal makes sense. The downside of this UI: it is harder for use to input complex conditions due to the limited horizontal space. Also it is not possible to input multiline conditions.
Though I think it should be fine.

If we do this for exception breakpoint I would do 100% the same UI for Function breakpoint conditions.

Maybe @connor4312 has an idea for the UI

I will have time this week, and once we settle on the UI I can look into adding it on Wed / Thu / Fri, thus assigning to November and we can always push back if we do not find enough time.

[weinand]

@isidorn I've added support for exception filter conditions in Mock Debug

[isidorn]

@weinand great, thanks!

[isidorn]

@weinand started looking into this. One question:

• supportsExceptionFilterOptions does not seem to be needed. I think if the exception breakpoint has a supportsCondition flag set that is enough for VS Code to handle everything appropriatly.

[isidorn]

@weinand filters property is mandatory.
So if any of the exception breakpoints has a condition I will send an empty filters [] and send all the exceptions breakpoints via the filterOptions

[weinand]

@isidorn thanks for feedback.

• supportsExceptionFilterOptions: is it possible that an exception breakpoint (with a condition) gets persisted and that a subsequent "setExceptionBreakpoints" request is executed against an old DA that does not support the filterOptionsproperty? In this case the supportsExceptionFilterOptions capability would protect VS Code from relying on an unsupported property.
• filters: good find. I will update the spec and explain that filters and filterOptions are additive. That covers your empty array too.

[isidorn]

• No, this should not be possible, since the exception breakpoint filters are set first thing once the session starts. So the new session would overwrite hte supportsCondition attribute. However it is probably possible to have a mutli session scenario and get into a bad state if one session is a DA which does not support, and one which does.
• Great, thanks

[isidorn]

@weinand is there some difference between sending undefined for condition or an empty string as part of filterOptions? What should vscode send for exception breakpoint that supportCondition but do not have a condition set (or a user unset it)

[weinand]

@isidorn just omit the property condition if an exception filter doesn't have a condition.

[weinand]

@isidorn After contemplating about the supportsExceptionFilterOptions question from above, I came to the conclusion that it is still better to have supportsExceptionFilterOptions. Here is why:

If simplifies the way clients have to call setExceptionBreakpoints. If supportsExceptionFilterOptions is true, clients can just use the filterOptions property on the setExceptionBreakpoints request and do not have to worry about the old filters property.
Without a supportsExceptionFilterOptions capability, clients have to iterate through all exceptionBreakpointFilters and check for the existence of the supportsCondition property now (and for additional properties in the future) in order to learn whether they can use the filterOptions property on the setExceptionBreakpoints request or not. This sounds more laborious and error prone.

[isidorn]

@weinand makes sense. We can keep it.

I have pushed a first version of this. I have tried this out with the Mock debug and seems to work fine.
Try it out and let me know your toughts.
I plan to write a test plan item, should I mention to use Mock debug or does some debug adapter already plan to support this?

.

[isidorn]

fyi @connor4312 not sure if js-debug could support this already

[weinand]

Since @connor4312 asked for the original DAP feature (microsoft/debug-adapter-protocol#137), he probably has a use case in mind that we could use in the test plan.

[isidorn]

That would be awesome. For now I created an empty template for the testplan item and I can fill it on Monday.
@connor4312 if you do not find time to support this in js-debug let us know and I can write the test plan item for Mock. Or I can assing you so you implement and test it (if it is not a lot of work).

[connor4312]

I'm on vacation next week so unfortunately won't be around to test this

[isidorn]

@connor4312 thanks for letting us know. Have a nice vacation and we can test this with mock debug

[connor4312]

I've now implemented this in js-debug 🙂

[isidorn]

@connor4312 awesome!