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

Add support for async lightbulb model. #53117

Merged
merged 68 commits into from
Jun 29, 2021
Merged

Add support for async lightbulb model. #53117

merged 68 commits into from
Jun 29, 2021

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 3, 2021 •
edited
Loading

Examples of this in action:


Note: the delays are exaggerated here to see the impact.

CyrusNajmabadi
CyrusNajmabadi commented May 3, 2021
View reviewed changes
src/Features/Core/Portable/CodeFixes/CodeFixService.cs
@@ -392,6 +398,9 @@ private bool TryGetWorkspaceFixersPriorityMap(Document document, [NotNullWhen(tr
{
cancellationToken.ThrowIfCancellationRequested();

if (highPriority != null && highPriority != fixer.IsHighPriority)
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

code fixing, refactoring, and analyzers all effectively have a way to say: if you want high-pri or normal-pri, i'll limit to that set.

mavasani
mavasani reviewed May 4, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall, this looks great and matches the implementation I had in mind. Just moving the filtering a bit further down should make the behavior common across all IDE document diagnostic computation code paths.

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request May 26, 2021
Closed
davidwengier
davidwengier approved these changes May 27, 2021
View reviewed changes
...ore/Analyzers/RemoveUnnecessaryImports/AbstractRemoveUnnecessaryImportsDiagnosticAnalyzer.cs
@@ -117,6 +118,7 @@ public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
}
}

public CodeActionRequestPriority RequestPriority => CodeActionRequestPriority.Normal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not to get into a bikeshedding argument here, but whats the thinking around whether there are high or normal? For example "Remove unused usings" only triggers from one spot in a document so would seem to me to be a decent candidate for high priority.

Perhaps its worth documenting some guidelines on the enum (if they're not already there.. haven't got that far yet)

Copy link
Member Author

Choose a reason for hiding this comment

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

Effectively, everything that previously produced a 'high pri code action' is in the 'high pri request' category. Anything that produces a normal/low pri code action is in the normal request category. I didn't want to change anything in that regard here. Right now we've had a long historical precedent of recognizing two code actions (one analzyer, one refactoring) that are so important that they should beat out everything else. This PR codifies things so that not only do those come first, they are computed first so we can display them asap for the user.

...orFeatures/Core.Wpf/Suggestions/SuggestedActionSource_ISuggestedActionsSourceExperimental.cs Outdated Show resolved Hide resolved
...orFeatures/Core.Wpf/Suggestions/SuggestedActionSource_ISuggestedActionsSourceExperimental.cs Outdated Show resolved Hide resolved
...orFeatures/Core.Wpf/Suggestions/SuggestedActionSource_ISuggestedActionsSourceExperimental.cs Outdated
ISuggestedActionCategorySet requestedActionCategories,
Document document,
SnapshotSpan range,
Func<string, IDisposable?> addOperationScope,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed? It's always _ => 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.

sure!

...orFeatures/Core.Wpf/Suggestions/SuggestedActionSource_ISuggestedActionsSourceExperimental.cs Outdated Show resolved Hide resolved
@CyrusNajmabadi CyrusNajmabadi merged commit 0d317a4 into dotnet:main-vs-deps Jun 29, 2021
@ghost ghost added this to the Next milestone Jun 29, 2021
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
@RikkiGibson RikkiGibson mentioned this pull request Jul 1, 2021
Closed
CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jul 12, 2021
@CyrusNajmabadi
Revert "Merge pull request dotnet#53117 from CyrusNajmabadi/asyncLigh…
e9b1a09 
…tbulb"

This reverts commit 0d317a4, reversing
changes made to 5336a84.
JoeRobich added a commit that referenced this pull request Jul 12, 2021
@JoeRobich
Merge pull request #54773 from CyrusNajmabadi/reversions
5f7ddf6 
Revert "Merge pull request #53117 from CyrusNajmabadi/asyncLightbulb"
@CyrusNajmabadi CyrusNajmabadi deleted the asyncLightbulb branch September 30, 2021 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alrz alrz alrz left review comments

@davidwengier davidwengier davidwengier approved these changes

@KirillOsenkov KirillOsenkov KirillOsenkov left review comments

@ryzngard ryzngard ryzngard left review comments

@Therzok Therzok Therzok left review comments

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@mavasani mavasani Awaiting requested review from mavasani

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.0.P2
Development

Successfully merging this pull request may close these issues.
9 participants
@CyrusNajmabadi @ryzngard @Therzok @jasonmalinowski @KirillOsenkov @davidwengier @alrz @mavasani @RikkiGibson