incremnenralLoading-pageIndex-fix #3852

Rosuavio · 2021-03-16T20:08:46Z

Fixes #3841

Only Increment CurrentPageIndex if Source.GetPagedItemsAsync's task completes with RanToCompletion

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

CurrentPageIndex is incremented before GetPagedItemsAsync completes. If it does not complete or fails it is still incremented.

What is the new behavior?

CurrentPageIndex is only incremented once GetPagedItemsAsync's Task completes with `RanToCompletion.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

ghost · 2021-03-16T20:08:49Z

Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Rosuavio · 2021-03-17T16:35:30Z

@michael-hawker, should this target 7.0.1?

ghost · 2021-03-17T22:40:05Z

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

michael-hawker · 2021-03-17T22:40:10Z

@msftbot merge if @azchohfi approves

ghost · 2021-03-17T22:40:15Z

Hello @michael-hawker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @azchohfi

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

michael-hawker · 2021-03-18T21:12:55Z

Added comment to #3841, think we should shore up tests here and then audit the desired and current behavior.

HppZ · 2021-04-21T03:32:56Z

Thinking we should have some tests around these scenario to ensure we don't skip pages either if multiple requests come in at once.

Originally posted by @michael-hawker in #3841 (comment)

michael-hawker · 2021-04-29T19:02:58Z

@azchohfi mind taking a quick look at this one? If you think this feels good, I'm good worrying about more tests later and just moving forward. Seems straight-forward enough.

michael-hawker · 2021-04-29T20:22:23Z

@HppZ since you filed the original issue, did you want to test this fix with your code? You can find out more about trying preview packages here.

If so, let us know here in the PR. We'll try and add a test in the meantime.

Rosuavio · 2021-05-04T19:11:47Z

@michael-hawker @HppZ Just added some tests and fixed an issue with multiple requests coming in asynchronously.

Microsoft.Toolkit.Uwp/IncrementalLoadingCollection/IncrementalLoadingCollection.cs

michael-hawker · 2021-05-04T21:25:34Z

Microsoft.Toolkit.Uwp/IncrementalLoadingCollection/IncrementalLoadingCollection.cs

-            var result = await Source.GetPagedItemsAsync(CurrentPageIndex++, ItemsPerPage, cancellationToken);
-            return result;
+            await _mutex.WaitAsync();
+            try


FYI @Sergio0694 added a helper here, but it's in the Media package. Just a note for the future, not sure if we want a TODO here for later. Not worth trying to centralize or expose now.

HppZ · 2021-05-05T01:58:44Z

how about this #3954

Rosuavio · 2021-05-05T16:44:04Z

how about this #3954

I think it would be better to open a separate PR for that at this time. I am tracking a few issues with the IncrementalLoadingCollection and looking to open more PRs.

…ompletes with RanToCompletion

…ction

…ollection

azchohfi · 2021-05-05T17:28:22Z

I love this. I would only add a test to test if one of the requests throws an exception. What is the expected behavior?

UnitTests/UnitTests.UWP/UI/Collection/Test_IncrementalLoadingCollection.cs

michael-hawker · 2021-05-06T16:58:49Z

@RosarioPulella stylecop is the build CI issue:

         D:\a\1\s\UnitTests\UnitTests.UWP\UI\Collection\DataSource.cs(35,9): error SA1505: An opening brace should not be followed by a blank line. [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]

opertation. Added and cleaned up tests.

Rosuavio · 2021-05-06T21:43:08Z

@azchohfi @michael-hawker this should be it for this PR, take a look.

HppZ · 2021-05-10T04:56:33Z

private static readonly SemaphoreSlim _mutex = new SemaphoreSlim(1);

why it is static ?

michael-hawker · 2021-05-10T16:56:37Z

@azchohfi we good here?

ghost requested review from michael-hawker, azchohfi and Kyaa-dost March 16, 2021 20:08

michael-hawker added this to the 7.0.1 milestone Mar 17, 2021

michael-hawker approved these changes Mar 17, 2021

View reviewed changes

michael-hawker added auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior labels Mar 17, 2021

Rosuavio changed the base branch from master to dev/7.0.1 March 18, 2021 15:37

michael-hawker modified the milestones: 7.0.1, 7.1 Mar 18, 2021

michael-hawker removed the auto merge ⚡ label Mar 18, 2021

michael-hawker marked this pull request as draft March 18, 2021 21:11

