From 19baf478aff0bc86497fc814946de1a31797716e Mon Sep 17 00:00:00 2001 From: Joeri Hendriks Date: Tue, 16 Apr 2024 11:20:58 +0200 Subject: [PATCH 1/3] Fix an event handler leak in `Binding.RemovePropertyEvent` Removing the handler in `RemovePropertyEvent` doesn't work, as the `Target` of the handler is not the `PropertyNotifyHelper` instance; it is the object that the handler is bound to. Instead, find the correct handler to remove by checking all invocations of the `PropertyChanged` event of the bound object, and comparing the internal handler of the invocation with the method's argument. Also, add unit tests to verify the correct behavior. --- src/Eto/Forms/Binding/Binding.helpers.cs | 56 +++++-- src/Eto/Forms/Binding/PropertyNotifyHelper.cs | 38 ----- .../Forms/Bindings/BindingHelpersTests.cs | 154 ++++++++++++++++++ 3 files changed, 195 insertions(+), 53 deletions(-) delete mode 100644 src/Eto/Forms/Binding/PropertyNotifyHelper.cs create mode 100644 test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs diff --git a/src/Eto/Forms/Binding/Binding.helpers.cs b/src/Eto/Forms/Binding/Binding.helpers.cs index 8e97b25387..80d4886ba9 100644 --- a/src/Eto/Forms/Binding/Binding.helpers.cs +++ b/src/Eto/Forms/Binding/Binding.helpers.cs @@ -192,9 +192,10 @@ public static IndirectBinding Property(string propertyName, bool /// public static void AddPropertyEvent(object obj, string propertyName, EventHandler eh) { - var notifyObject = obj as INotifyPropertyChanged; - if (notifyObject != null) - new PropertyNotifyHelper(notifyObject, propertyName).Changed += eh; + if (obj is not INotifyPropertyChanged notifyObject) + return; + + notifyObject.PropertyChanged += (s, e) => OnPropertyChanged(s, e, propertyName, eh); } /// @@ -211,13 +212,11 @@ public static void AddPropertyEvent(object obj, string propertyName, EventHandle /// public static void AddPropertyEvent(T obj, Expression> propertyExpression, EventHandler eh) { - var notifyObject = obj as INotifyPropertyChanged; - if (notifyObject != null) - { - var propertyInfo = propertyExpression.GetMemberInfo(); - if (propertyInfo != null) - new PropertyNotifyHelper(notifyObject, propertyInfo.Member.Name).Changed += eh; - } + var propertyInfo = propertyExpression.GetMemberInfo(); + if (propertyInfo == null) + return; + + AddPropertyEvent(obj, propertyInfo.Member.Name, eh); } /// @@ -228,12 +227,39 @@ public static void AddPropertyEvent(T obj, Expression public static void RemovePropertyEvent(object obj, EventHandler eh) { - var helper = eh.Target as PropertyNotifyHelper; - if (helper != null) + if (obj is not INotifyPropertyChanged notifyObject) + return; + + var propertyChangedField = GetPropertyChangedField(notifyObject); + if (propertyChangedField == null) + return; + + var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType() ?? Enumerable.Empty(); + var delegateToRemove = propertyChangedDelegates.FirstOrDefault(d => d.Target?.GetType().GetField("eh")?.GetValue(d.Target)?.Equals(eh) ?? false); + if (delegateToRemove == null) + return; + + notifyObject.PropertyChanged -= delegateToRemove; + } + + static void OnPropertyChanged(object sender, PropertyChangedEventArgs args, string propertyName, EventHandler handler) + { + if (args.PropertyName != propertyName) + return; + + handler?.Invoke(sender, EventArgs.Empty); + } + + static FieldInfo GetPropertyChangedField(object obj) + { + FieldInfo field = null; + var type = obj?.GetType(); + while (field == null && type != null) { - helper.Changed -= eh; - helper.Unregister(obj); + field = type.GetField(nameof(INotifyPropertyChanged.PropertyChanged), BindingFlags.Instance | BindingFlags.NonPublic); + type = type.BaseType; } + return field; } /// @@ -289,4 +315,4 @@ public static void ExecuteCommand(object dataContext, Expression -/// Helper to turn a property changed event to an EventHandler for binding -/// -/// -/// Use and to access -/// this functionality. -/// -class PropertyNotifyHelper -{ - public string PropertyName { get; private set; } - - public event EventHandler Changed; - - public PropertyNotifyHelper(INotifyPropertyChanged obj, string propertyName) - { - PropertyName = propertyName; - obj.PropertyChanged += obj_PropertyChanged; - } - - public void Unregister(object obj) - { - var notifyObject = obj as INotifyPropertyChanged; - if (notifyObject != null) - notifyObject.PropertyChanged -= obj_PropertyChanged; - } - - void obj_PropertyChanged(object sender, PropertyChangedEventArgs e) - { - if (e.PropertyName == PropertyName) - { - if (Changed != null) - Changed(sender, EventArgs.Empty); - } - } - -} \ No newline at end of file diff --git a/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs b/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs new file mode 100644 index 0000000000..c5ebcba8b2 --- /dev/null +++ b/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs @@ -0,0 +1,154 @@ +using NUnit.Framework; + +namespace Eto.Test.UnitTests.Forms.Bindings; + +[TestFixture] +public class BindingHelpersTests +{ + [Test] + public void AddingAPropertyEventByNameShouldWork() + { + var propertyValueChanged = false; + void Handler(object sender, EventArgs e) => propertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true }; + Binding.AddPropertyEvent(bindObject, "BoolProperty", Handler); + + // Act + bindObject.BoolProperty = false; + + // Assert + Assert.That(propertyValueChanged, Is.True); + } + + [Test] + public void AddingAPropertyEventByExpressionShouldWork() + { + var propertyValueChanged = false; + void Handler(object sender, EventArgs e) => propertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + + // Act + bindObject.BoolProperty = false; + + // Assert + Assert.That(propertyValueChanged, Is.True); + } + + [Test] + public void AddingMultiplePropertyEventsShouldWork() + { + var boolPropertyValueChanged = false; + var intPropertyValueChanged = false; + var stringPropertyValueChanged = false; + void BoolHandler(object sender, EventArgs e) => boolPropertyValueChanged = true; + void IntHandler(object sender, EventArgs e) => intPropertyValueChanged = true; + void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + + // Act + bindObject.BoolProperty = false; + bindObject.IntProperty = 4; + bindObject.StringProperty = "Test2"; + + // Assert + Assert.That(boolPropertyValueChanged, Is.True); + Assert.That(intPropertyValueChanged, Is.True); + Assert.That(stringPropertyValueChanged, Is.True); + } + + [Test] + public void APropertyEventShouldNotRespondToADifferentProperty() + { + var propertyValueChanged = false; + void Handler(object sender, EventArgs e) => propertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 1 }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + + // Act + bindObject.IntProperty = 2; + + // Assert + Assert.That(propertyValueChanged, Is.False); + } + + [Test] + public void RemovingAPropertyEventShouldWork() + { + var propertyValueChanged = false; + void Handler(object sender, EventArgs e) => propertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + + // Act + Binding.RemovePropertyEvent(bindObject, Handler); + bindObject.BoolProperty = false; + + // Assert + Assert.That(propertyValueChanged, Is.False); + } + + [Test] + public void RemovingAPropertyEventShouldKeepOtherEvents() + { + var boolPropertyValueChanged = false; + var intPropertyValueChanged = false; + var stringPropertyValueChanged = false; + void BoolHandler(object sender, EventArgs e) => boolPropertyValueChanged = true; + void IntHandler(object sender, EventArgs e) => intPropertyValueChanged = true; + void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + + // Act + Binding.RemovePropertyEvent(bindObject, IntHandler); + bindObject.BoolProperty = false; + bindObject.IntProperty = 4; + bindObject.StringProperty = "Test2"; + + // Assert + Assert.That(boolPropertyValueChanged, Is.True); + Assert.That(intPropertyValueChanged, Is.False); + Assert.That(stringPropertyValueChanged, Is.True); + } + + [Test] + public void RemovingAllPropertyEventsShouldWork() + { + var boolPropertyValueChanged = false; + var intPropertyValueChanged = false; + var stringPropertyValueChanged = false; + void BoolHandler(object sender, EventArgs e) => boolPropertyValueChanged = true; + void IntHandler(object sender, EventArgs e) => intPropertyValueChanged = true; + void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + + // Act + Binding.RemovePropertyEvent(bindObject, StringHandler); + Binding.RemovePropertyEvent(bindObject, BoolHandler); + Binding.RemovePropertyEvent(bindObject, IntHandler); + bindObject.BoolProperty = false; + bindObject.IntProperty = 4; + bindObject.StringProperty = "Test2"; + + // Assert + Assert.That(boolPropertyValueChanged, Is.False); + Assert.That(intPropertyValueChanged, Is.False); + Assert.That(stringPropertyValueChanged, Is.False); + } +} From f9b4a8572f8613e3ac4f25904b473252f70c56f7 Mon Sep 17 00:00:00 2001 From: Curtis Wensley Date: Thu, 16 May 2024 10:18:20 -0700 Subject: [PATCH 2/3] Update so it works to remove a single event vs. only one when hooked to the same delegate. --- src/Eto/Forms/Binding/Binding.helpers.cs | 119 +++++++++++++----- src/Eto/Forms/Binding/PropertyNotifyHelper.cs | 49 ++++++++ .../Forms/Bindings/BindingHelpersTests.cs | 79 ++++++++++-- 3 files changed, 210 insertions(+), 37 deletions(-) create mode 100644 src/Eto/Forms/Binding/PropertyNotifyHelper.cs diff --git a/src/Eto/Forms/Binding/Binding.helpers.cs b/src/Eto/Forms/Binding/Binding.helpers.cs index 80d4886ba9..88a67067ed 100644 --- a/src/Eto/Forms/Binding/Binding.helpers.cs +++ b/src/Eto/Forms/Binding/Binding.helpers.cs @@ -189,13 +189,18 @@ public static IndirectBinding Property(string propertyName, bool /// INotifyPropertyChanged object to attach the event handler to /// Name of the property to trigger the changed event. /// Event handler delegate to trigger when the specified property changes - /// - public static void AddPropertyEvent(object obj, string propertyName, EventHandler eh) + /// Object to pass to RemovePropertyEvent to unregister the event + /// + /// + public static object AddPropertyEvent(object obj, string propertyName, EventHandler eh) { - if (obj is not INotifyPropertyChanged notifyObject) - return; - - notifyObject.PropertyChanged += (s, e) => OnPropertyChanged(s, e, propertyName, eh); + if (obj is INotifyPropertyChanged notifyObject) + { + var helper = new PropertyNotifyHelper(notifyObject, propertyName); + helper.Changed += eh; + return helper; + } + return null; } /// @@ -209,45 +214,99 @@ public static void AddPropertyEvent(object obj, string propertyName, EventHandle /// INotifyPropertyChanged object to attach the event handler to /// Expression to the property to trigger the changed event. /// Event handler delegate to trigger when the specified property changes - /// - public static void AddPropertyEvent(T obj, Expression> propertyExpression, EventHandler eh) + /// Object to pass to RemovePropertyEvent to unregister the event + /// + /// + public static object AddPropertyEvent(T obj, Expression> propertyExpression, EventHandler eh) { var propertyInfo = propertyExpression.GetMemberInfo(); - if (propertyInfo == null) - return; - - AddPropertyEvent(obj, propertyInfo.Member.Name, eh); + if (propertyInfo != null) + { + return AddPropertyEvent(obj, propertyInfo.Member.Name, eh); + } + return null; } /// /// Removes an event handler previously attached with the AddPropertyEvent method. /// - /// INotifyPropertyChanged object to remove the event handler from + /// + /// Note that if you pass the object that the event is attached to instea of the object returned from AddPropertyEvent, + /// then this will unsubscribe from all property handlers that point to the same delegate specified by + /// + /// Object returned from AddPropertyEvent to unsubscribe, or the object the event is subscribed to /// Event handler delegate to remove /// public static void RemovePropertyEvent(object obj, EventHandler eh) { - if (obj is not INotifyPropertyChanged notifyObject) - return; - - var propertyChangedField = GetPropertyChangedField(notifyObject); - if (propertyChangedField == null) - return; - - var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType() ?? Enumerable.Empty(); - var delegateToRemove = propertyChangedDelegates.FirstOrDefault(d => d.Target?.GetType().GetField("eh")?.GetValue(d.Target)?.Equals(eh) ?? false); - if (delegateToRemove == null) - return; + if (obj is PropertyNotifyHelper helper) + { + helper.Changed -= eh; + helper.Unregister(obj); + } + else if (obj is INotifyPropertyChanged notifyObject) + { + var propertyChangedField = GetPropertyChangedField(notifyObject); + if (propertyChangedField == null) + return; - notifyObject.PropertyChanged -= delegateToRemove; + var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType() ?? Enumerable.Empty(); + foreach (var del in propertyChangedDelegates) + { + // find ones hooked up to the PropertyNotifyHelper, regardless of property + if (del.Target is PropertyNotifyHelper h && h.IsHookedTo(eh)) + { + h.Changed -= eh; + h.Unregister(obj); + } + } + } } - - static void OnPropertyChanged(object sender, PropertyChangedEventArgs args, string propertyName, EventHandler handler) + + /// + /// Removes an event handler previously attached with the AddPropertyEvent method. + /// + /// Object returned from AddPropertyEvent to unsubscribe + /// Expression for the property to remove the event handler for + /// Event handler delegate to remove + /// + public static void RemovePropertyEvent(T obj, Expression> propertyExpression, EventHandler eh) { - if (args.PropertyName != propertyName) - return; + var propertyInfo = propertyExpression.GetMemberInfo(); + if (propertyInfo != null) + { + RemovePropertyEvent(obj, propertyInfo.Member.Name, eh); + } + } + + /// + /// Removes an event handler previously attached with the AddPropertyEvent method. + /// + /// Object returned from AddPropertyEvent to unsubscribe + /// Name of the property to remove the event handler for + /// Event handler delegate to remove + /// + public static void RemovePropertyEvent(object obj, string propertyName, EventHandler eh) + { + if (obj is INotifyPropertyChanged notifyObject) + { + // this only works when PropertyChanged is a simple public event + // if it is implemented explicitly via the interface this won't work + var propertyChangedField = GetPropertyChangedField(notifyObject); + if (propertyChangedField == null) + return; - handler?.Invoke(sender, EventArgs.Empty); + var propertyChangedDelegates = ((Delegate)propertyChangedField.GetValue(notifyObject))?.GetInvocationList().OfType() ?? Enumerable.Empty(); + foreach (var del in propertyChangedDelegates) + { + // find ones hooked up to the PropertyNotifyHelper + if (del.Target is PropertyNotifyHelper h && h.PropertyName == propertyName && h.IsHookedTo(eh)) + { + h.Changed -= eh; + h.Unregister(obj); + } + } + } } static FieldInfo GetPropertyChangedField(object obj) diff --git a/src/Eto/Forms/Binding/PropertyNotifyHelper.cs b/src/Eto/Forms/Binding/PropertyNotifyHelper.cs new file mode 100644 index 0000000000..31e5a818f9 --- /dev/null +++ b/src/Eto/Forms/Binding/PropertyNotifyHelper.cs @@ -0,0 +1,49 @@ +namespace Eto.Forms; + +/// +/// Helper to turn a property changed event to an EventHandler for binding +/// +/// +/// Use and to access +/// this functionality. +/// +class PropertyNotifyHelper +{ + public string PropertyName { get; private set; } + + public event EventHandler Changed; + + public PropertyNotifyHelper(INotifyPropertyChanged obj, string propertyName) + { + PropertyName = propertyName; + obj.PropertyChanged += obj_PropertyChanged; + } + + public void Unregister(object obj) + { + var notifyObject = obj as INotifyPropertyChanged; + if (notifyObject != null) + notifyObject.PropertyChanged -= obj_PropertyChanged; + } + + void obj_PropertyChanged(object sender, PropertyChangedEventArgs e) + { + if (e.PropertyName == PropertyName) + { + if (Changed != null) + Changed(sender, EventArgs.Empty); + } + } + + public bool IsHookedTo(EventHandler eh) + { + foreach (var invocation in Changed.GetInvocationList()) + { + if (invocation == (Delegate)eh) + { + return true; + } + } + return false; + } +} \ No newline at end of file diff --git a/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs b/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs index c5ebcba8b2..4e98dea05a 100644 --- a/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs +++ b/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs @@ -86,7 +86,7 @@ public void RemovingAPropertyEventShouldWork() void Handler(object sender, EventArgs e) => propertyValueChanged = true; var bindObject = new BindObject { BoolProperty = true }; - Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + var obj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); // Act Binding.RemovePropertyEvent(bindObject, Handler); @@ -107,9 +107,9 @@ public void RemovingAPropertyEventShouldKeepOtherEvents() void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; - Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); - Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); - Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + var intObj = Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + var stringObj = Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); // Act Binding.RemovePropertyEvent(bindObject, IntHandler); @@ -134,9 +134,9 @@ public void RemovingAllPropertyEventsShouldWork() void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; - Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); - Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); - Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + var intObj = Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + var stringObj = Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); // Act Binding.RemovePropertyEvent(bindObject, StringHandler); @@ -151,4 +151,69 @@ public void RemovingAllPropertyEventsShouldWork() Assert.That(intPropertyValueChanged, Is.False); Assert.That(stringPropertyValueChanged, Is.False); } + + [Test] + public void UnsubscribingToSameEventShouldntUnsubscribeAllWhenPassingProperty() + { + var numchanged = 0; + void Handler(object sender, EventArgs e) => numchanged++; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, Handler); + + // Act + Binding.RemovePropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + bindObject.BoolProperty = false; + bindObject.IntProperty = 4; + bindObject.StringProperty = "Test2"; + + // Assert + Assert.That(numchanged, Is.EqualTo(2)); + } + + [Test] + public void UnsubscribingToSameEventWillUnsubscribeAll() + { + var numchanged = 0; + void Handler(object sender, EventArgs e) => numchanged++; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, Handler); + + // Act + Binding.RemovePropertyEvent(bindObject, Handler); + bindObject.BoolProperty = false; + bindObject.IntProperty = 4; + bindObject.StringProperty = "Test2"; + + // Assert + // ambiguous which one to remove, so it removed all -- need to specify property or return value from AddPropertyEvent + Assert.That(numchanged, Is.EqualTo(0)); + } + + [Test] + public void UnsubscribingToAReferencedEventShouldWork() + { + var numchanged = 0; + void Handler(object sender, EventArgs e) => numchanged++; + + var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; + var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, Handler); + + // Act + Binding.RemovePropertyEvent(boolObj, Handler); + bindObject.BoolProperty = false; + bindObject.IntProperty = 4; + bindObject.StringProperty = "Test2"; + + // Assert + Assert.That(numchanged, Is.EqualTo(2)); + } + } From e79a63b1a1d599003685bbbbd95c67ec7c0a58da Mon Sep 17 00:00:00 2001 From: Curtis Wensley Date: Sun, 15 Sep 2024 09:56:21 -0700 Subject: [PATCH 3/3] Fix breaking ABI changes - return for AddPropertyEvent is now void like before to avoid ABI breaks - Use PropertyBinding to wire up property events in DelegateBinding so it supports both INotifyPropertyChanged and MyPropertyChanged events --- src/Eto/Forms/Binding/Binding.helpers.cs | 21 +++++------ src/Eto/Forms/Binding/DelegateBinding.cs | 5 +-- src/Eto/Forms/Binding/PropertyNotifyHelper.cs | 2 +- .../Forms/Bindings/BindingHelpersTests.cs | 36 ++++--------------- .../Forms/Bindings/DelegateBindingTests.cs | 33 +++++++++++++++++ 5 files changed, 52 insertions(+), 45 deletions(-) create mode 100644 test/Eto.Test/UnitTests/Forms/Bindings/DelegateBindingTests.cs diff --git a/src/Eto/Forms/Binding/Binding.helpers.cs b/src/Eto/Forms/Binding/Binding.helpers.cs index 88a67067ed..4cc77ab4c0 100644 --- a/src/Eto/Forms/Binding/Binding.helpers.cs +++ b/src/Eto/Forms/Binding/Binding.helpers.cs @@ -189,18 +189,15 @@ public static IndirectBinding Property(string propertyName, bool /// INotifyPropertyChanged object to attach the event handler to /// Name of the property to trigger the changed event. /// Event handler delegate to trigger when the specified property changes - /// Object to pass to RemovePropertyEvent to unregister the event /// /// - public static object AddPropertyEvent(object obj, string propertyName, EventHandler eh) + public static void AddPropertyEvent(object obj, string propertyName, EventHandler eh) { if (obj is INotifyPropertyChanged notifyObject) { var helper = new PropertyNotifyHelper(notifyObject, propertyName); helper.Changed += eh; - return helper; } - return null; } /// @@ -214,27 +211,25 @@ public static object AddPropertyEvent(object obj, string propertyName, EventHand /// INotifyPropertyChanged object to attach the event handler to /// Expression to the property to trigger the changed event. /// Event handler delegate to trigger when the specified property changes - /// Object to pass to RemovePropertyEvent to unregister the event /// /// - public static object AddPropertyEvent(T obj, Expression> propertyExpression, EventHandler eh) + public static void AddPropertyEvent(T obj, Expression> propertyExpression, EventHandler eh) { var propertyInfo = propertyExpression.GetMemberInfo(); if (propertyInfo != null) { - return AddPropertyEvent(obj, propertyInfo.Member.Name, eh); + AddPropertyEvent(obj, propertyInfo.Member.Name, eh); } - return null; } /// /// Removes an event handler previously attached with the AddPropertyEvent method. /// /// - /// Note that if you pass the object that the event is attached to instea of the object returned from AddPropertyEvent, - /// then this will unsubscribe from all property handlers that point to the same delegate specified by + /// Note that this will unsubscribe from all property handlers that point to the same delegate specified by . + /// Use to only unsubscribe for a single property. /// - /// Object returned from AddPropertyEvent to unsubscribe, or the object the event is subscribed to + /// Object the event is subscribed to /// Event handler delegate to remove /// public static void RemovePropertyEvent(object obj, EventHandler eh) @@ -266,7 +261,7 @@ public static void RemovePropertyEvent(object obj, EventHandler eh) /// /// Removes an event handler previously attached with the AddPropertyEvent method. /// - /// Object returned from AddPropertyEvent to unsubscribe + /// INotifyPropertyChanged object to unsubscribe from /// Expression for the property to remove the event handler for /// Event handler delegate to remove /// @@ -282,7 +277,7 @@ public static void RemovePropertyEvent(T obj, Expression /// Removes an event handler previously attached with the AddPropertyEvent method. /// - /// Object returned from AddPropertyEvent to unsubscribe + /// INotifyPropertyChanged object to unsubscribe from /// Name of the property to remove the event handler for /// Event handler delegate to remove /// diff --git a/src/Eto/Forms/Binding/DelegateBinding.cs b/src/Eto/Forms/Binding/DelegateBinding.cs index def809f6c3..73b10b46c3 100644 --- a/src/Eto/Forms/Binding/DelegateBinding.cs +++ b/src/Eto/Forms/Binding/DelegateBinding.cs @@ -221,8 +221,9 @@ public class DelegateBinding : IndirectBinding DefaultSetValue = defaultSetValue; if (!string.IsNullOrEmpty(notifyProperty)) { - AddChangeEvent = (obj, eh) => { AddPropertyEvent(obj, notifyProperty, eh); return obj; }; - RemoveChangeEvent = (obj, eh) => RemovePropertyEvent(obj, eh); + var binding = new PropertyBinding(notifyProperty); + AddChangeEvent = (obj, eh) => binding.AddValueChangedHandler(obj, eh); + RemoveChangeEvent = (obj, eh) => binding.RemoveValueChangedHandler(obj, eh); } } diff --git a/src/Eto/Forms/Binding/PropertyNotifyHelper.cs b/src/Eto/Forms/Binding/PropertyNotifyHelper.cs index 31e5a818f9..b384d1d682 100644 --- a/src/Eto/Forms/Binding/PropertyNotifyHelper.cs +++ b/src/Eto/Forms/Binding/PropertyNotifyHelper.cs @@ -5,7 +5,7 @@ namespace Eto.Forms; /// /// /// Use and to access -/// this functionality. +/// this functionality, or better yet use the class. /// class PropertyNotifyHelper { diff --git a/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs b/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs index 4e98dea05a..caf7f0a4b6 100644 --- a/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs +++ b/test/Eto.Test/UnitTests/Forms/Bindings/BindingHelpersTests.cs @@ -86,7 +86,7 @@ public void RemovingAPropertyEventShouldWork() void Handler(object sender, EventArgs e) => propertyValueChanged = true; var bindObject = new BindObject { BoolProperty = true }; - var obj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); // Act Binding.RemovePropertyEvent(bindObject, Handler); @@ -107,9 +107,9 @@ public void RemovingAPropertyEventShouldKeepOtherEvents() void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; - var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); - var intObj = Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); - var stringObj = Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); // Act Binding.RemovePropertyEvent(bindObject, IntHandler); @@ -134,9 +134,9 @@ public void RemovingAllPropertyEventsShouldWork() void StringHandler(object sender, EventArgs e) => stringPropertyValueChanged = true; var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; - var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); - var intObj = Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); - var stringObj = Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, BoolHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, IntHandler); + Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, StringHandler); // Act Binding.RemovePropertyEvent(bindObject, StringHandler); @@ -194,26 +194,4 @@ public void UnsubscribingToSameEventWillUnsubscribeAll() // ambiguous which one to remove, so it removed all -- need to specify property or return value from AddPropertyEvent Assert.That(numchanged, Is.EqualTo(0)); } - - [Test] - public void UnsubscribingToAReferencedEventShouldWork() - { - var numchanged = 0; - void Handler(object sender, EventArgs e) => numchanged++; - - var bindObject = new BindObject { BoolProperty = true, IntProperty = 3, StringProperty = "Test1" }; - var boolObj = Binding.AddPropertyEvent(bindObject, obj => obj.BoolProperty, Handler); - Binding.AddPropertyEvent(bindObject, obj => obj.IntProperty, Handler); - Binding.AddPropertyEvent(bindObject, obj => obj.StringProperty, Handler); - - // Act - Binding.RemovePropertyEvent(boolObj, Handler); - bindObject.BoolProperty = false; - bindObject.IntProperty = 4; - bindObject.StringProperty = "Test2"; - - // Assert - Assert.That(numchanged, Is.EqualTo(2)); - } - } diff --git a/test/Eto.Test/UnitTests/Forms/Bindings/DelegateBindingTests.cs b/test/Eto.Test/UnitTests/Forms/Bindings/DelegateBindingTests.cs new file mode 100644 index 0000000000..047b8c6895 --- /dev/null +++ b/test/Eto.Test/UnitTests/Forms/Bindings/DelegateBindingTests.cs @@ -0,0 +1,33 @@ +using NUnit.Framework; + +namespace Eto.Test.UnitTests.Forms.Bindings; + +[TestFixture] +public class DelegateBindingTests +{ + [Test] + public void SubscribingToPropertyChangesShouldWork() + { + int propertyValueChanged = 0; + void Handler(object sender, EventArgs e) => propertyValueChanged++; + + var bindObject = new BindObject { BoolProperty = true }; + + var binding = new DelegateBinding(o => o.BoolProperty, (o,v) => o.BoolProperty = v, nameof(BindObject.BoolProperty)); + + // wire up handler + var bindingReference = binding.AddValueChangedHandler(bindObject, Handler); + + bindObject.BoolProperty = true; + + Assert.That(propertyValueChanged, Is.EqualTo(1), "Handler should have been fired"); + + // Remove the handler binding + binding.RemoveValueChangedHandler(bindingReference, Handler); + + bindObject.BoolProperty = false; + + // Handler should no longer be triggered + Assert.That(propertyValueChanged, Is.EqualTo(1), "Handler should not fire after being removed"); + } +}