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

Change commit to async when enter is pressed #75335

Merged
merged 28 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9f49243
Add options
Cosifne Oct 1, 2024
61ca735
Use the featureFlag in rename session
Cosifne Oct 1, 2024
2c8a742
Async commit in view model
Cosifne Oct 1, 2024
9aeb065
Add comment
Cosifne Oct 1, 2024
89cf25a
Use primary constructor
Cosifne Oct 1, 2024
0f0029e
Prevent change when rename is in-progress
Cosifne Oct 1, 2024
0478d09
Move return key to async
Cosifne Oct 1, 2024
76c722d
Reset one change
Cosifne Oct 2, 2024
bc379f8
guard more command handlers
Cosifne Oct 2, 2024
4c4ec72
Revert "Use primary constructor"
Cosifne Oct 2, 2024
b26ac48
Merge remote-tracking branch 'upstream/main' into dev/shech/MoveEnter…
Cosifne Oct 3, 2024
2fb97f4
Remove not used options
Cosifne Oct 3, 2024
b764740
Add listener provider
Cosifne Oct 3, 2024
c731d9c
Add async rename tests
Cosifne Oct 3, 2024
7b30ea8
More async token
Cosifne Oct 3, 2024
b12a1e3
Reset a not needed change
Cosifne Oct 3, 2024
67e1a6d
Move the token to caller
Cosifne Oct 3, 2024
362f502
Modify comment
Cosifne Oct 3, 2024
f37bc82
Delete the option entry in VS storage
Cosifne Oct 4, 2024
f21f843
Comment the storage
Cosifne Oct 4, 2024
c716abd
Modify the comment
Cosifne Oct 4, 2024
f0a2321
Remove one else clause and change comment
Cosifne Oct 15, 2024
05887bd
Aggregate exception and async token handling
Cosifne Oct 16, 2024
2eeead6
Clean up unused listners
Cosifne Oct 16, 2024
cb101d6
Add comments
Cosifne Oct 16, 2024
6a6894f
Clean usings
Cosifne Oct 16, 2024
7aa8db2
Reset to default value in integration test
Cosifne Oct 17, 2024
5a8ed41
Pass the missing context
Cosifne Oct 17, 2024
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 @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Telemetry;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using System.Threading.Tasks;
Cosifne marked this conversation as resolved.
Show resolved Hide resolved

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename
{
Expand Down Expand Up @@ -90,11 +91,12 @@ protected override void SetAdornmentFocusToPreviousElement(ITextView textView)
}
}

