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

Failure message from SetVariable request is not surfaced #80464

Closed
gjsjohnmurray opened this issue Sep 6, 2019 · 9 comments

Comments

@gjsjohnmurray
Copy link
Contributor

commented Sep 6, 2019

Similar to #80452 but for the SetVariableRequest. The target environment of my debugger can only change a variable value at the stack level of the execution point, so I want to notify a user if they try to make a change when they have selected a different stack level.

VS Code version: Code 1.38.0 (3db7e09, 2019-09-03T21:49:13.459Z)
OS version: Windows_NT x64 10.0.17134

@vscodebot vscodebot bot added the new release label Sep 6, 2019
@gjsjohnmurray gjsjohnmurray changed the title Surface failure message from SetVariable request Failure message from SetVariable request is not surfaced Sep 6, 2019
@vscodebot vscodebot bot removed the new release label Sep 8, 2019
@roblourens roblourens assigned isidorn and unassigned roblourens Sep 10, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I think you can just throw an error in setVariable and it should hopefully be nicely handled here
Did you try that? Does that not work?

@gjsjohnmurray

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

I don't think I tried throwing an error. I expected it to respect the following in my DebugProtocol.SetVariableResponse reply:
{success: false, message: 'Unable to set that value', ... }

`

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Unfortunetely I do not yet respect the success field. For now I suggest you try throwing an error in this case.
In the future I might handle this case better

@gjsjohnmurray

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@isidorn I just tested this with the mock-debug sample but it didn't work.

In initializeRequest I added response.body.supportsSetVariable = true;

I added this:

    protected async setVariableRequest(response: DebugProtocol.SetVariableResponse, args: DebugProtocol.SetVariableArguments): Promise<void> {

		response.success = false;
		response.message = 'Not yet implemented';

		// Workaround for https://github.com/microsoft/vscode/issues/80464#issuecomment-530713561
		if (!response.success) {
			throw new Error(response.message);
		}

		this.sendResponse(response);
	}

I used the debugger to verify that my code was called when I tried changing a variable. But the error I throw didn't surface anywhere that I noticed.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Can you please push your changes to a branch of mock debug so I simply clone it and try it out. Thanks a lot!

gjsjohnmurray added a commit to gjsjohnmurray/vscode-mock-debug that referenced this issue Sep 13, 2019
@gjsjohnmurray

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

@isidorn I have prepared a branch at https://github.com/gjsjohnmurray/vscode-mock-debug/tree/show-80464 in which I always throw an error here in setVariableRequest, but don't see my message surface in the UI.

@isidorn isidorn added bug and removed needs more info labels Sep 20, 2019
@isidorn isidorn closed this in ff570b5 Sep 20, 2019
@isidorn isidorn added this to the September 2019 milestone Sep 20, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

Great find, thanks for providing nice repro steps.

First of all, I was wrong you can not just throw an error from the debug adapter, you always have to return a response. Just the response.success has to be false - as you were originally doing.

Now I fixed in issue on the VSCode side that we were not showing an error returned. Now we keep the input box shown and the user can see the error.
Please try it out in Monday's vscode insiders and let us know if the issue is nicely fixed for you.
Thanks!

Screenshot 2019-09-20 at 11 14 39

@gjsjohnmurray

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@isidorn thanks for this. I have verified it.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Thank you, adding verified label.

@isidorn isidorn added the verified label Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.