diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessContents.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessContents.java index c7590d06739..df92f8f7848 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessContents.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessContents.java @@ -21,8 +21,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedList; @@ -30,7 +30,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; -import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.regex.Matcher; @@ -130,6 +129,15 @@ public abstract class QuickAccessContents { private TriggerSequence keySequence; private Job computeProposalsJob; + /** + * Orders entries by descending relevance score, then match quality. Applied + * with a stable sort so each provider's natural order (recency for previous + * picks, alphabetical otherwise) is preserved on ties. + */ + private static final Comparator BY_RELEVANCE = Comparator + .comparingInt(QuickAccessEntry::getMatchScore).reversed() + .thenComparingInt(QuickAccessEntry::getMatchQuality); + public QuickAccessContents(QuickAccessProvider[] providers) { this.providers = providers; } @@ -447,9 +455,6 @@ public IStatus runInUIThread(IProgressMonitor monitor) { elementsToProviders.put(element, provider); } } - if (!filter.isEmpty() && !sortedElements.isEmpty()) { - sortedElements = putPrefixMatchFirst(sortedElements, filter); - } elementsForProviders.put(provider, new ArrayList<>(sortedElements)); } } @@ -482,74 +487,55 @@ public IStatus runInUIThread(IProgressMonitor monitor) { } } } + QuickAccessMatcher matcher = new QuickAccessMatcher(); LinkedHashMap> entriesPerProvider = new LinkedHashMap<>( elementsForProviders.size()); if (showAllMatches) { - // Map elements to entries + // Map elements to entries, most relevant first within each provider for (Entry> elementsPerProvider : elementsForProviders .entrySet()) { QuickAccessProvider provider = elementsPerProvider.getKey(); List entries = elementsPerProvider.getValue().stream() // - .map(QuickAccessMatcher::new) // - .map(matcher -> matcher.match(finalFilter, provider)) // + .map(element -> matcher.match(element, finalFilter, provider)) // .filter(Objects::nonNull) // + .sorted(BY_RELEVANCE) // .collect(Collectors.toList()); if (!entries.isEmpty()) { entriesPerProvider.put(provider, entries); } } } else { - int numberOfSlotsLeft = perfectMatch != null ? maxNumberOfItemsInTable -1 : maxNumberOfItemsInTable; - while (!elementsForProviders.isEmpty() && numberOfSlotsLeft > 0) { - int nbEntriesPerProvider = numberOfSlotsLeft / elementsForProviders.size(); - if (nbEntriesPerProvider > 0) { - for (Entry> elementsPerProvider : elementsForProviders - .entrySet()) { - QuickAccessProvider provider = elementsPerProvider.getKey(); - List elements = elementsPerProvider.getValue(); - int toPickEntries = nbEntriesPerProvider; - while (toPickEntries > 0 && !elements.isEmpty()) { - QuickAccessElement element = elements.remove(0); - QuickAccessEntry entry = new QuickAccessMatcher(element).match(filter, provider); - if (entry != null) { - numberOfSlotsLeft--; - toPickEntries--; - if (!entriesPerProvider.containsKey(provider)) { - entriesPerProvider.put(provider, new LinkedList<>()); - } - entriesPerProvider.get(provider).add(entry); - } - } + int numberOfSlotsLeft = perfectMatch != null ? maxNumberOfItemsInTable - 1 : maxNumberOfItemsInTable; + // Score every candidate and keep the globally highest-ranked entries, so a + // strong match wins a slot regardless of which provider produced it + List matched = new ArrayList<>(); + for (Entry> elementsPerProvider : elementsForProviders + .entrySet()) { + if (aMonitor.isCanceled()) { + break; + } + QuickAccessProvider provider = elementsPerProvider.getKey(); + for (QuickAccessElement element : elementsPerProvider.getValue()) { + if (element == perfectMatch) { + continue; } - } else { - for (Entry> elementsForProvider : elementsForProviders - .entrySet()) { - if (numberOfSlotsLeft > 0) { - QuickAccessProvider provider = elementsForProvider.getKey(); - List elements = elementsForProvider.getValue(); - boolean entryPicked = false; - while (!entryPicked && !elements.isEmpty()) { - QuickAccessElement element = elements.remove(0); - QuickAccessEntry entry = new QuickAccessMatcher(element).match(filter, provider); - if (entry != null) { - numberOfSlotsLeft--; - entryPicked = true; - if (!entriesPerProvider.containsKey(provider)) { - entriesPerProvider.put(provider, new LinkedList<>()); - } - entriesPerProvider.get(provider).add(entry); - } - } - } + QuickAccessEntry entry = matcher.match(element, finalFilter, provider); + if (entry != null) { + matched.add(entry); } } - Set exhaustedProviders = new HashSet<>(); - elementsForProviders.forEach((provider, elements) -> { - if (elements.isEmpty()) { - exhaustedProviders.add(provider); - } - }); - exhaustedProviders.forEach(elementsForProviders::remove); + } + matched.sort(BY_RELEVANCE); + int slots = Math.max(0, numberOfSlotsLeft); + List winners = matched.subList(0, Math.min(slots, matched.size())); + // Group the winners back per provider for the table, keeping providers in + // registration order and entries in relevance order within each provider + for (QuickAccessProvider provider : elementsForProviders.keySet()) { + List group = winners.stream().filter(entry -> entry.provider == provider) + .collect(Collectors.toCollection(LinkedList::new)); + if (!group.isEmpty()) { + entriesPerProvider.put(provider, group); + } } } // @@ -563,34 +549,6 @@ public IStatus runInUIThread(IProgressMonitor monitor) { return (List[]) res.toArray(new List[res.size()]); } - /* - * Consider whether we could directly check the "matchQuality" here, but it - * seems to be a more expensive operation - */ - private static List putPrefixMatchFirst(List elements, String prefix) { - List res = new ArrayList<>(elements); - List matchingIndexes = new ArrayList<>(); - for (int i = 0; i < elements.size(); i++) { - if (elements.get(i).getLabel().toLowerCase().startsWith(prefix.toLowerCase())) { - matchingIndexes.add(Integer.valueOf(i)); - } - } - int currentMatchIndex = 0; - int currentNonMatchIndex = matchingIndexes.size(); - for (int i = 0; i < res.size(); i++) { - boolean isMatch = !matchingIndexes.isEmpty() && matchingIndexes.iterator().next().intValue() == i; - if (isMatch) { - matchingIndexes.remove(0); - res.set(currentMatchIndex, elements.get(i)); - currentMatchIndex++; - } else { - res.set(currentNonMatchIndex, elements.get(i)); - currentNonMatchIndex++; - } - } - return res; - } - Pattern categoryPattern; /** diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessEntry.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessEntry.java index d27ea8eed76..41e2fba46fa 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessEntry.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessEntry.java @@ -56,6 +56,9 @@ public class QuickAccessEntry { */ private final int matchQuality; + /** Continuous relevance score; higher is better. Set by the matcher. */ + int matchScore; + /** * Indicates the filter string was a perfect match to the label or there is no * filter applied @@ -273,4 +276,13 @@ public int getMatchQuality() { return matchQuality; } + /** + * Returns the continuous relevance score; higher is better. + * + * @return the relevance score + */ + public int getMatchScore() { + return matchScore; + } + } \ No newline at end of file diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatcher.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatcher.java index fa6f27957e9..abd28ff9c1c 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatcher.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatcher.java @@ -26,12 +26,6 @@ */ public final class QuickAccessMatcher { - private final QuickAccessElement element; - - public QuickAccessMatcher(QuickAccessElement element) { - this.element = element; - } - private static final int[][] EMPTY_INDICES = new int[0][0]; // whitespaces filter and patterns @@ -60,16 +54,28 @@ private Pattern getWildcardsPattern(String filter) { } /** - * If this element is a match (partial, complete, camel case, etc) to the given - * filter, returns a {@link QuickAccessEntry}. Otherwise returns - * null; + * Returns a {@link QuickAccessEntry} carrying highlight regions and a relevance + * score if {@code element} matches the filter, or null otherwise. * - * @param filter filter for matching - * @param providerForMatching the provider that will own the entry - * @return a quick access entry or null * @noreference This method is not intended to be referenced by clients. */ - public QuickAccessEntry match(String filter, QuickAccessProvider providerForMatching) { + public QuickAccessEntry match(QuickAccessElement element, String filter, QuickAccessProvider providerForMatching) { + QuickAccessEntry entry = doMatch(element, filter, providerForMatching); + if (entry != null) { + entry.matchScore = computeScore(element, filter, providerForMatching); + } + return entry; + } + + private static int computeScore(QuickAccessElement element, String filter, QuickAccessProvider provider) { + int score = QuickAccessMatching.score(element.getMatchLabel(), filter); + if (score == QuickAccessMatching.SCORE_NONE) { + score = QuickAccessMatching.score(provider.getName() + ' ' + element.getMatchLabel(), filter); + } + return score == QuickAccessMatching.SCORE_NONE ? 0 : score; + } + + private QuickAccessEntry doMatch(QuickAccessElement element, String filter, QuickAccessProvider providerForMatching) { String matchLabel = element.getMatchLabel(); String label = element.getLabel(); int quality = QuickAccessMatching.substringMatchQuality(matchLabel, label, filter); diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatching.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatching.java index 05df8faa3e2..7be4d95a0b5 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatching.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/quickaccess/QuickAccessMatching.java @@ -114,4 +114,82 @@ public static int substringMatchQuality(String matchLabel, String label, String } return QuickAccessEntry.MATCH_GOOD; } + + /** + * Sentinel returned by {@link #score} when the filter is not a subsequence of + * the text. + */ + public static final int SCORE_NONE = Integer.MIN_VALUE / 2; + + private static final int MATCH_BASE = 16; + private static final int BOUNDARY_BONUS = 30; + private static final int CONSECUTIVE_BONUS = 15; + private static final int PREFIX_BONUS = 8; + private static final int LEADING_GAP_PENALTY = 3; + private static final int MAX_LEADING_GAP = 5; + private static final int MAX_LENGTH_PENALTY = 20; + + /** + * Continuous relevance score for ranking; higher is better. Rewards matches at + * word boundaries, consecutive characters and a prefix, and lightly penalises + * leading gaps and longer text. Wildcard and whitespace characters in the + * filter are ignored and matching is case-insensitive. + * + * @param text the candidate text, in its original case + * @param filter the user filter + * @return the score, or {@link #SCORE_NONE} if the filter is not a subsequence + * of the text + */ + public static int score(String text, String filter) { + String needle = filter.toLowerCase().replaceAll("[\\s*?]", ""); //$NON-NLS-1$ //$NON-NLS-2$ + if (needle.isEmpty()) { + return 0; + } + int score = 0; + int firstMatch = -1; + int prevMatch = -2; + int j = 0; + for (int i = 0; i < text.length() && j < needle.length(); i++) { + if (Character.toLowerCase(text.charAt(i)) != needle.charAt(j)) { + continue; + } + if (firstMatch < 0) { + firstMatch = i; + } + int charScore = MATCH_BASE; + if (isBoundary(text, i)) { + charScore += BOUNDARY_BONUS; + } + if (prevMatch == i - 1) { + charScore += CONSECUTIVE_BONUS; + } + score += charScore; + prevMatch = i; + j++; + } + if (j < needle.length()) { + return SCORE_NONE; + } + if (firstMatch == 0) { + score += PREFIX_BONUS; + } + score -= Math.min(firstMatch, MAX_LEADING_GAP) * LEADING_GAP_PENALTY; + score -= Math.min(text.length(), MAX_LENGTH_PENALTY); + return score; + } + + private static boolean isBoundary(String text, int i) { + if (i == 0) { + return true; + } + char prev = text.charAt(i - 1); + char cur = text.charAt(i); + if (!Character.isLetterOrDigit(prev)) { + return true; + } + if (Character.isUpperCase(cur) && !Character.isUpperCase(prev)) { + return true; + } + return Character.isDigit(cur) && !Character.isDigit(prev); + } } diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/quickaccess/QuickAccessMatchingTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/quickaccess/QuickAccessMatchingTest.java index aec387e3318..c6b7c33f447 100644 --- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/quickaccess/QuickAccessMatchingTest.java +++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/quickaccess/QuickAccessMatchingTest.java @@ -109,4 +109,52 @@ public void safeCompileCompilesValidRegex() { Pattern p = QuickAccessMatching.safeCompile("foo.*"); assertTrue(p.matcher("foobar").matches()); } + + @Test + public void scoreReturnsNoneWhenNotSubsequence() { + assertEquals(QuickAccessMatching.SCORE_NONE, QuickAccessMatching.score("Rename", "xyz")); + } + + @Test + public void scoreReturnsZeroForEmptyFilter() { + assertEquals(0, QuickAccessMatching.score("Rename", "")); + } + + @Test + public void scorePrefersConsecutiveOverScattered() { + int consecutive = QuickAccessMatching.score("abcxyz", "abc"); + int scattered = QuickAccessMatching.score("axbxcx", "abc"); + assertTrue(consecutive > scattered, "consecutive " + consecutive + " should beat scattered " + scattered); + } + + @Test + public void scorePrefersPrefixOverMidWord() { + int prefix = QuickAccessMatching.score("Rename", "re"); + int midWord = QuickAccessMatching.score("Score", "re"); + assertTrue(prefix > midWord, "prefix " + prefix + " should beat mid-word " + midWord); + } + + @Test + public void scorePrefersShorterOnOtherwiseEqualMatch() { + int shorter = QuickAccessMatching.score("Re", "re"); + int longer = QuickAccessMatching.score("Renew", "re"); + assertTrue(shorter > longer, "shorter " + shorter + " should beat longer " + longer); + } + + @Test + public void scoreRewardsWordBoundaryInitials() { + int initials = QuickAccessMatching.score("New File", "nf"); + int midWord = QuickAccessMatching.score("Confirm", "nf"); + assertTrue(initials > midWord, "word-initial " + initials + " should beat mid-word " + midWord); + } + + @Test + public void scoreIgnoresWildcardChars() { + assertTrue(QuickAccessMatching.score("Rename Resource", "re*ce") > QuickAccessMatching.SCORE_NONE); + } + + @Test + public void scoreIsCaseInsensitive() { + assertTrue(QuickAccessMatching.score("RENAME", "rename") > QuickAccessMatching.SCORE_NONE); + } }