Merge pull request #1513 from github/refactor/simplify-VSGitExt

Simplify the VSGitExt service

Author: jcansdale

src/GitHub.Exports/Services/IVSUIContextFactory.cs

@@ -20,6 +20,6 @@ public VSUIContextChangedEventArgs(bool activated)
     public interface IVSUIContext
     {
         bool IsActive { get; }
-        event EventHandler<VSUIContextChangedEventArgs> UIContextChanged;
+        void WhenActivated(Action action);
     }
 }

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

@@ -24,8 +24,8 @@ public class VSGitExt : IVSGitExt
         static readonly ILogger log = LogManager.ForContext<VSGitExt>();
 
         readonly Func<Type, Task<object>> getServiceAsync;
-        readonly IVSUIContext context;
         readonly ILocalRepositoryModelFactory repositoryFactory;
+        readonly object refreshLock = new object();
 
         IGitExt gitService;
         IReadOnlyList<ILocalRepositoryModel> activeRepositories;
@@ -41,94 +41,50 @@ public VSGitExt(Func<Type, Task<object>> getServiceAsync, IVSUIContextFactory fa
             this.getServiceAsync = getServiceAsync;
             this.repositoryFactory = repositoryFactory;
 
-            // The IGitExt service isn't available when a TFS based solution is opened directly.
-            // It will become available when moving to a Git based solution (and cause a UIContext event to fire).
-            context = factory.GetUIContext(new Guid(Guids.GitSccProviderId));
-
             // Start with empty array until we have a chance to initialize.
             ActiveRepositories = Array.Empty<ILocalRepositoryModel>();
 
-            PendingTasks = InitializeAsync();
-        }
-
-        async Task InitializeAsync()
-        {
-            try
-            {
-                if (!context.IsActive || !await TryInitialize())
-                {
-                    // If we're not in the UIContext or TryInitialize fails, have another go when the UIContext changes.
-                    context.UIContextChanged += ContextChanged;
-                    log.Debug("VSGitExt will be initialized later");
-                }
-            }
-            catch (Exception e)
-            {
-                log.Error(e, "Initializing");
-            }
-        }
-
-        void ContextChanged(object sender, VSUIContextChangedEventArgs e)
-        {
-            if (e.Activated)
-            {
-                PendingTasks = ContextChangedAsync();
-            }
+            // The IGitExt service isn't available when a TFS based solution is opened directly.
+            // It will become available when moving to a Git based solution (and cause a UIContext event to fire).
+            var context = factory.GetUIContext(new Guid(Guids.GitSccProviderId));
+            context.WhenActivated(() => Initialize());
         }
 
-        async Task ContextChangedAsync()
+        void Initialize()
         {
-            try
+            PendingTasks = getServiceAsync(typeof(IGitExt)).ContinueWith(t =>
             {
-                // If we're in the UIContext and TryInitialize succeeds, we can stop listening for events.
-                // NOTE: this event can fire with UIContext=true in a TFS solution (not just Git).
-                if (await TryInitialize())
+                gitService = (IGitExt)t.Result;
+                if (gitService == null)
                 {
-                    context.UIContextChanged -= ContextChanged;
-                    log.Debug("Initialized VSGitExt on UIContextChanged");
+                    log.Error("Couldn't find IGitExt service");
+                    return;
                 }
-            }
-            catch (Exception e)
-            {
-                log.Error(e, "UIContextChanged");
-            }
-        }
 
-        async Task<bool> TryInitialize()
-        {
-            gitService = (IGitExt)await getServiceAsync(typeof(IGitExt));
-            if (gitService != null)
-            {
+                RefreshActiveRepositories();
                 gitService.PropertyChanged += (s, e) =>
                 {
                     if (e.PropertyName == nameof(gitService.ActiveRepositories))
                     {
-                        // Execute tasks in sequence using thread pool (TaskScheduler.Default).
-                        PendingTasks = PendingTasks.ContinueWith(_ => RefreshActiveRepositories(), TaskScheduler.Default);
+                        RefreshActiveRepositories();
                     }
                 };
-
-                // Do this after we start listening so we don't miss an event.
-                await Task.Run(() => RefreshActiveRepositories());
-
-                log.Debug("Found IGitExt service and initialized VSGitExt");
-                return true;
-            }
-
-            log.Error("Couldn't find IGitExt service");
-            return false;
+            }, TaskScheduler.Default);
         }
 
         void RefreshActiveRepositories()
         {
             try
             {
-                log.Debug(
-                    "IGitExt.ActiveRepositories (#{Id}) returned {Repositories}",
-                    gitService.GetHashCode(),
-                    gitService?.ActiveRepositories.Select(x => x.RepositoryPath));
+                lock (refreshLock)
+                {
+                    log.Debug(
+                        "IGitExt.ActiveRepositories (#{Id}) returned {Repositories}",
+                        gitService.GetHashCode(),
+                        gitService.ActiveRepositories.Select(x => x.RepositoryPath));
 
-                ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList();
+                    ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList();
+                }
             }
             catch (Exception e)
             {
@@ -160,6 +116,6 @@ private set
         /// <summary>
         /// Tasks that are pending execution on the thread pool.
         /// </summary>
-        public Task PendingTasks { get; private set; }
+        public Task PendingTasks { get; private set; } = Task.CompletedTask;
     }
 }

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

