Skip to content

Additional filter texts#63217

Merged
genlu merged 10 commits intodotnet:mainfrom
genlu:AdditionalFilterTexts
Aug 6, 2022
Merged

Additional filter texts#63217
genlu merged 10 commits intodotnet:mainfrom
genlu:AdditionalFilterTexts

Conversation

@genlu
Copy link
Copy Markdown
Member

@genlu genlu commented Aug 4, 2022

Here's an example. The snippet is changed to has "cw" as regular FilterText, and "Console" and "WriteLine" as AdditionalFilterTexts

MultipleFilterTexts

@ghost ghost added the Area-IDE label Aug 4, 2022
@genlu genlu force-pushed the AdditionalFilterTexts branch from e846955 to 055eeca Compare August 4, 2022 22:29
Copy link
Copy Markdown
Member

@akhera99 akhera99 left a comment

Choose a reason for hiding this comment

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

LGTM with tests!

get => _additionalFilterTexts;
set
{
Debug.Assert(!value.IsDefault && (value.IsEmpty || HasDifferentFilterText),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like this designation, makes sense to me.

@genlu genlu marked this pull request as ready for review August 5, 2022 22:16
@genlu genlu requested a review from a team as a code owner August 5, 2022 22:16
@genlu genlu force-pushed the AdditionalFilterTexts branch from 404d62b to 16a65e8 Compare August 5, 2022 22:57
@genlu genlu enabled auto-merge August 5, 2022 22:58
@dibarbet
Copy link
Copy Markdown
Member

dibarbet commented Aug 6, 2022

@akhera99 @genlu we should think about a proposal for the LSP spec as well, currently they only have a single filterText field
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItem

@genlu
Copy link
Copy Markdown
Member Author

genlu commented Aug 6, 2022

@dibarbet Sure, what's the process of making such a proposal?

@dibarbet
Copy link
Copy Markdown
Member

dibarbet commented Aug 6, 2022

@genlu we can file an issue on the LSP area path in ADO and ping Jose to bring to to an LSP sync meeting. We should bring a general outline of what we think the additional properties would look like and how they'd function

Once we've implemented it internally in the VS client and server we can consider porting to the public spec.

@genlu
Copy link
Copy Markdown
Member Author

genlu commented Aug 6, 2022

Sure will do.

Once we've implemented it internally in the VS client and server we can consider porting to the public spec.

The scenario that requires AdditionalFIiterTexts seems to be very limited at the moment, and everything that do require it is internal, so I'm a little concerned if this is something we want to publicly expose (until at least snippets and AdditionalFIiterTexts are made public).

My hesitance is also because right now the UX for items matched with AdditionalFIiterText isn't great, we make a best effort attempt to highlight base on the display text, but if we don't find a match there, it might be unclear to users why the item is being selected/available.

@genlu genlu merged commit b2693e6 into dotnet:main Aug 6, 2022
@ghost ghost added this to the Next milestone Aug 6, 2022
@genlu genlu deleted the AdditionalFilterTexts branch August 6, 2022 01:03
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants