From ee9ab4af7f71ad9c604a4013ba7b87836a5cffb8 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 2 Mar 2018 16:59:36 +0000 Subject: [PATCH] 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 --- .../Services/IVSUIContextFactory.cs | 2 +- .../Services/VSGitExt.cs | 90 +++++-------------- .../Services/VSUIContextFactory.cs | 28 +----- .../GitHub.TeamFoundation/VSGitExtTests.cs | 47 +++++++--- 4 files changed, 63 insertions(+), 104 deletions(-) diff --git a/src/GitHub.Exports/Services/IVSUIContextFactory.cs b/src/GitHub.Exports/Services/IVSUIContextFactory.cs index e3aca6cbb7..57c6542d9b 100644 --- a/src/GitHub.Exports/Services/IVSUIContextFactory.cs +++ b/src/GitHub.Exports/Services/IVSUIContextFactory.cs @@ -20,6 +20,6 @@ public VSUIContextChangedEventArgs(bool activated) public interface IVSUIContext { bool IsActive { get; } - event EventHandler UIContextChanged; + void WhenActivated(Action action); } } \ No newline at end of file diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index 0c0b6f3376..5626ad4cd1 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -24,8 +24,8 @@ public class VSGitExt : IVSGitExt static readonly ILogger log = LogManager.ForContext(); readonly Func> getServiceAsync; - readonly IVSUIContext context; readonly ILocalRepositoryModelFactory repositoryFactory; + readonly object refreshLock = new object(); IGitExt gitService; IReadOnlyList activeRepositories; @@ -41,94 +41,50 @@ public VSGitExt(Func> 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(); - 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 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 /// /// Tasks that are pending execution on the thread pool. /// - public Task PendingTasks { get; private set; } + public Task PendingTasks { get; private set; } = Task.CompletedTask; } } \ No newline at end of file diff --git a/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs b/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs index 6cedf85cd8..3797f75c02 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSUIContextFactory.cs +++ b/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,8 +15,7 @@ public IVSUIContext GetUIContext(Guid contextGuid) class VSUIContext : IVSUIContext { readonly UIContext context; - readonly Dictionary, EventHandler> handlers = - new Dictionary, EventHandler>(); + public VSUIContext(UIContext context) { this.context = context; @@ -26,27 +23,6 @@ public VSUIContext(UIContext context) public bool IsActive { get { return context.IsActive; } } - public event EventHandler UIContextChanged - { - add - { - EventHandler 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 handler = null; - if (handlers.TryGetValue(value, out handler)) - { - handlers.Remove(value); - context.UIContextChanged -= handler; - } - } - } + public void WhenActivated(Action action) => context.WhenActivated(action); } } diff --git a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs index e1007ece40..daa0dbd037 100644 --- a/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs +++ b/test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs @@ -36,9 +36,9 @@ public void GetServiceIGitExt_WhenUIContextChanged(bool activated, int expectCal var sp = Substitute.For(); var target = CreateVSGitExt(context, sp: sp); - var eventArgs = new VSUIContextChangedEventArgs(activated); - context.UIContextChanged += Raise.Event>(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>(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>(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(); - 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