Merge pull request #1681 from MahApps/punker76-flyoutcontrol-memory-leak

fix FlyoutsControl memory leak and exception

MahApps.Metro/Controls/Flyout.cs

@@ -57,6 +57,9 @@ public event RoutedEventHandler ClosingFinished
         public static readonly DependencyProperty CloseButtonVisibilityProperty = DependencyProperty.Register("CloseButtonVisibility", typeof(Visibility), typeof(Flyout), new FrameworkPropertyMetadata(Visibility.Visible));
         public static readonly DependencyProperty TitleVisibilityProperty = DependencyProperty.Register("TitleVisibility", typeof(Visibility), typeof(Flyout), new FrameworkPropertyMetadata(Visibility.Visible));
 
+        internal PropertyChangeNotifier IsOpenPropertyChangeNotifier { get; set; }
+        internal PropertyChangeNotifier ThemePropertyChangeNotifier { get; set; }
+
         /// <summary>
         /// Gets/sets if the title is visible in this flyout.
         /// </summary>

MahApps.Metro/Controls/FlyoutsControl.cs

@@ -1,8 +1,6 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
-using System.Collections.Specialized;
-using System.ComponentModel;
 using System.Linq;
 using System.Windows;
 using System.Windows.Controls;
@@ -54,60 +52,21 @@ protected override bool IsItemItsOwnContainerOverride(object item)
             return item is Flyout;
         }
 
-        protected override void OnItemsChanged(NotifyCollectionChangedEventArgs e)
+        protected override void PrepareContainerForItemOverride(DependencyObject element, object item)
         {
-            base.OnItemsChanged(e);
-
-            switch (e.Action)
-            {
-                case NotifyCollectionChangedAction.Add:
-                    this.AttachHandlers(this.GetFlyouts(e.NewItems));
-                    break;
-                case NotifyCollectionChangedAction.Replace:
-                    this.AttachHandlers(this.GetFlyouts(e.NewItems));
-                    this.DetachHandlers(this.GetFlyouts(e.OldItems));
-                    break;
-                case NotifyCollectionChangedAction.Remove:
-                    this.DetachHandlers(this.GetFlyouts(e.OldItems));
-                    break;
-                case NotifyCollectionChangedAction.Reset:
-                    List<Flyout> flyouts = this.GetFlyouts(this.Items).ToList();
-                    this.DetachHandlers(flyouts);
-                    this.AttachHandlers(flyouts);
-                    break;
-            }
+            base.PrepareContainerForItemOverride(element, item);
+            this.AttachHandlers((Flyout)element);
         }
 
-        private void AttachHandlers(IEnumerable<Flyout> items)
+        private void AttachHandlers(Flyout flyout)
         {
-            foreach (var item in items)
-            {
-                this.AttachHandlers(item);
-            }
-        }
+            var isOpenNotifier = new PropertyChangeNotifier(flyout, Flyout.IsOpenProperty);
+            isOpenNotifier.ValueChanged += FlyoutStatusChanged;
+            flyout.IsOpenPropertyChangeNotifier = isOpenNotifier;
 
-        private void AttachHandlers(Flyout item)
-        {
-            var isOpenChanged = DependencyPropertyDescriptor.FromProperty(Flyout.IsOpenProperty, typeof(Flyout));
-            var themeChanged = DependencyPropertyDescriptor.FromProperty(Flyout.ThemeProperty, typeof(Flyout));
-            isOpenChanged.AddValueChanged(item, this.FlyoutStatusChanged);
-            themeChanged.AddValueChanged(item, this.FlyoutStatusChanged);
-        }
-
-        private void DetachHandlers(IEnumerable<Flyout> items)
-        {
-            foreach (var item in items)
-            {
-                this.DetachHandlers(item);
-            }
-        }
-
-        private void DetachHandlers(Flyout item)
-        {
-            var isOpenChanged = DependencyPropertyDescriptor.FromProperty(Flyout.IsOpenProperty, typeof(Flyout));
-            var themeChanged = DependencyPropertyDescriptor.FromProperty(Flyout.ThemeProperty, typeof(Flyout));
-            isOpenChanged.RemoveValueChanged(item, this.FlyoutStatusChanged);
-            themeChanged.RemoveValueChanged(item, this.FlyoutStatusChanged);
+            var themeNotifier = new PropertyChangeNotifier(flyout, Flyout.ThemeProperty);
+            themeNotifier.ValueChanged += FlyoutStatusChanged;
+            flyout.ThemePropertyChangeNotifier = themeNotifier;
         }
 
         private void FlyoutStatusChanged(object sender, EventArgs e)

MahApps.Metro/Controls/PropertyChangeNotifier.cs

