VS: reduce some coupling between components #15000
Conversation
psfinaki
left a comment
psfinaki
left a comment
There was a problem hiding this comment.
@majocha thanks for your efforts to reduce the chaos in the Editor :)
I am still getting my head around this, but for now let me ask you this (maybe I misunderstand something).
Looking at your changes, I see we are getting rid of some parameters passed to some services, replacing them with calls to GlobalProvider. Are you sure it's a move to the right direction? Propagating all the same parameters through the code flow is not good but this feels a bit like Service Locator which is arguably not a nice thing either.
I know this is happening in some other places in code but I wonder if this is the consistency we want to achieve.
P.S. A small ask for the future. This is definitely a PR of an acceptable size - still it would benefit from a guiding description to the contributors - like where to start and so on. Although I hope AI will soon be able to generate this for us :)
|
@psfinaki this is mostly to unblock me on the quickinfo tests in the other PR. As I look at this again, I think keeping extension methods on IServiceProvider would be good enough, as GlobalProvider is always available from VS shell. I'll modify this. |
|
Alright. Yep, thanks for the explanation. Eventually, it should be exactly this - moving towards fewer bugs and better testability. |
|
I left Shell's GlobalProvider where it made the most sense and modified extension methods on IAsyncServiceProvider and IServiceProvider to resemble what Roslyn does. |
|
|
||
| | _ -> [] | ||
|
|
||
| // TODO: Uncomment code when VS has a fix for updating the status bar. |
There was a problem hiding this comment.
BTW do you think this is still relevant? :)
There was a problem hiding this comment.
😀 I bet VS statusbar works fine. Just not in our code. We should really stand of the shoulders of experts and either copy what VSIX Community Toolkit does or even just install their nuget.
|
|
||
| member _.TryGoToDefinition(position, cancellationToken) = | ||
| let gtd = GoToDefinition(metadataAsSource) | ||
| let statusBar = StatusBar() |
There was a problem hiding this comment.
So, here (and elsewhere) - are you sure creating a status bar from scratch is equivalent to passing around the thing that flows in?
There was a problem hiding this comment.
Looking again at this, IMV we should just yeet all the statusbar code. It does nothing anyway.
There was a problem hiding this comment.
Looks like it was just a clunky wrapper around SVsStatusbar service, when it worked, pretty much stateless on its own, so creating it ad hoc is ok. Removing this completely looks like serious work, this file is 1000+ freaking lines🙃
|
|
||
| [<AutoOpen>] | ||
| module internal ServiceProviderExtensions = | ||
| type internal System.IServiceProvider with |
There was a problem hiding this comment.
So once again, what's the need for this change? I'm not against it, just a bit paranoid and trying to better understand all this magic
There was a problem hiding this comment.
No immediate need. Although I just found a use for GetServiceAsync in another branch.
This is pretty much copied from Roslyn repo.
The problem is when coding VSIX stuff in C# you'll get warnings from analyzers when doing unsafe things like calling GetService from background thread, we don't have that.
This is not immediately a problem because we have all the related bugs Ironed out over the years, it's more so we don't run into problems in the future.
There was a problem hiding this comment.
Maybe leave the old overload as is, and name the new one GetServiceOnMainThread? What do you think @psfinaki ?
|
@psfinaki, I'll draft this, since I managed to fix my quickinfo pr with minimal changes and this is not urgently needed. |
|
@majocha actually it feels like we are getting to the point :) If you've noticed that StatusBar is useless, feel free to create a ticket about that and we can jump on it - either from this PR or from scratch. But this will be a major relief - the more crap we remove from the Editor code the better it is for everyone! Meanwhile thanks for all the investigation, I'm also learning a lot here. |
|
@psfinaki, makes sense, I'll make an issue. |
2 participants
Especially StatusBar was passed around waaay too much.
Also: grab VS services on the main thread just in case.