Initialize GitHubPane and menus asynchronously #1569

jcansdale · 2018-03-28T14:51:02Z

As suggested by @AArnott and @BertanAygun in #1560.

This PR

Makes GitHubPane initialize MEF on a background thread
Uses dedicated package for GitHubPane without MEF dependency
Creates commands using MEF on a background thread
Expose IGitHubPaneViewModel via GetViewModelAsync
Logs initialization exceptions in a TextBox

This PR would make #1560 obsolete.

Reviewers

Should we show anything while MEF is initializing (it doesn't look too bad if left blank)
If an exception is thrown, I'm simply showing it in a TextBox (to allow the user to copy/paste). Is this enough?
~~Please sanity check AsyncMenuPackage~~ (it was crazy and is now gone 😉)

Todo

Decide what to display when MEF is loading/refreshing
Decide how to present exceptions

Fixes #1550

There is no need to do this on the Main thread.

Call the IShowCurrentPullRequestCommand directly rather than using DTE.

This resolves the, "Logical tree depth exceeded while traversing the tree. This could indicate a cycle in the tree" exception.

The package that hosts GitHubPane mustn't initialize MEF (even on a background thread).

AArnott · 2018-03-28T16:40:52Z

Do we want to display anything while MEF is loading/refreshing?

You could do what we do with 15.6 async tool windows and display a caption and a spinner control indicating that you're initializing.

What should happen if MEF cache is corrupt/out-of-date and tool window throws?

If querying MEF throws an exception to you indicating the cache is out of date, you could display an error on your tool window advising the user of this state so they can restart.

AArnott · 2018-03-28T16:43:23Z

src/GitHub.InlineReviews/InlineReviewsPackage.cs

-            var componentModel = (IComponentModel)(await GetServiceAsync(typeof(SComponentModel)));
-            var exports = componentModel.DefaultExportProvider;
-
-            await JoinableTaskFactory.SwitchToMainThreadAsync();


I think you should bring back the SwitchToMainThreadAsync call that you removed. I expect the menuService is an STA COM object that requires the UI thread.

AArnott · 2018-03-28T16:45:07Z

src/GitHub.Services.Vssdk/AsyncMenuPackage.cs

+
+        sealed protected async override Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
+        {
+            vsUIShell = await GetServiceAsync(typeof(SVsUIShell)) as IVsUIShell;


You should use await this.JoinableTaskFactory.SwitchToMainThreadAsync(CancellationToken); as the first line in your method. Casting the SVsUIShell object to an interface itself generally requires the UI thread, and otherwise you're vulnerable to a deadlock by relying on RPC to get you to the UI thread.

There is an helper method btw that you can use as ServiceProvider.GetGlobalServiceAsync<SVsUIShell, IVsUIShell> that handles this automatically.

AArnott · 2018-03-28T16:45:29Z

src/GitHub.Services.Vssdk/AsyncMenuPackage.cs

+            vsUIShell = await GetServiceAsync(typeof(SVsUIShell)) as IVsUIShell;
+
+            var menuCommandService = (OleMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));
+            await InitializeMenusAsync(menuCommandService);


Can you pass CancellationToken into this method so that it can cancel async init operations if VS starts to shut down?

AArnott · 2018-03-28T16:47:17Z

src/GitHub.Services.Vssdk/AsyncMenuPackage.cs

+        {
+            if (serviceType == typeof(SVsUIShell))
+            {
+                return vsUIShell;


Your comment here is interesting. Returning this STA COM object object from a free-threaded method doesn't solve the problem that the caller will very likely cast this to an interface while still on the background thread that your comment suggests it will be on. You mustn't cast COM objects to any interface, or invoke those interfaces, while on a background thread unless you're sure they are free-threaded objects (and I don't think this one is one of those few).

AArnott · 2018-03-28T16:49:54Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

-                var provider = VisualStudio.Services.GitHubServiceProvider;
-                var teServiceHolder = provider.GetService<ITeamExplorerServiceHolder>();
-                teServiceHolder.ServiceProvider = serviceProvider;
+                InitializeAsync(serviceProvider).Forget();


If this async method throws, you'll never know.
I find it's best to create an extension method on Task (we call ours FileAndForget()) that will attach an OnlyOnFaulted continuation (or await) on the Task and file a failure report, or display an error to the user, or whatever... something to draw attention to the failure so it's more diagnosable.

AArnott · 2018-03-28T16:50:40Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

-            }
+            var factory = provider.GetService<IViewViewModelFactory>();
+            viewModel = provider.ExportProvider.GetExportedValue<IGitHubPaneViewModel>();
+            viewModel.InitializeAsync(this).Forget();


Same here. As I sometimes (flippantly) say around my own team, everywhere you use Forget() consider removing the line entirely -- since evidently you don't care whether it succeeds or fails. 😉

BertanAygun · 2018-03-28T16:02:05Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

+
+        async Task InitializeAsync(IServiceProvider serviceProvider)
+        {
+            // Allow MEF to initialize its cache asynchronously


Would be a good idea to assert that you are on main thread here using ThreadHelper.ThrowIfNotOnUIThread

The threading rules state async methods should not throw when called on the wrong thread, but rather should simply switch to the thread they require (using await SwitchToMainThreadAsync or await TaskScheduler.Default.)

BertanAygun · 2018-03-28T16:06:39Z

src/GitHub.Services.Vssdk/AsyncMenuPackage.cs

+            vsUIShell = await GetServiceAsync(typeof(SVsUIShell)) as IVsUIShell;
+
+            var menuCommandService = (OleMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));
+            await InitializeMenusAsync(menuCommandService);


Since menus have to be initialized on main thread, would be a good idea to switch to main thread before calling InitializeMenusAsync.

You could... but then as an async method of its own, it should be sure to switch within the method itself.

BertanAygun · 2018-03-28T17:09:03Z

src/GitHub.Services.Vssdk/AsyncMenuPackage.cs

+
+        sealed protected async override Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
+        {
+            vsUIShell = await GetServiceAsync(typeof(SVsUIShell)) as IVsUIShell;


There is an helper method btw that you can use as ServiceProvider.GetGlobalServiceAsync<SVsUIShell, IVsUIShell> that handles this automatically.

jcansdale · 2018-03-28T17:27:49Z

@AArnott @BertanAygun, Thank for the feedback! I'll dig into this properly tomorrow.

Just one quick question.

You mustn't cast COM objects to any interface, or invoke those interfaces, while on a background thread unless you're sure they are free-threaded objects (and I don't think this one is one of those few).

How do I know what VS services it's okay retrieve using GetServiceAsync and call on a background thread?

I know the menuService is a managed object, but it retrieves SVsUIShell via a call to GetService. I thought this was why it was failing when used from an AsyncPackage. Are you saying that because depends on SVsUIShell, it isn't safe to use from a background thread?

AArnott · 2018-03-28T17:45:23Z

How do I know what VS services it's okay retrieve using GetServiceAsync and call on a background thread?

We don't have a great answer for that. The best we have is "assume all services you query IServiceProvider/IAsyncServicePRovider for require the UI thread for all access unless documentation explicitly states it's a free-threaded object".

SComponentModel is a free-threaded object.

We are formulating a better answer to this with the vssdk-analyzers but it's not ready yet.

This reverts commit 5730008.

This reverts commit 0527497.

jcansdale · 2018-03-29T08:46:46Z

There is an helper method btw that you can use as ServiceProvider.GetGlobalServiceAsync<SVsUIShell, IVsUIShell> that handles this automatically.

Thanks for the tip. Is there an equivalent if this given an IServiceProvider object? I'd like something that I'd be able to call from my unit tests. For example, say I want to get hold of the DTE object from a free-threaded MEF component.

I'm guessing I could do something like this (not very pretty):

        [ImportingConstructor]
        FindDte([Import(typeof(SVsServiceProvider))] IServiceProvider sp)
        {
            var asp = (IAsyncServiceProvider)sp.GetService(
                typeof(Microsoft.VisualStudio.Shell.Interop.SAsyncServiceProvider));
            dte = (DTE)asp.GetServiceAsync(typeof(DTE)).Result;
        }

or using the GetGlobalServiceAsync method you mentioned:

        FindDte()
        {
            dte = await ServiceProvider.GetGlobalServiceAsync<EnvDTE.DTE, EnvDTE.DTE>();
        }

Is there a better way that's both concise and unit testable?

jcansdale · 2018-03-29T09:06:23Z

We don't have a great answer for that. The best we have is "assume all services you query IServiceProvider/IAsyncServicePRovider for require the UI thread for all access unless documentation explicitly states it's a free-threaded object".

Once I have a COM object, is there any way I can interrogate it to see if it's a STA component? I'm guessing not and the difference is baked into the code rather than the interfaces/meta-data?

I've noticed that VS SDK sample code tends to use GetService(typeof(SVsService)) as IVsService rather than (IVsService)GetService(typeof(SVsService)). Is there a reason for this beyond wanting to be sure that the cast doesn't throw? Generally I'd want that extra information to show up in my logs, so I'd cast rather than use as. Am I doing it wrong?

AArnott · 2018-03-29T14:47:32Z

Once I have a COM object, is there any way I can interrogate it to see if it's a STA component? I'm guessing not and the difference is baked into the code rather than the interfaces/meta-data?

There is not.

I've noticed that VS SDK sample code tends to use GetService(typeof(SVsService)) as IVsService rather than (IVsService)GetService(typeof(SVsService)).

I have a blog post on this topic that might help. In VS, services/packages may fail to load (or simply be skipped such as in safe mode) so we try to code defensively, in expecting nulls from GetService. As for cast failures, if we're prepared for a null result, then we'd already be prepared for a cast failure as well so long as it returns null instead of throwing. There are many valid reasons for a cast to fail when considering the code we write may support older versions of VS before the interface was implemented (or was written when an interface was still in development and not guaranteed to be present). Since so many times there is a valid reason for a null result or failed cast, I personally tend to (and advocate for) using as cast and failing gracefully on a null result wherever possible, without asking whether it's appropriate in this case since it does no harm if it will never happen, and the pattern is often good.

When you require a non-null service for your package/service to do anything useful, I throw an informative message by calling Assumes.Present(object) after the GetService call/cast, since that can tell the type you were expecting even if it's null (via the inferred generic type argument) and will throw an exception if it's null with a message explaining what was missing.

Similarly, I advocate for using GetServiceAsync whenever I'm in an async method even if the service I'm querying for may not (currently) be async. That way when the service becomes available asynchronously in the future, it's not my code that's holding the user back from experiencing the full benefits of not blocking the UI thread any more.

AArnott · 2018-03-29T14:52:24Z

Is there an equivalent [of GetServiceAsync<TService, TInterface>] given an IServiceProvider object?

I don't think so, and certainly one that makes it safe to call from a background thread. All GetService calls should be done from the UI thread -- for unit tests it seems perfectly excusable to call from any thread (if it works), but never in production code.

@jcansdale You included a snippet where you had an ImportingConstructor with a GetService call. I advise against this, because MEF is a free-threaded system and your importer (generally speaking) may be on any thread, you shouldn't assume you're on the UI thread as required for calling GetService. If you do assume it, then it can deadlock when you're not on that thread due to your caller's thread.

MEF part initialization should be free-threaded. If it has thread affinity, move it to a method that you'll call on the object after importing it.

We need an async way to obtain the IGitHubPaneViewModel.

jcansdale · 2018-04-04T10:03:06Z

@AArnott Thanks for your tips/advice. I've taken note.

I've updated this PR with the changes you requested. Any chance you could take another look?

jcansdale · 2018-04-04T10:14:50Z

@AArnott,

MEF part initialization should be free-threaded. If it has thread affinity, move it to a method that you'll call on the object after importing it.

The problem is my MEF component isn't tied to a particular package that it would be convenient to initialize it from. It could be used by a number of other MEF components.

Would something like this be okay?

        [ImportingConstructor]
        FindDte([Import(typeof(SVsServiceProvider))] IServiceProvider sp)
        {
            InitializeAsync(sp).FileAndForget();
        }

        async Task InitializeAsync(IServiceProvider sp)
        {
            await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
            dte = (DTE)sp.GetService(typeof(DTE));
        }

(assuming it was able to deal with dte being null for a while)

I'd add a shim that makes SwitchToMainThreadAsync into a nop when testing.

grokys

Looks great in general. Just a few small changes.

grokys · 2018-04-04T10:39:50Z

src/GitHub.InlineReviews/Services/PullRequestStatusBarManager.cs

@@ -112,42 +121,39 @@ StatusBar FindSccStatusBar(Window mainWindow)
            return contentControl?.Content as StatusBar;
        }

-        class RaisePullRequestCommand : ICommand
+        class UsageTrackingCommand : ICommand


Is this intended to be a more general class now? (the rename suggests that it is). If so, should it be moved to a place where it can be reused more easily and made to accept a lambda which identifies the usage model field? This doesn't need to be in this PR though.

I changed it to use the IShowCurrentPullRequestCommand directly rather than using dte.Commands.Raise. The problme was I lost the metrics.

I'm thinking it would be a better idea to record the metrics in the command itself? Only issue is, you wouldn't be able to differentiate where the user found the command (not that it matters yet in this case).

I'll fix this in another PR. It's working for the moment.

I've moved the tracking of NumberOfShowCurrentPullRequest into the command itself (where I think it belongs). RaisePullRequestCommand is now obsolete.

grokys · 2018-04-04T10:48:17Z

src/GitHub.InlineReviews/Services/PullRequestStatusBarManager.cs

            pullRequestStatusViewModel.Number = pullRequest.Number;
            pullRequestStatusViewModel.Title = pullRequest.Title;
            return pullRequestStatusViewModel;
        }

+        void IncrementNumberOfShowCurrentPullRequest()


Empty method?

grokys · 2018-04-04T10:52:43Z

src/GitHub.VisualStudio/GitHubPackage.cs

@@ -147,7 +148,8 @@ protected override Task InitializeAsync(CancellationToken cancellationToken, IPr
                ErrorHandler.Failed(frame.Show());
            }

-            var viewModel = (IGitHubPaneViewModel)((FrameworkElement)pane.Content).DataContext;
+            var gitHubPane = (GitHubPane)pane;
+            var viewModel = await gitHubPane.GetViewModelAsync();
            await viewModel.InitializeAsync(pane);


Won't viewModel.InitializeAsync have already been called by the time GetViewModelAsync returns?

grokys · 2018-04-04T11:06:52Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

@@ -131,6 +144,16 @@ public override void OnToolWindowCreated()
            UpdateSearchHost(pane?.IsSearchEnabled ?? false, pane?.SearchQuery);
        }

