-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Extract factory interface for AbstractSnippetExpansionClient #71752
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
Changes from 3 commits
68b70a2
fc0cda0
1e298dc
89ff162
5a9d9c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Completion; | ||
| using Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.SignatureHelp; | ||
| using Microsoft.CodeAnalysis.Editor.Shared.Utilities; | ||
| using Microsoft.CodeAnalysis.Host.Mef; | ||
| using Microsoft.CodeAnalysis.Options; | ||
| using Microsoft.VisualStudio.Editor; | ||
| using Microsoft.VisualStudio.LanguageServices.Implementation.Snippets; | ||
| using Microsoft.VisualStudio.Text; | ||
| using Microsoft.VisualStudio.Text.Editor; | ||
| using Microsoft.VisualStudio.Text.Editor.Commanding; | ||
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.CSharp.Snippets; | ||
|
|
||
| [ExportLanguageService(typeof(ISnippetExpansionClientFactory), LanguageNames.CSharp)] | ||
| [Shared] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class CSharpSnippetExpansionClientFactory( | ||
| IThreadingContext threadingContext, | ||
| SignatureHelpControllerProvider signatureHelpControllerProvider, | ||
| IEditorCommandHandlerServiceFactory editorCommandHandlerServiceFactory, | ||
| IVsEditorAdaptersFactoryService editorAdaptersFactoryService, | ||
| [ImportMany] IEnumerable<Lazy<ArgumentProvider, OrderableLanguageMetadata>> argumentProviders, | ||
| EditorOptionsService editorOptionsService) | ||
| : AbstractSnippetExpansionClientFactory(threadingContext) | ||
| { | ||
| private readonly IThreadingContext _threadingContext = threadingContext; | ||
| private readonly SignatureHelpControllerProvider _signatureHelpControllerProvider = signatureHelpControllerProvider; | ||
| private readonly IEditorCommandHandlerServiceFactory _editorCommandHandlerServiceFactory = editorCommandHandlerServiceFactory; | ||
| private readonly IVsEditorAdaptersFactoryService _editorAdaptersFactoryService = editorAdaptersFactoryService; | ||
| private readonly ImmutableArray<Lazy<ArgumentProvider, OrderableLanguageMetadata>> _argumentProviders = argumentProviders.ToImmutableArray(); | ||
| private readonly EditorOptionsService _editorOptionsService = editorOptionsService; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just get rid of these and use the constructor parameters? It'd definitely clean this type up nicely.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ For me, this change is blocked on insufficient resolution to #70419 |
||
|
|
||
| protected override AbstractSnippetExpansionClient CreateSnippetExpansionClient(ITextView textView, ITextBuffer subjectBuffer) | ||
| { | ||
| return new CSharpSnippetExpansionClient( | ||
| _threadingContext, | ||
| Guids.CSharpLanguageServiceId, | ||
|
||
| textView, | ||
| subjectBuffer, | ||
| _signatureHelpControllerProvider, | ||
| _editorCommandHandlerServiceFactory, | ||
| _editorAdaptersFactoryService, | ||
| _argumentProviders, | ||
| _editorOptionsService); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,16 +5,11 @@ | |
| #nullable disable | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.ComponentModel.Composition; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Threading; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Completion; | ||
| using Microsoft.CodeAnalysis.CSharp.Extensions; | ||
| using Microsoft.CodeAnalysis.Editor.CSharp.CompleteStatement; | ||
| using Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.SignatureHelp; | ||
| using Microsoft.CodeAnalysis.Editor.Shared.Extensions; | ||
| using Microsoft.CodeAnalysis.Editor.Shared.Utilities; | ||
| using Microsoft.CodeAnalysis.Host.Mef; | ||
|
|
@@ -24,11 +19,10 @@ | |
| using Microsoft.VisualStudio.Editor; | ||
| using Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion; | ||
| using Microsoft.VisualStudio.LanguageServices.Implementation.Snippets; | ||
| using Microsoft.VisualStudio.Shell; | ||
| using Microsoft.VisualStudio.Text; | ||
| using Microsoft.VisualStudio.Text.Editor; | ||
| using Microsoft.VisualStudio.Text.Editor.Commanding; | ||
| using Microsoft.VisualStudio.Text.Editor.Commanding.Commands; | ||
| using Microsoft.VisualStudio.TextManager.Interop; | ||
| using Microsoft.VisualStudio.Utilities; | ||
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.CSharp.Snippets | ||
|
|
@@ -45,21 +39,18 @@ internal sealed class SnippetCommandHandler : | |
| ICommandHandler<SurroundWithCommandArgs>, | ||
| IChainedCommandHandler<TypeCharCommandArgs> | ||
| { | ||
| private readonly ImmutableArray<Lazy<ArgumentProvider, OrderableLanguageMetadata>> _argumentProviders; | ||
| private readonly IVsEditorAdaptersFactoryService _editorAdaptersFactoryService; | ||
|
|
||
| [ImportingConstructor] | ||
| [SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] | ||
| [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| public SnippetCommandHandler( | ||
| IThreadingContext threadingContext, | ||
| SignatureHelpControllerProvider signatureHelpControllerProvider, | ||
| IEditorCommandHandlerServiceFactory editorCommandHandlerServiceFactory, | ||
| IVsEditorAdaptersFactoryService editorAdaptersFactoryService, | ||
| SVsServiceProvider serviceProvider, | ||
| [ImportMany] IEnumerable<Lazy<ArgumentProvider, OrderableLanguageMetadata>> argumentProviders, | ||
| IVsService<SVsTextManager, IVsTextManager2> textManager, | ||
| EditorOptionsService editorOptionsService) | ||
| : base(threadingContext, signatureHelpControllerProvider, editorCommandHandlerServiceFactory, editorAdaptersFactoryService, editorOptionsService, serviceProvider) | ||
| : base(threadingContext, editorOptionsService, textManager) | ||
| { | ||
| _argumentProviders = argumentProviders.ToImmutableArray(); | ||
| _editorAdaptersFactoryService = editorAdaptersFactoryService; | ||
| } | ||
|
|
||
| public bool ExecuteCommand(SurroundWithCommandArgs args, CommandExecutionContext context) | ||
|
|
@@ -115,37 +106,20 @@ public void ExecuteCommand(TypeCharCommandArgs args, Action nextCommandHandler, | |
| nextCommandHandler(); | ||
| } | ||
|
|
||
| protected override AbstractSnippetExpansionClient GetSnippetExpansionClient(ITextView textView, ITextBuffer subjectBuffer) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 This pull request decouples this factory method |
||
| { | ||
| if (!textView.Properties.TryGetProperty(typeof(AbstractSnippetExpansionClient), out AbstractSnippetExpansionClient expansionClient)) | ||
| { | ||
| expansionClient = new SnippetExpansionClient( | ||
| ThreadingContext, | ||
| Guids.CSharpLanguageServiceId, | ||
| textView, | ||
| subjectBuffer, | ||
| SignatureHelpControllerProvider, | ||
| EditorCommandHandlerServiceFactory, | ||
| EditorAdaptersFactoryService, | ||
| _argumentProviders, | ||
| EditorOptionsService); | ||
|
|
||
| textView.Properties.AddProperty(typeof(AbstractSnippetExpansionClient), expansionClient); | ||
| } | ||
|
|
||
| return expansionClient; | ||
| } | ||
|
|
||
| protected override bool TryInvokeInsertionUI(ITextView textView, ITextBuffer subjectBuffer, bool surroundWith = false) | ||
| { | ||
| if (!TryGetExpansionManager(out var expansionManager)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var document = subjectBuffer.CurrentSnapshot.GetOpenDocumentInCurrentContextWithChanges(); | ||
| if (document == null) | ||
| return false; | ||
|
|
||
| expansionManager.InvokeInsertionUI( | ||
| EditorAdaptersFactoryService.GetViewAdapter(textView), | ||
| GetSnippetExpansionClient(textView, subjectBuffer), | ||
| _editorAdaptersFactoryService.GetViewAdapter(textView), | ||
| GetSnippetExpansionClientFactory(document).GetSnippetExpansionClient(textView, subjectBuffer), | ||
| Guids.CSharpLanguageServiceId, | ||
| bstrTypes: surroundWith ? ["SurroundsWith"] : ["Expansion", "SurroundsWith"], | ||
| iCountTypes: surroundWith ? 1 : 2, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight waryness as i believe we support completion in non-Document cases. However, it's likely we don't support Custom completion there, so this is likely fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the callers already have a
Documentin scope. This is an internal interface that could be adjusted in the future if we need to support a new scenario.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ Left this one as-is