Merge remote-tracking branch 'remotes/origin/refactor/pr-list' into refactor/pr-list-0.6-update

Author: StanleyGoldman

src/GitHub.App/Collections/IVirtualizingListSource.cs

@@ -0,0 +1,40 @@
+using System;
+using System.Collections.Generic;
+using System.ComponentModel;
+using System.Threading.Tasks;
+
+namespace GitHub.Collections
+{
+    /// <summary>
+    /// A loader for a virtualizing list.
+    /// </summary>
+    /// <typeparam name="T">The item type.</typeparam>
+    /// <remarks>
+    /// This interface is used by the <see cref="VirtualizingList{T}"/> class to load pages of data.
+    /// </remarks>
+    public interface IVirtualizingListSource<T> : IDisposable, INotifyPropertyChanged
+    {
+        /// <summary>
+        /// Gets a value that indicates where loading is in progress.
+        /// </summary>
+        bool IsLoading { get; }
+
+        /// <summary>
+        /// Gets the page size of the list source.
+        /// </summary>
+        int PageSize { get; }
+
+        /// <summary>
+        /// Gets the total number of items in the list.
+        /// </summary>
+        /// <returns>A task returning the count.</returns>
+        Task<int> GetCount();
+
+        /// <summary>
+        /// Gets the numbered page of items.
+        /// </summary>
+        /// <param name="pageNumber">The page number.</param>
+        /// <returns>A task returning the page contents.</returns>
+        Task<IReadOnlyList<T>> GetPage(int pageNumber);
+    }
+}

src/GitHub.App/Collections/SequentialListSource.cs