+        void ShowInitializing()
+        {
+            View = new Label { Content = "Initializing MEF" };


I know you were asking what to display here. We probably shouldn't display a spinner as it will be really jerky while MEF is initializing. I think the following should suffice:

View = new TextBlock { Text = "Initializing...", HorizontalAlignment = HorizontalAlignment.Center, VerticalAlignment = VerticalAlignment.Center, };

I changed the text to just "Initializing..." as MEF might not mean much to users.

grokys · 2018-04-04T11:07:42Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

+
+        void ShowError(Exception e)
+        {
+            View = new TextBox { Text = e.ToString() };


Should make this TextBox read-only:

View = new TextBox { Text = e.ToString(), IsReadOnly = true, };

jcansdale · 2018-04-04T12:32:16Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

-            }
+            var factory = provider.GetService<IViewViewModelFactory>();
+            var viewModel = provider.ExportProvider.GetExportedValue<IGitHubPaneViewModel>();
+            viewModel.InitializeAsync(this).Catch(ShowError).Forget();


Should await this here.

GitHubPackage shouldn't be concerned about this.

AArnott · 2018-04-04T13:52:43Z

@jcansdale said:

I'd add a shim that makes SwitchToMainThreadAsync into a nop when testing.

Be careful how you add that shim. Don't avoid using the JTF type directly from all your code because only by using that type directly do the vs-threading analyzers help keep you on the safe threading path.
For testability in the context of a MEF part, you can [Import] JoinableTaskContext (I'm not sure what the minimum VS version is for that) and use its JTF instance. Your test can then export an instance of that.

InitializeAsync(sp).FileAndForget();

That sounds like your MEF part will be unusable for an undefined length of time, and all callers will have to be prepared to deal with that. What I rather do is make InitializeAsync a public/internal method on the type and anyone who imports my MEF part must await InitializeAsync before calling any other members on the export. That way deadlocks can be mitigated while ensuring the export will work as expected.

The problem is my MEF component isn't tied to a particular package that it would be convenient to initialize it from. It could be used by a number of other MEF components.

You shouldn't assume your package is loaded anyway when a MEF part is activated, since MEF can activate parts without the VS package ever having loaded (generally speaking). That's why I advocate for the caller initializing the part by invoking InitializeAsync().

AArnott · 2018-04-04T13:57:02Z

src/GitHub.InlineReviews/InlineReviewsPackage.cs

@@ -38,11 +38,14 @@ public class InlineReviewsPackage : AsyncPackage
            var menuService = (IMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));


Here you're casting what is likely an STA COM object to an interface without first ensuring your on the UI thread. Every async method is responsible for ensuring it's on the right thread rather than assuming anything from its caller, so you should add await this.JoinableTaskFactory.SwitchToMainThreadAsync(CancellationToken); here.
In particular, your caller is AsyncPackage.InitializeAsync which will be called on a background thread when the package isn't queried for synchronously. And although you use JTF.RunAsync with a UIThreadNormalPriority setting, that does not switch the delegate to the UI thread. Like all JTF.RunAsync overloads, the delegate is executed immediately on the caller's thread. The UIThreadNormalPriority argument you pass in only influences the priority with which a switch to the UI thread may occur. So you should have a switch to main thread before casting a VS COM service that you don't know is free-threaded.
Alternatively, use await GetServiceAsync<TService, TInterface>() which will do the cast while on the UI thread automatically, allowing you to remain on a background thread till you actually need to invoke the interface, which I see you do at the bottom after a switch to the main thread. :) This approach would leave this method running on a background thread during MEF's GetExportedValue method calls that you have below, which can be a huge win for customers when those exports come from an assembly that has not yet loaded.

