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 2 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
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
27 changes: 19 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 @@ -220,27 +220,38 @@ public boolean isItemTextBold(@NotNull CompletionItem item) {
* @param lookupItem
* @param priority
* @param item
* @param caseSensitive
*/
@ApiStatus.Internal
public void addLookupItem(@NotNull CompletionPrefix completionPrefix,
@NotNull CompletionResultSet result,
@NotNull LookupElement lookupItem,
int priority,
@NotNull CompletionItem item) {
@NotNull CompletionItem item,
boolean caseSensitive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it is internal, please try to avoid updating the signature. The caseSensitive can be get inside the method.

Copy link
Contributor Author

@SCWells72 SCWells72 Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Disregard the previous comment. Once I migrated stuff into client features it all made more sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need server definition, you must work with LSPCompeltionfeatures#caseIsensitive(PsiFile file)

The idea is that UserDefinedLanguageServerDefinition will return a custom class UserDefinedCompletionFeatures which impelemnts caseIsensitive(PsiFile file)

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
23 changes: 21 additions & 2 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,12 @@
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 com.redhat.devtools.lsp4ij.server.definition.LanguageServerDefinition;
import com.redhat.devtools.lsp4ij.server.definition.launching.ClientConfigurationLanguageSettings;
import com.redhat.devtools.lsp4ij.server.definition.launching.UserDefinedLanguageServerDefinition;
import org.eclipse.lsp4j.*;
import org.eclipse.lsp4j.jsonrpc.messages.Either;
import org.jetbrains.annotations.NotNull;
Expand All @@ -38,6 +41,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CancellationException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -125,6 +129,9 @@ private void addCompletionItems(@NotNull CompletionParameters parameters,
items.sort(completionProposalComparator);
int size = items.size();

// Determine whether or not the language is case-sensitive so that we can add completions accordingly
boolean caseSensitive = isCaseSensitive(languageServer, parameters.getOriginalFile());
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with IJ, I would rename with isCaseInsensitive


var completionFeature = languageServer.getClientFeatures().getCompletionFeature();
LSPCompletionFeature.LSPCompletionContext context = new LSPCompletionFeature.LSPCompletionContext(parameters, languageServer);
// Items now sorted by priority, low index == high priority
Expand All @@ -136,11 +143,23 @@ 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(completionPrefix, result, lookupItem, size - i, item, caseSensitive);
}
}
}

private static boolean isCaseSensitive(@NotNull LanguageServerItem languageServer, @NotNull PsiFile file) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be pretty straightforward, but basically try to infer language grammar case-sensitivity via the file => language ID => client-side settings.

LanguageServerDefinition serverDefinition = languageServer.getServerDefinition();
if (serverDefinition instanceof UserDefinedLanguageServerDefinition languageServerDefinition) {
Map<String, ClientConfigurationLanguageSettings> clientConfiguration = languageServerDefinition.getLanguageServerClientConfiguration();
Copy link
Contributor

@angelozerr angelozerr Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This code must not be here but in the custom implementation for user defined of ClientFeatures which will register a custom CompletionFeatures and implement the new method isCaseInsensitive(PsiFile)

String languageId = serverDefinition.getLanguageIdOrNull(file);
ClientConfigurationLanguageSettings languageSettings = (clientConfiguration != null) && (languageId != null) ? clientConfiguration.get(languageId) : null;
return (languageSettings != null) && languageSettings.caseSensitive;
}
// Default to case-insensitive if unspecified for backward-compatibility
return false;
}

protected void updateWithItemDefaults(@NotNull CompletionItem item,
@Nullable CompletionItemDefaults itemDefaults) {
if (itemDefaults == null) {
Expand Down
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;
}
}
39 changes: 28 additions & 11 deletions ...in/java/com/redhat/devtools/lsp4ij/launching/templates/LanguageServerTemplateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,26 @@
import com.intellij.openapi.vfs.JarFileSystem;
import com.intellij.openapi.vfs.VfsUtilCore;
import com.intellij.openapi.vfs.VirtualFile;
import com.redhat.devtools.lsp4ij.internal.StringUtils;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many of the changes to this file are from just running the Java formatter using the project code style.

import com.redhat.devtools.lsp4ij.server.definition.LanguageServerDefinition;
import com.redhat.devtools.lsp4ij.server.definition.launching.UserDefinedLanguageServerDefinition;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.redhat.devtools.lsp4ij.internal.StringUtils;
import com.redhat.devtools.lsp4ij.server.definition.launching.UserDefinedLanguageServerDefinition;

import java.io.*;
import java.io.ByteArrayOutputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.util.AbstractMap.SimpleEntry;
import java.util.ArrayList;
import java.util.List;
import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;
import java.nio.charset.StandardCharsets;
import java.util.List;

import static com.redhat.devtools.lsp4ij.launching.templates.LanguageServerTemplate.*;

