When we're removing the last project, fire SolutionRemoved#62257
When we're removing the last project, fire SolutionRemoved#62257jasonmalinowski merged 2 commits intodotnet:mainfrom
Conversation
So were none of these drop-caches working? |
|
@CyrusNajmabadi I didn't dig carefully, but probably not? Some code might have re-cleared though if a new solution was loaded. |
|
|
||
| // If this is our last project, remove the entire solution so components that expect to see a | ||
| // SolutionRemoved actually get it | ||
| if (w.CurrentSolution.ProjectIds.Count == 1) |
There was a problem hiding this comment.
not exactly sure how this works. is this because .CurrentSolution is not updated yet, despite teh call to _workspace.RemoveProjectFromTrackingMaps_NoLock(Id); above?
can we check this info immediately prior to doing any mutation to make things clearer? or at least doc why this is safe?
| } | ||
| } | ||
|
|
||
| base.OnProjectRemoved(projectId); |
There was a problem hiding this comment.
if we're not calling this here anymore, where is it calleD? i think i missed it.
There was a problem hiding this comment.
Which I think answers your other question too. Before we just called the overridden method, which did some cleanup and then deferred to the base. Now we call the cleanup, but let the caller call the appropriate On* method.
bc2c117 to
320e145
Compare
Subtle bug was found that makes the change much ickier.
320e145 to
37f4b33
Compare
37f4b33 to
4b3db49
Compare
|
@CyrusNajmabadi @ryzngard @davidwengier I've had to make some tweaks to this PR after discovering some subtle behaviors; this needs a rereview and you should just review from scratch. |
...ualStudio/Core/Def/ProjectSystem/VisualStudioWorkspaceImpl.RemoveProjectReferenceUndoUnit.cs
Outdated
Show resolved
Hide resolved
ryzngard
left a comment
There was a problem hiding this comment.
LGTM as far as I can tell...
|
Chatting further with @CyrusNajmabadi we figure this test is actually no longer accurate: And instead we should be indeed asserting the working folder is null once the solution is closed. I'm go to update this PR which means we'll be moving back to SolutionRemoved as the event that's fired. 😄 |
Not sure why this would be "DocumentRemoved", so updated here.
4b3db49 to
4af8155
Compare
We have a fair bit of code that assumed it could look for a WorkspaceChangeKind of SolutionRemoved or SolutionCleared to indicate when we've closed the solution and can drop caches and such. Except we never actually fired anything like that. This fires SolutionRemoved on the removal of the last project. We had a test that asserted that the solution persistence service would continue to return the solution path even once the solution was closed; this fixes that test to assert that it's no longer available.
4af8155 to
e443265
Compare
|
@CyrusNajmabadi Updated the new behavior per our conversation on Friday; @ryzngard if you want to re-review, now might be a good time. 😄 |
|
@jasonmalinowski is there an easy way to know what changed? The commits don't seem terribly helpful. |
|
@ryzngard: ah, I force pushed to abandon the old approach, which I should not have done. (Or at least, I should have squashed only later.) Here's the diff in any case: https://github.com/dotnet/roslyn/compare/4b3db498d52d1a09ba26555d9570b6db054a97a4..4af81555ff176a0b096b019b11516fd19865a9af |
ah gotcha. Yes, changes look good to me. Thanks for the diff link |
We have a fair bit of code that assumed it could look for a WorkspaceChangeKind of SolutionRemoved or SolutionCleared to indicate when we've closed the solution and can drop caches and such. Except we never actually fired anything like that. This fires SolutionRemoved on the removal of the last project.
We had a test that asserted that the solution persistence service would continue to return the solution path even once the solution was closed; this fixes that test to assert that it's no longer available.