AArnott · 2018-04-04T14:02:58Z

src/GitHub.VisualStudio/GitHubPackage.cs

@@ -59,17 +57,20 @@ void LogVersionInformation()
            var menuService = (IMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));


Same comment as I made to your other InitializeMenus() method.

Oops, I remember you said. I'm sorry I missed these. 😊

AArnott · 2018-04-04T14:06:20Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

        }

-        public void Initialize(IServiceProvider serviceProvider)
+        public Task<IGitHubPaneViewModel> GetViewModelAsync() => viewModelSource.Task;


This looks like it will likely be misused to violate the 3rd threading rule. If someone tries to await this Task within the context of a JTF.Run delegate, you'll quite likely deadlock.
Can you replace your TaskCompletionSource with a JoinableTask instead? Then call InitializeAsync within a JTF.RunAsync delegate and store the resulting JoinableTask in your field.
Then this method should return joinableTask.JoinAsync() (or simply await joinableTask) to mitigate deadlocks in callers.

We should always cast STA COM services on Main thread.

This avoids the risk deadlock if called within the context of a JTF.Run delegate. https://github.com/Microsoft/vs-threading/blob/master/doc/threading_rules.md#rule-3-if-ever-awaiting-work-that-was-started-earlier-that-work-must-be-joined https://blogs.msdn.microsoft.com/andrewarnottms/2014/05/07/asynchronous-and-multithreaded-programming-within-vs-using-the-joinabletaskfactory/

jcansdale · 2018-04-04T15:37:38Z

@AArnott I'm picking up lots here. Plenty of homework to do!

I've updated it to use JoinableTask rather than TaskCompletionSource and cast IMenuCommandService on Main thread. Hopefully it's getting there.

jcansdale · 2018-04-04T15:48:43Z

@AArnott,

For testability in the context of a MEF part, you can [Import] JoinableTaskContext (I'm not sure what the minimum VS version is for that) and use its JTF instance. Your test can then export an instance of that.

That would be handy. Unfortunately it looks like it's a VS 2017 thing.

That sounds like your MEF part will be unusable for an undefined length of time, and all callers will have to be prepared to deal with that.

This isn't such a problem because it has some extra initialization to do anyway before it starts firing events. All callers do is subscribe to those events.

AArnott · 2018-04-04T17:30:23Z

src/GitHub.VisualStudio/UI/GitHubPane.cs

@@ -83,30 +80,38 @@ public GitHubPane() : base(null)

        protected override void Initialize()
        {
-            InitializeAsync().Catch(ShowError).Forget();
+            viewModelTask = ThreadHelper.JoinableTaskFactory.RunAsync(InitializeAsync);


Can you have this class accept an AsyncPackage in construction and then use the AsyncPackage.JoinableTaskFactory instance instead of ThreadHelper.JoinableTaskFactory here?

That way if VS shuts down before this work is done, we won't risk crashing due to arbitrary work going on in background threads.

Rather than retrieving SComponentModel asynchronously, we can simply retrieve IGitHubServiceProvider (which has the dependency on SComponentModel).

jcansdale · 2018-04-06T09:23:50Z

@AArnott I've made a couple more updates.

Use JoinableTaskFactory from the parent AsyncPackage
Retrieve IGitHubServiceProvider asynchronously (which had the dependency on SComponentModel)

That way as much initialization as possible is done asynchronously.

That way if VS shuts down before this work is done, we won't risk crashing due to arbitrary work going on in background threads.

I don't completely understand what's going on here (I'm guessing each package has a collection of tasks associated with it). Could you expand a little?

AArnott · 2018-04-06T19:01:13Z

Your changes look good, @jcansdale.

I don't completely understand what's going on here (I'm guessing each package has a collection of tasks associated with it). Could you expand a little?

Yes: AsyncPackage defines a JoinableTaskCollection and an associated JoinableTaskFactory. Anytime you use that JoinableTaskFactory instance, all the work it schedules is added to the JoinableTaskCollection while that work is active.
Later when the AsyncPackage.Dispose method is called as VS shuts down, that method will block the caller (the UI thread) until the JoinableTaskCollection is empty, the idea being to ensure that no threads or async work is ongoing after the package has claimed to have completed shutting down.

When threads are still running at the time VS shuts down the CLR's AppDomain, crashes often result. That's why we curtail the problem this way.

jcansdale added 8 commits March 20, 2018 21:58

Create MEF commands on background thread

c20e618

There is no need to do this on the Main thread.

Allow menuService.AddCommands to be called from a b/g thread

0527497

Construct PullRequestStatusBarManager using MEF on b/g thread

266ec58

Call the IShowCurrentPullRequestCommand directly rather than using DTE.

Factor b/g loading menu code into AsyncMenuPackage

5730008

Restore missing metrics

ec46954

Allow MEF to refresh its cache on a background thread

48cb0cc

Use ContentPresenter instead of ContentControl

ca0a273

This resolves the, "Logical tree depth exceeded while traversing the tree. This could indicate a cycle in the tree" exception.

Use package without MEF dependency for GitHubPane

f66afaf

The package that hosts GitHubPane mustn't initialize MEF (even on a background thread).

jcansdale requested a review from AArnott March 28, 2018 14:51

jcansdale mentioned this pull request Mar 28, 2018

Ensure that MEF is initialized using an old style non-async package #1560

Closed

AArnott suggested changes Mar 28, 2018

View reviewed changes

BertanAygun reviewed Mar 28, 2018

View reviewed changes

jcansdale added 3 commits March 28, 2018 20:04

Revert "Factor b/g loading menu code into AsyncMenuPackage"

47d3753

This reverts commit 5730008.

Revert "Allow menuService.AddCommands to be called from a b/g thread"

e9c8db9

This reverts commit 0527497.

Add placeholder initializing and error views

2fc764b

jcansdale added 3 commits March 29, 2018 19:35

Add GetViewModelAsync for GitHubPane

e6ee44d

We need an async way to obtain the IGitHubPaneViewModel.

Remove redundant initialization code

ed0cb39

Fixed typo

5dcf605

jcansdale added the Ready for Review label Apr 3, 2018

jcansdale mentioned this pull request Apr 3, 2018

Create and add MEF commands on background thread #1548

Closed

grokys suggested changes Apr 4, 2018

View reviewed changes

jcansdale commented Apr 4, 2018

View reviewed changes

jcansdale added 4 commits April 4, 2018 14:35

Make error text box read only

29508bb

Show Initializing... while MEF is loading

89ffd87

Make GitHubPane await initialization of IGitHubPaneViewModel

e3158d2

GitHubPackage shouldn't be concerned about this.

Remove empty IncrementNumberOfShowCurrentPullRequest method

255b007

AArnott suggested changes Apr 4, 2018

View reviewed changes

jcansdale added 3 commits April 4, 2018 15:18

Cast IMenuCommandService on Main thread

4746859

We should always cast STA COM services on Main thread.

This page is intentionally left blank

0504119

AArnott approved these changes Apr 4, 2018

View reviewed changes

jcansdale added 2 commits April 6, 2018 08:49

Use JoinableTaskFactory from parent AsyncPackage

cb26608

Retrieve IGitHubServiceProvider asynchronously

7511113

Rather than retrieving SComponentModel asynchronously, we can simply retrieve IGitHubServiceProvider (which has the dependency on SComponentModel).

Moved NumberOfShowCurrentPullRequest tracking into command

0983ef0

grokys previously approved these changes Apr 9, 2018

View reviewed changes

Keep CA happy

39d77aa

jcansdale dismissed grokys’s stale review via 39d77aa April 9, 2018 13:39

grokys approved these changes Apr 9, 2018

View reviewed changes

jcansdale merged commit d69a629 into master Apr 9, 2018

jcansdale deleted the fixes/1550-async-initialize-GitHubPane branch April 9, 2018 15:07

		@@ -38,11 +38,14 @@ public class InlineReviewsPackage : AsyncPackage
		var menuService = (IMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));

		@@ -59,17 +57,20 @@ void LogVersionInformation()
		var menuService = (IMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService)));

Initialize GitHubPane and menus asynchronously #1569

Initialize GitHubPane and menus asynchronously #1569

Uh oh!

Conversation

jcansdale commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Reviewers

Todo

Uh oh!

AArnott commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Mar 28, 2018

Uh oh!

AArnott commented Mar 28, 2018

Uh oh!

jcansdale commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcansdale commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AArnott commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AArnott commented Mar 29, 2018

Uh oh!

jcansdale commented Apr 4, 2018

Uh oh!

jcansdale commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

jcansdale commented Mar 28, 2018 •

edited

Loading

jcansdale commented Mar 29, 2018 •

edited

Loading

jcansdale commented Mar 29, 2018 •

edited

Loading

AArnott commented Mar 29, 2018 •

edited

Loading

jcansdale commented Apr 4, 2018 •

edited

Loading

jcansdale commented Apr 4, 2018 •

edited

Loading