diff --git a/src/Controls/src/Core/Picker/Picker.cs b/src/Controls/src/Core/Picker/Picker.cs index f4e4a8aa7977..74690976079d 100644 --- a/src/Controls/src/Core/Picker/Picker.cs +++ b/src/Controls/src/Core/Picker/Picker.cs @@ -79,12 +79,17 @@ public partial class Picker : View, IFontElement, ITextElement, ITextAlignmentEl readonly Lazy> _platformConfigurationRegistry; + ItemsSourceCollectionChangedSubscription _itemsSourceCollectionChangedSubscription; + // WeakNotifyCollectionChangedProxy weakly references its handler, so the Picker must keep this delegate alive. + NotifyCollectionChangedEventHandler _collectionChangedEventHandler; + /// Initializes a new instance of the Picker class. public Picker() { ((INotifyCollectionChanged)Items).CollectionChanged += OnItemsCollectionChanged; _platformConfigurationRegistry = new Lazy>(() => new PlatformConfigurationRegistry(this)); } + /// Gets a value that indicates whether the font for the searchbar text is bold, italic, or neither. This is a bindable property. public FontAttributes FontAttributes { @@ -338,7 +343,7 @@ void OnItemsSourceChanged(IList oldValue, IList newValue) UnsubscribeFromItemsSourceCollection(); } - if (Handler is not null) + if (!_isItemsSourceSubscriptionPaused) { SubscribeToItemsSourceCollection(newValue as INotifyCollectionChanged); } @@ -356,8 +361,40 @@ void OnItemsSourceChanged(IList oldValue, IList newValue) } } + sealed class ItemsSourceCollectionChangedSubscription + { + readonly WeakNotifyCollectionChangedProxy _proxy = new(); + bool _isFinalizationSuppressed; + + ~ItemsSourceCollectionChangedSubscription() => _proxy.Unsubscribe(); + + public void Subscribe(INotifyCollectionChanged source, NotifyCollectionChangedEventHandler handler) + { + if (_isFinalizationSuppressed) + { + GC.ReRegisterForFinalize(this); + _isFinalizationSuppressed = false; + } + + _proxy.Subscribe(source, handler); + } + + public void Unsubscribe() + { + _proxy.Unsubscribe(); + + if (_isFinalizationSuppressed) + return; + + GC.SuppressFinalize(this); + _isFinalizationSuppressed = true; + } + } + readonly Queue _pendingIsOpenActions = new Queue(); INotifyCollectionChanged _subscribedItemsSourceCollection; + // Preserve source updates before the first handler; pause only after explicit detachment. + bool _isItemsSourceSubscriptionPaused; void OnIsOpenPropertyChanged(bool oldValue, bool newValue) { @@ -373,8 +410,11 @@ void OnIsOpenPropertyChanged(bool oldValue, bool newValue) protected override void OnHandlerChanged() { + var wasItemsSourceSubscriptionPaused = _isItemsSourceSubscriptionPaused; + if (Handler is null) { + _isItemsSourceSubscriptionPaused = true; UnsubscribeFromItemsSourceCollection(); } @@ -382,12 +422,15 @@ protected override void OnHandlerChanged() if (Handler is not null) { + _isItemsSourceSubscriptionPaused = false; SubscribeToItemsSourceCollection(ItemsSource as INotifyCollectionChanged); - // Keep display items in sync if this Picker is detached and later reattached. - if (ItemsSource is not null) + // Observable sources stay synchronized before the first handler; rebuild only after a + // detached interval or for non-observable sources that may have changed in place. + if (ItemsSource is not null && + (wasItemsSourceSubscriptionPaused || ItemsSource is not INotifyCollectionChanged)) { - ResetItems(); + ResyncItemsAndReconcileSelection(); } } @@ -418,18 +461,15 @@ void SubscribeToItemsSourceCollection(INotifyCollectionChanged collection) } UnsubscribeFromItemsSourceCollection(); + var subscription = _itemsSourceCollectionChangedSubscription ??= new ItemsSourceCollectionChangedSubscription(); + _collectionChangedEventHandler ??= CollectionChanged; + subscription.Subscribe(collection, _collectionChangedEventHandler); _subscribedItemsSourceCollection = collection; - _subscribedItemsSourceCollection.CollectionChanged += CollectionChanged; } void UnsubscribeFromItemsSourceCollection() { - if (_subscribedItemsSourceCollection is null) - { - return; - } - - _subscribedItemsSourceCollection.CollectionChanged -= CollectionChanged; + _itemsSourceCollectionChangedSubscription?.Unsubscribe(); _subscribedItemsSourceCollection = null; } @@ -516,6 +556,9 @@ void ResyncItemsAndReconcileSelection() Handler?.UpdateValue(nameof(IPicker.Items)); + if (SelectedItem is null && TryApplyPendingSelectedIndex(forceClamp: true)) + return; + if (SelectedItem is not null) { ClampSelectedIndex(ItemsSource.IndexOf(SelectedItem)); diff --git a/src/Controls/tests/Core.UnitTests/PickerTests.cs b/src/Controls/tests/Core.UnitTests/PickerTests.cs index 6d2236581ef4..867ee297d7d7 100644 --- a/src/Controls/tests/Core.UnitTests/PickerTests.cs +++ b/src/Controls/tests/Core.UnitTests/PickerTests.cs @@ -5,7 +5,11 @@ using System.ComponentModel; using System.Globalization; using System.Linq; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; using Microsoft.Maui.Graphics; +using Microsoft.Maui.Handlers; using NSubstitute; using Xunit; @@ -1042,6 +1046,383 @@ public void PickerPreservesSelectedItemAfterInsertingItemBeforeSelection() Assert.Equal(2, picker.SelectedIndex); } + [Fact, Category(TestCategory.Memory)] + public async Task PickerItemsSourceDoesNotLeak() + { + // A long-lived / shared collection (for example a ViewModel-owned ObservableCollection) + // must not keep every Picker it was assigned to alive. + var sharedItemsSource = new ObservableCollection { "a", "b", "c" }; + + var references = CreatePickerReferences(sharedItemsSource); + + Assert.False(await references.Picker.WaitForCollect(), "Picker should not be alive!"); + Assert.False(await references.Subscription.WaitForCollect(), "ItemsSource subscription should not be alive!"); + Assert.False(await references.Proxy.WaitForCollect(), "WeakNotifyCollectionChangedProxy should not be alive!"); + GC.KeepAlive(sharedItemsSource); + } + + [Fact, Category(TestCategory.Memory)] + public async Task PickerItemsSourceChangesStillApplyAfterGc() + { + var itemsSource = new ObservableCollection { "a", "b", "c" }; + var picker = new Picker { ItemsSource = itemsSource }; + + await TestHelpers.Collect(); + + itemsSource.Add("d"); + + Assert.Equal(4, picker.Items.Count); + Assert.Equal("d", picker.Items[3]); + GC.KeepAlive(picker); + } + + [Fact] + public void PickerItemsSourceChangesPauseWhileHandlerIsDetached() + { + var itemsSource = new ObservableCollection { "a", "b", "c" }; + var picker = new Picker { ItemsSource = itemsSource }; + + picker.Handler = new PickerHandlerStub(); + picker.Handler = null; + itemsSource.Add("d"); + + Assert.Equal(3, picker.Items.Count); + + picker.Handler = new PickerHandlerStub(); + + Assert.Equal(4, picker.Items.Count); + Assert.Equal("d", picker.Items[3]); + } + + [Fact] + public void PickerDoesNotResyncObservableItemsSourceOnFirstHandlerAttach() + { + var picker = new Picker + { + ItemsSource = new ObservableCollection { "a", "b", "c" } + }; + var handler = new PickerHandlerStub(); + + picker.Handler = handler; + + Assert.Single(handler.Updates, update => update.Property == nameof(IPicker.Items)); + } + + [Fact] + public void PickerResyncsNonObservableItemsSourceOnFirstHandlerAttach() + { + var itemsSource = new List { "a" }; + var picker = new Picker { ItemsSource = itemsSource }; + itemsSource.Add("b"); + + Assert.Single(picker.Items); + + picker.Handler = new PickerHandlerStub(); + + Assert.Equal(2, picker.Items.Count); + Assert.Equal("b", picker.Items[1]); + } + + [Fact] + public void PickerItemsSourceReplacementWhileHandlerIsDetachedUsesReplacementOnReattach() + { + var oldItemsSource = new ObservableCollection { "old" }; + var replacementItemsSource = new ObservableCollection { "replacement" }; + var picker = new Picker { ItemsSource = oldItemsSource }; + + picker.Handler = new PickerHandlerStub(); + picker.Handler = null; + picker.ItemsSource = replacementItemsSource; + + oldItemsSource.Add("ignored while detached"); + replacementItemsSource.Add("added while detached"); + + Assert.Single(picker.Items); + Assert.Equal("replacement", picker.Items[0]); + + picker.Handler = new PickerHandlerStub(); + + Assert.Equal(2, picker.Items.Count); + Assert.Equal("replacement", picker.Items[0]); + Assert.Equal("added while detached", picker.Items[1]); + + oldItemsSource.Add("ignored after reattach"); + replacementItemsSource.Add("added after reattach"); + + Assert.Equal(3, picker.Items.Count); + Assert.Equal("added after reattach", picker.Items[2]); + } + + [Fact] + public void PickerReconcilesRemovedSelectionWhenHandlerReattaches() + { + var itemsSource = new ObservableCollection { "A", "B", "C" }; + var picker = new Picker + { + ItemsSource = itemsSource, + SelectedItem = "B" + }; + + picker.Handler = new PickerHandlerStub(); + picker.Handler = null; + itemsSource.Remove("B"); + + Assert.Equal("B", picker.SelectedItem); + Assert.Equal(1, picker.SelectedIndex); + + picker.Handler = new PickerHandlerStub(); + + Assert.Equal(2, picker.Items.Count); + Assert.Null(picker.SelectedItem); + Assert.Equal(-1, picker.SelectedIndex); + } + + [Fact] + public void PickerPreservesSelectionWhenItemInsertedBeforeItWhileHandlerDetached() + { + var itemsSource = new ObservableCollection { "A", "B", "C" }; + var picker = new Picker + { + ItemsSource = itemsSource, + SelectedItem = "B" + }; + + picker.Handler = new PickerHandlerStub(); + picker.Handler = null; + itemsSource.Insert(0, "Inserted"); + + Assert.Equal("B", picker.SelectedItem); + Assert.Equal(1, picker.SelectedIndex); + + picker.Handler = new PickerHandlerStub(); + + Assert.Equal(4, picker.Items.Count); + Assert.Equal("B", picker.SelectedItem); + Assert.Equal(2, picker.SelectedIndex); + } + + [Fact] + public void PickerAppliesPendingSelectedIndexWhenHandlerReattaches() + { + var itemsSource = new ObservableCollection(); + var picker = new Picker + { + ItemsSource = itemsSource, + SelectedIndex = 1 + }; + + Assert.Equal(-1, picker.SelectedIndex); + Assert.Null(picker.SelectedItem); + + picker.Handler = new PickerHandlerStub(); + picker.Handler = null; + itemsSource.Add("A"); + itemsSource.Add("B"); + + Assert.Empty(picker.Items); + Assert.Equal(-1, picker.SelectedIndex); + Assert.Null(picker.SelectedItem); + + var handler = new PickerHandlerStub(); + picker.Handler = handler; + + Assert.Equal(2, picker.Items.Count); + Assert.Equal(1, picker.SelectedIndex); + Assert.Equal("B", picker.SelectedItem); + + var itemsUpdateIndex = handler.Updates.FindIndex( + update => update.Property == nameof(IPicker.Items) && + update.ItemsCount == 2 && + update.SelectedIndex == -1); + Assert.True(itemsUpdateIndex >= 0); + + var selectedIndexUpdateIndex = handler.Updates.FindIndex( + itemsUpdateIndex + 1, + update => update.Property == nameof(IPicker.SelectedIndex) && + update.ItemsCount == 2 && + update.SelectedIndex == 1); + Assert.True(selectedIndexUpdateIndex > itemsUpdateIndex); + } + + [Fact] + public void PickerItemsSourceClearReusesCollectionChangedSubscription() + { + var oldItemsSource = new ObservableCollection { "old" }; + var replacementItemsSource = new ObservableCollection { "replacement" }; + var picker = new Picker { ItemsSource = oldItemsSource }; + var originalReferences = GetItemsSourceSubscriptionObjects(picker); + + picker.ItemsSource = null; + oldItemsSource.Add("ignored"); + + Assert.Empty(picker.Items); + + picker.ItemsSource = replacementItemsSource; + var reusedReferences = GetItemsSourceSubscriptionObjects(picker); + + // Implementation/lifecycle invariant: reuse exercises finalizer suppression and + // re-registration on the same helper instead of masking lifecycle bugs with a new proxy. + Assert.Same(originalReferences.Subscription, reusedReferences.Subscription); + Assert.Same(originalReferences.Proxy, reusedReferences.Proxy); + + replacementItemsSource.Add("added"); + + Assert.Equal(2, picker.Items.Count); + Assert.Equal("added", picker.Items[1]); + } + + [Fact, Category(TestCategory.Memory)] + public async Task ReusedPickerItemsSourceSubscriptionDoesNotLeak() + { + var sharedItemsSource = new ObservableCollection { "a", "b", "c" }; + var references = CreateReusedPickerReferences(sharedItemsSource); + + Assert.False(await references.Picker.WaitForCollect(), "Picker should not be alive!"); + Assert.False(await references.Subscription.WaitForCollect(), "ItemsSource subscription should not be alive!"); + Assert.False(await references.Proxy.WaitForCollect(), "WeakNotifyCollectionChangedProxy should not be alive!"); + GC.KeepAlive(sharedItemsSource); + } + + [Fact, Category(TestCategory.Memory)] + public async Task PickerItemsSourceReassignmentMovesCollectionChangedSubscription() + { + var replacementItemsSource = new ObservableCollection { "replacement" }; + var picker = new Picker(); + var oldItemsSourceReference = AssignAndReplaceItemsSource(picker, replacementItemsSource); + + replacementItemsSource.Add("added"); + + Assert.Equal(2, picker.Items.Count); + Assert.Equal("added", picker.Items[1]); + Assert.False(await oldItemsSourceReference.WaitForCollect(), "Old ItemsSource should not be alive!"); + + replacementItemsSource.Add("added after GC"); + + Assert.Equal(3, picker.Items.Count); + Assert.Equal("added after GC", picker.Items[2]); + GC.KeepAlive(picker); + GC.KeepAlive(replacementItemsSource); + } + + [Fact] + public void PickerItemsSourceReassignmentIgnoresOldCollectionChanges() + { + var oldItemsSource = new ObservableCollection { "old" }; + var replacementItemsSource = new ObservableCollection { "replacement" }; + var picker = new Picker { ItemsSource = oldItemsSource }; + + picker.ItemsSource = replacementItemsSource; + oldItemsSource.Add("ignored"); + + Assert.Single(picker.Items); + Assert.Equal("replacement", picker.Items[0]); + + replacementItemsSource.Add("added"); + + Assert.Equal(2, picker.Items.Count); + Assert.Equal("added", picker.Items[1]); + } + + [Fact] + public void PickerItemsSourceCollectionChangedHandlerIsLazy() + { + // Implementation/performance invariant: a non-observable source must not allocate + // the collection-changed delegate or weak subscription infrastructure. + const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance; + var handlerField = typeof(Picker).GetField("_collectionChangedEventHandler", flags); + Assert.NotNull(handlerField); + var picker = new Picker { ItemsSource = new List { "item" } }; + + Assert.Null(handlerField.GetValue(picker)); + + picker.ItemsSource = new ObservableCollection { "observable item" }; + + Assert.NotNull(handlerField.GetValue(picker)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static (WeakReference Picker, WeakReference Subscription, WeakReference Proxy) CreatePickerReferences( + ObservableCollection itemsSource) + { + var picker = new Picker + { + Handler = new PickerHandlerStub(), + ItemsSource = itemsSource + }; + var (subscription, proxy) = GetItemsSourceSubscriptionReferences(picker); + return (new WeakReference(picker), subscription, proxy); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static (WeakReference Picker, WeakReference Subscription, WeakReference Proxy) CreateReusedPickerReferences( + ObservableCollection itemsSource) + { + var picker = new Picker + { + ItemsSource = new ObservableCollection { "temporary" } + }; + picker.ItemsSource = null; + picker.ItemsSource = itemsSource; + var (subscription, proxy) = GetItemsSourceSubscriptionReferences(picker); + return (new WeakReference(picker), subscription, proxy); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static WeakReference AssignAndReplaceItemsSource(Picker picker, ObservableCollection replacementItemsSource) + { + var oldItemsSource = new ObservableCollection { "old" }; + picker.ItemsSource = oldItemsSource; + picker.ItemsSource = replacementItemsSource; + return new WeakReference(oldItemsSource); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static (WeakReference Subscription, WeakReference Proxy) GetItemsSourceSubscriptionReferences(Picker picker) + { + var (subscription, proxy) = GetItemsSourceSubscriptionObjects(picker); + return (new WeakReference(subscription), new WeakReference(proxy)); + } + + static (object Subscription, object Proxy) GetItemsSourceSubscriptionObjects(Picker picker) + { + const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance; + var subscriptionField = typeof(Picker).GetField("_itemsSourceCollectionChangedSubscription", flags); + Assert.NotNull(subscriptionField); + + var subscription = subscriptionField.GetValue(picker); + Assert.NotNull(subscription); + + var proxyField = subscription.GetType().GetField("_proxy", flags); + Assert.NotNull(proxyField); + + var proxy = proxyField.GetValue(subscription); + Assert.NotNull(proxy); + return (subscription, proxy); + } + + sealed class PickerHandlerStub : ViewHandler + { + static readonly IPropertyMapper Mapper = + new PropertyMapper + { + [nameof(IPicker.Items)] = (handler, picker) => handler.RecordUpdate(nameof(IPicker.Items), picker), + [nameof(IPicker.SelectedIndex)] = (handler, picker) => handler.RecordUpdate(nameof(IPicker.SelectedIndex), picker), + }; + + public PickerHandlerStub() : base(Mapper) + { + } + + public List<(string Property, int ItemsCount, int SelectedIndex)> Updates { get; } = new(); + + protected override object CreatePlatformView() => new object(); + + void RecordUpdate(string property, Picker picker) + { + Updates.Add((property, picker.Items.Count, picker.SelectedIndex)); + } + } + // https://github.com/dotnet/maui/issues/33307 [Fact] public void PickerClearsSelectionWhenSelectedItemIsRemovedFromItemsSource()