Remove the WeakEventManager, as it was too complex

It's replaced with a BindWeak extension method, similar to the existing
Bind. This rejects delegates with compiler-generated targets

Stylet/PropertyChangedExtensions.cs

@@ -1,11 +1,13 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
+using System.Diagnostics;
 using System.Linq;
 using System.Linq.Expressions;
 using System.Runtime.CompilerServices;
 using System.Text;
 using System.Threading.Tasks;
+using System.Reflection;
 
 namespace Stylet
 {
@@ -46,6 +48,60 @@ namespace Stylet
             }
         }
 
+        internal class WeakPropertyChangedHandler<TSource, TProperty> : IEventBinding where TSource : class, INotifyPropertyChanged
+        {
+            private readonly WeakReference<TSource> source;
+            private Action<TProperty> handler;
+            private string propertyName;
+            private Func<TSource, TProperty> valueSelector;
+
+            public WeakPropertyChangedHandler(TSource source, Expression<Func<TSource, TProperty>> selector, Action<TProperty> handler)
+            {
+                // We keep a strong reference to the handler, and have the PropertyChangedEventManager keep a weak
+                // reference to us. This means that anyone retaining us will also retain the handler.
+
+                this.source = new WeakReference<TSource>(source);
+                this.handler = handler;
+                this.propertyName = selector.NameForProperty();
+                this.valueSelector = selector.Compile();
+
+                PropertyChangedEventManager.AddHandler(source, this.PropertyChangedHandler, this.propertyName);
+            }
+
+            private void PropertyChangedHandler(object sender, PropertyChangedEventArgs e)
+            {
+                TSource source;
+                var got = this.source.TryGetTarget(out source);
+                // We should never hit this case. The PropertyChangedeventManager shouldn't call us if the source became null
+                Debug.Assert(got);
+                this.handler(this.valueSelector(source));
+            }
+
+            public void Unbind()
+            {
+                TSource source;
+                if (this.source.TryGetTarget(out source))
+                    PropertyChangedEventManager.RemoveHandler(source, this.PropertyChangedHandler, this.propertyName);
+            }
+        }
+
+        internal class WeakPropertyChangedBinding : IEventBinding
+        {
+            private WeakReference<IEventBinding> wrappedBinding;
+
+            public WeakPropertyChangedBinding(IEventBinding wrappedBinding)
+            {
+                this.wrappedBinding = new WeakReference<IEventBinding>(wrappedBinding);
+            }
+
+            public void Unbind()
+            {
+                IEventBinding wrappedBinding;
+                if (this.wrappedBinding.TryGetTarget(out wrappedBinding))
+                    wrappedBinding.Unbind();
+            }
+        }
+
         /// <summary>
         /// Strongly bind to PropertyChanged events for a particular property on a particular object
         /// </summary>
@@ -78,5 +134,15 @@ namespace Stylet
 
             return listener;
         }
+
+        public static IEventBinding BindWeak<TBindTo, TMember>(this TBindTo target, Expression<Func<TBindTo, TMember>> targetSelector, Action<TMember> handler) where TBindTo : class, INotifyPropertyChanged
+        {
+            var attribute = handler.Target.GetType().GetCustomAttribute<CompilerGeneratedAttribute>();
+            if (attribute != null)
+                throw new InvalidOperationException("Handler passed to BindWeak refers to a compiler-generated class. You may not capture local variables in the handler");
+
+            var binding = new WeakPropertyChangedHandler<TBindTo, TMember>(target, targetSelector, handler);
+            return new WeakPropertyChangedBinding(binding);
+        }
     }
 }

Stylet/Screen.cs

@@ -30,38 +30,6 @@ namespace Stylet
             this.DisplayName = this.GetType().FullName;
         }
 