@@ -11,6 +11,22 @@
 
 namespace GitHub.Collections
 {
+    /// <summary>
+    /// An <see cref="IVirtualizingListSource{T}"/> that loads GraphQL pages sequentially, and
+    /// transforms items into a view model after reading.
+    /// </summary>
+    /// <typeparam name="TModel">The type of the model read from the remote data source.</typeparam>
+    /// <typeparam name="TViewModel">The type of the transformed view model.</typeparam>
+    /// <remarks>
+    /// GraphQL can only read pages of data sequentally, so in order to read item 450 (assuming a
+    /// page size of 100), the list source must read pages 1, 2, 3 and 4 in that order. Classes
+    /// deriving from this class only need to implement <see cref="LoadPage(string)"/> to load a
+    /// single page and this class will handle the rest.
+    /// 
+    /// In addition, items will usually need to be transformed into a view model after reading. The
+    /// implementing class overrides <see cref="CreateViewModel(TModel)"/> to carry out that
+    /// transformation.
+    /// </remarks>
     public abstract class SequentialListSource<TModel, TViewModel> : ReactiveObject, IVirtualizingListSource<TViewModel>
     {
         static readonly ILogger log = LogManager.ForContext<SequentialListSource<TModel, TViewModel>>();
@@ -26,23 +42,29 @@ public abstract class SequentialListSource<TModel, TViewModel> : ReactiveObject,
         int loadTo;
         string after;
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="SequentialListSource{TModel, TViewModel}"/> class.
+        /// </summary>
         public SequentialListSource()
         {
             dispatcher = Application.Current?.Dispatcher;
         }
 
+        /// <inheritdoc/>
         public bool IsLoading
         {
             get { return isLoading; }
             private set { this.RaiseAndSetIfChanged(ref isLoading, value); }
         }
 
+        /// <inheritdoc/>
         public virtual int PageSize => 100;
 
         event EventHandler PageLoaded;
 
         public void Dispose() => disposed = true;
 
+        /// <inheritdoc/>
         public async Task<int> GetCount()
         {
             dispatcher?.VerifyAccess();
@@ -55,6 +77,7 @@ public async Task<int> GetCount()
             return count.Value;
         }
 
+        /// <inheritdoc/>
         public async Task<IReadOnlyList<TViewModel>> GetPage(int pageNumber)
         {
             dispatcher?.VerifyAccess();
@@ -73,14 +96,31 @@ public async Task<IReadOnlyList<TViewModel>> GetPage(int pageNumber)
             return result;
         }
 
+        /// <summary>
+        /// When overridden in a derived class, transforms a model into a view model after loading.
+        /// </summary>
+        /// <param name="model">The model.</param>
+        /// <returns>The view model.</returns>
         protected abstract TViewModel CreateViewModel(TModel model);
+
+        /// <summary>
+        /// When overridden in a derived class reads a page of results from GraphQL.
+        /// </summary>
+        /// <param name="after">The GraphQL after cursor.</param>
+        /// <returns>A task which returns the page of results.</returns>
         protected abstract Task<Page<TModel>> LoadPage(string after);
 
+        /// <summary>
+        /// Called when the source begins loading pages.
+        /// </summary>
         protected virtual void OnBeginLoading()
         {
             IsLoading = true;
         }
 
+        /// <summary>
+        /// Called when the source finishes loading pages.
+        /// </summary>
         protected virtual void OnEndLoading()
         {
             IsLoading = false;

src/GitHub.App/Collections/VirtualizingList.cs

@@ -7,18 +7,42 @@
 using System.Threading.Tasks;
 using System.Windows;
 using System.Windows.Threading;
+using GitHub.Logging;
+using Serilog;
 
 namespace GitHub.Collections
 {
+    /// <summary>
+    /// A virtualizing list that loads data only when needed.
+    /// </summary>
+    /// <typeparam name="T">The list item type.</typeparam>
+    /// <remarks>
+    /// This class exposes a read-only list where the data is fetched as needed. When the indexer
+    /// getter is called, if the requested item is not yet available it calls the associated 
+    /// <see cref="IVirtualizingListSource{T}"/> to load the page of data containing the requested
+    /// item. While the data is being read, <see cref="Placeholder"/> is returned and when the
+    /// data is read <see cref="CollectionChanged"/> is raised.
+    /// 
+    /// Note that this implementation currently represents the minimum required for interaction
+    /// with WPF and as such many members are not yet implemented. In addition, if filtering is
+    /// required in the UI then the collection can be wrapped in a
+    /// <see cref="VirtualizingListCollectionView{T}"/>.
+    /// </remarks>
     public class VirtualizingList<T> : IReadOnlyList<T>, IList, INotifyCollectionChanged, INotifyPropertyChanged
     {
+        static readonly ILogger log = LogManager.ForContext<VirtualizingList<T>>();
         readonly Dictionary<int, IReadOnlyList<T>> pages = new Dictionary<int, IReadOnlyList<T>>();
         readonly IVirtualizingListSource<T> source;
         readonly IList<T> emptyPage;
         readonly IReadOnlyList<T> placeholderPage;
         readonly Dispatcher dispatcher;
         int? count;
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="VirtualizingList{T}"/> class.
+        /// </summary>
+        /// <param name="source">The list source.</param>
+        /// <param name="placeholder">The placeholder item.</param>
         public VirtualizingList(
             IVirtualizingListSource<T> source,
             T placeholder)
@@ -30,6 +54,11 @@ public VirtualizingList(
             dispatcher = Application.Current?.Dispatcher;
         }
 
+        /// <summary>
+        /// Gets an item by index.
+        /// </summary>
+        /// <param name="index">The index of the item.</param>
+        /// <returns>The item, or <see cref="Placeholder"/> if the item is not yet loaded.</returns>
         public T this[int index]
         {
             get
@@ -60,6 +89,13 @@ public T this[int index]
             set { throw new NotImplementedException(); }
         }
 
+        /// <summary>
+        /// Gets the total count of the collection, including not-yet-loaded items.
+        /// </summary>
+        /// <remarks>
+        /// If the count has not yet been loaded, this will return 0 and then raise a
+        /// <see cref="PropertyChanged"/> event when the count is loaded.
+        /// </remarks>
         public int Count
         {
             get
@@ -74,8 +110,19 @@ public int Count
             }
         }
 
+        /// <summary>
+        /// Gets the placeholder item that will be displayed while an item is loading.
+        /// </summary>
         public T Placeholder { get; }
+
+        /// <summary>
+        /// Gets the loaded pages of data.
+        /// </summary>
         public IReadOnlyDictionary<int, IReadOnlyList<T>> Pages => pages;
+
+        /// <summary>
+        /// Gets the page size of the associated <see cref="IVirtualizingListSource{T}"/>.
+        /// </summary>
         public int PageSize => source.PageSize;
 
         object IList.this[int index]
@@ -99,45 +146,14 @@ public IEnumerator<T> GetEnumerator()
             while (i < Count) yield return this[i++];
         }
 
-        int IList.Add(object value)
-        {
-            throw new NotImplementedException();
-        }
-
-        void IList.Clear()
-        {
-            throw new NotImplementedException();
-        }
-
-        bool IList.Contains(object value)
-        {
-            throw new NotImplementedException();
-        }
-
-        int IList.IndexOf(object value)
-        {
-            throw new NotImplementedException();
-        }
-
-        void IList.Insert(int index, object value)
-        {
-            throw new NotImplementedException();
-        }
-
-        void IList.Remove(object value)
-        {
-            throw new NotImplementedException();
-        }
-
-        void IList.RemoveAt(int index)
-        {
-            throw new NotImplementedException();
-        }
-
-        void ICollection.CopyTo(Array array, int index)
-        {
-            throw new NotImplementedException();
-        }
+        int IList.Add(object value) => throw new NotImplementedException();
+        void IList.Clear() => throw new NotImplementedException();
+        bool IList.Contains(object value) => throw new NotImplementedException();
+        int IList.IndexOf(object value) => throw new NotImplementedException();
+        void IList.Insert(int index, object value) => throw new NotImplementedException();
+        void IList.Remove(object value) => throw new NotImplementedException();
+        void IList.RemoveAt(int index) => throw new NotImplementedException();
+        void ICollection.CopyTo(Array array, int index) => throw new NotImplementedException();
 
         IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
 
@@ -166,9 +182,9 @@ void LoadCount()
                     }, TaskScheduler.FromCurrentSynchronizationContext());
                 }
             }
