Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Speed up PR list and other improvements #1737

Merged
merged 41 commits into from
Jul 4, 2018
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
599efea
Basic reimplementation of PR list.
grokys Jun 14, 2018
b9ce76b
Double clicking a PR opens it.
grokys Jun 14, 2018
71f57c6
Implement PR state filter.
grokys Jun 14, 2018
c084813
Display more accurate comment count.
grokys Jun 15, 2018
77c48cc
Implement Refresh.
grokys Jun 15, 2018
421ad8d
Cancel previous load on refresh.
grokys Jun 15, 2018
3c9cebb
Implement filtering.
grokys Jun 15, 2018
4246b53
Display messages when no PRs found.
grokys Jun 15, 2018
aec7a5a
Started implementing an author filter.
grokys Jun 15, 2018
0921da9
Make author filter work.
grokys Jun 15, 2018
f1c3255
Close author filter on selection.
grokys Jun 15, 2018
e19d3c1
Merge branch 'refactor/pr-models' into refactor/pr-list
grokys Jun 17, 2018
373226f
Style PR list a bit.
grokys Jun 17, 2018
9b722a4
Wire up create pull request.
grokys Jun 17, 2018
f30cab3
Use IIssueListItemViewModelBase not IViewModel.
grokys Jun 17, 2018
f4a8b8e
Display the current PR in the list.
grokys Jun 17, 2018
58a49b5
Fix dark theme.
grokys Jun 17, 2018
55b1611
Switch between fork/parent repository.
grokys Jun 19, 2018
6157a24
Merge branch 'master' into refactor/pr-list
jcansdale Jun 19, 2018
2cccd71
Fix non-compiling tests.
grokys Jun 20, 2018
19f56c0
Merge branch 'refactor/pr-list' of https://github.com/github/VisualSt…
grokys Jun 20, 2018
5b6ac81
Merge branch 'master' into refactor/pr-list
StanleyGoldman Jun 20, 2018
dbe8d3b
Make virtualizing list unit testable.
grokys Jun 20, 2018
1991f03
Added failing test.
grokys Jun 20, 2018
1636c1d
Fix failing test.
grokys Jun 20, 2018
4876f10
Merge branch 'refactor/pr-list' of https://github.com/github/VisualSt…
grokys Jun 20, 2018
a94466b
Merge branch 'master' into refactor/pr-list
grokys Jun 21, 2018
307b45a
Merge branch 'master' into refactor/pr-list
grokys Jun 28, 2018
7718388
Consolidate Octokit.GraphQL package version.
grokys Jun 28, 2018
aa85e58
Added XML docs and logging.
grokys Jun 28, 2018
dd6d3f8
Added/updated view model XML docs.
grokys Jun 28, 2018
ebc0127
Don't show NoOpenItems when there's search criteria.
grokys Jun 29, 2018
a5871c6
Hide author filter when no items.
grokys Jun 29, 2018
a6200b3
Implement opening PR list in browser.
grokys Jun 29, 2018
7598d20
Added context menu to PR list.
grokys Jul 3, 2018
809ebca
Hide PR count when list is loading.
grokys Jul 3, 2018
f59a513
Clear user filter when item selected.
grokys Jul 3, 2018
022271e
Navigate to PR with enter key from PR list.
grokys Jul 3, 2018
db00183
Fix typo.
grokys Jul 4, 2018
d52302a
ReadParentOwnerLogin -> FindParent.
grokys Jul 4, 2018
7ce21a3
Fix non-compiling tests.
grokys Jul 4, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added lib/Octokit.GraphQL.0.0.5-alpha.nupkg
Binary file not shown.
10 changes: 4 additions & 6 deletions src/GitHub.Api/GitHub.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,11 @@
<HintPath>..\..\packages\Newtonsoft.Json.10.0.3\lib\net45\Newtonsoft.Json.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Octokit.GraphQL, Version=0.0.4.0, Culture=neutral, PublicKeyToken=0be8860aee462442, processorArchitecture=MSIL">
<HintPath>..\..\packages\Octokit.GraphQL.0.0.4-alpha\lib\netstandard1.1\Octokit.GraphQL.dll</HintPath>
<Private>True</Private>
<Reference Include="Octokit.GraphQL, Version=0.0.5.0, Culture=neutral, PublicKeyToken=0be8860aee462442, processorArchitecture=MSIL">
<HintPath>..\..\packages\Octokit.GraphQL.0.0.5-alpha\lib\netstandard1.1\Octokit.GraphQL.dll</HintPath>
</Reference>
<Reference Include="Octokit.GraphQL.Core, Version=0.0.4.0, Culture=neutral, PublicKeyToken=0be8860aee462442, processorArchitecture=MSIL">
<HintPath>..\..\packages\Octokit.GraphQL.0.0.4-alpha\lib\netstandard1.1\Octokit.GraphQL.Core.dll</HintPath>
<Private>True</Private>
<Reference Include="Octokit.GraphQL.Core, Version=0.0.5.0, Culture=neutral, PublicKeyToken=0be8860aee462442, processorArchitecture=MSIL">
<HintPath>..\..\packages\Octokit.GraphQL.0.0.5-alpha\lib\netstandard1.1\Octokit.GraphQL.Core.dll</HintPath>
</Reference>
<Reference Include="Serilog, Version=2.0.0.0, Culture=neutral, PublicKeyToken=24c2f752a8e58a10, processorArchitecture=MSIL">
<HintPath>..\..\packages\Serilog.2.5.0\lib\net46\Serilog.dll</HintPath>
Expand Down
2 changes: 1 addition & 1 deletion src/GitHub.Api/packages.config
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Newtonsoft.Json" version="10.0.3" targetFramework="net461" />
<package id="Octokit.GraphQL" version="0.0.4-alpha" targetFramework="net461" />
<package id="Octokit.GraphQL" version="0.0.5-alpha" targetFramework="net461" />
<package id="Serilog" version="2.5.0" targetFramework="net461" />
</packages>
40 changes: 40 additions & 0 deletions src/GitHub.App/Collections/IVirtualizingListSource.cs
Original file line number Diff line number Diff line change
@@ -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 the the <see cref="VirtualizingList{T}"/> class to load pages of data.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"by the" not "the the"?

/// </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);
}
}
198 changes: 198 additions & 0 deletions src/GitHub.App/Collections/SequentialListSource.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Threading;
using GitHub.Logging;
using GitHub.Models;
using ReactiveUI;
using Serilog;

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>>();

readonly Dispatcher dispatcher;
readonly object loadLock = new object();
Dictionary<int, Page<TModel>> pages = new Dictionary<int, Page<TModel>>();
Task loading = Task.CompletedTask;
bool disposed;
bool isLoading;
int? count;
int nextPage;
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();

if (!count.HasValue)
{
count = (await EnsureLoaded(0).ConfigureAwait(false)).TotalCount;
}

return count.Value;
}

/// <inheritdoc/>
public async Task<IReadOnlyList<TViewModel>> GetPage(int pageNumber)
{
dispatcher?.VerifyAccess();

var page = await EnsureLoaded(pageNumber);

if (page == null)
{
return null;
}

var result = page.Items
.Select(CreateViewModel)
.ToList();
pages.Remove(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;
}

async Task<Page<TModel>> EnsureLoaded(int pageNumber)
{
if (pageNumber < nextPage)
{
return pages[pageNumber];
}

var pageLoaded = WaitPageLoaded(pageNumber);
loadTo = Math.Max(loadTo, pageNumber);

while (!disposed)
{
lock (loadLock)
{
if (loading.IsCompleted)
{
loading = Load();
}
}

await Task.WhenAny(loading, pageLoaded).ConfigureAwait(false);

if (pageLoaded.IsCompleted)
{
return pages[pageNumber];
}
}

return null;
}

Task WaitPageLoaded(int page)
{
var tcs = new TaskCompletionSource<bool>();
EventHandler handler = null;
handler = (s, e) =>
{
if (nextPage > page)
{
tcs.SetResult(true);
PageLoaded -= handler;
}
};
PageLoaded += handler;
return tcs.Task;
}

async Task Load()
{
OnBeginLoading();

while (nextPage <= loadTo && !disposed)
{
await LoadNextPage().ConfigureAwait(false);
PageLoaded?.Invoke(this, EventArgs.Empty);
}

OnEndLoading();
}

async Task LoadNextPage()
{
log.Debug("Loading page {Number} of {ModelType}", nextPage, typeof(TModel));

var page = await LoadPage(after).ConfigureAwait(false);
pages[nextPage++] = page;
after = page.EndCursor;
}
}
}
Loading