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

feat: Add client-side language server configuration with per-language case sensitivity as the first available option (issue 626) #659

Merged
merged 7 commits into from
Dec 4, 2024
Merged

feat: Add client-side language server configuration with per-language case sensitivity as the first available option (issue 626) #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
954164d
Issue 626 - Added client-side language server configuration with per-…
SCWells72 Dec 2, 2024
cbb923f
Issue 626 - Added JSON schema-based completion and validation for cli…
SCWells72 Dec 2, 2024
8a3e1de
Issue 626 - Added clientSettings.json files for all templates. Perhap…
SCWells72 Dec 2, 2024
e898618
Issue 626 - Made the JSON schema for client settings more strict.
SCWells72 Dec 2, 2024
500d7fd
Issue 626 - Implemented almost all feedback. Still need to add a User…
SCWells72 Dec 3, 2024
bc4d8ef
Issue 626 - Added a TODO for the remaining work around where isCaseSe…
SCWells72 Dec 3, 2024
696165d
Issue 626 - Refactored the new case-sensitivity stuff to be implement…
SCWells72 Dec 3, 2024
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
1 change: 1 addition & 0 deletions docs/LSPApi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ public class MyLSPCodeLensFeature extends LSPCodeLensFeature {
| boolean isStrikeout(CompletionItem item) | Returns true if the IntelliJ lookup is strike out and false otherwise. | use `item.getDeprecated()` or `item.getTags().contains(CompletionItemTag.Deprecated)` |
| String getTailText(CompletionItem item) | Returns the IntelliJ lookup tail text from the given LSP completion item and null otherwise. | `item.getLabelDetails().getDetail()` |
| boolean isItemTextBold(CompletionItem item) | Returns the IntelliJ lookup item text bold from the given LSP completion item and null otherwise. | `item.getKind() == CompletionItemKind.Keyword` |
| boolean isCaseSensitive(PsiFile file) | Determines whether or not completions should be offered in a case-sensitive manner. | Case-insensitive. |

## LSP Declaration Feature

Expand Down
10 changes: 8 additions & 2 deletions src/main/java/com/redhat/devtools/lsp4ij/LanguageServersRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ private void loadServersAndMappingFromSettings() {
launch.getUserEnvironmentVariables(),
launch.isIncludeSystemEnvironmentVariables(),
launch.getConfigurationContent(),
launch.getInitializationOptionsContent()),
launch.getInitializationOptionsContent(),
launch.getClientConfigurationContent()),
Copy link
Contributor Author

@SCWells72 SCWells72 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's obviously going to be a proliferation of these types of changes where I've added clientConfiguration as another property of UserDefinedLanguageServerDefinition and its peer types/methods/etc.

