Merge pull request #934 from github/grokys/pr-details-busy

shana · web-flow · commit cfc8f6cdb3c5 · 2017-03-28T13:39:56.000+02:00
Display indeterminate progress bar when updating PR details view.
diff --git a/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs b/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Reactive;
 using System.Threading.Tasks;
@@ -27,6 +28,8 @@ public class PullRequestUpdateStateDesigner : IPullRequestUpdateState
     [ExcludeFromCodeCoverage]
     public class PullRequestDetailViewModelDesigner : PanePageViewModelBase, IPullRequestDetailViewModel
     {
+        private List<IPullRequestChangeNode> changedFilesTree;
+
         public PullRequestDetailViewModelDesigner()
         {
             var repoPath = @"C:\Repo";
@@ -63,18 +66,19 @@ public PullRequestDetailViewModelDesigner()
             modelsDir.Files.Add(oldBranchModel);
             gitHubDir.Directories.Add(modelsDir);
 
-            ChangedFilesTree = new ReactiveList<IPullRequestChangeNode>();
-            ChangedFilesTree.Add(gitHubDir);
+            changedFilesTree = new List<IPullRequestChangeNode>();
+            changedFilesTree.Add(gitHubDir);
         }
 
         public IPullRequestModel Model { get; }
         public string SourceBranchDisplayName { get; set; }
         public string TargetBranchDisplayName { get; set; }
+        public bool IsLoading { get; }
         public bool IsBusy { get; }
         public bool IsCheckedOut { get; }
         public bool IsFromFork { get; }
         public string Body { get; }
-        public IReactiveList<IPullRequestChangeNode> ChangedFilesTree { get; }
+        public IReadOnlyList<IPullRequestChangeNode> ChangedFilesTree => changedFilesTree;
         public IPullRequestCheckoutState CheckoutState { get; set; }
         public IPullRequestUpdateState UpdateState { get; set; }
         public string OperationError { get; set; }
diff --git a/src/GitHub.App/SampleData/SampleViewModels.cs b/src/GitHub.App/SampleData/SampleViewModels.cs
@@ -374,11 +374,6 @@ public bool FilterTextIsEnabled
 
         public new string Title { get { return "Clone a GitHub Repository"; } }
 
-        public bool IsLoading
-        {
-            get { return false; }
-        }
-
         public IReactiveCommand<IReadOnlyList<IRemoteRepositoryModel>> LoadRepositoriesCommand
         {
             get;
diff --git a/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs
@@ -25,7 +25,7 @@ namespace GitHub.ViewModels
     [ExportViewModel(ViewType = UIViewType.PRDetail)]
     [PartCreationPolicy(CreationPolicy.NonShared)]
     [NullGuard(ValidationFlags.None)]
-    public class PullRequestDetailViewModel : PanePageViewModelBase, IPullRequestDetailViewModel, IHasBusy
+    public class PullRequestDetailViewModel : PanePageViewModelBase, IPullRequestDetailViewModel
     {
         readonly ILocalRepositoryModel repository;
         readonly IModelService modelService;
@@ -35,10 +35,12 @@ public class PullRequestDetailViewModel : PanePageViewModelBase, IPullRequestDet
         string sourceBranchDisplayName;
         string targetBranchDisplayName;
         string body;
+        IReadOnlyList<IPullRequestChangeNode> changedFilesTree;
         IPullRequestCheckoutState checkoutState;
         IPullRequestUpdateState updateState;
         string operationError;
         bool isBusy;
+        bool isLoading;
         bool isCheckedOut;
         bool isFromFork;
         bool isInCheckout;
@@ -147,7 +149,7 @@ public string TargetBranchDisplayName
         }
 
         /// <summary>
-        /// Gets a value indicating whether the view model is loading.
+        /// Gets a value indicating whether the view model is updating.
         /// </summary>
         public bool IsBusy
         {
@@ -163,6 +165,15 @@ public bool IsCheckedOut
             private set { this.RaiseAndSetIfChanged(ref isCheckedOut, value); }
         }
 
+        /// <summary>
+        /// Gets a value indicating whether the view model is loading.
+        /// </summary>
+        public bool IsLoading
+        {
+            get { return isLoading; }
+            private set { this.RaiseAndSetIfChanged(ref isLoading, value); }
+        }
+
         /// <summary>
         /// Gets a value indicating whether the pull request comes from a fork.
         /// </summary>
@@ -212,7 +223,11 @@ public string OperationError
         /// <summary>
         /// Gets the changed files as a tree.
         /// </summary>
-        public IReactiveList<IPullRequestChangeNode> ChangedFilesTree { get; } = new ReactiveList<IPullRequestChangeNode>();
+        public IReadOnlyList<IPullRequestChangeNode> ChangedFilesTree
+        {
+            get { return changedFilesTree; }
+            private set { this.RaiseAndSetIfChanged(ref changedFilesTree, value); }
+        }
 
         /// <summary>
         /// Gets a command that checks out the pull request locally.
@@ -252,7 +267,10 @@ public override void Initialize([AllowNull] ViewWithData data)
         {
             var prNumber = data?.Data != null ? (int)data.Data : Model.Number;
 
-            IsBusy = true;
+            if (Model == null)
+                IsLoading = true;
+            else
+                IsBusy = true;
 
             OperationError = null;
             modelService.GetPullRequest(repository, prNumber)
@@ -276,15 +294,8 @@ public async Task Load(IPullRequestModel pullRequest)
             TargetBranchDisplayName = GetBranchDisplayName(IsFromFork, pullRequest.Base.Label);
             Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown;
 
-            ChangedFilesTree.Clear();
-
-            var treeChanges = await pullRequestsService.GetTreeChanges(repository, pullRequest);
-
-            // WPF doesn't support AddRange here so iterate through the changes.
-            foreach (var change in CreateChangedFilesTree(pullRequest, treeChanges).Children)
-            {
-                ChangedFilesTree.Add(change);
-            }
+            var changes = await pullRequestsService.GetTreeChanges(repository, pullRequest);
+            ChangedFilesTree = CreateChangedFilesTree(pullRequest, changes).Children.ToList();
 
             var localBranches = await pullRequestsService.GetLocalBranches(repository, pullRequest).ToList();
 
@@ -350,7 +361,7 @@ public async Task Load(IPullRequestModel pullRequest)
                 UpdateState = null;
             }
 
-            IsBusy = false;
+            IsLoading = IsBusy = false;
 
             if (!isInCheckout)
             {
diff --git a/src/GitHub.App/ViewModels/RepositoryCloneViewModel.cs b/src/GitHub.App/ViewModels/RepositoryCloneViewModel.cs
@@ -35,7 +35,6 @@ public class RepositoryCloneViewModel : DialogViewModelBase, IRepositoryCloneVie
         readonly IRepositoryHost repositoryHost;
         readonly IOperatingSystem operatingSystem;
         readonly ReactiveCommand<object> browseForDirectoryCommand = ReactiveCommand.Create();
-        bool isLoading;
         bool noRepositoriesFound;
         readonly ObservableAsPropertyHelper<bool> canClone;
         string baseRepositoryPath;
@@ -66,13 +65,13 @@ public RepositoryCloneViewModel(
             repositories.Filter = FilterRepository;
             repositories.NewerComparer = OrderedComparer<IRemoteRepositoryModel>.OrderByDescending(x => x.UpdatedAt).Compare;
 
-            filterTextIsEnabled = this.WhenAny(x => x.IsLoading,
+            filterTextIsEnabled = this.WhenAny(x => x.IsBusy,
                 loading => loading.Value || repositories.UnfilteredCount > 0 && !LoadingFailed)
                 .ToProperty(this, x => x.FilterTextIsEnabled);
 
             this.WhenAny(
                 x => x.repositories.UnfilteredCount,
-                x => x.IsLoading,
+                x => x.IsBusy,
                 x => x.LoadingFailed,
                 (unfilteredCount, loading, failed) =>
                 {
@@ -124,17 +123,17 @@ public override void Initialize([AllowNull] ViewWithData data)
         {
             base.Initialize(data);
 
-            IsLoading = true;
+            IsBusy = true;
             repositoryHost.ModelService.GetRepositories(repositories);
             repositories.OriginalCompleted.Subscribe(
                 _ => { }
                 , ex =>
                 {
                     LoadingFailed = true;
-                    IsLoading = false;
+                    IsBusy = false;
                     log.Error("Error while loading repositories", ex);
                 },
-                () => IsLoading = false
+                () => IsBusy = false
             );
             repositories.Subscribe();
         }
@@ -244,12 +243,6 @@ public string FilterText
             set { this.RaiseAndSetIfChanged(ref filterText, value); }
         }
 
-        public bool IsLoading
-        {
-            get { return isLoading; }
-            private set { this.RaiseAndSetIfChanged(ref isLoading, value); }
-        }
-
         public bool LoadingFailed
         {
             get { return loadingFailed; }
diff --git a/src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs b/src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs
@@ -1,4 +1,5 @@
 ﻿using System;
+using System.Collections.Generic;
 using System.Reactive;
 using System.Threading.Tasks;
 using GitHub.Models;
@@ -62,7 +63,7 @@ public interface IPullRequestUpdateState
     /// <summary>
     /// Represents a view model for displaying details of a pull request.
     /// </summary>
-    public interface IPullRequestDetailViewModel : IViewModel, IHasBusy
+    public interface IPullRequestDetailViewModel : IViewModel, IHasLoading, IHasBusy
     {
         /// <summary>
         /// Gets the underlying pull request model.
@@ -97,7 +98,7 @@ public interface IPullRequestDetailViewModel : IViewModel, IHasBusy
         /// <summary>
         /// Gets the changed files as a tree.
         /// </summary>
-        IReactiveList<IPullRequestChangeNode> ChangedFilesTree { get; }
+        IReadOnlyList<IPullRequestChangeNode> ChangedFilesTree { get; }
 
         /// <summary>
         /// Gets the state associated with the <see cref="Checkout"/> command.
diff --git a/src/GitHub.Exports.Reactive/ViewModels/IRepositoryCloneViewModel.cs b/src/GitHub.Exports.Reactive/ViewModels/IRepositoryCloneViewModel.cs
@@ -19,11 +19,6 @@ public interface IRepositoryCloneViewModel : IBaseCloneViewModel, IRepositoryCre
 
         bool FilterTextIsEnabled { get; }
 
-        /// <summary>
-        /// Whether or not we are currently loading repositories.
-        /// </summary>
-        bool IsLoading { get; }
-
         /// <summary>
         /// If true, then we failed to load the repositories.
         /// </summary>
diff --git a/src/GitHub.Exports/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj
@@ -129,6 +129,7 @@
     <Reference Include="WindowsBase" />
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="ViewModels\IHasLoading.cs" />
     <Compile Include="ViewModels\IPanePageViewModel.cs" />
     <Compile Include="ViewModels\IViewModel.cs" />
     <None Include="..\..\script\Key.snk" Condition="$(Buildtype) == 'Internal'">
diff --git a/src/GitHub.Exports/ViewModels/IHasBusy.cs b/src/GitHub.Exports/ViewModels/IHasBusy.cs
@@ -5,6 +5,13 @@ namespace GitHub.ViewModels
     /// <summary>
     /// Interface for view models that have a busy state.
     /// </summary>
+    /// <remarks>
+    /// <see cref="IHasLoading"/> is similar to <see cref="IHasBusy"/> but they represent
+    /// different states:
+    /// - When <see cref="IHasLoading.IsLoading"/> is true: There is no data to display.
+    /// - When <see cref="IHasBusy.IsBusy"/> is true: There is data to display but that data is
+    /// being updated or is in the process of being loaded.
+    /// </remarks>
     public interface IHasBusy
     {
         bool IsBusy { get; }
diff --git a/src/GitHub.Exports/ViewModels/IHasLoading.cs b/src/GitHub.Exports/ViewModels/IHasLoading.cs
@@ -0,0 +1,19 @@
+﻿using System;
+
+namespace GitHub.ViewModels
+{
+    /// <summary>
+    /// Interface for view models that have a busy state.
+    /// </summary>
+    /// <remarks>
+    /// <see cref="IHasLoading"/> is similar to <see cref="IHasBusy"/> but they represent
+    /// different states:
+    /// - When <see cref="IHasLoading.IsLoading"/> is true: There is no data to display.
+    /// - When <see cref="IHasBusy.IsBusy"/> is true: There is data to display but that data is
+    /// being updated or is in the process of being loaded.
+    /// </remarks>
+    public interface IHasLoading
+    {
+        bool IsLoading { get; }
+    }
+}
diff --git a/src/GitHub.UI.Reactive/Assets/Controls/BusyStateView.xaml b/src/GitHub.UI.Reactive/Assets/Controls/BusyStateView.xaml
@@ -9,17 +9,29 @@
             <Setter.Value>
                 <ControlTemplate TargetType="{x:Type local:BusyStateView}">
                     <Grid>
+                        <Grid.RowDefinitions>
+                            <RowDefinition Height="Auto"/>
+                            <RowDefinition Height="*"/>
+                        </Grid.RowDefinitions>
+                        
                         <Grid.Resources>
                             <BooleanToVisibilityConverter x:Key="b2v"/>
                         </Grid.Resources>
-                        
+
+                        <ui:GitHubProgressBar Foreground="{DynamicResource GitHubAccentBrush}"
+                                              IsIndeterminate="True"
+                                              Style="{DynamicResource GitHubProgressBar}"
+                                              Visibility="{TemplateBinding IsBusy, Converter={ui:BooleanToHiddenVisibilityConverter}}"/>
+
                         <c:Spinner Name="spinner" 
+                                   Grid.Row="1"
                                    Width="48"
                                    Height="48"
                                    HorizontalAlignment="Center"
                                    VerticalAlignment="Center"
-                                   Visibility="{TemplateBinding IsBusy, Converter={ui:BooleanToHiddenVisibilityConverter}}"/>
-                        <ContentPresenter Visibility="{TemplateBinding IsBusy, Converter={ui:BooleanToInverseHiddenVisibilityConverter}}"/>
+                                   Visibility="{TemplateBinding IsLoading, Converter={ui:BooleanToHiddenVisibilityConverter}}"/>
+                        <ContentPresenter Grid.Row="1" 
+                                          Visibility="{TemplateBinding IsLoading, Converter={ui:BooleanToInverseHiddenVisibilityConverter}}"/>
                     </Grid>
                 </ControlTemplate>
             </Setter.Value>
diff --git a/src/GitHub.UI.Reactive/Controls/BusyStateView.cs b/src/GitHub.UI.Reactive/Controls/BusyStateView.cs
@@ -1,4 +1,5 @@
 ﻿using System;
+using System.Reactive.Disposables;
 using System.Reactive.Linq;
 using System.Reactive.Subjects;
 using System.Windows;
@@ -15,6 +16,9 @@ public class BusyStateView : ContentControl, IDisposable, IActivatable
         public static readonly DependencyProperty IsBusyProperty =
             DependencyProperty.Register(nameof(IsBusy), typeof(bool), typeof(BusyStateView));
 
+        public static readonly DependencyProperty IsLoadingProperty =
+            DependencyProperty.Register(nameof(IsLoading), typeof(bool), typeof(BusyStateView));
+
         readonly Subject<object> close = new Subject<object>();
         readonly Subject<object> cancel = new Subject<object>();
 
@@ -50,6 +54,12 @@ public bool IsBusy
             set { SetValue(IsBusyProperty, value); }
         }
 
+        public bool IsLoading
+        {
+            get { return (bool)GetValue(IsLoadingProperty); }
+            set { SetValue(IsLoadingProperty, value); }
+        }
+
         protected void NotifyDone()
         {
             if (disposed)
@@ -88,16 +98,37 @@ public void Dispose()
     }
 
     public class BusyStateView<TInterface, TImplementor> : BusyStateView, IViewFor<TInterface>, IView
-        where TInterface : class, IViewModel, IHasBusy
+        where TInterface : class, IViewModel
         where TImplementor : class
     {
         public static readonly DependencyProperty ViewModelProperty = DependencyProperty.Register(
             "ViewModel", typeof(TInterface), typeof(TImplementor), new PropertyMetadata(null));
 
+        IDisposable subscriptions;
+
         public BusyStateView()
         {
-            DataContextChanged += (s, e) => ViewModel = (TInterface)e.NewValue;
-            this.OneWayBind(ViewModel, x => x.IsBusy, x => x.IsBusy);
+            DataContextChanged += (s, e) =>
+            {
+                subscriptions?.Dispose();
+                ViewModel = (TInterface)e.NewValue;
+
+                var hasLoading = ViewModel as IHasLoading;
+                var hasBusy = ViewModel as IHasBusy;
+                var subs = new CompositeDisposable();
+
+                if (hasLoading != null)
+                {
+                    subs.Add(this.OneWayBind(hasLoading, x => x.IsLoading, x => x.IsLoading));
+                }
+
+                if (hasBusy != null)
+                {
+                    subs.Add(this.OneWayBind(hasBusy, x => x.IsBusy, x => x.IsBusy));
+                }
+
+                subscriptions = subs;
+            };
         }
 
         [AllowNull]
diff --git a/src/GitHub.VisualStudio/UI/Views/Controls/RepositoryCloneControl.xaml b/src/GitHub.VisualStudio/UI/Views/Controls/RepositoryCloneControl.xaml
@@ -162,7 +162,7 @@
                           Foreground="{DynamicResource GitHubAccentBrush}"
                           IsIndeterminate="True"
                           Style="{DynamicResource GitHubProgressBar}"
-                          Visibility="{Binding IsLoading, Mode=OneWay, Converter={ui:BooleanToVisibilityConverter}}" />
+                          Visibility="{Binding IsBusy, Mode=OneWay, Converter={ui:BooleanToVisibilityConverter}}" />
 
     <StackPanel x:Name="loadingFailedPanel"
                 Margin="0,4,0,4"
diff --git a/src/UnitTests/GitHub.App/ViewModels/RepositoryCloneViewModelTests.cs b/src/UnitTests/GitHub.App/ViewModels/RepositoryCloneViewModelTests.cs
