Suggestions on new conditional exception protocol

[gregg-miskelly]

I was looking at the new support for conditional exceptions and I have two suggestions:

1. Handling errors - The protocol doesn't specify what an adapter should do if the user input an invalid condition. I feel like the right answer is that an adapter can just fail the SetExceptionBreakpointsRequest. But I felt like maybe this should be called out?
2. Providing help text - for the C#/C++ extension, I don't know that it will be super obvious to folks what syntax they should use for conditions. It would be helpful if ExceptionBreakpointsFilter had an additional optional description field that would be displayed in a datatip similar to the experience of edited launch.json in VS Code. This would allow a debug adapter to provide examples and link to complete documentation.

[weinand]

@gregg-miskelly thanks for the feedback!

I've tried to address the first item by explaining what to do for invalid exception filters:

/** SetExceptionBreakpoints request; value of command field is 'setExceptionBreakpoints'.
    The request configures the debuggers response to thrown exceptions.
    If an exception is configured to break, a 'stopped' event is fired (with reason 'exception').
    Clients should only call this request if the capability 'exceptionBreakpointFilters' returns one or more filters.
    If a filter or filter option is invalid (e.g. due to an invalid 'condition'),
    the request should fail with an 'ErrorResponse' explaining the problem(s).
*/
  export interface SetExceptionBreakpointsRequest extends Request {
    // command: 'setExceptionBreakpoints';
    arguments: SetExceptionBreakpointsArguments;
  }

For the second item I've added a new description property to the ExceptionBreakpointsFilter. I'm interpreting your feedback that the description should apply to the ExceptionBreakpointFilter as a whole and not only to the condition property:

/** An ExceptionBreakpointsFilter is shown in the UI as an filter option for configuring how exceptions are dealt with. */
export interface ExceptionBreakpointsFilter {
  /** The internal ID of the filter option. This value is passed to the 'setExceptionBreakpoints' request. */
  filter: string;
  /** The name of the filter option. This will be shown in the UI. */
  label: string;
  /** An optional help text providing additional information like the condition's syntax.
      This string is typically shown in the UI as a hover and must be translated. */
  description?: string;
  /** Initial value of the filter option. If not specified a value 'false' is assumed. */
  default?: boolean;
  /** Controls whether a condition can be specified for this filter option.
      If false or missing, a condition can not be set. */
  supportsCondition?: boolean;
}

@gregg-miskelly @isidorn @connor4312 I'd appreciate your feedback

[isidorn]

@weinand this makes sense to me.

However another place in the UI which we should consider to allow debug adapters to contribute to is the placeholder text for the exception breakpoint conditions. I am not sure if C# would potentially want to customise this text. But I think this placeholder text is more easily seen than the hover.

[weinand]

@gregg-miskelly what's your take on Isi's suggestion?

[connor4312]

I agree on the placeholder text, and I think the placeholder supersedes the need for a 'default'. There's no sensible default I think debuggers would show there: the user just took an action to supply their own condition, and as a debugger I have no idea what it will be.

[gregg-miskelly]

Place holder text sounds does sound very useful. But I think a hover text would still be nice so we can link to complete documentation.

[weinand]

Thanks for the feedback.
I will add another optional property conditionPlaceholder to the ExceptionBreakpointsFilter.

@connor4312 what do you mean by 'default'?
Today we use a hardcoded placeholder text ('Break when expression evaluates to true'), but since a new placeholder property must be optional, we would have to keep the hardcoded placeholder text anyway.

[weinand]

Since the term "placeholder" is not used in DAP, I've decided to use conditionDescription for the new property:

/** An ExceptionBreakpointsFilter is shown in the UI as an filter option for configuring how exceptions are dealt with. */
export interface ExceptionBreakpointsFilter {
  /** The internal ID of the filter option. This value is passed to the 'setExceptionBreakpoints' request. */
  filter: string;
  /** The name of the filter option. This will be shown in the UI. */
  label: string;
  /** An optional help text providing additional information about the exception filter.
      This string is typically shown as a hover and must be translated. */
  description?: string;
  /** Initial value of the filter option. If not specified a value 'false' is assumed. */
  default?: boolean;
  /** Controls whether a condition can be specified for this filter option.
      If false or missing, a condition can not be set. */
  supportsCondition?: boolean;
  /** An optional help text providing information about the condition.
      This string is shown as the placeholder text for a text box and must be translated. */
  conditionDescription?: string;
}

@gregg-miskelly @isidorn @connor4312
If there are no objections I'll release the two DAP changes today.