Skip to content
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.

Sign up for GitHub

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

Move usings on paste off the UI thread #61092

Merged
merged 3 commits into from
May 3, 2022
+60 −19
Merged

Move usings on paste off the UI thread #61092

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bfb83c
Move usings on paste to the background and utilize IBackgroundWorkInd…
ryzngard May 2, 2022
d89d48a
Do correct async pattern
ryzngard May 2, 2022
932aab7
Fix integration tests
ryzngard May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/EditorFeatures/CSharp/AddImports/CSharpAddImportsPasteCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Utilities;

Expand All @@ -29,8 +30,8 @@ internal class CSharpAddImportsPasteCommandHandler : AbstractAddImportsPasteComm
{
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public CSharpAddImportsPasteCommandHandler(IThreadingContext threadingContext, IGlobalOptionService globalOptions)
: base(threadingContext, globalOptions)
public CSharpAddImportsPasteCommandHandler(IThreadingContext threadingContext, IGlobalOptionService globalOptions, IAsynchronousOperationListenerProvider listnerProvider)
: base(threadingContext, globalOptions, listnerProvider)
{
}

Expand Down
77 changes: 44 additions & 33 deletions src/EditorFeatures/Core/AddImports/AbstractAddImportsPasteCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.AddMissingImports;
using Microsoft.CodeAnalysis.CodeCleanup;
Expand All @@ -14,10 +15,13 @@
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Editor.Commanding.Commands;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.AddImport
{
Expand All @@ -35,11 +39,16 @@ internal abstract class AbstractAddImportsPasteCommandHandler : IChainedCommandH

private readonly IThreadingContext _threadingContext;
private readonly IGlobalOptionService _globalOptions;
private readonly IAsynchronousOperationListener _listener;

public AbstractAddImportsPasteCommandHandler(IThreadingContext threadingContext, IGlobalOptionService globalOptions)
public AbstractAddImportsPasteCommandHandler(
IThreadingContext threadingContext,
IGlobalOptionService globalOptions,
IAsynchronousOperationListenerProvider listenerProvider)
{
_threadingContext = threadingContext;
_globalOptions = globalOptions;
_listener = listenerProvider.GetListener(FeatureAttribute.GoToDefinition);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the right listener :) there should be a listener for AddImportsOnPaste (and, if not, you can add one).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shoudl also def have an integration test added to validate this works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woops :) copy-paste failure

}

public CommandState GetCommandState(PasteCommandArgs args, Func<CommandState> nextCommandHandler)
Expand Down Expand Up @@ -103,7 +112,6 @@ private void ExecuteCommandWorker(

// Applying the post-paste snapshot to the tracking span gives us the span of pasted text.
var snapshotSpan = trackingSpan.GetSpan(args.SubjectBuffer.CurrentSnapshot);
var textSpan = snapshotSpan.Span.ToTextSpan();

var sourceTextContainer = args.SubjectBuffer.AsTextContainer();
if (!Workspace.TryGetWorkspace(sourceTextContainer, out var workspace))
Expand All @@ -117,46 +125,49 @@ private void ExecuteCommandWorker(
return;
}

// We're showing our own UI, ensure the editor doesn't show anything itself.
executionContext.OperationContext.TakeOwnership();

var token = _listener.BeginAsyncOperation(nameof(ExecuteAsync));

ExecuteAsync(document, snapshotSpan, args.TextView)
.ReportNonFatalErrorAsync()
.CompletesAsyncOperation(token);
}

private async Task ExecuteAsync(Document document, SnapshotSpan snapshotSpan, ITextView textView)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. consider adding: _threadingContext.ThrowIfNotOnUIThread();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Do we need this work to be on the UI thread to start?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh... will the indicator factory fail?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var indicatorFactory = document.Project.Solution.Workspace.Services.GetRequiredService<IBackgroundWorkIndicatorFactory>();
var backgroundWorkContext = indicatorFactory.Create(
args.TextView,
using var backgroundWorkContext = indicatorFactory.Create(
textView,
snapshotSpan,
DialogText,
cancelOnEdit: true,
cancelOnFocusLost: true);

var cancellationToken = backgroundWorkContext.UserCancellationToken;

Task.Run(async () =>
// We're going to log the same thing on success or failure since this blocks the UI thread. This measurement is
// intended to tell us how long we're blocking the user from typing with this action.
using var blockLogger = Logger.LogBlock(FunctionId.CommandHandler_Paste_ImportsOnPaste, KeyValueLogMessage.Create(LogType.UserAction), cancellationToken);

var addMissingImportsService = document.GetRequiredLanguageService<IAddMissingImportsFeatureService>();

var cleanupOptions = await document.GetCodeCleanupOptionsAsync(_globalOptions, cancellationToken).ConfigureAwait(false);

var options = new AddMissingImportsOptions(
CleanupOptions: cleanupOptions,
HideAdvancedMembers: _globalOptions.GetOption(CompletionOptionsStorage.HideAdvancedMembers, document.Project.Language));

var textSpan = snapshotSpan.Span.ToTextSpan();
var updatedDocument = await addMissingImportsService.AddMissingImportsAsync(document, textSpan, options, cancellationToken).ConfigureAwait(false);

if (updatedDocument is null)
{
try
{
// We're going to log the same thing on success or failure since this blocks the UI thread. This measurement is
// intended to tell us how long we're blocking the user from typing with this action.
using var blockLogger = Logger.LogBlock(FunctionId.CommandHandler_Paste_ImportsOnPaste, KeyValueLogMessage.Create(LogType.UserAction), cancellationToken);

var addMissingImportsService = document.GetRequiredLanguageService<IAddMissingImportsFeatureService>();

var cleanupOptions = await document.GetCodeCleanupOptionsAsync(_globalOptions, cancellationToken).ConfigureAwait(false);

var options = new AddMissingImportsOptions(
CleanupOptions: cleanupOptions,
HideAdvancedMembers: _globalOptions.GetOption(CompletionOptionsStorage.HideAdvancedMembers, document.Project.Language));

var updatedDocument = await addMissingImportsService.AddMissingImportsAsync(document, textSpan, options, cancellationToken).ConfigureAwait(false);

if (updatedDocument is null)
{
return;
}

workspace.TryApplyChanges(updatedDocument.Project.Solution);
}
finally
{
backgroundWorkContext.Dispose();
}
}, cancellationToken);
return;
}

document.Project.Solution.Workspace.TryApplyChanges(updatedDocument.Project.Solution);
}
}
}
6 changes: 4 additions & 2 deletions src/EditorFeatures/VisualBasic/AddImports/VisualBasicAddImportsOnPasteCommandHandler.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Imports System.ComponentModel.Composition
Imports Microsoft.CodeAnalysis.AddImport
Imports Microsoft.CodeAnalysis.Host.Mef
Imports Microsoft.CodeAnalysis.Options
Imports Microsoft.CodeAnalysis.Shared.TestHooks
Imports Microsoft.VisualStudio.Commanding
Imports Microsoft.VisualStudio.Utilities

Expand All @@ -22,8 +23,9 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.AddImports
<ImportingConstructor>
<Obsolete(MefConstruction.ImportingConstructorMessage, True)>
Public Sub New(threadingContext As [Shared].Utilities.IThreadingContext,
globalOptions As IGlobalOptionService)
MyBase.New(threadingContext, globalOptions)
globalOptions As IGlobalOptionService,
listenerProvider As IAsynchronousOperationListenerProvider)
MyBase.New(threadingContext, globalOptions, listenerProvider)
End Sub

Public Overrides ReadOnly Property DisplayName As String = VBEditorResources.Add_Missing_Imports_on_Paste
Expand Down