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

Jump to bottom

Change Cancel() to async in InlineRenameSession. #74946

Closed
wants to merge 1 commit into from
Closed

Change Cancel() to async in InlineRenameSession. #74946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/EditorFeatures/Core.Wpf/InlineRename/UI/Adornment/RenameFlyoutViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using Microsoft.VisualStudio.Imaging.Interop;
using Microsoft.VisualStudio.PlatformUI.OleComponentSupport;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Threading;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename
{
Expand Down Expand Up @@ -223,7 +224,7 @@ public bool Submit()
public void Cancel()
{
SmartRenameViewModel?.Cancel();
Session.Cancel();
_ = Session.CancelAsync();
Copy link
Member

Choose a reason for hiding this comment

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

i'm not a fan of this being fire-forget here. i think we shoudl make the containing call async, and decide waht to do (on a case by case basis) for every caller. Some will be able to await, some can be explicit fire-and-forget, and some can block.

Note: Fire-and-forget here is not good for rename tracking as we have untracked async work being kicked off. anywhere we do this, we need IAsyncOpListener hooked up to keep track of things.

}

public void Dispose()
Expand Down
9 changes: 8 additions & 1 deletion src/EditorFeatures/Core.Wpf/InlineRename/UI/Dashboard/RenameDashboard.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Automation.Peers;
using System.Windows.Controls;
Expand Down Expand Up @@ -320,7 +321,13 @@ private void OnTextViewLostAggregateFocus(object sender, EventArgs e)

private void CloseButton_Click(object sender, RoutedEventArgs e)
{
_model.Session.Cancel();
_ = CancelAsync();
}

private async Task CancelAsync()
{
//.ConfigureAwait(true) because we need to set focus later
await _model.Session.CancelAsync().ConfigureAwait(true);
_textView.VisualElement.Focus();
Copy link
Member

Choose a reason for hiding this comment

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

why not set focus on the _textView prior to cancelling the session (which seems like it could take unbounded time).

Copy link
Member

Choose a reason for hiding this comment

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

stepping back (and maybe this pr will calrify it), it's unclear to me why cancelling (which just should cancel some CT tokens, and return the buffers to the initial state) would be async. IT seems like it should be fast and UI oriented. not sure why cancelling itself is async.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I've made a different change here: #74948

Cancellation here should be synchronous here.

}

Expand Down
2 changes: 1 addition & 1 deletion src/EditorFeatures/Core/IInlineRenameSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ internal interface IInlineRenameSession
/// <summary>
/// Cancels the rename session, and undoes any edits that had been performed by the session.
/// </summary>
void Cancel();
Task CancelAsync();

/// <summary>
/// Dismisses the rename session, completing the rename operation across all files.
Expand Down
3 changes: 2 additions & 1 deletion src/EditorFeatures/Core/InlineRename/AbstractInlineRenameUndoManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Operations;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename;

Expand Down Expand Up @@ -154,7 +155,7 @@ public void Undo(ITextBuffer _)
}
else
{
this.InlineRenameService.ActiveSession.Cancel();
this.InlineRenameService.ActiveSession.CancelAsync().ReportNonFatalErrorAsync();
}
}

Expand Down
13 changes: 11 additions & 2 deletions ...rFeatures/Core/InlineRename/CommandHandlers/AbstractRenameCommandHandler_EscapeHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading.Tasks;
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Text.Editor.Commanding.Commands;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename;

Expand All @@ -16,11 +18,18 @@ public bool ExecuteCommand(EscapeKeyCommandArgs args, CommandExecutionContext co
{
if (_renameService.ActiveSession != null)
{
_renameService.ActiveSession.Cancel();
SetFocusToTextView(args.TextView);
_ = CancelAsync(args).ReportNonFatalErrorAsync();
return true;
}

return false;
}

private async Task CancelAsync(EscapeKeyCommandArgs args)
{
RoslynDebug.AssertNotNull(_renameService.ActiveSession);
// ConfigureAwait(true) since we need to set focus back to UI later.
await _renameService.ActiveSession.CancelAsync().ConfigureAwait(true);
SetFocusToTextView(args.TextView);
}
}
2 changes: 1 addition & 1 deletion src/EditorFeatures/Core/InlineRename/InlineRenameSession.OpenTextBufferManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ private void OnTextViewClosed(object sender, EventArgs e)
var view = sender as ITextView;
view.Closed -= OnTextViewClosed;
_textViews.Remove(view);
_session.Cancel();
_ = _session.CancelAsync().ReportNonFatalErrorAsync();
}

internal void ConnectToView(ITextView textView)
Expand Down
14 changes: 3 additions & 11 deletions src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs args)
m["Kind"] = Enum.GetName(typeof(WorkspaceChangeKind), args.Kind);
}));