Expand Down Expand Up @@ -82,21 +83,22 @@ private LanguageServerTemplateManager() {

/**
* Load the resources/templates file as a VirtualFile from the jar
*
* @return template root as virtual file or null, if one couldn't be found
*/
@Nullable
public VirtualFile getTemplateRoot() {
URL url = LanguageServerTemplateManager.class.getClassLoader().getResource(TEMPLATES_DIR);
if (url == null) {
LOGGER.warn("No "+TEMPLATES_DIR+ " directory/url found");
LOGGER.warn("No " + TEMPLATES_DIR + " directory/url found");
return null;
}
try {
// url looks like jar:file:/Users/username/Library/Application%20Support/JetBrains/IDEVersion/plugins/LSP4IJ/lib/instrumented-lsp4ij-version.jar!/templates
String filePart = url.toURI().getRawSchemeSpecificPart(); // get un-decoded, URI compatible part
// filePart looks like file:/Users/username/Library/Application%20Support/JetBrains/IDEVersion/plugins/LSP4IJ/lib/instrumented-lsp4ij-version.jar!/templates
LOGGER.debug("Templates filePart : {}", filePart);
String resourcePath = new URI(filePart).getSchemeSpecificPart();// get decoded part (i.e. converts %20 to spaces ...)
String resourcePath = new URI(filePart).getSchemeSpecificPart();// get decoded part (i.e. converts %20 to spaces ...)
// resourcePath looks like /Users/username/Library/Application Support/JetBrains/IDEVersion/plugins/LSP4IJ/lib/instrumented-lsp4ij-version.jar!/templates/
LOGGER.debug("Templates resources path from uri : {}", resourcePath);
return JarFileSystem.getInstance().findFileByPath(resourcePath);
Expand All @@ -112,6 +114,7 @@ public List<LanguageServerTemplate> getTemplates() {

/**
* Import language server template from a directory
*
* @param templateFolder directory that contains the template files
* @return LanguageServerTemplate or null if one couldn't be created
*/
Expand All @@ -122,6 +125,7 @@ public LanguageServerTemplate importLsTemplate(@NotNull VirtualFile templateFold

/**
* Parses the template files to create a LanguageServerTemplate
*
* @param templateFolder directory that contains the template files
* @return LanguageServerTemplate or null if one couldn't be created
* @throws IOException if an IO error occurs when loading the text from any template file
Expand All @@ -131,6 +135,7 @@ public LanguageServerTemplate createLsTemplate(@NotNull VirtualFile templateFold
String templateJson = null;
String settingsJson = null;
String initializationOptionsJson = null;
String clientSettingsJson = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The real changes in this file begin here where we add handling for clientSettings.json.

String description = null;

for (VirtualFile file : templateFolder.getChildren()) {
Expand All @@ -147,6 +152,9 @@ public LanguageServerTemplate createLsTemplate(@NotNull VirtualFile templateFold
case INITIALIZATION_OPTIONS_FILE_NAME:
initializationOptionsJson = VfsUtilCore.loadText(file);
break;
case CLIENT_SETTINGS_FILE_NAME:
clientSettingsJson = VfsUtilCore.loadText(file);
break;
case README_FILE_NAME:
description = VfsUtilCore.loadText(file);
break;
Expand All @@ -165,6 +173,9 @@ public LanguageServerTemplate createLsTemplate(@NotNull VirtualFile templateFold
if (initializationOptionsJson == null) {
initializationOptionsJson = "{}";
}
if (clientSettingsJson == null) {
clientSettingsJson = "{}";
}

GsonBuilder builder = new GsonBuilder();
builder.registerTypeAdapter(LanguageServerTemplate.class, new LanguageServerTemplateDeserializer());
Expand All @@ -173,6 +184,7 @@ public LanguageServerTemplate createLsTemplate(@NotNull VirtualFile templateFold
LanguageServerTemplate template = gson.fromJson(templateJson, LanguageServerTemplate.class);
template.setConfiguration(settingsJson);
template.setInitializationOptions(initializationOptionsJson);
template.setClientConfiguration(clientSettingsJson);
if (StringUtils.isNotBlank(description)) {
template.setDescription(description);
}
Expand All @@ -182,7 +194,8 @@ public LanguageServerTemplate createLsTemplate(@NotNull VirtualFile templateFold

/**
* Exports one or more language server templates to a zip file
* @param exportZip target zip
*
* @param exportZip target zip
* @param lsDefinitions to export
*/
public int exportLsTemplates(@NotNull VirtualFile exportZip, @NotNull List<LanguageServerDefinition> lsDefinitions) {
Expand All @@ -200,6 +213,7 @@ public int exportLsTemplates(@NotNull VirtualFile exportZip, @NotNull List<Langu

/**
* Creates a zip file by handling each user defined language server definitions
*
* @return zip file as a byte array
* @throws IOException if an IO error occurs when writing to the zip file
*/
Expand All @@ -216,11 +230,13 @@ private SimpleEntry<Integer, byte[]> createZipFromLanguageServers(@NotNull List<
String template = gson.toJson(lsDefinition);
String initializationOptions = ((UserDefinedLanguageServerDefinition) lsDefinition).getInitializationOptionsContent();
String settings = ((UserDefinedLanguageServerDefinition) lsDefinition).getConfigurationContent();
String clientSettings = ((UserDefinedLanguageServerDefinition) lsDefinition).getClientConfigurationContent();
lsName = lsDefinition.getDisplayName();

writeToZip(TEMPLATE_FILE_NAME, template, zos);
writeToZip(INITIALIZATION_OPTIONS_FILE_NAME, initializationOptions, zos);
writeToZip(SETTINGS_FILE_NAME, settings, zos);
writeToZip(CLIENT_SETTINGS_FILE_NAME, clientSettings, zos);
zos.closeEntry();
count++;
}
Expand All @@ -231,9 +247,10 @@ private SimpleEntry<Integer, byte[]> createZipFromLanguageServers(@NotNull List<

/**
* Writes a file (name + content) to a zip output stream
*
* @param filename name of the file to write
* @param content file content
* @param zos to write the file to
* @param content file content
* @param zos to write the file to
*/
private void writeToZip(String filename, String content, ZipOutputStream zos) throws IOException {
if (StringUtils.isBlank(content)) {
Expand Down
Loading
Loading