-        #region WeakEventManager
-
-        private IWeakEventManager _weakEventManager;
-        /// <summary>
-        /// WeakEventManager owned by this screen (lazy)
-        /// </summary>
-        protected virtual IWeakEventManager weakEventManager
-        {
-            get
-            {
-                if (this._weakEventManager == null)
-                    this._weakEventManager = new WeakEventManager();
-                return this._weakEventManager;
-            }
-        }
-
-        /// <summary>
-        /// Proxy around this.weakEventManager.BindWeak. Binds to an INotifyPropertyChanged source, in a way which doesn't cause us to be retained
-        /// </summary>
-        /// <example>this.BindWeak(objectToBindTo, x => x.PropertyToBindTo, newValue => handlerForNewValue)</example>
-        /// <param name="source">Object to observe for PropertyChanged events</param>
-        /// <param name="selector">Expression for selecting the property to observe, e.g. x => x.PropertyName</param>
-        /// <param name="handler">Handler to be called when that property changes</param>
-        /// <returns>A resource which can be used to undo the binding</returns>
-        protected virtual IEventBinding BindWeak<TSource, TProperty>(TSource source, Expression<Func<TSource, TProperty>> selector, Action<TProperty> handler)
-            where TSource : class, INotifyPropertyChanged
-        {
-            return this.weakEventManager.BindWeak(source, selector, handler);
-        }
-
-        #endregion
-
         #region IHaveDisplayName
 
         private string _displayName;

Stylet/Stylet.csproj

@@ -59,7 +59,6 @@
     <Compile Include="MessageBox.cs" />
     <Compile Include="StyletConductorExtensions.cs" />
     <Compile Include="ValidatingModelBase.cs" />
-    <Compile Include="WeakEventManager.cs" />
     <Compile Include="Xaml\ActionExtension.cs" />
     <Compile Include="AssemblySource.cs" />
     <Compile Include="BindableCollection.cs" />

Stylet/WeakEventManager.cs

@@ -1,118 +0,0 @@
-using System;
-using System.Collections.Generic;
-using System.ComponentModel;
-using System.Diagnostics;
-using System.Linq;
-using System.Linq.Expressions;
-using System.Text;
-using System.Threading.Tasks;
-
-namespace Stylet
-{
-    /// <summary>
-    /// A manager capable of creating a weak event subscription for INotifyPropertyChanged events from a source to a subscriber. Manager MUST be owned by the subscriber.
-    /// </summary>
-    public interface IWeakEventManager
-    {
-        /// <summary>
-        /// Create a weak event subscription from the source, to the given handler
-        /// </summary>
-        /// <typeparam name="TSource">Type of the source</typeparam>
-        /// <typeparam name="TProperty">Type of the property to subscribe to on the source</typeparam>
-        /// <param name="source">Source object, whic implements INotifyPropertyChanged, to subscribe to</param>
-        /// <param name="selector">Describes which property to observe, e.g. (x => x.SomeProperty)</param>
-        /// <param name="handler">Callback to be called whenever the property changes. Is passed the new value of the property</param>
-        /// <returns>An event binding, which can be used to unregister the subscription</returns>
-        IEventBinding BindWeak<TSource, TProperty>(TSource source, Expression<Func<TSource, TProperty>> selector, Action<TProperty> handler)
-            where TSource : class, INotifyPropertyChanged;
-    }
-
-    internal class WeakPropertyBinding<TSource, TProperty> : IEventBinding where TSource : class, INotifyPropertyChanged
-    {
-        // Make sure we don't end up retaining the source
-        private readonly WeakReference<TSource> source;
-        private readonly string propertyName;
-        private readonly Func<TSource, TProperty> valueSelector;
-        private readonly Action<TProperty> handler;
-        private readonly Action<IEventBinding> remover;
-
-        public WeakPropertyBinding(TSource source, Expression<Func<TSource, TProperty>> selector, Action<TProperty> handler, Action<IEventBinding> remover)
-        {
-            this.source = new WeakReference<TSource>(source);
-            this.propertyName = selector.NameForProperty();
-            this.valueSelector = selector.Compile();
-            this.handler = handler;
-            this.remover = remover;
-
-            PropertyChangedEventManager.AddHandler(source, this.PropertyChangedHandler, this.propertyName);
-        }
-
-        internal void PropertyChangedHandler(object sender, PropertyChangedEventArgs e)
-        {
-            TSource source;
-            var got = this.source.TryGetTarget(out source);
-            // We should never hit this case. The PropertyChangedeventManager shouldn't call us if the source became null
-            Debug.Assert(got);
-            if (got)
-                this.handler(this.valueSelector(source));
-        }
-
-        public void Unbind()
-        {
-            TSource source;
-            if (this.source.TryGetTarget(out source))
-                PropertyChangedEventManager.RemoveHandler(source, this.PropertyChangedHandler, this.propertyName);
-            this.remover(this);
-        }
-    }
-
-    /// <summary>
-    /// Default implementation of IWeakEventManager: a manager capable of creating a weak event subscription for INotifyPropertyChanged events from a source to a subscriber. Manager MUST be owned by the subscriber.
-    /// </summary>
-    public class WeakEventManager : IWeakEventManager
-    {
-        private object bindingsLock = new object();
-        private List<IEventBinding> bindings = new List<IEventBinding>();
-
-        /// <summary>
-        /// Create a weak event subscription from the source, to the given handler
-        /// </summary>
-        /// <typeparam name="TSource">Type of the source</typeparam>
-        /// <typeparam name="TProperty">Type of the property to subscribe to on the source</typeparam>
-        /// <param name="source">Source object, whic implements INotifyPropertyChanged, to subscribe to</param>
-        /// <param name="selector">Describes which property to observe, e.g. (x => x.SomeProperty)</param>
-        /// <param name="handler">Callback to be called whenever the property changes. Is passed the new value of the property</param>
-        /// <returns>An event binding, which can be used to unregister the subscription</returns>
-        public IEventBinding BindWeak<TSource, TProperty>(TSource source, Expression<Func<TSource, TProperty>> selector, Action<TProperty> handler)
-            where TSource : class, INotifyPropertyChanged
-        {
-            // So, the handler's target might point to the class that owns us, or it might point to a compiler-generated class
-            // We assume we're owned by whatever determines how long the handler's target should live for
-            // Therefore we'll retain the handler's target for as long as we're alive (unless it's unregistered)
-
-            // The PropertyChangedEventManager is safe to use with delegates whose targets aren't compiler-generated, so we can
-            // ensure we provide a delegate with a non-compiler-generated target.
-            // To do this, we'll create a new WeakPropertyBinding instance, and retain it ourselves (so it lives as long as we do,
-            // and therefore as long as the thing that owns us does). The PropertyChangedEventManager will have a weak reference to 
-            // the WeakPropertyBinding instance, so once we release it, it will too.
-
-            var propertyName = selector.NameForProperty();
-
-            var binding = new WeakPropertyBinding<TSource, TProperty>(source, selector, handler, this.Remove);
-            lock (this.bindingsLock)
-            {
-                this.bindings.Add(binding);
-            }
-
-            return binding;
-        }
-
-        internal void Remove(IEventBinding binding)
-        {
-            lock (this.bindingsLock)
-            {
-                this.bindings.Remove(binding);
-            }
-        }
-    }
-}