protected override void CommitAndSetFocus(InlineRenameSession activeSession, ITextView textView, IUIThreadOperationContext operationContext)
protected override async Task CommitAndSetFocusAsync(InlineRenameSession activeSession, ITextView textView, IUIThreadOperationContext operationContext)
{
try
{
base.CommitAndSetFocus(activeSession, textView, operationContext);
// ConfigureAwait(true) to show notification when error occurs.
await base.CommitAndSetFocusAsync(activeSession, textView, operationContext).ConfigureAwait(true);
}
catch (NotSupportedException ex)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@
using Microsoft.VisualStudio.Imaging.Interop;
using Microsoft.VisualStudio.PlatformUI.OleComponentSupport;
using Microsoft.VisualStudio.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename
{
internal class RenameFlyoutViewModel : INotifyPropertyChanged, IDisposable
{
private readonly bool _registerOleComponent;
private readonly IGlobalOptionService _globalOptionService;
private readonly IAsynchronousOperationListener _listener;
private OleComponent? _oleComponent;
private bool _disposedValue;
private bool _isReplacementTextValid = true;
Expand All @@ -56,6 +58,7 @@ public RenameFlyoutViewModel(
Session.CommitStateChange += CommitStateChange;
StartingSelection = selectionSpan;
InitialTrackingSpan = session.TriggerSpan.CreateTrackingSpan(SpanTrackingMode.EdgeInclusive);
_listener = listenerProvider.GetListener(FeatureAttribute.Rename);
var smartRenameSession = smartRenameSessionFactory?.Value.CreateSmartRenameSession(Session.TriggerSpan);
if (smartRenameSession is not null)
{
Expand Down Expand Up @@ -228,7 +231,9 @@ public bool Submit()
}

SmartRenameViewModel?.Commit(IdentifierText);
Session.Commit();

var token = _listener.BeginAsyncOperation(nameof(Submit));
_ = Session.CommitAsync(previewChanges: false).ReportNonFatalErrorAsync().CompletesAsyncOperation(token);
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

Expand Down
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 All @@ -15,6 +16,7 @@
using Microsoft.CodeAnalysis.Editor.Implementation.InlineRename.HighlightTags;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Notification;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Telemetry;
using Microsoft.VisualStudio.Text.Classification;
using Microsoft.VisualStudio.Text.Editor;
Expand All @@ -26,6 +28,7 @@ internal partial class RenameDashboard : InlineRenameAdornment
private readonly RenameDashboardViewModel _model;
private readonly IWpfTextView _textView;
private readonly IAdornmentLayer _findAdornmentLayer;
private readonly IAsynchronousOperationListener _listener;
private PresentationSource _presentationSource;
private DependencyObject _rootDependencyObject;
private IInputElement _rootInputElement;
Expand All @@ -47,14 +50,16 @@ internal partial class RenameDashboard : InlineRenameAdornment
public RenameDashboard(
RenameDashboardViewModel model,
IEditorFormatMapService editorFormatMapService,
IWpfTextView textView)
IWpfTextView textView,
IAsynchronousOperationListenerProvider listenerProvider)
{
_model = model;
InitializeComponent();

_tabNavigableChildren = [this.OverloadsCheckbox, this.CommentsCheckbox, this.StringsCheckbox, this.FileRenameCheckbox, this.PreviewChangesCheckbox, this.ApplyButton, this.CloseButton];

_textView = textView;
_listener = listenerProvider.GetListener(FeatureAttribute.Rename);
this.DataContext = model;

_textView.GotAggregateFocus += OnTextViewGotAggregateFocus;
Expand Down Expand Up @@ -243,7 +248,9 @@ protected override void OnAccessKey(AccessKeyEventArgs e)
}
else if (string.Equals(e.Key, RenameShortcutKey.Apply, StringComparison.OrdinalIgnoreCase))
{
this.Commit();
var token = _listener.BeginAsyncOperation($"{nameof(OnAccessKey)}.{nameof(RenameShortcutKey.Apply)}");
// CommitAsync catch & report all the exceptions.
_ = this.CommitAsync().CompletesAsyncOperation(token);
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -318,13 +325,21 @@ private void CloseButton_Click(object sender, RoutedEventArgs e)
}

private void Apply_Click(object sender, RoutedEventArgs e)
=> Commit();
{
var token = _listener.BeginAsyncOperation(nameof(Apply_Click));
// CommitAsync catch & report all the exceptions.
_ = CommitAsync().CompletesAsyncOperation(token);
}

private void Commit()
/// <remarks>
/// CommitAsync is non-throwing.
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
/// </remarks>
private async Task CommitAsync()
{
try
{
_model.Session.Commit();
//.ConfigureAwait(true) because UI thread is need to change focus
await _model.Session.CommitAsync(previewChanges: false).ConfigureAwait(true);
_textView.VisualElement.Focus();
}
catch (NotSupportedException ex)
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,8 @@ private void UpdateAdornments()
var newAdornment = new RenameDashboard(
(RenameDashboardViewModel)s_createdViewModels.GetValue(_renameService.ActiveSession, session => new RenameDashboardViewModel(session, _threadingContext, _textView)),
_editorFormatMapService,
_textView);
_textView,
_listenerProvider);

return newAdornment;
}
Expand Down
3 changes: 3 additions & 0 deletions src/EditorFeatures/Core/IInlineRenameSession.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,8 @@ internal interface IInlineRenameSession
/// <summary>
/// Dismisses the rename session, completing the rename operation across all files.
/// </summary>
/// <remarks>
/// It will only be async when InlineRenameUIOptionsStorage.CommitRenameAsynchronously is set to true.
/// </remarks>
Task CommitAsync(bool previewChanges, IUIThreadOperationContext editorOperationContext = null);
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ private void HandlePossibleTypingCommand<TArgs>(TArgs args, Action nextHandler,
return;
}

if (renameService.ActiveSession.IsCommitInProgress)
{
return;
}

var selectedSpans = args.TextView.Selection.GetSnapshotSpansOnBuffer(args.SubjectBuffer);

if (selectedSpans.Count > 1)
Expand Down Expand Up @@ -104,4 +109,7 @@ private void Commit(IUIThreadOperationContext operationContext)
RoslynDebug.AssertNotNull(renameService.ActiveSession);
renameService.ActiveSession.Commit(previewChanges: false, operationContext);
}

private bool IsRenameCommitInProgress()
=> renameService.ActiveSession?.IsCommitInProgress is true;
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ public CommandState GetCommandState(MoveSelectedLinesUpCommandArgs args)

public bool ExecuteCommand(MoveSelectedLinesUpCommandArgs args, CommandExecutionContext context)
{
if (IsRenameCommitInProgress())
return true;

CommitIfActive(args, context.OperationContext);
return false;
}
Expand All @@ -24,6 +27,9 @@ public CommandState GetCommandState(MoveSelectedLinesDownCommandArgs args)

public bool ExecuteCommand(MoveSelectedLinesDownCommandArgs args, CommandExecutionContext context)
{
if (IsRenameCommitInProgress())
return true;

CommitIfActive(args, context.OperationContext);
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ public CommandState GetCommandState(ReorderParametersCommandArgs args)

public bool ExecuteCommand(ReorderParametersCommandArgs args, CommandExecutionContext context)
{
if (IsRenameCommitInProgress())
return true;

CommitIfActive(args, context.OperationContext);
return false;
}
Expand All @@ -27,6 +30,9 @@ public CommandState GetCommandState(RemoveParametersCommandArgs args)

public bool ExecuteCommand(RemoveParametersCommandArgs args, CommandExecutionContext context)
{
if (IsRenameCommitInProgress())
return true;

CommitIfActive(args, context.OperationContext);
return false;
}
Expand All @@ -36,6 +42,9 @@ public CommandState GetCommandState(ExtractInterfaceCommandArgs args)

public bool ExecuteCommand(ExtractInterfaceCommandArgs args, CommandExecutionContext context)
{
if (IsRenameCommitInProgress())
return true;

CommitIfActive(args, context.OperationContext);
return false;
}
Expand All @@ -45,6 +54,9 @@ public CommandState GetCommandState(EncapsulateFieldCommandArgs args)

public bool ExecuteCommand(EncapsulateFieldCommandArgs args, CommandExecutionContext context)
{
if (IsRenameCommitInProgress())
return true;

CommitIfActive(args, context.OperationContext);
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,14 @@ private async Task ExecuteCommandAsync(RenameCommandArgs args, IUIThreadOperatio
// If there is already an active session, commit it first
if (renameService.ActiveSession != null)
{
// Is the caret within any of the rename fields in this buffer?
// If so, focus the dashboard
if (renameService.ActiveSession.TryGetContainingEditableSpan(caretPoint.Value, out _))
if (renameService.ActiveSession.IsCommitInProgress)
{
return;
}
else if (renameService.ActiveSession.TryGetContainingEditableSpan(caretPoint.Value, out _))
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
{
// Is the caret within any of the rename fields in this buffer?
// If so, focus the dashboard
SetFocusToAdornment(args.TextView);
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// 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.CodeAnalysis.Shared.TestHooks;
using Microsoft.VisualStudio.Commanding;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Editor.Commanding.Commands;
using Microsoft.VisualStudio.Utilities;
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.InlineRename;

Expand All @@ -18,16 +21,19 @@ public bool ExecuteCommand(ReturnKeyCommandArgs args, CommandExecutionContext co
{
if (renameService.ActiveSession != null)
{
CommitAndSetFocus(renameService.ActiveSession, args.TextView, context.OperationContext);
var token = listener.BeginAsyncOperation(nameof(CommitAndSetFocusAsync));
_ = CommitAndSetFocusAsync(renameService.ActiveSession, args.TextView, context.OperationContext)
.ReportNonFatalErrorAsync().CompletesAsyncOperation(token);
return true;
}

return false;
}

protected virtual void CommitAndSetFocus(InlineRenameSession activeSession, ITextView textView, IUIThreadOperationContext operationContext)
protected virtual async Task CommitAndSetFocusAsync(InlineRenameSession activeSession, ITextView textView, IUIThreadOperationContext operationContext)
{
Commit(operationContext);
// ConfigureAwait(true) because UI thread is needed to change the focus of text view.
await activeSession.CommitAsync(previewChanges: false, operationContext).ConfigureAwait(true);
SetFocusToTextView(textView);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,43 @@ public CommandState GetCommandState(RedoCommandArgs args)

public bool ExecuteCommand(UndoCommandArgs args, CommandExecutionContext context)
{
if (renameService.ActiveSession != null)
if (renameService.ActiveSession == null)
{
for (var i = 0; i < args.Count && renameService.ActiveSession != null; i++)
{
renameService.ActiveSession.UndoManager.Undo(args.SubjectBuffer);
}
return false;
}

if (renameService.ActiveSession.IsCommitInProgress)
{
// When rename commit is in progress, handle the command so it won't change the workspace
return true;
}

return false;
for (var i = 0; i < args.Count && renameService.ActiveSession != null; i++)
{
renameService.ActiveSession.UndoManager.Undo(args.SubjectBuffer);
}

return true;
}

public bool ExecuteCommand(RedoCommandArgs args, CommandExecutionContext context)
{
if (renameService.ActiveSession != null)
if (renameService.ActiveSession == null)
{
for (var i = 0; i < args.Count && renameService.ActiveSession != null; i++)
{
renameService.ActiveSession.UndoManager.Redo(args.SubjectBuffer);
}
return false;
}

if (renameService.ActiveSession.IsCommitInProgress)
{
// When rename commit is in progress, handle the command so it won't change the workspace
return true;
}

return false;
for (var i = 0; i < args.Count && renameService.ActiveSession != null; i++)
{
renameService.ActiveSession.UndoManager.Redo(args.SubjectBuffer);
}

return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ private bool HandleWordDeleteCommand(ITextBuffer subjectBuffer, ITextView view,
return false;
}

if (renameService.ActiveSession.IsCommitInProgress)
{
// When rename commit is in progress, handle the command so it won't change the workspace
Cosifne marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

var caretPoint = view.GetCaretPoint(subjectBuffer);
if (caretPoint.HasValue)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ private bool CommitSynchronously(bool previewChanges, IUIThreadOperationContext
/// </remarks>
public async Task CommitAsync(bool previewChanges, IUIThreadOperationContext editorOperationContext = null)
{
if (this.RenameService.GlobalOptions.GetOption(InlineRenameSessionOptionsStorage.RenameAsynchronously))
if (this.RenameService.GlobalOptions.ShouldCommitAsynchronously())
{
await CommitWorkerAsync(previewChanges, canUseBackgroundWorkIndicator: true, editorOperationContext).ConfigureAwait(false);
}
Expand Down Expand Up @@ -782,6 +782,12 @@ private async Task<bool> CommitWorkerAsync(bool previewChanges, bool canUseBackg
return false;
}

// Don't dup commit.
if (this.IsCommitInProgress)
{
return false;
}

previewChanges = previewChanges || PreviewChanges;

if (editorUIOperationContext is not null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,10 @@ internal static class InlineRenameSessionOptionsStorage
public static readonly Option2<bool> RenameInComments = new("dotnet_rename_in_comments", defaultValue: false);
public static readonly Option2<bool> RenameFile = new("dotnet_rename_file", defaultValue: true);
public static readonly Option2<bool> PreviewChanges = new("dotnet_preview_inline_rename_changes", defaultValue: false);
public static readonly Option2<bool> RenameAsynchronously = new("dotnet_rename_asynchronously", defaultValue: true);

public static readonly Option2<bool?> CommitRenameAsynchronously = new("dotnet_commit_rename_asynchronously", defaultValue: null);
public static readonly Option2<bool> CommitRenameAsynchronouslyFeatureFlag = new("dotnet_commit_rename_asynchronously_feature_flag", defaultValue: false);

public static bool ShouldCommitAsynchronously(this IGlobalOptionService globalOptionService)
=> globalOptionService.GetOption(CommitRenameAsynchronously) ?? globalOptionService.GetOption(CommitRenameAsynchronouslyFeatureFlag);
}
Loading
Loading