From 8d839f6649af6b7a810ef514a7b451cfeb6fac0b Mon Sep 17 00:00:00 2001 From: Jan Karger Date: Fri, 28 Nov 2014 19:28:24 +0100 Subject: [PATCH 1/5] add PropertyChangeNotifier as alternative to AddValueChanged of dependency property descriptor --- .../Controls/PropertyChangeNotifier.cs | 109 ++++++++++++++++++ MahApps.Metro/MahApps.Metro.NET45.csproj | 1 + MahApps.Metro/MahApps.Metro.csproj | 1 + 3 files changed, 111 insertions(+) create mode 100644 MahApps.Metro/Controls/PropertyChangeNotifier.cs diff --git a/MahApps.Metro/Controls/PropertyChangeNotifier.cs b/MahApps.Metro/Controls/PropertyChangeNotifier.cs new file mode 100644 index 0000000000..aae08b6bd8 --- /dev/null +++ b/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 +{ + /// + /// 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/ + /// + public 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; + } + } + } + + /// + /// Identifies the dependency property + /// + public static readonly DependencyProperty ValueProperty + = DependencyProperty.Register("Value", typeof(object), typeof(PropertyChangeNotifier), + new FrameworkPropertyMetadata(null, new PropertyChangedCallback(OnPropertyChanged))); + + /// + /// Returns/sets the value of the property + /// + /// + [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); + } + } +} \ No newline at end of file diff --git a/MahApps.Metro/MahApps.Metro.NET45.csproj b/MahApps.Metro/MahApps.Metro.NET45.csproj index c4314098e1..ed1ff51213 100644 --- a/MahApps.Metro/MahApps.Metro.NET45.csproj +++ b/MahApps.Metro/MahApps.Metro.NET45.csproj @@ -171,6 +171,7 @@ + diff --git a/MahApps.Metro/MahApps.Metro.csproj b/MahApps.Metro/MahApps.Metro.csproj index 9929ede8cf..406d81223e 100644 --- a/MahApps.Metro/MahApps.Metro.csproj +++ b/MahApps.Metro/MahApps.Metro.csproj @@ -135,6 +135,7 @@ + From 7d224d803fef60b344f309c766c462b980c24f77 Mon Sep 17 00:00:00 2001 From: Jan Karger Date: Fri, 28 Nov 2014 19:29:16 +0100 Subject: [PATCH 2/5] use PropertyChangeNotifier instead adding removing events --- MahApps.Metro/Controls/Flyout.cs | 3 ++ MahApps.Metro/Controls/FlyoutsControl.cs | 61 ++++-------------------- 2 files changed, 13 insertions(+), 51 deletions(-) diff --git a/MahApps.Metro/Controls/Flyout.cs b/MahApps.Metro/Controls/Flyout.cs index 28478dfb37..df1025aa42 100644 --- a/MahApps.Metro/Controls/Flyout.cs +++ b/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; } + /// /// Gets/sets if the title is visible in this flyout. /// diff --git a/MahApps.Metro/Controls/FlyoutsControl.cs b/MahApps.Metro/Controls/FlyoutsControl.cs index 4f59520590..ad6c7486ec 100644 --- a/MahApps.Metro/Controls/FlyoutsControl.cs +++ b/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 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 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 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) From cf4b557327016b109bb146702ee1ecd06607dd14 Mon Sep 17 00:00:00 2001 From: Jan Karger Date: Fri, 28 Nov 2014 22:03:29 +0100 Subject: [PATCH 3/5] side-fix RaisesIsOpenChangedEvent --- Mahapps.Metro.Tests/FlyoutTest.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Mahapps.Metro.Tests/FlyoutTest.cs b/Mahapps.Metro.Tests/FlyoutTest.cs index 6414f65e2a..08f1bb8bc5 100644 --- a/Mahapps.Metro.Tests/FlyoutTest.cs +++ b/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 From 5c23cef7426d06868a6a9197a25bf94489e80184 Mon Sep 17 00:00:00 2001 From: Jan Karger Date: Fri, 28 Nov 2014 22:06:40 +0100 Subject: [PATCH 4/5] PropertyChangeNotifier is now internal --- MahApps.Metro/Controls/PropertyChangeNotifier.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MahApps.Metro/Controls/PropertyChangeNotifier.cs b/MahApps.Metro/Controls/PropertyChangeNotifier.cs index aae08b6bd8..c2ce7e0c0b 100644 --- a/MahApps.Metro/Controls/PropertyChangeNotifier.cs +++ b/MahApps.Metro/Controls/PropertyChangeNotifier.cs @@ -18,7 +18,7 @@ namespace MahApps.Metro.Controls /// /// Complete implementation can be found here: http://agsmith.wordpress.com/2008/04/07/propertydescriptor-addvaluechanged-alternative/ /// - public sealed class PropertyChangeNotifier : DependencyObject, IDisposable + internal sealed class PropertyChangeNotifier : DependencyObject, IDisposable { private WeakReference _propertySource; From 3cf1feec8020c290fe83e06032653f03977da64d Mon Sep 17 00:00:00 2001 From: Jan Karger Date: Sat, 29 Nov 2014 15:46:13 +0100 Subject: [PATCH 5/5] update 1.0.0.md --- docs/release-notes/1.0.0.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/release-notes/1.0.0.md b/docs/release-notes/1.0.0.md index a6371d7d34..617ccc7f61 100644 --- a/docs/release-notes/1.0.0.md +++ b/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