diff --git a/src/Controls/src/Core/Button/Button.cs b/src/Controls/src/Core/Button/Button.cs index 3ccba944ec54..2c9452712458 100644 --- a/src/Controls/src/Core/Button/Button.cs +++ b/src/Controls/src/Core/Button/Button.cs @@ -616,5 +616,7 @@ private protected override string GetDebuggerDisplay() var commandText = DebuggerDisplayHelpers.GetDebugText(nameof(Command), Command, false); return $"{base.GetDebuggerDisplay()}, {textString}, {commandText}"; } + + WeakCommandSubscription ICommandElement.CleanupTracker { get; set; } } } diff --git a/src/Controls/src/Core/Cells/TextCell.cs b/src/Controls/src/Core/Cells/TextCell.cs index 544f6ba58615..283ccedc653b 100644 --- a/src/Controls/src/Core/Cells/TextCell.cs +++ b/src/Controls/src/Core/Cells/TextCell.cs @@ -1,42 +1,27 @@ #nullable disable using System; using System.Windows.Input; +using Microsoft.Maui.Controls.Internals; using Microsoft.Maui.Graphics; namespace Microsoft.Maui.Controls { /// - public class TextCell : Cell + public class TextCell : Cell, ICommandElement { /// Bindable property for . - public static readonly BindableProperty CommandProperty = BindableProperty.Create(nameof(Command), typeof(ICommand), typeof(TextCell), default(ICommand), - propertyChanging: (bindable, oldvalue, newvalue) => - { - var textCell = (TextCell)bindable; - var oldcommand = (ICommand)oldvalue; - if (oldcommand != null) - oldcommand.CanExecuteChanged -= textCell.OnCommandCanExecuteChanged; - }, propertyChanged: (bindable, oldvalue, newvalue) => - { - var textCell = (TextCell)bindable; - var newcommand = (ICommand)newvalue; - if (newcommand != null) - { - textCell.IsEnabled = newcommand.CanExecute(textCell.CommandParameter); - newcommand.CanExecuteChanged += textCell.OnCommandCanExecuteChanged; - } - }); + public static readonly BindableProperty CommandProperty = + BindableProperty.Create(nameof(Command), typeof(ICommand), typeof(TextCell), + propertyChanging: CommandElement.OnCommandChanging, + propertyChanged: CommandElement.OnCommandChanged); /// Bindable property for . - public static readonly BindableProperty CommandParameterProperty = BindableProperty.Create(nameof(CommandParameter), typeof(object), typeof(TextCell), default(object), - propertyChanged: (bindable, oldvalue, newvalue) => - { - var textCell = (TextCell)bindable; - if (textCell.Command != null) - { - textCell.IsEnabled = textCell.Command.CanExecute(newvalue); - } - }); + public static readonly BindableProperty CommandParameterProperty = + BindableProperty.Create(nameof(CommandParameter), + typeof(object), + typeof(TextCell), + null, + propertyChanged: CommandElement.OnCommandParameterChanged); /// Bindable property for . public static readonly BindableProperty TextProperty = BindableProperty.Create(nameof(Text), typeof(string), typeof(TextCell), default(string)); @@ -104,9 +89,14 @@ protected internal override void OnTapped() Command?.Execute(CommandParameter); } - void OnCommandCanExecuteChanged(object sender, EventArgs eventArgs) + void ICommandElement.CanExecuteChanged(object sender, EventArgs eventArgs) { + if (Command is null) + return; + IsEnabled = Command.CanExecute(CommandParameter); } + + WeakCommandSubscription ICommandElement.CleanupTracker { get; set; } } } \ No newline at end of file diff --git a/src/Controls/src/Core/CommandElement.cs b/src/Controls/src/Core/CommandElement.cs index 7c84c4f3f1a6..6a4a70de5519 100644 --- a/src/Controls/src/Core/CommandElement.cs +++ b/src/Controls/src/Core/CommandElement.cs @@ -10,15 +10,24 @@ static class CommandElement public static void OnCommandChanging(BindableObject bo, object o, object n) { var commandElement = (ICommandElement)bo; - if (o is ICommand oldCommand) - oldCommand.CanExecuteChanged -= commandElement.CanExecuteChanged; + commandElement.CleanupTracker?.Dispose(); + commandElement.CleanupTracker = null; } public static void OnCommandChanged(BindableObject bo, object o, object n) { var commandElement = (ICommandElement)bo; - if (n is ICommand newCommand) - newCommand.CanExecuteChanged += commandElement.CanExecuteChanged; + + if (n is null) + { + commandElement.CleanupTracker?.Dispose(); + commandElement.CleanupTracker = null; + } + else + { + commandElement.CleanupTracker = new WeakCommandSubscription(bo, (ICommand)n, commandElement.CanExecuteChanged); + } + commandElement.CanExecuteChanged(bo, EventArgs.Empty); } @@ -36,4 +45,4 @@ public static bool GetCanExecute(ICommandElement commandElement) return commandElement.Command.CanExecute(commandElement.CommandParameter); } } -} +} \ No newline at end of file diff --git a/src/Controls/src/Core/DepdendentHandle.cs b/src/Controls/src/Core/DepdendentHandle.cs new file mode 100644 index 000000000000..c625dde69c00 --- /dev/null +++ b/src/Controls/src/Core/DepdendentHandle.cs @@ -0,0 +1,96 @@ +#nullable enable + +#if NETSTANDARD2_0 || NETSTANDARD2_1 +using System; +using System.Runtime; +using System.Runtime.CompilerServices; +using System.Windows.Input; + +namespace System.Runtime; + +/// +/// A wrapper around ConditionalWeakTable that replicates DependentHandle behavior. +/// Creates a dependency between a primary object and a dependent object where +/// the dependent object becomes eligible for collection when the primary is collected. +/// +internal class DependentHandle : IDisposable +{ + private readonly ConditionalWeakTable _table; + private readonly WeakReference _primaryRef; + private bool _disposed; + + /// + /// Initializes a new instance of DependentHandle with a primary and dependent object. + /// + /// The primary object that controls the lifetime of the dependent object. + /// The dependent object that will be collected when primary is collected. + public DependentHandle(object primary, object? dependent) + { + _table = new ConditionalWeakTable(); + _primaryRef = new WeakReference(primary); + + // Store the dependent object in the table, keyed by the primary object + if (dependent is not null) + { + _table.Add(primary, dependent); + } + } + + /// + /// Gets the primary object if it's still alive, otherwise returns null. + /// + public object? Target + { + get + { + if (_disposed) + return null; + + return _primaryRef.TryGetTarget(out var target) ? target : null; + } + } + + /// + /// Gets the dependent object if the primary object is still alive, otherwise returns null. + /// + public object? Dependent + { + get + { + if (_disposed) + return null; + + if (_primaryRef.TryGetTarget(out var primary) && + _table.TryGetValue(primary, out var dependent)) + { + return dependent; + } + + return null; + } + } + + /// + /// Checks if both primary and dependent objects are still alive. + /// + public bool IsAllocated => Target is not null && Dependent is not null; + + /// + /// Disposes the DependentHandleCWT, clearing all references. + /// + public void Dispose() + { + if (_disposed) + return; + + _disposed = true; + + // Clear the table - this will allow dependent objects to be collected + // even if the primary object is still alive + if (_primaryRef.TryGetTarget(out var primary)) + { + _table.Remove(primary); + } + } +} +#endif \ No newline at end of file diff --git a/src/Controls/src/Core/ICommandElement.cs b/src/Controls/src/Core/ICommandElement.cs index 973bd254458b..13445dcfbac0 100644 --- a/src/Controls/src/Core/ICommandElement.cs +++ b/src/Controls/src/Core/ICommandElement.cs @@ -12,5 +12,7 @@ interface ICommandElement // implement these explicitly void CanExecuteChanged(object? sender, EventArgs e); + + WeakCommandSubscription? CleanupTracker { get; set; } } } diff --git a/src/Controls/src/Core/ImageButton/ImageButton.cs b/src/Controls/src/Core/ImageButton/ImageButton.cs index 738fdaf40c65..73ecfed62810 100644 --- a/src/Controls/src/Core/ImageButton/ImageButton.cs +++ b/src/Controls/src/Core/ImageButton/ImageButton.cs @@ -288,5 +288,13 @@ void IButton.Released() Color IButtonStroke.StrokeColor => (Color)GetValue(BorderColorProperty); int IButtonStroke.CornerRadius => (int)GetValue(CornerRadiusProperty); + + + WeakCommandSubscription ICommandElement.CleanupTracker + { + get; + set; + } + } } diff --git a/src/Controls/src/Core/Menu/MenuItem.cs b/src/Controls/src/Core/Menu/MenuItem.cs index 8518f14e801e..20db2f7a3264 100644 --- a/src/Controls/src/Core/Menu/MenuItem.cs +++ b/src/Controls/src/Core/Menu/MenuItem.cs @@ -198,5 +198,11 @@ void OnImageSourceSourceChanged(object sender, EventArgs e) { OnPropertyChanged(IconImageSourceProperty.PropertyName); } + + WeakCommandSubscription ICommandElement.CleanupTracker + { + get; + set; + } } } diff --git a/src/Controls/src/Core/RefreshView/RefreshView.cs b/src/Controls/src/Core/RefreshView/RefreshView.cs index 1c5848377a3c..cfeb83afc7a3 100644 --- a/src/Controls/src/Core/RefreshView/RefreshView.cs +++ b/src/Controls/src/Core/RefreshView/RefreshView.cs @@ -156,5 +156,11 @@ private protected override string GetDebuggerDisplay() var debugText = DebuggerDisplayHelpers.GetDebugText(nameof(Command), Command, nameof(IsRefreshing), IsRefreshing, false); return $"{base.GetDebuggerDisplay()}, {debugText}"; } + + WeakCommandSubscription ICommandElement.CleanupTracker + { + get; + set; + } } } diff --git a/src/Controls/src/Core/SearchBar/SearchBar.cs b/src/Controls/src/Core/SearchBar/SearchBar.cs index 84019d04da89..4b00f3da8047 100644 --- a/src/Controls/src/Core/SearchBar/SearchBar.cs +++ b/src/Controls/src/Core/SearchBar/SearchBar.cs @@ -160,5 +160,11 @@ private protected override string GetDebuggerDisplay() var debugText = DebuggerDisplayHelpers.GetDebugText(nameof(SearchCommand), SearchCommand); return $"{base.GetDebuggerDisplay()}, {debugText}"; } + + WeakCommandSubscription ICommandElement.CleanupTracker + { + get; + set; + } } } diff --git a/src/Controls/src/Core/Shell/SearchHandler.cs b/src/Controls/src/Core/Shell/SearchHandler.cs index bf6e893d5197..7f00c823b114 100644 --- a/src/Controls/src/Core/Shell/SearchHandler.cs +++ b/src/Controls/src/Core/Shell/SearchHandler.cs @@ -666,16 +666,16 @@ void ClearPlaceholderCanExecuteChanged(object sender, EventArgs e) ClearPlaceholderEnabledCore = ClearPlaceholderCommand.CanExecute(ClearPlaceholderCommandParameter); } + internal WeakCommandSubscription ClearPlaceholderCommandSubscription { get; set; } + void OnClearPlaceholderCommandChanged(ICommand oldCommand, ICommand newCommand) { - if (oldCommand != null) - { - oldCommand.CanExecuteChanged -= ClearPlaceholderCanExecuteChanged; - } + ClearPlaceholderCommandSubscription?.Dispose(); + ClearPlaceholderCommandSubscription = null; if (newCommand != null) { - newCommand.CanExecuteChanged += ClearPlaceholderCanExecuteChanged; + ClearPlaceholderCommandSubscription = new WeakCommandSubscription(this, newCommand, ClearPlaceholderCanExecuteChanged); ClearPlaceholderEnabledCore = ClearPlaceholderCommand.CanExecute(ClearPlaceholderCommandParameter); } else @@ -690,16 +690,16 @@ void OnClearPlaceholderCommandParameterChanged() ClearPlaceholderEnabledCore = ClearPlaceholderCommand.CanExecute(CommandParameter); } + internal WeakCommandSubscription CommandSubscription { get; set; } + void OnCommandChanged(ICommand oldCommand, ICommand newCommand) { - if (oldCommand != null) - { - oldCommand.CanExecuteChanged -= CanExecuteChanged; - } + CommandSubscription?.Dispose(); + CommandSubscription = null; - if (newCommand != null) + if (newCommand is not null) { - newCommand.CanExecuteChanged += CanExecuteChanged; + CommandSubscription = new WeakCommandSubscription(this, newCommand, CanExecuteChanged); IsSearchEnabledCore = Command.CanExecute(CommandParameter); } else diff --git a/src/Controls/src/Core/WeakCommandSubscription.cs b/src/Controls/src/Core/WeakCommandSubscription.cs new file mode 100644 index 000000000000..ffa1be4c0559 --- /dev/null +++ b/src/Controls/src/Core/WeakCommandSubscription.cs @@ -0,0 +1,88 @@ +#nullable enable +using System; +using System.Runtime; +using System.Windows.Input; + +namespace Microsoft.Maui.Controls +{ + class WeakCommandSubscription : IDisposable + { + internal CommandCanExecuteSubscription Proxy { get; } + public WeakCommandSubscription( + BindableObject bindableObject, + ICommand command, + Action canExecuteChangedHandler) + { + Proxy = new CommandCanExecuteSubscription(bindableObject, command, canExecuteChangedHandler); + } + + ~WeakCommandSubscription() + { + Dispose(false); + } + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + Proxy.Dispose(); + } + + internal class CommandCanExecuteSubscription : IDisposable + { + DependentHandle _dependentHandle; + ICommand? _command; + bool _disposed; + + public CommandCanExecuteSubscription( + BindableObject bindableObject, + ICommand command, + Action canExecuteChangedHandler) + { + _command = command; + // Create a DependentHandle linking the BindableObject (primary) to the handler (dependent) + _dependentHandle = new DependentHandle(bindableObject, canExecuteChangedHandler); + _command.CanExecuteChanged += CanExecuteChanged; + } + + public void Dispose() + { + if (_disposed) + return; + + _disposed = true; + + if (_command is not null) + { + _command.CanExecuteChanged -= CanExecuteChanged; + _command = null; + } + _dependentHandle.Dispose(); + _disposed = true; + } + + void CanExecuteChanged(object? arg1, EventArgs args) + { + if (_disposed) + return; + + // Try to get both the primary (BindableObject) and dependent (handler) objects + var bindableObject = _dependentHandle.Target; + var handler = _dependentHandle.Dependent as Action; + + if (bindableObject is not null && handler is not null) + { + handler.Invoke(bindableObject, args); + } + else + { + // If either object has been collected, dispose the subscription + Dispose(); + } + } + } + } +} \ No newline at end of file diff --git a/src/Controls/tests/Core.UnitTests/CommandTests.cs b/src/Controls/tests/Core.UnitTests/CommandTests.cs index 764c804c36d0..1d7c53578c6e 100644 --- a/src/Controls/tests/Core.UnitTests/CommandTests.cs +++ b/src/Controls/tests/Core.UnitTests/CommandTests.cs @@ -1,4 +1,8 @@ using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using System.Windows.Input; +using Microsoft.Maui.Controls.Internals; using Xunit; namespace Microsoft.Maui.Controls.Core.UnitTests @@ -251,5 +255,110 @@ public void ExecuteDoesNotRunIfValueTypeAndSetToNull() command.Execute(null); // "null is not a valid value for int" Assert.True(executions == 0, "the command should not have executed"); } + + [Theory] + [InlineData(typeof(Button), true)] + [InlineData(typeof(Button), false)] + [InlineData(typeof(RefreshView), true)] + [InlineData(typeof(RefreshView), false)] + [InlineData(typeof(TextCell), true)] + [InlineData(typeof(TextCell), false)] + [InlineData(typeof(ImageButton), true)] + [InlineData(typeof(ImageButton), false)] + [InlineData(typeof(MenuItem), true)] + [InlineData(typeof(MenuItem), false)] + [InlineData(typeof(SearchBar), true)] + [InlineData(typeof(SearchBar), false)] + [InlineData(typeof(SearchHandler), true)] + [InlineData(typeof(SearchHandler), false)] + public async Task CommandsSubscribedToCanExecuteCollect(Type controlType, bool useWeakEventHandler) + { + // Create a view model with a Command + ICommand command; + + if (!useWeakEventHandler) + command = new CommandWithoutWeakEventHandler(); + else + command = new Command(() => { }); + + List weakReferences = new List(); + + // Create a button in a separate scope to ensure no references remain + { + var control = (BindableObject)Activator.CreateInstance(controlType); + switch (control) + { + case Button b: + b.Command = command; + break; + case RefreshView r: + r.Command = command; + break; + case TextCell t: + t.Command = command; + break; + case ImageButton i: + i.Command = command; + break; + case MenuItem m: + m.Command = command; + break; + case SearchBar s: + s.SearchCommand = command; + break; + case SearchHandler sh: + sh.Command = command; + sh.ClearPlaceholderCommand = command; + break; + } + + // Create a weak reference to the button + weakReferences.Add(new WeakReference(control)); + + if (control is ICommandElement commandElement) + { + // Add weak references to the command and its cleanup tracker + weakReferences.Add(new WeakReference(commandElement.CleanupTracker)); + weakReferences.Add(new WeakReference(commandElement.CleanupTracker.Proxy)); + } + else if(control is SearchHandler searchHandler) + { + // Add weak references to the command and its cleanup tracker + weakReferences.Add(new WeakReference(searchHandler.CommandSubscription)); + weakReferences.Add(new WeakReference(searchHandler.CommandSubscription.Proxy)); + weakReferences.Add(new WeakReference(searchHandler.ClearPlaceholderCommandSubscription)); + weakReferences.Add(new WeakReference(searchHandler.ClearPlaceholderCommandSubscription.Proxy)); + } + + await TestHelpers.Collect(); + await TestHelpers.Collect(); + + // Make sure everything is still alive if the button is still in scope + // We need to reference the button here again to keep it alive + // awaiting a Task appears to move us to a new scope and causes the button to be collected + Assert.NotNull(control); + + foreach (var weakRef in weakReferences) + { + Assert.True(weakRef.IsAlive); + } + } + + foreach (var weakRef in weakReferences) + { + Assert.False(await weakRef.WaitForCollect()); + } + } + + class CommandWithoutWeakEventHandler : ICommand + { + public event EventHandler CanExecuteChanged; + + public bool CanExecute(object parameter) => true; + + public void Execute(object parameter) { } + + public void ChangeCanExecute() => CanExecuteChanged?.Invoke(this, EventArgs.Empty); + } } -} +} \ No newline at end of file diff --git a/src/Controls/tests/Core.UnitTests/TestHelpers.cs b/src/Controls/tests/Core.UnitTests/TestHelpers.cs index 7bfc4b1c7c9d..a1d7ef9e2bf9 100644 --- a/src/Controls/tests/Core.UnitTests/TestHelpers.cs +++ b/src/Controls/tests/Core.UnitTests/TestHelpers.cs @@ -10,6 +10,10 @@ public static async Task Collect() await Task.Yield(); GC.Collect(); GC.WaitForPendingFinalizers(); + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + GC.Collect(2, GCCollectionMode.Forced, true); + await Task.Yield(); } @@ -17,9 +21,7 @@ public static async Task WaitForCollect(this WeakReference reference) { for (int i = 0; i < 40 && reference.IsAlive; i++) { - await Task.Yield(); - GC.Collect(); - GC.WaitForPendingFinalizers(); + await Collect(); } return reference.IsAlive;