-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix caret affinity in GetReferencedSymbolsToLeftOfCaretAsync #52245
Fix caret affinity in GetReferencedSymbolsToLeftOfCaretAsync #52245
Conversation
5bd566b
to
d10df88
Compare
Fixes the inability to insert a snippet for 'object.Equals'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good but the "two different types of method names" thing does look a bit funky. If that can be simplified do it, otherwise if it needs to be different a comment would be good.
src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetExpansionClient.cs
Outdated
Show resolved
Hide resolved
/// </summary> | ||
public void ResetOptions() | ||
{ | ||
ResetOption(CompletionOptions.EnableArgumentCompletionSnippets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than calling this one out specifically, I think we can reset generally through the option service. This is the one reason option keys themselves get exported, to allow a general reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but I encountered problems with it last time I tried. Am reserving that [highly desired] change for a separate PR.
@@ -541,7 +541,7 @@ private bool TryInsertArgumentCompletionSnippet(SnapshotSpan triggerSpan, Snapsh | |||
if (expansion.InsertSpecificExpansion(doc, textSpan, this, LanguageServiceGuid, pszRelativePath: null, out _state._expansionSession) == VSConstants.S_OK) | |||
{ | |||
Debug.Assert(_state._expansionSession != null); | |||
_state._methodNameForInsertFullMethodCall = methodName; | |||
_state._methodNameForInsertFullMethodCall = methodSymbols.First().Name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit odd that we have two ways we get the method name, one from symbols (which is correct) and the other one which maybe isn't. Should we just initialize methodName this way on 535? Or if it's not the same sometimes, add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 One is the method name as it appears in source code, and the other is the method name as it appears in the symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
soundsl ike we should prefer the former? (but i don't care much).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently argument completion does not work for the following scenario:
;
ConfigureAwait
This change fixes caret affinity to allow it to work.
There are now two additional bug fixes in this pull request:
object.Equals
(previously the code would insert theEquals
snippet, which made no sense at this location).ctor
, and dismiss the session before providing arguments)