Keep track of trigger location for individual CompletionItem #55035
Conversation
genlu
force-pushed
the
fixRazorCompletion
branch
from
02f9a5c
to
cc6b41e
Compare
genlu
changed the title
Fix committing completion item and expander in projection scenarios
genlu
force-pushed
the
fixRazorCompletion
branch
from
cc6b41e
to
fff91e2
Compare
| var snapshotForDocument = data.InitialSortedList.FirstOrDefault(item => item.Properties.ContainsProperty(CompletionSource.TriggerLocation)) is VSCompletionItem firstItem && | ||
| firstItem.Properties.GetProperty(CompletionSource.TriggerLocation) is SnapshotPoint triggerLocation | ||
| ? triggerLocation.Snapshot | ||
| : data.Snapshot; |
There was a problem hiding this comment.
Data is captured here in the lambda, most of the other lambdas are static in the method here.
There was a problem hiding this comment.
this is just really hard to read. can you make this a simple helper function.
There was a problem hiding this comment.
which should always be available
If it shoudl always be available, we should either assert/contract so we get a stack when something breaks expectations. being defensive just means unexplainable bugs happen and we have no idea what is going on.
There was a problem hiding this comment.
Data is captured here in the lambda, most of the other lambdas are static in the method here.
I don't see anything got captured, but I did extract a local function here
If it shoudl always be available, we should either assert/contract so we get a stack when something breaks expectations. being defensive just means unexplainable bugs happen and we have no idea what is going on.
Add some comments to clarify
src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs
Outdated
Show resolved
Hide resolved
| // since the intial trigger we are able to get from IAsyncCompletionSession might not be the same (e.g. in projection scenarios,) | ||
| // so when they are requested via expander later, we can retrieve it,. | ||
| // Techinically we should save the trigger location for each individual service that made such claim, but in reality , only Roslyn's completion service uses | ||
| // expander, so we can get away with not making such distinction. |
There was a problem hiding this comment.
i really don't get teh explanation.
There was a problem hiding this comment.
i think this might actually be an issue with how Editor handle expander in old razor editor.
In the example below, when completion is triggered, our implementation of GetCompletionContextAsync is called twice, once with trigger location in C# buffer, another with trigger location in TS buffer. However, when user then hit expander, GetExpandedCompletionContextAsync is only called once with triggerLocation pointing to the HTML buffer, as a result we don't know which underlying document we should ask for expanded item for (i.e. cs vs ts).
But as mentioned in the comment, only Roslyn's provider actually use Expander, so in reality this will not cause any problem.
|
@jinujoseph @vatsalyaagrawal I think we should take this for 16.11. Please confirm then I will create a shiproom bug for it. Thanks |
src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs
Outdated
Show resolved
Hide resolved
…etion/CompletionSource.cs Co-authored-by: Allison Chou <[email protected]>
…etion/ItemManager.cs Co-authored-by: Allison Chou <[email protected]>
|
Thanks @allisonchou! |
Fix committing completion item and expander in projection scenarios. Address #51702