diff --git a/src/Markup/Perspex.Markup/Data/ExpressionNode.cs b/src/Markup/Perspex.Markup/Data/ExpressionNode.cs index c138050cb4..100bb3d07f 100644 --- a/src/Markup/Perspex.Markup/Data/ExpressionNode.cs +++ b/src/Markup/Perspex.Markup/Data/ExpressionNode.cs @@ -8,46 +8,55 @@ namespace Perspex.Markup.Data { internal abstract class ExpressionNode : IObservable { - private object _target; + protected static readonly WeakReference UnsetReference = + new WeakReference(PerspexProperty.UnsetValue); + + private WeakReference _target; private Subject _subject; - private object _value = PerspexProperty.UnsetValue; + private WeakReference _value = UnsetReference; public ExpressionNode Next { get; set; } - public object Target + public WeakReference Target { - get { return _target; } + get + { + return _target; + } set { - if (!object.Equals(value, _target)) + var newInstance = value?.Target; + var oldInstance = _target?.Target; + + if (!object.Equals(oldInstance, newInstance)) { - if (_target != null) + if (oldInstance != null) { - Unsubscribe(_target); + Unsubscribe(oldInstance); } _target = value; - if (_target != null) + if (newInstance != null) { SubscribeAndUpdate(_target); } else { - CurrentValue = PerspexProperty.UnsetValue; + CurrentValue = UnsetReference; } if (Next != null) { - Next.Target = CurrentValue; + Next.Target = _value; } } } } - public object CurrentValue + public WeakReference CurrentValue { get { @@ -63,7 +72,7 @@ namespace Perspex.Markup.Data Next.Target = value; } - _subject?.OnNext(value); + _subject?.OnNext(value.Target); } } @@ -85,14 +94,14 @@ namespace Perspex.Markup.Data _subject = new Subject(); } - observer.OnNext(CurrentValue); + observer.OnNext(CurrentValue.Target); return _subject.Subscribe(observer); } } - protected virtual void SubscribeAndUpdate(object target) + protected virtual void SubscribeAndUpdate(WeakReference reference) { - CurrentValue = target; + CurrentValue = reference; } protected virtual void Unsubscribe(object target) diff --git a/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs b/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs index 097e1cb3cf..8bbedd52cc 100644 --- a/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs +++ b/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs @@ -27,7 +27,7 @@ namespace Perspex.Markup.Data new InpcPropertyAccessorPlugin(), }; - private readonly object _root; + private readonly WeakReference _root; private readonly Func _rootGetter; private readonly IObservable _rootObservable; private readonly IObservable _update; @@ -45,7 +45,7 @@ namespace Perspex.Markup.Data { Contract.Requires(expression != null); - _root = root; + _root = new WeakReference(root); if (!string.IsNullOrWhiteSpace(expression)) { @@ -115,7 +115,7 @@ namespace Perspex.Markup.Data if (_rootGetter != null && _node != null) { - _node.Target = _rootGetter(); + _node.Target = new WeakReference(_rootGetter()); } try @@ -155,7 +155,7 @@ namespace Perspex.Markup.Data } else { - return _root?.GetType(); + return _root.Target?.GetType(); } } finally @@ -220,7 +220,9 @@ namespace Perspex.Markup.Data { if (_update == null) { - return Observable.Never().StartWith(_root).Subscribe(observer); + return Observable.Never() + .StartWith(_root.Target) + .Subscribe(observer); } else { @@ -238,16 +240,18 @@ namespace Perspex.Markup.Data { if (_rootGetter != null) { - _node.Target = _rootGetter(); + _node.Target = new WeakReference(_rootGetter()); if (_update != null) { - _updateSubscription = _update.Subscribe(x => _node.Target = _rootGetter()); + _updateSubscription = _update.Subscribe(x => + _node.Target = new WeakReference(_rootGetter())); } } else if (_rootObservable != null) { - _rootObserverSubscription = _rootObservable.Subscribe(x => _node.Target = x); + _rootObserverSubscription = _rootObservable.Subscribe(x => + _node.Target = new WeakReference(x)); } else { diff --git a/src/Markup/Perspex.Markup/Data/IndexerNode.cs b/src/Markup/Perspex.Markup/Data/IndexerNode.cs index 7a6dd39301..a91536a30a 100644 --- a/src/Markup/Perspex.Markup/Data/IndexerNode.cs +++ b/src/Markup/Perspex.Markup/Data/IndexerNode.cs @@ -22,9 +22,11 @@ namespace Perspex.Markup.Data public IList Arguments { get; } - protected override void SubscribeAndUpdate(object target) + protected override void SubscribeAndUpdate(WeakReference reference) { - CurrentValue = GetValue(target); + object target = reference.Target; + + CurrentValue = new WeakReference(GetValue(target)); var incc = target as INotifyCollectionChanged; @@ -50,7 +52,7 @@ namespace Perspex.Markup.Data } if (typeInfo.GetDeclaredProperty(e.PropertyName).GetIndexParameters().Any()) { - CurrentValue = GetValue(sender); + CurrentValue = new WeakReference(GetValue(sender)); } } @@ -105,7 +107,7 @@ namespace Perspex.Markup.Data if (update) { - CurrentValue = GetValue(sender); + CurrentValue = new WeakReference(GetValue(sender)); } } diff --git a/src/Markup/Perspex.Markup/Data/Plugins/IPropertyAccessorPlugin.cs b/src/Markup/Perspex.Markup/Data/Plugins/IPropertyAccessorPlugin.cs index 012d28eaed..7bbdb0ff68 100644 --- a/src/Markup/Perspex.Markup/Data/Plugins/IPropertyAccessorPlugin.cs +++ b/src/Markup/Perspex.Markup/Data/Plugins/IPropertyAccessorPlugin.cs @@ -6,7 +6,7 @@ using System; namespace Perspex.Markup.Data.Plugins { /// - /// Defines how a member is read, written and observed by a + /// Defines how a member is read, written and observed by an /// . /// public interface IPropertyAccessorPlugin @@ -14,20 +14,23 @@ namespace Perspex.Markup.Data.Plugins /// /// Checks whether this plugin can handle accessing the properties of the specified object. /// - /// The object. + /// A weak reference to the object. /// True if the plugin can handle the object; otherwise false. - bool Match(object instance); + bool Match(WeakReference reference); /// /// Starts monitoring the value of a property on an object. /// - /// The object. + /// A weak reference to the object. /// The property name. /// A function to call when the property changes. /// /// An interface through which future interactions with the /// property will be made, or null if the property was not found. /// - IPropertyAccessor Start(object instance, string propertyName, Action changed); + IPropertyAccessor Start( + WeakReference reference, + string propertyName, + Action changed); } } diff --git a/src/Markup/Perspex.Markup/Data/Plugins/InpcPropertyAccessorPlugin.cs b/src/Markup/Perspex.Markup/Data/Plugins/InpcPropertyAccessorPlugin.cs index c52d1a3a26..a35edc2572 100644 --- a/src/Markup/Perspex.Markup/Data/Plugins/InpcPropertyAccessorPlugin.cs +++ b/src/Markup/Perspex.Markup/Data/Plugins/InpcPropertyAccessorPlugin.cs @@ -6,6 +6,7 @@ using System.ComponentModel; using System.Linq; using System.Reactive.Linq; using System.Reflection; +using Perspex.Utilities; namespace Perspex.Markup.Data.Plugins { @@ -18,11 +19,11 @@ namespace Perspex.Markup.Data.Plugins /// /// Checks whether this plugin can handle accessing the properties of the specified object. /// - /// The object. + /// The object. /// True if the plugin can handle the object; otherwise false. - public bool Match(object instance) + public bool Match(WeakReference reference) { - Contract.Requires(instance != null); + Contract.Requires(reference != null); return true; } @@ -30,24 +31,28 @@ namespace Perspex.Markup.Data.Plugins /// /// Starts monitoring the value of a property on an object. /// - /// The object. + /// The object. /// The property name. /// A function to call when the property changes. /// /// An interface through which future interactions with the /// property will be made, or null if the property was not found. /// - public IPropertyAccessor Start(object instance, string propertyName, Action changed) + public IPropertyAccessor Start( + WeakReference reference, + string propertyName, + Action changed) { - Contract.Requires(instance != null); + Contract.Requires(reference != null); Contract.Requires(propertyName != null); Contract.Requires(changed != null); + var instance = reference.Target; var p = instance.GetType().GetRuntimeProperties().FirstOrDefault(_ => _.Name == propertyName); if (p != null) { - return new Accessor(instance, p, changed); + return new Accessor(reference, p, changed); } else { @@ -55,40 +60,49 @@ namespace Perspex.Markup.Data.Plugins } } - private class Accessor : IPropertyAccessor + private class Accessor : IPropertyAccessor, IWeakSubscriber { - private readonly object _instance; + private readonly WeakReference _reference; private readonly PropertyInfo _property; private readonly Action _changed; - public Accessor(object instance, PropertyInfo property, Action changed) + public Accessor( + WeakReference reference, + PropertyInfo property, + Action changed) { - Contract.Requires(instance != null); + Contract.Requires(reference != null); Contract.Requires(property != null); - _instance = instance; + _reference = reference; _property = property; _changed = changed; - var inpc = instance as INotifyPropertyChanged; + var inpc = reference.Target as INotifyPropertyChanged; if (inpc != null) { - inpc.PropertyChanged += PropertyChanged; + WeakSubscriptionManager.Subscribe( + inpc, + nameof(inpc.PropertyChanged), + this); } } public Type PropertyType => _property.PropertyType; - public object Value => _property.GetValue(_instance); + public object Value => _property.GetValue(_reference.Target); public void Dispose() { - var inpc = _instance as INotifyPropertyChanged; + var inpc = _reference.Target as INotifyPropertyChanged; if (inpc != null) { - inpc.PropertyChanged -= PropertyChanged; + //WeakSubscriptionManager.Unsubscribe( + // inpc, + // nameof(inpc.PropertyChanged), + // this); } } @@ -96,14 +110,14 @@ namespace Perspex.Markup.Data.Plugins { if (_property.CanWrite) { - _property.SetValue(_instance, value); + _property.SetValue(_reference.Target, value); return true; } return false; } - private void PropertyChanged(object sender, PropertyChangedEventArgs e) + public void OnEvent(object sender, PropertyChangedEventArgs e) { if (e.PropertyName == _property.Name || string.IsNullOrEmpty(e.PropertyName)) { diff --git a/src/Markup/Perspex.Markup/Data/Plugins/PerspexPropertyAccessorPlugin.cs b/src/Markup/Perspex.Markup/Data/Plugins/PerspexPropertyAccessorPlugin.cs index a5b9f00371..b8e7cadfc1 100644 --- a/src/Markup/Perspex.Markup/Data/Plugins/PerspexPropertyAccessorPlugin.cs +++ b/src/Markup/Perspex.Markup/Data/Plugins/PerspexPropertyAccessorPlugin.cs @@ -16,37 +16,41 @@ namespace Perspex.Markup.Data.Plugins /// /// Checks whether this plugin can handle accessing the properties of the specified object. /// - /// The object. + /// A weak reference to the object. /// True if the plugin can handle the object; otherwise false. - public bool Match(object instance) + public bool Match(WeakReference reference) { - Contract.Requires(instance != null); + Contract.Requires(reference != null); - return instance is PerspexObject; + return reference.Target is PerspexObject; } /// /// Starts monitoring the value of a property on an object. /// - /// The object. + /// A weak reference to the object. /// The property name. /// A function to call when the property changes. /// /// An interface through which future interactions with the /// property will be made, or null if the property was not found. /// - public IPropertyAccessor Start(object instance, string propertyName, Action changed) + public IPropertyAccessor Start( + WeakReference reference, + string propertyName, + Action changed) { - Contract.Requires(instance != null); + Contract.Requires(reference != null); Contract.Requires(propertyName != null); Contract.Requires(changed != null); + var instance = reference.Target; var o = (PerspexObject)instance; var p = PerspexPropertyRegistry.Instance.FindRegistered(o, propertyName); if (p != null) { - return new Accessor(o, p, changed); + return new Accessor(new WeakReference(o), p, changed); } else { @@ -56,23 +60,36 @@ namespace Perspex.Markup.Data.Plugins private class Accessor : IPropertyAccessor { - private readonly PerspexObject _instance; + private readonly WeakReference _reference; private readonly PerspexProperty _property; private IDisposable _subscription; - public Accessor(PerspexObject instance, PerspexProperty property, Action changed) + public Accessor( + WeakReference reference, + PerspexProperty property, + Action changed) { - Contract.Requires(instance != null); + Contract.Requires(reference != null); Contract.Requires(property != null); - _instance = instance; + _reference = reference; _property = property; - _subscription = instance.GetObservable(property).Skip(1).Subscribe(changed); + _subscription = Instance.GetObservable(property).Skip(1).Subscribe(changed); + } + + public PerspexObject Instance + { + get + { + PerspexObject result; + _reference.TryGetTarget(out result); + return result; + } } public Type PropertyType => _property.PropertyType; - public object Value => _instance.GetValue(_property); + public object Value => Instance.GetValue(_property); public void Dispose() { @@ -84,7 +101,7 @@ namespace Perspex.Markup.Data.Plugins { if (!_property.IsReadOnly) { - _instance.SetValue(_property, value); + Instance.SetValue(_property, value); return true; } diff --git a/src/Markup/Perspex.Markup/Data/PropertyAccessorNode.cs b/src/Markup/Perspex.Markup/Data/PropertyAccessorNode.cs index 546b38ab0b..9208c9d081 100644 --- a/src/Markup/Perspex.Markup/Data/PropertyAccessorNode.cs +++ b/src/Markup/Perspex.Markup/Data/PropertyAccessorNode.cs @@ -43,15 +43,17 @@ namespace Perspex.Markup.Data } } - protected override void SubscribeAndUpdate(object target) + protected override void SubscribeAndUpdate(WeakReference reference) { - if (target != null) + var instance = reference.Target; + + if (instance != null) { - var plugin = ExpressionObserver.PropertyAccessors.FirstOrDefault(x => x.Match(target)); + var plugin = ExpressionObserver.PropertyAccessors.FirstOrDefault(x => x.Match(reference)); if (plugin != null) { - _accessor = plugin.Start(target, PropertyName, SetCurrentValue); + _accessor = plugin.Start(reference, PropertyName, SetCurrentValue); if (_accessor != null) { @@ -61,7 +63,7 @@ namespace Perspex.Markup.Data } } - CurrentValue = PerspexProperty.UnsetValue; + CurrentValue = UnsetReference; } protected override void Unsubscribe(object target) @@ -81,11 +83,11 @@ namespace Perspex.Markup.Data // We may need to make this a more general solution. if (observable != null && command == null) { - CurrentValue = PerspexProperty.UnsetValue; + CurrentValue = UnsetReference; set = true; _subscription = observable .ObserveOn(SynchronizationContext.Current) - .Subscribe(x => CurrentValue = x); + .Subscribe(x => CurrentValue = new WeakReference(x)); } else if (task != null) { @@ -95,13 +97,13 @@ namespace Perspex.Markup.Data { if (task.Status == TaskStatus.RanToCompletion) { - CurrentValue = resultProperty.GetValue(task); + CurrentValue = new WeakReference(resultProperty.GetValue(task)); set = true; } else { task.ContinueWith( - x => CurrentValue = resultProperty.GetValue(task), + x => CurrentValue = new WeakReference(resultProperty.GetValue(task)), TaskScheduler.FromCurrentSynchronizationContext()) .ConfigureAwait(false); } @@ -109,13 +111,13 @@ namespace Perspex.Markup.Data } else { - CurrentValue = value; + CurrentValue = new WeakReference(value); set = true; } if (!set) { - CurrentValue = PerspexProperty.UnsetValue; + CurrentValue = UnsetReference; } } } diff --git a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs index d336afa884..a6ed3dd858 100644 --- a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs +++ b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs @@ -174,6 +174,42 @@ namespace Perspex.Markup.UnitTests.Data Assert.Equal(expected, result); } + [Fact] + public void Should_Not_Keep_Source_Alive_ObservableCollection() + { + Func> run = () => + { + var source = new { Foo = new ObservableCollection { "foo", "bar" } }; + var target = new ExpressionObserver(source, "Foo"); + return Tuple.Create(target, new WeakReference(source.Foo)); + }; + + var result = run(); + result.Item1.Subscribe(x => { }); + + GC.Collect(); + + Assert.Null(result.Item2.Target); + } + + [Fact] + public void Should_Not_Keep_Source_Alive_NonIntegerIndexer() + { + Func> run = () => + { + var source = new NonIntegerIndexer(); + var target = new ExpressionObserver(source, "Foo"); + return Tuple.Create(target, new WeakReference(source)); + }; + + var result = run(); + result.Item1.Subscribe(x => { }); + + GC.Collect(); + + Assert.Null(result.Item2.Target); + } + private class NonIntegerIndexer : NotifyingBase { private Dictionary storage = new Dictionary(); diff --git a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs index ac82e411be..2207268bb8 100644 --- a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs +++ b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Reactive; +using System.Reactive.Linq; using System.Reactive.Subjects; using Microsoft.Reactive.Testing; using Perspex.Markup.Data; diff --git a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_PerspexProperty.cs b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_PerspexProperty.cs index b126e122d0..05093d81be 100644 --- a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_PerspexProperty.cs +++ b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_PerspexProperty.cs @@ -41,6 +41,24 @@ namespace Perspex.Markup.UnitTests.Data sub.Dispose(); } + [Fact] + public void Should_Not_Keep_Source_Alive() + { + Func> run = () => + { + var source = new Class1(); + var target = new ExpressionObserver(source, "Foo"); + return Tuple.Create(target, new WeakReference(source)); + }; + + var result = run(); + result.Item1.Subscribe(x => { }); + + GC.Collect(); + + Assert.Null(result.Item2.Target); + } + private class Class1 : PerspexObject { public static readonly StyledProperty FooProperty = diff --git a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs index a24e10f650..ee3ef3df78 100644 --- a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs +++ b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs @@ -331,6 +331,24 @@ namespace Perspex.Markup.UnitTests.Data Assert.Equal(0, second.SubscriptionCount); } + [Fact] + public void Should_Not_Keep_Source_Alive() + { + Func> run = () => + { + var source = new Class1 { Foo = "foo" }; + var target = new ExpressionObserver(source, "Foo"); + return Tuple.Create(target, new WeakReference(source)); + }; + + var result = run(); + result.Item1.Subscribe(x => { }); + + GC.Collect(); + + Assert.Null(result.Item2.Target); + } + private interface INext { int SubscriptionCount { get; }