Tweak completion filtering behavior.#16892
Conversation
CyrusNajmabadi
added
the
PR For Personal Review Only
|
Tagging @dotnet/roslyn-ide . |
|
cc @mattwar for API changes |
| public override string ToString() => DisplayText; | ||
|
|
||
| internal bool ShouldBeFilteredOutOfCompletionList( | ||
| ImmutableDictionary<CompletionItemFilter, bool> filterState) |
There was a problem hiding this comment.
Does this function belong on this type? It doesn't use any of the class's state. It seems to be an operation concerned with the behavior of filtering and not the representation of the data that is the completion item.
There was a problem hiding this comment.
I'll find a good home for it.
| /// <summary> | ||
| /// The default <see cref="CompletionTrigger"/> when none is specified. | ||
| /// </summary> | ||
| [Obsolete("Do not use.", error: true)] |
There was a problem hiding this comment.
[Obsolete("Do not use.", error: true)] [](start = 8, length = 38)
It seems bizarre to have a Default that should not be used. Should the default just be changed to default to Invoke instead?
There was a problem hiding this comment.
I can do that. part o the reason i wanted to make this change was to force people to have to think about what they were doing. But i think it's acceptably reasonable to have hte 'Default' and having it be set to 'Invoke'.
|
Ok. Have responded to PR feedback. |
CyrusNajmabadi
removed
the
PR For Personal Review Only
| Deletion, | ||
| NonInsertionOrDeletion, | ||
| #if false | ||
| // If necessary, we could add additional filter reasons. For example, for the below items. |
There was a problem hiding this comment.
This comment feels much like "I'm commenting out code I thought I needed". Perhaps rewrite to more clear prose?
| // 2. They brough up completion with ctrl-j or through deletion. In these | ||
| // cases we just always keep all the items in the list. | ||
|
|
||
| var wasTriggeredByDeleteOrSimpleInvoke = |
There was a problem hiding this comment.
Reorder these to match the cases above.
There was a problem hiding this comment.
But massive thanks for actually explaining in English what the heck this is trying to do.
| internal enum CompletionFilterReason | ||
| { | ||
| Insertion, | ||
| Deletion, |
There was a problem hiding this comment.
Please add a doc comment explaining this is either delete or backspace.
| { | ||
| internal partial class Session | ||
| { | ||
| private struct FilterResult |
There was a problem hiding this comment.
File name here isn't matching the nesting structure.
5 participants
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems?id=362890