Refresh PullRequestSessionManager.CurrentSession when PR branch changes #1422

jcansdale · 2018-01-17T18:56:16Z

What's in this PR

Add TeamExplorerContext service for repo and branch change events
Change PullRequestSessionManager to use the ITeamExplorerContext service
Change GitHubPaneViewModel to use ITeamExplorerContext
- This prevents GitHub pane from changing to PR list when PR checkout touches .sln file
Revert IVSGitExt hack from PullRequestDetailViewModel
- This stops it from receiving multiple events when nothing of interest has changed
Fix null exception in IsPullRequestFromRepository when repo doesn't have a remote
Fix PullRequestSessionManager to refresh when PR Owner or Number changes
Add NUnit tests project GitHub.App.UnitTests for TeamExplorerContext tests
- This is to avoid stepping on @meaghanlewis's unit test refactorings

The TeamExplorerContext service

namespace GitHub.Services
{
    public interface ITeamExplorerContext : INotifyPropertyChanged
    {
        ILocalRepositoryModel ActiveRepository { get; }
        event EventHandler StatusChanged;
    }
}

The PropertyChanged event is fired when the repo changes
The StatusChanged event is fired when the branch name, head SHA or tracking SHA changes
- Duplicate events from IVSGitExt are ignored
When a PR touches the .sln file only the StatusChanged event is fired
- Previously ActiveRepository would be set to null and then again to the original repo
When a solution that isn't in Git is loaded ActiveRepository will be set to null

[Feature] Show current PR on status bar: [Feature] Show current PR on status bar #1396
Visual Studio crashes when navigating to Manage Connections page in Team Explorer Visual Studio crashes when navigating to Manage Connections page in Team Explorer #23
Make sure we don't crash when VS's git service returns bogus data: bc3d148

Notes

Re: issue #23, it looks like the current implementation of Team Explorer won't return null.

The original report was from a user running VS 2015 14.0.23107.0 (which is pre-Update 1). The above code is from VS 2015 14.0.25431.01 Update 3. This implementation of TeamExplorerContext wouldn't crash if ActiveRepositories did come back null.

@meaghanlewis Did some tests using VS 2015 14.0.23107.0 RTM. It appears that the IGitExt service is available as a global service even on this early version. I'm confused about how the service could come up null. I wonder if the IServiceProvider used by the Team Explorer UI might have had a bug that made it sometimes erroneously return null? 😕

Fixes #1408
Fixes #1421

Also fixes issue when IsPullRequestFromRepository is called on a local repository with no remote URL.

This service is used to access IGitExt from Visual Studio 2015 and 2017.

ActiveRepository should only be set to null when moving to a solution that isn't in Git. Avoid issue with multiple loads and unloads.

Converted to use IPullRequestSessionManager property change event.

Create a unit test project for GitHub.App.

This fixes an issue where a PR is checked out causing the PR list to reset.

Reduce logging verbosity.

grokys

Looking great in general! A few things to fix up however.

grokys · 2018-01-19T11:26:45Z

src/GitHub.App/Services/TeamExplorerContext.cs

+            this.log = log;
+            this.testing = testing;
+
+            gitExtType = gitExtType ?? FindGitExtType();


Why interface with IGitExt via reflection? Why not just import the type? It would mean having a dependency from GitHub.App to Microsoft.TeamFoundation.Git.Provider but I don't see that being a problem?

The problem is there are two versions of this assembly with no binding redirections between them. I wanted to avoid dependencies on GitHub.TeamFoundation.14/15.

Ahhh, ok yeah that makes more sense then. Maybe add a comment to this effect to the source.

grokys · 2018-01-19T11:28:34Z

src/GitHub.App/Services/TeamExplorerContext.cs

+        bool testing;
+
+        [ImportingConstructor]
+        public TeamExplorerContext([Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider)


Most other services depend on IGitHubServiceProvider to get their services - this gives you typed GetService/TryGetService methods .

grokys · 2018-01-19T11:32:17Z

src/GitHub.App/Services/TeamExplorerContext.cs

+            Type gitExtType, bool testing)
+        {
+            this.log = log;
+            this.testing = testing;


Other parts of the code use splat for detecting if we're in a unit test runner, either via Splat.ModeDetector.InUnitTestRunner() or Guard.InUnitTestRunner.

grokys · 2018-01-19T11:41:25Z

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

+                    await ThreadingHelper.SwitchToMainThreadAsync();
+                    await UpdateContent(teamExplorerContext.ActiveRepository);
+                }
+            };


You can subscribe to property changed subscriptions using .WhenAnyValue, so the above block could be reduced to:

