Extract factory interface for AbstractSnippetExpansionClient #71752

sharwell · 2024-01-22T18:03:44Z

Prerequisite to #71731

sharwell · 2024-01-22T18:04:39Z

src/VisualStudio/CSharp/Impl/Snippets/SnippetCommandHandler.cs

            nextCommandHandler();
        }

-        protected override AbstractSnippetExpansionClient GetSnippetExpansionClient(ITextView textView, ITextBuffer subjectBuffer)


📝 This pull request decouples this factory method

sharwell · 2024-01-22T18:05:12Z

src/VisualStudio/VisualBasic/Impl/Snippets/SnippetExpansionClient.vb

                       editorOptionsService)
        End Sub

-        Public Shared Function GetSnippetExpansionClient(


📝 This pull request decouples this factory method

CyrusNajmabadi · 2024-01-23T18:27:55Z

src/EditorFeatures/Core/Extensibility/Completion/ICustomCommitCompletionProvider.cs

    internal interface ICustomCommitCompletionProvider
    {
-        void Commit(CompletionItem completionItem, ITextView textView, ITextBuffer subjectBuffer, ITextSnapshot triggerSnapshot, char? commitChar);
+        void Commit(CompletionItem completionItem, Document document, ITextView textView, ITextBuffer subjectBuffer, ITextSnapshot triggerSnapshot, char? commitChar);


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.

All of the callers already have a Document in scope. This is an internal interface that could be adjusted in the future if we need to support a new scenario.

➡️ Left this one as-is

CyrusNajmabadi · 2024-01-23T18:28:59Z

src/VisualStudio/CSharp/Impl/Snippets/CSharpSnippetExpansionClientFactory.cs

+        IEditorCommandHandlerServiceFactory editorCommandHandlerServiceFactory,
+        IVsEditorAdaptersFactoryService editorAdaptersFactoryService,
+        [ImportMany] IEnumerable<Lazy<ArgumentProvider, OrderableLanguageMetadata>> argumentProviders,
+        EditorOptionsService editorOptionsService)


consider primary constructor.

➡️ Switched to primary constructor

src/VisualStudio/CSharp/Impl/Snippets/CSharpSnippetExpansionClientFactory.cs

jasonmalinowski · 2024-01-23T18:30:48Z

src/VisualStudio/Core/Def/Snippets/AbstractSnippetExpansionClientFactory.cs

+        if (!textView.Properties.TryGetProperty(typeof(AbstractSnippetExpansionClient), out AbstractSnippetExpansionClient expansionClient))
+        {
+            expansionClient = CreateSnippetExpansionClient(textView, subjectBuffer);
+            textView.Properties.AddProperty(typeof(AbstractSnippetExpansionClient), expansionClient);
+        }


There's a GetOrCreateSingleton method for this exact pattern.

GetOrCreateSingleton doesn't support a non-capturing caller, so I kept the pattern we were already using.

➡️ Left this one as-is

What's the problem? I don't care if this would allocate a lambda -- the allocation cost here is unmeasurable...

I ended up keeping this one not because I think the allocation cost would be a problem, but because the old code used the current pattern and I can't be sure the allocation cost wouldn't be a problem.

CyrusNajmabadi · 2024-01-23T18:30:53Z

src/VisualStudio/Core/Def/Snippets/AbstractSnippetExpansionClientFactory.cs

+
+    public AbstractSnippetExpansionClient GetSnippetExpansionClient(ITextView textView, ITextBuffer subjectBuffer)
+    {
+        if (!textView.Properties.TryGetProperty(typeof(AbstractSnippetExpansionClient), out AbstractSnippetExpansionClient expansionClient))


would it be possible to add some threading asserts here? it's not a big deal, it just makes me feel less oogy about doing weird stuff with proeprty bags on random threads.

➡️ Added assertion

CyrusNajmabadi · 2024-01-23T18:31:51Z

src/VisualStudio/Core/Def/Snippets/ISnippetExpansionClientFactory.cs

+
+internal interface ISnippetExpansionClientFactory : ILanguageService
+{
+    AbstractSnippetExpansionClient GetSnippetExpansionClient(ITextView textView, ITextBuffer subjectBuffer);


nit: for the future would prefer if this exposed a particular interface. I don't love interfaces exposing abstract-reified types like this. But it's def not blocking, and i don't mind this corner of the LS working this way.

➡️ Left this one as-is

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb

CyrusNajmabadi · 2024-01-23T18:32:43Z

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb

+        Private ReadOnly _editorCommandHandlerServiceFactory As IEditorCommandHandlerServiceFactory
+        Private ReadOnly _editorAdaptersFactoryService As IVsEditorAdaptersFactoryService
+        Private ReadOnly _argumentProviders As ImmutableArray(Of Lazy(Of ArgumentProvider, OrderableLanguageMetadata))
+        Private ReadOnly _editorOptionsService As EditorOptionsService


nit. lots of duplication between the C# and VB factories. would love this to move down.

➡️ The duplication does not appear to be avoidable, so for now left this one as-is.

Moving it down would add a large number of parameters to the base class, and would not simplify either of the derived type constructors. The current form makes it easier to see if any specific implementation no longer needs its parameters.

I think if we were going to unify this further, the trick would be to get rid of the derivations of AbstractSnippetExpansionClient, and instead move any of the different behaviors there into a language service with the appropriate methods...but one step at a time seems reasonable here.

That would help tremendously for testing. I'll see how viable that is next.

➡️ Now refactored in the latest commit

CyrusNajmabadi · 2024-01-23T18:33:09Z

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb

+            _signatureHelpControllerProvider = signatureHelpControllerProvider
+            _editorCommandHandlerServiceFactory = editorCommandHandlerServiceFactory
+            _editorAdaptersFactoryService = editorAdaptersFactoryService
+            _argumentProviders = argumentProviders.ToImmutableArray()


same. this is duplicated with teh C# version. Could move into the abstract base (including with a primary cosntructor :D). I would really like to see that in this pr :)

➡️ Refactored to reduce duplication

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb

CyrusNajmabadi · 2024-01-23T18:34:01Z

Signing off with the strong ask about moving the shared field code to the base class. I'm ok with that happening in a quick followup PR though if you would prefer to get this in and unblcokign you.

jasonmalinowski · 2024-01-24T00:24:17Z

src/VisualStudio/CSharp/Impl/Snippets/CSharpSnippetExpansionClientFactory.cs

+    {
+        return new CSharpSnippetExpansionClient(
+            _threadingContext,
+            Guids.CSharpLanguageServiceId,


Does anything ever pass in a different constant for the this? Or should this just be set directly in CSharpSnippetExpansionClient.cs?

➡️ In light of #71752 (comment), leaving this for a follow-up

➡️ Fixed now

jasonmalinowski · 2024-01-24T00:24:33Z

src/VisualStudio/CSharp/Impl/Snippets/CSharpSnippetExpansionClientFactory.cs

+    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;


Should we just get rid of these and use the constructor parameters? It'd definitely clean this type up nicely.

➡️ For me, this change is blocked on insufficient resolution to #70419

jasonmalinowski · 2024-01-24T00:31:34Z

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb

+        Private ReadOnly _editorCommandHandlerServiceFactory As IEditorCommandHandlerServiceFactory
+        Private ReadOnly _editorAdaptersFactoryService As IVsEditorAdaptersFactoryService
+        Private ReadOnly _argumentProviders As ImmutableArray(Of Lazy(Of ArgumentProvider, OrderableLanguageMetadata))
+        Private ReadOnly _editorOptionsService As EditorOptionsService


I think if we were going to unify this further, the trick would be to get rid of the derivations of AbstractSnippetExpansionClient, and instead move any of the different behaviors there into a language service with the appropriate methods...but one step at a time seems reasonable here.

src/VisualStudio/Core/Test/Snippets/SnippetTestState.vb

jasonmalinowski · 2024-01-24T00:37:18Z

src/VisualStudio/Core/Test/Snippets/SnippetTestState.vb

+            If languageName = LanguageNames.CSharp Then
+                snippetCommandHandler = Workspace.ExportProvider.GetExportedValues(Of ICommandHandler)().Single(
+                    Function(commandHandler)
+                        Return TryCast(commandHandler, CSharp.Snippets.SnippetCommandHandler) IsNot Nothing
+                    End Function)
+            Else
+                snippetCommandHandler = Workspace.ExportProvider.GetExportedValues(Of ICommandHandler)().Single(
+                    Function(commandHandler)
+                        Return TryCast(commandHandler, VisualBasic.Snippets.SnippetCommandHandler) IsNot Nothing
+                    End Function)
+            End If
+
+            Me.SnippetCommandHandler = DirectCast(snippetCommandHandler, AbstractSnippetCommandHandler)


Any reason we can't just pick out the one that derives from AbstractSnippetCommandHandler and filter with MEF metadata? This feels pretty clunky.

➡️ Fixed by using an existing helper method

jasonmalinowski · 2024-01-24T00:38:34Z

src/VisualStudio/Core/Test/Snippets/SnippetTestState.vb

+            End Function
+        End Class
+
+        <ExportLanguageService(GetType(ISnippetExpansionClientFactory), LanguageNames.VisualBasic, ServiceLayer.Test)>


We should be able to do a single export for both languages, right?

➡️ Also leaving this one for #71752 (comment)

➡️ Only one ExportLanguageService attribute can be applied to a type

…ntations

sharwell added 2 commits January 19, 2024 15:08

Extract helper interface ISnippetExpansionClientFactory

68b70a2

Simplify dependencies in AbstractSnippetCommandHandler

fc0cda0

sharwell requested a review from a team as a code owner January 22, 2024 18:03

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 22, 2024

sharwell commented Jan 22, 2024

View reviewed changes

CyrusNajmabadi reviewed Jan 23, 2024

View reviewed changes

sharwell changed the title ~~Extract factory interface for AbstractSnippetCommandHandler~~ Extract factory interface for AbstractSnippetExpansionClient Jan 23, 2024

CyrusNajmabadi reviewed Jan 23, 2024

View reviewed changes

src/VisualStudio/CSharp/Impl/Snippets/CSharpSnippetExpansionClientFactory.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Jan 23, 2024

View reviewed changes

CyrusNajmabadi reviewed Jan 23, 2024

View reviewed changes

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 23, 2024

View reviewed changes

src/VisualStudio/VisualBasic/Impl/Snippets/VisualBasicSnippetExpansionClientFactory.vb Outdated Show resolved Hide resolved

CyrusNajmabadi approved these changes Jan 23, 2024

View reviewed changes

Updates based on code review

1e298dc

jasonmalinowski reviewed Jan 24, 2024

View reviewed changes

sharwell added 2 commits January 24, 2024 11:01

Updates based on code review

89ff162

Refactor expansion handler to avoid language-specific derived impleme…

5a9d9c4

…ntations

dotnet deleted a comment from azure-pipelines bot Feb 13, 2024

sharwell merged commit 5eae863 into dotnet:main Feb 13, 2024

sharwell deleted the extract-factory branch February 13, 2024 16:44

ghost added this to the Next milestone Feb 13, 2024

Cosifne mentioned this pull request Feb 21, 2024

[Automated] PRs inserted in VS build main-34620.00 #72198

Closed

jjonescz removed this from the Next milestone Feb 27, 2024

jjonescz added this to the 17.10 P2 milestone Feb 27, 2024

Extract factory interface for AbstractSnippetExpansionClient #71752

Extract factory interface for AbstractSnippetExpansionClient #71752

Uh oh!

Conversation

sharwell commented Jan 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Jan 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sharwell Jan 23, 2024 •

edited

Loading