Prevent CodeActions commits during Inline Renames #683
Conversation
|
@heejaechang @shyamnamboodiripad For now I'm just preventing lightbulb items from being committed while Inline Rename is up (to prevent this actual known crash) while we think through the larger problem. Does this look okay? |
| { | ||
| if (notificationService != null) | ||
| { | ||
| notificationService.SendNotification(EditorFeaturesResources.CannotApplyOperationWhileRenameSessionIsActive, severity: NotificationSeverity.Error); |
There was a problem hiding this comment.
notificationService?.SendNotification(...);
|
|
|
👍 |
|
Do you have a story tracking the bigger work item to disallow two consumers from forking a Workspace (per the discussion we had offline). In another offline discussion with Sri and HeeJae, looks like we are going to need that for other cases too... Light bulb will be the main consumer for this right now - but I think this is something we need more generally to avoid problems like this. |
|
|
||
| Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Utilities | ||
| Friend Class TestNotificationService | ||
| Implements INotificationService |
There was a problem hiding this comment.
Rather than implement this, you can use the real EditorNotificationService and cast to the internal INotificationServiceCallback interface which lets you set an Action<> NotificationCallback to be invoked instead of the real notification getting fired. INotificationServiceCallback was created specifically for testing.
There was a problem hiding this comment.
Amusingly, we already had 3 test implementations of this! Will switch to the INotificationServiceCallback approach. Thanks!
|
@shyamnamboodiripad Yep, #681 (it's also mentioned in the commit comment so people can find it from here in the future). The issue is mostly a placeholder for now, but we can adjust it & fill in the details. I also had some more conversations with @mattwar and the IDE team about this yesterday, so let's combine ideas over at #681. |
|
@dpoeschl #681 seems to specifically talk about rename. However, the workspace ownership is a general problem in other scenarios too. For example, when a debug session is active but code is not at a breakpoint we have similar problem where light bulb offers fixes - but fixes silently fail because workspace refuses to apply the changes in this mode. If we had a mechanism to track workspace ownership, then we could just hide the light bulb in such cases / at least avoid failing silently (we could say that debugger owns the workspace and hence a fix can't modify it). I'll add same comment in #681 too. |
|
👍 |
Fix dotnet#554: This fixes a crash that was caused by invoking a lightbulb item that caused an Inline Rename session to launch when there's already an inline rename session active. We now check for inline rename sessions during lightbulb commit. If an inline rename session is active, we halt the lightbulb commit and tell the user to complete their rename session. This only prevents one (but probably the most frequent) manifestation of a larger problem in which rename is trying to perform a series of changes to the workspace that can be interrupted by some other feature applying its changes to the workspace. This larger design problem is tracked as dotnet#681.
dpoeschl
force-pushed
the
1120490InlineRenameStillActive
branch
from
e5efcbb
to
b21230a
Compare
Prevent CodeActions commits during Inline Rename
Fix #554: This fixes a crash that was caused by invoking a lightbulb
item that caused an Inline Rename session to launch when there's already
an inline rename session active. We now check for inline rename sessions
during lightbulb commit. If an inline rename session is active, we halt
the lightbulb commit and tell the user to complete their rename session.
This only prevents one (but probably the most frequent) manifestation of
a larger problem in which rename is trying to perform a series of
changes to the workspace that can be interrupted by some other feature
applying its changes to the workspace. This larger design problem is
tracked as #681.