@@ -1,6 +1,4 @@
 using System;
-using System.Collections.Generic;
-using System.ComponentModel.Composition;
 using Microsoft.VisualStudio.Shell;
 using GitHub.Services;
 
@@ -17,36 +15,14 @@ public IVSUIContext GetUIContext(Guid contextGuid)
     class VSUIContext : IVSUIContext
     {
         readonly UIContext context;
-        readonly Dictionary<EventHandler<VSUIContextChangedEventArgs>, EventHandler<UIContextChangedEventArgs>> handlers =
-            new Dictionary<EventHandler<VSUIContextChangedEventArgs>, EventHandler<UIContextChangedEventArgs>>();
+
         public VSUIContext(UIContext context)
         {
             this.context = context;
         }
 
         public bool IsActive { get { return context.IsActive; } }
 
-        public event EventHandler<VSUIContextChangedEventArgs> UIContextChanged
-        {
-            add
-            {
-                EventHandler<UIContextChangedEventArgs> handler = null;
-                if (!handlers.TryGetValue(value, out handler))
-                {
-                    handler = (s, e) => value.Invoke(s, new VSUIContextChangedEventArgs(e.Activated));
-                    handlers.Add(value, handler);
-                }
-                context.UIContextChanged += handler;
-            }
-            remove
-            {
-                EventHandler<UIContextChangedEventArgs> handler = null;
-                if (handlers.TryGetValue(value, out handler))
-                {
-                    handlers.Remove(value);
-                    context.UIContextChanged -= handler;
-                }
-            }
-        }
+        public void WhenActivated(Action action) => context.WhenActivated(action);
     }
 }

test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs

@@ -36,9 +36,9 @@ public void GetServiceIGitExt_WhenUIContextChanged(bool activated, int expectCal
             var sp = Substitute.For<IAsyncServiceProvider>();
             var target = CreateVSGitExt(context, sp: sp);
 
-            var eventArgs = new VSUIContextChangedEventArgs(activated);
-            context.UIContextChanged += Raise.Event<EventHandler<VSUIContextChangedEventArgs>>(context, eventArgs);
+            context.IsActive = activated;
 
+            target.PendingTasks.Wait();
             sp.Received(expectCalls).GetServiceAsync(typeof(IGitExt));
         }
 
@@ -106,8 +106,7 @@ public void WhenUIContextChanged_ActiveRepositoriesChangedIsFired()
             bool wasFired = false;
             target.ActiveRepositoriesChanged += () => wasFired = true;
 
-            var eventArgs = new VSUIContextChangedEventArgs(true);
-            context.UIContextChanged += Raise.Event<EventHandler<VSUIContextChangedEventArgs>>(context, eventArgs);
+            context.IsActive = true;
             target.PendingTasks.Wait();
 
             Assert.That(wasFired, Is.True);
@@ -123,8 +122,7 @@ public void WhenUIContextChanged_FiredUsingThreadPoolThread()
             bool threadPool = false;
             target.ActiveRepositoriesChanged += () => threadPool = Thread.CurrentThread.IsThreadPoolThread;
 
-            var eventArgs = new VSUIContextChangedEventArgs(true);
-            context.UIContextChanged += Raise.Event<EventHandler<VSUIContextChangedEventArgs>>(context, eventArgs);
+            context.IsActive = true;
             target.PendingTasks.Wait();
 
             Assert.That(threadPool, Is.True);
@@ -236,11 +234,40 @@ static IGitExt CreateGitExt(params string[] repositoryPaths)
         return gitExt;
     }
 
-    static IVSUIContext CreateVSUIContext(bool isActive)
+    static MockVSUIContext CreateVSUIContext(bool isActive)
     {
-        var context = Substitute.For<IVSUIContext>();
-        context.IsActive.Returns(isActive);
-        return context;
+        return new MockVSUIContext { IsActive = isActive };
+    }
+
+    class MockVSUIContext : IVSUIContext
+    {
+        bool isActive;
+        Action action;
+
+        public bool IsActive
+        {
+            get { return isActive; }
+            set
+            {
+                isActive = value;
+                if (isActive && action != null)
+                {
+                    action.Invoke();
+                    action = null;
+                }
+            }
+        }
+
+        public void WhenActivated(Action action)
+        {
+            if (isActive)
+            {
+                action.Invoke();
+                return;
+            }
+
+            this.action = action;
+        }
     }
 
     class MockGitExt : IGitExt