Start writing tests for (and fixing) PropertyChangedExtensions

Currently a test is breaking, not sure why

Stylet/PropertyChangedBase.cs

@@ -24,15 +24,12 @@ namespace Stylet
         }
 
         protected virtual void NotifyOfPropertyChange([CallerMemberName] string propertyName = "")
-        {
-            Execute.OnUIThread(() =>
         {
             var handler = this.PropertyChanged;
             if (handler != null)
             {
                 handler(this, new PropertyChangedEventArgs(propertyName));
             }
-            });
         }
 
         protected virtual void SetAndNotify<T>(ref T field, T value, [CallerMemberName] string propertyName = "")

Stylet/PropertyChangedExtensions.cs

@@ -77,45 +77,20 @@ namespace Stylet
             }
         }
 
-        public static IPropertyChangedBinding BindWeak<TMember>(this object binder, Expression<Func<TMember>> targetSelector, Action<TMember> handler)
+        public static IPropertyChangedBinding BindWeak<TBindTo, TMember>(this TBindTo target, object binder, Expression<Func<TBindTo, TMember>> targetSelector, Action<TMember> handler) where TBindTo : class, INotifyPropertyChanged
         {
-            return BindInternal(binder, targetSelector, handler, true);
+            var propertyName = targetSelector.NameForProperty();
-        }
+            var propertyAccess = targetSelector.Compile();
+            var weakTarget = new WeakReference<TBindTo>(target);
 
-        public static IPropertyChangedBinding Bind<TMember>(this object binder, Expression<Func<TMember>> targetSelector, Action<TMember> handler)
-        {
-            return BindInternal(binder, targetSelector, handler, false);
-        }
-
-        private static IPropertyChangedBinding BindInternal<TMember>(object binder, Expression<Func<TMember>> targetSelector, Action<TMember> handler, bool weak)
-        {
-            var memberSelector = targetSelector.Body as MemberExpression;
-            if (memberSelector == null)
-                throw new ArgumentException("Must be in the form () => someInstance.SomeProperty", "targetSelector");
-
-            var propertyName = memberSelector.Member.Name;
-            var targetExpression = memberSelector.Expression as MemberExpression;
-            if (targetExpression == null)
-                throw new ArgumentException("Must be in the form () => someInstance.SomeProperty", "targetSelector");
-
-            var target = Expression.Lambda<Func<object>>(targetExpression).Compile()();
-            var inpc = target as INotifyPropertyChanged;
-            if (inpc == null)
-                throw new ArgumentException("The someInstance in () => someInstance.SomeProperty must be an INotifyPropertyChanged", "targetSelector");
-
-            var propertyAccess = Expression.Lambda<Func<TMember>>(memberSelector).Compile();
-
-            IPropertyChangedBinding listener;
-
-            if (weak)
-            {
             EventHandler<PropertyChangedEventArgs> ourHandler = (o, e) =>
             {
-                    handler(propertyAccess());
+                TBindTo strongTarget;
+                if (weakTarget.TryGetTarget(out strongTarget))
+                    handler(propertyAccess(strongTarget));
             };
 
-                WeakPropertyChangedBinding weakListener = new WeakPropertyChangedBinding(binder, inpc, propertyName, ourHandler);
+            WeakPropertyChangedBinding weakListener = new WeakPropertyChangedBinding(binder, target, propertyName, ourHandler);
-                listener = weakListener;
 
             // Right, we have target, propertyName, binder.
             // Now we have to keep the handler we're about to build (which has a reference to the handler we were passed) alive as long as
@@ -133,22 +108,32 @@ namespace Stylet
                 listeners.Add(weakListener);
             }
 
-                PropertyChangedEventManager.AddHandler(inpc, ourHandler, propertyName);
+            PropertyChangedEventManager.AddHandler(target, ourHandler, propertyName);
+
+            return weakListener;
         }
-            else
+
+        public static IPropertyChangedBinding Bind<TBindTo, TMember>(this TBindTo target, Expression<Func<TBindTo, TMember>> targetSelector, Action<TMember> handler) where TBindTo : class, INotifyPropertyChanged
         {
+            var propertyName = targetSelector.NameForProperty();
+            var propertyAccess = targetSelector.Compile();
+            // Make sure we don't capture target strongly, otherwise we'll retain it when we shouldn't
+            // If it does get released, we're released from the delegate list
+            var weakTarget = new WeakReference<TBindTo>(target);
+
             PropertyChangedEventHandler ourHandler = (o, e) =>
             {
                 if (e.PropertyName == propertyName || e.PropertyName == String.Empty)
                 {
-                        handler(propertyAccess());
+                    TBindTo strongTarget;
+                    if (weakTarget.TryGetTarget(out strongTarget))
+                        handler(propertyAccess(strongTarget));
                 }
             };
 
-                inpc.PropertyChanged += ourHandler;
+            target.PropertyChanged += ourHandler;
 
-                listener = new StrongPropertyChangedBinding(inpc, ourHandler);
+            var listener = new StrongPropertyChangedBinding(target, ourHandler);
-            }
 
             return listener;
         }

StyletUnitTests/PropertyChangedExtensionsTests.cs

@@ -0,0 +1,195 @@
+using NUnit.Framework;
+using Stylet;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace StyletUnitTests
+{
+    [TestFixture]
+    public class PropertyChangedExtensionsTests
+    {
+        class NotifyingClass : PropertyChangedBase
+        {
+            private string _foo;
+            public string Foo
+            {
+                get { return this._foo; }
+                set { SetAndNotify(ref this._foo, value); }
+            }
+
+            private string _bar;
+            public string Bar
+            {
+                get { return this._bar; }
+                set { SetAndNotify(ref this._bar, value);  }
+            }
+
+            public void NotifyAll()
+            {
+                this.NotifyOfPropertyChange(String.Empty);
+            }
+        }
+
+        class BindingClass
+        {
+            public string LastFoo;
+
+            public IPropertyChangedBinding BindStrong(NotifyingClass notifying)
+            {
+                // Must make sure the compiler doesn't generate an inner class for this, otherwise we're not testing the right thing
+                return notifying.Bind(x => x.Foo, x => this.LastFoo = x);
+            }
+
+            public IPropertyChangedBinding BindWeak(NotifyingClass notifying)
+            {
+                return notifying.BindWeak(this, x => x.Foo, x => this.LastFoo = x);
+            }
+        }
+
+        [Test]
+        public void StrongBindingBinds()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            c1.Bind(x => x.Foo, x => newVal = x);
+            c1.Foo = "bar";
+
+            Assert.AreEqual("bar", newVal);
+        }
+
+        [Test]
+        public void StrongBindingIgnoresOtherProperties()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            c1.Bind(x => x.Bar, x => newVal = x);
+            c1.Foo = "bar";
+
+            Assert.AreEqual(null, newVal);
+        }
+
+        [Test]
+        public void StrongBindingListensToEmptyString()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            c1.Bar = "bar";
+            c1.Bind(x => x.Bar, x => newVal = x);
+            c1.NotifyAll();
+
+            Assert.AreEqual("bar", newVal);
+        }
+
+        [Test]
+        public void StrongBindingRetainsBindingClass()
+        {
+            var binding = new BindingClass();
+
+            // Means of determining whether the class has been disposed
+            var weakBinding = new WeakReference<BindingClass>(binding);
+
+            var notifying = new NotifyingClass();
+            binding.BindStrong(notifying);
+
+            binding = null;
+            GC.Collect();
+            Assert.IsTrue(weakBinding.TryGetTarget(out binding));
+        }
+
+        [Test]
+        public void StrongBindingDoesNotRetainNotifier()
+        {
+            var binding = new BindingClass();
+            var notifying = new NotifyingClass();
+            // Means of determining whether the class has been disposed
+            var weakNotifying = new WeakReference<NotifyingClass>(notifying);
+            // Retain the IPropertyChangedBinding, in case that causes NotifyingClass to be retained
+            var binder = binding.BindStrong(notifying);
+
+            notifying = null;
+            GC.Collect();
+            Assert.IsFalse(weakNotifying.TryGetTarget(out notifying));
+        }
+
+        [Test]
+        public void StrongBindingUnbinds()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            var binding = c1.Bind(x => x.Bar, x => newVal = x);
+            binding.Unbind();
+            c1.Bar = "bar";
+
+            Assert.AreEqual(null, newVal);
+        }
+
+        [Test]
+        public void WeakBindingBinds()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            c1.BindWeak(this, x => x.Foo, x => newVal = x);
+            c1.Foo = "bar";
+
+            Assert.AreEqual("bar", newVal);
+        }
+
+        [Test]
+        public void WeakBindingIgnoresOtherProperties()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            c1.BindWeak(this, x => x.Bar, x => newVal = x);
+            c1.Foo = "bar";
+
+            Assert.AreEqual(null, newVal);
+        }
+
+        [Test]
+        public void WeakBindingListensToEmptyString()
+        {
+            string newVal = null;
+            var c1 = new NotifyingClass();
+            c1.Bar = "bar";
+            c1.BindWeak(this, x => x.Bar, x => newVal = x);
+            c1.NotifyAll();
+
+            Assert.AreEqual("bar", newVal);
+        }
+
+        [Test]
+        public void WeakBindingDoesNotRetainBindingClass()
+        {
+            var binding = new BindingClass();
+
+            // Means of determining whether the class has been disposed
+            var weakBinding = new WeakReference<BindingClass>(binding);
+
+            var notifying = new NotifyingClass();
+            // Retain binder, in case that affects anything
+            var binder = binding.BindWeak(notifying);
+
+            binding = null;
+            GC.Collect();
+            Assert.IsFalse(weakBinding.TryGetTarget(out binding));
+        }
+
+        [Test]
+        public void WeakBindingDoesNotRetainNotifier()
+        {
+            var binding = new BindingClass();
+            var notifying = new NotifyingClass();
+            // Means of determining whether the class has been disposed
+            var weakNotifying = new WeakReference<NotifyingClass>(notifying);
+            // Retain binder, in case that affects anything
+            var binder = binding.BindWeak(notifying);
+
+            notifying = null;
+            GC.Collect();
+            Assert.IsFalse(weakNotifying.TryGetTarget(out notifying));
+        }
+    }
+}

StyletUnitTests/StyletUnitTests.csproj

@@ -47,6 +47,7 @@
     <Compile Include="BindableCollectionTests.cs" />
     <Compile Include="EventAggregatorTests.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
+    <Compile Include="PropertyChangedExtensionsTests.cs" />
     <Compile Include="StyletIoC\StyletIoCAutobindingTests.cs" />
     <Compile Include="StyletIoC\StyletIoCBindingChecksTests.cs" />
     <Compile Include="StyletIoC\StyletIoCParameterInjectionTests.cs" />