diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/CompletionOrderingTests.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/CompletionOrderingTests.java index fdf61e9ae..5dabeca45 100644 --- a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/CompletionOrderingTests.java +++ b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/CompletionOrderingTests.java @@ -150,6 +150,21 @@ public void testOrderWithLongInsert() throws Exception { 62, orderedResults); } + @Test + public void testSortTextIsComparedLexicographically() throws Exception { + final var completions = new ArrayList(); + + final var item15 = createCompletionItem("15", CompletionItemKind.Class); + item15.setSortText("15"); + completions.add(item15); + + final var item5 = createCompletionItem("5", CompletionItemKind.Class); + item5.setSortText("5"); + completions.add(item5); + + confirmCompletionResults(completions, "", 0, new String[] { "15", "5" }); + } + @Test public void testMovingOffset() throws Exception { final var range = new Range(new Position(0, 0), new Position(0, 4)); diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/LSCompletionProposalComparatorTest.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/LSCompletionProposalComparatorTest.java new file mode 100644 index 000000000..e09b8e45a --- /dev/null +++ b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/LSCompletionProposalComparatorTest.java @@ -0,0 +1,184 @@ +/******************************************************************************* + * Copyright (c) 2025 Vegard IT GmbH and others. + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Sebastian Thomschke (Vegard IT GmbH) - initial implementation + *******************************************************************************/ +package org.eclipse.lsp4e.test.completion; + +import static org.junit.jupiter.api.Assertions.*; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; + +import org.eclipse.jface.text.Document; +import org.eclipse.jface.text.IDocument; +import org.eclipse.lsp4e.operations.completion.LSCompletionProposal; +import org.eclipse.lsp4e.operations.completion.LSCompletionProposalComparator; +import org.eclipse.lsp4j.CompletionItem; +import org.junit.jupiter.api.Test; + +public class LSCompletionProposalComparatorTest { + + /** + * DocumentFilter length currently has top priority in LSCompletionProposalComparator. + * This test encodes the expectation that, once proposals are considered matching, + * sortText should decide the order, not the document filter length. + */ + @Test + public void testDocumentFilterLengthDoesNotOverrideSortText() { + IDocument document = new Document(""); + + final var item1 = new CompletionItem("p1"); + item1.setSortText("B"); + final var item2 = new CompletionItem("p2"); + item2.setSortText("A"); + + final var proposalWithLongFilter = new StubProposal(document, item1, "longFilter"); + final var proposalWithShortFilter = new StubProposal(document, item2, "x"); + + final var comparator = new LSCompletionProposalComparator(); + final var proposals = new ArrayList(); + proposals.add(proposalWithLongFilter); + proposals.add(proposalWithShortFilter); + + proposals.sort(comparator); + + // Desired order: sortText ascending -> "A", then "B" + assertEquals("A", proposals.get(0).getSortText()); + assertEquals("B", proposals.get(1).getSortText()); + } + + /** + * CompletionProposalPopup.computeFilteredProposals filters an already sorted + * list of proposals. To avoid surprising reordering if re-sorting is added + * later, LSCompletionProposalComparator must not change the relative order + * of proposals solely because their documentFilter length changes. + * + * This test verifies that changing the documentFilter does not change the + * comparator outcome for otherwise identical proposals. + */ + @Test + public void testFilteredProposalsShouldBeResortedWhenFilterChanges() { + IDocument document = new Document(""); + + final var item1 = new CompletionItem("p1"); + final var item2 = new CompletionItem("p2"); + + final int initialOffset = 1; + final int updatedOffset = 2; + + final var proposalA = new VarFilterProposal(document, item1); + final var proposalB = new VarFilterProposal(document, item2); + + // At the initial offset, proposalA has a longer filter than proposalB + // so A should be ordered before B. + proposalA.setFilterForOffset(initialOffset, "xx"); + proposalB.setFilterForOffset(initialOffset, "x"); + + // After typing more, the filters swap lengths so the desired order + // (if resorted) would become B before A. + proposalA.setFilterForOffset(updatedOffset, "x"); + proposalB.setFilterForOffset(updatedOffset, "xx"); + + final var comparator = new LSCompletionProposalComparator(); + + // Initial sort at invocation offset + final var initiallySorted = new ArrayList(); + initiallySorted.add(proposalA); + initiallySorted.add(proposalB); + proposalA.setOffsetForSorting(initialOffset); + proposalB.setOffsetForSorting(initialOffset); + initiallySorted.sort(comparator); + + // Simulate JFace behaviour: keep the original order and just filter + final var filteredWithoutResort = new ArrayList<>(initiallySorted); + + // Expected behaviour: update filters for the new offset and resort + final var expectedResorted = new ArrayList(); + expectedResorted.add(proposalA); + expectedResorted.add(proposalB); + proposalA.setOffsetForSorting(updatedOffset); + proposalB.setOffsetForSorting(updatedOffset); + expectedResorted.sort(comparator); + + // We would like the filtered list to reflect the re-sorted order. + assertEquals(expectedResorted, filteredWithoutResort); + } + + private static class StubProposal extends LSCompletionProposal { + + private final String filter; + + StubProposal(IDocument document, CompletionItem item, String filter) { + super(document, 0, item, null, null, false); + this.filter = filter; + } + + @Override + public String getDocumentFilter() { + return filter; + } + + @Override + public String getDocumentFilter(int offset) { + return filter; + } + + @Override + public int getRankCategory() { + return 5; + } + + @Override + public int getRankScore() { + return 0; + } + } + + private static class VarFilterProposal extends LSCompletionProposal { + + private final Map filtersByOffset = new HashMap<>(); + private int sortOffset; + + VarFilterProposal(IDocument document, CompletionItem item) { + super(document, 0, item, null, null, false); + } + + void setFilterForOffset(int offset, String filter) { + filtersByOffset.put(Integer.valueOf(offset), filter); + } + + void setOffsetForSorting(int offset) { + this.sortOffset = offset; + } + + @Override + public String getDocumentFilter() { + String filter = filtersByOffset.get(Integer.valueOf(sortOffset)); + return filter != null ? filter : ""; + } + + @Override + public String getDocumentFilter(int offset) { + String filter = filtersByOffset.get(Integer.valueOf(offset)); + return filter != null ? filter : ""; + } + + @Override + public int getRankCategory() { + return 5; + } + + @Override + public int getRankScore() { + return 0; + } + } +} diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/CompletionProposalTools.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/CompletionProposalTools.java index 0d4413e14..5dffbb750 100644 --- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/CompletionProposalTools.java +++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/CompletionProposalTools.java @@ -20,6 +20,12 @@ private CompletionProposalTools() { // to avoid instances, requested by sonar } + /** + * Category used when the document filter does not meaningfully match + * the completion filter (catch-all / no-match case). + */ + public static final int CATEGORY_NO_MATCH = 5; + /** * The portion of the document leading up to the cursor that is being used as a * filter for requesting completion assist @@ -101,9 +107,9 @@ public static int getCategoryOfFilterMatch(String documentFilter, String complet documentFilter = documentFilter.toLowerCase(); completionFilter = completionFilter.toLowerCase(); int subIndex = completionFilter.indexOf(documentFilter); - int topCategory = 5; + int topCategory = CATEGORY_NO_MATCH; if (subIndex == -1) { - return isSubstringFoundOrderedInString(documentFilter, completionFilter) ? 4 : 5; + return isSubstringFoundOrderedInString(documentFilter, completionFilter) ? 4 : CATEGORY_NO_MATCH; } final int documentFilterLength = documentFilter.length(); final int completionFilterLength = completionFilter.length(); diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposal.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposal.java index a29117706..442eef939 100644 --- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposal.java +++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposal.java @@ -258,7 +258,7 @@ public int getRankCategory() { getFilterString()); } catch (BadLocationException e) { LanguageServerPlugin.logError(e); - rankCategory = 5; + rankCategory = CompletionProposalTools.CATEGORY_NO_MATCH; } this.rankCategory = rankCategory; return rankCategory; diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposalComparator.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposalComparator.java index d8036ad66..eb31be4f4 100644 --- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposalComparator.java +++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposalComparator.java @@ -13,43 +13,60 @@ *******************************************************************************/ package org.eclipse.lsp4e.operations.completion; +import static org.eclipse.lsp4e.operations.completion.CompletionProposalTools.*; + import java.util.Comparator; import org.eclipse.jface.text.BadLocationException; import org.eclipse.lsp4e.LanguageServerPlugin; -final class LSCompletionProposalComparator implements Comparator { +public final class LSCompletionProposalComparator implements Comparator { @Override public int compare(LSCompletionProposal o1, LSCompletionProposal o2) { - try { - int docFilterLen1 = o1.getDocumentFilter().length(); - int docFilterLen2 = o2.getDocumentFilter().length(); - if (docFilterLen1 > docFilterLen2) { - return -1; - } else if (docFilterLen1 < docFilterLen2) { - return +1; + int category1 = o1.getRankCategory(); + int category2 = o2.getRankCategory(); + + // Prefer proposals that have a stronger match (categories 1-4), + // but use the document filter length only for those categories so that + // completely unmatched proposals (category CATEGORY_NO_MATCH) are not affected. + if (category1 < CATEGORY_NO_MATCH && category2 < CATEGORY_NO_MATCH) { + try { + int docFilterLen1 = o1.getDocumentFilter().length(); + int docFilterLen2 = o2.getDocumentFilter().length(); + if (docFilterLen1 > docFilterLen2) { + return -1; + } else if (docFilterLen1 < docFilterLen2) { + return 1; + } + } catch (BadLocationException e) { + LanguageServerPlugin.logError(e); } - } catch (BadLocationException e) { - LanguageServerPlugin.logError(e); } - if (o1.getRankCategory() < o2.getRankCategory()) { + + if (category1 < category2) { return -1; - } else if (o1.getRankCategory() > o2.getRankCategory()) { - return +1; + } else if (category1 > category2) { + return 1; } - if ((o1.getRankCategory() < 5 && o2.getRankCategory() < 5) - && (!(o1.getRankScore() == -1 && o2.getRankScore() == -1))) { - if (o2.getRankScore() == -1 || o1.getRankScore() < o2.getRankScore()) { - return -1; - } else if (o1.getRankScore() == -1 || o1.getRankScore() > o2.getRankScore()) { - return +1; + + if (category1 < CATEGORY_NO_MATCH /* && category2 < CATEGORY_NO_MATCH */) { + int score1 = o1.getRankScore(); + int score2 = o2.getRankScore(); + if (!(score1 == -1 && score2 == -1)) { + if (score1 == -1) { + return 1; + } else if (score2 == -1) { + return -1; + } else if (score1 < score2) { + return -1; + } else if (score1 > score2) { + return 1; + } } } + String c1 = o1.getSortText(); String c2 = o2.getSortText(); - if (c1 == null) { - return -1; - } return c1.compareToIgnoreCase(c2); } -} \ No newline at end of file +}