Move usings on paste off the UI thread #61092
Conversation
| using var _ = executionContext.OperationContext.AddScope(allowCancellation: true, DialogText); | ||
| var cancellationToken = executionContext.OperationContext.UserCancellationToken; | ||
| var indicatorFactory = document.Project.Solution.Workspace.Services.GetRequiredService<IBackgroundWorkIndicatorFactory>(); | ||
| var backgroundWorkContext = indicatorFactory.Create( |
There was a problem hiding this comment.
why not put this in a using block?
see the code in GoToDefinitionCommandHandler for a suitable pattern on how to do the sync->async jump that does all this without the need for a TAsk.Run
|
nice! Does typing also cancel, or only escape? |
Typing and loss of focus also dismisses and stops the action. |
| { | ||
| _threadingContext = threadingContext; | ||
| _globalOptions = globalOptions; | ||
| _listener = listenerProvider.GetListener(FeatureAttribute.GoToDefinition); |
There was a problem hiding this comment.
not the right listener :) there should be a listener for AddImportsOnPaste (and, if not, you can add one).
There was a problem hiding this comment.
we shoudl also def have an integration test added to validate this works
There was a problem hiding this comment.
woops :) copy-paste failure
| } | ||
|
|
||
| private async Task ExecuteAsync(Document document, SnapshotSpan snapshotSpan, ITextView textView) | ||
| { |
There was a problem hiding this comment.
nit. consider adding: _threadingContext.ThrowIfNotOnUIThread();
There was a problem hiding this comment.
Interesting. Do we need this work to be on the UI thread to start?
There was a problem hiding this comment.
oh... will the indicator factory fail?
There was a problem hiding this comment.
It may be fine (I think I wrote things to be fine either way). However, this would be helpful for documenting how things bounce across threads. It's totally optional on your part of you want to do this here.
|
Logic looks good! Can we do an integration tests? It shoudln't be hard right? The benefit of hte async listener stuff is that it allowed you to do the paste, then say "i want to wait for all the async worked kicked of for feature 'X'" then validate the final state once that async work is done. I imagine we could easily validate this by like pasing |
Will update the existing test :) Wanted to check logic was good before mucking around too much with it |
| @@ -160,9 +161,14 @@ static void Main(string[] args) | |||
|
|
|||
| private async Task PasteAsync(string text, CancellationToken cancellationToken) | |||
| { | |||
| var provider = await TestServices.Shell.GetComponentModelServiceAsync<IAsynchronousOperationListenerProvider>(HangMitigatingCancellationToken); | |||
| var waiter = (IAsynchronousOperationWaiter)provider.GetListener(FeatureAttribute.AddImportsOnPaste); | |||
There was a problem hiding this comment.
note: i don't think we shoudl do this in paste. paste should just send the paste through. however, the test that wants to validate add-import-on-paste can itself then wait on this async work.
There was a problem hiding this comment.
All of the tests need to validate this. Even if no work is added the waiter should be there to validate that it's not being triggered right? Otherwise we could be incorrectly testing the negative. We could accidentally have it enabled and not wait on the work being done, so the test would still pass for broken code.
There was a problem hiding this comment.
All of the tests need to validate this. Even if no work is added the waiter should be there to validate that it's not being triggered right? Otherwise we could be incorrectly testing the negative. We could accidentally have it enabled and not wait on the work being done, so the test would still pass for broken code.
That's a compelling argument. in that case, i woudl say that we're not waiting on AddImportsOnPaste, we're effectivley waiting on any paste feature. so i would call this FeatureAttribute.Paste :)
(doesn't ahve to happen now).
ghost
added this to the 
…ures/required-members * upstream/main: (368 commits) Use new helpers Cleanup Update src/EditorFeatures/CSharpTest/StringCopyPaste/StringCopyPasteCommandHandlerTests.cs Restore Add docs Fix Update Tools|Options code ordering to match waht is in the UI Use correct directory for loghub logs (#61104) Move usings on paste off the UI thread (#61092) Create shared helper to handle finding extensions in analyzer references Code style to convert byte arrays to UTF8 strings (#60647) Fixup Enable semantic token LSP tests + re-enable quick info tests (#61098) Simplify initialization logic Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern ...
…o poison * upstream/features/required-members: (369 commits) Update tests after merge Use new helpers Cleanup Update src/EditorFeatures/CSharpTest/StringCopyPaste/StringCopyPasteCommandHandlerTests.cs Restore Add docs Fix Update Tools|Options code ordering to match waht is in the UI Use correct directory for loghub logs (dotnet#61104) Move usings on paste off the UI thread (dotnet#61092) Create shared helper to handle finding extensions in analyzer references Code style to convert byte arrays to UTF8 strings (dotnet#60647) Fixup Enable semantic token LSP tests + re-enable quick info tests (dotnet#61098) Simplify initialization logic Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern Simplify common CWT pattern ...
Fixes AB#1370350
Gif below had an artificial wait added to showcase the feature