Simplify error handling and notifications in rename.#61515
Simplify error handling and notifications in rename.#61515CyrusNajmabadi merged 11 commits intodotnet:mainfrom
Conversation
| var notificationService = _workspace.Services.GetService<INotificationService>(); | ||
| notificationService.SendNotification( | ||
| error, EditorFeaturesResources.Rename_Symbol, NotificationSeverity.Error); | ||
| } |
There was a problem hiding this comment.
single place in inline-rename, and single place in rename-code-action where we notify the user of an error we couldnt' handle.
| var root = renameTrackingSolutionSet.RenamedSolution.GetDocument(docId).GetSyntaxRootSynchronously(CancellationToken.None); | ||
| finalSolution = finalSolution.WithDocumentSyntaxRoot(docId, root); | ||
| RenameTrackingDismisser.DismissRenameTracking(workspace, changedDocuments); | ||
| } |
There was a problem hiding this comment.
fairly certain not being in a finally here was a bug. the return at https://github.com/dotnet/roslyn/pull/61515/files?diff=unified&w=1#diff-444ebec94ff45aa833894aec3cf66ed4c3f13f5f4fc661c5b5ce17d730597d48L140 meant we could bail out without dismissing our rename tracking state.
| NotificationSeverity.Error); | ||
| return; | ||
| } | ||
| return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_was_cancelled_or_is_not_valid); |
There was a problem hiding this comment.
super unfortunate that this external-notification api we have doesn't have any way for the component returning 'false' to say what the issue is. So we have to give this unhelpful message here.
|
|
||
| if (_workspace.TryApplyChanges(finalSolution)) | ||
| if (!_workspace.TryApplyChanges(finalSolution)) | ||
| return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_could_not_complete_due_to_external_change_to_workspace); |
There was a problem hiding this comment.
changed from the silent failure approach from ebfore to at least having a notification about what happened.
| EditorFeaturesResources.Rename_Symbol, | ||
| NotificationSeverity.Information); | ||
| } | ||
| return (NotificationSeverity.Information, EditorFeaturesResources.Rename_operation_was_not_properly_completed_Some_file_might_not_have_been_updated); |
There was a problem hiding this comment.
kept this informational. but it certainly seems weird that we're effectivley saying "hey... heads up... things might be bad, but we have no idea why or waht you can do about it" :(
|
|
||
| if (!workspace.TryApplyChanges(newSolution)) | ||
| { | ||
| Contract.Fail("Rename Tracking could not update solution."); |
There was a problem hiding this comment.
fairly certain this would just corrupt state in the rename engine (due to the lack of finallys).
| var undoPrimitiveAfter = new UndoPrimitive(_stateMachine.Buffer, trackingSessionId, shouldRestoreStateOnUndo: false); | ||
| localUndoTransaction.AddUndo(undoPrimitiveAfter); | ||
|
|
||
| localUndoTransaction.Complete(); |
There was a problem hiding this comment.
not sure waht the best logic is to have here. previously, if applying failed, we would throw in Contract.Fail. so this code woudl never run. i'm preserving that logic here... which i think is desirable. if we didn't change the workspace we don't need an undo added to the stack.
but i'm not sure. we coudl always add these no matter what, but i'm not sure the implication of that.
we will at least add these on success, which was our logic from before.
There was a problem hiding this comment.
| finally | ||
| { | ||
| // If we successfully update the workspace then make sure the undo transaction is committed and is | ||
| // always able to undo anything any other external listener did. |
There was a problem hiding this comment.
same concept as before. if the workspace application fails, we don't add these. but if it succeeds, we always add this, regardless of waht our external-notification system does.
| RenameService.ActiveSession = null; | ||
| } | ||
|
|
||
| void LogPositionChanged(object sender, CaretPositionChangedEventArgs e) |
There was a problem hiding this comment.
We can get rid of the LogPositionChanged portion now. It is being resolved
There was a problem hiding this comment.
Oh? Do tell, I'm interested :-)
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs
Outdated
Show resolved
Hide resolved
…vider.RenameTrackingCommitter.cs
src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs
Outdated
Show resolved
Hide resolved
…vider.RenameTrackingCommitter.cs
| EditorFeaturesResources.Rename_Symbol, | ||
| NotificationSeverity.Information); | ||
| if (!_refactorNotifyServices.TryOnAfterGlobalSymbolRenamed(workspace, changedDocuments, symbol, newName, throwOnFailure: false)) | ||
| return (NotificationSeverity.Information, EditorFeaturesResources.Rename_operation_was_not_properly_completed_Some_file_might_not_have_been_updated); |
There was a problem hiding this comment.
what is the difference in experience if an error vs information is returned? Just thinking if something fails wouldn't that always be an error, or is failure here expected?
There was a problem hiding this comment.
I believe it affects the glyph we show in the notification window. i agree on this being wonky, but i decided to preserve semantics here vs change things...
Followup to #61512.