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

Don't compute the information for the code definition window if not open #57307

Conversation

jasonmalinowski
Copy link
Member

If the symbol your caret is over is from metadata, producing the output might be really expensive, so let's not do it in that case. The window clears anything when it's opened, so the user won't know it was never doing anything when closed.

@jasonmalinowski jasonmalinowski requested a review from a team as a code owner October 21, 2021 19:42
@jasonmalinowski jasonmalinowski force-pushed the do-not-run-code-definition-window-unless-open branch from a2c4ff0 to 8408670 Compare October 21, 2021 19:43
@RikkiGibson
Copy link
Contributor

@jasonmalinowski are we ready to enable auto-merge on this?

If the symbol your caret is over is from metadata, producing the
output might be really expensive, so let's not do it in that case. The
window clears anything when it's opened, so the user won't know it was
never doing anything when closed.
@jasonmalinowski jasonmalinowski force-pushed the do-not-run-code-definition-window-unless-open branch from f7625d0 to e3f5ffc Compare October 21, 2021 21:24
@jasonmalinowski jasonmalinowski self-assigned this Oct 21, 2021
@jasonmalinowski
Copy link
Member Author

@RikkiGibson Now set.

var vsCodeDefView = await GetVsCodeDefViewAsync().ConfigureAwait(true);

// Switch to the UI thread before using the IVsCodeDefView service
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
Copy link
Member

@sharwell sharwell Oct 21, 2021

Choose a reason for hiding this comment

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

💡 This method will be more efficient if this call is moved to the start of the method (removes two thread context switches that would occur if the caller is on a background thread and the window is not already initialized, and no penalty for the other cases).

@@ -40,7 +67,7 @@ public async Task SetContextAsync(ImmutableArray<CodeDefinitionWindowLocation> l
return;
}

var vsCodeDefView = await _asyncServiceProvider.GetServiceAsync<SVsCodeDefView, IVsCodeDefView>().ConfigureAwait(false);
var vsCodeDefView = await GetVsCodeDefViewAsync().ConfigureAwait(true);

// Switch to the UI thread before using the IVsCodeDefView service
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

📝 This should also be moved up to before the call to GetServiceAsync (same reason)

// so the user won't notice we weren't doing anything when it was open.
if (!await _codeDefinitionWindowService.IsWindowOpenAsync(cancellationToken).ConfigureAwait(false))
{
return;
Copy link
Member

Choose a reason for hiding this comment

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

❔ How much penalty is there for calling SetContextAsync with an empty set of locations for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be the goal of doing that? Its entirely possible the service will throw away the call, I'm not certain.

@jasonmalinowski jasonmalinowski merged commit db85ae1 into dotnet:main Oct 22, 2021
@ghost ghost added this to the Next milestone Oct 22, 2021
@RikkiGibson RikkiGibson modified the milestones: Next, 17.1.P1 Oct 25, 2021
@jasonmalinowski jasonmalinowski deleted the do-not-run-code-definition-window-unless-open branch October 27, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants