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.

Sign up for GitHub

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

Jump to bottom

Don't show expanded items in certain cases #67049

Merged
merged 3 commits into from
Mar 28, 2023
Merged

Don't show expanded items in certain cases #67049

merged 3 commits into from
Mar 28, 2023

Conversation

genlu
Copy link
Member

@genlu genlu commented Feb 25, 2023 •
edited by Pilchie
Loading

This change hides expanded items (i.e. unimported items) in certain scenarios even if the option is enabled AND Editor's responsive completion option is also enabled. The goal is to address both usability (showing too many unimported items and cluttered the list, when we don't have enough hint to filter down the list) and perf issues.

Addressing the perf cost in matching and sorting items AB#1749264

Commit-by-commit review is recommended.

  • The first commit is pure refactoring w/o behavior change. After the change, we will always have a separate task for computing expanded items, checking for responsive typing option value and combining them into the list is now handled by ItemManager.
  • The actual change as described above is in 84e59e9. 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, but for 0 (happens quite often since we now trigger completion in argument list by default, where unimported types are rarely needed) or 1 typed char, I don't really see how including thousands of those items would be useful
    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 suggest an expanded item for us to select)

In type context, only show expanded items after at least two characters are typed:

TypeContext

In member context, no change behavior:

MemberContext

@genlu genlu requested a review from a team as a code owner February 25, 2023 00:57
@genlu genlu changed the title Don't show expanded items in certain scenarios Don't show expanded items when filter text is very short Mar 2, 2023
@genlu
Copy link
Member Author

genlu commented Mar 10, 2023

The perf issue is mostly addressed by #67249, but I think there's still value in usability to not show expanded item when filter text is empty

@genlu genlu changed the title Don't show expanded items when filter text is very short Don't show expanded items when filter text length <= 1 Mar 10, 2023
@genlu genlu changed the title Don't show expanded items when filter text length <= 1 Don't show expanded items when filter text length <2 Mar 10, 2023
@genlu genlu marked this pull request as draft March 13, 2023 17:31
@genlu genlu changed the title Don't show expanded items when filter text length <2 Don't show expanded items in certain cases Mar 24, 2023
@genlu genlu marked this pull request as ready for review March 24, 2023 22:09
@genlu
Copy link
Member Author

genlu commented Mar 24, 2023 •
edited
Loading

@dotnet/roslyn-ide @CyrusNajmabadi @sharwell This is now ready for review. Please see the description for the new behavior. Thanks!

genlu
genlu commented Mar 24, 2023
View reviewed changes
src/EditorFeatures/Core/IntelliSense/AsyncCompletion/AsyncCompletionLogger.cs

internal static void LogDelayedImportCompletionIncluded()
=> s_countLogAggregator.IncreaseCount(ActionInfo.SessionWithTypeImportCompletionEnabled);

Copy link
Member Author

Choose a reason for hiding this comment

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

These telemetry are no longer needed

@genlu
Hide expanded item from completion list in certain case
84e59e9 
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)
CyrusNajmabadi
CyrusNajmabadi reviewed Mar 27, 2023
View reviewed changes
src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CompletionSource.cs
_textView.Options.GlobalOptions.SetOptionValue(s_nonBlockingCompletionEditorOption, !_editorOptionsService.GlobalOptions.GetOption(CompletionViewOptionsStorage.BlockForCompletionItems, service.Language));
_responsiveCompletionEnabled = _textView.Options.GetOptionValue(DefaultOptions.ResponsiveCompletionOptionId);
var blockForCompletionItem = _editorOptionsService.GlobalOptions.GetOption(CompletionViewOptionsStorage.BlockForCompletionItems, service.Language);
_textView.Options.GlobalOptions.SetOptionValue(s_nonBlockingCompletionEditorOption, !blockForCompletionItem);
Copy link
Member

Choose a reason for hiding this comment

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

we have another option as well? This now feels like 3 separate options ion this space :) Def confusing. I don't get what each is supposed to control..

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have context on this option (CompletionViewOptionsStorage.BlockForCompletionItems), but it seems it was only used for TS and FS to make completion in their files non-blocking.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha... i would love if you could talk to them if we can remove this and they can just use the VS concepts here :) (not blocking (no pun intended) for this pr).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 27, 2023
View reviewed changes
src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CompletionSource.cs
AsyncCompletionLogger.LogImportCompletionGetContext(isBlocking: false, delayed: false);

return CombineCompletionContext(session, nonExpandedContext, expandedContext);
_ = _expandedItemsTaskCancellationSeries.CreateNext(CancellationToken.None);
Copy link
Member

Choose a reason for hiding this comment

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

this needs docs as well. i think i get it. you're cancelling any outstanding work since it couldn't result in items that would matter (since you're in exclusive mode). but needs docs to explain and be certain.

Copy link
Member

Choose a reason for hiding this comment

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

question: if we're going to cancel the work anyways... can we not check if things are exclusive first before kicking off the work in the first place?

Copy link
Member Author

@genlu genlu Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, your understanding is correct. I will doc this. The reason we kick off the "unimported" task first is because, waiting for the value of exclusivity from regular providers means computation of unimported items won't start until we return those regular items to editor. This combined with the existing behavior of not showing unimported items until they are calculated and only add them during completion list refresh, would mean increased chance users won't see those items for the first few characters they typed.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense. thanks.

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 27, 2023
View reviewed changes
src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.cs Outdated
// 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)
Copy link
Member

Choose a reason for hiding this comment

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

i don't understand #3. can you expand on that more. Primarily, i don't know what defaults actually means in the context of completion.

CyrusNajmabadi
CyrusNajmabadi requested changes Mar 27, 2023
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Looking good. But i def have some docs requests, as well as some ideas on where we could either be simpler, or we need docs to explain current complexity.

@genlu genlu requested a review from CyrusNajmabadi March 27, 2023 23:58
@genlu genlu enabled auto-merge March 28, 2023 00:19
@genlu genlu merged commit 736c17b into dotnet:main Mar 28, 2023
@ghost ghost added this to the Next milestone Mar 28, 2023
@genlu genlu deleted the ItemManager branch March 28, 2023 17:02
@allisonchou allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17.6 P3
Development

Successfully merging this pull request may close these issues.
4 participants
@genlu @sharwell @CyrusNajmabadi @allisonchou