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

[iOS] Prevent crash when adding item to source of CollectionView in tab #7940

Merged
merged 7 commits into from
Oct 16, 2019

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Oct 11, 2019

Description of Change

If

  • a CollectionView is on a tab that has never been displayed
  • the CollectionView's ItemsSource implements INotifyCollectionChanged
  • an item is added to that ItemsSource

on iOS 13 a layout error will be raised and the CollectionView will be left in a weird state. Pre-13, it'll just crash.

These changes detect that scenario and avoid the crash.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Automated test.

PR Checklist

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

@samhouts samhouts added blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. i/critical t/bug 🐛 labels Oct 11, 2019
@hartez hartez added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Oct 13, 2019
@hartez
Copy link
Contributor Author

hartez commented Oct 13, 2019

Adding a DNM for now - this also needs to handle the same edge case for grouping.

@hartez hartez removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Oct 13, 2019
Copy link
Member

@jfversluis jfversluis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Xamarin.Forms.Platform.iOS\CollectionView\GroupableItemsViewController.cs(33,68): Error CS1503: Argument 2: cannot convert from 'UIKit.UICollectionView' to 'UIKit.UICollectionViewController'"
Review

and some small conflicts. Code in itself seems to look good

Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/Users/runner/runners/2.158.0/work/1/s/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7700.cs(49,20): error CS0117: 'FlagTestHelpers' does not contain a definition for 'SetCollectionViewTestFlag' [/Users/runner/runners/2.158.0/work/1/s/Xamarin.Forms.Controls/Xamarin.Forms.Controls.csproj]
/Users/runner/runners/2.158.0/work/1/s/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7993.xaml.cs(31,54): error CS0117: 'CollectionView' does not contain a definition for 'CollectionViewExperimental' [/Users/runner/runners/2.158.0/work/1/s/Xamarin.Forms.Controls/Xamarin.Forms.Controls.csproj]

Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Add some quick instructions to that first page on 7700? then merge?

@samhouts samhouts assigned PureWeen and unassigned jfversluis Oct 16, 2019
@PureWeen PureWeen mentioned this pull request Oct 16, 2019
2 tasks
@hartez hartez merged commit 10b3d77 into 4.3.0 Oct 16, 2019
@samhouts samhouts added this to the 4.3.0 milestone Oct 17, 2019
@hartez hartez deleted the fix-gh7700 branch October 23, 2019 14:50
CliffAgius pushed a commit to CliffAgius/Xamarin.Forms that referenced this pull request Dec 6, 2019
…ab (xamarin#7940)

* Prevent crash when adding item to source of CollectionView that's never been displayed;
fixes xamarin#7700

* Fix renaming error in UI test

* Add test and fix for adding groups offscreen

* Fix rebase error

* Remove flags

* Add instructions

* Better instructions
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/collectionview blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often p/iOS 🍎 t/bug 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants