From 9e4264b06445ed37ec223768b06b5be87f312d9f Mon Sep 17 00:00:00 2001 From: =?utf8?q?Javier=20Su=C3=A1rez=20Ruiz?= Date: Mon, 4 Nov 2019 21:08:43 +0100 Subject: [PATCH] Fix crash using an object that implements IEnumerable and INotifyCollectionChanged (#8283) fixes #8200 --- .../Issue8200.cs | 169 +++++++++++++++++++++ .../Xamarin.Forms.Controls.Issues.Shared.projitems | 1 + .../CollectionView/ItemsSourceFactory.cs | 2 + .../CollectionView/ObservableItemsSource.cs | 61 ++++++-- .../CollectionView/ItemsSourceFactory.cs | 11 +- .../CollectionView/ObservableGroupedSource.cs | 2 +- .../CollectionView/ObservableItemsSource.cs | 65 ++++++-- 7 files changed, 286 insertions(+), 25 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8200.cs diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8200.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8200.cs new file mode 100644 index 0000000..bc169ec --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8200.cs @@ -0,0 +1,169 @@ +using System.Collections; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Linq; +using System.Windows.Input; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; + +#if UITEST +using NUnit.Framework; +using Xamarin.Forms.Core.UITests; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ +#if UITEST + [Category(UITestCategories.CollectionView)] +#endif + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 8200, "CollectionView on iOS assumes INotifyCollectionChanged is an IList", PlatformAffected.iOS)] + public class Issue8200 : TestContentPage + { + CollectionView _collectionView; + + public Issue8200() + { + Title = "Issue 8200"; + BindingContext = new Issue8200ViewModel(); + } + + protected override void Init() + { + var instructions = new Label + { + Text = "If you can see a CollectionView below with items, the test has passed." + }; + + var buttonsLayout = new StackLayout + { + Orientation = StackOrientation.Horizontal + }; + + var addButton = new Button + { + Text = "Add Item" + }; + + addButton.SetBinding(Button.CommandProperty, "AddItemCommand"); + + buttonsLayout.Children.Add(addButton); + + _collectionView = new CollectionView + { + BackgroundColor = Color.LightGreen, + ItemTemplate = CreateDataGridTemplate(), + SelectionMode = SelectionMode.None + }; + + _collectionView.SetBinding(ItemsView.ItemsSourceProperty, "Items"); + + var stack = new StackLayout(); + + stack.Children.Add(instructions); + stack.Children.Add(buttonsLayout); + stack.Children.Add(_collectionView); + + Content = stack; + } + + DataTemplate CreateDataGridTemplate() + { + var template = new DataTemplate(() => + { + var grid = new Grid(); + var cell = new Label(); + cell.SetBinding(Label.TextProperty, "Text"); + cell.FontSize = 20; + cell.BackgroundColor = Color.LightBlue; + grid.Children.Add(cell); + + return grid; + }); + + return template; + } + } + + [Preserve(AllMembers = true)] + public class Issue8200Model + { + public string Text { get; set; } + } + + [Preserve(AllMembers = true)] + public class Issue8200ViewModel : BindableObject + { + Issue8200Collection _items; + + public Issue8200ViewModel() + { + LoadItems(); + } + + public Issue8200Collection Items + { + get { return _items; } + set + { + _items = value; + OnPropertyChanged(); + } + } + + public ICommand AddItemCommand => new Command(AddItem); + + void LoadItems() + { + Items = new Issue8200Collection(); + + for (int i = 0; i < 30; i++) + { + Items.AddNewItem(new Issue8200Model { Text = i.ToString() }); + } + } + + void AddItem() + { + var itemsCount = Items.Count(); + Items.AddNewItem(new Issue8200Model { Text = itemsCount.ToString() }); + } + } + + public class Issue8200Collection : IEnumerable, INotifyCollectionChanged + { + readonly List _internalList = new List(); + + public IEnumerable GetItems() + { + foreach (var item in _internalList) + { + yield return item; + } + } + + public IEnumerator GetEnumerator() + { + return GetItems().GetEnumerator(); + } + + public void AddNewItem(Issue8200Model newItem) + { + int index = _internalList.Count; + _internalList.Add(newItem); + OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, newItem, index)); + } + + public event NotifyCollectionChangedEventHandler CollectionChanged; + + protected void OnCollectionChanged(NotifyCollectionChangedEventArgs e) + { + CollectionChanged?.Invoke(this, e); + } + + IEnumerator IEnumerable.GetEnumerator() + { + return GetEnumerator(); + } + } +} \ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index e1b207c..085a618 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -1143,6 +1143,7 @@ Issue7886.xaml + diff --git a/Xamarin.Forms.Platform.Android/CollectionView/ItemsSourceFactory.cs b/Xamarin.Forms.Platform.Android/CollectionView/ItemsSourceFactory.cs index a03fe0a..3053db0 100644 --- a/Xamarin.Forms.Platform.Android/CollectionView/ItemsSourceFactory.cs +++ b/Xamarin.Forms.Platform.Android/CollectionView/ItemsSourceFactory.cs @@ -18,6 +18,8 @@ namespace Xamarin.Forms.Platform.Android { case IList _ when itemsSource is INotifyCollectionChanged: return new ObservableItemsSource(itemsSource as IList, notifier); + case IEnumerable _ when itemsSource is INotifyCollectionChanged: + return new ObservableItemsSource(itemsSource as IEnumerable, notifier); case IEnumerable generic: return new ListSource(generic); } diff --git a/Xamarin.Forms.Platform.Android/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.Android/CollectionView/ObservableItemsSource.cs index 068ee0b..37ecb2b 100644 --- a/Xamarin.Forms.Platform.Android/CollectionView/ObservableItemsSource.cs +++ b/Xamarin.Forms.Platform.Android/CollectionView/ObservableItemsSource.cs @@ -6,18 +6,18 @@ namespace Xamarin.Forms.Platform.Android { internal class ObservableItemsSource : IItemsViewSource { - readonly IList _itemsSource; + readonly IEnumerable _itemsSource; readonly ICollectionChangedNotifier _notifier; bool _disposed; - public ObservableItemsSource(IList itemSource, ICollectionChangedNotifier notifier) + public ObservableItemsSource(IEnumerable itemSource, ICollectionChangedNotifier notifier) { - _itemsSource = itemSource; + _itemsSource = itemSource as IList ?? itemSource as IEnumerable; _notifier = notifier; ((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged; } - public int Count => _itemsSource.Count + (HasHeader ? 1 : 0) + (HasFooter ? 1 : 0); + public int Count => ItemsCount() + (HasHeader ? 1 : 0) + (HasFooter ? 1 : 0); public bool HasHeader { get; set; } public bool HasFooter { get; set; } @@ -39,9 +39,9 @@ namespace Xamarin.Forms.Platform.Android public int GetPosition(object item) { - for (int n = 0; n < _itemsSource.Count; n++) + for (int n = 0; n < ItemsCount(); n++) { - if (_itemsSource[n] == item) + if (ElementAt(n) == item) { return AdjustPositionForHeader(n); } @@ -52,7 +52,7 @@ namespace Xamarin.Forms.Platform.Android public object GetItem(int position) { - return _itemsSource[AdjustIndexForHeader(position)]; + return ElementAt(AdjustIndexForHeader(position)); } protected virtual void Dispose(bool disposing) @@ -122,7 +122,7 @@ namespace Xamarin.Forms.Platform.Android void Add(NotifyCollectionChangedEventArgs args) { - var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _itemsSource.IndexOf(args.NewItems[0]); + var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]); startIndex = AdjustPositionForHeader(startIndex); var count = args.NewItems.Count; @@ -163,7 +163,7 @@ namespace Xamarin.Forms.Platform.Android void Replace(NotifyCollectionChangedEventArgs args) { - var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _itemsSource.IndexOf(args.NewItems[0]); + var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]); startIndex = AdjustPositionForHeader(startIndex); var newCount = args.NewItems.Count; @@ -187,5 +187,48 @@ namespace Xamarin.Forms.Platform.Android // have to be updated. So we just have to use NotifyDataSetChanged and let the RecyclerView update everything _notifier.NotifyDataSetChanged(); } + + internal int ItemsCount() + { + if (_itemsSource is IList list) + return list.Count; + + int count = 0; + foreach (var item in _itemsSource) + count++; + return count; + } + + internal object ElementAt(int index) + { + if (_itemsSource is IList list) + return list[index]; + + int count = 0; + foreach (var item in _itemsSource) + { + if (count == index) + return item; + count++; + } + + return -1; + } + + internal int IndexOf(object item) + { + if (_itemsSource is IList list) + return list.IndexOf(item); + + int count = 0; + foreach (var i in _itemsSource) + { + if (i == item) + return count; + count++; + } + + return -1; + } } } \ No newline at end of file diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs index b38261b..98fd16d 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsSourceFactory.cs @@ -16,12 +16,15 @@ namespace Xamarin.Forms.Platform.iOS switch (itemsSource) { - case INotifyCollectionChanged _: + case IList _ when itemsSource is INotifyCollectionChanged: return new ObservableItemsSource(itemsSource as IList, collectionViewController); - case IEnumerable _: - default: - return new ListSource(itemsSource); + case IEnumerable _ when itemsSource is INotifyCollectionChanged: + return new ObservableItemsSource(itemsSource as IEnumerable, collectionViewController); + case IEnumerable generic: + return new ListSource(generic); } + + return new ListSource(itemsSource); } public static IItemsViewSource CreateGrouped(IEnumerable itemsSource, UICollectionViewController collectionViewController) diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs index fe2f166..f85623f 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs @@ -121,7 +121,7 @@ namespace Xamarin.Forms.Platform.iOS for (int n = 0; n < _groupSource.Count; n++) { - if (_groupSource[n] is INotifyCollectionChanged && _groupSource[n] is IList list) + if (_groupSource[n] is INotifyCollectionChanged && _groupSource[n] is IEnumerable list) { _groups.Add(new ObservableItemsSource(list, _collectionViewController, n)); } diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs index 1d9d749..d493321 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs @@ -12,10 +12,10 @@ namespace Xamarin.Forms.Platform.iOS readonly UICollectionView _collectionView; readonly bool _grouped; readonly int _section; - readonly IList _itemsSource; + readonly IEnumerable _itemsSource; bool _disposed; - public ObservableItemsSource(IList itemSource, UICollectionViewController collectionViewController, int group = -1) + public ObservableItemsSource(IEnumerable itemSource, UICollectionViewController collectionViewController, int group = -1) { _collectionViewController = collectionViewController; _collectionView = _collectionViewController.CollectionView; @@ -23,14 +23,14 @@ namespace Xamarin.Forms.Platform.iOS _section = group < 0 ? 0 : group; _grouped = group >= 0; - _itemsSource = itemSource; - + _itemsSource = itemSource as IList ?? itemSource as IEnumerable; + ((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged; } - public int Count => _itemsSource.Count; + public int Count => ItemsCount(); - public object this[int index] => _itemsSource[index]; + public object this[int index] => ElementAt(index); public void Dispose() { @@ -52,7 +52,7 @@ namespace Xamarin.Forms.Platform.iOS public int ItemCountInGroup(nint group) { - return _itemsSource.Count; + return ItemsCount(); } public object Group(NSIndexPath indexPath) @@ -62,7 +62,7 @@ namespace Xamarin.Forms.Platform.iOS public NSIndexPath GetIndexForItem(object item) { - for (int n = 0; n < _itemsSource.Count; n++) + for (int n = 0; n < ItemsCount(); n++) { if (this[n] == item) { @@ -75,7 +75,7 @@ namespace Xamarin.Forms.Platform.iOS public int GroupCount => 1; - public int ItemCount => _itemsSource.Count; + public int ItemCount => ItemsCount(); public object this[NSIndexPath indexPath] { @@ -141,7 +141,7 @@ namespace Xamarin.Forms.Platform.iOS void Add(NotifyCollectionChangedEventArgs args) { - var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _itemsSource.IndexOf(args.NewItems[0]); + var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]); var count = args.NewItems.Count; if (NotLoadedYet()) @@ -198,7 +198,7 @@ namespace Xamarin.Forms.Platform.iOS if (newCount == args.OldItems.Count) { - var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : _itemsSource.IndexOf(args.NewItems[0]); + var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]); // We are replacing one set of items with a set of equal size; we can do a simple item range update _collectionView.ReloadItems(CreateIndexesFrom(startIndex, newCount)); @@ -228,5 +228,48 @@ namespace Xamarin.Forms.Platform.iOS var end = Math.Max(args.OldStartingIndex, args.NewStartingIndex) + count; _collectionView.ReloadItems(CreateIndexesFrom(start, end)); } + + internal int ItemsCount() + { + if (_itemsSource is IList list) + return list.Count; + + int count = 0; + foreach (var item in _itemsSource) + count++; + return count; + } + + internal object ElementAt(int index) + { + if (_itemsSource is IList list) + return list[index]; + + int count = 0; + foreach (var item in _itemsSource) + { + if (count == index) + return item; + count++; + } + + return -1; + } + + internal int IndexOf(object item) + { + if (_itemsSource is IList list) + return list.IndexOf(item); + + int count = 0; + foreach (var i in _itemsSource) + { + if (i == item) + return count; + count++; + } + + return -1; + } } } -- 2.7.4