Handling defaults provided by IAsyncCompletionDefaultsSource by genlu · Pull Request #57685 · dotnet/roslyn

genlu · 2021-11-10T22:58:43Z

IAsyncCompletionDefaultsSource returns the first token of whatever whole line prediction is displayed, the goal is to make selected item in completion more consistent with WLC prediction.

So, if you see something like:

The suggested default would be “Console”. If you accept Console and type ‘.’, the suggested default would be “WriteLine”.

The basic algorithm for using the suggested default is:

Roslyn decides which item to select.
If we have a good reason to select this item, indicated by it being preselected (e.g. this bug ), use that item instead of suggested default.
Otherwise, compare the pattern matching result of the current selection (with the filter text) and compare it with the pattern matching result of the suggested defaults . If the suggested default is no worse than the current selected item ~~(in a case-insensitive manner)~~, use the suggested default. Otherwise use the original selected item.

Using the same example above, if user typed "C", roslyn might select "CancellationToken", but with suggested default, we will end up selecting "Console".

The handling logic is based on the original implementation in VS (see this), with the additional change to fix this bug and some general cleanup

See https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform/pullrequest/353067

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

dpugh · 2021-11-11T01:03:31Z

Sorry, not used to github diffs, I'm too used to code flow and didn't notice the dates associated with the commits. Changes look good now that I'm looking at the right set of commits.

CyrusNajmabadi · 2021-11-11T01:29:44Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

        private readonly RecentItemsManager _recentItemsManager;
        private readonly IGlobalOptionService _globalOptions;

+        public const string AggressiveDefaultsMatchingOptionName = "AggressiveDefaultsMatchingOption";


what does 'aggressive defaults' mean?

From @dpugh:

With aggressive matching turned on, the default was use as long as what the user typed in the applicable to span was a prefix of the item

I believe the purpose of this option is to provide a way to experiment with different selection logic.

the default was use as long as what the user typed in the applicable to span was a prefix of the item

i still don't know what this means. can we have a comment explaining this somewhere (ideally with examples).

CyrusNajmabadi · 2021-11-11T01:30:38Z

Can you update the OP of thei PR to indicate what user perceivable change this has to the completion experience?

genlu · 2021-11-11T19:04:09Z

@CyrusNajmabadi Added some description of the change. You can also find more examples in the tests.

genlu · 2021-11-12T20:37:00Z

@dotnet/roslyn-ide Please review, thanks!

genlu · 2021-11-18T01:44:23Z

@CyrusNajmabadi could you please take a look?

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

CyrusNajmabadi · 2021-11-22T19:47:40Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

-                    highlightMatchingPortions,
-                    completionHelper);
+                    selectionHint,
+                    centerSelection: true,


what does 'centerSelection' mean?

this was not answered.

"Whether selected item should be displayed in the center of the list. Usually, this is true" from the comment

CyrusNajmabadi · 2021-11-22T19:48:09Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

+                {
+                    Contract.ThrowIfNull(document);
+                    return (itemsWithPatternMatches, text) => completionService.FilterItems(document, itemsWithPatternMatches, text);
+                }


i have absolutely no idea what is going on here.

CyrusNajmabadi · 2021-11-22T19:48:42Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

        }

