Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Expose IVSGitExt as async VS service #1510
Conversation
Lock prevent tasks being executed at the same time but doesn't stop them being started out of order.
PendingTasks is used to execute tasks in order on the thread pool.
This will allow this service to be used by a package that is initializing on a background thread.
| PendingTasks = InitializeAsync(); | ||
| } | ||
|
|
||
| async Task InitializeAsync() |
jcansdale
Feb 27, 2018
Author
Collaborator
We could explicitly call this after constructing the service.
We could explicitly call this after constructing the service.
jcansdale
Feb 28, 2018
Author
Collaborator
Since IVSGitExt is also consumed as a MEF service, we probably want the VS service to return as soon as possible (since there is no async loading for MEF services). Awaiting InitializeAsync before the service returns is probably a bad idea. Better to continue initializing in the background.
Since IVSGitExt is also consumed as a MEF service, we probably want the VS service to return as soon as possible (since there is no async loading for MEF services). Awaiting InitializeAsync before the service returns is probably a bad idea. Better to continue initializing in the background.
| @@ -107,12 +112,28 @@ public GHClient(IProgram program) | |||
| } | |||
| } | |||
|
|
|||
| [PartCreationPolicy(CreationPolicy.Shared)] | |||
| public class VSGitExtPart | |||
jcansdale
Feb 28, 2018
Author
Collaborator
I put this next to the export for IGitHubClient. Maybe we should collect all of the service exports together into a single file?
I put this next to the export for IGitHubClient. Maybe we should collect all of the service exports together into a single file?
shana
Feb 28, 2018
Collaborator
This shouldn't be here (and GitHubClient shouldn't be either so ignore that example :P). Ideally it should be right next to the implementation, but since we don't want to have multiple exports for both teamfoundation assemblies, and we don't have a separate assembly yet for mef implementations, this should probably go into GitHub.Exports or into the Services folder here in GitHub.VisualStudio.
It should also follow the convention of the other mef dispatchers and be called VSGitExtDispatcher. For it to actually work as a MEF dispatcher, it needs to implement IVSGitExt and have a mef export for that interface.
This shouldn't be here (and GitHubClient shouldn't be either so ignore that example :P). Ideally it should be right next to the implementation, but since we don't want to have multiple exports for both teamfoundation assemblies, and we don't have a separate assembly yet for mef implementations, this should probably go into GitHub.Exports or into the Services folder here in GitHub.VisualStudio.
It should also follow the convention of the other mef dispatchers and be called VSGitExtDispatcher. For it to actually work as a MEF dispatcher, it needs to implement IVSGitExt and have a mef export for that interface.
|
|
||
| IVSGitExt CreateVSGitExt(IGitHubServiceProvider sp) | ||
| { | ||
| switch (VSVersion.Major) |
jcansdale
Feb 28, 2018
Author
Collaborator
In the past I've always used DTE.Version to differentiate versions of Visual Studio. This always returns a string with the major version number with a .0 (even for different minor versions). I used VSVersion because it was there, but I think it might hit the file system when reading the file version number.
In the past I've always used DTE.Version to differentiate versions of Visual Studio. This always returns a string with the major version number with a .0 (even for different minor versions). I used VSVersion because it was there, but I think it might hit the file system when reading the file version number.
Keep it consistent with GetService<T> and GetService<T, Ret>. Fix failing VSGitExt unit tests.
81cc078
to
738b046
Rather than extending IGitHubServiceProvider, pass a delegate for GetServiceAsync.
| return new Lazy<IVSGitExt>(() => new VSGitExt14(GetServiceAsync)).Value; | ||
| case "15.0": | ||
| return new Lazy<IVSGitExt>(() => new VSGitExt15(GetServiceAsync)).Value; | ||
| default: |
shana
Feb 28, 2018
Collaborator
The logic to construct the correct type should be in the VSGitExtDispatcher, so callers don't have to worry about it.
Also, since it needs to call GetServiceAsync on an IAsyncServiceProvider, it should take IAsyncServiceProvider in its constructor, so we know that that's a dependency that this type has. Later, when GitHubServiceProvider supports async calls, we can implement IAsyncServiceProvider on it and pass it in without major changes to the code.
The logic to construct the correct type should be in the VSGitExtDispatcher, so callers don't have to worry about it.
Also, since it needs to call GetServiceAsync on an IAsyncServiceProvider, it should take IAsyncServiceProvider in its constructor, so we know that that's a dependency that this type has. Later, when GitHubServiceProvider supports async calls, we can implement IAsyncServiceProvider on it and pass it in without major changes to the code.
| switch (dte.Version) | ||
| { | ||
| case "14.0": | ||
| return new Lazy<IVSGitExt>(() => new VSGitExt14(sp.GetServiceAsync)).Value; |
shana
Feb 28, 2018
Collaborator
Why not just pass the IAsyncServiceProvider into VSGitExt14/15 instead?
Also, any specific reason why you need a Lazy here? You're evaluating it immediately after creating, which feels like it defeats the purpose of the Lazy in the first place.
Why not just pass the IAsyncServiceProvider into VSGitExt14/15 instead?
Also, any specific reason why you need a Lazy here? You're evaluating it immediately after creating, which feels like it defeats the purpose of the Lazy in the first place.
jcansdale
Feb 28, 2018
Author
Collaborator
Why not just pass the IAsyncServiceProvider into VSGitExt14/15 instead?
The multiple implementations of IAsyncServiceProvider wreck havoc. The GitHub.TeamFoundation assemblies pick up more dependencies and I end up having to use extern alias. Passing GetServiceAsync as a delegate ends up being much simpler.
You're evaluating it immediately after creating, which feels like it defeats the purpose of the Lazy in the first place.
I've added some comments and change it to use Func<IVSGitExt> instead (using Lazy was a bit of a hack). By referencing VSGitExt14 and VSGitExt15 inside a lambda expression we can avoid loading the incompatible version into VS (e.g. we mustn't load VSGitExt15 into VS 2015).
Why not just pass the IAsyncServiceProvider into VSGitExt14/15 instead?
The multiple implementations of IAsyncServiceProvider wreck havoc. The GitHub.TeamFoundation assemblies pick up more dependencies and I end up having to use extern alias. Passing GetServiceAsync as a delegate ends up being much simpler.
You're evaluating it immediately after creating, which feels like it defeats the purpose of the Lazy in the first place.
I've added some comments and change it to use Func<IVSGitExt> instead (using Lazy was a bit of a hack). By referencing VSGitExt14 and VSGitExt15 inside a lambda expression we can avoid loading the incompatible version into VS (e.g. we mustn't load VSGitExt15 into VS 2015).
|
|
||
| public async static Task<IVSGitExt> Create(IAsyncServiceProvider sp) | ||
| { | ||
| var dte = await sp.GetServiceAsync(typeof(DTE)) as DTE; |
shana
Feb 28, 2018
Collaborator
ApplicationInfo.GetVersion will give you the running VS version as a proper type, so you don't have to grab a separate service for that here
ApplicationInfo.GetVersion will give you the running VS version as a proper type, so you don't have to grab a separate service for that here
Cache the call to GetService<IVSGitExt>(). Use Func<IVSGitExt> rather than Lazy<IVSGitExt> (which was a hack).
c78162b
to
fc06da7
- Use `UIContext.WhenActivated` instead of UIContext.UIContextChanged event - Don't use async InitializeAsync now we're initializing on background thread - No more need to use ContinueWith
Simplify the VSGitExt service
This PR does the following
PullRequestStatusBarPackagecompletely on a background threadIVSGitExtas an async VS serviceIVSGitExtas a MEF serviceExtendIGitHubServiceProviderwithGetServiceAsync<T>()and<T, Ret>IAsyncServiceProviderimplementationsIVSGitExtfor the running DTE versionIt takes over from where #1506 left off, doing initialization asynchronously using
GetServiceAsyncto retrieveIGitExt. This means that we don't need to explicitly switch to the Main thread when retrievingIGitExt.