Merge pull request #218 from github/fixes/203-message-bad

Fix race condition when finishing ui flows (PR #209 retargeting to master)

Author: shana

src/GitHub.App/Controllers/UIController.cs

@@ -153,10 +153,9 @@ public IObservable<bool> ListenToCompletionState()
         void End(bool success)
         {
             uiProvider.RemoveService(typeof(IConnection));
-            transition.OnCompleted();
             completion?.OnNext(success);
             completion?.OnCompleted();
-            completion = null;
+            transition.OnCompleted();
         }
 
         void RunView(UIViewType viewType)

src/GitHub.Exports/Services/IUIProvider.cs

@@ -24,5 +24,6 @@ public interface IUIProvider : IServiceProvider
         IObservable<UserControl> SetupUI(UIControllerFlow controllerFlow, IConnection connection);
         void RunUI();
         void RunUI(UIControllerFlow controllerFlow, IConnection connection);
+        IObservable<bool> ListenToCompletionState();
     }
 }

src/GitHub.VisualStudio/Services/UIProvider.cs

@@ -220,6 +220,19 @@ public IObservable<UserControl> SetupUI(UIControllerFlow controllerFlow, [AllowN
             return creation;
         }
 
+        public IObservable<bool> ListenToCompletionState()
+        {
+            var ui = currentUIFlow?.Value;
+            if (ui == null)
+            {
+                log.Error("UIProvider:ListenToCompletionState:Cannot call ListenToCompletionState without calling SetupUI first");
+#if DEBUG
+                throw new InvalidOperationException("Cannot call ListenToCompletionState without calling SetupUI first");
+#endif
+            }
+            return ui?.ListenToCompletionState() ?? Observable.Return(false);
+        }
+
         public void RunUI()
         {
             if (!Initialized)

src/GitHub.VisualStudio/TeamExplorer/Connect/GitHubConnectSection.cs

@@ -330,14 +330,18 @@ void StartFlow(UIControllerFlow controllerFlow)
 
             var uiProvider = ServiceProvider.GetExportedValue<IUIProvider>();
             uiProvider.GitServiceProvider = ServiceProvider;
-            var ret = uiProvider.SetupUI(controllerFlow, SectionConnection);
-            ret.Subscribe(_ => {}, () =>
-            {
-                if (controllerFlow == UIControllerFlow.Clone)
-                    isCloning = true;
-                else if (controllerFlow == UIControllerFlow.Create)
-                    isCreating = true;
-            });
+            uiProvider.SetupUI(controllerFlow, SectionConnection);
+            uiProvider.ListenToCompletionState()
+                .Subscribe(success =>
+                {
+                    if (success)
+                    {
+                        if (controllerFlow == UIControllerFlow.Clone)
+                            isCloning = true;
+                        else if (controllerFlow == UIControllerFlow.Create)
+                            isCreating = true;
+                    }
+                });
             uiProvider.RunUI();
 
             notifications.RemoveListener();

src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs

@@ -118,19 +118,20 @@ void ShowPublish()
             var ui = uiflow.Value;
             var creation = ui.SelectFlow(UIControllerFlow.Publish);
             bool success = false;
-            ui.ListenToCompletionState().Subscribe(s => success = s, () =>
-            {
-                // there's no real cancel button in the publish form, but if support a back button there, then we want to hide the form
-                IsVisible = false;
-                if (success)
-                    ServiceProvider.TryGetService<ITeamExplorer>()?.NavigateToPage(new Guid(TeamExplorerPageIds.Home), null);
-            });
+            ui.ListenToCompletionState().Subscribe(s => success = s);
 
             creation.Subscribe(c =>
             {
                 SectionContent = c;
                 c.DataContext = this;
                 ((IView)c).IsBusy.Subscribe(x => IsBusy = x);
+            },
+            () =>
+            {
+                // there's no real cancel button in the publish form, but if support a back button there, then we want to hide the form
+                IsVisible = false;
+                if (success)
+                    ServiceProvider.TryGetService<ITeamExplorer>()?.NavigateToPage(new Guid(TeamExplorerPageIds.Home), null);
             });
             ui.Start(null);
         }
@@ -142,9 +143,8 @@ protected override void Dispose(bool disposing)
             {
                 if (!disposed)
                 {
-                    if (disposable != null)
-                        disposable.Dispose();
                     disposed = true;
+                    disposable?.Dispose();
                 }
             }
             base.Dispose(disposing);

src/UnitTests/GitHub.App/Controllers/UIProviderTests.cs

@@ -0,0 +1,38 @@
+using System;
+using System.ComponentModel.Composition;
+using System.Reactive.Linq;
+using System.Windows.Controls;
+using GitHub.Controllers;
+using GitHub.Models;
+using GitHub.Services;
+using GitHub.UI;
+using NSubstitute;
+using Xunit;
+using UnitTests;
+using GitHub.ViewModels;
+using ReactiveUI;
+using System.Collections.Generic;
+using GitHub.Authentication;
+using System.Collections.ObjectModel;
+using GitHub.VisualStudio;
+
+public class UIProviderTests : TestBaseClass
+{
+    [Fact]
+    public void ListenToCompletionDoesNotThrowInRelease()
+    {
+        var provider = Substitutes.GetFullyMockedServiceProvider();
+
+        using (var p = new UIProvider(provider))
+        {
+#if DEBUG
+            Assert.ThrowsAny<InvalidOperationException>(() =>
+            {
+#endif
+                p.ListenToCompletionState();
+#if DEBUG
+            });
+#endif
+        }
+    }
+}

src/UnitTests/UnitTests.csproj

@@ -139,6 +139,7 @@
     <Compile Include="GitHub.Api\SimpleApiClientTests.cs" />
     <Compile Include="GitHub.App\Caches\CredentialCacheTests.cs" />
     <Compile Include="GitHub.App\Caches\ImageCacheTests.cs" />
+    <Compile Include="GitHub.App\Controllers\UIProviderTests.cs" />
     <Compile Include="GitHub.App\Models\ModelServiceTests.cs" />
     <Compile Include="GitHub.App\Controllers\UIControllerTests.cs" />
     <Compile Include="GitHub.App\Models\PullRequestModelTests.cs" />