-        private FilteredCompletionModel? HandleNormalFiltering(
+        private (int selectedItemIndex, UpdateSelectionHint selcetionHint, VSCompletionItem? uniqueItem)? HandleNormalFiltering(


selcetionHint mispelled.

Also, what does UpdateSelectionHint even mean?

was this PR comment handled?

CyrusNajmabadi · 2021-11-22T19:49:40Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

+                for (var i = 0; (i < itemsWithMatch.Count); ++i)
+                {
+                    var itemWithMatch = itemsWithMatch[i];
+                    if (itemWithMatch.RoslynCompletionItem.FilterText == defaultText)


why are we looking at filter-text? we should likely be looking at display text right (in other words, we're seeing that two things in the display match up, what is in teh buffer, and waht is shown the user. if these are the same, we should pick ours?

Makes sense, I changed it to DisplayText. FYI @dpugh

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

CyrusNajmabadi · 2021-11-22T19:53:56Z

The complexity in ItemManager is really too much to handle at this point. We shoudl extract out a new type with clear responsibility as to what it's doing, and what the phases are taht it's doing that work in.

There should ideally be a very clear top level method that explains something like:

here are the 5 steps we take here.
1. step 1, do blah (a single method call)
2. step 2, filter it down to blah
3. step 3, select blah blah
4. step 4, etc.

It really needs to be exceptionally simple and clear waht each phase is trying to accomplish, and what minimal data it needs to have access to to perform that. right now, i just see a ton of data/logic intermixed and i really have no idea if things are happening at the right layer or how the ordering of anything impacts the rest.

No change to the code other than extracting a helper class here.

This is a refactoring only commit

genlu · 2021-11-23T23:12:04Z

@CyrusNajmabadi Did some refactoring around ItemManager as you suggested, there's no other code change in the last two commits. Hopefully it's provide a bit more clarity here. I will do some additional clean-up and adding comments next.

genlu · 2021-11-24T01:58:15Z

@CyrusNajmabadi Based on your feedback, I have tuned it down to match case-sensitively for default items. FYI @dpugh

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

CyrusNajmabadi · 2021-12-06T20:19:30Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                    var associatedWithUnselectedExpander = false;
+                    foreach (var itemFilter in item.Filters)
+                    {
+                        if (itemFilter is CompletionExpander)


what is a completion-expander?

It's this plus button at the beginning of the filter list

CyrusNajmabadi · 2021-12-06T20:21:51Z

this is looking really good. so much easier to understand than before.

my top bit of feedback is: ground the algorithms with concrete examples of what sort of cases the user is running into, and why we're doing things. so cases that go "The item is 'Console' and the user has typed 'C'" are fantastic. They really help the reader get a grasp of what is being handled here and give much more of an intuition about why this behavior is desirable.

We need to clarify the design before implementing it

CyrusNajmabadi · 2021-12-07T19:49:34Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                // Only use data.Snapshot in the theoretically possible but rare case when all items we are handling are from some non-Roslyn CompletionSource.
+                var snapshotForDocument = TryGetInitialTriggerLocation(_data, out var intialTriggerLocation)
+                    ? intialTriggerLocation.Snapshot
+                    : _data.Snapshot;


non-roslyn completionsource is a terrifying concept to me :) this is not on you, but in cases like this we really need to call out what those cases are. because i genuinely have no idea how we get into this state.

You are not alone :)
I'm not certain about this either, therefore reluctant to make change here until understand this better.

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

CyrusNajmabadi · 2021-12-07T19:52:08Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/Helpers.cs

-            }
+                EditorAsyncCompletionData.CompletionTriggerReason.InvokeAndCommitIfUnique => CompletionTriggerKind.InvokeAndCommitIfUnique,
+                EditorAsyncCompletionData.CompletionTriggerReason.Insertion => CompletionTriggerKind.Insertion,
+                EditorAsyncCompletionData.CompletionTriggerReason.Deletion or EditorAsyncCompletionData.CompletionTriggerReason.Backspace => CompletionTriggerKind.Deletion,


so this seems to unify Delete/Backspace to just Delete (which is fine). but later on i see roslyn code checkking for backspace as well... which then seems weird to me.

CyrusNajmabadi · 2021-12-07T19:52:35Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                    // Decide the item to be selected for this completion session.
+                    // The selection is mostly based on how well the item matches with the filter text, but we also need to
+                    // take into consideration for things like CompletionTrigger, MatchPriority, MRU, etc. 
+                    var initialSelection = InitialTriggerReason == CompletionTriggerReason.Backspace || InitialTriggerReason == CompletionTriggerReason.Deletion


checking for Backspace and Delete seems weird to me given that it looked like your earlier helpers collapsed things down so we only see one of these.

Previously we have mixed usage of Roslyn's CompeltionTriggerKind and Editor's CompletionTriggerReason in the ItemManager, which is confusing. So I changed to use Editor's CompletionTriggerReason only in async completion layer, unless we are trying to interact with APIs in Roslyn's completion system (i.e. In Features layer)

CyrusNajmabadi · 2021-12-07T19:54:04Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+
+                // Use a dedicated pool to minimize potentially repeated large allocations,
+                // since the completion list could be long with import completion enabled.
+                var itemsToBeIncluded = s_listOfMatchResultPool.Allocate();


much nicer :)

CyrusNajmabadi · 2021-12-07T20:00:26Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                // Convert intial and update trigger reasons to corresponding Roslyn type so 
+                // we can interact with Roslyn's completion system
+                var roslynInitialTriggerKind = Helpers.GetRoslynTriggerKind(InitialTriggerReason);
+                var roslynFilterReason = Helpers.GetFilterReason(UpdateTriggerReason);


i feel like we should do this up front, and only expose the roslyn side of htings. is there a reason we ever want to work with the non-roslyn side?

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

CyrusNajmabadi · 2021-12-07T20:05:01Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                    if (_completionRules.DismissIfEmpty)
+                    {
+                        return null;
+                    }


how is this different from the line at: https://github.com/dotnet/roslyn/pull/57685/files#diff-e43ef74201be5195fc625ca928b4c20e14e5dd304a4d42302dba2cfcc2c03557R372 ?

This applies to all initial trigger kinds, where the dismissal in HandleDeletionTrigger applies to deletion trigger only (and we keep all items in the list in that case, same as handling ctrl+j). It seems we might be able to rewrite this so the dismissal in HandleDeletionTrigger can happen earlier (i.e. handled by this line), but I'd need to think about it more and maybe do it in a follow up PR

CyrusNajmabadi · 2021-12-07T20:09:52Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+
+                // If the user has turned on some filtering states, and we filtered down to
+                // nothing, then we do want the UI to show that to them.  That way the user
+                // can turn off filters they don't want and get the right set of items.


this comment doesnt' seem to match the logic in the method. we seem to unilaterally bail out above, even if filters are set.

we only bail when the update was triggered by insertion. There's a separate trigger reason for FilterChange

genlu · 2021-12-09T20:06:45Z

@CyrusNajmabadi Hey, is this good to merge now?

genlu added 4 commits November 5, 2021 16:50

Copy the handling of defaults source

410fcc2

refactor completion list filtering

d197676

Move the handling of IAsyncCompletionDefaultsSource to Roslyn

ff7f245

See https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform/pullrequest/353067

Fix

3d9f6aa

genlu requested a review from a team as a code owner November 10, 2021 22:58

ghost added the Area-IDE label Nov 10, 2021

dpugh reviewed Nov 10, 2021

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 11, 2021

View reviewed changes

CyrusNajmabadi reviewed Nov 22, 2021

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 22, 2021

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 22, 2021

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 22, 2021

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs Outdated Show resolved Hide resolved

genlu added 3 commits November 22, 2021 12:11

Merge remote-tracking branch 'dotnet/main' into DefaultItems

75c2df5

Move main ItemManager logic into a separate helper class

2579b6d

No change to the code other than extracting a helper class here.

Refactor ItemManager

06e0f49

This is a refactoring only commit

genlu added 3 commits November 23, 2021 17:11

Use a disposable wrapper for pooled object

6610099

Use DisplayText to match defaults

2f200fa

Use CaseInsensitive match for defaults

d7920b7

CyrusNajmabadi reviewed Dec 6, 2021

View reviewed changes

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 6, 2021

View reviewed changes

genlu added 5 commits December 6, 2021 18:15

Address review comments

97d0dd2

Avoid unnecessary computation

5619ddf

Add comments

0ac323f

Combine loops

7658f5b

Remove the handling of AggressiveDefaultsMatching option

bc0593f

We need to clarify the design before implementing it

genlu mentioned this pull request Dec 7, 2021

Handling AggressiveDefaultsMatchingOption for completion #58169

Closed

CyrusNajmabadi reviewed Dec 7, 2021

View reviewed changes

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 7, 2021

View reviewed changes

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 7, 2021

View reviewed changes

genlu added 2 commits December 7, 2021 12:48

Address review comments

8b54846

Replace delegate object with static local function

108a8fb

runfoapp bot mentioned this pull request Dec 7, 2021

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

genlu enabled auto-merge December 8, 2021 01:05

CyrusNajmabadi approved these changes Dec 29, 2021

View reviewed changes

genlu merged commit a2d06f6 into dotnet:main Dec 29, 2021

ghost added this to the Next milestone Dec 29, 2021

genlu deleted the DefaultItems branch December 29, 2021 18:55

Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022

genlu mentioned this pull request Dec 30, 2022

Prefer default over starred item #66159

Merged

Conversation

genlu commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dpugh commented Nov 11, 2021 via email • edited by genlu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 11, 2021

Uh oh!

genlu commented Nov 11, 2021

Uh oh!

genlu commented Nov 12, 2021

Uh oh!

genlu commented Nov 18, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 22, 2021

Uh oh!

genlu commented Nov 23, 2021

Uh oh!

genlu commented Nov 24, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu commented Dec 9, 2021

genlu commented Nov 10, 2021 •

edited

Loading

dpugh commented Nov 11, 2021 via email •

edited by genlu

Loading