feat: Allow disabling of editor templates when accepting method/function invocation completions (fixes #718) #744
Conversation
| @@ -100,14 +114,16 @@ public void handleInsert(@NotNull InsertionContext context) { | |||
| updateCompletionItemFromResolved(); | |||
| Template template = null; | |||
| if (item.getInsertTextFormat() == InsertTextFormat.Snippet) { | |||
| // Adjust the snippet content as appropriate based on client config | |||
| String snippetContent = adjustSnippetContent(getInsertText()); | |||
There was a problem hiding this comment.
Now we adjust the LSP-provided snippet before parsing it into a template if appropriate based on client config and the snippet contents.
| // Get the indentation settings | ||
| LspSnippetIndentOptions indentOptions = CompletionProposalTools.createLspIndentOptions(snippetContent, file); | ||
| // Load the insert text to build: | ||
| // - an IJ Template instance which will take care of replacement of placeholders | ||
| // - the insert text without placeholders | ||
| template = SnippetTemplateFactory.createTemplate(snippetContent, context.getProject(), name -> getVariableValue(name), indentOptions); | ||
| template = SnippetTemplateFactory.createTemplate(snippetContent, context.getProject(), this::getVariableValue, indentOptions); |
There was a problem hiding this comment.
Just changed to a method reference. Same behavior.
| @@ -116,7 +132,12 @@ public void handleInsert(@NotNull InsertionContext context) { | |||
| // Apply all text edits | |||
| apply(context.getDocument(), context.getOffset(CompletionInitializationContext.SELECTION_END_OFFSET)); | |||
|
|
|||
| if (shouldStartTemplate(template)) { | |||
| // Just move to the first tab stop if there's a single invocation argument with no default value | |||
| if (shouldMoveToFirstTabStop(template)) { | |||
There was a problem hiding this comment.
Now we only move to the first tab stop for a template with a single non-end variable that has no value. That happens independent of client config because it doesn't make sense to start an editor template for a single tab stop with no default value. Instead just place the caret in that same location and let the user type what they need.
| @@ -155,12 +176,57 @@ private void updateCompletionItemFromResolved() { | |||
| this.item = resolved; | |||
| } | |||
|
|
|||
| @NotNull | |||
| private String adjustSnippetContent(@NotNull String snippetContent) { | |||
There was a problem hiding this comment.
Hopefully this method is pretty self-explanatory, but basically if client config says not to use editor templates for invocation arg-only completion snippets, see if the snippet contains what appears to be an invocation arg list with template variables, and if so and if there are no non-end variables outside of that invocation arg list, just update the snippet to be <beforeArgList>($0)<afterArgList> with any other $0 removed. That way when the snippet is parsed into a template, it will just result in the caret being moved into the invocation arg parens.
| return snippetContent; | ||
| } | ||
|
|
||
| @Contract("null -> false") |
There was a problem hiding this comment.
I added @Contract annotations here and below because the IDE's nullability inspection was having trouble determining the relationships between the params and the results and was raising false inspection positives.
| /** | ||
| * Common interface for language server definitions that support client configuration. | ||
| */ | ||
| public interface ClientConfigurableLanguageServerDefinition { |
There was a problem hiding this comment.
This is the part that probably warrants the most discussion, and it also exists in the upcoming PR for client-side on-type formatting. Basically we need to be able to change client config settings from unit tests, but prior to this change, only UserDefinedLanguageServerDefinition has had access to client configuration. However, we need MockLanguageServerDefinition to have client configuration as well, and ideally without having to push that notion up into LanguageServerDefinition. As a result, I added this common interface for LanguageServerDefinition implementations that are client-configurable, and I implemented that interface on both UserDefinedLanguageServerDefinition and MockLanguageServerDefinition. There will obviously be a few other related changes below, but hopefully that sets the context. And I'm certainly open to other options for how client config could be exposed in unit tests, but this certainly works nicely and without any major ripple-effect.
| @@ -24,8 +25,15 @@ public class UserDefinedCompletionFeature extends LSPCompletionFeature { | |||
|
|
|||
| @Override | |||
| public boolean useContextAwareSorting(@NotNull PsiFile file) { | |||
| UserDefinedLanguageServerDefinition serverDefinition = (UserDefinedLanguageServerDefinition) getClientFeatures().getServerDefinition(); | |||
| ClientConfigurableLanguageServerDefinition serverDefinition = (ClientConfigurableLanguageServerDefinition) getClientFeatures().getServerDefinition(); | |||
There was a problem hiding this comment.
Previously it was deemed safe to blindly cast to UserDefinedLanguageServerDefinition so I'm doing the same here with ClientConfigurableLanguageServerDefinition. However, I would feel better checking the type before casting with this change. Thoughts? If you agree, I'll update all such usages to be more defensive.
| @@ -34,7 +35,7 @@ | |||
| * {@link com.redhat.devtools.lsp4ij.server.definition.LanguageServerDefinition} implementation to start a | |||
| * language server with a process command defined by the user. | |||
| */ | |||
| public class UserDefinedLanguageServerDefinition extends LanguageServerDefinition { | |||
| public class UserDefinedLanguageServerDefinition extends LanguageServerDefinition implements ClientConfigurableLanguageServerDefinition { | |||
There was a problem hiding this comment.
Here's where it has been updated to implement the client-configurable interface.
| @@ -1,7 +1,8 @@ | |||
| { | |||
| "caseSensitive": true, | |||
| "completion": { | |||
| "useContextAwareSorting": true | |||
| "useContextAwareSorting": true, | |||
| "useTemplateForInvocationOnlySnippet": false | |||
There was a problem hiding this comment.
I updated all of the templates to disable editor templates for invocation-only snippets similar to how I enabled context-aware sorting in the templates.
| /** | ||
| * Tests code completion client configuration settings using a mock CSS language server. | ||
| */ | ||
| public class CssCompletionClientConfigTest extends LSPCompletionClientConfigFixtureTestCase { |
There was a problem hiding this comment.
The CSS test exercises whitespace-delimited argument list snippets as well as some complex snippets with variable values that are enumerated lists/choices.
| /** | ||
| * Tests code completion client configuration settings using a mock TypeScript language server. | ||
| */ | ||
| public class TypeScriptCompletionClientConfigTest extends LSPCompletionClientConfigFixtureTestCase { |
There was a problem hiding this comment.
Hopefully this test will be straightforward, but it verifies the following scenarios:
- Single invocation variable with default settings => template-inserted arg and caret after the invocation parens
- Single invocation variable with editor templates disabled => caret inside the invocation parens
- Single valueless invocation variable with default settings => caret inside the invocation parens
- Single valueless invocation variable with editor templates disabled => caret inside the invocation parens
- Multiple invocation variables with default settings => template-inserted args and caret after the invocation parens
- Multiple invocation variables with editor templates disabled => caret inside the invocation parens
- No invocation variables with default settings => Caret after the invocation parens
- No invocation variables with editor templates disabled => Caret after the invocation parens
- Multiple invocation variables and one non-invocation variable with default settings => template-inserted args and caret after the snippet
- Multiple invocation variables and one non-invocation variable with editor templates disabled => template-inserted args and caret after the snippet
Note that in a unit test context, the template is evaluated as if the user had tabbed through all tab stops, so for those where a template is expected, I'm just confirming that the expected arg defaults were inserted in the appropriate places and the caret was moved to the template's designated end offset.
| CompletionList mockCompletionList = JSONUtils.getLsp4jGson().fromJson(mockTextCompletionJson, CompletionList.class); | ||
| MockLanguageServer.INSTANCE.setCompletionList(mockCompletionList); | ||
|
|
||
| CompletionItem mockCompletionItem = JSONUtils.getLsp4jGson().fromJson(mockCompletionItemResolveJson, CompletionItem.class); |
There was a problem hiding this comment.
I had to add the ability to specify the mock resolved completion item.
| } | ||
|
|
||
| // Configure the language server's completion client configuration | ||
| LanguageServerItem languageServer = ContainerUtil.getFirstItem(languageServers); |
There was a problem hiding this comment.
Now we need to get the language server that's going to be used and set it up properly for client config testing.
|
|
||
| // Use a configurable completion feature | ||
| LSPClientFeatures clientFeatures = languageServer.getClientFeatures(); | ||
| clientFeatures.setCompletionFeature(new UserDefinedCompletionFeature()); |
There was a problem hiding this comment.
Make sure that the mock server is using a configurable completion feature.
| clientFeatures.setCompletionFeature(new UserDefinedCompletionFeature()); | ||
|
|
||
| // Enable completion item resolution | ||
| languageServer.getServerCapabilities().setCompletionProvider(new CompletionOptions(true, null)); |
There was a problem hiding this comment.
By default the mock completion provider's options don't enable resolution, so must enable that for this test.
| languageServer.getServerCapabilities().setCompletionProvider(new CompletionOptions(true, null)); | ||
|
|
||
| // Update client configuration as required for this test scenario | ||
| if (clientConfigCustomizer != null) { |
There was a problem hiding this comment.
And finally, get the client configuration from the server and apply any config changes for this test scenario.
| clientConfigCustomizer.accept(clientConfiguration); | ||
| } | ||
|
|
||
| // Move to the offset at which completion should be triggered |
There was a problem hiding this comment.
Hopefully the rest is pretty self-explanatory.
| @@ -162,6 +162,10 @@ public void setCompletionList(CompletionList completionList) { | |||
| this.textDocumentService.setMockCompletionList(completionList); | |||
| } | |||
|
|
|||
| public void setCompletionItem(CompletionItem completionItem) { | |||
There was a problem hiding this comment.
Like I said above, need to be able to set the mock resolved completion item.
|
|
||
| /** | ||
| * {@link LanguageServerDefinition} implementation to register the mock language server. | ||
| */ | ||
| public class MockLanguageServerDefinition extends LanguageServerDefinition { | ||
| public class MockLanguageServerDefinition extends LanguageServerDefinition implements ClientConfigurableLanguageServerDefinition { |
There was a problem hiding this comment.
The mock language server is now client-configurable as well.
|
|
||
| private static final String SERVER_ID = "mock-server-id"; | ||
|
|
||
| private final ClientConfigurationSettings clientConfigurationSettings = new ClientConfigurationSettings(); |
There was a problem hiding this comment.
And it just starts with a default client configuration.
| @@ -35,6 +35,7 @@ | |||
| public class MockTextDocumentService implements TextDocumentService { | |||
|
|
|||
| private CompletionList mockCompletionList; | |||
| private CompletionItem mockCompletionItem; | |||
There was a problem hiding this comment.
Other stuff required to be able to mock the resolved completion item.
…on-only snippet so that it happens during the transformation from snippet to template.
| context.getProject(), | ||
| this::getVariableValue, | ||
| indentOptions, | ||
| useTemplateForInvocationOnlySnippet |
There was a problem hiding this comment.
We now pass this state to the snippet-to-template transform.
| template, | ||
| variableResolver, | ||
| indentOptions, | ||
| useTemplateForInvocationOnlySnippet |
There was a problem hiding this comment.
And propagate it to the template loader.
|
|
||
| private final List<String> existingVariables; | ||
| private final List<SnippetTemplateSegment> templateSegments = new LinkedList<>(); |
There was a problem hiding this comment.
Instead of building the template on-the-fly, we accumulate the exact same information that we'd need to build the template, and we do it in endSnippet() instead. That allows us to have full context of the resulting template segments so we can see whether/how it can be transformed/simplified.
| List<SnippetTemplateSegment> effectiveTemplateSegments = templateSegments; | ||
|
|
||
| // If we should not use a template for an invocation-only snippet, see whether this fits the pattern | ||
| if (!useTemplateForInvocationOnlySnippet) { |
There was a problem hiding this comment.
And here's where the actual simplification logic exists. If configured to simplify invocation-only snippets, we look at the segments that were accumulated and, if all (non-end) variables were found within what seems to be an invocation (basically balanced paired parentheses), all variables are removed from the template and it ends up becoming:
<textSegmentsBeforeAndIncludingInvocationStart><endVariable><textSegmentsAfterAndIncludingInvocationEnd>
This also guards against snippets with nested invocation parens and more than one invocation pattern.
|
|
||
| // If we found an invocation and all variables were inside of it, use the simplified template segments | ||
| if (hasInvocation && !hasVariablesOutsideInvocation) { | ||
| effectiveTemplateSegments = simplifiedTemplateSegments; |
There was a problem hiding this comment.
If it looks like an invocation-only snippet, use the simplified segments below when building the actual template.
…vocations and checking for proper balancing before using the simplified template segments.
| } | ||
|
|
||
| @Override | ||
| public void text(String text) { | ||
| template.addTextSegment(formatText(text)); | ||
| templateSegments.add(SnippetTemplateSegment.textSegment(formatText(text))); |
There was a problem hiding this comment.
From here down, what you'll see is the change from building the template itself to accumulating what's needed to build the template in endSnippet().
| * This is a simple wrapper for a {@link com.intellij.codeInsight.template.Template} segment that allows deferred | ||
| * creation of the corresponding template with all segment information retained. | ||
| */ | ||
| class SnippetTemplateSegment { |
There was a problem hiding this comment.
And this is the way we accumulate that state. I've basically added factory methods for each of the types of template segment that are created above and simple ways to check which type of segment each is so it can be used safely/properly when building the final template.
…ll fixes around nested parens.
| /** | ||
| * Tests behavior of {@link SnippetTemplateLoader} via {@link SnippetTemplateFactory}. | ||
| */ | ||
| public class SnippetTemplateLoaderTest extends LSPCompletionFixtureTestCase { |
There was a problem hiding this comment.
Direct tests for SnippetTemplateLoader to exercise common and corner-case scenarios.
|
It works like a charm, great job @SCWells72 ! |
This is a fresh branch-based PR as a replacmeent for #738.
As stated/shown in #718, the editor template that is currently started when accepting a code completion that is a method/function invocation
with a single argumentis quite disruptive to the standard editor flow.This PR adds a new client configuration option,completion.useTemplateForSingleArgument, that, when disabled (enabled by default for backward-compatibility), instead just places the caret into the invocation arguments so that the user can populate them as they would in any of JetBrains' natively-supported language editors. I've also updated all existing language server definition templates to disable editor templates in this case so that the "default" behavior for newly-created language server definitions is the smoother/more natural editor flow, but existing language server definitions will not see a change in behavior.UPDATE: This has now been generalized to allow disabling editor templates for any invocation-only snippet. An invocation-only snippet is one where all non-end (
$0) variables are found as a comma-/whitespace-delimited list in invocation parentheses. If other invocation patterns are found in LSP languages, recognition support can be added for them as well.The configuration option is now
completion.useTemplateForInvocationOnlySnippetto reflect this more general behavior. As before, it is disabled by default in code but enabled by default in the bundled language server templates.This also properly handles the situation of a single no-value variable snippet by forgoing an editor template and moving to the correct caret position for that variable with no need for any client config change.