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.

Sign up for GitHub

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

Jump to bottom

Surfacing new Exception DAP in UI #22948

Merged
merged 6 commits into from
Mar 22, 2017
Merged

Surfacing new Exception DAP in UI #22948

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b304851
Added support for querying the debug adapter for the exception info.
michelkaporin Mar 21, 2017
8e293fd
Added exception condition message. Moved exceptionInfo query logic to…
michelkaporin Mar 21, 2017
4444eeb
Added missing null check.
michelkaporin Mar 21, 2017
21680a0
Updated MockSession with exceptionInfo signature.
michelkaporin Mar 21, 2017
c5ca501
Introduced an exception abstraction, moved the exception info check l…
michelkaporin Mar 22, 2017
e53980c
Remove redundant check as it is already checked in the thread attribute.
michelkaporin Mar 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions src/vs/workbench/parts/debug/browser/exceptionWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const $ = dom.$;

export class ExceptionWidget extends ZoneWidget {

constructor(editor: ICodeEditor, private lineNumber: number,
constructor(editor: ICodeEditor, private exceptionInfo: DebugProtocol.ExceptionInfoResponse, private lineNumber: number,
@IContextViewService private contextViewService: IContextViewService,
@IDebugService private debugService: IDebugService
) {
Expand All @@ -33,15 +33,40 @@ export class ExceptionWidget extends ZoneWidget {
const fontInfo = this.editor.getConfiguration().fontInfo;
this.container.style.fontSize = `${fontInfo.fontSize}px`;
this.container.style.lineHeight = `${fontInfo.lineHeight}px`;

let title = $('.title');
title.textContent = nls.localize('exceptionThrown', 'Exception occurred');
dom.append(container, title);

const thread = this.debugService.getViewModel().focusedThread;

if (thread && thread.stoppedDetails) {
let title = $('.title');
let msg = $('.message');
msg.textContent = thread.stoppedDetails.text;

if (this.exceptionInfo) {
let conditionMessage;
switch (this.exceptionInfo.body.breakMode) {
case 'never':
conditionMessage = nls.localize('neverException', 'User-handled exception has occurred.');
break;
case 'always':
conditionMessage = nls.localize('alwaysException', 'Always-breaking exception has occurred.');
break;
case 'unhandled':
conditionMessage = nls.localize('unhandledException', 'Unhandled exception has occurred.');
break;
case 'userUnhandled':
conditionMessage = nls.localize('userUnhandledException', 'User-unhandled exception has occurred.');
break;
default:
conditionMessage = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if the default case had the same Exception occurred message as the case when there is no exceptionInfo

break;
}

title.textContent = `${conditionMessage} ${this.exceptionInfo.body.description}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing a colon here between the condition message and the description

Copy link
Contributor Author

@michelkaporin michelkaporin Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we agreed on putting a colon there, but I did not put it explicitly and used a dot on the conditionMessage instead, because description itself contains a colon after the exception type, so that it ends up as Unhandled exception has occurred: Error: test message which looks ugly with two colons in my opinion.
So currently it is printed like that:
Unhandled exception has occurred. Error: test message
Although I can use quotes on the description to make it look better with two colons:
Unhandled exception has occurred: 'Error: test message'

What do you think is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how it looks now:

image

msg.textContent = this.exceptionInfo.body.details.stackTrace;
} else {
title.textContent = nls.localize('exceptionThrown', 'Exception occurred');
msg.textContent = thread.stoppedDetails.text;
}

dom.append(container, title);
dom.append(container, msg);
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/vs/workbench/parts/debug/common/debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export interface IExpression extends ITreeElement, IExpressionContainer {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

export interface ISession {
stackTrace(args: DebugProtocol.StackTraceArguments): TPromise<DebugProtocol.StackTraceResponse>;
exceptionInfo(args: DebugProtocol.ExceptionInfoArguments): TPromise<DebugProtocol.ExceptionInfoResponse>;
scopes(args: DebugProtocol.ScopesArguments): TPromise<DebugProtocol.ScopesResponse>;
variables(args: DebugProtocol.VariablesArguments): TPromise<DebugProtocol.VariablesResponse>;
evaluate(args: DebugProtocol.EvaluateArguments): TPromise<DebugProtocol.EvaluateResponse>;
Expand Down Expand Up @@ -133,6 +134,11 @@ export interface IThread extends ITreeElement {
*/
stoppedDetails: IRawStoppedDetails;

/**
* Information about the exception if an 'exception' stopped event raised and DA supports the 'exceptionInfo' request, otherwise null.
*/
exceptionInfo: TPromise<DebugProtocol.ExceptionInfoResponse>;

/**
* Gets the callstack if it has already been received from the debug
* adapter, otherwise it returns null.
Expand Down
12 changes: 12 additions & 0 deletions src/vs/workbench/parts/debug/common/debugModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,18 @@ export class Thread implements IThread {
});
}

/**
* Returns exception info promise if the exception was thrown and the debug adapter supports 'exceptionInfo' request, otherwise null
*/
public get exceptionInfo(): TPromise<DebugProtocol.ExceptionInfoResponse> {
const session = this.process.session;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love if the exception widget always calls this method.
And all the logic is in this method, meaning that if we are not stopped we return null, if the session does not supportExceptionInfoRequest we also return something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to introduce an interface on our side as a pair of adapter protocol exception interface - to get rid of the body and to handle the case when the session does not support exception request

if (session.capabilities.supportsExceptionInfoRequest && this.stoppedDetails && this.stoppedDetails.reason === 'exception') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like this check for this.stoppedDetails and this.stoppedDetails.reason === 'exception' since now we do it two times. Once before calling this method and once in the method.

We should do it in only one of those places - whichever feels better to you.
You just have to figure out what is the contract of this method and when should it be calle

return session.exceptionInfo({ threadId: this.threadId });
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can not just return null when a promise is expected, you should
return TPromise.as(null)

}

public next(): TPromise<any> {
return this.process.session.next({ threadId: this.threadId });
}
Expand Down
8 changes: 5 additions & 3 deletions src/vs/workbench/parts/debug/electron-browser/debugEditorContribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,16 +361,18 @@ export class DebugEditorContribution implements IDebugEditorContribution {
if (this.exceptionWidget && !sameUri) {
this.closeExceptionWidget();
} else if (sameUri && focusedSf.thread.stoppedDetails && focusedSf.thread.stoppedDetails.reason === 'exception') {
this.showExceptionWidget(exceptionSf.lineNumber, exceptionSf.column);
focusedSf.thread.exceptionInfo.then((exceptionInfo) => {
this.showExceptionWidget(exceptionInfo, exceptionSf.lineNumber, exceptionSf.column);
});
}
}

private showExceptionWidget(lineNumber: number, column: number): void {
private showExceptionWidget(exceptionInfo: DebugProtocol.ExceptionInfoResponse, lineNumber: number, column: number): void {
if (this.exceptionWidget) {
this.exceptionWidget.dispose();
}

this.exceptionWidget = this.instantiationService.createInstance(ExceptionWidget, this.editor, lineNumber);
this.exceptionWidget = this.instantiationService.createInstance(ExceptionWidget, this.editor, exceptionInfo, lineNumber);
this.exceptionWidget.show({ lineNumber, column }, 0);
}

Expand Down
4 changes: 4 additions & 0 deletions src/vs/workbench/parts/debug/electron-browser/rawDebugSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,10 @@ export class RawDebugSession extends v8.V8Protocol implements debug.ISession {
return this.send('stackTrace', args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

}

public exceptionInfo(args: DebugProtocol.ExceptionInfoArguments): TPromise<DebugProtocol.ExceptionInfoResponse> {
return this.send('exceptionInfo', args);
}

public scopes(args: DebugProtocol.ScopesArguments): TPromise<DebugProtocol.ScopesResponse> {
return this.send('scopes', args);
}
Expand Down
9 changes: 9 additions & 0 deletions src/vs/workbench/parts/debug/test/common/mockDebug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,15 @@ export class MockSession implements debug.ISession {
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

}

public exceptionInfo(args: DebugProtocol.ExceptionInfoArguments): TPromise<DebugProtocol.ExceptionInfoResponse> {
return TPromise.as({
body: {
exceptionId: 'mockExceptionId',
breakMode: 'unhandled'
}
});
}

public attach(args: DebugProtocol.AttachRequestArguments): TPromise<DebugProtocol.AttachResponse> {
return TPromise.as(null);
}
Expand Down