Consolidate VS Command Architecture #1502

grokys · 2018-02-22T17:58:33Z

Previously in Commands

We previously had two separate command architectures

GitHub.VisualStudio.Menus

These classes had some problems:

Defined in GitHub.VisualStudio so couldn't be used in lower-level assemblies
No support for command parameters
The serviceProvider.AddCommandHandler extension method did some sync service requests making it non-ideal for use in AsyncPackages
Can't be bound to UI elements

GitHub.InlineReviews.Commands

These classes were written because of the problems with the prior framework (most notably point 1) but had their own problems:

Were defined in GitHub.InlineReviews so couldn't be used in lower-level assemblies (I'd done that with the intention of doing this consolidation at some point)
Made calls to the Package for services
Used a "magic" MEF registration that could cause all MEF assemblies to be loaded when not needed

This PR

This PR consolidates the two frameworks and attempts to solve the problems of the two. There are the same VsCommand and VsCommand<T> classes as in the GitHub.InlineReviews.Commands namespace, but:

They don't use "magic" package registration
Instead I've added some extension methods to IMenuCommandService to register them and removed the AddCommandHandler extension methods that used sync GetService calls
They implement ICommand so they can be bound in the UI
They inherit directly from OleCommand rather than wrapping an OleCommand so they can be used wherever an OleCommand is accepted
I also added an extension method IMenuCommandService.BindCommand to bind an existing ICommand (such as a ReactiveCommand) to a VS command. This is used in the GitHubPaneViewModel

In addition, I've added a new assembly: GitHub.Services.Vssdk for the implementations. We've been talking about this for a while, and it's really needed here. I originally called it GitHub.Services.VisualStudio but decided to change it to Vssdk because we already use VisualStudio in GitHub.VisualStudio and thought it was clearer that it means "abstractions over the VSSDK"

Fixes #1489

Fixes #1489.

- Moved `VsCommand` etc out of `GitHub.InlineReviews` and into a new assembly `GitHub.Services.Vssdk` which will be a home for services which have a hard VSSDK dependency. - Moved the command interfaces into `GitHub.Exports` - Made the `VsCommand` classes inherit from `OleMenuCommand` - Removed the automatic package registration stuff

And use it in the `GitHubPaneViewModel`.

And remove old menu framework-related code.

As it causes Rx to get loaded.

Thought I'd need it, turns out I didn't.

jcansdale · 2018-03-01T12:50:06Z

src/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModel.cs

-                         usageTracker.IncrementCounter(x => x.NumberOfGitHubPaneHelpClicks).Forget();
-                     });
+                var menuService = (IMenuCommandService)paneServiceProvider.GetService(typeof(IMenuCommandService));
+                BindNavigatorCommand(menuService, PkgCmdIDList.pullRequestCommand, showPullRequests);


Is there a reason you're not using the BindCommand extension method?

