Consolidate data tracked over an async completion session #59553
Conversation
src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionItemData.cs
Outdated
Show resolved
Hide resolved
| private CompletionItemData(RoslynCompletionItem roslynItem, SnapshotPoint? triggerLocation) | ||
| { | ||
| RoslynItem = roslynItem; | ||
| TriggerLocation = triggerLocation ?? new(); |
There was a problem hiding this comment.
❕ Don't use new T() (or new()) for value types unless they have a default constructor. Also, I'm concerned this line may be treating new() as new SnapshotPoint() and not new Optional<SnapshotPoint>() (even if it's not, it's confusing).
There was a problem hiding this comment.
You are right, new() here actually is new SnapshotPoint(). Fixed
| public bool HasSuggestionItemOptions { get; set; } | ||
|
|
||
| public Optional<SnapshotPoint> ExpandedItemTriggerLocation { get; set; } | ||
| public Optional<TextSpan> CompletionListSpan { get; set; } |
There was a problem hiding this comment.
can all of these be optional at different times? Or does one being provided mean the rest are provided as well?
There was a problem hiding this comment.
I think the following three could be in one such group, but I'm not sure I'd like to have another type to bundle them together (if that's what you are suggesting)
public Optional<SnapshotPoint> ExpandedItemTriggerLocation { get; set; }
public Optional<TextSpan> CompletionListSpan { get; set; }
public Optional<ImmutableArray<char>> ExcludedCommitCharacters { get; set; }
| internal const string ExpandedItemsTask = nameof(ExpandedItemsTask); | ||
|
|
||
| // Don't change this property! Editor code currently has a dependency on it. | ||
| internal const string ExcludedCommitCharacters = nameof(ExcludedCommitCharacters); |
There was a problem hiding this comment.
OK, turns out editor depend on this property so we have to keep it for now. Only took me one full day of debugging to figure it out...
https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform?path=/src/Editor/Language/Impl/Language/AsyncCompletion/AsyncCompletionSession.cs&version=GBmain&line=1168&lineEnd=1169&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents
There was a problem hiding this comment.
@olegtk @AmadeusW This feels so wrong. Any idea how can we remove this dependency?
tracking bug: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1484552
ghost
added this to the 
Fix #59262
Based on the public async completion API, it's not mentioned that the same session object would be used over an async completion session, even though the underlying implementation guarantees it. So I have aggregated all data into a single object but still uses the property bag to track it over the session, instead of using a CWT.