await UpdateContent(teamExplorerContext.ActiveRepository); teamExplorerContext.WhenAnyValue(x => x.ActiveRepository) .Skip(1) .ObserveOn(RxApp.MainThreadScheduler) .Subscribe(x => UpdateContent(x).Forget());

WhenAnyValue immediately fires with the current value of the property, but we ignore this value, instead calling UpdateContent manually so we can await it. For subsequent changes, we fire and forget.

grokys · 2018-01-19T11:44:31Z

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

                WebUrl = LocalRepository.CloneUrl.ToRepositoryUrl().Append("pull/" + number);
                modelService = await modelServiceFactory.CreateAsync(connection);
-                vsGitExt.ActiveRepositoriesChanged += ActiveRepositoriesChanged;
+                sessionManager.PropertyChanged += ActiveRepositoriesChanged;


Similarly use sessionManager.WhenAnyValue(x => x.CurrentSession) to listen to the current session being changed.

grokys · 2018-01-19T11:48:10Z

src/GitHub.Exports/Services/ITeamExplorerContext.cs

+namespace GitHub.Services
+{
+    /// <summary>
+    /// Responsible for watching the active repository in Team Explorer.


Nit: the summary should typically be a single sentence summary of what the class does - the details should be moved to <remarks>, e.g.:

/// <summary> /// Tracks the active repository in Team Explorer. /// </summary> /// <remarks> /// The <see cref="PropertyChanged"/> event is fired for <see cref="ActiveRepository"/> when /// moving to a new repository. The <see cref="StatusChanged"/> event is fired when the /// CurrentBranch or HeadSha changes. /// </remarks>

grokys · 2018-01-19T11:53:02Z

src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs

+            {
+                RepoChanged(teamExplorerContext.ActiveRepository).Forget();
+            };
+            teamExplorerContext.PropertyChanged += (s, e) =>


Again, you can use WhenAnyValue here.

grokys · 2018-01-19T11:56:24Z

src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs

+            teamExplorerContext.StatusChanged += (s, e) =>
+            {
+                RepoChanged(teamExplorerContext.ActiveRepository).Forget();
+            };


If you wanted to be reactive here you could use:

Observable.FromEventPattern(teamExplorerContext, nameof(teamExplorerContext.StatusChanged)) .StartWith((EventPattern<object>)null) .Subscribe(_ => RepoChanged(teamExplorerContext.ActiveRepository).Forget());

The StartWith operator makes sure that the observable is fired immediately, making the initial RepoChanged(teamExplorerContext.ActiveRepository).Forget(); unnecessary. It's not particularly more readable or succinct to do it like this though, so I'm ambivalent.

It's good to know what's possible. :-)

I'm going with Rx for the property change and regular event for this one.

grokys · 2018-01-19T11:58:13Z

test/GitHub.App.UnitTests/GitHub.App.UnitTests.csproj

@@ -0,0 +1,94 @@
+<?xml version="1.0" encoding="utf-8"?>


I'm not sure about creating this test assembly now with a single test class in it... I know we plan to move everything over at some point, but if that doesn't happen for some reason we've now got this outlier... I think I'd prefer to put this test with the others for now.

I wanted to avoid treading on the work @meaghanlewis has been doing when we merge. Maybe we could merge the unit test refactoring first and then move this test into the new NUnit UnitTests project?

grokys · 2018-01-19T12:04:49Z

test/UnitTests/GitHub.App/ViewModels/GitHubPane/GitHubPaneViewModelTests.cs

-            var target = CreateTarget(teServiceHolder: te);
+            var te = Substitute.For<ITeamExplorerContext>();
+            te.ActiveRepository.Returns(default(ILocalRepositoryModel));
+            var target = CreateTarget(teamExplorerContext: te);


Now that we're using a properly mockable class for the team explorer stuff, we can probably simplify these tests...

What did you have in mind?

A lot of tests are creating a mock ITeamExplorerContext that returns an ActiveRepository of null. Could this could be made the default in CreateTarget if no mock is passed in?

The VSUIContextChangedEventArgs class can be used directly and doesn't need an interface.

We're exporting the IVSUIContextFactory service not IVSUIContext.

TeamExplorerServiceHolder no longer watches for UIContextChanged events and refreshes VSGitExt.

shana · 2018-01-26T13:24:42Z

src/GitHub.App/Services/TeamExplorerContext.cs