Rosuavio changed the base branch from dev/7.0.1 to master March 18, 2021 21:15

Rosuavio marked this pull request as ready for review April 15, 2021 20:56

michael-hawker added hotfix 🌶 priority 🚩 labels Apr 29, 2021

michael-hawker reviewed May 4, 2021

View reviewed changes

Microsoft.Toolkit.Uwp/IncrementalLoadingCollection/IncrementalLoadingCollection.cs Outdated Show resolved Hide resolved

michael-hawker approved these changes May 4, 2021

View reviewed changes

michael-hawker reviewed May 4, 2021

View reviewed changes

michael-hawker added the auto merge ⚡ label May 4, 2021

Rosuavio added 3 commits May 5, 2021 13:12

Only Increment CurrentPageIndex if Source.GetPagedItemsAsync's task c…

10bc0ab

…ompletes with RanToCompletion

Add syncronus and asyncronus tests for the incremtnetal loading colle…

3d6b98f

…ction

Fix issue with mumtiple requests comming to the incremental loading c…

81a8009

…ollection

michael-hawker reviewed May 5, 2021

View reviewed changes

UnitTests/UnitTests.UWP/UI/Collection/Test_IncrementalLoadingCollection.cs Outdated Show resolved Hide resolved

Rosuavio added 5 commits May 6, 2021 16:30

Move async lock to handle entire load and update

cb00c1d

opertation. Added and cleaned up tests.

Better handle faults and cancelations from data source

214520d

Fix syle cop issue

53e1c15

Clean up tests

84e799a

Fix test not checking order

ce5b153

Rosuavio requested a review from michael-hawker May 6, 2021 21:42

michael-hawker approved these changes May 7, 2021

View reviewed changes

make mutex not static

70e9271

azchohfi approved these changes May 10, 2021

View reviewed changes

ghost merged commit e348f61 into CommunityToolkit:main May 10, 2021

Rosuavio deleted the incremnenralLoading-pageIndex-fix branch May 10, 2021 17:56

michael-hawker modified the milestones: 7.1, 7.0.2 May 11, 2021

Kyaa-dost mentioned this pull request Jun 2, 2021

[IncrementalLoadingCollection] Should reset HasMoreItems to True if data is not null #3954

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

incremnenralLoading-pageIndex-fix #3852

incremnenralLoading-pageIndex-fix #3852

Rosuavio commented Mar 16, 2021 •

edited

Loading

ghost commented Mar 16, 2021

Rosuavio commented Mar 17, 2021 •

edited

Loading

ghost commented Mar 17, 2021

michael-hawker commented Mar 17, 2021

ghost commented Mar 17, 2021

michael-hawker commented Mar 18, 2021

HppZ commented Apr 21, 2021

michael-hawker commented Apr 29, 2021

michael-hawker commented Apr 29, 2021

Rosuavio commented May 4, 2021

michael-hawker May 4, 2021

HppZ commented May 5, 2021

Rosuavio commented May 5, 2021 •

edited

Loading

azchohfi commented May 5, 2021

michael-hawker commented May 6, 2021

Rosuavio commented May 6, 2021

HppZ commented May 10, 2021

michael-hawker commented May 10, 2021

incremnenralLoading-pageIndex-fix #3852

incremnenralLoading-pageIndex-fix #3852

Conversation

Rosuavio commented Mar 16, 2021 • edited Loading

Fixes #3841

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

ghost commented Mar 16, 2021

Rosuavio commented Mar 17, 2021 • edited Loading

ghost commented Mar 17, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

michael-hawker commented Mar 17, 2021

ghost commented Mar 17, 2021

michael-hawker commented Mar 18, 2021

HppZ commented Apr 21, 2021

michael-hawker commented Apr 29, 2021

michael-hawker commented Apr 29, 2021

Rosuavio commented May 4, 2021

michael-hawker May 4, 2021

Choose a reason for hiding this comment

HppZ commented May 5, 2021

Rosuavio commented May 5, 2021 • edited Loading

azchohfi commented May 5, 2021

michael-hawker commented May 6, 2021

Rosuavio commented May 6, 2021

HppZ commented May 10, 2021

michael-hawker commented May 10, 2021

Rosuavio commented Mar 16, 2021 •

edited

Loading

Rosuavio commented Mar 17, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Rosuavio commented May 5, 2021 •

edited

Loading