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

Completion should select IntelliCode item when possible #37066

Merged
merged 11 commits into from
Jul 10, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public AsyncCompletionData.CommitResult TryCommit(
roslynItem, completionListSpan, commitChar, triggerSnapshot, serviceRules,
filterText, cancellationToken);

_recentItemsManager.MakeMostRecentItem(roslynItem.DisplayText);
_recentItemsManager.MakeMostRecentItem(roslynItem.FilterText);
return new AsyncCompletionData.CommitResult(isHandled: true, commitBehavior);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
using Microsoft.VisualStudio.Text;
using AsyncCompletionData = Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Data;
using RoslynTrigger = Microsoft.CodeAnalysis.Completion.CompletionTrigger;
using RoslynCompletionItem = Microsoft.CodeAnalysis.Completion.CompletionItem;
using VSCompletionItem = Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Data.CompletionItem;

namespace Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.AsyncCompletion
{
Expand Down Expand Up @@ -63,7 +65,7 @@ internal static CompletionFilterReason GetFilterReason(AsyncCompletionData.Compl
}
}

internal static bool IsFilterCharacter(CompletionItem item, char ch, string textTypedSoFar)
internal static bool IsFilterCharacter(RoslynCompletionItem item, char ch, string textTypedSoFar)
{
// First see if the item has any specific filter rules it wants followed.
foreach (var rule in item.Rules.FilterCharacterRules)
Expand Down Expand Up @@ -97,5 +99,15 @@ internal static bool IsFilterCharacter(CompletionItem item, char ch, string text

return false;
}

// This is a temporarily method to support preferrrence of IntelliCode items comparing to non-IntelliCode items.
ivanbasov marked this conversation as resolved.
Show resolved Hide resolved
// We expect that Editor will intorduce this support and we will get rid of relying on the "★" then.
internal static bool IsPreferredItem(this RoslynCompletionItem completionItem)
Copy link
Member

Choose a reason for hiding this comment

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

❓ How is sorting performed? Can we not use the same strategy here (i.e. among two items with the same filter text, prefer to select the first one in sort order)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no. Editor maintains multiple items lists: full, sorted, and filtered. They ask us for sorting before. However, they send us the full unsorted list for filtering.

=> completionItem.DisplayText.StartsWith("★");

// This is a temporarily method to support preferrrence of IntelliCode items comparing to non-IntelliCode items.
// We expect that Editor will intorduce this support and we will get rid of relying on the "★" then.
internal static bool IsPreferredItem(this VSCompletionItem completionItem)
=> completionItem.DisplayText.StartsWith("★");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,12 @@ private FilteredCompletionModel HandleNormalFiltering(
if (bestItem != null)
{
selectedItemIndex = itemsInList.IndexOf(i => Equals(i.FilterResult.CompletionItem, bestItem));
var deduplicatedList = matchingItems.Where(r => !r.DisplayText.StartsWith("★"));
var deduplicatedListCount = matchingItems.Where(r => !r.IsPreferredItem()).Count();
if (selectedItemIndex > -1 &&
deduplicatedList.Count() == 1 &&
deduplicatedListCount == 1 &&
filterText.Length > 0)
{
var uniqueItemIndex = itemsInList.IndexOf(i => Equals(i.FilterResult.CompletionItem, deduplicatedList.First()));
var uniqueItemIndex = itemsInList.IndexOf(i => Equals(i.FilterResult.CompletionItem, bestItem));
uniqueItem = highlightedList[uniqueItemIndex].CompletionItem;
}
}
Expand Down Expand Up @@ -391,7 +391,7 @@ private FilteredCompletionModel HandleDeletionTrigger(
hardSelect = false;
}

var deduplicatedListCount = matchingItems.Where(r => !r.VSCompletionItem.DisplayText.StartsWith("★")).Count();
var deduplicatedListCount = matchingItems.Where(r => !r.VSCompletionItem.IsPreferredItem()).Count();

return new FilteredCompletionModel(
highlightedList, index, filters,
Expand Down Expand Up @@ -493,7 +493,8 @@ internal static RoslynCompletionItem GetBestCompletionItemBasedOnMRU(
var mruIndex1 = GetRecentItemIndex(recentItems, bestItem);
var mruIndex2 = GetRecentItemIndex(recentItems, chosenItem);

if (mruIndex2 < mruIndex1)
if ((mruIndex2 < mruIndex1) ||
(mruIndex2 == mruIndex1 && !bestItem.IsPreferredItem() && chosenItem.IsPreferredItem()))
Copy link
Member

Choose a reason for hiding this comment

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

💡 !bestItem.IsPreferredItem() is an unnecessary check here

Suggested change
(mruIndex2 == mruIndex1 && !bestItem.IsPreferredItem() && chosenItem.IsPreferredItem()))
(mruIndex2 == mruIndex1 && chosenItem.IsPreferredItem()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it depends. For example, there are starred Last and Length, and match by L. Which one we should prefer? The current idea is to prefer the first item. Also the pattern we follow in this method is to use the chosen item only if it exceeds the current best one. So, let us follow the pattern consistently.

{
bestItem = chosenItem;
}
Expand All @@ -513,7 +514,8 @@ internal static RoslynCompletionItem GetBestCompletionItemBasedOnMRU(
var bestItemPriority = bestItem.Rules.MatchPriority;
var currentItemPriority = chosenItem.Rules.MatchPriority;

if (currentItemPriority > bestItemPriority)
if ((currentItemPriority > bestItemPriority) ||
((currentItemPriority == bestItemPriority) && !bestItem.IsPreferredItem() && chosenItem.IsPreferredItem()))
{
bestItem = chosenItem;
}
Expand All @@ -524,7 +526,7 @@ internal static RoslynCompletionItem GetBestCompletionItemBasedOnMRU(

internal static int GetRecentItemIndex(ImmutableArray<string> recentItems, RoslynCompletionItem item)
{
var index = recentItems.IndexOf(item.DisplayText);
var index = recentItems.IndexOf(item.FilterText);
return -index;
}

Expand Down Expand Up @@ -565,7 +567,13 @@ internal static bool IsBetterDeletionMatch(FilterResult result1, FilterResult re
{
return true;
}

if (result1.CompletionItem.IsPreferredItem() && !result2.CompletionItem.IsPreferredItem())
{
return true;
}
}

return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5057,13 +5057,112 @@ class C

state.SendTypeChars(".len")
Await state.AssertCompletionSession()
state.AssertCompletionItemsContainAll({"Length", "★ Length3", "★ Length2"})
state.AssertCompletionItemsContainAll({"Length", "★ Length", "★ Length2"})
state.SendCommitUniqueCompletionListItem()
Await state.AssertNoCompletionSession()
Assert.Contains("s.Length", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
End Using
End Function

' Implementation for the Modern completion only
<InlineData(CompletionImplementation.Modern)>
<WpfTheory, Trait(Traits.Feature, Traits.Features.Completion)>
Public Async Function IntelliCodeItemPreferredAfterCommitingIntelliCodeItem(completionImplementation As CompletionImplementation) As Task
Dim provider = New IntelliCodeMockProvider()
Using state = TestStateFactory.CreateCSharpTestState(completionImplementation,
<Document>
class C
{
void Method()
{
var s = "";
s$$
}
}
</Document>, {provider})

state.Workspace.Options = state.Workspace.Options.WithChangedOption(
CompletionOptions.TriggerOnDeletion, LanguageNames.CSharp, True)

state.SendTypeChars(".nor")
Await state.AssertCompletionSession()
state.AssertCompletionItemsContainAll({"Normalize", "★ Normalize"})
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")
state.SendTab()
Await state.AssertNoCompletionSession()
Assert.Contains("s.Normalize", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
For i = 1 To "ze".Length
state.SendBackspace()
Next
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")

state.SendEscape()
For i = 1 To "Normali".Length
state.SendBackspace()
Next
state.SendEscape()
Assert.Contains("s.", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)

state.SendInvokeCompletionList()
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")
state.SendEscape()

state.SendTypeChars("n")
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")
End Using
End Function

' Implementation for the Modern completion only
<InlineData(CompletionImplementation.Modern)>
<WpfTheory, Trait(Traits.Feature, Traits.Features.Completion)>
Public Async Function IntelliCodeItemPreferredAfterCommitingNonIntelliCodeItem(completionImplementation As CompletionImplementation) As Task
Dim provider = New IntelliCodeMockProvider()
Using state = TestStateFactory.CreateCSharpTestState(completionImplementation,
<Document>
class C
{
void Method()
{
var s = "";
s$$
}
}
</Document>, {provider})

state.Workspace.Options = state.Workspace.Options.WithChangedOption(
CompletionOptions.TriggerOnDeletion, LanguageNames.CSharp, True)

state.SendTypeChars(".nor")
Await state.AssertCompletionSession()
state.AssertCompletionItemsContainAll({"Normalize", "★ Normalize"})
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")

state.NavigateToDisplayText("Normalize")
state.SendTab()

Await state.AssertNoCompletionSession()
Assert.Contains("s.Normalize", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
For i = 1 To "ze".Length
state.SendBackspace()
Next
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")

state.SendEscape()
For i = 1 To "Normali".Length
state.SendBackspace()
Next
state.SendEscape()
Assert.Contains("s.", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)

state.SendInvokeCompletionList()
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")
state.SendEscape()

state.SendTypeChars("n")
Await state.AssertSelectedCompletionItem("★ Normalize", displayTextSuffix:="()")
End Using
End Function

<WorkItem(35614, "https://github.com/dotnet/roslyn/issues/35614")>
<MemberData(NameOf(AllCompletionImplementations))>
<WpfTheory, Trait(Traits.Feature, Traits.Features.Completion)>
Expand Down Expand Up @@ -5451,33 +5550,37 @@ class C

Public Overrides Function ProvideCompletionsAsync(context As CompletionContext) As Task
context.AddItem(CompletionItem.Create(displayText:="★ Length", filterText:="Length"))
context.AddItem(CompletionItem.Create(displayText:="★ Normalize", filterText:="Normalize"))
context.AddItem(CompletionItem.Create(displayText:="★ Normalize", filterText:="Normalize", displayTextSuffix:="()"))
context.AddItem(CompletionItem.Create(displayText:="Length", filterText:="Length"))
context.AddItem(CompletionItem.Create(displayText:="ToString()", filterText:="ToString"))
context.AddItem(CompletionItem.Create(displayText:="First()", filterText:="First"))
context.AddItem(CompletionItem.Create(displayText:="ToString", filterText:="ToString", displayTextSuffix:="()"))
context.AddItem(CompletionItem.Create(displayText:="First", filterText:="First", displayTextSuffix:="()"))
Return Task.CompletedTask
End Function

Public Overrides Function ShouldTriggerCompletion(text As SourceText, caretPosition As Integer, trigger As CompletionTrigger, options As OptionSet) As Boolean
Return True
End Function

Public Overrides Function GetChangeAsync(document As Document, item As CompletionItem, commitKey As Char?, cancellationToken As CancellationToken) As Task(Of CompletionChange)
Dim commitText = item.DisplayText
If commitText.StartsWith("★") Then
' remove the star and the following space
commitText = commitText.Substring(2)
End If

Return Task.FromResult(CompletionChange.Create(New TextChange(item.Span, commitText)))
End Function
End Class

' Simulates a situation where IntelliCode provides items not included into the Rolsyn original list.
' We want to ignore these items in CommitIfUnique.
' This situation should not happen. Tests with this provider were added to cover protective scenarios.
Private Class IntelliCodeMockWeirdProvider
Inherits CompletionProvider
Inherits IntelliCodeMockProvider

Public Overrides Function ProvideCompletionsAsync(context As CompletionContext) As Task
context.AddItem(CompletionItem.Create(displayText:="★ Length2", filterText:="Length2"))
context.AddItem(CompletionItem.Create(displayText:="Length", filterText:="Length"))
context.AddItem(CompletionItem.Create(displayText:="★ Length3", filterText:="Length3"))
Return Task.CompletedTask
End Function

Public Overrides Function ShouldTriggerCompletion(text As SourceText, caretPosition As Integer, trigger As CompletionTrigger, options As OptionSet) As Boolean
Return True
Public Overrides Async Function ProvideCompletionsAsync(context As CompletionContext) As Task
Await MyBase.ProvideCompletionsAsync(context).ConfigureAwait(False)
context.AddItem(CompletionItem.Create(displayText:="★ Length2", filterText:="Length"))
End Function
End Class
End Class
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
Dim completionItems = session.GetComputedItems(CancellationToken.None)
' During the computation we can explicitly dismiss the session or we can return no items.
' Each of these conditions mean that there is no active completion.
Assert.True(session.IsDismissed OrElse completionItems.Items.Count() = 0)
Assert.True(session.IsDismissed OrElse completionItems.Items.Count() = 0, "AssertNoCompletionSession")
End Function

Public Overrides Sub AssertNoCompletionSessionWithNoBlock()
Expand Down Expand Up @@ -188,7 +188,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense

Await WaitForAsynchronousOperationsAsync()
Dim session = GetExportedValue(Of IAsyncCompletionBroker)().GetSession(view)
Assert.NotNull(session)
Assert.True(session IsNot Nothing, "AssertCompletionSession")
End Function

Public Overrides Async Function AssertCompletionSessionAfterTypingHash() As Task
Expand Down Expand Up @@ -291,15 +291,6 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
End If
End Sub

Private Function GetRoslynCompletionItemOpt(editorCompletionItem As Data.CompletionItem) As CompletionItem
Dim roslynCompletionItem As CompletionItem = Nothing
If editorCompletionItem?.Properties.TryGetProperty(RoslynItem, roslynCompletionItem) Then
Return roslynCompletionItem
End If

Return Nothing
End Function

Public Overrides Function GetCompletionItems() As IList(Of CompletionItem)
WaitForAsynchronousOperationsAsync()
Dim session = GetExportedValue(Of IAsyncCompletionBroker)().GetSession(TextView)
Expand Down
25 changes: 25 additions & 0 deletions src/EditorFeatures/TestUtilities2/Intellisense/TestStateBase.vb
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,31 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense

Public MustOverride Function WaitForUIRenderedAsync() As Task

Public Sub NavigateToDisplayText(targetText As String)
Dim currentText = GetSelectedItem().DisplayText

' GetComputedItems provided by the Editor for tests does not guarantee that
' the order of items match the order of items actually displayed in the completion popup.
' For example, they put starred items (intellicode) below non-starred ones.
' And the order they display those items in the UI is opposite.
' Therefore, we do the full traverse: down to the bottom and if not found up to the top.
Do While currentText <> targetText
SendDownKey()
Dim newText = GetSelectedItem().DisplayText
If currentText = newText Then
' Nothing found on going down. Try going up
Do While currentText <> targetText
SendUpKey()
newText = GetSelectedItem().DisplayText
Assert.True(newText <> currentText, "Reached the bottom, then the top and didn't find the match")
currentText = newText
Loop
End If

currentText = newText
Loop
End Sub

#End Region

#Region "Signature Help Operations"
Expand Down