+        {
+            try
+            {
+                var repo = gitExt.ActiveRepositories?.FirstOrDefault();


This needs to be done on a background thread, see TeamExplorerServiceHolder for how it does this.

I've fixed it so that ActiveRepositories is initialized by VSGitExt on a thread pool thread.

shana · 2018-01-26T13:47:11Z

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs


+        void ContextChanged(object sender, VSUIContextChangedEventArgs e)
+        {
+            // If we're in the GitSccProvider context and TryInitialize succeeds, we can stop listening for events.


We also need to listen for the context to be deactivated, and throw away the instance of GitExt if that happens, and get it back when it activates again.

I've done a bunch of experiments changing to and from solutions in TFS repos. Here are some findings:

When changing to a TFS repo, there can be two UIContext events, first with IsActive=false then with IsActive=true

When changing back to a Git repo there are (often?) no events fired

The IGitExt service will only be null when directly opening a TFS solution

Once set the returned IGitExt service object will remain constant not return

This is consistent with the Microsoft.TeamFoundation.Git.Provider.SccExtensibilityService being delay loaded but never unloaded.

Given that the context is deactivated and then activated again when opening a TFS based solution, I'm reluctant to do anything to the IGitExt object when this happens. We'd still be listening when in a TFS solution and continue to use the same object when returning to a Git based solution.

Does that make sense? I should update the comments to reflect this.

shana · 2018-01-26T14:03:00Z

src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs

+            this.gitService = gitService;
            syncContext = SynchronizationContext.Current;
+
+            UpdateActiveRepo();


This cannot happen here, it will call ActiveRepositories which is a potentially blocking call. It needs to be shuffled to a background thread.

I've shuffled the initial fetching of ActiveRepositories to a thread pool thread so this is now safe to call.

jcansdale · 2018-01-26T16:25:01Z

Should make sure this can't happen:

There's no reason for ActiveRepositories to return null rather than an empty collection.

Make sure ActiveRepositories isn't read during the construction of our service. ActiveRepositories is first initialized on ThreadPoolThread. It is later updated when the property changed event is fired.

shana · 2018-01-26T18:47:56Z

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs

+
+        void ContextChanged(object sender, VSUIContextChangedEventArgs e)
+        {
+            // If we're in the GitSccProvider context and TryInitialize succeeds, we can stop listening for events.


We need to continually listen for events so we stop using GitExt if the scc provider becomes deactivated.

Could you explain why this is? Isn't @jcansdale's analysis correct? Looking at the code in ILSpy I don't see how IGitExt can become null once we have an instance unless the package is unloaded (which I think only happens when VS closes?).

Ensure that ActiveRepositories is only read on thread pool thread or its own property change event.

Added LocalRepositoryModelFactory service (and interface) so we can check the correct repositorys are being created. Create repositories on thread pool rather than calling thread. Return empty list if there's an exception creating repositories.

shana · 2018-01-29T11:09:09Z

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs

+            catch (Exception e)
+            {
+                log.Error(e, "Error refreshing repositories");
+                ActiveRepositories = new ILocalRepositoryModel[0];


Shouldn't this be raising the ActiveRepositoriesChanged event as well? And if so, shouldn't this event be raised in the setter of the property directly?

Oops! Good catch. This is why I should have done it on the property itself!

Clear the list and fire repo changes event if there is an exception when reading or creating repositories (there shouldn't be).

There's now no reason not to return a IReadOnlyList like IGitExt.ActiveRepositories.

Keep the property change event pattern consistent.

shana · 2018-01-29T13:31:01Z

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs


        IGitExt gitService;
-        IEnumerable<ILocalRepositoryModel> activeRepositories;
+        IReadOnlyList<ILocalRepositoryModel> activeRepositories;


nice! was going to mention that and then forgot 👍

out of date

jcansdale added 8 commits January 16, 2018 17:28

Add TeamExplorerContext MEF service

b834336

This service is used to access IGitExt from Visual Studio 2015 and 2017.

Harden reflection code and add logging

4e9d1ec

Only fire StatusChanged on BranchName/HeadSha change

e374c36

Ignore when ActiveRepositories is empty

416ec35

Don't throw if CloneUrl is null

768eca1

Change PullRequestSessionManager to use ITeamExplorerContext

00051bb

Change PR session if Number or Owner changes

f06ff66

GetSession doesn't accept a null pullRequest

8079f17

jcansdale mentioned this pull request Jan 17, 2018

[Feature] Show current PR on status bar #1396

Merged

9 tasks

jcansdale changed the title ~~[WIP] Refresh PullRequestSessionManager.CurrentSession when PR branch changes~~ Refresh PullRequestSessionManager.CurrentSession when PR branch changes Jan 17, 2018

jcansdale requested a review from grokys January 17, 2018 19:17

jcansdale added 11 commits January 18, 2018 12:25

Set ActiveRepository to null when solution isn't in Git

d31d9a4

ActiveRepository should only be set to null when moving to a solution that isn't in Git. Avoid issue with multiple loads and unloads.

Remove IVSGitExt hack from PullRequestDetailViewModel

6fbfb0a

Converted to use IPullRequestSessionManager property change event.

Add tests for TeamExplorerContext

7c43171

Create a unit test project for GitHub.App.

Add tests for TeamExplorerContext.PropertyChanged event

4b67153

Add tests for TeamExplorerContext.StatusChanged event

cb8c1b6

Change GitHubPaneViewModel to use ITeamExplorerContext

034d09b

This fixes an issue where a PR is checked out causing the PR list to reset.

Remove PullRequestDetailViewModel's dependency on IVSGitExt

03a41e7

Fix CA error

360ba9a

Add some docs for ITeamExplorerContext

c8dd2f6

Don't log when unit testing

b119585

Log exceptions in Refresh event

834f39d

Reduce logging verbosity.

grokys previously requested changes Jan 19, 2018

View reviewed changes

jcansdale added 7 commits January 19, 2018 12:32

Use IGitHubServiceProvider instead of IServiceProvider

18eba88

Use Splat.ModeDetector.InUnitTestRunner to detect runner

be1b5b4

Use Rx for property change subscriptions

65d0be6

Format xmldoc comments

04452ef

Use Rx to marshal onto main thread

a35dca7

Add remarks about non-UI thread

c809b66

Merge branch 'master' into fixes/1408-change-branch-event-squashed

fd5b45f

jcansdale added 4 commits January 26, 2018 12:17

Remove IVSUIContextChangedEventArgs interface

a801953

The VSUIContextChangedEventArgs class can be used directly and doesn't need an interface.

Make IVSUIContextFactory filename consistent with service

79ea850

We're exporting the IVSUIContextFactory service not IVSUIContext.

Add logging for VSGitExt

2ccabb1

Update and add xmldoc comments

7e9405a

TeamExplorerServiceHolder no longer watches for UIContextChanged events and refreshes VSGitExt.

shana reviewed Jan 26, 2018

View reviewed changes

Ensure ActiveRepositories is read using ThreadPoolThread

77bd3b2

Make sure ActiveRepositories isn't read during the construction of our service. ActiveRepositories is first initialized on ThreadPoolThread. It is later updated when the property changed event is fired.

shana reviewed Jan 26, 2018

View reviewed changes

jcansdale added 4 commits January 26, 2018 18:51

Update comments on how we're using UIContext

dd39202

Cleanup usings

e2c51e8

Don't read ActiveRepositories on UIContext thread

edf9ff8

Ensure that ActiveRepositories is only read on thread pool thread or its own property change event.

shana reviewed Jan 29, 2018

View reviewed changes

jcansdale added 7 commits January 29, 2018 11:30

Ensure repo changed event fires even when exception

7ea9d81

Clear the list and fire repo changes event if there is an exception when reading or creating repositories (there shouldn't be).

Use IReadOnlyList and keep IVSGitExt consistent with IGitExt

a41dbc1

There's now no reason not to return a IReadOnlyList like IGitExt.ActiveRepositories.

Merge branch 'master' into fixes/1408-change-branch-event-squashed

b420d4b

Fix merge error

6a494e3

Only fire property change event if property has changed

a466295

Keep the property change event pattern consistent.

Clean up redundant usings

01c81b2

Update test to reflect no duplicate change events

7f8011e

shana reviewed Jan 29, 2018

View reviewed changes

shana approved these changes Jan 29, 2018

View reviewed changes

Merge branch 'master' into fixes/1408-change-branch-event-squashed

4995603

grokys approved these changes Jan 29, 2018

View reviewed changes

grokys merged commit 97e4570 into master Jan 29, 2018

grokys deleted the fixes/1408-change-branch-event-squashed branch March 5, 2018 17:28

donokuda mentioned this pull request Mar 6, 2018

WIP: Spike of Pull Request Reviews feature. #1415

Closed

Refresh PullRequestSessionManager.CurrentSession when PR branch changes #1422

Refresh PullRequestSessionManager.CurrentSession when PR branch changes #1422

Uh oh!

Conversation

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

Uh oh!

What's in this PR

The TeamExplorerContext service

Related

Notes

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!

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 Jan 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Jan 26, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

jcansdale commented Jan 17, 2018 •

edited

Loading

jcansdale Jan 26, 2018 •

edited

Loading

jcansdale commented Jan 26, 2018 •

edited

Loading

grokys Jan 26, 2018 •

edited

Loading