Uri Presentation Part 2, aka Cohost Html on-demand generation #10359
Conversation
…xtDocument version
....AspNetCore.Razor.LanguageServer/DocumentPresentation/TextDocumentUriPresentationEndpoint.cs
Outdated
Show resolved
Hide resolved
| // TODO: Eventually, for VS Code, the following piece of logic needs to make an LSP call rather than directly update the | ||
| // buffer, but the assembly this code currently lives in doesn't ship in VS Code, so we need to solve a few other things | ||
| // before we get there. |
There was a problem hiding this comment.
Does this need an issue filed for tracking?
There was a problem hiding this comment.
IMO, no, this is just a note to future explorers when bringing cohosting to VS Code. An item specifically for this would be overkill.
...anguageServices.Razor/LanguageClient/Cohost/HtmlDocumentSynchronizer.RazorDocumentVersion.cs
Outdated
Show resolved
Hide resolved
| ILoggerFactory loggerFactory) | ||
| : IHtmlDocumentSynchronizer | ||
| { | ||
| private static readonly Task<bool> s_falseTask = Task.FromResult(false); |
There was a problem hiding this comment.
Should there be a SpecializedTasks helper class like Roslyn's in Shared.Utilities?
https://sourceroslyn.io/#Microsoft.CodeAnalysis.Workspaces/SpecializedTasks.cs,25dbc38fd8ac0df3
There was a problem hiding this comment.
To be honest, I'm not sure why I even did this. I don't even expect the scenario where its used to be super common that it would show up on a trace. Having said that, creating finding all of the places a SpecializedTasks helper could be used sounds like a good thursday evening job :)
|
Note to self: Can't merge before the corresponding Roslyn insertion |
|
|
||
| namespace Microsoft.CodeAnalysis.Razor.DocumentPresentation; | ||
|
|
||
| internal static class UriPresentationHelper | ||
| { | ||
| public static Uri? GetComponentFileNameFromUriPresentationRequest(RazorLanguageKind languageKind, Uri[]? uris, ILogger logger) | ||
| public static Uri? GetComponentFileNameFromUriPresentationRequest(Uri[]? uris, ILogger logger) |
There was a problem hiding this comment.
Observation: (nit since it is an internal API anyway, but) This API no longer takes language kind and seems like it is now HTML-specific. Your comments are at the caller level where you've moved this logic out to, but at the API level here, it is not obvious for someone looking at it later to know this is actually now HTML-specific (class or API or implementation doesn't make it obvious).
There was a problem hiding this comment.
IMO it's quite the opposite. Except for the block of code I moved, the method has nothing to do with a specific language at all, its only job is to reason about the set of Uris passed in. What any caller chooses to do with the result is up to the caller. Thats why the language check was out of place.
Part of #9519
Commit at a time might be easier. Not really sure.
This PR finished off Uri Presentation in cohosting, but more importantly it brings html document generation to cohosting. The generation is done completely on-demand as we get a request for a document, and in general is much simpler than the current system, because whilst we still need to push the document to a VS buffer, we don't have to wait for the document to be pushed to VS from the LSP server, and synchronize those two pushes. This isn't doing any buffer management specifically for cohosting yet, but all of that is still needed for C# anyway, at the moment.