Windowscommunitytoolkit: IncrementalLoadingCollection reload

[ianier]

We should be able to tell IncrementalLoadingCollection to reload data from its source, something like a Refresh() method. The use case I have is implementing a filter on the collection.
I presume it's as simple as clearing the collection, setting CurrentPageIndex to zero and HasMoreItems to true. Unfortunately the HasMoreItems setter is private.

[hermitdave]

I've had a similar need before and played about with clearing items and setting HasMoreItems to true. We could add these. Feel free to raise a PR if you have time.

[deltakosh]

Agree. this is a good idea

[deltakosh]

@ianier fancy doing the PR?

[ianier]

@deltakosh I tried, but unfortunately found no elegant solution.
Here's what I did in my Refresh method: If not Loading, clear the collection, set CurrentPageIndex to zero and HasMoreItems to true. If Loading, then set a flag to defer the same actions when loading completes, and in the UI thread.
The problem is that whenever the collection is empty (e.g. when loading didn't return any items) there seems to be no way to tell a GridView to load more items again other than adding something to the collection. I tried raising OnCollectionChanged from the Refresh method, but that didn't work either.
Kicking a new load from the Refresh method would work; however, we would then need to think about how to synchronize.
Any ideas?
BTW, I also found that having an event for loading instead of - or in addition to - an interface is more convenient in some scenarios (e.g. multiple collections in the same view model).

[deltakosh]

we can still add an event this is not a problem. regarding the refresh issue with empty collection I did not check yet. If someone wants to give a try:)

[ScottIsAFool]

Why not just create a new instance of your collection? Would that not have the same effect?

[ianier]

@ScottIsAFool Good idea as long as the source allows simultaneous calls for different pages (e.g. if the collection is switched while loading is in progress). This is the same requirement as when connecting multiple collections to the same source, btw.

[h82258652]

It is necessary to add a Refresh method in IncrementalLoadingCollection.
This method will clear all the items and reset the CurrentPageIndex.
I think it is better than create another collection and reset the binding.

[deltakosh]

I tend to agree (marking this one as Help wanted)

[ianier]

@h82258652 I had tried that solution - see my comment above -, but unfortunately it doesn't work when the collection is empty, at least not with a GridView.

[h82258652]

@ianier
Make sure the HasMoreItems property is True after clear all the items.
And then, call LoadMoreItemsAsync(1). At least it work for me now.

[ianier]

@h82258652 : that's one of the things I tried, but there was something I didn't like about it: either we don't await on LoadMoreItemsAsync, or make the refresh method something like RefreshAsync. I would have preferred Refresh not to be awaitable, but I dislike the idea of not awaiting on LoadMoreItemsAsync. @deltakosh can you give us some guidance on what's acceptable from an API design perspective?
Also bear in mind that we still need to synchronize things so that refreshing is deferred when loading is already in progress. This requires some carefully written code.
Please feel free to do a PR and I'd be happy to review/contribute.

[deltakosh]

Simplicity should be our guide. So anything that requires no code from user is great

[ianier]

OK, let's give it a first try:

```
public void Refresh()
{
if (this.IsLoading)
{
_refreshOnLoad = true;
}
else
{
this.Clear();
this.CurrentPageIndex = 0;
this.HasMoreItems = true;
LoadMoreItemsAsync(1); // don't await
}
}

...where _refreshOnLoad is a private bool class member. Then in LoadDataAsync:

...
finally
{
IsLoading = false;

            // added this
            if (_refreshOnLoad)
            {
                _refreshOnLoad = false;
                await Window.Current.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () => Refresh());
            }
        }

```

[deltakosh]

seems good as first glance. Just concerned by the _refreshOnLoad variable. We may need to protect it into a critical section

[hermitdave]

@ianier better make a PR so we can have a better view

[h82258652]

@ianier good.
Maybe we should add a OnRefresh Method in IIncrementalSource<TSource> interface?
But I don't know is it necessary.

[ianier]

@deltakosh I'll try replacing AsyncInfo.Run((c) => LoadMoreItemsAsync(count, c)); by LoadMoreItemsAsync(count, new CancellationToken()).AsAsyncOperation() to run all continuations in the caller thread context (i.e. the UI thread) and therefore get rid of all the synchronization code.

[ianier]

FYI, I'm ready with the changes on my local repository. Now I'm trying to figure out how to do the PR (I'm new to GitHub).
Edit: In the meantime I posted a Gist with the full code here