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

Surface new Exception DAP in UI #22078

Closed
weinand opened this issue Mar 6, 2017 · 22 comments
Closed

Surface new Exception DAP in UI #22078

weinand opened this issue Mar 6, 2017 · 22 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Mar 6, 2017

In the last milestone we've added new debug adapter protocol to retrieve information about a thrown exception and we've introduced a peek UI to show this information to the user.

Now we have to bring both things together.

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Mar 6, 2017
@weinand weinand added this to the March 2017 milestone Mar 6, 2017
@kieferrm kieferrm mentioned this issue Mar 6, 2017
58 tasks
@weinand
Copy link
Contributor Author

weinand commented Mar 16, 2017

I've implemented a first cut of the new exception info request in node-debug.
Now VS Code needs to make use of this.

If the capability object returned from the InitializeRequest has a value true for the supportsExceptionInfoRequest, the exceptionInfoRequest is supported by the DA. In this case VS Code should populate the exception peek UI from the values returned from the exceptionInfoRequest instead of using the StoppedEvent.

The exceptionInfoRequest expects the threadId as the only input argument.
It returns the following structure:

2017-03-16_16-11-34

The additional information (for now) is the breakMode and the details.stacktrace. Both should be surfaced in the peek UI.

@isidorn
Copy link
Contributor

isidorn commented Mar 22, 2017

This has been done by @michelkaporin in #22948

@isidorn isidorn closed this as completed Mar 22, 2017
@isidorn
Copy link
Contributor

isidorn commented Mar 22, 2017

Tried it out and it looks good to me.
@weinand why does the protocol surround the stack trace with quotes - I think it would look nicer if no quotes were shown
@roblourens it would be cool if node2 debug adapter also picks up the support for exceptions so we do not have inconsistencies between these two expeirences
@michelkaporin can you please create a short test plan item for this feature

screen shot 2017-03-22 at 12 47 13

@weinand
Copy link
Contributor Author

weinand commented Mar 22, 2017

@isidorn The stack trace is surrounded by quotes because it is a "string" value (and we always do the value formatting in the backend so that VS Code does not have to second guess). So I will remove the quotes in this case.

@michelkaporin I suggest to introduce some space between the exception description ("Unhandled exception has occurred....") and the stack trace (e.g. 1/2 a line). Without this the UI looks a bit crammed.

@weinand
Copy link
Contributor Author

weinand commented Mar 23, 2017

I'm removing the quotes from the stacktrace now:
microsoft/vscode-node-debug@81c69ed

@roblourens
Copy link
Member

Btw I just took a PR to apply sourcemaps to stacktraces. I think it will look very nice with this widget.

microsoft/vscode-chrome-debug-core#190

@weinand
Copy link
Contributor Author

weinand commented Mar 23, 2017

But this requires the ExceptionInfo request for node2...

@roblourens
Copy link
Member

I have an issue to implement it for node2

@michelkaporin
Copy link
Contributor

michelkaporin commented Mar 29, 2017

@weinand You've mentioned:

The additional information (for now) is the breakMode and the details.stacktrace. Both should be surfaced in the peek UI.

Sometimes details.stacktrace is not available from the DA response. What would be your suggestion to show instead of it in the exception widget then?
Currently DA response if surfaced the following way (structure with current showcase below):

breakMode-type exception has occurred. description
details.stackTrace

image

My proposal would be to fall back to the UI case when DA does not support exceptionInfo request, but with breakMode-type message included (see the image below).

image

Would be good to hear @isidorn opinion also.

This decision is needed as part of fixing #23388.

@michelkaporin
Copy link
Contributor

michelkaporin commented Mar 29, 2017

FYI This is how it looks like when DA does not support exceptionInfo request:
image

@weinand
Copy link
Contributor Author

weinand commented Mar 29, 2017

If details (and hence the stacktrace) is missing we should still continue to use the new layout and not fallback to the old layout.

The rational behind this is as follows:
In the future there will be more details coming through the ExceptionInfo request and we will have to integrate them into the layout as well. Always falling back to the old layout doesn't make a lot of sense because there will be always some details provided that are not covered by the old layout at all. And we do not want to lose those.

@isidorn
Copy link
Contributor

isidorn commented Mar 29, 2017

I agree with @weinand

@michelkaporin
Copy link
Contributor

michelkaporin commented Mar 29, 2017

Makes perfect sense. So this is the way we go for then, omitting the stack trace completely if it is not returned by a DA:
image

@weinand
Copy link
Contributor Author

weinand commented Mar 29, 2017

the latest DA will always return an exceptionID (as spec'ed). So this will result in:

Unhandled exception has occurred. string: heello

@michelkaporin
Copy link
Contributor

Currently we don't surface exceptionID, so I would be glad to hear how do we do it in a better way, because node DA description already contains exceptionID in its current implementation.

Printing it twice would make the information redundant.

@weinand
Copy link
Contributor Author

weinand commented Mar 29, 2017

In the original protocol change request 'Exception ID' got this example usage:

Exception ID (e.g. "CLR/System.ArgumentException")

In VS it is shown prominently as the title:
2017-02-02_17-48-49

So my proposal would be:

${breakMode} exception: ${exceptionID}
${description}

${details.stackTrace}

@michelkaporin
Copy link
Contributor

@weinand I agree with your proposal, and currently it looks the following way with the latest version of node DA.

  • Node DA with exceptionInfo request support:
    image
    image

  • Node DA without exceptionInfo request support:
    image

It would be great to hear also @isidorn's feedback on that, and then to have your change in node DA so that it does not return redundant information as in the first image, for instance.

@weinand
Copy link
Contributor Author

weinand commented Mar 29, 2017

Since the description is optional, I'm planning to omit the description if it already appears in the stack trace.

@michelkaporin Please make sure that there are not two empty lines in that case.

@weinand
Copy link
Contributor Author

weinand commented Mar 29, 2017

@roblourens VS Code now uses the following layout for the exception peek UI:

${breakMode} exception: ${exceptionID}
${description}

${details.stackTrace}

To avoid duplication of the description ("Error: heello" from above), I'm now omitting the description from the response if it already appears at the beginning of the stack trace (I prefer this approach over removing the first line from the stacktrace because it leaves the stacktrace intact).

@roblourens
Copy link
Member

I see 'undefined' in the exception widget now:

image

@michelkaporin
Copy link
Contributor

michelkaporin commented Mar 30, 2017

@roblourens VS Code now uses the following layout for the exception peek UI:

I have not pushed my changes yet to make this layout, waiting for our UI master @isidorn's feedback and then will update the widget accordingly.

Hence, @roblourens you started seeing 'undefined' because of @weinand change above, which is not yet in sync with current widget UI implementation.

@roblourens
Copy link
Member

Oh ok. I'll make the same change in node2.

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

4 participants