mappings);
}
} catch (Exception e) {
Expand Down Expand Up @@ -365,6 +366,7 @@ private void addServerDefinitionWithoutNotification(@NotNull LanguageServerDefin
}
settings.setConfigurationContent(definitionFromSettings.getConfigurationContent());
settings.setInitializationOptionsContent(definitionFromSettings.getInitializationOptionsContent());
settings.setClientConfigurationContent(definitionFromSettings.getClientConfigurationContent());
UserDefinedLanguageServerSettings.getInstance().setLaunchConfigSettings(languageServerId, settings);
}
}
Expand Down Expand Up @@ -469,6 +471,7 @@ private void removeAssociationsFor(LanguageServerDefinition definition) {
request.serverDefinition().setIncludeSystemEnvironmentVariables(request.includeSystemEnvironmentVariables());
request.serverDefinition().setConfigurationContent(request.configurationContent());
request.serverDefinition().setInitializationOptionsContent(request.initializationOptionsContent());
request.serverDefinition().setClientConfigurationContent(request.clientConfigurationContent());

// remove associations
removeAssociationsFor(request.serverDefinition());
Expand All @@ -483,13 +486,15 @@ private void removeAssociationsFor(LanguageServerDefinition definition) {
boolean mappingsChanged = !Objects.deepEquals(settings.getMappings(), request.mappings());
boolean configurationContentChanged = !Objects.equals(settings.getConfigurationContent(), request.configurationContent());
boolean initializationOptionsContentChanged = !Objects.equals(settings.getInitializationOptionsContent(), request.initializationOptionsContent());
// Not checking whether client config changed because that shouldn't result in a LanguageServerChangedEvent
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically calling out that we don't need to include client config changes when computing whether or not to send an event here because the actual language server doesn't care at all about client-side config changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the moment I agree with you. Lets keep like this, but we could have a client settings which could impact the editor (like enable/disable lsp codelens which should refresh the editor and in this case we should send an event)


settings.setServerName(request.name());
settings.setCommandLine(request.commandLine());
settings.setUserEnvironmentVariables(request.userEnvironmentVariables());
settings.setIncludeSystemEnvironmentVariables(request.includeSystemEnvironmentVariables());
settings.setConfigurationContent(request.configurationContent());
settings.setInitializationOptionsContent(request.initializationOptionsContent());
settings.setClientConfigurationContent(request.clientConfigurationContent);
settings.setMappings(request.mappings());

if (nameChanged || commandChanged || userEnvironmentVariablesChanged || includeSystemEnvironmentVariablesChanged ||
Expand Down Expand Up @@ -578,7 +583,8 @@ public record UpdateServerDefinitionRequest(@NotNull Project project,
boolean includeSystemEnvironmentVariables,
@NotNull List<ServerMappingSettings> mappings,
@Nullable String configurationContent,
@Nullable String initializationOptionsContent) {
@Nullable String initializationOptionsContent,
@Nullable String clientConfigurationContent) {
}

/**
Expand Down
42 changes: 34 additions & 8 deletions src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPCompletionFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,32 +215,47 @@ public boolean isItemTextBold(@NotNull CompletionItem item) {
/**
* Don't override this method, we need to revisit the API and the prefix computation (to customize it).
*
* @param context
* @param completionPrefix
* @param result
* @param lookupItem
* @param priority
* @param item
*/
@ApiStatus.Internal
public void addLookupItem(@NotNull CompletionPrefix completionPrefix,
public void addLookupItem(@NotNull LSPCompletionContext context,
@NotNull CompletionPrefix completionPrefix,
@NotNull CompletionResultSet result,
@NotNull LookupElement lookupItem,
int priority,
@NotNull CompletionItem item) {
// Determine whether or not completions should be case-sensitive
boolean caseSensitive = isCaseSensitive(context.getParameters().getOriginalFile());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that both getParameters() and getOriginalFile() are annotated as @NotNull, so this is safe.


var prioritizedLookupItem = PrioritizedLookupElement.withPriority(lookupItem, priority);

// Compute the prefix
var textEditRange = ((LSPCompletionProposal) lookupItem).getTextEditRange();
String prefix = textEditRange != null ? completionPrefix.getPrefixFor(textEditRange, item) : null;
if (prefix != null) {
// Add the IJ completion item (lookup item) by using the computed prefix
result.withPrefixMatcher(prefix)
.caseInsensitive()
.addElement(prioritizedLookupItem);
// Add the IJ completion item (lookup item) by using the computed prefix respecting the language's case-sensitivity
if (caseSensitive) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could condense these two conditionals slightly be extracting result.withPrefixMatcher() into a variable and setting other state conditionally, but I didn't find that quite as readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I need to revisit this addLookupItem method so please dont loose your time about it

result.withPrefixMatcher(prefix)
.addElement(prioritizedLookupItem);
} else {
result.withPrefixMatcher(prefix)
.caseInsensitive()
.addElement(prioritizedLookupItem);
}
} else {
// Should happen rarely, only when text edit is for multi-lines or if completion is triggered outside the text edit range.
// Add the IJ completion item (lookup item) which will use the IJ prefix
result.caseInsensitive()
.addElement(prioritizedLookupItem);
// Add the IJ completion item (lookup item) which will use the IJ prefix respecting the language's case-sensitivity
if (caseSensitive) {
result.addElement(prioritizedLookupItem);
} else {
result.caseInsensitive()
.addElement(prioritizedLookupItem);
}
}
}

Expand Down Expand Up @@ -278,4 +293,15 @@ public void setServerCapabilities(@Nullable ServerCapabilities serverCapabilitie
completionCapabilityRegistry.setServerCapabilities(serverCapabilities);
}
}

/**
* Determines whether or not completions for the file should be offered in a case-sensitive manner.
*
* @param file the file
* @return true if completions should be offered in a case-sensitive manner; otherwise false
*/
public boolean isCaseSensitive(@NotNull PsiFile file) {
// Default to case-insensitive
return false;
}
}
5 changes: 2 additions & 3 deletions src/main/java/com/redhat/devtools/lsp4ij/features/completion/LSPCompletionContributor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@
import com.redhat.devtools.lsp4ij.LSPIJUtils;
import com.redhat.devtools.lsp4ij.LanguageServerItem;
import com.redhat.devtools.lsp4ij.client.ExecuteLSPFeatureStatus;
import com.redhat.devtools.lsp4ij.client.indexing.ProjectIndexingManager;
import com.redhat.devtools.lsp4ij.client.features.LSPCompletionFeature;
import com.redhat.devtools.lsp4ij.client.features.LSPCompletionProposal;
import com.redhat.devtools.lsp4ij.client.indexing.ProjectIndexingManager;
import org.eclipse.lsp4j.*;
import org.eclipse.lsp4j.jsonrpc.messages.Either;
import org.jetbrains.annotations.NotNull;
Expand Down Expand Up @@ -136,7 +136,7 @@ private void addCompletionItems(@NotNull CompletionParameters parameters,
// Create lookup item
LookupElement lookupItem = completionFeature.createLookupElement(item, context);
if (lookupItem != null) {
completionFeature.addLookupItem(completionPrefix, result, lookupItem, size- i, item);
completionFeature.addLookupItem(context, completionPrefix, result, lookupItem, size - i, item);
}
}
}
Expand Down Expand Up @@ -217,5 +217,4 @@ private static final String getCompletionChar(int offset, Document document) {
}
return null;
}

}
12 changes: 10 additions & 2 deletions src/main/java/com/redhat/devtools/lsp4ij/launching/UserDefinedLanguageServerSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ public static class UserDefinedLanguageServerItemSettings {

private String initializationOptionsContent;

private String clientConfigurationContent = "{}";
Copy link
Contributor Author

@SCWells72 SCWells72 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The default value of {} here is what allows smooth backward-compatibility with existing language server definitions. I think there is an open question of whether we should try to update existing language server definitions for case-sensitive languages with a client config saying as much. I don't know if there's any kind of existing precedent for updating existing definitions with new config information...


@XCollection(elementTypes = ServerMappingSettings.class)
private List<ServerMappingSettings> mappings;

Expand Down Expand Up @@ -206,6 +208,14 @@ public String getInitializationOptionsContent() {
public void setInitializationOptionsContent(String initializationOptionsContent) {
this.initializationOptionsContent = initializationOptionsContent;
}

public String getClientConfigurationContent() {
return clientConfigurationContent;
}

public void setClientConfigurationContent(String clientConfigurationContent) {
this.clientConfigurationContent = clientConfigurationContent;
}
}

public static class MyState {
Expand All @@ -215,7 +225,5 @@ public static class MyState {

MyState() {
}

}

}
9 changes: 9 additions & 0 deletions src/main/java/com/redhat/devtools/lsp4ij/launching/templates/LanguageServerTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public String getName() {
public static final String TEMPLATE_FILE_NAME = "template.json";
public static final String INITIALIZATION_OPTIONS_FILE_NAME = "initializationOptions.json";
public static final String SETTINGS_FILE_NAME = "settings.json";
public static final String CLIENT_SETTINGS_FILE_NAME = "clientSettings.json";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the existing pattern of the server-side config file being named settings.json and naming the client-side config clientSettings.json.

public static final String README_FILE_NAME = "README.md";

public static final String LANGUAGE_ID_JSON_PROPERTY = "languageId";
Expand Down Expand Up @@ -70,6 +71,7 @@ public String getName() {

private String configuration;
private String initializationOptions;
private String clientConfiguration;

public String getName() {
return name;
Expand Down Expand Up @@ -150,4 +152,11 @@ public void setInitializationOptions(String initializationOptions) {
this.initializationOptions = initializationOptions;
}

public String getClientConfiguration() {
return clientConfiguration;
}

public void setClientConfiguration(String clientConfiguration) {
this.clientConfiguration = clientConfiguration;
}
}
Loading
Loading