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
Merged

Conversation

@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 19 commits Aug 9, 2017
This isn't pretty but gets the UI control to where we want it.
Initialize with InlineReviewsPackage.
This should probably initialize earlier from its own package.
Remove view when there is no PR session.
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 7 commits Dec 19, 2017
Thanks @jcansdale!
jcansdale added 8 commits Feb 28, 2018
Rather than extending IGitHubServiceProvider, pass a delegate for GetServiceAsync.
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
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 4 commits Mar 2, 2018
- 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
…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 from the Microsoft.VisualStudio.Shell.Immutable.14.0 assembly.
@shana
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
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
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.

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
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
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
Projects
2.4.0
  
Done
2.4.3
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.