Skip to content

Fix Razor completion tooltip documentation #5839

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

Merged
allisonchou merged 4 commits into from
Jun 21, 2023
Merged

Fix Razor completion tooltip documentation #5839

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
87e4638
Fix Razor completion documentation
allisonchou Jun 21, 2023
40ac682
Review feedback
allisonchou Jun 21, 2023
19fc69d
Review comments
allisonchou Jun 21, 2023
efcdbf2
Review comments
allisonchou Jun 21, 2023
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
26 changes: 18 additions & 8 deletions src/razor/src/Completion/RazorCompletionItemProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { getUriPath } from '../UriPaths';
import { ProvisionalCompletionOrchestrator } from './ProvisionalCompletionOrchestrator';
import { LanguageKind } from '../RPC/LanguageKind';
import { RoslynLanguageServer } from '../../../lsptoolshost/roslynLanguageServer';
import { CompletionItem, CompletionParams, CompletionTriggerKind } from 'vscode-languageclient';
import { CompletionItem, CompletionList, CompletionParams, CompletionTriggerKind } from 'vscode-languageclient';
import { UriConverter } from '../../../lsptoolshost/uriConverter';
import * as RazorConventions from '../RazorConventions';
import { MappingHelpers } from '../Mapping/MappingHelpers';
Expand All @@ -35,11 +35,6 @@ export class RazorCompletionItemProvider

let completions: vscode.CompletionList | vscode.CompletionItem[];

// For CSharp, completions need to keep the "data" field
// on the completion item for lazily resolving the edits in
// the resolveCompletionItem step. Using the vs code command
// drops that field because it doesn't exist in the declared vs code
// CompletionItem type.
if (language === LanguageKind.CSharp) {
const params: CompletionParams = {
context: {
Expand Down Expand Up @@ -71,6 +66,13 @@ export class RazorCompletionItemProvider
completions instanceof Array ? completions // was vscode.CompletionItem[]
: completions ? completions.items // was vscode.CompletionList
: [];

// For CSharp, completions need to keep the "data" field
Comment thread
allisonchou marked this conversation as resolved.
Outdated
// on the completion item for lazily resolving the edits in
// the resolveCompletionItem step. Using the vs code command
// drops that field because it doesn't exist in the declared vs code
// CompletionItem type.
const data = (<CompletionList>completions)?.itemDefaults?.data;

// There are times when the generated code will not line up with the content of the .razor/.cshtml file.
// Therefore, we need to offset all completion items' characters by a certain amount in order
Expand All @@ -79,7 +81,7 @@ export class RazorCompletionItemProvider
const completionCharacterOffset = projectedPosition.character - hostDocumentPosition.character;
for (const completionItem of completionItems) {
const doc = completionItem.documentation as vscode.MarkdownString;
if (doc) {
if (doc && doc.value) {
// Without this, the documentation doesn't get rendered in the editor.
const newDoc = new vscode.MarkdownString(doc.value);
newDoc.isTrusted = doc.isTrusted;
Expand Down Expand Up @@ -128,6 +130,8 @@ export class RazorCompletionItemProvider
completionItem.insertText = intellicodeCompletion.textEditText;
}
}

(<CompletionItem>completionItem).data = data;

@davidwengier davidwengier Jun 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this override the data with the default in all cases, even if the completion item has provided its own?

@ryzngard ryzngard Jun 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

afaik we shouldn't be modifying this

@allisonchou allisonchou Jun 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

}

const isIncomplete = completions instanceof Array ? false
Expand Down Expand Up @@ -204,6 +208,13 @@ export class RazorCompletionItemProvider

item = newItem;

// 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);

@ryzngard ryzngard Jun 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@allisonchou allisonchou Jun 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:
image

After the cast above and the creation of a new MarkdownString, we're able to return the shape VS Code expects:
image

@davidwengier davidwengier Jun 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@allisonchou allisonchou Jun 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

if (markdownString && markdownString.value) {
item.documentation = new vscode.MarkdownString(markdownString.value);
}

if (item.command && item.command.arguments?.length === 4) {
let uri = vscode.Uri.parse(item.command.arguments[0]);

Expand Down Expand Up @@ -238,4 +249,3 @@ function getTriggerKind(triggerKind: vscode.CompletionTriggerKind): CompletionTr

}
}