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.

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: Improved completion sorting that takes into account the current word and prefix (strict then camel-hump) #705

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public boolean isCompletionSupported(@NotNull PsiFile file) {
* @param file the file.
* @return true the file associated with a language server can support resolve completion and false otherwise.
*/
public boolean isResolveCompletionSupported(@Nullable PsiFile file) {
public boolean isResolveCompletionSupported(@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.

All callers were passing non-null values and the isResolveCompletionSupported() expects a non-null argument, so I changed the nullability annotation to match reality.

return getCompletionCapabilityRegistry().isResolveCompletionSupported(file);
}

Expand Down Expand Up @@ -182,7 +182,7 @@ public Icon getIcon(@NotNull CompletionItem item) {
/**
* Returns true if the IntelliJ lookup is strike out and false otherwise.
*
* @param item
* @param item the completion item
* @return true if the IntelliJ lookup is strike out and false otherwise.
*/
public boolean isStrikeout(@NotNull CompletionItem item) {
Expand Down Expand Up @@ -215,12 +215,12 @@ 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
* @param context the completion context
* @param completionPrefix the completion prefix
* @param result the completion result set
* @param lookupItem the lookup item
* @param priority the completion priority
* @param item the completion item
*/
@ApiStatus.Internal
public void addLookupItem(@NotNull LSPCompletionContext context,
Expand Down Expand Up @@ -293,4 +293,15 @@ public void setServerCapabilities(@Nullable ServerCapabilities serverCapabilitie
completionCapabilityRegistry.setServerCapabilities(serverCapabilities);
}
}

/**
* Determines whether or not client-side context-aware completion sorting should be used for the specified file.
*
* @param file the file
* @return true if client-side context-aware completion sorting should be used; otherwise false
*/
public boolean useContextAwareSorting(@NotNull PsiFile file) {
// Default to disabled
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,30 @@
******************************************************************************/
package com.redhat.devtools.lsp4ij.features.completion;

import java.util.Comparator;

import com.intellij.codeInsight.completion.PrefixMatcher;
import com.intellij.openapi.util.text.StringUtil;
import org.eclipse.lsp4j.CompletionItem;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.util.Comparator;

/**
* Compares {@link CompletionItem}s by their sortText property (falls back to comparing labels)
*/
public class CompletionItemComparator implements Comparator<CompletionItem> {
private final PrefixMatcher prefixMatcher;
private final String currentWord;
private final boolean caseSensitive;

public CompletionItemComparator(@Nullable PrefixMatcher prefixMatcher,
@Nullable String currentWord,
boolean caseSensitive) {
this.prefixMatcher = prefixMatcher;
this.currentWord = currentWord;
this.caseSensitive = caseSensitive;
}

@Override
public int compare(CompletionItem item1, CompletionItem item2) {
if (item1 == item2) {
Expand All @@ -29,24 +44,100 @@ public int compare(CompletionItem item1, CompletionItem item2) {
return 1;
}

int comparison = compareNullable(item1.getSortText(), item2.getSortText());
// If one is a better match for the current word than the other, sort it higher
int comparison = compareAgainstCurrentWord(item1, item2);
if (comparison != 0) {
return comparison;
}

// If one is a better completion for the current prefix than the other, sort it higher
comparison = compareAgainstPrefix(item1, item2);
if (comparison != 0) {
return comparison;
}

// Order by language server-provided sort text
comparison = compare(item1.getSortText(), item2.getSortText());
if (comparison != 0) {
return comparison;
}

// If sortText is equal, fall back to comparing labels
if (comparison == 0) {
comparison = item1.getLabel().compareTo(item2.getLabel());
return compare(item1.getLabel(), item2.getLabel());
}

private int compare(@Nullable String string1, @Nullable String string2) {
return StringUtil.compare(string1, string2, !caseSensitive);
}

private boolean equals(@Nullable String string1, @Nullable String string2) {
return StringUtil.compare(string1, string2, !caseSensitive) == 0;
}

private boolean startsWith(@Nullable String string, @Nullable String prefix) {
if ((string == null) || (prefix == null)) {
return false;
}
return caseSensitive ? StringUtil.startsWith(string, prefix) : StringUtil.startsWithIgnoreCase(string, prefix);
}

private int compareAgainstCurrentWord(@NotNull CompletionItem item1, @NotNull CompletionItem item2) {
if (currentWord != null) {
String label1 = item1.getLabel();
String label2 = item2.getLabel();
// Don't do this for completion offerings that are quoted strings
if (((label1 == null) || !StringUtil.isQuotedString(label1)) &&
((label2 == null) || !StringUtil.isQuotedString(label2))) {
// Exact match
if (equals(currentWord, label1) &&
((label2 == null) || !equals(currentWord, label2))) {
return -1;
} else if (equals(currentWord, label2) &&
((label1 == null) || !equals(currentWord, label1))) {
return 1;
}

return comparison;
// Starts with
else if ((startsWith(currentWord, label1) || startsWith(label1, currentWord)) &&
((label2 == null) || !(startsWith(currentWord, label2) || startsWith(label2, currentWord)))) {
return -1;
} else if ((startsWith(currentWord, label2) || startsWith(label2, currentWord)) &&
((label1 == null) || !(startsWith(currentWord, label1) || startsWith(label1, currentWord)))) {
return 1;
}
}
}

return 0;
}

private int compareNullable(@Nullable String s1, @Nullable String s2) {
if (s1 == s2) {
return 0;
} else if (s1 == null) {
return -1;
} else if (s2 == null) {
return 1;
private int compareAgainstPrefix(@NotNull CompletionItem item1, @NotNull CompletionItem item2) {
if (prefixMatcher != null) {
String prefix = prefixMatcher.getPrefix();
String label1 = item1.getLabel();
String label2 = item2.getLabel();
// Don't do this for completion offerings that are quoted strings
if (((label1 == null) || !StringUtil.isQuotedString(label1)) &&
((label2 == null) || !StringUtil.isQuotedString(label2))) {
// Start starts with
if (startsWith(label1, prefix) &&
(!startsWith(label2, prefix))) {
return -1;
} else if (startsWith(label2, prefix) &&
(!startsWith(label1, prefix))) {
return 1;
}
// Loose/camel-hump starts with
else if ((label1 != null) && prefixMatcher.isStartMatch(label1) &&
((label2 == null) || !prefixMatcher.isStartMatch(label2))) {
return -1;
} else if ((label2 != null) && prefixMatcher.isStartMatch(label2) &&
((label1 == null) || !prefixMatcher.isStartMatch(label1))) {
return 1;
}
}
}
return s1.compareTo(s2);

return 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.intellij.openapi.editor.Editor;
import com.intellij.openapi.progress.ProcessCanceledException;
import com.intellij.openapi.progress.ProgressManager;
import com.intellij.openapi.util.TextRange;
import com.intellij.openapi.util.UserDataHolder;
import com.intellij.openapi.vfs.VirtualFile;
import com.intellij.psi.PsiFile;
Expand All @@ -25,6 +26,7 @@
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.features.LSPClientFeatures;
import com.redhat.devtools.lsp4ij.client.features.LSPCompletionFeature;
import com.redhat.devtools.lsp4ij.client.features.LSPCompletionProposal;
import com.redhat.devtools.lsp4ij.client.indexing.ProjectIndexingManager;
Expand Down Expand Up @@ -102,8 +104,6 @@ public void fillCompletionVariants(@NotNull CompletionParameters parameters, @No
}
}

private static final CompletionItemComparator completionProposalComparator = new CompletionItemComparator();

private void addCompletionItems(@NotNull CompletionParameters parameters,
@NotNull CompletionPrefix completionPrefix,
@NotNull Either<List<CompletionItem>, CompletionList> completion,
Expand All @@ -119,12 +119,19 @@ private void addCompletionItems(@NotNull CompletionParameters parameters,
items.addAll(completionList.getItems());
}

// Sort by item.sortText
items.sort(completionProposalComparator);
PsiFile originalFile = parameters.getOriginalFile();
LSPClientFeatures clientFeatures = languageServer.getClientFeatures();

// Sort the completions as appropriate based on client configuration
boolean useContextAwareSorting = clientFeatures.getCompletionFeature().useContextAwareSorting(originalFile);
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 new features are now opt-in. When disabled (which is the default), the only difference vs. prior behavior should be properly respecting the language case-sensitivity config setting.

PrefixMatcher prefixMatcher = useContextAwareSorting ? result.getPrefixMatcher() : null;
String currentWord = useContextAwareSorting ? getCurrentWord(parameters) : null;
boolean caseSensitive = clientFeatures.isCaseSensitive(originalFile);
items.sort(new CompletionItemComparator(prefixMatcher, currentWord, caseSensitive));
int size = items.size();

Set<String> addedLookupStrings = new HashSet<>();
var completionFeature = languageServer.getClientFeatures().getCompletionFeature();
var completionFeature = clientFeatures.getCompletionFeature();
LSPCompletionFeature.LSPCompletionContext context = new LSPCompletionFeature.LSPCompletionContext(parameters, languageServer);
// Items now sorted by priority, low index == high priority
for (int i = 0; i < size; i++) {
Expand Down Expand Up @@ -186,6 +193,23 @@ private void addCompletionItems(@NotNull CompletionParameters parameters,
}
}

@Nullable
private static String getCurrentWord(@NotNull CompletionParameters parameters) {
PsiFile originalFile = parameters.getOriginalFile();
VirtualFile virtualFile = originalFile.getVirtualFile();
Document document = virtualFile != null ? LSPIJUtils.getDocument(virtualFile) : null;
if (document != null) {
int offset = parameters.getOffset();
TextRange wordTextRange = LSPIJUtils.getWordRangeAt(document, originalFile, offset);
if (wordTextRange != null) {
CharSequence documentChars = document.getCharsSequence();
CharSequence wordChars = documentChars.subSequence(wordTextRange.getStartOffset(), wordTextRange.getEndOffset());
return wordChars.toString();
}
}
return null;
}

protected void updateWithItemDefaults(@NotNull CompletionItem item,
@Nullable CompletionItemDefaults itemDefaults) {
if (itemDefaults == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,17 @@
*/
public class ClientConfigurationSettings {
/**
* Client-side code workspace symbol settings.
* Client-side code completion settings.
*/
public static class ClientConfigurationCompletionSettings {
/**
* Whether or not client-side context-aware completion sorting should be used. Defaults to false.
*/
public boolean useContextAwareSorting = false;
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 opt-in config option.

}

/**
* Client-side workspace symbol settings.
*/
public static class ClientConfigurationWorkspaceSymbolSettings {
/**
Expand All @@ -35,7 +45,12 @@ public static class ClientConfigurationWorkspaceSymbolSettings {
public boolean caseSensitive = false;

/**
* Client-side code workspace symbol settings
* Client-side code completion settings
*/
public @NotNull ClientConfigurationCompletionSettings completion = new ClientConfigurationCompletionSettings();

/**
* Client-side workspace symbol settings
*/
public @NotNull ClientConfigurationWorkspaceSymbolSettings workspaceSymbol = new ClientConfigurationWorkspaceSymbolSettings();
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public UserDefinedClientFeatures() {
super();

// Use the extended feature implementations
setCompletionFeature(new UserDefinedCompletionFeature());
setWorkspaceSymbolFeature(new UserDefinedWorkspaceSymbolFeature());
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*******************************************************************************
* Copyright (c) 2024 Red Hat Inc. and others.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
* which is available at https://www.apache.org/licenses/LICENSE-2.0.
*
* SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
*
* Contributors:
* Red Hat Inc. - initial API and implementation
*******************************************************************************/
package com.redhat.devtools.lsp4ij.server.definition.launching;

import com.intellij.psi.PsiFile;
import com.redhat.devtools.lsp4ij.client.features.LSPCompletionFeature;
import org.jetbrains.annotations.NotNull;

/**
* Adds client-side code completion configuration features.
*/
public class UserDefinedCompletionFeature extends LSPCompletionFeature {

@Override
public boolean useContextAwareSorting(@NotNull PsiFile file) {
UserDefinedLanguageServerDefinition serverDefinition = (UserDefinedLanguageServerDefinition) getClientFeatures().getServerDefinition();
ClientConfigurationSettings clientConfiguration = serverDefinition.getLanguageServerClientConfiguration();
return clientConfiguration != null ? clientConfiguration.completion.useContextAwareSorting : super.useContextAwareSorting(file);
}
}
13 changes: 13 additions & 0 deletions src/main/resources/jsonSchema/clientSettings.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,19 @@
"description": "Whether or not the language grammar is case-sensitive.",
"default": false
},
"completion": {
"type": "object",
"title": "Client-side completion configuration",
"additionalProperties": false,
"properties": {
"useContextAwareSorting": {
"type": "boolean",
"title": "Use context-aware completion sorting",
"description": "Whether or not client-side context-aware completion sorting should be used.",
"default": false
}
}
},
"workspaceSymbol": {
"type": "object",
"title": "Client-side workspace symbol configuration",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
import java.util.ArrayList;
import java.util.List;

import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class CompletionItemComparatorTest {
private final CompletionItemComparator comparator = new CompletionItemComparator();
private final CompletionItemComparator comparator = new CompletionItemComparator(null, null, false);
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'll add more test cases here to confirm proper behavior of the new context-aware features of this comparator.


private final CompletionItem one = newItem("one", "1");
private final CompletionItem nil = newItem("", null);
Expand Down
Loading