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

Include tooltip to Razor provisional completion #7440

Merged
merged 7 commits into from
Aug 19, 2024

Conversation

jordi1215
Copy link
Member

@jordi1215 jordi1215 commented Aug 12, 2024

This resolves #7250. The cause was that when we added a provisional dot to the generated virtual csharp document, the change didn't get reflected on disk before the Roslyn server took the completion request.

Changes made in completionHandler.ts:

  1. Made changes so that both provisional and non-provisional code completion uses the same provideCompletionsCommand (roslyn.provideCompletions).
  2. On top of adding a temporary provisional dot "." after each provisional completion request, we are adding a "resolve" provisional dot for each resolve completion request. The dot is essential for the resolve request to be correct and therefor for the tooltip to show up.
  3. Use vscode.workspace.onDidChangeTextDocument to ensure that the virtual documents get updated before sending the request to the roslyn server for both completion and resolve completion requests.

Before

image

What could have gone wrong if it wasn't for David's comment

Screen.Recording.2024-08-14.at.3.23.43.PM.mov

After

image
Screen.Recording.2024-08-15.at.9.45.26.AM.mov

@jordi1215 jordi1215 requested a review from a team as a code owner August 12, 2024 23:55
@jordi1215 jordi1215 changed the title Dev/jordi1215/fix provisional completion Fix Razor provisional completion Aug 12, 2024
@jordi1215 jordi1215 changed the title Fix Razor provisional completion Include Tooltip to Razor provisional completion Aug 12, 2024
@jordi1215 jordi1215 changed the title Include Tooltip to Razor provisional completion Include tooltip to Razor provisional completion Aug 12, 2024
@davidwengier
Copy link
Contributor

davidwengier commented Aug 13, 2024

The documentation gets synced afterwards

This is only if the completion is actually taken, right? Do things still work correctly otherwise? eg, type DateTime., press ESC to dismiss completion, and then scroll the text into view and out of view, or hover over something etc. I would expect not cleaning up the . would cause disco colors etc.

TL;DR, If the Razor document doesn't change after typing the dot, presumably the generated document never gets re-synced

@jordi1215
Copy link
Member Author

jordi1215 commented Aug 13, 2024

The documentation gets synced afterwards

This is only if the completion is actually taken, right? Do things still work correctly otherwise? eg, type DateTime., press ESC to dismiss completion, and then scroll the text into view and out of view, or hover over something etc. I would expect not cleaning up the . would cause disco colors etc.

TL;DR, If the Razor document doesn't change after typing the dot, presumably the generated document never gets re-synced

Thanks for the heads up! I did manage to get it into a "bad" state. Assuming var name="Jordi", @name. will trigger completion. If nothing gets accepted then C# will think it is an incomplete statement and give the red squiggles whereas HTML will interpret it as Jordi..

I think I will try add and remove the provisional dot for both completion and resolveCompletion command.

Thanks so much!

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I agree with Andrew, it would be much better if we can hook the didChange rather than use a delay. The rest makes sense, though I must admit I find this whole area confusing and hard to reason about. Could just be my TypeScript skills :)

@@ -80,6 +82,7 @@ export class CSharpProjectedDocument implements IProjectedDocument {
if (this.provisionalEditAt && this.preProvisionalContent) {
// Undo provisional edit if one was applied.
this.setContent(this.preProvisionalContent);
this.resolveProvisionalEditAt = this.provisionalEditAt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this field should be cleared in addProvisionalDotAt, just to ensure the previous resolve index doesn't stick around?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here is right now the resolve completion request doesn't know whether the completion request was provisional or not. So one bad state could be:
-> (Provisional completion) user triggers provisional completion
-> (resolve completion) provisional dot are added and removed as the user toggles completion list
-> (regular completion) as the user continues to type, completion list shows up
-> (resolve completion) provisional dot are added and removed still since the field is not cleared

src/razor/src/csharp/csharpProjectedDocument.ts Outdated Show resolved Hide resolved
@jordi1215 jordi1215 enabled auto-merge (squash) August 19, 2024 13:48
@jordi1215 jordi1215 merged commit a8a9c24 into main Aug 19, 2024
13 checks passed
@JoeRobich JoeRobich deleted the dev/jordi1215/fix-provisional-completion branch October 8, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provisional Completion in Razor should show full tooltip info
3 participants