Enable Project Context status item for Razor files #7333
Conversation
| @@ -58,6 +72,20 @@ export class ProjectContextService { | |||
| this._contextChangeEmitter.fire({ uri, context }); | |||
| } | |||
|
|
|||
| private async getVirtualCSharpUri(uri: vscode.Uri): Promise<vscode.Uri | undefined> { | |||
| const response = await vscode.commands.executeCommand<ProvideDynamicFileResponse>( | |||
There was a problem hiding this comment.
@davidwengier I'm not sure if there will be any problem using this endpoint or if there's something else that should be used to find the dynamic file URI
There was a problem hiding this comment.
I don't think it would cause any issues. In theory it could trigger Razor activation, but if this only gets called when a Razor document is open in the editor, that either has happened, or is about to anyway.
I guess the only risk here is that the uri in this method is different from the one Roslyn sends us, but points to the same physical file. Not sure what would happen in that case
There was a problem hiding this comment.
I think it would get added to the document list with a wrong uri and either be fixed or show up in misc files
| @@ -58,6 +72,20 @@ export class ProjectContextService { | |||
| this._contextChangeEmitter.fire({ uri, context }); | |||
| } | |||
|
|
|||
| private async getVirtualCSharpUri(uri: vscode.Uri): Promise<vscode.Uri | undefined> { | |||
| const response = await vscode.commands.executeCommand<ProvideDynamicFileResponse>( | |||
There was a problem hiding this comment.
I don't think it would cause any issues. In theory it could trigger Razor activation, but if this only gets called when a Razor document is open in the editor, that either has happened, or is about to anyway.
I guess the only risk here is that the uri in this method is different from the one Roslyn sends us, but points to the same physical file. Not sure what would happen in that case
| ); | ||
|
|
||
| const responseUri = response.generatedFiles[0]; | ||
| if (!responseUri) { |
There was a problem hiding this comment.
Does this need an explicit check for null too?
There was a problem hiding this comment.
The false-y check should cover null, undefined, and i think '' as well.
| @@ -119,6 +119,8 @@ export class RazorDocumentSynchronizer { | |||
| } | |||
|
|
|||
| private removeSynchronization(context: SynchronizationContext) { | |||
| context.dispose(); | |||
There was a problem hiding this comment.
@davidwengier Does this seem right? What I was seeing in CI and locally was that the document and text document were up to date. So the context was removed. The token then got cancelled and the callbacks were rejected. This created an unhandled exception. I figure if we are removing the context, then we don't care about cancellation and can remove the callbacks.
There was a problem hiding this comment.
Not sure I see why the token would get cancelled, but clearing the rejections like this doesn't seem like it would be an issue.
JoeRobich
dismissed
davidwengier’s stale review
Could I get another review for the razorDocumentSynchronizer.ts changes?
| @@ -119,6 +119,8 @@ export class RazorDocumentSynchronizer { | |||
| } | |||
|
|
|||
| private removeSynchronization(context: SynchronizationContext) { | |||
| context.dispose(); | |||
There was a problem hiding this comment.
Not sure I see why the token would get cancelled, but clearing the rejections like this doesn't seem like it would be an issue.
| @@ -119,6 +119,8 @@ export class RazorDocumentSynchronizer { | |||
| } | |||
|
|
|||
| private removeSynchronization(context: SynchronizationContext) { | |||
| context.dispose(); | |||
|
|
|||
| const documentKey = getUriPath(context.projectedDocument.uri); | |||
| const synchronizations = this.synchronizations[documentKey]; | |||
| clearTimeout(context.timeoutId); | |||
There was a problem hiding this comment.
Nit: Consider moving this inside context.Dispose() now that it exists?
There was a problem hiding this comment.
Since integration tests have been flaky for me, I will take your suggestions as a follow up and get this merged for snap.
| while (rejectionsForCancel.length > 0) { | ||
| rejectionsForCancel.pop(); | ||
| } |
There was a problem hiding this comment.
Nit: I think rejectionsForCancel.length = 0 works too
Requests the virtual C# file path and uses that to get project context information.
When used with DevKit:
