Skip to content

Commit

Permalink
fix(ItemsControl): Don't hold a strong reference on ItemsSource INCC/…
Browse files Browse the repository at this point in the history 
…VectorChanged
  • Loading branch information
@jeromelaban
jeromelaban committed Jan 21, 2022
1 parent 2902363 commit 753a9b9
Show file tree
Hide file tree
Showing 2 changed files with 235 additions and 75 deletions.
110 changes: 91 additions & 19 deletions src/Uno.UI/UI/Xaml/Controls/ItemCollection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,38 +197,110 @@ private void ObserveCollectionChanged(object itemsSource)
}
else if (itemsSource is INotifyCollectionChanged existingObservable)
{
// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
NotifyCollectionChangedEventHandler handler = OnItemsSourceCollectionChanged;
_itemsSourceCollectionChangeDisposable.Disposable = Disposable.Create(() =>
existingObservable.CollectionChanged -= handler
);
existingObservable.CollectionChanged += handler;
ObserveCollectionChangedInner(existingObservable);
}
else if (itemsSource is IObservableVector<object> observableVector)
{
// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
VectorChangedEventHandler<object> handler = OnItemsSourceVectorChanged;
_itemsSourceCollectionChangeDisposable.Disposable = Disposable.Create(() =>
observableVector.VectorChanged -= handler
);
observableVector.VectorChanged += handler;
ObserveCollectionChangedInner(observableVector);
}
else if (itemsSource is IObservableVector genericObservableVector)
{
VectorChangedEventHandler handler = OnItemsSourceVectorChanged;
_itemsSourceCollectionChangeDisposable.Disposable = Disposable.Create(() =>
genericObservableVector.UntypedVectorChanged -= handler
);
genericObservableVector.UntypedVectorChanged += handler;
ObserveCollectionChangedInner(genericObservableVector);
}
else
{
_itemsSourceCollectionChangeDisposable.Disposable = null;
}
}

private void ObserveCollectionChangedInner(INotifyCollectionChanged existingObservable)
{
var thatRef = new WeakReference(this);

void handler(object s, NotifyCollectionChangedEventArgs e)
{
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsCollection. The ItemsCollection is holding
// a reference to the items source, so it won`t be collected
// unless unset.Note that this block is not extracted to a separate
// helper to avoid the cost of creating additional delegates.

if (thatRef.Target is ItemCollection that)
{
that.OnItemsSourceCollectionChanged(s, e);
}
else
{
existingObservable.CollectionChanged -= handler;
}
}

// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
_itemsSourceCollectionChangeDisposable.Disposable = Disposable.Create(() =>
existingObservable.CollectionChanged -= handler
);
existingObservable.CollectionChanged += handler;
}

private void ObserveCollectionChangedInner(IObservableVector<object> observableVector)
{
var thatRef = new WeakReference(this);

void handler(IObservableVector<object> s, IVectorChangedEventArgs e)
{
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsCollection.The ItemsCollection is holding
// a reference to the items source, so it won`t be collected
// unless unset.Note that this block is not extracted to a separate
// helper to avoid the cost of creating additional delegates.

if (thatRef.Target is ItemCollection that)
{
that.OnItemsSourceVectorChanged(s, e);
}
else
{
observableVector.VectorChanged -= handler;
}
}

// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
_itemsSourceCollectionChangeDisposable.Disposable = Disposable.Create(() =>
observableVector.VectorChanged -= handler
);
observableVector.VectorChanged += handler;
}

private void ObserveCollectionChangedInner(IObservableVector genericObservableVector)
{
var thatRef = new WeakReference(this);

void handler(object s, IVectorChangedEventArgs e)
{
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsCollection.The ItemsCollection is holding
// a reference to the items source, so it won`t be collected
// unless unset.Note that this block is not extracted to a separate
// helper to avoid the cost of creating additional delegates.

if (thatRef.Target is ItemCollection that)
{
that.OnItemsSourceVectorChanged(s, e);
}
else
{
genericObservableVector.UntypedVectorChanged -= handler;
}
}

_itemsSourceCollectionChangeDisposable.Disposable = Disposable.Create(() =>
genericObservableVector.UntypedVectorChanged -= handler
);
genericObservableVector.UntypedVectorChanged += handler;
}

