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.

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

Jump to bottom

Enable Project Context status item for Razor files #7333

Merged
merged 3 commits into from
Jul 12, 2024
Merged

Enable Project Context status item for Razor files #7333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47de8d4
Enable Project Context status item for Razor files
JoeRobich Jul 11, 2024
a11c42a
Change where files are opened in Razor integration tests
JoeRobich Jul 12, 2024
d79b0ca
Dispose the cancellation callbacks used in Razor document synchroniza…
JoeRobich Jul 12, 2024
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
9 changes: 5 additions & 4 deletions src/lsptoolshost/languageStatusBar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,13 @@ class WorkspaceStatus {

class ProjectContextStatus {
static createStatusItem(context: vscode.ExtensionContext, languageServer: RoslynLanguageServer) {
const documentSelector = combineDocumentSelectors(
languageServerOptions.documentSelector,
RazorLanguage.documentSelector
);
const projectContextService = languageServer._projectContextService;

const item = vscode.languages.createLanguageStatusItem(
'csharp.projectContextStatus',
languageServerOptions.documentSelector
);
const item = vscode.languages.createLanguageStatusItem('csharp.projectContextStatus', documentSelector);
item.name = vscode.l10n.t('C# Project Context Status');
item.detail = vscode.l10n.t('Active File Context');
context.subscriptions.push(item);
Expand Down
32 changes: 30 additions & 2 deletions src/lsptoolshost/services/projectContextService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import { TextDocumentIdentifier } from 'vscode-languageserver-protocol';
import { UriConverter } from '../uriConverter';
import { LanguageServerEvents } from '../languageServerEvents';
import { ServerState } from '../serverStateChange';
import { DynamicFileInfoHandler } from '../../razor/src/dynamicFile/dynamicFileInfoHandler';
import { ProvideDynamicFileResponse } from '../../razor/src/dynamicFile/provideDynamicFileResponse';
import { ProvideDynamicFileParams } from '../../razor/src/dynamicFile/provideDynamicFileParams';

export interface ProjectContextChangeEvent {
uri: vscode.Uri;
Expand Down Expand Up @@ -39,11 +42,22 @@ export class ProjectContextService {

public async refresh() {
const textEditor = vscode.window.activeTextEditor;
if (textEditor?.document?.languageId !== 'csharp') {
const languageId = textEditor?.document?.languageId;
if (languageId !== 'csharp' && languageId !== 'aspnetcorerazor') {
return;
}

const uri = textEditor.document.uri;
let uri = textEditor!.document.uri;

// If the active document is a Razor file, we need to map it back to a C# file.
if (languageId === 'aspnetcorerazor') {
const virtualUri = await this.getVirtualCSharpUri(uri);
if (!virtualUri) {
return;
}

uri = virtualUri;
}

// If we have an open request, cancel it.
this._source.cancel();
Expand All @@ -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>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@davidwengier davidwengier Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would get added to the document list with a wrong uri and either be fixed or show up in misc files

DynamicFileInfoHandler.provideDynamicFileInfoCommand,
new ProvideDynamicFileParams([uri.fsPath])
);

const responseUri = response.generatedFiles[0];
if (!responseUri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need an explicit check for null too?

vscode-csharp/src/razor/src/dynamicFile/dynamicFileInfoHandler.ts

Line 47 in d7439d3

virtualUris.push(null);

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 false-y check should cover null, undefined, and i think '' as well.

return undefined;
}

return UriConverter.deserialize(responseUri);
}

private async getProjectContexts(
uri: vscode.Uri,
token: vscode.CancellationToken
Expand Down
8 changes: 8 additions & 0 deletions src/razor/src/document/razorDocumentSynchronizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ export class RazorDocumentSynchronizer {
}

private removeSynchronization(context: SynchronizationContext) {
context.dispose();
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


const documentKey = getUriPath(context.projectedDocument.uri);
const synchronizations = this.synchronizations[documentKey];
clearTimeout(context.timeoutId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Consider moving this inside context.Dispose() now that it exists?

Copy link
Member Author

@JoeRobich JoeRobich Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since integration tests have been flaky for me, I will take your suggestions as a follow up and get this merged for snap.

Expand Down Expand Up @@ -167,6 +169,11 @@ export class RazorDocumentSynchronizer {
reject(reason);
}
},
dispose: () => {
while (rejectionsForCancel.length > 0) {
rejectionsForCancel.pop();
}
Comment on lines +173 to +175
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think rejectionsForCancel.length = 0 works too

},
projectedDocumentSynchronized,
onProjectedDocumentSynchronized,
projectedTextDocumentSynchronized,
Expand Down Expand Up @@ -271,4 +278,5 @@ interface SynchronizationContext {
readonly projectedTextDocumentSynchronized: () => void;
readonly onProjectedTextDocumentSynchronized: Promise<void>;
readonly cancel: (reason: string) => void;
readonly dispose: () => void;
}
7 changes: 5 additions & 2 deletions test/razorIntegrationTests/formatting.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import * as path from 'path';
import * as vscode from 'vscode';
import { describe, beforeAll, afterAll, test, expect } from '@jest/globals';
import { describe, beforeAll, afterAll, test, expect, beforeEach } from '@jest/globals';
import testAssetWorkspace from './testAssets/testAssetWorkspace';
import * as integrationHelpers from '../integrationTests/integrationHelpers';

Expand All @@ -20,10 +20,13 @@ describe(`Razor Formatting ${testAssetWorkspace.description}`, function () {
const htmlConfig = vscode.workspace.getConfiguration('html');
await htmlConfig.update('format.enable', true);

await integrationHelpers.openFileInWorkspaceAsync(path.join('Pages', 'BadlyFormatted.razor'));
await integrationHelpers.activateCSharpExtension();
});

beforeEach(async function () {
await integrationHelpers.openFileInWorkspaceAsync(path.join('Pages', 'BadlyFormatted.razor'));
});

afterAll(async () => {
await testAssetWorkspace.cleanupWorkspace();
});
Expand Down
7 changes: 5 additions & 2 deletions test/razorIntegrationTests/hover.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import * as path from 'path';
import * as vscode from 'vscode';
import { describe, beforeAll, afterAll, test, expect } from '@jest/globals';
import { describe, beforeAll, afterAll, test, expect, beforeEach } from '@jest/globals';
import testAssetWorkspace from './testAssets/testAssetWorkspace';
import * as integrationHelpers from '../integrationTests/integrationHelpers';

Expand All @@ -15,10 +15,13 @@ describe(`Razor Hover ${testAssetWorkspace.description}`, function () {
return;
}

await integrationHelpers.openFileInWorkspaceAsync(path.join('Pages', 'Index.cshtml'));
await integrationHelpers.activateCSharpExtension();
});

beforeEach(async function () {
await integrationHelpers.openFileInWorkspaceAsync(path.join('Pages', 'Index.cshtml'));
});

afterAll(async () => {
await testAssetWorkspace.cleanupWorkspace();
});
Expand Down
Loading