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

use global options for async completion #32045

Merged
merged 4 commits into from
Jan 4, 2019
Merged

use global options for async completion #32045

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef538f4
use global options for async completion
ivanbasov Dec 28, 2018
1bdb788
code review feedback
ivanbasov Jan 2, 2019
fea1904
code review feedback
ivanbasov Jan 4, 2019
8c0e95d
Update AsyncCompletionService.cs
ivanbasov Jan 4, 2019
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
23 changes: 20 additions & 3 deletions src/EditorFeatures/Core/Implementation/IntelliSense/Completion/AsyncCompletionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,27 @@ private bool UseLegacyCompletion(ITextView textView, ITextBuffer subjectBuffer)
{
if (!_newCompletionAPIEnabled.HasValue)
{
if (Workspace.TryGetWorkspace(subjectBuffer.AsTextContainer(), out var workspace))
int userSetting = 0;
if (textView.Options.GlobalOptions.IsOptionDefined(UseAsyncCompletionOptionDefinition.OptionName, localScopeOnly: false))
{
var experimentationService = workspace.Services.GetService<IExperimentationService>();
_newCompletionAPIEnabled = experimentationService.IsExperimentEnabled(WellKnownExperimentNames.CompletionAPI);
userSetting = textView.Options.GlobalOptions.GetOptionValue<int>(UseAsyncCompletionOptionDefinition.OptionName);
}

if (userSetting == 1)
{
_newCompletionAPIEnabled = true;
}
else if (userSetting == -1)
{
_newCompletionAPIEnabled = false;
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 4, 2019 •
edited by ivanbasov
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ivanbasov ivanbasov Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! Thank you, @CyrusNajmabadi! Let me add a comment #Resolved

Copy link
Contributor

@AmadeusW AmadeusW Jan 4, 2019 •
edited by ivanbasov
Loading

Choose a reason for hiding this comment

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

It corresponds to values of a three-state checkbox #Resolved

Copy link
Contributor

@AmadeusW AmadeusW Jan 4, 2019 •
edited by ivanbasov
Loading

Choose a reason for hiding this comment

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

and it is an convention used in VS settings #Resolved

else
{
if (Workspace.TryGetWorkspace(subjectBuffer.AsTextContainer(), out var workspace))
{
var experimentationService = workspace.Services.GetService<IExperimentationService>();
_newCompletionAPIEnabled = experimentationService.IsExperimentEnabled(WellKnownExperimentNames.CompletionAPI);
Copy link
Contributor

@AmadeusW AmadeusW Jan 4, 2019 •
edited by ivanbasov
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

}
}
}

Expand Down
26 changes: 26 additions & 0 deletions ...eatures/Core/Implementation/IntelliSense/Completion/UseAsyncCompletionOptionDefinition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.using System;

using System;
using System.ComponentModel.Composition;
using Microsoft.VisualStudio.Text.Editor;

namespace Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.Completion
{
[Export(typeof(EditorOptionDefinition))]
Copy link
Contributor

@AmadeusW AmadeusW Dec 28, 2018 •
edited by ivanbasov
Loading

Choose a reason for hiding this comment

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

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

internal class UseAsyncCompletionOptionDefinition : EditorOptionDefinition
{
public const string OptionName = "UseAsyncCompletion";

/// <summary>
/// The meaning of this option definition's values:
/// -1 - user disabled async completion
/// 0 - no changes from the user; check the experimentation service for whether to use async completion
/// 1 - user enabled async completion
/// </summary>
public override object DefaultValue => 0;

public override Type ValueType => typeof(int);

public override string Name => OptionName;
}
}