Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Show current PR on status bar #1396

Merged
merged 87 commits into from Mar 5, 2018

Conversation

Projects
6 participants
@jcansdale
Copy link
Collaborator

jcansdale commented Dec 18, 2017

The following PRs target this one:

  • Expose IVSGitExt as async VS service: #1510 (extends closed PR #1506)
  • Use custom UIContext to load GitHubPackage / PullRequestStatusBarPackage in repo context: #1512
  • Simplify the VSGitExt service: #1513

Functionality

  • Show PR associated with current branch on status bar
  • Click on PR to navigate to PR details

image

  • Tooltip shows PR number and title

image

To do

Related issues

  • Show PR branch on status bar and allow navigation to PR details: #1102
  • Highlight checked out PR in list: #1106

jcansdale added some commits Aug 9, 2017

Add PullRequestStatusView to SccStatusBar
This isn't pretty but gets the UI control to where we want it.
Show PR status after solution has loaded
Initialize with InlineReviewsPackage.
This should probably initialize earlier from its own package.
Only create PR status view when session changes
Remove view when there is no PR session.
Merge pull request #1102 from github/ui/pr-context
Show current PR on status bar and allow navigation to PR details MVP

@jcansdale jcansdale added the feature label Dec 18, 2017

@donokuda donokuda self-assigned this Dec 18, 2017

donokuda and others added some commits Dec 19, 2017

jcansdale added some commits Feb 28, 2018

Use a delegate for GetServiceAsync
Rather than extending IGitHubServiceProvider, pass a delegate for GetServiceAsync.
Add comments to factory method
Cache the call to GetService<IVSGitExt>().
Use Func<IVSGitExt> rather than Lazy<IVSGitExt> (which was a hack).
this.usageTracker = usageTracker;
this.serviceProvider = serviceProvider;

OnActiveRepositoriesChanged();

This comment has been minimized.

@jcansdale

jcansdale Mar 1, 2018

Author Collaborator

This might be called twice!

This comment has been minimized.

@jcansdale

jcansdale Mar 5, 2018

Author Collaborator

This will be fixed in #1512.

@donokuda

This comment has been minimized.

Copy link
Member

donokuda commented Mar 2, 2018

Just a heads up, I want to focus on some stuff around the code review workflow and won't be able to fix the icon color soon. Since it's not a critical blocker for this feature, we could consider merging if this PR looks good otherwise, and I can address it in a separate issue / PR.

jcansdale added some commits Mar 2, 2018

Simplify the VSGitExt service
- 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
Merge pull request #1510 from github/refactor/convert-VSGitExt-to-ser…
…vice

Expose IVSGitExt as async VS service
public VSGitExt(IGitHubServiceProvider serviceProvider)
: this(serviceProvider, new VSUIContextFactory(), new LocalRepositoryModelFactory())
public VSGitExt(Func<Type, Task<object>> getServiceAsync)
: this(getServiceAsync, new VSUIContextFactory(), new LocalRepositoryModelFactory())

This comment has been minimized.

@shana

shana Mar 5, 2018

Collaborator

Any reason to send a callback instead of sending in IAsyncServiceProvider (which provides GetServiceAsync) here?

This comment has been minimized.

@jcansdale

jcansdale Mar 5, 2018

Author Collaborator

The issue was that AsyncPackage implements 3 interfaces called IAsyncServiceProvider, 2 of which exist in the same namespace. Passing a delegate was a way to avoid taking a new dependency on Microsoft.VisualStudio.Shell.Immutable.14.0 and needing to disambiguate using extern alias (which works but is a bit messy).

I've pushed a commit that uses the interface from Microsoft.VisualStudio.Shell.Immutable.14.0. If we're likely to be using IAsyncServiceProvider elsewhere, having the correct assembly to hand is maybe a good thing (even if we need to use extern alias).

An alternative approach would be to use the lower level Interop.IAsyncServiceProvider interface from here:

namespace Microsoft.VisualStudio.Shell.Interop
{
    public interface IAsyncServiceProvider
    {
        IVsTask QueryServiceAsync([ComAliasName("OLE.REFGUID")] ref Guid guidService);
    }
}

The fact that this takes a Guid and returns a IVsTask again makes it a bit messy.

My instinct is to go with this last commit. What do you think?

Use IAsyncServiceProvider rather than delegate
Use IAsyncServiceProvider from the Microsoft.VisualStudio.Shell.Immutable.14.0 assembly.
@shana

This comment has been minimized.

Copy link
Collaborator

shana commented Mar 5, 2018

The issue was that AsyncPackage implements 3 interfaces called IAsyncServiceProvider, 2 of which exist in the same namespace. Passing a delegate was a way to avoid taking a new dependency on Microsoft.VisualStudio.Shell.Immutable.14.0 and needing to disambiguate using extern alias (which works but is a bit messy).

Which two assemblies provide the same type?

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Mar 5, 2018

@shana,

It's a collision between Microsoft.VisualStudio.Shell.Immutable.14.0 and Microsoft.VisualStudio.Shell.Framework, Version=15.

1>------ Build started: Project: GitHub.TeamFoundation.15, Configuration: Debug Any CPU ------
1>C:\Source\github.com\github\VisualStudio\src\GitHub.TeamFoundation.14\Services\VSGitExt.cs(15,60,15,81): error CS0433: The type 'IAsyncServiceProvider' exists in both
'Microsoft.VisualStudio.Shell.Framework, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 
'Microsoft.VisualStudio.Shell.Immutable.14.0, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
========== Build: 0 succeeded, 1 failed, 20 up-to-date, 0 skipped ==========

I'll see what happens if I remove the Microsoft.VisualStudio.Shell.Framework, Version=15 dependency from GitHub.TeamFoundation.15...

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Mar 5, 2018

Ah, we're forced to reference Microsoft.VisualStudio.Shell.Framework, Version=15 because of this line. 😭

image

This would however be an easy reflection call.

Give alias to ..Shell.Framework rather than ..Shell.Immutable.14
Use an alias on the up-level assembly rather than the shared one.
We're likely to want to use IAsyncServiceProvider from Microsoft.VisualStudio.Shell.Immutable.14 again, so give alias to Microsoft.VisualStudio.Shell.Framework instead.
@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Mar 5, 2018

I've changed it so that rather than giving an alias to the shared Microsoft.VisualStudio.Shell.Immutable.14 assembly that contains the IAsyncServiceProvider implementation that we want, we give an alias to Microsoft.VisualStudio.Shell.Framework, v15 that we only use once in VSGitServices.

This will make finding the correct IAsyncServiceProvider a lot easier in future!

@shana

shana approved these changes Mar 5, 2018

@jcansdale jcansdale merged commit 6fe4bf1 into master Mar 5, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

2.4.0 automation moved this from Punted to Done Mar 5, 2018

2.4.3 automation moved this from In progress to Done Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.