@@ -0,0 +1,109 @@
+using System;
+using System.ComponentModel;
+using System.Windows;
+using System.Windows.Data;
+
+namespace MahApps.Metro.Controls
+{
+    /// <summary>
+    /// AddValueChanged of dependency property descriptor results in memory leak as you already know.
+    /// So, as described here, you can create custom class PropertyChangeNotifier to listen
+    /// to any dependency property changes.
+    /// 
+    /// This class takes advantage of the fact that bindings use weak references to manage associations
+    /// so the class will not root the object who property changes it is watching. It also uses a WeakReference
+    /// to maintain a reference to the object whose property it is watching without rooting that object.
+    /// In this way, you can maintain a collection of these objects so that you can unhook the property
+    /// change later without worrying about that collection rooting the object whose values you are watching.
+    /// 
+    /// Complete implementation can be found here: http://agsmith.wordpress.com/2008/04/07/propertydescriptor-addvaluechanged-alternative/
+    /// </summary>
+    internal sealed class PropertyChangeNotifier : DependencyObject, IDisposable
+    {
+        private WeakReference _propertySource;
+
+        public PropertyChangeNotifier(DependencyObject propertySource, string path)
+            : this(propertySource, new PropertyPath(path))
+        {
+        }
+
+        public PropertyChangeNotifier(DependencyObject propertySource, DependencyProperty property)
+            : this(propertySource, new PropertyPath(property))
+        {
+        }
+
+        public PropertyChangeNotifier(DependencyObject propertySource, PropertyPath property)
+        {
+            if (null == propertySource)
+            {
+                throw new ArgumentNullException("propertySource");
+            }
+            if (null == property)
+            {
+                throw new ArgumentNullException("property");
+            }
+            this._propertySource = new WeakReference(propertySource);
+            var binding = new Binding();
+            binding.Path = property;
+            binding.Mode = BindingMode.OneWay;
+            binding.Source = propertySource;
+            BindingOperations.SetBinding(this, ValueProperty, binding);
+        }
+
+        public DependencyObject PropertySource
+        {
+            get
+            {
+                try
+                {
+                    // note, it is possible that accessing the target property
+                    // will result in an exception so i’ve wrapped this check
+                    // in a try catch
+                    return this._propertySource.IsAlive
+                        ? this._propertySource.Target as DependencyObject
+                        : null;
+                }
+                catch
+                {
+                    return null;
+                }
+            }
+        }
+
+        /// <summary>
+        /// Identifies the <see cref="Value"/> dependency property
+        /// </summary>
+        public static readonly DependencyProperty ValueProperty
+            = DependencyProperty.Register("Value", typeof(object), typeof(PropertyChangeNotifier),
+                                          new FrameworkPropertyMetadata(null, new PropertyChangedCallback(OnPropertyChanged)));
+
+        /// <summary>
+        /// Returns/sets the value of the property
+        /// </summary>
+        /// <seealso cref="ValueProperty"/>
+        [Description("Returns/sets the value of the property")]
+        [Category("Behavior")]
+        [Bindable(true)]
+        public object Value
+        {
+            get { return (object)this.GetValue(ValueProperty); }
+            set { this.SetValue(ValueProperty, value); }
+        }
+
+        private static void OnPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
+        {
+            var notifier = (PropertyChangeNotifier)d;
+            if (null != notifier.ValueChanged)
+            {
+                notifier.ValueChanged(notifier.PropertySource, EventArgs.Empty);
+            }
+        }
+
+        public event EventHandler ValueChanged;
+
+        public void Dispose()
+        {
+            BindingOperations.ClearBinding(this, ValueProperty);
+        }
+    }
+}

MahApps.Metro/MahApps.Metro.NET45.csproj

@@ -171,6 +171,7 @@
     <Compile Include="Controls\Pivot.cs" />
     <Compile Include="Controls\PivotItem.cs" />
     <Compile Include="Controls\Position.cs" />
+    <Compile Include="Controls\PropertyChangeNotifier.cs" />
     <Compile Include="Controls\RangeParameterChangedEventArgs.cs" />
     <Compile Include="Controls\SafeRaise.cs" />
     <Compile Include="Controls\Helper\ScrollBarHelper.cs" />

MahApps.Metro/MahApps.Metro.csproj

@@ -135,6 +135,7 @@
     <Compile Include="Controls\Pivot.cs" />
     <Compile Include="Controls\PivotItem.cs" />
     <Compile Include="Controls\Position.cs" />
+    <Compile Include="Controls\PropertyChangeNotifier.cs" />
     <Compile Include="Controls\RangeParameterChangedEventArgs.cs" />
     <Compile Include="Controls\SafeRaise.cs" />
     <Compile Include="Controls\ScrollViewerOffsetMediator.cs" />

Mahapps.Metro.Tests/FlyoutTest.cs

@@ -1,7 +1,9 @@
-using System.Threading.Tasks;
+using System;
+using System.Threading.Tasks;
 using System.Windows;
 using System.Windows.Controls;
 using System.Windows.Media;
+using System.Windows.Threading;
 using ExposedObject;
 using MahApps.Metro.Tests.TestHelpers;
 using MahApps.Metro;
@@ -161,14 +163,14 @@ public async Task RaisesIsOpenChangedEvent()
 
             bool eventRaised = false;
 
-            window.RightFlyout.IsOpenChanged += (sender, args) =>
-            {
+            window.RightFlyout.IsOpenChanged += (sender, args) => {
                 eventRaised = true;
             };
 
             window.RightFlyout.IsOpen = true;
 
-            Assert.True(eventRaised);
+            // IsOpen fires IsOpenChangedEvent with DispatcherPriority.Background
+            window.RightFlyout.Dispatcher.BeginInvoke(DispatcherPriority.Background, new Action(() => Assert.True(eventRaised)));
         }
 
         public class ColorTest

docs/release-notes/1.0.0.md

@@ -43,3 +43,4 @@ A migration guide for the breaking changes is available here: https://github.com
 - Fixed `MetroProgressBar` System.Windows.Media.Animation Warning 6 #1664
 - Fixed opening animation of dynamically created flyouts #1665 #1655
 - Fixed `WindowSettings` SaveWindowPosition saves to position 0,0 if window not shown #1671 #1672
+- Fixed memory leak and exception with dynamically created Flyouts (mostly happens with Caliburn) #1681