Catch and report when we are unable to make an invisible editor #78236
Conversation
ghost
added
Area-IDE
untriaged
| using var invisibleEditor = OpenInvisibleEditor(documentId); | ||
| TextEditApplication.UpdateText(newText, invisibleEditor.TextBuffer, EditOptions.None); | ||
| } | ||
| catch (System.Runtime.InteropServices.COMException ex) when (FatalError.ReportAndCatch(ex)) |
There was a problem hiding this comment.
Note this may not do much for us to diagnose this -- the COMException usually means the native stack has already unwound and returned to us an HRESULT.
There was a problem hiding this comment.
It won't hurt to merge this, it just might not be as useful as you're hoping.
There was a problem hiding this comment.
The old version of this code will cause an entire Fix All to fail to apply, and show an error to the user. I'm concerned the new code allows the Fix All to silently fail to apply part of a change and not present anything to the user.
There was a problem hiding this comment.
The old version just silently fails, without a message. Furthermore, I can't do anything.
This version at least succeeds, tells is there is an issue, and let's me fix the remaining files manually
There was a problem hiding this comment.
I would like to take this through. THe cons of hte current system are that it fails, and you have no recourse.
This approach allows most of hte operation to succeed and gives us information in the form of NFWs that we can use to justify investigations into fixing this. e.g. how many hits we're getting of this underlying problem. Right now, we've had ethe problem for years, with no movement. So this seems strictly better for customers, and mildly better for us.
There was a problem hiding this comment.
Ah I had missed this was report and catch and not report and propagate.
My worry with blindly catching is we're going to get some number of bugs where a fix applies part way, and the user will report that something didn't update, and we won't really know if this was the cause or not. How hard would it be to have a boolean that we clear at the start of TryApplyChanges, set if that catch triggers, and at the end of TryApplyChanges just pop a dialog that says "due to an internal error, the change partially applied"? Assuming users will at least say "hey I got that error message" we know what happened for them versus there being a bug in the refactoring that meant it missed a place to update in the first place.
There was a problem hiding this comment.
I mean, we get no message today when things partially apply (existing files remain edited, we don't roll them back), or fail to apply. So it seems like we're already bad.
But i can try to add some message. But this really feels like overkill when we're already in a bad state. This just gets us marginally better for users (some more changes, instead of less), and gets us information on the problem.
|
@jasonmalinowski ptal. |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
Really nice, and I like this applies to all failures, not just the invisible editor path specifically if something else goes wrong.
| private bool _isExternalErrorDiagnosticUpdateSourceSubscribedToSolutionBuildEvents; | ||
|
|
||
| /// <summary> | ||
| /// Only read/written on hte UI thread. |
There was a problem hiding this comment.
| /// Only read/written on hte UI thread. | |
| /// Only read/written on the UI thread. |
| // All info bar actions (except for 'show stack trace') dismiss the info bar, putting us back in the | ||
| // "we're not showing the user anything" state. | ||
| _isShowingDocumentChangeErrorInfoBar = false; |
There was a problem hiding this comment.
Too bad there's not a generic way for us just to pass a handler for "any way this goes away".... :-/
4 participants
Hit this while trying to actually do fix-all in roslyn. i'd like us to not block changes while also getting NFWs to help track down what causes this.