disable completion for immediate window commands#32631
disable completion for immediate window commands#32631ivanbasov merged 10 commits intodotnet:masterfrom
Conversation
AmadeusW
left a comment
There was a problem hiding this comment.
Please use IFeatureService instead of interacting with the property bag
There was a problem hiding this comment.
If you use the feature service, you'll be able to remove this code #Resolved
There was a problem hiding this comment.
you'll be able to remove this code when you use the completion service
There was a problem hiding this comment.
please use feature service to disable "AsyncCompletion" for in this textView #Resolved
There was a problem hiding this comment.
please maintain the status of completion feature here in this class, and call featureService.Disable right here #Resolved
There was a problem hiding this comment.
Disabling of feature should happen in AbstractLanguageService2.IVsImmediateStatementCompletion2.cs`
jasonmalinowski
left a comment
There was a problem hiding this comment.
@ivanbasov So this will still leave signature help and formatting running. They wouldn't have been running in the old code but still will be now. Is that still a problem? Or put another way, what is the expected contract for what EnableStatementCompletion is supposed to do?
There was a problem hiding this comment.
nameof for the variable name. #Resolved
src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerTextView.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/LanguageService/AbstractLanguageService`2.cs
Outdated
Show resolved
Hide resolved
Is there a "why" that explains this? Thanks! #Resolved |
|
@jasonmalinowski , @dpoeschl please review #Resolved |
I don't quite understand either. I think it's to prevent two completion windows from showing up based on the linked bug, but that's all I understand from reading the code. #Resolved |
|
@ivanbasov can you write up a summary (in the original PR description) about all that we learned here? #Resolved |
...Implementation/LanguageService/AbstractLanguageService`2.IVsImmediateStatementCompletion2.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
If we get a call to Disable but somebody else had already temporarily disabled it for some other reason, shouldn't we still make a token? #Resolved
There was a problem hiding this comment.
We cannot match pairs of disable and enable calls. So, if there are two disable calls in a row and then a single enable one, we should enable after. Therefore, do not need to disable if already disabled and do not need to enable if already enabled. #Resolved
There was a problem hiding this comment.
How is this working? If GetOpenDocumentInCurentContextWithChanges returned a document, it implies this document is already open... #Resolved
There was a problem hiding this comment.
It seems that GetOpenDocumentInCurrentContextWithChanges does not open the document. Not? #Resolved
There was a problem hiding this comment.
It does not. But if the document isn't open, it'll return null and you would have taken that early return above. It seems like this code is trying to open a document that is already open based on the earlier check. #Resolved
There was a problem hiding this comment.
Thank you! Verified. It seems to be unnecessary. Removing.
In reply to: 270161524 [](ancestors = 270161524)
|
I need to rebase once again #Resolved |
1ae7119 to
7722276
Compare
@dpoeschl, @CyrusNajmabadi |
|
@jasonmalinowski could you please review? #Resolved |
There was a problem hiding this comment.
It does not. But if the document isn't open, it'll return null and you would have taken that early return above. It seems like this code is trying to open a document that is already open based on the earlier check. #Resolved
There was a problem hiding this comment.
If some other caller has also disabled completion through this API, why are we bailing? It seems like the rest of this should still run. Otherwise the EnableCompletion() was ignored; if the other disable is re-enabled, then it'll still not work? #Resolved
There was a problem hiding this comment.
-
Please note we are disabling the feature per WpfTextView not globally.
-
If some other caller disabled completion and we do not have a token, what can we do? We can ignore, notify debugger or crash. I discussed sending notifications about "failed to enable" to Dustin from the debugger team. Our API allows to do so. He said they are not interested to be informed about this.
In reply to: 270161962 [](ancestors = 270161962)
There was a problem hiding this comment.
I imagine the intent of the feature service API to be cooperative: any number of disables can happen, independently. The feature in the end is only active if there aren't any disables. If somebody else has disabled completion too -- maybe globally, maybe on the same view -- that's not the concern of this code. Just cancel your disabling, and whatever is going on globally is their problem.
Put another way, if we didn't have these enable/disable functions already here and weren't trying to maintain compat with the existing behavior, I imagine we'd just tell the debugger to use the feature service directly. And they'd just enable/disable their token and wouldn't care if somebody else also has a disable token. If somebody else had a disable token, they'd take it up with that person, not us anyways. #Resolved
There was a problem hiding this comment.
I do not think that the feature service API implements the idea you described. They provide only the Disable action (local and global). However, to Enable the feature back you need to know the token. If you do not have the token in hand, you are helpless.
With this restrictions, a better transparent API that we could build should respond to the debugger that we either succeeded to enable or disable or failed to do so. I had this implementation in an intermediate version a couple months ago. However, Dustin said the debugger is not ready to work with it.
I agree the debugger could do all (most of) the work on their side: they can create a token and then dispose it.
In reply to: 270216214 [](ancestors = 270216214)
There was a problem hiding this comment.
I think the debugger may not know that they call Roslyn (not some other provider) and should disable the feature service for the case. If they should do the same for all providers, they definitely should do this on their side.
In reply to: 270228721 [](ancestors = 270228721,270216214)
There was a problem hiding this comment.
They provide only the Disable action (local and global). However, to Enable the feature back you need to know the token. If you do not have the token in hand, you are helpless.
I think the editor API is well designed. But here you should just be enable/disable with a token, and not looking at the effective result. Figuring out the effective result is the completion provider, but shouldn't be done here.
All I'm saying is just delete the IsEnabled call -- you just don't need it here, but it means something else also calling enable/disable can introduce bugs. #Resolved
There was a problem hiding this comment.
OK. Thank you! Let us try to remove the check and the call of GetOpenDocumentInCurrentContextWithChanges
In reply to: 270232538 [](ancestors = 270232538)
There was a problem hiding this comment.
There was a problem hiding this comment.
Why the synchronization? I would have imagined we only call this on the UI thread? #Resolved
There was a problem hiding this comment.
I want to believe that all calls are performed sequentially. However, I do not see any checks for the UI thread on our side in this API, and have no guarantees that this actually happens. Should we just hope so and remove extra checks?
In reply to: 270162103 [](ancestors = 270162103)
There was a problem hiding this comment.
This entire class is UI affinitized -- no two methods can run at the same time. The other fields are all mutable and have no guards either.
If you want to actually put foreground assserts I wouldn't complain. #Resolved
There was a problem hiding this comment.
I could not see an elegant way to get a threading context to DebuggerIntelliSenseFilter or to AbstractLanguageService<TPackage, TLanguageService> that could allow to add asserts. If you know, please let me know.
Removing locks.
In reply to: 270217422 [](ancestors = 270217422)
There was a problem hiding this comment.
How? Didn't we conclude this didn't work anymore? #Resolved
There was a problem hiding this comment.
Thank you! Double checked by disabling this line. It does not change anything. Removing it.
In reply to: 270162682 [](ancestors = 270162682)
There was a problem hiding this comment.
You may want to leave it for now just in case we're wrong, but the comment most definitely isn't correct. #Resolved
There was a problem hiding this comment.
This is a pure function....this can definitely be deleted. #Resolved
There was a problem hiding this comment.
(although whatever you think it's doing it's not...) #Resolved
There was a problem hiding this comment.
|
@jasonmalinowski please review |
src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs
Outdated
Show resolved
Hide resolved
|
At this point the code matches what I'd expect to see, but my concern is it's still missing something to handle the legacy case if we care about it. I don't think the legacy controller is checking the modern bit, a concern I also had here. That might need a fix too? |
jasonmalinowski
left a comment
There was a problem hiding this comment.
You can't use IWpfTextView at the EditorFeatures (non-WPF) layer. It looks like your use was unnecessary so you can just delete the code.
There was a problem hiding this comment.
Why are we going to the adapter and back?
There was a problem hiding this comment.
This is forbidden at this layer since you can't be using WPF specific code.
src/EditorFeatures/Core/Implementation/IntelliSense/Completion/AsyncCompletionService.cs
Outdated
Show resolved
Hide resolved
|
@ivanbasov Should this just target master now? Thank you! Will re-target |
|
@jinujoseph, @vatsalyaagrawal please approve (Ask mode for 16.1 preview 3) |
* dotnet/master: (495 commits) Roslyn Installer: Stop processes that block VSIX installation. (dotnet#34886) Remove unused helper BeginInvokeOnUIThread Apply a hang mitigating timeout to InvokeOnUIThread Apply a hang mitigating timeout in RestoreNuGetPackages Apply a hang mitigating timeout to WaitForApplicationIdle Fix Value/Ref checks for pattern Index/Range (dotnet#35004) Fix assert in remove unused member analyzer Treat unconstrained type parameters declared in `#nullable disable` context as having an oblivious nullability in case they are substituted with a reference type. (dotnet#34797) Add symbol name to tests. Fix to be the correct name emitted PR feedback Improve IDE0052 diagnostic message for properties with used setter but unused getter Use original definition symbol for fetching control flow graph of generic local functions. Properly treat ambiguous explicit interface implementations (dotnet#34584) Remove the dependence between the order in NullableAnnotation and metadata attribute values (dotnet#34909) Fix warning level test. Fix bootstrap on Linux/Mac (dotnet#34978) disable completion for immediate window commands (dotnet#32631) Updated PreviewWarning color Implement design changes for "pattern" Index/Range indexers (dotnet#34897) Add IVTs to 16.1 version of RemoteLS ...
Fix #32724
There is a legacy command completion (not Roslyn old completion) in the Immediate window appearing after one types
>there. Code owners of the immediate window said they cannot remove the legacy completion for the case. They want Roslyn to disable/enable regular (both old Roslyn and new Editor) completions when calledEnableStatementCompletion.