Skip to content

Commit

Permalink
Hide expanded item from completion list in certain case
Browse files Browse the repository at this point in the history 
We only show expanded item if any of the following conditions is true:
1. filter text length >= 2 (it's common to use filter of length 2 for camel case match, e.g. `AB` for `ArrayBuiler`)
2. the completion is triggered after `.` (i.e. in the context of listing members, which usually has much fewer items and more often used for browsing purpose)
3. defaults is not empty (it might suggests an expanded item)
  • Loading branch information
@genlu
genlu committed Mar 24, 2023
1 parent e82b2ad commit fae0854
Show file tree
Hide file tree
Showing 5 changed files with 277 additions and 59 deletions.
52 changes: 28 additions & 24 deletions src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CompletionSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ private bool TryInvokeSnippetCompletion(
}

var syntaxFacts = services.GetService<ISyntaxFactsService>();
// Snippets are included if the user types: <quesiton><tab>
// Snippets are included if the user types: <question><tab>
// If at least one condition for snippets do not hold, bail out.
if (syntaxFacts == null ||
caretPoint < 3 ||
Expand Down Expand Up @@ -240,10 +240,14 @@ public async Task<VSCompletionContext> GetCompletionContextAsync(
// #1 is the essence of completion so we'd always wait until its task is completed and return the results. However, because we have
// a really tight perf budget in completion, and computing those items in #2 could be expensive especially in a large solution
// (e.g. requires syntax/symbol indices and/or runs in OOP,) we decide to kick off the computation in parallel when completion is
// triggered, but only include its results if it's completed by the time task #1 is completed, otherwise we don't wait on it and
// just return items from #1 immediately. Task #2 will still be running in the background (until session is dismissed/committed,)
// and we'd check back to see if it's completed whenever we have a chance to update the completion list, i.e. when user typed another
// character, a filter was selected, etc. If so, those items will be added as part of the refresh.
// triggered, but only include its results if:
//
// (a) it's completed by the time task #1 is completed and
// (b) including them won't interfere with users' ability to browse the list (e.g. when the list is too long since filter text is short)
//
// Otherwise we don't wait on it and return items from #1 immediately. Task #2 will still be running in the background
// (until session is dismissed/committed) and we'd check back to see if it's completed whenever we have a chance to update the completion list,
// i.e. when user typed another character, a filter was selected, etc. If so, those items will be added as part of the refresh.
//
// The reason of adopting this approach is we want to minimize typing delays. There are two ways user might perceive a delay in typing.
// First, they could see a delay between typing a character and completion list being displayed if they want to examine the items available.
Expand All @@ -254,12 +258,12 @@ public async Task<VSCompletionContext> GetCompletionContextAsync(
//
// This approach would ensure the computation of #2 will not be the cause of such delays, with the obvious trade off of potentially not providing
// expanded items until later (or never) in a completion session even if the feature is enabled. Note that in most cases we'd expect task #2 to finish
// in time and complete result would be available from the start of the session. However, even in the case only partial result is returned at the start,
// we still believe this is acceptable given how critical perf is in typing scenario.
// in time and complete result would be available when it's most likely needed (see `ShouldHideExpandedItems` helper in ItemManager for details.)
// However, even in the case only partial result is returned at the start, we still believe this is acceptable given how critical perf is in typing scenario.
// Additionally, expanded items are usually considered complementary. The need for them only rise occasionally (it's rare when users need to add imports,)
// and when they are needed, our hypothesis is because of their more intrusive nature (adding an import to the document) users would more likely to
// contemplate such action thus typing slower before commit and/or spending more time examining the list, which give us some opportunities
// to still provide those items later before they are truly required.
// to still provide those items later before they are truly required.

var showCompletionItemFilters = _editorOptionsService.GlobalOptions.GetOption(CompletionViewOptionsStorage.ShowCompletionItemFilters, document.Project.Language);
var options = _editorOptionsService.GlobalOptions.GetCompletionOptions(document.Project.Language) with
Expand Down Expand Up @@ -296,7 +300,7 @@ public async Task<VSCompletionContext> GetCompletionContextAsync(
// If the core items are exclusive, we don't include expanded items.
// This would cancel expandedItemsTask.
if (sessionData.IsExclusive)
{
{
_ = _expandedItemsTaskCancellationSeries.CreateNext(CancellationToken.None);
}
else
Expand Down Expand Up @@ -345,26 +349,26 @@ public async Task<VSCompletionContext> GetExpandedCompletionContextAsync(

if (sessionData.CombinedSortedList is null)
{
// We only reach here when expanded items are disabled, but user requested them explicitly via expander.
// In this case, enable expanded items and trigger the completion only for them.
var document = initialTriggerLocation.Snapshot.GetOpenDocumentInCurrentContextWithChanges();
if (document != null)
{
// User selected expander explicitly, which means we need to collect and return
// items from unimported namespace (and only those items) regardless of whether it's enabled.
var options = _editorOptionsService.GlobalOptions.GetCompletionOptions(document.Project.Language) with
// We only reach here when expanded items are disabled, but user requested them explicitly via expander.
// In this case, enable expanded items and trigger the completion only for them.
var document = initialTriggerLocation.Snapshot.GetOpenDocumentInCurrentContextWithChanges();
if (document != null)
{
ShowItemsFromUnimportedNamespaces = true,
ExpandedCompletionBehavior = ExpandedCompletionMode.ExpandedItemsOnly
};
// User selected expander explicitly, which means we need to collect and return
// items from unimported namespace (and only those items) regardless of whether it's enabled.
var options = _editorOptionsService.GlobalOptions.GetCompletionOptions(document.Project.Language) with
{
ShowItemsFromUnimportedNamespaces = true,
ExpandedCompletionBehavior = ExpandedCompletionMode.ExpandedItemsOnly
};

var (context, completionList) = await GetCompletionContextWorkerAsync(session, document, initialTrigger, initialTriggerLocation, options, cancellationToken).ConfigureAwait(false);
UpdateSessionData(session, sessionData, completionList, initialTriggerLocation);
var (context, completionList) = await GetCompletionContextWorkerAsync(session, document, initialTrigger, initialTriggerLocation, options, cancellationToken).ConfigureAwait(false);
UpdateSessionData(session, sessionData, completionList, initialTriggerLocation);

return context;
return context;
}
}
}
}

return VSCompletionContext.Empty;
}
Expand Down
6 changes: 0 additions & 6 deletions src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,6 @@ private bool ShouldDismissCompletionListImmediately()
}

return false;

static bool IsAfterDot(ITextSnapshot snapshot, ITrackingSpan applicableToSpan)
{
var position = applicableToSpan.GetStartPoint(snapshot).Position;
return position > 0 && snapshot[position - 1] == '.';
}
}

private void AddCompletionItems(List<MatchResult> list, ThreadLocal<PatternMatchHelper> threadLocalPatternMatchHelper, CancellationToken cancellationToken)
Expand Down
28 changes: 26 additions & 2 deletions src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.CodeAnalysis.Options;
using Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion;
using Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Data;
using Microsoft.VisualStudio.Text;
using Roslyn.Utilities;

using RoslynCompletionItem = Microsoft.CodeAnalysis.Completion.CompletionItem;
Expand All @@ -20,9 +21,17 @@ namespace Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.AsyncComplet
{
internal sealed partial class ItemManager : IAsyncCompletionItemManager2
{
/// <summary>
/// The threshold for us to consider exclude (potentially large amount of) expanded items from completion list.
/// Showing a large amount of expanded items to user would introduce noise and render the list too long to browse.
/// Not processing those expanded items also has perf benefit (e.g. matching and highlighting could be expensive.)
/// We set it to 2 because it's common to use filter of length 2 for camel case match, e.g. `AB` for `ArrayBuiler`.
/// </summary>
internal const int FilterTextLengthToExcludeExpandedItemsExclusive = 2;

private readonly RecentItemsManager _recentItemsManager;
private readonly EditorOptionsService _editorOptionsService;

internal ItemManager(RecentItemsManager recentItemsManager, EditorOptionsService editorOptionsService)
{
_recentItemsManager = recentItemsManager;
Expand Down Expand Up @@ -93,7 +102,7 @@ private static SegmentedList<VSCompletionItem> SortCompletionItems(AsyncCompleti
data = new AsyncCompletionSessionDataSnapshot(sessionData.CombinedSortedList, data.Snapshot, data.Trigger, data.InitialTrigger, data.SelectedFilters,
data.IsSoftSelected, data.DisplaySuggestionItem, data.Defaults);
}
else if (sessionData.ExpandedItemsTask != null)
else if (!ShouldHideExpandedItems() && sessionData.ExpandedItemsTask is not null)
{
var task = sessionData.ExpandedItemsTask;

Expand Down Expand Up @@ -133,6 +142,21 @@ private static SegmentedList<VSCompletionItem> SortCompletionItems(AsyncCompleti
{
AsyncCompletionLogger.LogItemManagerUpdateDataPoint(stopwatch.Elapsed, isCanceled: cancellationToken.IsCancellationRequested);
}

// Don't hide expanded items if all these conditions are met:
// 1. filter text length >= 2 (it's common to use filter of length 2 for camel case match, e.g. `AB` for `ArrayBuiler`)
// 2. the completion is triggered in the context of listing members (it usually has much fewer items and more often used for browsing purpose)
// 3. defaults is not empty (it might suggests an expanded item)
bool ShouldHideExpandedItems()
=> session.ApplicableToSpan.GetText(data.Snapshot).Length < FilterTextLengthToExcludeExpandedItemsExclusive
&& !IsAfterDot(data.Snapshot, session.ApplicableToSpan)
&& data.Defaults.IsEmpty;
}

private static bool IsAfterDot(ITextSnapshot snapshot, ITrackingSpan applicableToSpan)
{
var position = applicableToSpan.GetStartPoint(snapshot).Position;
return position > 0 && snapshot[position - 1] == '.';
}

private sealed class VSItemComparer : IComparer<VSCompletionItem>
Expand Down
35 changes: 8 additions & 27 deletions src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6860,7 +6860,7 @@ namespace NS1
{
void M()
{
$$
Un$$
}
}
}
Expand Down Expand Up @@ -7111,7 +7111,6 @@ public class AA

Await state.SendInvokeCompletionListAndWaitForUiRenderAsync()

' import completion is disabled, so we shouldn't have expander selected by default
state.AssertCompletionItemExpander(isAvailable:=True, isSelected:=False)
Await state.AssertCompletionItemsContain("DDProp1", "")
Await state.AssertCompletionItemsDoNotContainAny("DD")
Expand Down Expand Up @@ -7954,7 +7953,7 @@ namespace NS1
{
void M()
{
$$
unimportedtype$$
}
}
}
Expand All @@ -7973,16 +7972,7 @@ namespace <%= containingNamespace %>
state.Workspace.GlobalOptions.SetGlobalOption(CompletionOptionsStorage.ForceExpandedCompletionIndexCreation, True)
state.Workspace.GlobalOptions.SetGlobalOption(CompletionOptionsStorage.ShowItemsFromUnimportedNamespaces, LanguageNames.CSharp, True)

Await state.SendInvokeCompletionListAndWaitForUiRenderAsync()

' make sure expander is selected
state.AssertCompletionItemExpander(isAvailable:=True, isSelected:=True)

state.SendEscape()
Await state.AssertNoCompletionSession()

state.SendTypeChars("unimportedtype")
Await state.WaitForAsynchronousOperationsAsync()
Await state.SendCommitUniqueCompletionListItemAsync()

' make sure expander is selected
state.AssertCompletionItemExpander(isAvailable:=True, isSelected:=True)
Expand Down Expand Up @@ -8021,7 +8011,7 @@ namespace NS1
{
void M()
{
$$
mytask$$
}
}

Expand All @@ -8042,16 +8032,7 @@ namespace NS2
state.Workspace.GlobalOptions.SetGlobalOption(CompletionOptionsStorage.ForceExpandedCompletionIndexCreation, True)
state.Workspace.GlobalOptions.SetGlobalOption(CompletionOptionsStorage.ShowItemsFromUnimportedNamespaces, LanguageNames.CSharp, True)

Await state.SendInvokeCompletionListAndWaitForUiRenderAsync()

' make sure expander is selected
state.AssertCompletionItemExpander(isAvailable:=True, isSelected:=True)

state.SendEscape()
Await state.AssertNoCompletionSession()

state.SendTypeChars("mytask")
Await state.WaitForAsynchronousOperationsAsync()
Await state.SendCommitUniqueCompletionListItemAsync()

' make sure expander is selected
state.AssertCompletionItemExpander(isAvailable:=True, isSelected:=True)
Expand Down Expand Up @@ -10449,7 +10430,7 @@ class C
Public Async Function TestNonBlockingExpandCompletionDoesNotChangeItemOrder() As Task
Using state = TestStateFactory.CreateCSharpTestState(
<Document>
$$
Test$$
</Document>,
extraExportedTypes:={GetType(TestProvider)}.ToList())

Expand Down Expand Up @@ -10482,7 +10463,7 @@ class C

' Now delayed expand item task is completed, following up by typing and delete a character to trigger
' update so the list would contains all items
Await state.SendTypeCharsAndWaitForUiRenderAsync("t")
Await state.SendTypeCharsAndWaitForUiRenderAsync("U")
Await state.AssertCompletionItemsContain("TestUnimportedItem", "")
state.AssertCompletionItemExpander(isAvailable:=True, isSelected:=True)

Expand All @@ -10496,7 +10477,7 @@ class C
state.SendEscape()
Await state.AssertNoCompletionSession()

' Now disable expand item delay, so intial trigger should contain full list
' Now disable expand item delay, so initial trigger should contain full list
state.TextView.Options.SetOptionValue(DefaultOptions.ResponsiveCompletionOptionId, False)

state.SendInvokeCompletionList()
Expand Down
Loading

0 comments on commit fae0854

Please sign in to comment.