-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add another UI context to control Razor cohost pre-init #79875
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
Add another UI context to control Razor cohost pre-init #79875
Conversation
fc7466d
to
540014c
Compare
540014c
to
e260767
Compare
@@ -11,6 +11,8 @@ internal static class Constants | |||
{ | |||
public const string RazorLanguageName = LanguageInfoProvider.RazorLanguageName; | |||
|
|||
// The UI context is provided by Razor, so this guid must match the one in https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/RazorConstants.cs | |||
// These UI contexts are provided by Razor, so must match https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/RazorConstants.cs |
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.
Should Razor simply use these values from the EA?
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 already think it's a bit weird that a UI context defined in Razor is only used in Roslyn. Making it so that a UI context defined in Razor, and used in Roslyn, has a Guid defined in Roslyn, is not really any weirder, though it would mean our RazorPackage would need IVT to the EA. I don't know at what point any of this feels too weird to be worth doing.
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.
Yeah maybe not worth the trouble then. Is there a unit test in razor to ensure these are in sync?
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.
No, but thats a great idea! Will add that to the Razor side of things
Putting this up as draft for now, gonna see if I can do a dual test insertion to confirm this makes RPS happy. The UI context is defined in dotnet/razor#12079 so its a little bit painful to validate everything together :)This PR does two things:
Until Razor is in this won't do anything, but runs in the dual test insertion (https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/664449) look good so far.