private void OnItemsSourceVectorChanged(object sender, IVectorChangedEventArgs args)
{
(VectorChanged, _untypedVectorChanged).TryRaise(this, args);
Expand Down
200 changes: 144 additions & 56 deletions src/Uno.UI/UI/Xaml/Controls/ItemsControl/ItemsControl.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -686,25 +686,12 @@ internal void ObserveCollectionChanged()
//Subscribe to changes on grouped source that is an observable collection
else if (unwrappedSource is CollectionView collectionView && collectionView.CollectionGroups != null && collectionView.InnerCollection is INotifyCollectionChanged observableGroupedSource)
{
// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
NotifyCollectionChangedEventHandler handler = OnItemsSourceGroupsChanged;
_notifyCollectionChanged.Disposable = Disposable.Create(() =>
observableGroupedSource.CollectionChanged -= handler
);
observableGroupedSource.CollectionChanged += handler;
ObserveCollectionChanged(observableGroupedSource);
}
//Subscribe to changes on ICollectionView that is grouped
else if (unwrappedSource is ICollectionView iCollectionView && iCollectionView.CollectionGroups != null)
{
// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
VectorChangedEventHandler<object> handler = OnItemsSourceGroupsVectorChanged;
_notifyCollectionChanged.Disposable = Disposable.Create(() =>
iCollectionView.CollectionGroups.VectorChanged -= handler
);
iCollectionView.CollectionGroups.VectorChanged += handler;

ObserveCollectionChanged(iCollectionView);
}
else
{
Expand All @@ -716,61 +703,162 @@ internal void ObserveCollectionChanged()
//Subscribe to group changes if they are observable collections
if (unwrappedSource is ICollectionView collectionViewGrouped && collectionViewGrouped.CollectionGroups != null)
{
var disposables = new CompositeDisposable();
int i = -1;
foreach (ICollectionViewGroup group in collectionViewGrouped.CollectionGroups)
ObserveCollectionChangedGrouped(collectionViewGrouped);
}
}

private void ObserveCollectionChanged(INotifyCollectionChanged observableGroupedSource)
{
var thatRef = (this as IWeakReferenceProvider)?.WeakReference;

// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
void handler(object s, NotifyCollectionChangedEventArgs e)
{
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsControl. The ItemsControl is holding
// a reference to the items source, so it won`t be collected
// unless unset. Note that this block is not extracted to a separate
// helper to avoid the cost of creating additional delegates.
if (thatRef.Target is ItemsControl that)
{
//Hidden empty groups shouldn't be counted because they won't appear to UICollectionView
if (!AreEmptyGroupsHidden || group.GroupItems.Count > 0)
{
i++;
}
var insideLoop = i;

// TODO: At present we listen to changes on ICollectionViewGroup.Group, which supports 'observable of observable groups'
// using CollectionViewSource. The correct way to do this would be for CollectionViewGroup.GroupItems to instead implement
// INotifyCollectionChanged.
INotifyCollectionChanged observableGroup = group.GroupItems as INotifyCollectionChanged ?? group.Group as INotifyCollectionChanged;
// Prefer INotifyCollectionChanged for, eg, batched item changes
if (observableGroup != null)
that.OnItemsSourceGroupsChanged(s, e);
}
else
{
observableGroupedSource.CollectionChanged -= handler;
}
}

_notifyCollectionChanged.Disposable = Disposable.Create(() =>
observableGroupedSource.CollectionChanged -= handler
);
observableGroupedSource.CollectionChanged += handler;
}

private void ObserveCollectionChanged(ICollectionView iCollectionView)
{
var thatRef = (this as IWeakReferenceProvider)?.WeakReference;

// This is a workaround for a bug with EventRegistrationTokenTable on Xamarin, where subscribing/unsubscribing to a class method directly won't
// remove the handler.
void handler(IObservableVector<object> s, IVectorChangedEventArgs e)
{
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsControl.The ItemsControl is holding
// a reference to the items source, so it won`t be collected
// unless unset.Note that this block is not extracted to a separate
// helper to avoid the cost of creating additional delegates.
if (thatRef.Target is ItemsControl that)
{
that.OnItemsSourceGroupsVectorChanged(s, e);
}
else
{
iCollectionView.CollectionGroups.VectorChanged -= handler;
}
}

_notifyCollectionChanged.Disposable = Disposable.Create(() =>
iCollectionView.CollectionGroups.VectorChanged -= handler
);
iCollectionView.CollectionGroups.VectorChanged += handler;
}

private void ObserveCollectionChangedGrouped(ICollectionView collectionViewGrouped)
{
var thatRef = (this as IWeakReferenceProvider)?.WeakReference;

var disposables = new CompositeDisposable();
int i = -1;
foreach (ICollectionViewGroup group in collectionViewGrouped.CollectionGroups)
{
//Hidden empty groups shouldn't be counted because they won't appear to UICollectionView
if (!AreEmptyGroupsHidden || group.GroupItems.Count > 0)
{
i++;
}
var insideLoop = i;

// TODO: At present we listen to changes on ICollectionViewGroup.Group, which supports 'observable of observable groups'
// using CollectionViewSource. The correct way to do this would be for CollectionViewGroup.GroupItems to instead implement
// INotifyCollectionChanged.
INotifyCollectionChanged observableGroup = group.GroupItems as INotifyCollectionChanged ?? group.Group as INotifyCollectionChanged;
// Prefer INotifyCollectionChanged for, eg, batched item changes
if (observableGroup != null)
{
void onCollectionChanged(object o, NotifyCollectionChangedEventArgs e)
{
NotifyCollectionChangedEventHandler onCollectionChanged = (o, e) => OnItemsSourceSingleCollectionChanged(o, e, insideLoop);
Disposable.Create(() => observableGroup.CollectionChanged -= onCollectionChanged)
.DisposeWith(disposables);
observableGroup.CollectionChanged += onCollectionChanged;
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsControl.The ItemsControl is holding
// a reference to the items source, so it won`t be collected
// unless unset.Note that this block is not extracted to a separate
// helper to avoid the cost of creating additional delegates.
if (thatRef.Target is ItemsControl that)
{
that.OnItemsSourceSingleCollectionChanged(o, e, insideLoop);
}
else
{
observableGroup.CollectionChanged -= onCollectionChanged;
}
}
else

Disposable.Create(() => observableGroup.CollectionChanged -= onCollectionChanged)
.DisposeWith(disposables);
observableGroup.CollectionChanged += onCollectionChanged;
}
else
{
void onVectorChanged(IObservableVector<object> o, IVectorChangedEventArgs e)
{
VectorChangedEventHandler<object> onVectorChanged = (o, e) => OnItemsSourceSingleCollectionChanged(o, e.ToNotifyCollectionChangedEventArgs(), insideLoop);
Disposable.Create(() =>
group.GroupItems.VectorChanged -= onVectorChanged
)
.DisposeWith(disposables);
group.GroupItems.VectorChanged += onVectorChanged;
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsControl.The ItemsControl is holding
// a reference to the items source, so it won`t be collected
// unless unset.
if (thatRef.Target is ItemsControl that)
{
that.OnItemsSourceSingleCollectionChanged(o, e.ToNotifyCollectionChangedEventArgs(), insideLoop);
}
else
{
group.GroupItems.VectorChanged -= onVectorChanged;
}
}

if (group is global::System.ComponentModel.INotifyPropertyChanged bindableGroup)
Disposable.Create(() =>
group.GroupItems.VectorChanged -= onVectorChanged
)
.DisposeWith(disposables);
group.GroupItems.VectorChanged += onVectorChanged;
}

if (group is global::System.ComponentModel.INotifyPropertyChanged bindableGroup)
{
Disposable.Create(() =>
bindableGroup.PropertyChanged -= onPropertyChanged
)
.DisposeWith(disposables);
bindableGroup.PropertyChanged += onPropertyChanged;
OnGroupPropertyChanged(group, insideLoop);

void onPropertyChanged(object sender, global::System.ComponentModel.PropertyChangedEventArgs e)
{
Disposable.Create(() =>
bindableGroup.PropertyChanged -= onPropertyChanged
)
.DisposeWith(disposables);
bindableGroup.PropertyChanged += onPropertyChanged;
OnGroupPropertyChanged(group, insideLoop);

void onPropertyChanged(object sender, global::System.ComponentModel.PropertyChangedEventArgs e)
if (e.PropertyName == "Group")
{
if (e.PropertyName == "Group")
// Wrap the registered delegate to avoid creating a strong
// reference to this ItemsControl.
if (thatRef.Target is ItemsControl that)
{
OnGroupPropertyChanged(group, insideLoop);
that.OnGroupPropertyChanged(group, insideLoop);
}
}
}
}
_notifyCollectionGroupsChanged.Disposable = disposables;

UpdateGroupCounts();
}
_notifyCollectionGroupsChanged.Disposable = disposables;

UpdateGroupCounts();
}

private void UpdateGroupCounts()
Expand Down

0 comments on commit 753a9b9

Please sign in to comment.