-
Notifications
You must be signed in to change notification settings - Fork 230
Fix disco colors when modifying documents during project load #10354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Worth noting, both of these changes fix the issue at hand, but I feel like the existing behaviour in both cases is just objectively wrong, and only used to work because we guessed a project, and I think guessing is objectively wrong too. |
| new HostDocument(DocumentFilePath2, "C:/path/to/document2.cshtml"), CreateEmptyTextLoader()); | ||
| }); | ||
|
|
||
| using var listener = _projectManager.ListenToNotifications(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this listener?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't, thank you. Left over from copy-and-paste
| new HostDocument(DocumentFilePath1, "other/document1.cshtml"), CreateTextLoader(SourceText.From("Hello"))); | ||
| }); | ||
|
|
||
| using var listener = _projectManager.ListenToNotifications(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this listener?
alexgav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
| newTargetPath = newTargetPath[projectDirectory.Length..]; | ||
| } | ||
|
|
||
| var newHostDocument = new HostDocument(documentSnapshot.FilePath, newTargetPath, documentSnapshot.FileKind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that using the other HostDocument ctor may cause the logic to set FileKind to change, hopefully that's okay. (will no longer be null)
razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/HostDocument.cs
Lines 27 to 37 in 1a1d246
| public HostDocument(string filePath, string targetPath) | |
| : this(filePath, targetPath, fileKind: null) | |
| { | |
| } | |
| public HostDocument(string filePath, string targetPath, string? fileKind) | |
| { | |
| FilePath = filePath ?? throw new ArgumentNullException(nameof(filePath)); | |
| TargetPath = targetPath ?? throw new ArgumentNullException(nameof(targetPath)); | |
| FileKind = fileKind ?? FileKinds.GetFileKindFromFilePath(filePath); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other constructor determines the file kind from the file path, and it seemed unnecessary to do that if we already know the file kind. If the file kind didn't match the file path (ie, somehow we had a .razor file with a kind of "mvc") then the previous code would have "fixed" it, whereas the current code will keep it, but I honestly don't know which of those situations is right or wrong.
Fixes an issue Andrew found in speedometer tests. When opening files and modifying them, before we migrate the document to the "real" project, things get out of sync.
Also matches some stacks on PRISM, but I think this bug was only introduced in the last two weeks, so I don't think its the only cause thats for sure.