Fix various folding range, and general VS Code issues #9134
Conversation
This file contains 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
In hindsight this change was just causing more pain than it was worth. The document version cache maintains a list of document snapshots, and those snapshots are inherently tied to a project. Tieing the key to a project as well just made more work for more scenarios, eg when a document moves from the miscellaneous project to a real one, we wouldn't get a "document open" for it, but we would need to check if we were tracking the document in the misc project, and if so start tracking it in the real project etc. Doing this on just the file path makes that problem go away, and since a single Razor document across all projects has the same content, it shouldn't cause any issues.
The HandleCoreAsync method is called in a loop, and retries things, but also uses IEnumerables which meant that the actual calculation was deferred until the result was sent across the wire. I thought maybe this was causing issues with things operating on newer versions of a Razor document, with delegated responses coming from an older version. It also just didn't make sense with the retry logic. The other change here is to stop a notification appearing in VS Code, that the user can't do anything about anyway, and switch to a log message.
This matches what happens in OpenDocument, just doing it at Add time if its a document that is already open. The worry is that if we open a document in Misc Project, when we move it to a real project, we won't have "primed the pump" like we did when it was first opened.
One potential cause of incorrect folding ranges is if the Html content is "doubled" in the generated document. This is exactly the behaviour we saw with C# documents, I just completely missed applying it to Html. As with C# documents, the "hack" part of this will be addressed in a future PR I'm preparing.
| @@ -15,7 +14,7 @@ internal class DefaultDocumentVersionCache : DocumentVersionCache | |||
| internal const int MaxDocumentTrackingCount = 20; | |||
|
|
|||
| // Internal for testing | |||
| internal readonly Dictionary<DocumentKey, List<DocumentEntry>> DocumentLookup_NeedsLock; | |||
| internal readonly Dictionary<string, List<DocumentEntry>> DocumentLookup_NeedsLock; | |||
There was a problem hiding this comment.
Unrelated to your change, it terrifies me a bit when I see an identifier containing the words "NeedsLock" but also "Internal for testing". 😄
...or/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectSystem/DefaultRazorProjectService.cs
Outdated
Show resolved
Hide resolved
DustinCampbell
approved these changes
Aug 17, 2023
ryzngard
approved these changes
Aug 17, 2023
| @@ -15,7 +14,7 @@ internal class DefaultDocumentVersionCache : DocumentVersionCache | |||
| internal const int MaxDocumentTrackingCount = 20; | |||
|
|
|||
| // Internal for testing | |||
| internal readonly Dictionary<DocumentKey, List<DocumentEntry>> DocumentLookup_NeedsLock; | |||
| internal readonly Dictionary<string, List<DocumentEntry>> DocumentLookup_NeedsLock; | |||
ghost
added this to the 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1867769 hopefully
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1868418/ probably
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1868479 optimistically
Fixes #9131
Fixes #9125
Fixes #9060
This is a bit of a strange one, but while investigating the above issue I saw some definite quirks around project handling and folding range handling in VS Code. All of it I suspect is my fault, and most of it I've been able to fix by just pulling a few choice commits out of the dynamic document uri work I'm already doing.
The first commit is something I have done in my other branch, and fixes an issue in multi-targeting in VS. I saw the same issue today when debugging though, where the misc project and real project in our language server act as a sort of multi-targetted set.
The second commit is to fix a surprising experience when debugging folding ranges, where we only "fix" the results well after we'd calculated them. Given things like SourceText can be deferred, it seemed like a bad idea, though I don't think was actually causing the issues necessarily.
The third commit is also pulled out of my other branch, and again is to deal with odd transitions from misc project to real project.
The final commit is what I would call the "real" fix. Previously we saw doubled-up C# content cause issues, and applied a fix for it. My other branch formalizes that fix behind a feature flag. Why we didn't think to apply that fix to Html documents, I do not know, but the symptoms I saw during debugging indicate it was needed.