Make event-hookup-completion async, and hook up to the background work indicator #72584

CyrusNajmabadi · 2024-03-18T18:42:53Z

Fixes AB#1991866

CyrusNajmabadi · 2024-03-18T18:44:13Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

 using Roslyn.Utilities;

 namespace Microsoft.CodeAnalysis.Editor.CSharp.EventHookup;

 internal partial class EventHookupCommandHandler : IChainedCommandHandler<TabKeyCommandArgs>
 {
-    public void ExecuteCommand(TabKeyCommandArgs args, Action nextHandler, CommandExecutionContext context)
+    public CommandState GetCommandState(TabKeyCommandArgs args, Func<CommandState> nextHandler)


moved the simple GetCommandState helper to the top.

CyrusNajmabadi · 2024-03-18T18:44:43Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

    {
        _threadingContext.ThrowIfNotOnUIThread();
-        if (EventHookupSessionManager.CurrentSession != null)
+        if (!TryExecuteCommand(args, nextHandler))


Execute first calls into the up front synchronous portion of TryExecute. if that part fails, we ensure we're reset back to normal and pass the tab into the editor.

CyrusNajmabadi · 2024-03-18T18:45:07Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

+            return false;
+
+        // Now emit the event asynchronously.
+        var token = _asyncListener.BeginAsyncOperation(nameof(ExecuteCommand));


from this point on, we're doing all our work asynchronously, with a BWI in effect.

CyrusNajmabadi · 2024-03-18T18:46:16Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

+            var eventHandlerMethodName = await getEventNameTask.WithCancellation(cancellationToken).ConfigureAwait(false);
+
+            var solutionAndRenameSpan = await TryGetNewSolutionWithAddedMethodAsync(
+                document, eventHandlerMethodName, initialCaretPoint.Position, cancellationToken).ConfigureAwait(false);


doing everything expensive asynchronously.

CyrusNajmabadi · 2024-03-18T18:46:34Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

            {
                nextHandler();
-                EventHookupSessionManager.CancelAndDismissExistingSessions();
                return;
            }


here we check that nothing happened on the ui thread while we were doing the expensive work.

CyrusNajmabadi · 2024-03-18T18:49:43Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

-
-            // The new solution is created, so start user observable changes
+            var (solutionWithEventHandler, renameSpan) = solutionAndRenameSpan.Value;
+            var workspace = document.Project.Solution.Workspace;


application to the workspace and start of rename session both happen on the UI thread as required.

CyrusNajmabadi · 2024-03-18T18:54:00Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

+
+        // Ensure no matter what that once tab is hit that we're back to the initial no-session state. We do not
+        // want to cancel the bg tasks kicked off as we need their values to actually emit the event.
+        EventHookupSessionManager.DismissExistingSessions(cancelBackgroundTasks: false);


this dismissed the (Press TAB to insert) tooltip. we'll then pop up the Generating event... tooltip below in the BWI.

Pilchie · 2024-03-18T19:51:36Z

✅ Successfully linked to Azure Boards work item(s):

Bug 1991866

sharwell

Not quite done with review but have a few comments queued

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TypeCharCommand.cs

src/EditorFeatures/CSharp/EventHookup/EventHookupSessionManager.cs

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

CyrusNajmabadi · 2024-03-18T20:35:26Z

src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_SessionCancellingCommands.cs

@@ -19,7 +18,7 @@ internal partial class EventHookupCommandHandler :
    public bool ExecuteCommand(EscapeKeyCommandArgs args, CommandExecutionContext context)
    {
        _threadingContext.ThrowIfNotOnUIThread();
-        EventHookupSessionManager.CancelAndDismissExistingSessions();
+        EventHookupSessionManager.DismissExistingSessions();


Dismiss inherently will cancel, if the work has not been detached from teh session.

CyrusNajmabadi · 2024-03-19T07:17:22Z

@sharwell ptal

CyrusNajmabadi · 2024-03-19T07:38:36Z

src/EditorFeatures/CSharp/EventHookup/EventHookupSessionManager_EventHookupSession.cs

            var cancellationToken = _cancellationTokenSource.Token;
            _textView = textView;
            _subjectBuffer = subjectBuffer;
            _globalOptions = globalOptions;
            this.TESTSessionHookupMutex = testSessionHookupMutex;

-            var document = textView.TextSnapshot.GetOpenDocumentInCurrentContextWithChanges();
-            var workspace = textView.TextSnapshot.TextBuffer.GetWorkspace();
-            if (document != null && workspace != null && workspace.CanApplyChange(ApplyChangesKind.ChangeDocument))


moved to the caller htat creates the session.

CyrusNajmabadi · 2024-03-19T17:08:03Z

src/EditorFeatures/CSharp/EventHookup/EventHookupSessionManager.cs

        }
    }
-
-    internal bool IsTrackingSession()


was actually unused. so just removed.

CyrusNajmabadi added 4 commits March 18, 2024 10:38

In progress

9e216ac

In progress

bc43807

Almost done

7f51760

Fixes

21a9d0e

CyrusNajmabadi requested a review from sharwell March 18, 2024 18:42

CyrusNajmabadi requested a review from a team as a code owner March 18, 2024 18:42

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 18, 2024

Usings

1467b86

CyrusNajmabadi commented Mar 18, 2024

View reviewed changes

Remove assert

244847f

CyrusNajmabadi commented Mar 18, 2024

View reviewed changes

CyrusNajmabadi added 3 commits March 18, 2024 11:54

Tweak

a453e3e

Lint

f9e9b5b

lint

383d897

sharwell requested changes Mar 18, 2024

View reviewed changes

CyrusNajmabadi added 5 commits March 18, 2024 13:13

Comment

41d9222

Remove unused method

6bdcef6

Remove checks

9e71c1b

Cleanup

cfb756d

Detach later

4657126

CyrusNajmabadi requested a review from sharwell March 18, 2024 20:35

CyrusNajmabadi commented Mar 18, 2024

View reviewed changes

Error paths

3668aa9

CyrusNajmabadi and others added 6 commits March 18, 2024 13:44

Rename

6bf26cf

Simplify

1453e39

Make static

c9f8dd9

Fix

c2c4be0

Add check

0383ef4

Fix

cd71e7f

better thread transitions

9c73cd2

CyrusNajmabadi commented Mar 19, 2024

View reviewed changes

sharwell approved these changes Mar 19, 2024

View reviewed changes

CyrusNajmabadi merged commit 991d1f2 into dotnet:main Mar 19, 2024
27 checks passed

CyrusNajmabadi deleted the eventHookup branch March 19, 2024 17:31

dotnet-policy-service bot added this to the Next milestone Mar 19, 2024

dotnet-bot mentioned this pull request Mar 21, 2024

[Automated] PRs inserted in VS build main-34720.100 #72629

Closed

RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make event-hookup-completion async, and hook up to the background work indicator #72584

Make event-hookup-completion async, and hook up to the background work indicator #72584

CyrusNajmabadi commented Mar 18, 2024 •

edited by Pilchie

Loading

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi Mar 18, 2024

Pilchie commented Mar 18, 2024

sharwell left a comment

CyrusNajmabadi Mar 18, 2024

CyrusNajmabadi commented Mar 19, 2024

CyrusNajmabadi Mar 19, 2024

CyrusNajmabadi Mar 19, 2024

Make event-hookup-completion async, and hook up to the background work indicator #72584

Make event-hookup-completion async, and hook up to the background work indicator #72584

Conversation

CyrusNajmabadi commented Mar 18, 2024 • edited by Pilchie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Mar 18, 2024

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 18, 2024 •

edited by Pilchie

Loading