StyletUnitTests/PropertyChangedExtensionsTests.cs

@@ -36,7 +36,6 @@ namespace StyletUnitTests
         class BindingClass
         {
             public string LastFoo;
-            private WeakEventManager weakEventManager = new WeakEventManager();
 
             public IEventBinding BindStrong(NotifyingClass notifying)
             {
@@ -46,16 +45,24 @@ namespace StyletUnitTests
 
             public IEventBinding BindWeak(NotifyingClass notifying)
             {
-                return this.weakEventManager.BindWeak(notifying, x => x.Foo, x => this.LastFoo = x);
+                return notifying.BindWeak(x => x.Foo, x => this.LastFoo = x);
             }
         }
 
+        private string newVal;
+
         [TestFixtureSetUp]
         public void SetUpFixture()
         {
             Execute.TestExecuteSynchronously = true;
         }
 
+        [SetUp]
+        public void SetUp()
+        {
+            this.newVal = null;
+        }
+
         [Test]
         public void StrongBindingBinds()
         {
@@ -120,38 +127,32 @@ namespace StyletUnitTests
         [Test]
         public void WeakBindingBinds()
         {
-            var manager = new WeakEventManager();
-            string newVal = null;
             var c1 = new NotifyingClass();
-            manager.BindWeak(c1, x => x.Foo, x => newVal = x);
+            c1.BindWeak(x => x.Foo, x => this.newVal = x);
             c1.Foo = "bar";
 
-            Assert.AreEqual("bar", newVal);
+            Assert.AreEqual("bar", this.newVal);
         }
 
         [Test]
         public void WeakBindingIgnoresOtherProperties()
         {
-            var manager = new WeakEventManager();
-            string newVal = null;
             var c1 = new NotifyingClass();
-            manager.BindWeak(c1, x => x.Bar, x => newVal = x);
+            c1.BindWeak(x => x.Bar, x => this.newVal = x);
             c1.Foo = "bar";
 
-            Assert.AreEqual(null, newVal);
+            Assert.IsNull(this.newVal);
         }
 
         [Test]
         public void WeakBindingListensToEmptyString()
         {
-            var manager = new WeakEventManager();
-            string newVal = null;
             var c1 = new NotifyingClass();
             c1.Bar = "bar";
-            manager.BindWeak(c1, x => x.Bar, x => newVal = x);
+            c1.BindWeak(x => x.Bar, x => this.newVal = x);
             c1.NotifyAll();
 
-            Assert.AreEqual("bar", newVal);
+            Assert.AreEqual("bar", this.newVal);
         }
 
         [Test]
@@ -177,7 +178,7 @@ namespace StyletUnitTests
             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
+            // Retain binder, as that shouldn't affect anything
             var binder = binding.BindWeak(notifying);
 
             notifying = null;
@@ -188,14 +189,20 @@ namespace StyletUnitTests
         [Test]
         public void WeakBindingUnbinds()
         {
-            var manager = new WeakEventManager();
-            string newVal = null;
             var c1 = new NotifyingClass();
-            var binding = manager.BindWeak(c1, x => x.Bar, x => newVal = x);
+            var binding = c1.BindWeak(x => x.Bar, x => this.newVal = x);
             binding.Unbind();
             c1.Bar = "bar";
 
-            Assert.AreEqual(null, newVal);
+            Assert.IsNull(this.newVal);
+        }
+        
+        [Test]
+        public void BindWeakThrowsIfTargetIsCompilerGenerated()
+        {
+            var c1 = new NotifyingClass();
+            string newVal = null;
+            Assert.Throws<InvalidOperationException>(() => c1.BindWeak(x => x.Foo, x => newVal = x));
         }
     }
 }

