Change Cancel() to async in InlineRenameSession. #74946
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
|
||
| // This wait is safe. We are not passing the async callback to DismissUIAndRollbackEditsAndEndRenameSessionAsync. | ||
| // So everything in that method will happen synchronously. | ||
| DismissUIAndRollbackEditsAndEndRenameSessionAsync( |
There was a problem hiding this comment.
Key change in the PR.
I feel we don't need _threadingContext.ThrowIfNotOnUIThread(); because in DismissUIAndRollbackEditsAndEndRenameSessionAsync it immediately switches to main thread.
| @@ -223,7 +224,7 @@ public bool Submit() | |||
| public void Cancel() | |||
| { | |||
| SmartRenameViewModel?.Cancel(); | |||
| Session.Cancel(); | |||
| _ = Session.CancelAsync(); | |||
There was a problem hiding this comment.
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.
| private async Task CancelAsync() | ||
| { | ||
| //.ConfigureAwait(true) because we need to set focus later | ||
| await _model.Session.CancelAsync().ConfigureAwait(true); | ||
| _textView.VisualElement.Focus(); |
There was a problem hiding this comment.
why not set focus on the _textView prior to cancelling the session (which seems like it could take unbounded time).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok. I've made a different change here: #74948
Cancellation here should be synchronous here.
There is a Task.Wait() in Cancel() method of inline rename session.
This PR removes it and make all other calls to be async