Cancel();
_ = CancelAsync().ReportNonFatalErrorAsync();
}
}
}
Expand Down Expand Up @@ -647,15 +647,7 @@ private void LogRenameSession(RenameLogMessage.UserActionOutcome outcome, bool p
}
}

public void Cancel()
{
_threadingContext.ThrowIfNotOnUIThread();

// This wait is safe. We are not passing the async callback to DismissUIAndRollbackEditsAndEndRenameSessionAsync.
// So everything in that method will happen synchronously.
DismissUIAndRollbackEditsAndEndRenameSessionAsync(
Copy link
Member Author

Choose a reason for hiding this comment

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

Key change in the PR.
I feel we don't need _threadingContext.ThrowIfNotOnUIThread(); because in DismissUIAndRollbackEditsAndEndRenameSessionAsync it immediately switches to main thread.

RenameLogMessage.UserActionOutcome.Canceled, previewChanges: false).Wait();
}
public Task CancelAsync() => DismissUIAndRollbackEditsAndEndRenameSessionAsync(RenameLogMessage.UserActionOutcome.Canceled, previewChanges: false);

private async Task DismissUIAndRollbackEditsAndEndRenameSessionAsync(
RenameLogMessage.UserActionOutcome outcome,
Expand Down Expand Up @@ -782,7 +774,7 @@ private async Task<bool> CommitWorkerAsync(bool previewChanges, bool canUseBackg
if (this.ReplacementText == string.Empty ||
this.ReplacementText == _initialRenameText)
{
Cancel();
await CancelAsync().ConfigureAwait(false);
return false;
}

Expand Down
20 changes: 10 additions & 10 deletions src/EditorFeatures/Test2/Rename/InlineRenameTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ End Class

<WpfTheory>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
Public Sub SimpleEditAndCancel(host As RenameTestHost)
Public Async Function SimpleEditAndCancel(host As RenameTestHost) As Task
Using workspace = CreateWorkspaceWithWaiter(
<Workspace>
<Project Language="C#" CommonReferences="true">
Expand All @@ -750,15 +750,15 @@ End Class

textBuffer.Insert(caretPosition, "Bar")

session.Cancel()
Await session.CancelAsync()

' Assert the file is what it started as
Assert.Equal(initialTextSnapshot.GetText(), textBuffer.CurrentSnapshot.GetText())

' Assert the file name didn't change
VerifyFileName(workspace, "Test1.cs")
End Using
End Sub
End Function

<WpfTheory, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/539513")>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
Expand Down Expand Up @@ -793,7 +793,7 @@ End Class

<WpfTheory>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
Public Sub ReadOnlyRegionsCreated(host As RenameTestHost)
Public Async Function ReadOnlyRegionsCreated(host As RenameTestHost) As Task
Using workspace = CreateWorkspaceWithWaiter(
<Workspace>
<Project Language="C#" CommonReferences="true">
Expand All @@ -820,17 +820,17 @@ End Class
Assert.True(buffer.IsReadOnly(0))
Assert.True(buffer.IsReadOnly(buffer.CurrentSnapshot.Length))

session.Cancel()
Await session.CancelAsync()

' Assert the file name didn't change
VerifyFileName(workspace, "Test1.cs")
End Using
End Sub
End Function

<WpfTheory>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
<WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/543018")>
Public Sub ReadOnlyRegionsCreatedWhichHandleBeginningOfFileEdgeCase(host As RenameTestHost)
Public Async Function ReadOnlyRegionsCreatedWhichHandleBeginningOfFileEdgeCase(host As RenameTestHost) As Task
Using workspace = CreateWorkspaceWithWaiter(
<Workspace>
<Project Language="C#" CommonReferences="true">
Expand All @@ -848,11 +848,11 @@ End Class
' Replacing our span should work
Assert.False(buffer.IsReadOnly(New Span(workspace.Documents.Single(Function(d) d.CursorPosition.HasValue).CursorPosition.Value, length:=1)))

session.Cancel()
Await session.CancelAsync()

VerifyFileName(workspace, "Test1.cs")
End Using
End Sub
End Function

<Theory>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
Expand Down Expand Up @@ -1027,7 +1027,7 @@ End Class
Await WaitForRename(workspace)
Await VerifyTagsAreCorrect(workspace)

session.Cancel()
Await session.CancelAsync()
Await VerifyNoRenameTrackingTags(renameTrackingTagger, workspace, document)

VerifyFileName(workspace, "Test1.cs")
Expand Down
16 changes: 8 additions & 8 deletions src/EditorFeatures/Test2/Rename/RenameCommandHandlerTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ End Class

<WpfTheory>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
Public Sub TypingSpaceDuringRename(host As RenameTestHost)
Public Async Function TypingSpaceDuringRename(host As RenameTestHost) As Task
Using workspace = CreateWorkspaceWithWaiter(
<Workspace>
<Project Language="C#" CommonReferences="true">
Expand All @@ -214,9 +214,9 @@ End Class
Sub() AssertEx.Fail("Space should not have been passed to the editor."),
Utilities.TestCommandExecutionContext.Create())

session.Cancel()
Await session.CancelAsync()
End Using
End Sub
End Function

<WpfTheory>
<CombinatorialData, Trait(Traits.Feature, Traits.Features.Rename)>
Expand Down Expand Up @@ -258,7 +258,7 @@ End Class

Assert.Equal(3, view.Caret.Position.BufferPosition.GetContainingLineNumber())

session.Cancel()
Await session.CancelAsync()
End Using
End Function

Expand Down Expand Up @@ -450,7 +450,7 @@ Goo f;
Utilities.TestCommandExecutionContext.Create())
Assert.Equal(Span.FromBounds(startPosition + 1, lineEnd), view.Selection.SelectedSpans.Single().Span)
#End Region
session.Cancel()
Await session.CancelAsync()
End Using
End Function

Expand Down Expand Up @@ -481,7 +481,7 @@ Goo f;

Await VerifyTagsAreCorrect(workspace)

session.Cancel()
Await session.CancelAsync()
End Using
End Function

Expand Down Expand Up @@ -568,7 +568,7 @@ Goo f;
Await VerifyTagsAreCorrect(workspace)
Assert.NotNull(workspace.GetService(Of IInlineRenameService).ActiveSession)

session.Cancel()
Await session.CancelAsync()
End Using
End Function

Expand Down Expand Up @@ -605,7 +605,7 @@ Goo f;
Await VerifyTagsAreCorrect(workspace)
Assert.NotNull(workspace.GetService(Of IInlineRenameService).ActiveSession)

session.Cancel()
Await session.CancelAsync()
End Using
End Function

Expand Down
8 changes: 4 additions & 4 deletions src/EditorFeatures/Test2/Rename/RenameTagProducerTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
Assert.True(Not sessionCommit Or Not sessionCancel)

If sessionCancel Then
session.Cancel()
Await session.CancelAsync()
VerifyBufferContentsInWorkspace(actualWorkspace, actualWorkspace)
ElseIf sessionCommit Then
Await session.CommitAsync(previewChanges:=False)
Expand Down Expand Up @@ -136,7 +136,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
Dim session = StartSession(workspace)

Await VerifyTaggedSpans(HighlightTags.RenameFieldBackgroundAndBorderTag.Instance, workspace, renameService)
session.Cancel()
Await session.CancelAsync()
End Using
End Function

Expand Down Expand Up @@ -1610,7 +1610,7 @@ static class E
Dim conflictTaggedSpans = Await GetTagsOfType(RenameConflictTag.Instance, workspace, renameService)
Dim conflictExpectedSpans = workspace.Documents.Single(Function(d) d.AnnotatedSpans.Count > 0).AnnotatedSpans("conflict").Select(Function(ts) ts.ToSpan())

session.Cancel()
Await session.CancelAsync()

AssertEx.Equal(validExpectedSpans, validTaggedSpans)
AssertEx.Equal(conflictExpectedSpans, conflictTaggedSpans)
Expand Down Expand Up @@ -1643,7 +1643,7 @@ static class E
Dim conflictTaggedSpans = Await GetTagsOfType(RenameConflictTag.Instance, workspace, renameService)
Dim conflictExpectedSpans = workspace.Documents.Single(Function(d) d.AnnotatedSpans.Count > 0).AnnotatedSpans("conflict").Select(Function(ts) ts.ToSpan())

session.Cancel()
Await session.CancelAsync()

AssertEx.Equal(validExpectedSpans, validTaggedSpans)
AssertEx.Equal(conflictExpectedSpans, conflictTaggedSpans)
Expand Down
2 changes: 1 addition & 1 deletion src/EditorFeatures/Test2/Rename/RenameViewModelTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ class D : B
Assert.True(QuickInfoSession.Dismissed)
End Using

sessionInfo.Session.Cancel()
Await sessionInfo.Session.CancelAsync()
End Using
End Function

Expand Down
6 changes: 3 additions & 3 deletions src/EditorFeatures/VisualBasicTest/LineCommit/CommitTestData.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,11 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.LineCommit
Private Class MockInlineRenameSession
Implements IInlineRenameSession

Public Sub Cancel() Implements IInlineRenameSession.Cancel
Public Function CommitAsync(previewChanges As Boolean, Optional editorOperationContext As IUIThreadOperationContext = Nothing) As Task Implements IInlineRenameSession.CommitAsync
Throw New NotImplementedException()
End Sub
End Function

Public Function CommitAsync(previewChanges As Boolean, Optional editorOperationContext As IUIThreadOperationContext = Nothing) As Task Implements IInlineRenameSession.CommitAsync
Public Function CancelAsync() As Task Implements IInlineRenameSession.CancelAsync
Throw New NotImplementedException()
End Function
End Class
Expand Down
Loading