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.

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

Always take ownership from UIThreadContext when rename commit #74889

Merged
merged 10 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,11 @@ protected override void SetAdornmentFocusToPreviousElement(ITextView textView)
}
}

protected override void Commit(InlineRenameSession activeSession, ITextView textView)
protected override void CommitAndSetFocus(InlineRenameSession activeSession, ITextView textView, IUIThreadOperationContext operationContext)
{
try
{
base.Commit(activeSession, textView);
base.CommitAndSetFocus(activeSession, textView, operationContext);
}
catch (NotSupportedException ex)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public bool ExecuteCommand(ExtractMethodCommandArgs args, CommandExecutionContex
// wait indicator for Extract Method
if (_renameService.ActiveSession != null)
{
_threadingContext.JoinableTaskFactory.Run(() => _renameService.ActiveSession.CommitAsync(previewChanges: false));
_threadingContext.JoinableTaskFactory.Run(() => _renameService.ActiveSession.CommitAsync(previewChanges: false, context.OperationContext));
}

if (!args.SubjectBuffer.SupportsRefactorings())
Expand Down
3 changes: 2 additions & 1 deletion src/EditorFeatures/Core/IInlineRenameSession.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.Utilities;

namespace Microsoft.CodeAnalysis.Editor;

Expand Down Expand Up @@ -50,5 +51,5 @@ internal interface IInlineRenameSession
/// <summary>
/// Dismisses the rename session, completing the rename operation across all files.
/// </summary>
Task CommitAsync(bool previewChanges);
Task CommitAsync(bool previewChanges, IUIThreadOperationContext editorOperationContext = null);
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Editor.Commanding;
using Microsoft.VisualStudio.Utilities;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename;

Expand Down Expand Up @@ -55,7 +57,7 @@ private CommandState GetCommandState(Func<CommandState> nextHandler)
private CommandState GetCommandState()
=> _renameService.ActiveSession != null ? CommandState.Available : CommandState.Unspecified;

private void HandlePossibleTypingCommand<TArgs>(TArgs args, Action nextHandler, Action<InlineRenameSession, SnapshotSpan> actionIfInsideActiveSpan)
private void HandlePossibleTypingCommand<TArgs>(TArgs args, Action nextHandler, IUIThreadOperationContext operationContext, Action<InlineRenameSession, IUIThreadOperationContext, SnapshotSpan> actionIfInsideActiveSpan)
where TArgs : EditorCommandArgs
{
if (_renameService.ActiveSession == null)
Expand All @@ -78,14 +80,14 @@ private void HandlePossibleTypingCommand<TArgs>(TArgs args, Action nextHandler,
if (_renameService.ActiveSession.TryGetContainingEditableSpan(singleSpan.Start, out var containingSpan) &&
containingSpan.Contains(singleSpan))
{
actionIfInsideActiveSpan(_renameService.ActiveSession, containingSpan);
actionIfInsideActiveSpan(_renameService.ActiveSession, operationContext, containingSpan);
}
else if (_renameService.ActiveSession.IsInOpenTextBuffer(singleSpan.Start))
{
// It's in a read-only area that is open, so let's commit the rename
// and then let the character go through

CommitIfActiveAndCallNextHandler(args, nextHandler);
CommitIfActiveAndCallNextHandler(args, nextHandler, operationContext);
}
else
{
Expand All @@ -94,23 +96,29 @@ private void HandlePossibleTypingCommand<TArgs>(TArgs args, Action nextHandler,
}
}

private void CommitIfActive(EditorCommandArgs args)
private void CommitIfActive(EditorCommandArgs args, IUIThreadOperationContext operationContext)
{
if (_renameService.ActiveSession != null)
{
var selection = args.TextView.Selection.VirtualSelectedSpans.First();

_renameService.ActiveSession.Commit();
Commit(operationContext);

var translatedSelection = selection.TranslateTo(args.TextView.TextBuffer.CurrentSnapshot);
args.TextView.Selection.Select(translatedSelection.Start, translatedSelection.End);
args.TextView.Caret.MoveTo(translatedSelection.End);
}
}

private void CommitIfActiveAndCallNextHandler(EditorCommandArgs args, Action nextHandler)
private void CommitIfActiveAndCallNextHandler(EditorCommandArgs args, Action nextHandler, IUIThreadOperationContext operationContext)
{
CommitIfActive(args);
CommitIfActive(args, operationContext);
nextHandler();
}

private void Commit(IUIThreadOperationContext operationContext)
{
RoslynDebug.AssertNotNull(_renameService.ActiveSession);
_renameService.ActiveSession.Commit(previewChanges: false, operationContext);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public CommandState GetCommandState(DeleteKeyCommandArgs args, Func<CommandState

public void ExecuteCommand(BackspaceKeyCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, _, span) =>
{
var caretPoint = args.TextView.GetCaretPoint(args.SubjectBuffer);
if (!args.TextView.Selection.IsEmpty || caretPoint.Value != span.Start)
Expand All @@ -33,7 +33,7 @@ public void ExecuteCommand(BackspaceKeyCommandArgs args, Action nextHandler, Com

public void ExecuteCommand(DeleteKeyCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, _, span) =>
{
var caretPoint = args.TextView.GetCaretPoint(args.SubjectBuffer);
if (!args.TextView.Selection.IsEmpty || caretPoint.Value != span.End)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public CommandState GetCommandState(CutCommandArgs args, Func<CommandState> next

public void ExecuteCommand(CutCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, _, span) =>
{
nextHandler();
});
Expand All @@ -27,7 +27,7 @@ public CommandState GetCommandState(PasteCommandArgs args, Func<CommandState> ne

public void ExecuteCommand(PasteCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, _, span) =>
{
nextHandler();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public CommandState GetCommandState(MoveSelectedLinesUpCommandArgs args)

public bool ExecuteCommand(MoveSelectedLinesUpCommandArgs args, CommandExecutionContext context)
{
CommitIfActive(args);
CommitIfActive(args, context.OperationContext);
return false;
}

Expand All @@ -24,7 +24,7 @@ public CommandState GetCommandState(MoveSelectedLinesDownCommandArgs args)

public bool ExecuteCommand(MoveSelectedLinesDownCommandArgs args, CommandExecutionContext context)
{
CommitIfActive(args);
CommitIfActive(args, context.OperationContext);
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ public CommandState GetCommandState(OpenLineAboveCommandArgs args, Func<CommandS

public void ExecuteCommand(OpenLineAboveCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, operationContext, span) =>
{
activeSession.Commit();
Commit(operationContext);
nextHandler();
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ public CommandState GetCommandState(OpenLineBelowCommandArgs args, Func<CommandS

public void ExecuteCommand(OpenLineBelowCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, operationContext, span) =>
{
activeSession.Commit();
Commit(operationContext);
nextHandler();
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public CommandState GetCommandState(ReorderParametersCommandArgs args)

public bool ExecuteCommand(ReorderParametersCommandArgs args, CommandExecutionContext context)
{
CommitIfActive(args);
CommitIfActive(args, context.OperationContext);
return false;
}

Expand All @@ -27,7 +27,7 @@ public CommandState GetCommandState(RemoveParametersCommandArgs args)

public bool ExecuteCommand(RemoveParametersCommandArgs args, CommandExecutionContext context)
{
CommitIfActive(args);
CommitIfActive(args, context.OperationContext);
return false;
}

Expand All @@ -36,7 +36,7 @@ public CommandState GetCommandState(ExtractInterfaceCommandArgs args)

public bool ExecuteCommand(ExtractInterfaceCommandArgs args, CommandExecutionContext context)
{
CommitIfActive(args);
CommitIfActive(args, context.OperationContext);
return false;
}

Expand All @@ -45,7 +45,7 @@ public CommandState GetCommandState(EncapsulateFieldCommandArgs args)

public bool ExecuteCommand(EncapsulateFieldCommandArgs args, CommandExecutionContext context)
{
CommitIfActive(args);
CommitIfActive(args, context.OperationContext);
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Text.Editor.Commanding.Commands;
using Microsoft.VisualStudio.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename;

Expand Down Expand Up @@ -43,11 +44,11 @@ public bool ExecuteCommand(RenameCommandArgs args, CommandExecutionContext conte
}

var token = _listener.BeginAsyncOperation(nameof(ExecuteCommand));
_ = ExecuteCommandAsync(args).CompletesAsyncOperation(token);
_ = ExecuteCommandAsync(args, context.OperationContext).CompletesAsyncOperation(token);
return true;
}

private async Task ExecuteCommandAsync(RenameCommandArgs args)
private async Task ExecuteCommandAsync(RenameCommandArgs args, IUIThreadOperationContext editorOperationContext)
{
_threadingContext.ThrowIfNotOnUIThread();

Expand All @@ -63,12 +64,6 @@ private async Task ExecuteCommandAsync(RenameCommandArgs args)
return;
}

var backgroundWorkIndicatorFactory = workspace.Services.GetRequiredService<IBackgroundWorkIndicatorFactory>();
using var context = backgroundWorkIndicatorFactory.Create(
args.TextView,
args.TextView.GetTextElementSpan(caretPoint.Value),
EditorFeaturesResources.Finding_token_to_rename);

// If there is already an active session, commit it first
if (_renameService.ActiveSession != null)
{
Expand All @@ -82,10 +77,16 @@ private async Task ExecuteCommandAsync(RenameCommandArgs args)
else
{
// Otherwise, commit the existing session and start a new one.
_renameService.ActiveSession.Commit();
Commit(editorOperationContext);
}
}

var backgroundWorkIndicatorFactory = workspace.Services.GetRequiredService<IBackgroundWorkIndicatorFactory>();
using var context = backgroundWorkIndicatorFactory.Create(
args.TextView,
args.TextView.GetTextElementSpan(caretPoint.Value),
EditorFeaturesResources.Finding_token_to_rename);

var cancellationToken = context.UserCancellationToken;

var document = await args
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Editor.Commanding.Commands;
using Microsoft.VisualStudio.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename;

Expand All @@ -17,20 +18,16 @@ public bool ExecuteCommand(ReturnKeyCommandArgs args, CommandExecutionContext co
{
if (_renameService.ActiveSession != null)
{
// Prevent Editor's typing responsiveness auto canceling the rename operation.
// InlineRenameSession will call IUIThreadOperationExecutor to sets up our own IUIThreadOperationContext
context.OperationContext.TakeOwnership();

Commit(_renameService.ActiveSession, args.TextView);
CommitAndSetFocus(_renameService.ActiveSession, args.TextView, context.OperationContext);
return true;
}

return false;
}

protected virtual void Commit(InlineRenameSession activeSession, ITextView textView)
protected virtual void CommitAndSetFocus(InlineRenameSession activeSession, ITextView textView, IUIThreadOperationContext operationContext)
{
activeSession.Commit();
Commit(operationContext);
SetFocusToTextView(textView);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public bool ExecuteCommand(SaveCommandArgs args, CommandExecutionContext context
{
if (_renameService.ActiveSession != null)
{
_renameService.ActiveSession.Commit();
Commit(context.OperationContext);
SetFocusToTextView(args.TextView);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void ExecuteCommand(TabKeyCommandArgs args, Action nextHandler, CommandEx
return;
}

HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, _, span) =>
{
var spans = new NormalizedSnapshotSpanCollection(
activeSession.GetBufferManager(args.SubjectBuffer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public CommandState GetCommandState(TypeCharCommandArgs args, Func<CommandState>

public void ExecuteCommand(TypeCharCommandArgs args, Action nextHandler, CommandExecutionContext context)
{
HandlePossibleTypingCommand(args, nextHandler, (activeSession, span) =>
HandlePossibleTypingCommand(args, nextHandler, context.OperationContext, (activeSession, _, span) =>
{
var document = args.SubjectBuffer.CurrentSnapshot.GetOpenDocumentInCurrentContextWithChanges();
if (document == null)
Expand Down
29 changes: 21 additions & 8 deletions src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs
Original file line number Diff line number Diff line change
Expand Up @@ -723,36 +723,42 @@ void DismissUIAndRollbackEdits()
}
}

public void Commit(bool previewChanges = false)
=> CommitSynchronously(previewChanges);
/// <remarks>
/// Caller should pass in the IUIThreadOperationContext if it is called from editor so rename commit operation could set up the its own context correctly.
/// </remarks>
public void Commit(bool previewChanges = false, IUIThreadOperationContext editorOperationContext = null)
=> CommitSynchronously(previewChanges, editorOperationContext);

/// <returns><see langword="true"/> if the rename operation was committed, <see
/// langword="false"/> otherwise</returns>
private bool CommitSynchronously(bool previewChanges)
private bool CommitSynchronously(bool previewChanges, IUIThreadOperationContext operationContext = null)
{
// We're going to synchronously block the UI thread here. So we can't use the background work indicator (as
// it needs the UI thread to update itself. This will force us to go through the Threaded-Wait-Dialog path
// which at least will allow the user to cancel the rename if they want.
//
// In the future we should remove this entrypoint and have all callers use CommitAsync instead.
return _threadingContext.JoinableTaskFactory.Run(() => CommitWorkerAsync(previewChanges, canUseBackgroundWorkIndicator: false));
return _threadingContext.JoinableTaskFactory.Run(() => CommitWorkerAsync(previewChanges, canUseBackgroundWorkIndicator: false, operationContext));
}

public async Task CommitAsync(bool previewChanges)
/// <remarks>
/// Caller should pass in the IUIThreadOperationContext if it is called from editor so rename commit operation could set up the its own context correctly.
/// </remarks>
public async Task CommitAsync(bool previewChanges, IUIThreadOperationContext editorOperationContext = null)
{
if (this.RenameService.GlobalOptions.GetOption(InlineRenameSessionOptionsStorage.RenameAsynchronously))
{
await CommitWorkerAsync(previewChanges, canUseBackgroundWorkIndicator: true).ConfigureAwait(false);
await CommitWorkerAsync(previewChanges, canUseBackgroundWorkIndicator: true, editorOperationContext).ConfigureAwait(false);
}
else
{
CommitSynchronously(previewChanges);
CommitSynchronously(previewChanges, editorOperationContext);
}
}

/// <returns><see langword="true"/> if the rename operation was committed, <see
/// langword="false"/> otherwise</returns>
private async Task<bool> CommitWorkerAsync(bool previewChanges, bool canUseBackgroundWorkIndicator)
private async Task<bool> CommitWorkerAsync(bool previewChanges, bool canUseBackgroundWorkIndicator, IUIThreadOperationContext editorUIOperationContext)
{
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();
VerifyNotDismissed();
Expand All @@ -775,6 +781,13 @@ private async Task<bool> CommitWorkerAsync(bool previewChanges, bool canUseBackg

previewChanges = previewChanges || PreviewChanges;

if (editorUIOperationContext is not null)
{
// Prevent Editor's typing responsiveness auto canceling the rename operation.
// InlineRenameSession will call IUIThreadOperationExecutor to sets up our own IUIThreadOperationContext
editorUIOperationContext.TakeOwnership();
}

try
{
if (canUseBackgroundWorkIndicator)
Expand Down
Loading
Loading