Check UI context before trying to initialize cohosting #73172
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
davidwengier
force-pushed
the
RazorUIContext
branch
from
7722de5 to
ec833c0
Compare
Co-authored-by: ady109 <[email protected]>
| var uiContext = UIContext.FromUIContextGuid(Constants.RazorCohostingUIContext); | ||
| uiContext.WhenActivated(() => | ||
| { | ||
| // Not using the cancellation token passed in, as the context could be activated well after LSP server initialization |
There was a problem hiding this comment.
Hmm, might need to expose a way for something to access a general server cancellation token for things that should be cancelled when the server shutsdown.
There was a problem hiding this comment.
IDisposable was enough for this, and much simpler.
One thing I noticed from my testing though, as FYI, the "AlwaysActive" server initializes when Roslyn loads, and then shuts down when a solution is closed. Fine, makes sense. BUT if I then open a solution again, it doesn't get initialized again until a C# file is opened. In other words. No idea if that could/would be a problem ever.
There was a problem hiding this comment.
Right, dispose should take care of it since its an LSP service - good call.
BUT if I then open a solution again, it doesn't get initialized again until a C# file is opened. In other words. No idea if that could/would be a problem ever.
Yes, theres a potential bug there - workspace level requests won't get fulfilled on the second solution open unti la file is opened. I think its because the VS workspace doesn't get recreated (so the event listener doesn't trigger).
Register the UI context for Razor, so it can be used in Roslyn (see dotnet/roslyn#73172) Will eventual fix #10279 once the Roslyn side is in Part of #9519
|
|
||
| namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.Cohost; | ||
|
|
||
| [ExportCSharpVisualBasicLspServiceFactory(typeof(RazorDynamicRegistrationService), WellKnownLspServerKinds.AlwaysActiveVSLspServer), Shared] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class RazorDynamicRegistrationServiceFactory([Import(AllowDefault = true)] IRazorCohostDynamicRegistrationService? dynamicRegistrationService) : ILspServiceFactory | ||
| internal sealed class RazorDynamicRegistrationServiceFactory( | ||
| [Import(AllowDefault = true)] IUIContextActivationService? uIContextActivationService, |
There was a problem hiding this comment.
Do we have to allow null here because this is used in vscode?
There was a problem hiding this comment.
This is null because I'm paranoid, and an interface defined in one layer, and implemented in another, seems like a recipe for null to me. This doesn't ship in VS Code, at least not in the .roslyn folder, but it is present in OOP, and who knows where else. It shouldn't be imported by anything other than the always active LSP server, so maybe this is worrying about nothing?
There was a problem hiding this comment.
Ah if it is in OOP it might not be present - but then editor features I don't think is in OOP, so maybe the interface definition needs to move down? Not 100% sure on that one
There was a problem hiding this comment.
EditorFeatures is in OOP on my machine at least. Perhaps it shouldn't be, but in future if we split the EA into language server and editor features layers, this would stick with editors features anyway I think. Plus I can't see any obvious place to put this that makes sense in Features :P
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.Shared.Utilities; | ||
|
|
||
| internal interface IUIContextActivationService |
There was a problem hiding this comment.
Editor features is not available in vscode. If you're expecting the code in the EA layer to run in vscode, this needs to be moved down, to the protocol layer or features layer.
There was a problem hiding this comment.
As mentioned, this doesn't ship in VS Code. We'll need a new EA assembly for that eventually.
Fixes dotnet/razor#10279 and a 4 image load regression on the latest Razor insertion
Fixes AB#2045232