Fix Razor completion tooltip documentation #5839
Conversation
| } | ||
| } | ||
|
|
||
| (<CompletionItem>completionItem).data = data; |
There was a problem hiding this comment.
Does this override the data with the default in all cases, even if the completion item has provided its own?
There was a problem hiding this comment.
afaik we shouldn't be modifying this
There was a problem hiding this comment.
We do need to explicitly set the Data property on the completion item in the case where C# returns to us a Data property that is only tied to the completion list.
Does this override the data with the default in all cases, even if the completion item has provided its own?
Ah, good point - we shouldn't override if the completion item has provided its own Data property. Will push a change to fix this
|
|
||
| // The documentation object Roslyn returns can have a different | ||
| // shape than what the client expects, so we do a conversion here. | ||
| const markdownString = <vscode.MarkdownString>(item.documentation); |
There was a problem hiding this comment.
This is weird... and probably against my understanding of typescript. We're casting to vscode.MarkdownString to make a new one? Does something magic happen in the constructor?
There was a problem hiding this comment.
Typescript seems to be weird in that we can have a vscode.MarkdownString but the properties on the type may not be what is expected.
For example, at the beginning of this method we get passed something that technically passes as a vscode.MarkdownString, but has totally different properties:
After the cast above and the creation of a new MarkdownString, we're able to return the shape VS Code expects:
There was a problem hiding this comment.
I suspect this is actually wrong, but not in a way that makes a difference. I would guess that Roslyn LSP is returning the LSP type MarkupContent, which has a kind and a value. This code is casting it to a vscode.MarkdownString which also has a value, so it can read the value property without a compilation error. Since types, and therefore casts, don't really exist in the compiled JavaScript this works fine, as the property names we care about line up. The creation of the new markdown string is then really a conversion from the LSP type to the VS Code type.
Since the LSP documentation property is string | MarkupContent, the real fix is probably to do a type check and see if it is MarkupContent, and if so convert to MarkdownString. If it's string it can be left alone, as the VS Code API is string | MarkdownString, but item is typed as vscode.CompletionItem and not the LSP type, so I've no idea. Maybe that is wrong too?
There was a problem hiding this comment.
Appreciate the insight! That would make sense actually. I pushed a change that checks for MarkupContent instead, it's a little hacky since we have to convert to unknown first, but it's probably better than what was there before.
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
I think there are probably issues with the types used in this provider as a whole, but the completion resolve stuff was always a hack, so happy that this fixes a bug and moves us forward.
3 participants
Razor completion tooltips were previously missing for C# items and HTML snippets.
The tooltips are sometimes cut off (see screenshot), but this is a problem with C# files as well and is likely an editor issue.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1831344/
Before:
After:
