-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Initialize GitHubPane and menus asynchronously #1569
Changes from 8 commits
c20e618
0527497
266ec58
5730008
ec46954
48cb0cc
ca0a273
f66afaf
47d3753
e9c8db9
2fc764b
e6ee44d
ed0cb39
5dcf605
29508bb
89ffd87
e3158d2
255b007
4746859
a9c5c16
0504119
cb26608
7511113
0983ef0
39d77aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,10 @@ | |
using System.Windows.Controls; | ||
using System.Windows.Controls.Primitives; | ||
using System.ComponentModel.Composition; | ||
using GitHub.Commands; | ||
using GitHub.InlineReviews.Views; | ||
using GitHub.InlineReviews.ViewModels; | ||
using GitHub.Services; | ||
using GitHub.VisualStudio; | ||
using GitHub.Models; | ||
using GitHub.Logging; | ||
using GitHub.Extensions; | ||
|
@@ -19,21 +19,28 @@ namespace GitHub.InlineReviews.Services | |
/// <summary> | ||
/// Manage the UI that shows the PR for the current branch. | ||
/// </summary> | ||
[Export(typeof(PullRequestStatusBarManager))] | ||
public class PullRequestStatusBarManager | ||
{ | ||
static readonly ILogger log = LogManager.ForContext<PullRequestStatusBarManager>(); | ||
const string StatusBarPartName = "PART_SccStatusBarHost"; | ||
|
||
readonly IUsageTracker usageTracker; | ||
readonly IGitHubServiceProvider serviceProvider; | ||
readonly IShowCurrentPullRequestCommand showCurrentPullRequestCommand; | ||
|
||
IPullRequestSessionManager pullRequestSessionManager; | ||
// More the moment this must be constructed on the main thread. | ||
// TeamExplorerContext needs to retrieve DTE using GetService. | ||
readonly Lazy<IPullRequestSessionManager> pullRequestSessionManager; | ||
|
||
[ImportingConstructor] | ||
public PullRequestStatusBarManager(IUsageTracker usageTracker, IGitHubServiceProvider serviceProvider) | ||
public PullRequestStatusBarManager( | ||
IUsageTracker usageTracker, | ||
IShowCurrentPullRequestCommand showCurrentPullRequestCommand, | ||
Lazy<IPullRequestSessionManager> pullRequestSessionManager) | ||
{ | ||
this.usageTracker = usageTracker; | ||
this.serviceProvider = serviceProvider; | ||
this.showCurrentPullRequestCommand = showCurrentPullRequestCommand; | ||
this.pullRequestSessionManager = pullRequestSessionManager; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -46,8 +53,7 @@ public void StartShowingStatus() | |
{ | ||
try | ||
{ | ||
pullRequestSessionManager = serviceProvider.GetService<IPullRequestSessionManager>(); | ||
pullRequestSessionManager.WhenAnyValue(x => x.CurrentSession) | ||
pullRequestSessionManager.Value.WhenAnyValue(x => x.CurrentSession) | ||
.Subscribe(x => RefreshCurrentSession()); | ||
} | ||
catch (Exception e) | ||
|
@@ -58,21 +64,24 @@ public void StartShowingStatus() | |
|
||
void RefreshCurrentSession() | ||
{ | ||
var pullRequest = pullRequestSessionManager.CurrentSession?.PullRequest; | ||
var pullRequest = pullRequestSessionManager.Value.CurrentSession?.PullRequest; | ||
var viewModel = pullRequest != null ? CreatePullRequestStatusViewModel(pullRequest) : null; | ||
ShowStatus(viewModel); | ||
} | ||
|
||
PullRequestStatusViewModel CreatePullRequestStatusViewModel(IPullRequestModel pullRequest) | ||
{ | ||
var dte = serviceProvider.TryGetService<EnvDTE.DTE>(); | ||
var command = new RaisePullRequestCommand(dte, usageTracker); | ||
var pullRequestStatusViewModel = new PullRequestStatusViewModel(command); | ||
var trackingCommand = new UsageTrackingCommand(showCurrentPullRequestCommand, usageTracker); | ||
var pullRequestStatusViewModel = new PullRequestStatusViewModel(trackingCommand); | ||
pullRequestStatusViewModel.Number = pullRequest.Number; | ||
pullRequestStatusViewModel.Title = pullRequest.Title; | ||
return pullRequestStatusViewModel; | ||
} | ||
|
||
void IncrementNumberOfShowCurrentPullRequest() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty method? |
||
{ | ||
} | ||
|
||
void ShowStatus(PullRequestStatusViewModel pullRequestStatusViewModel = null) | ||
{ | ||
var statusBar = FindSccStatusBar(Application.Current.MainWindow); | ||
|
@@ -112,42 +121,39 @@ StatusBar FindSccStatusBar(Window mainWindow) | |
return contentControl?.Content as StatusBar; | ||
} | ||
|
||
class RaisePullRequestCommand : ICommand | ||
class UsageTrackingCommand : ICommand | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to use the 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll fix this in another PR. It's working for the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've moved the tracking of |
||
{ | ||
readonly string guid = Guids.guidGitHubCmdSetString; | ||
readonly int id = PkgCmdIDList.showCurrentPullRequestCommand; | ||
|
||
readonly EnvDTE.DTE dte; | ||
readonly ICommand command; | ||
readonly IUsageTracker usageTracker; | ||
|
||
internal RaisePullRequestCommand(EnvDTE.DTE dte, IUsageTracker usageTracker) | ||
internal UsageTrackingCommand(ICommand command, IUsageTracker usageTracker) | ||
{ | ||
this.dte = dte; | ||
this.command = command; | ||
this.usageTracker = usageTracker; | ||
} | ||
|
||
public bool CanExecute(object parameter) => true; | ||
|
||
public void Execute(object parameter) | ||
public event EventHandler CanExecuteChanged | ||
{ | ||
try | ||
add | ||
{ | ||
object customIn = null; | ||
object customOut = null; | ||
dte?.Commands.Raise(guid, id, ref customIn, ref customOut); | ||
command.CanExecuteChanged += value; | ||
} | ||
catch (Exception e) | ||
|
||
remove | ||
{ | ||
log.Error(e, "Couldn't raise {Guid}:{ID}", guid, id); | ||
command.CanExecuteChanged -= value; | ||
} | ||
} | ||
|
||
usageTracker.IncrementCounter(x => x.NumberOfShowCurrentPullRequest).Forget(); | ||
public bool CanExecute(object parameter) | ||
{ | ||
return command.CanExecute(parameter); | ||
} | ||
|
||
public event EventHandler CanExecuteChanged | ||
public void Execute(object parameter) | ||
{ | ||
add { } | ||
remove { } | ||
command.Execute(parameter); | ||
usageTracker.IncrementCounter(x => x.NumberOfShowCurrentPullRequest).Forget(); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
using System; | ||
using System.Threading; | ||
using System.ComponentModel.Design; | ||
using Microsoft.VisualStudio.Shell; | ||
using Microsoft.VisualStudio.Shell.Interop; | ||
using Task = System.Threading.Tasks.Task; | ||
|
||
namespace GitHub.Services.Vssdk | ||
{ | ||
public abstract class AsyncMenuPackage : AsyncPackage | ||
{ | ||
IVsUIShell vsUIShell; | ||
|
||
sealed protected async override Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress) | ||
{ | ||
vsUIShell = await GetServiceAsync(typeof(SVsUIShell)) as IVsUIShell; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is an helper method btw that you can use as ServiceProvider.GetGlobalServiceAsync<SVsUIShell, IVsUIShell> that handles this automatically. |
||
|
||
var menuCommandService = (OleMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService))); | ||
await InitializeMenusAsync(menuCommandService); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since menus have to be initialized on main thread, would be a good idea to switch to main thread before calling InitializeMenusAsync. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could... but then as an async method of its own, it should be sure to switch within the method itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you pass |
||
} | ||
|
||
protected abstract Task InitializeMenusAsync(OleMenuCommandService menuService); | ||
|
||
// The IDesignerHost, ISelectionService and IVsUIShell are requested by the MenuCommandService. | ||
// This override allows IMenuCommandService.AddCommands to be called form a background thread. | ||
protected override object GetService(Type serviceType) | ||
{ | ||
if (serviceType == typeof(SVsUIShell)) | ||
{ | ||
return vsUIShell; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
} | ||
|
||
if (serviceType == typeof(ISelectionService) || serviceType == typeof(IDesignerHost)) | ||
{ | ||
return null; | ||
} | ||
|
||
return base.GetService(serviceType); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,19 +2,18 @@ | |
using System.Windows; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using System.ComponentModel.Design; | ||
using System.ComponentModel.Composition; | ||
using System.Runtime.InteropServices; | ||
using GitHub.Api; | ||
using GitHub.Commands; | ||
using GitHub.Helpers; | ||
using GitHub.Info; | ||
using GitHub.Exports; | ||
using GitHub.Logging; | ||
using GitHub.Services; | ||
using GitHub.Services.Vssdk.Commands; | ||
using GitHub.ViewModels.GitHubPane; | ||
using GitHub.VisualStudio.UI; | ||
using GitHub.Services.Vssdk; | ||
using Microsoft.VisualStudio; | ||
using Microsoft.VisualStudio.ComponentModelHost; | ||
using Microsoft.VisualStudio.Shell; | ||
|
@@ -29,38 +28,19 @@ namespace GitHub.VisualStudio | |
[Guid(Guids.guidGitHubPkgString)] | ||
[ProvideMenuResource("Menus.ctmenu", 1)] | ||
[ProvideAutoLoad(Guids.UIContext_Git, PackageAutoLoadFlags.BackgroundLoad)] | ||
[ProvideToolWindow(typeof(GitHubPane), Orientation = ToolWindowOrientation.Right, Style = VsDockStyle.Tabbed, Window = EnvDTE.Constants.vsWindowKindSolutionExplorer)] | ||
[ProvideOptionPage(typeof(OptionsPage), "GitHub for Visual Studio", "General", 0, 0, supportsAutomation: true)] | ||
public class GitHubPackage : AsyncPackage | ||
public class GitHubPackage : AsyncMenuPackage | ||
{ | ||
static readonly ILogger log = LogManager.ForContext<GitHubPackage>(); | ||
|
||
protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress) | ||
protected async override Task InitializeMenusAsync(OleMenuCommandService menuService) | ||
{ | ||
LogVersionInformation(); | ||
await base.InitializeAsync(cancellationToken, progress); | ||
await GetServiceAsync(typeof(IUsageTracker)); | ||
|
||
// Avoid delays when there is ongoing UI activity. | ||
// See: https://github.com/github/VisualStudio/issues/1537 | ||
await JoinableTaskFactory.RunAsync(VsTaskRunContext.UIThreadNormalPriority, InitializeMenus); | ||
} | ||
|
||
void LogVersionInformation() | ||
{ | ||
var packageVersion = ApplicationInfo.GetPackageVersion(this); | ||
var hostVersionInfo = ApplicationInfo.GetHostVersionInfo(); | ||
log.Information("Initializing GitHub Extension v{PackageVersion} in {$FileDescription} ({$ProductVersion})", | ||
packageVersion, hostVersionInfo.FileDescription, hostVersionInfo.ProductVersion); | ||
} | ||
|
||
async Task InitializeMenus() | ||
{ | ||
var menuService = (IMenuCommandService)(await GetServiceAsync(typeof(IMenuCommandService))); | ||
var componentModel = (IComponentModel)(await GetServiceAsync(typeof(SComponentModel))); | ||
var exports = componentModel.DefaultExportProvider; | ||
|
||
await JoinableTaskFactory.SwitchToMainThreadAsync(); | ||
menuService.AddCommands( | ||
exports.GetExportedValue<IAddConnectionCommand>(), | ||
exports.GetExportedValue<IBlameLinkCommand>(), | ||
|
@@ -72,6 +52,14 @@ async Task InitializeMenus() | |
exports.GetExportedValue<IShowGitHubPaneCommand>()); | ||
} | ||
|
||
void LogVersionInformation() | ||
{ | ||
var packageVersion = ApplicationInfo.GetPackageVersion(this); | ||
var hostVersionInfo = ApplicationInfo.GetHostVersionInfo(); | ||
log.Information("Initializing GitHub Extension v{PackageVersion} in {$FileDescription} ({$ProductVersion})", | ||
packageVersion, hostVersionInfo.FileDescription, hostVersionInfo.ProductVersion); | ||
} | ||
|
||
async Task EnsurePackageLoaded(Guid packageGuid) | ||
{ | ||
var shell = await GetServiceAsync(typeof(SVsShell)) as IVsShell; | ||
|
@@ -147,7 +135,8 @@ public async Task<IGitHubPaneViewModel> ShowGitHubPane() | |
ErrorHandler.Failed(frame.Show()); | ||
} | ||
|
||
var viewModel = (IGitHubPaneViewModel)((FrameworkElement)pane.Content).DataContext; | ||
var gitHubPane = (GitHubPane)pane; | ||
var viewModel = (IGitHubPaneViewModel)(gitHubPane.View).DataContext; | ||
await viewModel.InitializeAsync(pane); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't |
||
return viewModel; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
using System; | ||
using System.Runtime.InteropServices; | ||
using GitHub.VisualStudio.UI; | ||
using Microsoft.VisualStudio.Shell; | ||
|
||
namespace GitHub.VisualStudio | ||
{ | ||
/// <summary> | ||
/// This is the host package for the <see cref="GitHubPane"/> tool window. | ||
/// </summary> | ||
/// <remarks> | ||
/// This package mustn't use MEF. | ||
/// See: https://github.com/github/VisualStudio/issues/1550 | ||
/// </remarks> | ||
[PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)] | ||
[Guid(Guids.GitHubPanePackageId)] | ||
[ProvideToolWindow(typeof(GitHubPane), Orientation = ToolWindowOrientation.Right, | ||
Style = VsDockStyle.Tabbed, Window = EnvDTE.Constants.vsWindowKindSolutionExplorer)] | ||
public sealed class GitHubPanePackage : AsyncPackage | ||
{ | ||
} | ||
} |
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 think you should bring back the
SwitchToMainThreadAsync
call that you removed. I expect themenuService
is an STA COM object that requires the UI thread.