Not supplying evaluateName does not hide "Add to Watch"

[DanTup]

According to #25166:

If "evaluateName" is falsy, all actions relying on it should not be available (e.g. "Add to watch").

However, the context menu still seems to show Add to Watch in the Variables display, and when clicking it it adds an empty expression to the watch window.

It's not a bad thing - I never implemented evaluateName (oops) since I incorrectly expected it to fall back to name (I'm fixing that now though!).

[isidorn]

If there is no evaluateName we use the name of the variable. Which I think is good behavior for most debug adapters.

I understand this is not conforming with the docs. So we should either

• Change the docs
• Change the actions to not be available (or disabled) when evaluate name is not there

Currently I think that using the name when evaluateName is missing might not be a bad strategy, but am overall open for suggestions.
@weinand thoughts?

[DanTup]

I'd be fine with it falling back to name, but it doesn't seem to do that today as when you click "Add to watch" you get an empty expression in the watch window (whereas if you provide evaluateName it prefills with that).

So, if the docs are going to be changed (and the actions are not removed), I think it's a bug that "Add to watch" isn't filling in the name (it might not be the only thing that might not be falling back to name).

[isidorn]

@DanTup It is very strange that it is not falling back to name for you
https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/browser/debugActions.ts#L608

Can you put a breakpoint there and investigate deeper what is going on

[DanTup]

Hmm, that is strange - I'll do some investigation. When it didn't work, I found #25166 and assumed it was intended that evaluateName not being provided would hide/remove this functionality. In my testing, it definitely seems to add empty watches without evaluateName and the correct name with it.

I'll dig more and post back next week though!

[DanTup]

@isidorn Ok, I looked at this code again:

const name = this.expression instanceof Variable ? this.expression.evaluateName : this.expression.name;

This is not falling back to name because this.expression is an instance of Variable, it just doesn't have an evaluateName. Therefore the code above takes the undefined of this.expression.evaluateName.

Shouldn't it be something like:

const name = this.expression instanceof Variable && this.expression.evaluateName ? this.expression.evaluateName : this.expression.name;

?

[DanTup]

Also; I still think the behaviour mentioned in the linked comment at the top would be good - what happens if we fabricate a variable for the Variables window (eg. an exception) and don't have a good way to add it to the Watch window?

[isidorn]

@DanTup makes sense, thanks for feedback
I have pushed a change that now actions depedning on evaluateName are disabled when evaluateName is not there.

[DanTup]

Super, thanks! 👍

[DanTup]

Checked in Insiders, seems to work as expected. Thanks!