Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[CollectionView] Fix crash on iOS adding items (ObservableCollection) #7711

Merged
merged 1 commit into from Dec 4, 2019

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Both, CollectionView and CarouselView, had a crash when adding items to a binder ObservableCollection (mainly played using asynchronous calls, etc.).

The native error was something like:
"Invalid update: invalid number of items in section 0. The number of items contained in an existing section after the update (1) must be equal to the number of items contained in that section before the update (1), plus or minus the number of items inserted or deleted from that section (1 inserted, 0 deleted) and plus or minus the number of items moved into or out of that section (0 moved in, 0 moved out)".

NOTE: We could get the same error in other cases (removing items, etc).

Using performBatchUpdates the item count returned by collectionView(_:numberOfItemsInSection:) should be sync with the updates made inside. We had a mismatch sometimes between the number of items in the ObservableCollection and the UICollectionView.

This article describe a similar issue: https://fangpenlin.com/posts/2016/04/29/uicollectionview-invalid-number-of-items-crash-issue/

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)
  • iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Before

Captura de pantalla 2019-09-27 a las 13 54 29

After

7578-ios-after

Testing Procedure

Launch Core Gallery and navigate to the Issue 7678. Without a crash, everything is working fine.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@rmarinho rmarinho added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Sep 28, 2019
@rmarinho
Copy link
Member

retarget 4.3 and a UItest pleaaasseeee :)

@jsuarezruiz jsuarezruiz changed the base branch from master to 4.3.0 September 30, 2019 14:20
@jsuarezruiz jsuarezruiz removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Sep 30, 2019
@samhouts samhouts removed this from In Review in v4.4.0 Sep 30, 2019
@samhouts samhouts added this to In Review in v4.3.0 Sep 30, 2019
@samhouts
Copy link
Member

@jsuarezruiz needs rebase. also, a uitest would be snazzy

@samhouts samhouts added approved Has two approvals, no pending reviews, and no changes requested in-progress This issue has an associated pull request that may resolve it! labels Oct 1, 2019
@adrianknight89
Copy link
Contributor

I'm encountering this error lately when I go from 0 items to adding an item to the items source after CollectionView is rendered.

@samhouts
Copy link
Member

samhouts commented Oct 2, 2019

@jsuarezruiz please rebase :)

@samhouts samhouts removed the in-progress This issue has an associated pull request that may resolve it! label Oct 2, 2019
@jsuarezruiz
Copy link
Contributor Author

jsuarezruiz commented Oct 3, 2019

@samhouts Done!. This is an issue repeated several times, what do you think if I include several tests?

@rmarinho
Copy link
Member

rmarinho commented Oct 3, 2019

We have a gallery for this case in CollectionView no? maybe we just need to add more UItests to that gallery

@jsuarezruiz jsuarezruiz added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Oct 4, 2019
@RhomGit
Copy link

RhomGit commented Nov 27, 2019

Is there any news on this?
Also, somewhat related ... how do you test workarounds on iOS 12 if you don't have a physical device? Visual Studio emulators are all iOS 13.

@berlamont
Copy link

@RhomGit
If you go into xcode preferences on your Mac you can download older simulator images on the components tab

@RhomGit
Copy link

RhomGit commented Nov 28, 2019

@berlamont Thanks for your advice, I have taken a look and it works as you say, excellent. Unfortunately (or fortunately since we didn't roll it out) our workaround for this issue did not work but at least now we have something to test against.

@jfversluis
Copy link
Member

Failing unit test?

@jsuarezruiz jsuarezruiz removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Dec 3, 2019
@samhouts samhouts moved this from In Progress to In Review in v4.5.0 Dec 3, 2019
@samhouts
Copy link
Member

samhouts commented Dec 3, 2019

@jsuarezruiz Also, please retarget to 4.3.0

@jsuarezruiz jsuarezruiz changed the base branch from master to 4.3.0 December 4, 2019 11:30
@jsuarezruiz jsuarezruiz removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Dec 4, 2019
@samhouts samhouts removed this from In Review in v4.5.0 Dec 4, 2019
@samhouts samhouts added this to In Review in v4.3.0 Dec 4, 2019
@rmarinho rmarinho merged commit 7365887 into 4.3.0 Dec 4, 2019
v4.3.0 automation moved this from In Review to Done Dec 4, 2019
CollectionView automation moved this from In Review to Done Dec 4, 2019
CarouselView automation moved this from In Review to Done Dec 4, 2019
@samhouts samhouts modified the milestones: 4.4.0, 4.3.0 Dec 10, 2019
@samhouts samhouts added i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often and removed i/critical labels Feb 13, 2020
@samhouts samhouts removed this from Done in CollectionView May 6, 2020
@samhouts samhouts removed this from Done in CarouselView May 6, 2020
@samhouts samhouts deleted the fix-7678-ios branch June 26, 2020 00:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/carouselview a/collectionview approved Has two approvals, no pending reviews, and no changes requested i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often p/iOS 🍎 t/bug 🐛
Projects
No open projects
Sprint 159
  
Continued in next sprint
v4.3.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet