Keep track of trigger location for individual CompletionItem#55035
Keep track of trigger location for individual CompletionItem#55035genlu merged 4 commits intodotnet:release/dev16.11from
Conversation
02f9a5c to
cc6b41e
Compare
Fix committing completion item and expander in projection scenarios
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
| // 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 |
allisonchou
left a comment
There was a problem hiding this comment.
LGTM, although I'm not very familiar with the code so just basing my approval off what I can deduce from the PR 😃
…etion/CompletionSource.cs Co-authored-by: Allison Chou <allichou@microsoft.com>
…etion/ItemManager.cs Co-authored-by: Allison Chou <allichou@microsoft.com>
|
Thanks @allisonchou! |
Fix committing completion item and expander in projection scenarios. Address #51702