-            catch (Exception)
+            catch (Exception ex)
             {
-                // Handle exception.
+                log.Error(ex, "Error loading virtualizing list count");
             }
         }
 
@@ -187,9 +203,9 @@ async void LoadPage(int number)
                     SendReset();
                 }
             }
-            catch (Exception)
+            catch (Exception ex)
             {
-                // Handle exception.
+                log.Error(ex, "Error loading virtualizing list page {Number}", number);
             }
         }
 

src/GitHub.App/Collections/VirtualizingListCollectionView.cs

@@ -6,23 +6,39 @@
 
 namespace GitHub.Collections
 {
+    /// <summary>
+    /// A <see cref="CollectionView"/> that adds filtering to a <see cref="VirtualizingList{T}"/>.
+    /// </summary>
+    /// <typeparam name="T">The item type.</typeparam>
     public class VirtualizingListCollectionView<T> : CollectionView, IList
     {
         List<int> filtered;
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="VirtualizingListCollectionView{T}"/> class.
+        /// </summary>
+        /// <param name="inner">The inner virtualizing list.</param>
         public VirtualizingListCollectionView(VirtualizingList<T> inner)
             : base(inner)
         {
         }
 
+        /// <summary>
+        /// Gets the count of the filtered items.
+        /// </summary>
         public override int Count => filtered?.Count ?? Inner.Count;
+
+        /// <inheritdoc/>
         public override bool IsEmpty => Count == 0;
 
         bool IList.IsReadOnly => true;
         bool IList.IsFixedSize => false;
         object ICollection.SyncRoot => null;
         bool ICollection.IsSynchronized => false;
 
+        /// <summary>
+        /// Gets the inner virtualizing list.
+        /// </summary>
         protected VirtualizingList<T> Inner => (VirtualizingList<T>)SourceCollection;
 
         object IList.this[int index]
@@ -31,6 +47,7 @@ object IList.this[int index]
             set { throw new NotImplementedException(); }
         }
 
+        /// <inheritdoc/>
         public override object GetItemAt(int index)
         {
             if (filtered == null)
@@ -43,14 +60,14 @@ public override object GetItemAt(int index)
             }
         }
 
-        int IList.Add(object value) { throw new NotSupportedException(); }
-        bool IList.Contains(object value) { throw new NotImplementedException(); }
-        void IList.Clear() { throw new NotSupportedException(); }
-        int IList.IndexOf(object value) { throw new NotImplementedException(); }
-        void IList.Insert(int index, object value) { throw new NotSupportedException(); }
-        void IList.Remove(object value) { throw new NotSupportedException(); }
-        void IList.RemoveAt(int index) { throw new NotSupportedException(); }
-        void ICollection.CopyTo(Array array, int index) { throw new NotImplementedException(); }
+        int IList.Add(object value) => throw new NotSupportedException();
+        bool IList.Contains(object value) => throw new NotImplementedException();
+        void IList.Clear() => throw new NotSupportedException();
+        int IList.IndexOf(object value) => throw new NotImplementedException();
+        void IList.Insert(int index, object value) => throw new NotSupportedException();
+        void IList.Remove(object value) => throw new NotSupportedException();
+        void IList.RemoveAt(int index) => throw new NotSupportedException();
+        void ICollection.CopyTo(Array array, int index) => throw new NotImplementedException();
 
         protected override void RefreshOverride()
         {

src/GitHub.App/GitHub.App.csproj

@@ -194,6 +194,9 @@
       <HintPath>..\..\packages\Rx-XAML.2.2.5-custom\lib\net45\System.Reactive.Windows.Threading.dll</HintPath>
       <Private>True</Private>
     </Reference>
+    <Reference Include="System.ValueTuple, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
+      <HintPath>..\..\packages\System.ValueTuple.4.5.0\lib\net461\System.ValueTuple.dll</HintPath>
+    </Reference>
     <Reference Include="System.Web" />
     <Reference Include="System.Xaml" />
     <Reference Include="System.Xml.Linq" />
@@ -204,6 +207,7 @@
     <Reference Include="WindowsBase" />
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="Collections\IVirtualizingListSource.cs" />
     <Compile Include="Collections\SequentialListSource.cs" />
     <Compile Include="Collections\VirtualizingList.cs" />
     <Compile Include="Collections\VirtualizingListCollectionView.cs" />

src/GitHub.App/SampleData/PullRequestListItemViewModelDesigner.cs

@@ -13,7 +13,6 @@ public class PullRequestListItemViewModelDesigner : ViewModelBase, IPullRequestL
         public IActorViewModel Author { get; set; }
         public int CommentCount { get; set; }
         public bool IsCurrent { get; set; }
-        public IReadOnlyList<ILabelViewModel> Labels { get; set; }
         public int Number { get; set; }
         public string Title { get; set; }
         public DateTimeOffset UpdatedAt { get; set; }