-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Tweak completion filtering behavior. #16892
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
Changes from all commits
fe691f5
b0d15bc
dc754cf
62ba011
39f4d7a
3456a4f
46dcf76
17bb3af
4e39d36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Completion | ||
| { | ||
| internal enum CompletionFilterReason | ||
| { | ||
| Insertion, | ||
| Deletion, | ||
| NonInsertionOrDeletion, | ||
| #if false | ||
| // If necessary, we could add additional filter reasons. For example, for the below items. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment feels much like "I'm commenting out code I thought I needed". Perhaps rewrite to more clear prose? |
||
| // However, we have no need for them currently. That somewhat makes sense. We only want | ||
| // to really customize our filtering behavior depending on if a user was typing/deleting | ||
| // in the buffer. | ||
|
|
||
| Snippets, | ||
| ItemFiltersChanged, | ||
| CaretPositionChanged, | ||
| Invoke, | ||
| InvokeAndCommitIfUnique | ||
| #endif | ||
| } | ||
|
|
||
| internal static class CompletionTriggerExtensions | ||
| { | ||
| public static CompletionFilterReason GetFilterReason(this CompletionTrigger trigger) | ||
| => trigger.Kind.GetFilterReason(); | ||
| } | ||
|
|
||
| internal static class CompletionTriggerKindExtensions | ||
| { | ||
| public static CompletionFilterReason GetFilterReason(this CompletionTriggerKind kind) | ||
| { | ||
| switch (kind) | ||
| { | ||
| case CompletionTriggerKind.Insertion: | ||
| return CompletionFilterReason.Insertion; | ||
| case CompletionTriggerKind.Deletion: | ||
| return CompletionFilterReason.Deletion; | ||
| case CompletionTriggerKind.Snippets: | ||
| case CompletionTriggerKind.Invoke: | ||
| case CompletionTriggerKind.InvokeAndCommitIfUnique: | ||
| return CompletionFilterReason.NonInsertionOrDeletion; | ||
| default: | ||
| throw ExceptionUtilities.UnexpectedValue(kind); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,6 @@ internal partial class Session | |
| { | ||
| public void FilterModel( | ||
| CompletionFilterReason filterReason, | ||
| bool dismissIfEmptyAllowed, | ||
| bool recheckCaretPosition, | ||
| ImmutableDictionary<CompletionItemFilter, bool> filterState) | ||
| { | ||
|
|
@@ -44,29 +43,7 @@ public void FilterModel( | |
| } | ||
|
|
||
| return FilterModelInBackground( | ||
| model, localId, caretPosition, recheckCaretPosition, dismissIfEmptyAllowed, filterReason); | ||
| }); | ||
| } | ||
|
|
||
| public void IdentifyBestMatchAndFilterToAllItems( | ||
| CompletionFilterReason filterReason, bool recheckCaretPosition, bool dismissIfEmptyAllowed) | ||
| { | ||
| AssertIsForeground(); | ||
|
|
||
| var caretPosition = GetCaretPointInViewBuffer(); | ||
|
|
||
| // Use an interlocked increment so that reads by existing filter tasks will see the | ||
| // change. | ||
| Interlocked.Increment(ref _filterId); | ||
| var localId = _filterId; | ||
| Computation.ChainTaskAndNotifyControllerWhenFinished(model => | ||
| { | ||
| var filteredModel = FilterModelInBackground( | ||
| model, localId, caretPosition, recheckCaretPosition, dismissIfEmptyAllowed, filterReason); | ||
|
|
||
| return filteredModel != null | ||
| ? filteredModel.WithFilteredItems(filteredModel.TotalItems).WithSelectedItem(filteredModel.SelectedItemOpt) | ||
| : null; | ||
| model, localId, caretPosition, recheckCaretPosition, filterReason); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -75,13 +52,12 @@ private Model FilterModelInBackground( | |
| int id, | ||
| SnapshotPoint caretPosition, | ||
| bool recheckCaretPosition, | ||
| bool dismissIfEmptyAllowed, | ||
| CompletionFilterReason filterReason) | ||
| { | ||
| using (Logger.LogBlock(FunctionId.Completion_ModelComputation_FilterModelInBackground, CancellationToken.None)) | ||
| { | ||
| return FilterModelInBackgroundWorker( | ||
| model, id, caretPosition, recheckCaretPosition, dismissIfEmptyAllowed, filterReason); | ||
| model, id, caretPosition, recheckCaretPosition, filterReason); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -90,7 +66,6 @@ private Model FilterModelInBackgroundWorker( | |
| int id, | ||
| SnapshotPoint caretPosition, | ||
| bool recheckCaretPosition, | ||
| bool dismissIfEmptyAllowed, | ||
| CompletionFilterReason filterReason) | ||
| { | ||
| if (model == null) | ||
|
|
@@ -155,7 +130,8 @@ private Model FilterModelInBackgroundWorker( | |
| return model; | ||
| } | ||
|
|
||
| if (ItemIsFilteredOut(currentItem, effectiveFilterItemState)) | ||
| if (CompletionItemFilter.ShouldBeFilteredOutOfCompletionList( | ||
| currentItem, effectiveFilterItemState)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
@@ -170,11 +146,23 @@ private Model FilterModelInBackgroundWorker( | |
| } | ||
| else | ||
| { | ||
| if (filterText.Length <= 1) | ||
| // The item didn't match the filter text. We'll still keep it in the list | ||
| // if one of two things is true: | ||
| // | ||
| // 1. The user has only typed a single character. In this case they might | ||
| // have just typed the character to get completion. Filtering out items | ||
| // here is not desirable. | ||
| // | ||
| // 2. They brough up completion with ctrl-j or through deletion. In these | ||
| // cases we just always keep all the items in the list. | ||
|
|
||
| var wasTriggeredByDeleteOrSimpleInvoke = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reorder these to match the cases above.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But massive thanks for actually explaining in English what the heck this is trying to do. |
||
| model.Trigger.Kind == CompletionTriggerKind.Deletion || | ||
| model.Trigger.Kind == CompletionTriggerKind.Invoke; | ||
| var shouldKeepItem = filterText.Length <= 1 || wasTriggeredByDeleteOrSimpleInvoke; | ||
|
|
||
| if (shouldKeepItem) | ||
| { | ||
| // Even though the rule provider didn't match this, we'll still include it | ||
| // since we want to allow a user typing a single character and seeing all | ||
| // possibly completions. | ||
| filterResults.Add(new FilterResult( | ||
| currentItem, filterText, matchedFilterText: false)); | ||
| } | ||
|
|
@@ -186,14 +174,14 @@ private Model FilterModelInBackgroundWorker( | |
| // If no items matched the filter text then determine what we should do. | ||
| if (filterResults.Count == 0) | ||
| { | ||
| return HandleAllItemsFilteredOut(model, filterReason, dismissIfEmptyAllowed); | ||
| return HandleAllItemsFilteredOut(model, filterReason); | ||
| } | ||
|
|
||
| // If this was deletion, then we control the entire behavior of deletion | ||
| // ourselves. | ||
| if (model.Trigger.Kind == CompletionTriggerKind.Deletion) | ||
| { | ||
| return HandleDeletionTrigger(model, filterResults); | ||
| return HandleDeletionTrigger(model, filterReason, filterResults); | ||
| } | ||
|
|
||
| return HandleNormalFiltering( | ||
|
|
@@ -219,7 +207,7 @@ private static ImmutableDictionary<CompletionItemFilter, bool> ComputeEffectiveF | |
| return filterState; | ||
| } | ||
|
|
||
| private Boolean IsAfterDot(Model model, ITextSnapshot textSnapshot, Dictionary<TextSpan, string> textSpanToText) | ||
| private bool IsAfterDot(Model model, ITextSnapshot textSnapshot, Dictionary<TextSpan, string> textSpanToText) | ||
| { | ||
| var span = model.OriginalList.Span; | ||
|
|
||
|
|
@@ -335,8 +323,21 @@ private CompletionItem GetBestCompletionItemBasedOnMRU( | |
| return bestItem; | ||
| } | ||
|
|
||
| private Model HandleDeletionTrigger(Model model, List<FilterResult> filterResults) | ||
| private Model HandleDeletionTrigger( | ||
| Model model, CompletionFilterReason filterReason, List<FilterResult> filterResults) | ||
| { | ||
| if (filterReason == CompletionFilterReason.Insertion && | ||
| !filterResults.Any(r => r.MatchedFilterText)) | ||
| { | ||
| // The user has typed something, but nothing in the actual list matched what | ||
| // they were typing. In this case, we want to dismiss completion entirely. | ||
| // The thought process is as follows: we aggressively brough up completion | ||
| // to help them when they typed delete (in case they wanted to pick another | ||
| // item). However, they're typing something that doesn't seem to match at all | ||
| // The completion list is just distracting at this point. | ||
| return null; | ||
| } | ||
|
|
||
| FilterResult? bestFilterResult = null; | ||
| int matchCount = 0; | ||
| foreach (var currentFilterResult in filterResults.Where(r => r.MatchedFilterText)) | ||
|
|
@@ -403,28 +404,12 @@ private bool IsBetterDeletionMatch(FilterResult result1, FilterResult result2) | |
| return false; | ||
| } | ||
|
|
||
| private struct FilterResult | ||
| { | ||
| public readonly CompletionItem CompletionItem; | ||
| public readonly bool MatchedFilterText; | ||
| public readonly string FilterText; | ||
|
|
||
| public FilterResult(CompletionItem completionItem, string filterText, bool matchedFilterText) | ||
| { | ||
| CompletionItem = completionItem; | ||
| MatchedFilterText = matchedFilterText; | ||
| FilterText = filterText; | ||
| } | ||
| } | ||
|
|
||
| private static Model HandleAllItemsFilteredOut( | ||
| Model model, | ||
| CompletionFilterReason filterReason, | ||
| bool dismissIfEmptyAllowed) | ||
| CompletionFilterReason filterReason) | ||
| { | ||
| if (dismissIfEmptyAllowed && | ||
| model.DismissIfEmpty && | ||
| filterReason == CompletionFilterReason.TypeChar) | ||
| if (model.DismissIfEmpty && | ||
| filterReason == CompletionFilterReason.Insertion) | ||
| { | ||
| // If the user was just typing, and the list went to empty *and* this is a | ||
| // language that wants to dismiss on empty, then just return a null model | ||
|
|
@@ -464,7 +449,7 @@ private static bool MatchesFilterText( | |
| // Specifically, to avoid being too aggressive when matching an item during | ||
| // completion, we require that the current filter text be a prefix of the | ||
| // item in the list. | ||
| if (filterReason == CompletionFilterReason.BackspaceOrDelete && | ||
| if (filterReason == CompletionFilterReason.Deletion && | ||
| trigger.Kind == CompletionTriggerKind.Deletion) | ||
| { | ||
| return item.FilterText.GetCaseInsensitivePrefixLength(filterText) > 0; | ||
|
|
@@ -507,34 +492,6 @@ private static bool IsAllDigits(string filterText) | |
| return true; | ||
| } | ||
|
|
||
| private bool ItemIsFilteredOut( | ||
| CompletionItem item, | ||
| ImmutableDictionary<CompletionItemFilter, bool> filterState) | ||
| { | ||
| if (filterState == null) | ||
| { | ||
| // No filtering. The item is not filtered out. | ||
| return false; | ||
| } | ||
|
|
||
| foreach (var filter in CompletionItemFilter.AllFilters) | ||
| { | ||
| // only consider filters that match the item | ||
| var matches = filter.Matches(item); | ||
| if (matches) | ||
| { | ||
| // if the specific filter is enabled then it is not filtered out | ||
| if (filterState.TryGetValue(filter, out var enabled) && enabled) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // The item was filtered out. | ||
| return true; | ||
| } | ||
|
|
||
| private bool IsHardSelection( | ||
| Model model, | ||
| CompletionItem bestFilterMatch, | ||
|
|
@@ -648,4 +605,4 @@ private static bool IsAllPunctuation(string filterText) | |
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a doc comment explaining this is either delete or backspace.