Fix iOS] Fix CarouselViewLoopNoFreeze so that it's passing on both CV1 and CV2 sets of handlers #26053
Conversation
jsuarezruiz
added
the
area-controls-collectionview
src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs
Outdated
Show resolved
Hide resolved
NanthiniMahalingam
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
sheiksyedm
added
the
partner/syncfusion
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
mattleibow
left a comment
mattleibow
left a comment
There was a problem hiding this comment.
This needs a rebase and test update.
I see the CollectionViewUpdated code is pretty much the same as in CV2. But, there is a _gotoPosition = -1; commented out in CV2 that is still here. And your PR description says you removed it. So just checking.
|
|
||
| readonly ViewModelIssue12574 _viewModel; | ||
| readonly CarouselView2 _carouselView; | ||
| readonly CarouselView1 _carouselView; |
There was a problem hiding this comment.
If we do this, we lose the tests for CV2.
Since we got logic to have the system run this twice, can we drop the 1 or 2 and use the base CarouselView and let CI now run for both.
I think the original plan was to have a test for CV2 and not CV1 because CV1 was crashing, now you fixed it.
There was a problem hiding this comment.
If we do this, we lose the tests for CV2.
Since we got logic to have the system run this twice, can we drop the 1 or 2 and use the base CarouselView and let CI now run for both.
I think the original plan was to have a test for CV2 and not CV1 because CV1 was crashing, now you fixed it.
Hi @mattleibow,
I've updated the test to use the base CarouselView instead of CarouselView2 so that it runs for both CarouselView1 and CarouselView2.
NanthiniMahalingam
force-pushed
the
fix-25775
branch
from
043e212 to
197dcb1
Compare
Hi @mattleibow , |
NanthiniMahalingam
force-pushed
the
fix-25775
branch
from
ee40e36 to
6a78ceb
Compare
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix two issues with CarouselView on iOS when Loop=True and items are removed dynamically: (1) incorrect item display when removing the last item, and (2) crashes when removing all items. The fix modifies the CV1 (Items/) handler implementation and changes the test to use CarouselView instead of CarouselView2.
Key changes:
- Switch test from CarouselView2 to CarouselView (CV1 handlers)
- Add reload logic for visible cells when items are removed in loop mode
- Fix GetCorrectedIndex to handle empty collections
- Adjust loop scrolling logic from
<to<=comparison
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.HostApp/Issues/CarouselViewLoopNoFreeze.cs | Changes test to use CarouselView (CV1) instead of CarouselView2 (CV2) |
| src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs | Adds visible cell reload logic for item removal, early return for empty collections in GetCorrectedIndex, and adjusts loop scrolling comparison logic |
| var increment = currentCarouselPosition - newPosition; | ||
| var incrementAbs = Math.Abs(increment); | ||
|
|
||
| int goToPosition; |
There was a problem hiding this comment.
This change from strict less-than (<) to less-than-or-equal (<=) alters the logic for determining which direction to scroll when looping. While this might be intentional for fixing the reported issue, the PR description doesn't explain why this change is necessary. Without understanding the rationale, this could introduce subtle behavioral changes or edge cases. Consider documenting in a comment why the equality case needs to be included in this branch.
| int goToPosition; | |
| int goToPosition; | |
| // Prefer looping back to the start when the wrap-around distance is equal to | |
| // the direct distance (diffToStart == incrementAbs) to ensure deterministic | |
| // behavior in tie cases. |
| if (itemsCount == 0) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| if (indexToCorrect < itemsCount && indexToCorrect >= 0) | ||
| { | ||
| return indexToCorrect; | ||
| } |
There was a problem hiding this comment.
The refactored guard logic is good for handling the itemsCount == 0 case early. However, there's a subtle issue: when itemsCount is 0, this method returns 0, but the calling code in GetScrollToIndexPath (line 375) and other places may still try to create an NSIndexPath with this index, which could lead to issues if the CollectionView truly has no items. Consider whether returning 0 is the correct behavior when there are no items, or if the calling code should check for empty collections before calling GetScrollToIndexPath.
| if (e.Action == NotifyCollectionChangedAction.Remove) | ||
| { | ||
| //Since we can be removing the item that is already created next to the current item we need to update the visible cells | ||
| if (ItemsView.Loop) | ||
| { | ||
| CollectionView.ReloadItems(new NSIndexPath[] { GetScrollToIndexPath(targetPosition) }); | ||
| } | ||
| } |
There was a problem hiding this comment.
When all items are removed from the collection (ItemsSource.ItemCount == 0), this code will still try to reload items by calling CollectionView.ReloadItems with an NSIndexPath pointing to index 0. This could cause an exception because there are no items to reload. Consider adding a check to ensure ItemsSource.ItemCount > 0 before attempting to reload items, or at minimum before calling GetScrollToIndexPath.
|
|
||
| readonly ViewModelIssue12574 _viewModel; | ||
| readonly CarouselView2 _carouselView; | ||
| readonly CarouselView _carouselView; |
There was a problem hiding this comment.
The test file was changed from CarouselView2 to CarouselView, which means it's now using CV1 (Items/) handlers instead of CV2 (Items2/) handlers. However, according to issue #25775, this test needs to pass on BOTH CV1 and CV2 handlers. The PR only fixes the CV1 implementation in src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs, but the corresponding fix needs to be applied to the CV2 implementation in src/Controls/src/Core/Handlers/Items2/iOS/CarouselViewController2.cs as well.
The CV2 handler already has similar logic at line 318-322 that reloads items when Loop is enabled, but it doesn't restrict this to Remove actions only. However, the critical issue is that CV2's GetCorrectedIndexFromIndexPath method (lines 733-747) doesn't guard against ItemCount being 0, which could cause the same crash scenario when removing all items.
5 participants
Issue Details
Root cause:
Issue12574Test
RemoveItemsQuickly
Description Changes:
Issue12574Test
RemoveItemsQuickly
Validated the behaviour in the following platforms
Fixes #25775
Output:
Issue12574Test
ViewHanlder1.mp4
CV1Updated.mp4
RemoveItemsQuickly
Before changes
AfterChanges.mp4