Fix for CollectionView with RefreshView when ObservableCollection<T>.Clear() throws System.ArgumentOutOfRangeException #26573
Conversation
jfversluis
added
the
area-controls-collectionview
sheiksyedm
added
the
partner/syncfusion
SuthiYuvaraj
requested review from
jfversluis,
jsuarezruiz and
a team
as code owners
There was a problem hiding this comment.
Can you add the following test to this PR?
When you copy the test over can you make sure to put it inside the XFIssue folder?
It looks like the ObservableItemSource changes were made in the following PR xamarin/Xamarin.Forms#7711 and I worry that the fixes here are reverting those fixes a bit.
@PureWeen Regarding the changes to ObservableItemSource, we’d like to clarify that our fix now ensures functionality without modifying the Count property directly. Instead, we’ve implemented proper index validation to prevent exceptions when the list is empty or the index is out of range. This approach preserves the integrity of the existing logic and aligns with the previous changes introduced in PR xamarin/Xamarin.Forms#7711. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| @@ -279,7 +279,7 @@ internal int ItemsCount() | |||
| internal object ElementAt(int index) | |||
| { | |||
| if (_itemsSource is IList list) | |||
| return list[index]; | |||
| return (index >= 0 && index < list.Count) ? list[index] : -1; | |||
There was a problem hiding this comment.
This is the ElementAt call, so should return the object or null, not -1 (index). I also see further down it is also returning -1, which is also quite wrong. If we now start returning null, what else will break?
I think this code was copied from the IndexOf method below.
But also, if we are getting here, there is something else wrong - the collection view is rendering a non-existent item? How did it get to this point?
There was a problem hiding this comment.
@mattleibow You are correct. Previously, we returned -1 based on the existing else part value. Now, we have changed the return value from -1 to null. Returning null is a more appropriate approach when a requested item does not exist in the ElementAt method. Please let us know if you have any concerns or additional feedback regarding this change.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@PureWeen Should something still be done with your comment about putting things in the XFIssue folder? |
|
@mattleibow we're good Another issue/pr was created to cover that case |
Root Cause:
The issue arises because the
UICollectionView's internal_itemSourcebecomes empty during pull-to-refresh, even though theItemsSourcein the controller still holds a valid count. This discrepancy causes an invalid state, where theGetSizeForItemmethod tries to access an index that no longer exists, resulting in aSystem.ArgumentOutOfRangeException. The issue was further compounded by the fact thatGetSizeForItemwas being called before theCollectionChangedevent was triggered. This occurs because theContentOffsetis set during the pull-to-refresh operation, which prematurely triggers layout recalculations, leading to an invalid state.Description of Change:
The change ensures the
ElementAtmethod safely accesses elements by validating the index with(index >= 0 && index < list.Count). Using a ternary operator, it immediately returns the item or-1for invalid indices, preventingSystem.ArgumentOutOfRangeExceptionand simplifying the logic for better reliability.Issues Fixed
Fixes #23868
Tested the behaviour in the following platforms
###Output Video
withoutfix.mov
withfix.mov