From e5fbb8466e2ea077a92d6ff96e1e5a34e9397fc7 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Tue, 15 Sep 2015 18:43:28 +0200 Subject: [PATCH 1/4] Clear publish form properly, part I --- .../TeamExplorer/Sync/GitHubPublishSection.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs b/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs index 18e429571f..8262f87a71 100644 --- a/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs +++ b/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs @@ -13,6 +13,8 @@ using GitHub.Extensions; using GitHub.Api; using GitHub.VisualStudio.TeamExplorer; +using System.Reactive.Disposables; +using System.Windows.Controls; namespace GitHub.VisualStudio.TeamExplorer.Sync { @@ -24,7 +26,7 @@ public class GitHubPublishSection : TeamExplorerSectionBase, IGitHubInvitationSe readonly Lazy lazyBrowser; readonly IRepositoryHosts hosts; - IDisposable disposable; + readonly CompositeDisposable disposables = new CompositeDisposable(); bool loggedIn; [ImportingConstructor] @@ -132,7 +134,7 @@ void ShowPublish() var uiProvider = ServiceProvider.GetExportedValue(); var factory = uiProvider.GetService(); var uiflow = factory.UIControllerFactory.CreateExport(); - disposable = uiflow; + disposables.Add(uiflow); var ui = uiflow.Value; var creation = ui.SelectFlow(UIControllerFlow.Publish); creation.Subscribe(c => @@ -151,8 +153,7 @@ protected override void Dispose(bool disposing) { if (!disposed) { - if (disposable != null) - disposable.Dispose(); + disposables.Dispose(); disposed = true; } } From 25811edb61c1df1cc59bdc9ae39f1101968c9a6d Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Tue, 15 Sep 2015 18:44:26 +0200 Subject: [PATCH 2/4] Clean unused code and add helpful comments --- .../TeamExplorer/Sync/GitHubPublishSection.cs | 28 +++++-------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs b/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs index 8262f87a71..d6f01f9679 100644 --- a/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs +++ b/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs @@ -52,7 +52,7 @@ public GitHubPublishSection(ISimpleApiClientFactory apiFactory, ITeamExplorerSer view.DataContext = this; } - async void RTMSetup() + async void Setup() { if (ActiveRepo != null && ActiveRepoUri == null) { @@ -66,39 +66,23 @@ async void RTMSetup() IsVisible = false; } - async void PreRTMSetup() - { - if (ActiveRepo != null && ActiveRepoUri == null) - { - IsVisible = true; - loggedIn = await connectionManager.IsLoggedIn(hosts); - if (loggedIn) - ShowPublish(); - else - { - ShowGetStarted = true; - ShowSignup = true; - } - } - else - IsVisible = false; - } - public override void Initialize(object sender, SectionInitializeEventArgs e) { base.Initialize(sender, e); - RTMSetup(); + Setup(); } protected override void RepoChanged() { base.RepoChanged(); - RTMSetup(); + Setup(); } public async void Connect() { loggedIn = await connectionManager.IsLoggedIn(hosts); + // we run the login on a separate UI flow because the login + // dialog is a modal dialog while the publish dialog is inlined in Team Explorer if (loggedIn) ShowPublish(); else @@ -130,7 +114,9 @@ void StartFlow(UIControllerFlow controllerFlow) void ShowPublish() { + // set the loading indicator while we prep the form IsBusy = true; + var uiProvider = ServiceProvider.GetExportedValue(); var factory = uiProvider.GetService(); var uiflow = factory.UIControllerFactory.CreateExport(); From c108c777c326e64afdfa365ef5afc95149ec472d Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Tue, 15 Sep 2015 18:50:40 +0200 Subject: [PATCH 3/4] Dismiss publish form when done and handle errors UI errors were getting shown directly from the underlying view model, fix that. Make sure everything about the publish dialog gets disposed once it's done, and hide it once it's done. --- .../ViewModels/RepositoryPublishViewModel.cs | 12 ++---- src/GitHub.Exports/UI/IView.cs | 1 + .../Controls/SimpleViewUserControl.cs | 10 +++++ .../TeamExplorer/Sync/GitHubPublishSection.cs | 39 +++++++++++++++++-- .../Controls/RepositoryPublishControl.xaml.cs | 13 +++++-- 5 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs b/src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs index df50a548c4..665918f114 100644 --- a/src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs +++ b/src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs @@ -151,23 +151,19 @@ ReactiveCommand InitializePublishRepositoryCommand() return ReactiveCommand.CreateAsyncObservable(canCreate, OnPublishRepository); } - private IObservable OnPublishRepository(object arg) + IObservable OnPublishRepository(object arg) { var newRepository = GatherRepositoryInfo(); var account = SelectedAccount; return repositoryPublishService.PublishRepository(newRepository, account, SelectedHost.ApiClient) .SelectUnit() - .Do(_ => vsServices.ShowMessage("Repository published successfully.")) .Catch(ex => { + log.Error(ex); if (!ex.IsCriticalException()) - { - log.Error(ex); - var error = new PublishRepositoryUserError(ex.Message); - vsServices.ShowError((error.ErrorMessage + Environment.NewLine + error.ErrorCauseOrResolution).TrimEnd()); - } - return Observable.Return(Unit.Default); + ex = new UnhandledUserErrorException(new PublishRepositoryUserError(ex.Message)); + return Observable.Throw(ex); }); } diff --git a/src/GitHub.Exports/UI/IView.cs b/src/GitHub.Exports/UI/IView.cs index 5c7697e755..3320088d03 100644 --- a/src/GitHub.Exports/UI/IView.cs +++ b/src/GitHub.Exports/UI/IView.cs @@ -8,5 +8,6 @@ public interface IView IObservable Done { get; } IObservable Cancel { get; } IObservable IsBusy { get; } + IObservable Error { get; } } } diff --git a/src/GitHub.UI.Reactive/Controls/SimpleViewUserControl.cs b/src/GitHub.UI.Reactive/Controls/SimpleViewUserControl.cs index 8bb5b79ee8..afc1977cb2 100644 --- a/src/GitHub.UI.Reactive/Controls/SimpleViewUserControl.cs +++ b/src/GitHub.UI.Reactive/Controls/SimpleViewUserControl.cs @@ -18,6 +18,7 @@ public class SimpleViewUserControl : UserControl, IDisposable, IActivatable { readonly Subject close = new Subject(); readonly Subject cancel = new Subject(); + readonly Subject error = new Subject(); readonly Subject isBusy = new Subject(); public SimpleViewUserControl() @@ -39,6 +40,8 @@ public SimpleViewUserControl() public IObservable Cancel { get { return cancel; } } + public IObservable Error { get { return error; } } + public IObservable IsBusy{ get { return isBusy; } } protected void NotifyDone() @@ -53,6 +56,12 @@ protected void NotifyCancel() cancel.OnCompleted(); } + protected void NotifyError(string message) + { + error.OnNext(message); + error.OnCompleted(); + } + protected void NotifyIsBusy(bool busy) { isBusy.OnNext(busy); @@ -66,6 +75,7 @@ protected virtual void Dispose(bool disposing) if (disposed) return; close.Dispose(); + error.Dispose(); disposed = true; } } diff --git a/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs b/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs index d6f01f9679..e6df0dfc8b 100644 --- a/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs +++ b/src/GitHub.VisualStudio/TeamExplorer/Sync/GitHubPublishSection.cs @@ -8,11 +8,9 @@ using GitHub.Models; using GitHub.Services; using GitHub.Info; -using ReactiveUI; using System.Reactive.Linq; using GitHub.Extensions; using GitHub.Api; -using GitHub.VisualStudio.TeamExplorer; using System.Reactive.Disposables; using System.Windows.Controls; @@ -28,6 +26,7 @@ public class GitHubPublishSection : TeamExplorerSectionBase, IGitHubInvitationSe readonly IRepositoryHosts hosts; readonly CompositeDisposable disposables = new CompositeDisposable(); bool loggedIn; + readonly UserControl view; [ImportingConstructor] public GitHubPublishSection(ISimpleApiClientFactory apiFactory, ITeamExplorerServiceHolder holder, @@ -47,7 +46,7 @@ public GitHubPublishSection(ISimpleApiClientFactory apiFactory, ITeamExplorerSer ShowGetStarted = false; IsVisible = false; IsExpanded = true; - var view = new GitHubInvitationContent(); + view = new GitHubInvitationContent(); SectionContent = view; view.DataContext = this; } @@ -123,11 +122,43 @@ void ShowPublish() disposables.Add(uiflow); var ui = uiflow.Value; var creation = ui.SelectFlow(UIControllerFlow.Publish); + var busyTracker = new SerialDisposable(); creation.Subscribe(c => { SectionContent = c; c.DataContext = this; - ((IView)c).IsBusy.Subscribe(x => IsBusy = x); + + var v = (IView)c; + busyTracker.Disposable = v.IsBusy.Subscribe(x => IsBusy = x); + disposables.Add(v.Error.Subscribe(x => + { + var vsServices = ServiceProvider.GetExportedValue(); + vsServices.ShowError(x as string); + })); + }, + () => + { + var vsServices = ServiceProvider.GetExportedValue(); + vsServices.ShowMessage("Repository published successfully."); + + var v = SectionContent as IView; + // the IsPublishing flag takes way too long to fire, don't wait for it, we're done + if (IsBusy) + { + // we only want to dispose things when all the events are processed (IsPublishing is the last one) + busyTracker.Disposable = v.IsBusy.Subscribe(_ => + { + disposables.Clear(); + busyTracker.Dispose(); + }); + IsBusy = false; + } + else + { + busyTracker.Dispose(); + disposables.Clear(); + } + SectionContent = view; }); ui.Start(null); } diff --git a/src/GitHub.VisualStudio/UI/Views/Controls/RepositoryPublishControl.xaml.cs b/src/GitHub.VisualStudio/UI/Views/Controls/RepositoryPublishControl.xaml.cs index 84249b2f60..74da35a998 100644 --- a/src/GitHub.VisualStudio/UI/Views/Controls/RepositoryPublishControl.xaml.cs +++ b/src/GitHub.VisualStudio/UI/Views/Controls/RepositoryPublishControl.xaml.cs @@ -42,10 +42,17 @@ public RepositoryPublishControl() d(this.OneWayBind(ViewModel, vm => vm.IsPublishing, v => v.description.IsEnabled, x => x == false)); d(this.OneWayBind(ViewModel, vm => vm.IsPublishing, v => v.accountsComboBox.IsEnabled, x => x == false)); - ViewModel.PublishRepository.Subscribe(_ => NotifyDone()); + d(ViewModel.PublishRepository.Subscribe(_ => NotifyDone())); + d(ViewModel.PublishRepository.ThrownExceptions.Subscribe(ex => + { + var error = ex as UnhandledUserErrorException; + if (error == null) + NotifyError("Error publishing repository." + Environment.NewLine + ex.Message); + else + NotifyError((error.ReportedError.ErrorMessage + Environment.NewLine + error.ReportedError.ErrorCauseOrResolution).TrimEnd()); + })); - d(this.WhenAny(x => x.ViewModel.IsPublishing, x => x.Value) - .Subscribe(x => NotifyIsBusy(x))); + d(this.WhenAny(x => x.ViewModel.IsPublishing, x => x.Value).Subscribe(x => NotifyIsBusy(x))); nameText.Text = ViewModel.DefaultRepositoryName; }); From 7c432c08b7a6f8952e38149a8b4f66ddcdf73125 Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Wed, 16 Sep 2015 13:39:04 +0200 Subject: [PATCH 4/4] Fix tests --- .../ViewModels/RepositoryPublishViewModelTests.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/UnitTests/GitHub.App/ViewModels/RepositoryPublishViewModelTests.cs b/src/UnitTests/GitHub.App/ViewModels/RepositoryPublishViewModelTests.cs index afc02776e0..979367b8bc 100644 --- a/src/UnitTests/GitHub.App/ViewModels/RepositoryPublishViewModelTests.cs +++ b/src/UnitTests/GitHub.App/ViewModels/RepositoryPublishViewModelTests.cs @@ -339,10 +339,10 @@ public async Task DisplaysSuccessMessageWhenCompletedWithoutError() vm.RepositoryName = "repo-name"; + var obs = Substitute.For>(); + vm.PublishRepository.ThrownExceptions.Subscribe(obs); await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(Unit.Default)); - - vsServices.Received().ShowMessage("Repository published successfully."); - vsServices.DidNotReceive().ShowError(Args.String); + obs.DidNotReceiveWithAnyArgs().OnNext(null); } [Fact] @@ -358,10 +358,11 @@ public async Task DisplaysRepositoryExistsErrorWithVisualStudioNotifications() var vm = Helpers.SetupConnectionsAndViewModel(hosts, repositoryPublishService, vsServices, cm); vm.RepositoryName = "repo-name"; + var obs = Substitute.For>(); + vm.PublishRepository.ThrownExceptions.Subscribe(obs); await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(Unit.Default)); - - vsServices.DidNotReceive().ShowMessage(Args.String); - vsServices.Received().ShowError("There is already a repository named 'repo-name' for the current account."); + obs.ReceivedWithAnyArgs().OnNext(null); + Assert.True(obs.ReceivedCalls().Any(x => ((UnhandledUserErrorException)x.GetArguments()[0]).Message == "There is already a repository named 'repo-name' for the current account.")); } [Fact] @@ -391,6 +392,7 @@ public async Task ClearsErrorsWhenSwitchingHosts() vm.RepositoryName = "repo-name"; + vm.PublishRepository.ThrownExceptions.Subscribe(); await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(Unit.Default)); vm.SelectedConnection = conns.First(x => x != vm.SelectedConnection); @@ -422,6 +424,7 @@ public async Task ClearsErrorsWhenSwitchingAccounts() vm.RepositoryName = "repo-name"; + vm.PublishRepository.ThrownExceptions.Subscribe(); await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(Unit.Default)); vm.SelectedAccount = accounts[1];