StyletUnitTests/ScreenTests.cs

@@ -23,11 +23,6 @@ namespace StyletUnitTests
                 set { base.validator = value; }
             }
 
-            public IWeakEventManager WeakEventManager
-            {
-                get { return base.weakEventManager; }
-            }
-
             public MyScreen() { }
             public MyScreen(IModelValidator validator) : base(validator) { }
 
@@ -62,21 +57,6 @@ namespace StyletUnitTests
             }
         }
 
-        private class WeakEventScreen : Screen
-        {
-            public IWeakEventManager WeakEventManager;
-            protected override IWeakEventManager weakEventManager
-            {
-                get { return this.WeakEventManager; }
-            }
-
-            public new IEventBinding BindWeak<TSource, TProperty>(TSource source, Expression<Func<TSource, TProperty>> selector, Action<TProperty> handler)
-                where TSource : class, INotifyPropertyChanged
-            {
-                return base.BindWeak(source, selector, handler);
-            }
-        }
-
         private MyScreen screen;
 
         [TestFixtureSetUp]
@@ -279,28 +259,5 @@ namespace StyletUnitTests
             var screen = new MyScreen(adapter.Object);
             Assert.AreEqual(adapter.Object, screen.Validator);
         }
-
-        [Test]
-        public void WeakEventManagerReturnsConsistentObject()
-        {
-            var w1 = screen.WeakEventManager;
-            var w2 = screen.WeakEventManager;
-            Assert.AreEqual(w1, w2);
-        }
-
-        [Test]
-        public void BindWeakProxies()
-        {
-            var s = new WeakEventScreen();
-            var m = new Mock<IWeakEventManager>();
-            s.WeakEventManager = m.Object;
-
-            var source = new LabelledValue<int>("test", 5);
-            Expression<Func<LabelledValue<int>, int>> selector = x => x.Value;
-            Action<int> handler = x => { };
-            s.BindWeak(source, selector, handler);
-
-            m.Verify(x => x.BindWeak(source, selector, handler));
-        }
     }
 }