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

Merged
merged 18 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
723f68f
First cut at completion sorting improvements.
SCWells72 Dec 14, 2024
373fc08
And now the prefix is also taken into account for both strict and loo…
SCWells72 Dec 14, 2024
0aaba07
More flexibility when comparing against the current word.
SCWells72 Dec 14, 2024
8ef74d0
Yet more flexibility when comparing against the current word.
SCWells72 Dec 14, 2024
43063be
Yet even more flexibility when comparing against the current word. Ho…
SCWells72 Dec 14, 2024
c115402
Current word and prefix comparisons are not performed for quoted stri…
SCWells72 Dec 14, 2024
8f2a3df
Merge branch 'redhat-developer:main' into lsp_completion_sorting
SCWells72 Dec 16, 2024
291fbad
Merge branch 'redhat-developer:main' into lsp_completion_sorting
SCWells72 Dec 17, 2024
63e6637
Merge branch 'redhat-developer:main' into lsp_completion_sorting
SCWells72 Dec 17, 2024
3316257
Merge branch 'redhat-developer:main' into lsp_completion_sorting
SCWells72 Dec 19, 2024
4edcca2
Made context-aware code completion sorting an opt-in client-side conf…
SCWells72 Dec 22, 2024
78feed2
Reverted some comment changes that should not have occurred.
SCWells72 Dec 22, 2024
7d34a8f
Added more extensive unit tests for the CompletionItemComparator's ne…
SCWells72 Jan 6, 2025
5a573b3
Addressed PR review feedback.
SCWells72 Jan 6, 2025
d2aec52
Fixed an issue where result set caching for a shorter prefix could re…
SCWells72 Jan 10, 2025
6fea7a1
This makes "useContextAwareSorting" enabled by default.
SCWells72 Jan 10, 2025
ca11ad8
Revert "This makes "useContextAwareSorting" enabled by default."
SCWells72 Jan 10, 2025
1c66029
Added "completion.useContextAwareSorting=true" to all templates so th…
SCWells72 Jan 10, 2025
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.

SCWells72 marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -260,11 +260,11 @@ public void addLookupItem(@NotNull LSPCompletionContext context,
}

/**
* Returns true if completion item must be resolved and false otherwise when completion item is applied.
* Returns true if completion item must be resolved and false otherwise when completion item is used.
*
* @param item the completion item which is applied.
* @param item the completion item which is used.
* @param file the file.
* @return true if completion item must be resolved and false otherwise when completion item is applied.
* @return true if completion item must be resolved and false otherwise when completion item is used.
*/
public boolean shouldResolveOnApply(@NotNull CompletionItem item,
@NotNull PsiFile file) {
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the new feature is now enabled by default.

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.
SCWells72 marked this conversation as resolved.
Show resolved Hide resolved
*
* 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);
SCWells72 marked this conversation as resolved.
Show resolved Hide resolved
}
}
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);
SCWells72 marked this conversation as resolved.
Show resolved Hide resolved

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