BindNavigatorCommand does use BindCommand. It's just shorthand so we don't have to specify new CommandID(Guids.guidGitHubToolbarCmdSet, PkgCmdIDList.foo) for every command and can just pass the PkgCmdIDList.foo part`.

jcansdale · 2018-03-01T13:19:24Z

src/GitHub.VisualStudio/Commands/CreateGistCommand.cs

+        protected override void QueryStatus()
+        {
+            Log.Assert(SelectedTextProvider != null, "Could not get an instance of ISelectedTextProvider");
+            Visible = !string.IsNullOrWhiteSpace(SelectedTextProvider?.GetSelectedText());


Unrelated to this PR, but just while I remember. We should default to selecting all text. I bet most people don't even know this command exists! ;-)

Added an issue here: #1534

jcansdale · 2018-03-01T13:27:23Z

src/GitHub.VisualStudio/Commands/CreateGistCommand.cs

+        protected CreateGistCommand(IGitHubServiceProvider serviceProvider)
+            : base(CommandSet, CommandId)
+        {
+            dialogService = new Lazy<IDialogService>(() => serviceProvider.TryGetService<IDialogService>());


I'm curious why we're not passing Lazy<IDialogService> (and others) into the importing constructor?

Good question: basically this is how we were doing it before. I'll try using Lazy<>.

jcansdale

Should we be loading InlineReviewsPackage on a background thread?

jcansdale · 2018-03-02T11:02:09Z

src/GitHub.InlineReviews/InlineReviewsPackage.cs

-    [PackageRegistration(UseManagedResourcesOnly = true)]
+    [PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]
    [Guid(Guids.InlineReviewsPackageId)]
    [ProvideAutoLoad(UIContextGuids80.SolutionExists)]


This will cause the package to auto-load on the Main thread. I think we should be using this:

[ProvideAutoLoad(UIContextGuids80.SolutionExists, PackageAutoLoadFlags.BackgroundLoad)]

In future we might want to only auto-load when there is an active Git repository (see #1512)?

[ProvideAutoLoad(Guids.UIContext_Git, PackageAutoLoadFlags.BackgroundLoad)]

jcansdale · 2018-03-02T11:03:21Z

src/GitHub.InlineReviews/InlineReviewsPackage.cs

        {
-            base.Initialize();
-            PackageResources.Register(this);
+            await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();


I don't think we should be on the Main thread.

No need to manually construct Lazy loading using GetService.

Avoid using SwitchToMainThreadAsync because it can hang when called before a solution is loaded.

jcansdale · 2018-03-14T12:31:19Z

I've made some auto-load related tweaks to the packages that install commands (InlineReviewsPackage and GitHubPackage).

Rather than using await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync() (which can hang in certain circumstances), change package registration to AllowsBackgroundLoading = false. This is the default and will cause the package to initialize on the Main thread. It is then okay to install commands without calling SwitchToMainThreadAsync.

I've changed InlineReviewsPackage to use: [ProvideAutoLoad(Guids.UIContext_Git)]. This will cause it to only load when we're in the context of a Git repo.

There is also some merge related cleanup.

Be explicit about which code needs the Main thread Don't auto-load in background to prevent potential SwitchToMainThreadAsync hang. SwitchToMainThreadAsync can hang so use ThreadingHelper.MainThreadDispatcher.InvokeAsync instead.

An old issue crept back in during merge.

jcansdale · 2018-03-14T14:59:51Z

Forget what I said above about using AllowsBackgroundLoading = false. I've changed it back to using the following:

[PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]
[ProvideAutoLoad(Guids.UIContext_Git, PackageAutoLoadFlags.BackgroundLoad)]

I'm using the same workaround as in PullRequestStatusBarPackage to avoid using SwitchToMainThreadAsync. By using await ThreadingHelper.MainThreadDispatcher.InvokeAsync we avoid the potential hang.

grokys added 6 commits February 15, 2018 22:58

Make InlineReviewsPackage async.

01bbc84

Fixes #1489.

Add a way to bind ICommands to VS commands.

0ccf448

And use it in the `GitHubPaneViewModel`.

Use VsCommand in GitHub.VisualStudio.

b80e0ab

And remove old menu framework-related code.

Lazy load IDialogService.

fc267e9

As it causes Rx to get loaded.

Removed unused assembly.

8c1e9ab

Thought I'd need it, turns out I didn't.

grokys requested review from StanleyGoldman, jcansdale and shana February 22, 2018 17:58

grokys added 2 commits February 22, 2018 19:01

Added assembly info to GitHub.Services.Vssdk

c2ec33d

Removed unused stuff.

dc1209c

jcansdale reviewed Mar 1, 2018

View reviewed changes

jcansdale mentioned this pull request Mar 1, 2018

GitSccProvider is overly active #1511

Closed

jcansdale suggested changes Mar 2, 2018

View reviewed changes

Merge branch 'master' into refactor/vs-commands

d4daa1e

jcansdale force-pushed the refactor/vs-commands branch from 4fecb2c to d4daa1e Compare March 14, 2018 10:07

jcansdale added 8 commits March 14, 2018 10:17

Merge branch 'master' into refactor/vs-commands

b802bc9

Clean up usings

d27e551

Remove obsolete IMenuProvider

6a0e9a5

Remove obsolete files

e09e33f

Pass Lazy services into CreateGistCommand

494dfeb

No need to manually construct Lazy loading using GetService.

Prune usings

5ad71c4

Initialize menus on Main thread

cfe23e1

Avoid using SwitchToMainThreadAsync because it can hang when called before a solution is loaded.

Only auto-load InlineReviewsPackage when in context of Git repo

584ebc5

jcansdale added 3 commits March 14, 2018 13:21

Fix hanging GitHubPaneViewModel.InitializeAsync regression

8936ea9

An old issue crept back in during merge.

Remove unused GitHubPackage serviceProvider field

1baa921

jcansdale force-pushed the refactor/vs-commands branch from 9cd28b2 to 1baa921 Compare March 14, 2018 14:51

jcansdale approved these changes Mar 14, 2018

View reviewed changes

jcansdale mentioned this pull request Mar 14, 2018

Make InlineReviewsPackage async. #1497

Closed

jcansdale merged commit f8b3e9d into master Mar 14, 2018

jcansdale deleted the refactor/vs-commands branch March 14, 2018 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Consolidate VS Command Architecture #1502

Consolidate VS Command Architecture #1502

Uh oh!

grokys commented Feb 22, 2018 •

edited by jcansdale

Loading

Uh oh!

jcansdale Mar 1, 2018

Uh oh!

grokys Mar 1, 2018 •

edited

Loading

Uh oh!

jcansdale Mar 1, 2018

Uh oh!

jcansdale Mar 14, 2018

Uh oh!

jcansdale Mar 1, 2018

Uh oh!

grokys Mar 1, 2018

Uh oh!

jcansdale left a comment

Uh oh!

jcansdale Mar 2, 2018

Uh oh!

jcansdale Mar 2, 2018

Uh oh!

jcansdale commented Mar 14, 2018

Uh oh!

jcansdale commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Consolidate VS Command Architecture #1502

Consolidate VS Command Architecture #1502

Uh oh!

Conversation

grokys commented Feb 22, 2018 • edited by jcansdale Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Previously in Commands

GitHub.VisualStudio.Menus

GitHub.InlineReviews.Commands

This PR

Uh oh!

jcansdale Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Mar 14, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

grokys Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale left a comment

Choose a reason for hiding this comment

Uh oh!

jcansdale Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Mar 14, 2018

Uh oh!

jcansdale commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grokys commented Feb 22, 2018 •

edited by jcansdale

Loading

grokys Mar 1, 2018 •

edited

Loading