Skip to content

Soft select the item with longest common prefix#45576

Merged
genlu merged 1 commit intodotnet:masterfrom
genlu:SelectLongestCommonPrefix
Jul 1, 2020
Merged

Soft select the item with longest common prefix#45576
genlu merged 1 commit intodotnet:masterfrom
genlu:SelectLongestCommonPrefix

Conversation

@genlu
Copy link
Member

@genlu genlu commented Jul 1, 2020

instead of the first item, when there's no item matches filter text. This seems to be the behavior of old C# completion in VS 2013 (prior to Roslyn)
Fix https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1128749

@CyrusNajmabadi @dotnet/roslyn-ide

@genlu genlu requested a review from CyrusNajmabadi July 1, 2020 00:48
@genlu genlu requested a review from a team as a code owner July 1, 2020 00:48
@genlu genlu added Area-IDE IDE-IntelliSense Completion, Signature Help, Quick Info labels Jul 1, 2020
@genlu
Copy link
Member Author

genlu commented Jul 1, 2020

@CyrusNajmabadi I don't know if dropping the old behavior was a deliberate decision, but I like it so I'm bringing it back.

@genlu genlu requested a review from jasonmalinowski July 1, 2020 00:56
bestOrFirstMatchResult = itemsInList.FirstOrDefault();
// We do not have matches: pick the one with longest common prefix or the first item from the list.
selectedItemIndex = 0;
bestOrFirstMatchResult = itemsInList[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

do we know at thsi point that itemsInList is non-empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it was checked in the caller

If e.Items.Items.Select(Function(i) i.FilterText).Skip(1).Any() Then
' If there is 0 or more than one item left, then it means this was the filter operation that resulted and we're done.
' Otherwise we know a Dismiss operation is coming so we should wait for it.
If e.Items.Items.Count() <> 1 Then
Copy link
Contributor

Choose a reason for hiding this comment

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

this is scary. why change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

n/m i see.

Copy link
Member Author

Choose a reason for hiding this comment

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

we have a special case which is when we return an empty list instead of directly dismiss the session. W/o this change the test would hang.
http://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs,475

@genlu genlu merged commit 25aa984 into dotnet:master Jul 1, 2020
@ghost ghost added this to the Next milestone Jul 1, 2020
@genlu genlu deleted the SelectLongestCommonPrefix branch July 1, 2020 03:52
@JoeRobich JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE IDE-IntelliSense Completion, Signature Help, Quick Info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants