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

Unify the final state management processing in inline-rename. #61512

Merged
merged 5 commits into from
May 26, 2022
Merged

Unify the final state management processing in inline-rename. #61512

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c7386f
Simplify rename implementation
CyrusNajmabadi May 25, 2022
55d3c68
Rename
CyrusNajmabadi May 25, 2022
9696920
Unify all end operations that rename performs
CyrusNajmabadi May 25, 2022
2173ca0
restore code
CyrusNajmabadi May 25, 2022
b1914a6
Remove unnecessary code'
CyrusNajmabadi May 25, 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
4 changes: 2 additions & 2 deletions src/EditorFeatures/Core/InlineRename/InlineRenameSession.OpenTextBufferManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ internal void ApplyReplacementText(bool updateSelection = true)
}
}

internal void Disconnect(bool documentIsClosed, bool rollbackTemporaryEdits)
internal void DisconnectAndRollbackEdits(bool documentIsClosed)
{
_session._threadingContext.ThrowIfNotOnUIThread();

Expand All @@ -307,7 +307,7 @@ internal void Disconnect(bool documentIsClosed, bool rollbackTemporaryEdits)
// Remove any old read only regions we had
UpdateReadOnlyRegions(removeOnly: true);

if (rollbackTemporaryEdits && !documentIsClosed)
if (!documentIsClosed)
{
_session.UndoManager.UndoTemporaryEdits(_subjectBuffer, disconnect: true);
}
Expand Down
145 changes: 65 additions & 80 deletions src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Debugging;
using Microsoft.CodeAnalysis.Editor.Host;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
Expand All @@ -21,7 +20,6 @@
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.Notification;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Rename;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.TestHooks;
Expand Down Expand Up @@ -367,34 +365,6 @@ public void SetPreviewChanges(bool value)
_previewChanges = value;
}

private void Dismiss(bool rollbackTemporaryEdits)
Copy link
Member Author

Choose a reason for hiding this comment

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

Dismiss was called from one location only. So it was inlined as a local function in that one place to make sure nothing else calls it directly. It was entirely unchanged.

{
_dismissed = true;
_workspace.WorkspaceChanged -= OnWorkspaceChanged;
_textBufferAssociatedViewService.SubjectBuffersConnected -= OnSubjectBuffersConnected;

// Reenable completion now that the inline rename session is done
_completionDisabledToken.Dispose();

foreach (var textBuffer in _openTextBuffers.Keys)
{
var document = textBuffer.CurrentSnapshot.GetOpenDocumentInCurrentContextWithChanges();
var isClosed = document == null;

var openBuffer = _openTextBuffers[textBuffer];
openBuffer.Disconnect(isClosed, rollbackTemporaryEdits);
}

this.UndoManager.Disconnect();

if (_triggerView != null && !_triggerView.IsClosed)
{
_triggerView.Selection.Clear();
}

RenameService.ActiveSession = null;
}

private void VerifyNotDismissed()
{
if (_dismissed)
Expand Down Expand Up @@ -660,16 +630,69 @@ private void LogRenameSession(RenameLogMessage.UserActionOutcome outcome, bool p
}

public void Cancel()
=> Cancel(rollbackTemporaryEdits: true);

private void Cancel(bool rollbackTemporaryEdits)
{
_threadingContext.ThrowIfNotOnUIThread();
VerifyNotDismissed();

LogRenameSession(RenameLogMessage.UserActionOutcome.Canceled, previewChanges: false);
Dismiss(rollbackTemporaryEdits);
EndRenameSession();
DismissUIAndRollbackEditsAndEndRenameSession(RenameLogMessage.UserActionOutcome.Canceled, previewChanges: false);
}

private void DismissUIAndRollbackEditsAndEndRenameSession(
RenameLogMessage.UserActionOutcome outcome,
bool previewChanges,
Action finalCommitAction = null)
Copy link
Member Author

Choose a reason for hiding this comment

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

two codepaths are just ones that dismiss/cleanup without committing. And one codepath actually has some commit logic it performs before final cleanup. So i represented this as this single method with the optional commit action to perform.

{
// Note: this entire sequence of steps is not cancellable. We must perform it all to get back to a correct
// state for all the editors the user is interacting with.

// Remove all our adornments and restore all buffer texts to their initial state.
DismissUIAndRollbackEdits();

// We're about to perform the final commit action. No need to do any of our BG work to find-refs or compute conflicts.
_cancellationTokenSource.Cancel();
_conflictResolutionTaskCancellationSource.Cancel();
Copy link
Member Author

Choose a reason for hiding this comment

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

these two cancels were in CancelAllOpenDocumentTrackingTasks. This was the only place that called that, so this was inlined here.


// Perform the actual commit step if we've been asked to.
finalCommitAction?.Invoke();

// Log the result so we know how well rename is going in practice.
LogRenameSession(outcome, previewChanges);

// Remove all our rename trackers from the text buffer properties.
RenameTrackingDismisser.DismissRenameTracking(_workspace, _workspace.GetOpenDocumentIds());

// Log how long the full rename took.
_inlineRenameSessionDurationLogBlock.Dispose();
Copy link
Member Author

Choose a reason for hiding this comment

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

The above code is waht was in EndRenameSession. It was only called here, so i inlined as well.


return;

void DismissUIAndRollbackEdits()
{
_dismissed = true;
_workspace.WorkspaceChanged -= OnWorkspaceChanged;
_textBufferAssociatedViewService.SubjectBuffersConnected -= OnSubjectBuffersConnected;

// Reenable completion now that the inline rename session is done
_completionDisabledToken.Dispose();

foreach (var textBuffer in _openTextBuffers.Keys)
{
var document = textBuffer.CurrentSnapshot.GetOpenDocumentInCurrentContextWithChanges();
var isClosed = document == null;

var openBuffer = _openTextBuffers[textBuffer];
openBuffer.DisconnectAndRollbackEdits(isClosed);
}

this.UndoManager.Disconnect();

if (_triggerView != null && !_triggerView.IsClosed)
{
_triggerView.Selection.Clear();
}

RenameService.ActiveSession = null;
}
}

public void Commit(bool previewChanges = false)
Expand Down Expand Up @@ -709,29 +732,14 @@ private bool CommitWorker(bool previewChanges)

if (result == UIThreadOperationStatus.Canceled)
{
LogRenameSession(RenameLogMessage.UserActionOutcome.Canceled | RenameLogMessage.UserActionOutcome.Committed, previewChanges);
Dismiss(rollbackTemporaryEdits: true);
EndRenameSession();

DismissUIAndRollbackEditsAndEndRenameSession(
RenameLogMessage.UserActionOutcome.Canceled | RenameLogMessage.UserActionOutcome.Committed, previewChanges);
return false;
}

return true;
}

private void EndRenameSession()
{
CancelAllOpenDocumentTrackingTasks();
RenameTrackingDismisser.DismissRenameTracking(_workspace, _workspace.GetOpenDocumentIds());
_inlineRenameSessionDurationLogBlock.Dispose();
}

private void CancelAllOpenDocumentTrackingTasks()
{
_cancellationTokenSource.Cancel();
_conflictResolutionTaskCancellationSource.Cancel();
}

private void CommitCore(IUIThreadOperationContext operationContext, bool previewChanges)
{
var eventName = previewChanges ? FunctionId.Rename_CommitCoreWithPreview : FunctionId.Rename_CommitCore;
Expand Down Expand Up @@ -760,35 +768,12 @@ private void CommitCore(IUIThreadOperationContext operationContext, bool preview
}
}

// The user hasn't cancelled by now, so we're done waiting for them. Off to
// rename!
// The user hasn't canceled by now, so we're done waiting for them. Off to rename!
using var _ = operationContext.AddScope(allowCancellation: false, EditorFeaturesResources.Updating_files);

Dismiss(rollbackTemporaryEdits: true);
CancelAllOpenDocumentTrackingTasks();

_triggerView.Caret.PositionChanged += LogPositionChanged;

ApplyRename(newSolution, operationContext);

LogRenameSession(RenameLogMessage.UserActionOutcome.Committed, previewChanges);

EndRenameSession();

_triggerView.Caret.PositionChanged -= LogPositionChanged;

void LogPositionChanged(object sender, CaretPositionChangedEventArgs e)
{
try
{
throw new InvalidOperationException("Caret position changed during application of rename");
}
catch (InvalidOperationException ex) when (FatalError.ReportAndCatch(ex))
{
// Unreachable code due to ReportAndCatch
Contract.ThrowIfTrue(true);
}
}
DismissUIAndRollbackEditsAndEndRenameSession(
RenameLogMessage.UserActionOutcome.Committed, previewChanges,
() => ApplyRename(newSolution, operationContext));
}
}

Expand Down