Make sure we always subscribe to shared hierarchy eventing#26215
Merged
jasonmalinowski merged 1 commit intodotnet:dev15.7.xfrom Apr 20, 2018
Merged
Conversation
In d9e3002 I restored the code that was subscribing to IVsHierarchy event notifications to know when shared contexts change. Unfortunately it was put in the event handler for IVisualStudioHostDocument.Opened, which only runs when the document is being opened that was already part of the workspace. If the reverse order happens: a file is already open, and only _then_ added to the workspace, the code was skipped. Curiously, Workspace has an overridable method OnDocumentClosing which you can use to hook calls, but not an equivalent OnDocumentOpening. It's unclear to me if this design was something we want to extend (it's a bit strange) and so I'm not touching that for now. Fixes dotnet/project-system#3467.
Pilchie
approved these changes
Apr 18, 2018
| if (project.PushingChangesToWorkspace) | ||
| { | ||
| project.ProjectTracker.NotifyWorkspace(workspace => workspace.OnDocumentOpened(document.Id, document.GetOpenTextBuffer().AsTextContainer(), isCurrentContext)); | ||
| project.ProjectTracker.NotifyWorkspace(workspace => |
Member
There was a problem hiding this comment.
Consider extracting something like VisualStudioProjectTracker.NotifyWorkspaceOfOpenedDocument that does this, instead of adding it in 3 places.
Member
Author
There was a problem hiding this comment.
I thought about it, but then I'm spending time refactoring code I'm already deleting in a branch. 😄
Member
Author
|
@Pilchie @jinujoseph is this something we want to take for 15.7? The original bug was tagged as such. If so, I'll make the escrow bug. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In d9e3002 I restored the code that was subscribing to IVsHierarchy event notifications to know when shared contexts change. Unfortunately it was put in the event handler for IVisualStudioHostDocument.Opened, which only runs when the document is being opened that was already part of the workspace. If the reverse order happens: a file is already open, and only then added to the workspace, the code was skipped.
Curiously, Workspace has an overridable method OnDocumentClosing which you can use to hook calls, but not an equivalent OnDocumentOpening. It's unclear to me if this design was something we want to extend (it's a bit strange) and so I'm not touching that for now.
Fixes dotnet/project-system#3467.
Ask Mode template
Customer scenario
Open a CPS-based project in 15.7 that does multitargeting, or uses shared projects. If you open a file prior to the project system having fully updated the language (an operation that happens asynchronously), the project switcher won't work.
Bugs this fixes
dotnet/project-system#3467
Workarounds, if any
Close and reopen the affected file. There's no hint to the user that this workaround would work.
Risk
Low: just moving some code around to cover some missed cases.
Performance impact
None: code just moving around.
Is this a regression from a previous update?
Yes, code was refactored since 15.6 and this was introduced.
Root cause analysis
We connect to an open text buffer through two different codepaths:
We ran some additional connection code only in the first case, but not the second. The second now happens regularly in CPS, because CPS is now deferring informing the language service about files until much later in the load process, so it's easy to open a file before the project information has been wired up.
For preventing regressions, this area of code is being entirely refactored to address a number of issues; I'm not going to write tests that would soon be deleted.
How was the bug found?
Customer reported.