use global options for async completion #32045
Conversation
|
Corresponding PRs in Visual Studio, FYI |
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.Completion | ||
| { | ||
| [Export(typeof(EditorOptionDefinition))] |
There was a problem hiding this comment.
I'm not sure if it's appropriate to export the option definition twice.
I'm exporting the definition in my PR so that I can write the value.
However, to read it, we identify it by string UseAsyncCompletion without the need for the definition. #Resolved
|
@AmadeusW please re-review |
| @@ -85,12 +85,30 @@ public bool TryGetController(ITextView textView, ITextBuffer subjectBuffer, out | |||
|
|
|||
| private bool UseLegacyCompletion(ITextView textView, ITextBuffer subjectBuffer) | |||
| { | |||
| const string useAsyncCompletionOptionName = "UseAsyncCompletion"; | |||
There was a problem hiding this comment.
Consider putting inside the "if", so it's easier to scan structure first. #Resolved
There was a problem hiding this comment.
|
tagging @jinujoseph for approval |
| else if (userSetting == -1) | ||
| { | ||
| _newCompletionAPIEnabled = false; | ||
| } |
There was a problem hiding this comment.
i don't really understand where teh 1 and -1 values came from... How do you know it will be those values. Do we have code elsewhere that sets these? If so, can we define constants somewhere that we can share between the reading/writing code? #Resolved
There was a problem hiding this comment.
Good catch! Thank you, @CyrusNajmabadi! Let me add a comment #Resolved
There was a problem hiding this comment.
It corresponds to values of a three-state checkbox #Resolved
There was a problem hiding this comment.
and it is an convention used in VS settings #Resolved
| if (Workspace.TryGetWorkspace(subjectBuffer.AsTextContainer(), out var workspace)) | ||
| { | ||
| var experimentationService = workspace.Services.GetService<IExperimentationService>(); | ||
| _newCompletionAPIEnabled = experimentationService.IsExperimentEnabled(WellKnownExperimentNames.CompletionAPI); |
There was a problem hiding this comment.
if there is a chance of the if statement failing, consider setting _newCompletionAPIEnabled to something so that we avoid repeating work. If this won't fail, though, don't worry about it. #Resolved
There was a problem hiding this comment.
As soon as we get a workspace and an experimentation service, we should set _newCompletionAPIEnabled to a value. We should try checking unit this. Not?
In reply to: 245185354 [](ancestors = 245185354)
Tagging @AmadeusW for review as well