IncrementalLoadingCollection Fixes #1942
Conversation
| /// which triggers an automatic reload of the first page | ||
| /// </summary> | ||
| public void Refresh() | ||
| public Task RefreshAsync() |
There was a problem hiding this comment.
When was this made obsolete?
There was a problem hiding this comment.
You mean the RefreshAsync method? It was made obsolete on the previous fix, as it was thought that just by clearing and changing the properties of the collection, a reload would be triggered automatically. Having no reason to make the Refresh asynchronous, it was refactored to keep the same functionality but marked as obsolete.
There was a problem hiding this comment.
So why is the Refresh method now obsolete?
There was a problem hiding this comment.
Because the fix requires that, in a particular case, the LoadMoreItemsAsync function is called. Being an asynchronous function, I think it'd be better to encourage the developers to move towards a fully asynchronous implementation.
There was a problem hiding this comment.
Since this is 3.0, shouldn't we just pull the refresh method and mark it as a breaking change for the update?
There was a problem hiding this comment.
I'm ok with that. @kbrons , can you remove the method?
There was a problem hiding this comment.
Just removed it, also updated the sample app to use the RefreshAsync method and the PR description to reflect the breaking change.
| if (previousCount == 0) | ||
| { | ||
| // When the list was empty before clearing, the automatic reload isn't fired, so force a reload. | ||
| var loadMoreItemsTask = LoadMoreItemsAsync(0).AsTask(); |
There was a problem hiding this comment.
Just return LoadMoreItemsAsync
return LoadMoreItemsAsync(0);
There was a problem hiding this comment.
I can't call the ConfigureAwait in the following line if it's not a task, and removing it causes a deadlock when calling the Refresh method from the UI thread.
There was a problem hiding this comment.
Returning the task directly should not cause a deadlock. Work is still being done a separate thread, and work still needs to be returned to the UI thread regardless.
If we configure to not return to the calling thread here, it will still be returned when the method completes.
In this case, we're kicking off work on another thread, returning from that thread (and not going back to the original calling thread) and then returning to the calling (UI) thread which will have the requirement of returning back to the original thread. No matter what, flow is returned to the UI thread
There was a problem hiding this comment.
Sorry, you're right. Previously I was waiting the task on the Refresh method and that was causing the actual deadlock, not the context. Anyhow, I still need to return it as a task as it doesn't have a return value (required for IAsyncOperation), and I'd like to avoid copying the same call used inside that method.
Issue: #1859 #1886
PR Type
What kind of change does this PR introduce?
What is the current behavior?
In a previous version, the collection was triggering several unnecessary loads (issue #1664). The fix for that (removing the forced reload when refreshing) caused that, when the collection is empty, calling refresh does nothing.
What is the new behavior?
The collection refreshes, even if it was previously empty, while not triggering several unnecessary page loads.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information
The Refresh method was removed in favor of RefreshAsync, as the bugfix requires a call to an asynchronous method (LoadMoreItemsAsync).