dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

[Feature] General purpose data binding support? #15140

Open kruncher opened 9 years ago

kruncher commented 9 years ago

Please forgive me if this is the wrong place to post this; but it certainly seems like a good place :)

I think that it would be extremely useful if there were a general-purpose data binding mechanism providing a consistent way to synchronize the properties of objects making it for developers to have consistent support for patterns that benefit from data binding such as the popular MVVM and MVPVM design patterns.

Also including the IValueConverter interface and related functionality that is included in the System.Windows.Data namespace of the fully featured .NET framework but perhaps in a less platform-specific namespace like System.Data?

Whilst not applicable in the following example; perhaps a general purpose XAML implementation would also be of benefit perhaps allowing a XAML representation of a custom selection of UI controls outside the scope of Windows or Xamarin forms? I see this being of particular use in .NET Core based game engines.

Example Use Case (User Code)

For example, if the Unity game engine was using .NET Core with such a feature it would then be quite straightforward to add binding support to the Unity UI controls simply by having them implement INotifyPropertyChanged.

Programmatic Bindings

Continuing with the Unity example; some MonoView<TViewModel>-derived component could be added to the root of each composition of UI controls allowing for manual binding similar to this:

// ExampleView.cs
public class ExampleView : MonoView<ExampleViewModel> {
    [SerializeField]
    private InputField _firstNameInputField;
    ...
    protected override void OnBind() {
        Bindings.Add(new Binding(_firstNameInputField, "text", ViewModel, "FirstName", BindingMode.TwoWay));
    }
}

Where the base classes look like this:

// MonoView.cs
public abstract class MonoView : MonoBehaviour {
    public MonoView() {
        Bindings = new BindingCollection();
    }
    protected BindingCollection Bindings { get; private set; }
    ...
    protected virtual void OnBind() { }
    protected virtual void OnUnbind() {
        // Properly disposes all bindings (unsubscribes from source/target objects).
        foreach (var binding in Bindings) {
            binding.Dispose();
        }
        Bindings.Clear();
    }
}

// MonoView{T}.cs
public abstract class MonoView<TViewModel> : MonoView where TViewModel : class { ... }

Designer Specified Bindings

In addition to programmatic binding (like above) it would also be fairly straightforward to create a custom "PropertyBinder" MonoBehaviour component which could be added to UI controls to allow designers to make similar associations using just the Unity "Inspector" window.

+---------------------------------------------------------------------------+
|  Property Bindings Component:                                             |
|  +-------------------------------------------------[ Remove Binding ]--+  |
|  |                         ___________________________________________ |  |
|  | Control Property:       | text                               | \/ | |  |
|  |                         ------------------------------------------- |  |
|  |                         ___________________________________________ |  |
|  | Data Context Property:  | FirstName                          | \/ | |  |
|  |                         ------------------------------------------- |  |
|  +---------------------------------------------------------------------+  |
|  _________________________                                                |
|  |  Add Another Binding  |                                                |
|  -------------------------                                                |
+---------------------------------------------------------------------------+

Bindings properly disposed when removed/cleared from the Bindings collection

When the view is unbound from the data context all Bindings would be properly disposed allowing the view to be reused and bound to another view model.

Closing Words

I'm sure that you folks have probably already got something like this planned; but I wanted to post this feature request just in case it isn't on the roadmap :)

svick commented 9 years ago

FYI, XAML implementation has been discussed in dotnet/runtime#14862.

Givemeyourgits commented 9 years ago

Would useful especially when creating multimedia apps or games.

OtherCrashOverride commented 9 years ago

I think this needs more detail. Its currently too abstract.

[edit] The first problem area that comes to mind is WPF/Silverlight DependencyProperties. Does this replace those?

kruncher commented 9 years ago

What is Binding? What properties, methods and events is it supposed to provide? After I create a binding, what do I do with it?

The interface is similar to here: https://msdn.microsoft.com/en-us/library/system.windows.forms.binding(v=vs.110).aspx

Okay; this is a working (but rough prototype) example:

I have renamed BindingCollection to BindingManager since I feel that it would be better if it took ownership of its binding objects. BindingManager.UnbindAll can be used to instantly nuke all bindings so that the view can be recycled (which is desirable in some circumstances; like in Unity where view resources are a little more expensive to setup).

Binding Mecanism

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Reflection;

namespace Example.Bindings {

    public interface IBinding {

        /// <summary>
        /// Unsubscribe from source and target objects.
        /// </summary>
        void DestroyBinding();

        /// <summary>
        /// Force update source object from target object.
        /// </summary>
        void UpdateSourceFromTarget();

        /// <summary>
        /// Force update target object from source object.
        /// </summary>
        void UpdateTargetFromSource();

    }

    public enum BindingMode {
        /// <summary>
        /// One time binding from source to target; no event subscriptions.
        /// </summary>
        OneTime,
        /// <summary>
        /// Bind from source to target only.
        /// </summary>
        OneWay,
        /// <summary>
        /// Bind in both directions.
        /// </summary>
        TwoWay,
    }

    public sealed class Binding : IBinding {

        private static void CheckBindableArgument(string argumentName, object obj, string propertyName) {
            if (obj == null)
                throw new ArgumentNullException(argumentName + "Object");
            if (propertyName == null)
                throw new ArgumentNullException(argumentName + "PropertyName");
            if (propertyName == "")
                throw new ArgumentException("Empty string.", argumentName + "PropertyName");
        }

        private static void CheckBindingModeArgument(BindingMode bindingMode) {
            if (bindingMode != BindingMode.OneTime && bindingMode != BindingMode.OneWay && bindingMode != BindingMode.TwoWay)
                throw new ArgumentException(string.Format("Unexpected value '{0}'.", bindingMode), "bindingMode");
        }

        private object _sourceObject;
        private PropertyInfo _sourceProperty;
        private string _sourcePropertyName;
        private object _targetObject;
        private PropertyInfo _targetProperty;
        private string _targetPropertyName;

        public Binding(object sourceObject, string sourcePropertyName, object targetObject, string targetPropertyName, BindingMode bindingMode = BindingMode.TwoWay) {
            CheckBindableArgument("source", sourceObject, sourcePropertyName);
            CheckBindableArgument("target", targetObject, targetPropertyName);
            CheckBindingModeArgument(bindingMode);

            _sourceObject = sourceObject;
            _sourceProperty = sourceObject.GetType().GetProperty(sourcePropertyName, BindingFlags.Instance | BindingFlags.Public);
            _sourcePropertyName = sourcePropertyName;
            _targetObject = targetObject;
            _targetProperty = targetObject.GetType().GetProperty(targetPropertyName, BindingFlags.Instance | BindingFlags.Public);
            _targetPropertyName = targetPropertyName;

            if (_sourceProperty == null)
                throw new InvalidOperationException(string.Format("Cannot locate property '{0}' in source object.", sourcePropertyName));
            if (_targetProperty == null)
                throw new InvalidOperationException(string.Format("Cannot locate property '{0}' in target object.", targetPropertyName));

            UpdateTargetFromSource();

            if (bindingMode == BindingMode.OneTime) {
                DestroyBinding();
                return;
            }

            var observableSource = _sourceObject as INotifyPropertyChanged;
            if (observableSource != null && _targetProperty.CanWrite)
                observableSource.PropertyChanged += ObservableSource_PropertyChanged;

            if (bindingMode == BindingMode.TwoWay) {
                var observableTarget = _targetObject as INotifyPropertyChanged;
                if (observableTarget != null && _sourceProperty.CanWrite)
                    observableTarget.PropertyChanged += ObservableTarget_PropertyChanged;
            }
        }

        public void DestroyBinding() {
            var observableSource = _sourceObject as INotifyPropertyChanged;
            if (observableSource != null)
                observableSource.PropertyChanged -= ObservableSource_PropertyChanged;

            var observableTarget = _targetObject as INotifyPropertyChanged;
            if (observableTarget != null)
                observableTarget.PropertyChanged -= ObservableTarget_PropertyChanged;

            _sourceObject = null;
            _sourceProperty = null;
            _sourcePropertyName = null;
            _targetObject = null;
            _targetProperty = null;
            _targetPropertyName = null;
        }

        private void ObservableSource_PropertyChanged(object sender, PropertyChangedEventArgs e) {
            if (e.PropertyName == _sourcePropertyName)
                UpdateTargetFromSource();
        }

        private void ObservableTarget_PropertyChanged(object sender, PropertyChangedEventArgs e) {
            if (e.PropertyName == _targetPropertyName)
                UpdateSourceFromTarget();
        }

        public void UpdateSourceFromTarget() {
            var targetValue = _targetProperty.GetValue(_targetObject, null);

            //!TODO: Implement IValueConverter logic here...

            _sourceProperty.SetValue(_sourceObject, targetValue, null);
        }

        public void UpdateTargetFromSource() {
            var sourceValue = _sourceProperty.GetValue(_sourceObject, null);

            //!TODO: Implement IValueConverter logic here...

            _targetProperty.SetValue(_targetObject, sourceValue, null);
        }

    }

    public interface IBindingManager {

        int Count { get; }

        void Add(IBinding binding);

        void UnbindAll();

        bool Unbind(IBinding binding);

        int UnbindWhere(Func<IBinding, bool> predicate);

    }

    public class BindingManager : IBindingManager {

        private static void CheckBindingArgument(IBinding binding) {
            if (binding == null)
                throw new ArgumentNullException("binding");
        }

        private readonly HashSet<IBinding> _bindings = new HashSet<IBinding>();

        public int Count {
            get { return _bindings.Count; }
        }

        public void Add(IBinding binding) {
            CheckBindingArgument(binding);

            _bindings.Add(binding);
        }

        public void UnbindAll() {
            foreach (var binding in _bindings)
                binding.DestroyBinding();
            _bindings.Clear();
        }

        public bool Unbind(IBinding binding) {
            CheckBindingArgument(binding);

            bool hasRemoved = _bindings.Remove(binding);
            if (hasRemoved)
                binding.DestroyBinding();

            return hasRemoved;
        }

        public int UnbindWhere(Func<IBinding, bool> predicate) {
            int removedCount = 0;

            foreach (var binding in _bindings.Where(predicate)) {
                binding.DestroyBinding();
                _bindings.Remove(binding);
            }

            return removedCount;
        }

    }

}

Example Custom Framework

using Example.Bindings;
using System.Collections.Generic;
using System.ComponentModel;

namespace Example.CustomFramework {

    public abstract class ObservableObject : INotifyPropertyChanged {

        public event PropertyChangedEventHandler PropertyChanged;

        protected virtual void OnPropertyChanged(string propertyName) {
            var handler = PropertyChanged;
            if (handler != null)
                handler.Invoke(this, new PropertyChangedEventArgs(propertyName));
        }

        protected bool SetPropertyField<T>(ref T field, T value, string propertyName) {
            if (EqualityComparer<T>.Default.Equals(field, value))
                return false;

            field = value;
            OnPropertyChanged(propertyName);

            return true;
        }

    }

    public abstract class ViewModelBase : ObservableObject {

    }

    public abstract class ViewBase {

        public ViewBase(IBindingManager bindingManager) {
            Bindings = bindingManager;
        }

        protected IBindingManager Bindings { get; private set; }

        private object _dataContext;

        public object DataContext {
            get { return _dataContext; }
            set {
                if (ReferenceEquals(_dataContext, value))
                    return;

                if (!ReferenceEquals(_dataContext, null))
                    Unbind();

                _dataContext = value;
                OnDataContextChanged();

                if (!ReferenceEquals(_dataContext, null))
                    Bind();
            }
        }

        protected virtual void OnDataContextChanged() { }

        private void Bind() {
            OnBind();
        }

        private void Unbind() {
            OnUnbind();
        }

        protected virtual void OnBind() {
        }

        protected virtual void OnUnbind() {
        }

    }

    public abstract class ViewBase<TViewModel> : ViewBase where TViewModel : class {

        public ViewBase(IBindingManager bindingManager) : base(bindingManager) {
        }

        protected override void OnDataContextChanged() {
            _viewModel = DataContext as TViewModel;
        }

        private TViewModel _viewModel;
        public TViewModel ViewModel {
            get { return _viewModel; }
            set { DataContext = value; }
        }

    }

}

Example "UI" Control

using Example.CustomFramework;

namespace Example.Controls {

    public abstract class UIControlBase : ObservableObject {

    }

    public class ExampleUIControl : UIControlBase {

        private string _text = "";
        public string Text {
            get { return _text; }
            set { SetPropertyField(ref _text, value, nameof(Text)); }
        }

    }

}

Example Application

using Example.Bindings;
using Example.Controls;
using Example.CustomFramework;
using System;

namespace Example.Program {

    class Program {

        static void Main(string[] args) {
            var exampleView = new ExampleView(new BindingManager());
            exampleView.ViewModel = new ExampleViewModel();

            exampleView.SimulateViewInteractions();
        }

    }

    public class ExampleViewModel : ViewModelBase {

        private string _firstName = "";
        public string FirstName {
            get { return _firstName; }
            set { SetPropertyField(ref _firstName, value, nameof(FirstName)); }
        }

        private string _secondName = "";
        public string SecondName {
            get { return _secondName; }
            set { SetPropertyField(ref _secondName, value, nameof(SecondName)); }
        }

        private string _exampleWithInitialValue = "Hello, World!";
        public string ExampleWithInitialValue {
            get { return _exampleWithInitialValue; }
            set { SetPropertyField(ref _exampleWithInitialValue, value, nameof(ExampleWithInitialValue)); }
        }

    }

    public class ExampleView : ViewBase<ExampleViewModel> {

        private ExampleUIControl _firstNameControl;
        private ExampleUIControl _secondNameControl;
        private ExampleUIControl _exampleWithInitialValueControl;

        public ExampleView(IBindingManager bindingManager) : base(bindingManager) {
            _firstNameControl = new ExampleUIControl();
            _secondNameControl = new ExampleUIControl();
            _exampleWithInitialValueControl = new ExampleUIControl();
        }

        protected override void OnBind() {
            Bindings.Add(new Binding(ViewModel, "FirstName", _firstNameControl, "Text"));
            Bindings.Add(new Binding(ViewModel, "SecondName", _secondNameControl, "Text"));
            Bindings.Add(new Binding(ViewModel, "ExampleWithInitialValue", _exampleWithInitialValueControl, "Text"));
        }

        public void SimulateViewInteractions() {
            Console.WriteLine("ViewModel Before:");
            Console.WriteLine("---------------------------------------------");
            Console.WriteLine("FirstName:\t\t\t" + ViewModel.FirstName);
            Console.WriteLine("SecondName:\t\t\t" + ViewModel.SecondName);
            Console.WriteLine("ExampleWithInitialValue:\t" + ViewModel.ExampleWithInitialValue);

            Console.WriteLine();
            Console.WriteLine("Controls After Binding:");
            Console.WriteLine("---------------------------------------------");
            Console.WriteLine("FirstName:\t\t\t" + _firstNameControl.Text);
            Console.WriteLine("SecondName:\t\t\t" + _secondNameControl.Text);
            Console.WriteLine("ExampleWithInitialValue:\t" + _exampleWithInitialValueControl.Text);

            Console.WriteLine();
            Console.WriteLine("Simulating User Input...");
            Console.WriteLine("---------------------------------------------");
            _firstNameControl.Text = "Bob";
            _secondNameControl.Text = "Hoskins";
            _exampleWithInitialValueControl.Text = "Foo";

            Console.WriteLine();
            Console.WriteLine("ViewModel Afterwards:");
            Console.WriteLine("---------------------------------------------");
            Console.WriteLine("FirstName:\t\t\t" + ViewModel.FirstName);
            Console.WriteLine("SecondName:\t\t\t" + ViewModel.SecondName);
            Console.WriteLine("ExampleWithInitialValue:\t" + ViewModel.ExampleWithInitialValue);

            Console.WriteLine();
            Console.WriteLine("Simulating Business Logic...");
            Console.WriteLine("---------------------------------------------");
            ViewModel.FirstName = "Jessica";
            ViewModel.SecondName = "Alba";
            ViewModel.ExampleWithInitialValue = "Bar";

            Console.WriteLine();
            Console.WriteLine("Controls Afterwards:");
            Console.WriteLine("---------------------------------------------");
            Console.WriteLine("FirstName:\t\t\t" + _firstNameControl.Text);
            Console.WriteLine("SecondName:\t\t\t" + _secondNameControl.Text);
            Console.WriteLine("ExampleWithInitialValue:\t" + _exampleWithInitialValueControl.Text);

            Console.ReadKey();
        }

    }

}

Example Application Output

bindingsexampleoutput

Why should we derive from a base class? Typically reflection and attributes are used to provide such decorations.

I am not sure what you mean here; the ViewModelBase class? This is just a user framework convenience; the only thing that is important is the INotifyPropertyChanged interface.

Is this method flexible enough to be a general purpose solution for everyone's use, or is it particularly suited to a specific problem domain?

I believe that a simple binding mechanism like this would be useful in a general sense; absolutely.

As I said in my first post, a XAML-like template parser would be quite nice for some use-cases; but in the case of an platform like Unity it would be nice if the XAML control generation could be custom implemented; i.e. just using the XML as a template with bindings whereby the output is platform-specific.

In the above example there is no IValueConverter implementation; primarily because I am not entirely sure how that would be best implemented (inject IValueConverter versus an application-wide IValueConverter; or perhaps an application-wide implementation but allowing specific bindings or binding managers to override them).

I hope that this helps to elaborate on my request :)

[Edit: Finished implementing DestroyBinding()]

kruncher commented 9 years ago

[edit] The first problem area that comes to mind is WPF/Silverlight DependencyProperties. Does this replace those?

I am not familiar with the WPF/Silverlight DependencyProperties; but looking at the following page of the MSDN documentation I don't see why a IBindingManager implementation couldn't just expose methods to create similar bindings for these by way of using reflection.

https://msdn.microsoft.com/en-gb/library/cc903933(v=vs.95).aspx

OtherCrashOverride commented 9 years ago

Since the foundation of your design is that properties are observable, I think you should formalize that into an ObservableProperty<T>. It would provide: T Get(); void Set(T); and EventHandler<PropertyChanged> Changed.

class ExampleViewModel
{
   ObservableProperty<string> firstName = new ObservableProperty<string>("Default Value");

   public ObservableProperty<string> FirstName
   {  get { return firstName.Get(); set { firstName.Set(value); } }
}

I think public enum BindingMode should be replaced with an explicit producer/consumer model: a Binding works in a single direction, to make it bidirectional, use two. (solving cycles would be necessary) Binding then becomes Binding<T>(ObservableProperty<T> producer, ObservableProperty<T> consumer)

Now BindingManager (mediator pattern) is unnecessary, you can store bindings individually or as part of a List<Binding<T>>.

[edit] spotted the issue with this as soon as I put in an IDE

class ExampleViewModel
{
   ObservableProperty<string> firstName = new ObservableProperty<string>("Default Value");

   public ObservableProperty<string> FirstName
   {  get { return firstName; } }
}
kruncher commented 9 years ago

I considered (and even prototyped) using wrapper properties like this; but I thought that it was quite a bit messier to add validation to the view model because of the need to plumb delegates into the wrapper.

Also note that it is not necessary to implement the INotifyPropertyChanged interface if properties do not change; and the binding mecanism can respect read-only/write-only properties. This makes it easy to bind simple POCO's without the need for special wrapper properties and avoiding the need of framework specifics in the UI controls (INotifyPropertyChanged has been a standard part in the .NET framework for quite some time and in my view makes implementation in user code quite clean).

The BindingManager makes it easy to programmatically bind properties between objects whilst also providing a way to quickly nuke all of the bindings that are associated with a view.

I believe that the BindingManager could also act as a useful centralized extension point which could be overridden to cater for project specific needs without the complexity of swapping out the property wrappers.

kruncher commented 9 years ago

It is also possible to define specialized IBinding implementations to bridge the gap between this binding mecanism and non-standard implementations. A sort of adapted binding.

For example, at the moment Unity UI doesn't implement the INotifyPropertyChanged interface; and thus the following would be beneficial:

public class InputFieldBinding : IBinding {
    private object _sourceObject;
    private string _sourcePropertyName;
    private InputField _inputField;
    ...
    public InputFieldBinding(object sourceObject, string sourceProperty, InputField inputField) {
        ...
        _inputField.onTextChanged.AddListener(OnInputFieldTextChanged);
    }
    ...
    private void OnInputFieldTextChanged(string newText) {
        UpdateSourceFromTarget();
    }
    private void OnSourcePropertyChanged(object sender, PropertyChangedEventArgs e) {
        if (_sourcePropertyName == e.PropertyName)
            UpdateTargetFromSource();
    }
    private void UpdateTargetFromSource() {
        // TODO: Utilize IValueConverter here...
        _inputField.text = _sourceProperty.GetValue(_sourcePropertyName, null);
    }
    private void UpdateSourceFromTarget() {
        // TODO: Utilize IValueConverter here...
        _sourceProperty.SetValue(_sourcePropertyName, _inputField.text, null);
    }
}

Which can then be used in the view like follows:

Bindings.Add(new InputFieldBinding(ViewModel, nameof(ViewModel.FirstName), _firstNameInputField));

The view still has the ability to automatically nuke the bindings when its cleanup time.

OtherCrashOverride commented 9 years ago

The main issue I see is that you need to guarantee an observable property. Currently, there is no way to do that.

Adding an IBindable and changing the Binding constructor to public Binding(IBindable sourceObject, string sourcePropertyName, object targetObject, string targetPropertyName) (must be one way.) and public Binding(IBindable sourceObject, string sourcePropertyName, IBindable targetObject, string targetPropertyName, BindingMode bindingMode = BindingMode.TwoWay) gets you part way there but there is still no guarantee the property you are interested in will be bindable.

OtherCrashOverride commented 9 years ago

The solution needs to follow C#'s "strongly typed" nature. A binding should fail at compile time, not runtime. If it fails at runtime then its more "dynamic typed".

kruncher commented 9 years ago

If a class doesn't implement INotifyPropertyChanged then it is not observable (without some sort of adaptor). But surely it should still be possible for UI controls to initialize their values from these properties?

If a class does implement INotifyPropertyChanged and that class doesn't raise the PropertyChanged event then that class is not properly implemented... or its documentation needs to explain why some properties do not raise the event.

As far as I can tell; the INotifyPropertyChanged and INotifyPropertyChanging interfaces were devised with this sort of data binding in mind and many people are already using these mecanisms in their code bases; so if such people want to reuse their application logic with a different selection of UI controls; this approach would make that transition quite straightforward.

https://msdn.microsoft.com/en-us/library/system.componentmodel.inotifypropertychanged(v=vs.110).aspx

In fact; this approach is suggested in the Prism project by Microsoft Practices: https://msdn.microsoft.com/en-us/library/gg405484(v=pandp.40).aspx

Excerpt:

public class Questionnaire : INotifyPropertyChanged
{
    private string favoriteColor;
    public event PropertyChangedEventHandler PropertyChanged;
    ...
    public string FavoriteColor
    {
        get { return this.favoriteColor; }
        set
        {
            if (value != this.favoriteColor)
            {
                this.favoriteColor = value;

                var handler = this.PropertyChanged;
                if (handler != null)
                {
                    handler(this,
                          new PropertyChangedEventArgs("FavoriteColor"));
                }
            }
        }
    }
}

For instance, a lot of developers are applying patterns like MVVM using the INotifyPropertyChanged interface; and on Windows/Xamarin the binding facilities are already in place and work pretty well. Unfortunately; these binding facilities seem to be built-in to the platform-specific UI which makes it hard to efficiently reuse the same view models/observables without resorting to manual data binding.

Edit: Having a separate Binding class acts as a bridge between the two separate observable objects; which I think is a nice separation of concerns. The general purpose binding can treat source and target in a consistent way; and the fact that specialized binding adaptors can be created is a massive plus.

I apologise if I have misunderstood your comments.

kruncher commented 9 years ago

The solution needs to follow C#'s "strongly typed" nature. A binding should fail at compile time, not runtime. If it fails at runtime then its more "dynamic typed".

hmm, I wonder; Roslyn is quite powerful... perhaps there is a way to create one of those new "code analyzers" to detect non-existent properties ahead of time and present a non-blocking warning?.. just a thought

I would experiment with that; but I haven't figured out how to make those yet.

OtherCrashOverride commented 9 years ago

I think its important to get this right if you want to make it part of CoreFX (force it on others). INotifyPropertyChanged is very fragile. It would also be preferable to get away from string propertyName and replace it with an object that actually refers to the property instead. Its too easy to break through refactors, typos, and name-hiding.

OtherCrashOverride commented 9 years ago

As I was prototyping an adaptor for INotifyPropertyChanged, I noticed it has another design defect: since its an interface, there is no guarantee that the object notifying you of a change is the object that actually has the property you are interested in.

void Sample(INotifyPropertyChanged target)
{
   ...

   propertyInfo = target.GetType().GetProperty(propertyName, BindingFlags.Instance | BindingFlags.Public);

   ...
}

target may or may not be the object that actually has the property. So you need to explicitly record both the object that has the properties and object notifying you of changes to those properties.

OtherCrashOverride commented 9 years ago

@kruncher

If a class doesn't implement INotifyPropertyChanged then it is not observable (without some sort of adaptor).

As noted above, since its an interface, you can not make that assertion. A class can be observable with a different class providing INotifyPropertyChanged.

perhaps there is a way to create one of those new "code analyizers" to detect non-existent properties ahead of time?

It also means that is not conclusive.

kruncher commented 9 years ago

Hmm, in what scenario might another class want to implement INotifyPropertyChanged on another's behalf? just curious

I will spend some more time experimenting with the property wrapper approach; I already have a prototype of that implemented.

OtherCrashOverride commented 9 years ago

Hmm, in what scenario might another class want to implement INotifyPropertyChanged on another's behalf? just curious

The only concern is that the design allows for it.

shmuelie commented 9 years ago

It should be noted that

a) Yes you could make a Roslyn analyzer to catch mistakes in string property names but also b) You can use C# 6's new nameof operator to solver the problem

private class A : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    private string prop;
    public string Prop
    {
        get
        {
            return prop;
        }
        set
        {
            if (value != prop)
            {
                prop = value;
                PropertyChangedEventHandler handler = PropertyChanged;
                if (handler != null)
                {
                    handler.Invoke(this, new PropertyChangedEventArgs(nameof(Prop)));
                }
            }
        }
    }
}

private class B : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    private string prop;
    public string Prop
    {
        get
        {
            return prop;
        }
        set
        {
             if (value != prop)
             {
                prop = value;
                PropertyChangedEventHandler handler = PropertyChanged;
                if (handler != null)
                {
                    handler.Invoke(this, new PropertyChangedEventArgs(nameof(Prop)));
                }
            }
        }
    }
}

static void Main(string[] args)
{
    var a = new A();
    var b = new B();
    var bind = new Binding.Binding(a, nameof(A.Prop), b, nameof(b.Prop));
    a.Prop = "TEST";
    var c = a.Prop == b.Prop;
}
kruncher commented 9 years ago

@SamuelEnglard Excellent point; thanks!

shmuelie commented 9 years ago

@kruncher You're welcome. I do like the idea of having a binding framework independent of a UI framework.

Another thought though would be to have weak bindings as well as string ones, so that bindings don't have to keep objects alive

kruncher commented 9 years ago

Absolutely; the introduction of a WeakBinding implementation of IBinding would be very beneficial.

For platforms that support JIT it's also possible to create some extension methods on the binding manager for extra syntax sugar by using expressions to determine the source/target object and the property name:

Bindings.Add(() => a.Prop, () => b.Prop);

But, of course, in order to support platforms that are constrained to AOT it is still necessary to use the more traditional approach with the likes of strings and the awesome new nameof feature.

OtherCrashOverride commented 9 years ago

I had considered nameof but it does not take into account existing code that will still break during a refactor. If you are going to rely on a new compiler feature then its probably better to have the compiler itself do all the work of generating the boilerplate code:

private class A : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    private string prop;

    [Observable]
    public string Prop
    {
        get { return prop;   }
        set  { prop = value; }
    }
}

This still does not correct the design defect in INotifyPropertyChanged. Since only Name is reported, observing code has to use the Sender to do anything meaningful with the event. This involves reflection to obtain the property and its value. Additionally, since the INotifyPropertyChanged object is not guaranteed to be the object with the property being observed, it may not be possible to obtain the property value at all.

kruncher commented 9 years ago

Dependence upon a ObservableAttribute is less useful because it makes it harder to bind properties that never change. For instance, when developing web applications the binding only needs to happen once and so there is no need to even implement INotifyPropertyChanged on the object that is being observed.

I don't really see anything wrong with using reflection here.

I think that it's way more important that such a binding system supports all the hundreds of thousands of classes that have already been implemented using this mechanism. The objective of the binding mechanism is to bind the properties of two objects without those two objects needing to know about the binding mechanism.

By following this standardized approach it is easily possible to reuse all existing view models with Microsoft/Xamarin XAML; with this general purpose binding mechanism and with Windows Forms data binding.

There is nothing to prevent you from being able to use this same mechanism with the property-wrapper approach if that is what you prefer; since you could just implement a custom IBinding which knows how to deal with that approach:

Bindings.Add(new ObservablePropertyBinding(a.ObservablePropertyA, b.ObservablePropertyB));
kruncher commented 9 years ago

I had considered nameof but it does not take into account existing code that will still break during a refactor.

Existing code wont be using these proposed non-standard binding interfaces (i.e. ObservableProperty's and ObservableAttribute's); so I don't see how it would break since it wouldn't actually work in the first place...

If developers use nameof in their new code and gradually transition their existing code bases to using this then future refactoring will be a breeze.

OtherCrashOverride commented 9 years ago

One of the use-cases I envision (and why I am pursuing an implementation of this) is in animation. Its common to "bind" a timeline to class properties. This means the code will be called at least 60 times (or more) a second by multiple callers. This constraint implies the code needs to be highly performant, not produce garbage, and avoid boxing/unboxing. It also needs to be highly durable without assumptions at runtime. The current INotify design is none of these things.

kruncher commented 9 years ago

It is straightforward to avoid the garbage produced when raising the NotifyPropertyChanged event since NotifyPropertyChangedEventArgs instances can be cached for reuse since the only unique property is the "PropertyName".

You can avoid boxing/unboxing of values inside the binding for JIT-enabled platforms since you can build a delegate which operates on the value types instead.

OtherCrashOverride commented 9 years ago

Dependence upon a ObservableAttribute is less useful because it makes it harder to bind properties that never change.

The intent was that [Observable] makes the compiler creates the boilerplate code for you invisibly:

set  {
             var oldValue = prop;
             var newValue = value;

             CALL_USER_CODE_SET_FUNCTION_HERE(value);

             if (newValue != oldValue )
             {
                PropertyChangedEventHandler handler = PropertyChanged;
                if (handler != null)
                {
                    handler.Invoke(this, new PropertyChangedEventArgs(nameof(Prop)));
                }
            }
      }

This will have no impact on binding TO a property. To bind FROM a property, this functionality is already required.

For instance, when developing web applications the binding only needs to happen once and so there is no need to even implement INotifyPropertyChanged on the object that is being observed.

What exactly does "BindOnce" do behavior wise? If something is only ever bound once, why is a binding necessary? Wouldn't it be more efficient and intuitive to just specify SomeClass.Prop = OtherClass.Prop? Is the intent to not actually "BindOnce" but provide "Manual" binding update behavior? If so, it is poorly named.

OtherCrashOverride commented 9 years ago

You can avoid boxing/unboxing of values inside the binding for JIT-enabled platforms since you can build a delegate which operates on the value types instead.

I would have to see an example use of that to understand what is meant. Requiring methods to be dynamically constructed at runtime is a rather obtuse work-around. As I understand it, currently you have to use reflection to get the property and then call a method to set its value. The method that does that (SetValue(object obj, object value) has object as the type for its parameters. This will box for all ValueTypes.

kruncher commented 9 years ago

The intent was that [Observable] makes the compiler creates the boilerplate code for you invisibly:

You have to modify existing application code to add the attribute.

What exactly does "BindOnce" do behavior wise?

It executes the binding one time; yes you can just manually assign the value if you prefer; but this does have uses when automatically generating bindings:

<CheckBox IsChecked="{Binding IsHardMode, Mode=OneTime}" />

Not to forget the fact that the binding mechanism applies IValueConverter value conversion. Utilizing the same binding mechanism is an easy way to assure consistent value conversion.

This is a typical mode of operation for binding:

The method that does that (SetValue(object obj, object value) has object as the type for its parameters.

Many reflection-based operations can be optimized by having .NET emit the code at runtime which improves performance and can help to avoid boxing:

http://blogs.msdn.com/b/csharpfaq/archive/2009/09/14/generating-dynamic-methods-with-expression-trees-in-visual-studio-2010.aspx

I believe that Linq utilizes a lot of expression trees.

kruncher commented 9 years ago

So to get the syntax sugar with an expression tree:

http://stackoverflow.com/questions/671968/retrieving-property-name-from-lambda-expression

PropertyInfo propertyInfo = GetPropertyInfo(someUserObject, u => u.UserID);
                             ^-- this method uses an expression tree to interpret lambda
OtherCrashOverride commented 9 years ago

You have to modify existing application code to add the attribute.

You also have to modify existing application code to use nameof. The point was that if you are going to depend on features only present in a specific version of a compiler, you may as well go all the way and have the compiler implement first class support through an attribute or new keyword.

Which brings me to another possiblity: why not add a new keyword for the compiler to generate the syntactic sugar to implement ObservableProperty<T>?

It should be noted that IValueConverter also boxes:

Object Convert(
    Object value,
    Type targetType,
    Object parameter,
    CultureInfo culture
)

These are just comments and suggestions of things I encountered. Feel free to completely disregard any or all of it.

kruncher commented 9 years ago

You don't HAVE to update your existing view models to use nameof; that's the thing; you CAN, but you don't need to. At least not initially when your focus is more on porting your existing application to a different UI.

These are just comments and suggestions of things I encountered. Feel free to completely disregard any or all of it.

Healthy debate is always useful in getting to the right approach. I am all for the right approach; whatever that might be. But supporting INotifyPropertyChanged is critical imo. Although I can see value in having different IBinding implementations which meet different needs; like suggested above with the weak reference binding support.

OtherCrashOverride commented 9 years ago

I contemplated "WeakBindings" too. I am not perusing it because it has the potential to create developer confusion when bindings are "incomplete". If you implement the feature, be sure to allow a method to report back to the developer which bindings fail because the object was gone.

If someone has a demonstration of real world use-case where "WeakBindings" are desirable, it would be good to post that information here.

shmuelie commented 9 years ago

@kruncher That expression tree trick is a good one. Entity Framework uses something similar to get members of types so I think it would be a fine way to do so.

I think what should be done is have INotifyPropertyChanged there with [Obsolete] on it for migrating existing code and the stronger expression style (or other method) for new code.

kruncher commented 9 years ago

I think what should be done is have INotifyPropertyChanged there with [Obsolete] on it for migrating existing code and the stronger expression style (or other method) for new code.

Unfortunately target platforms that only support AOT cannot use the expression tree trick; at least in the case of the Unity game engine. So when the presentation components of such a platform require AOT it does make sense to support the more verbose syntax.

It is also beneficial to support one-time and one-way property binding one a source/target that doesn't actually implement the INotifyPropertyChanged interface since in many use cases the binding only actually needs to happen the one time (like when rendering web pages).

shmuelie commented 9 years ago

Unfortunately target platforms that only support AOT cannot use the expression tree trick; at least in the case of the Unity game engine. So when the presentation components of such a platform require AOT it does make sense to support the more verbose syntax.

That is a problem I did not know of. Hmmm

It is also beneficial to support one-time and one-way property binding one a source/target that doesn't actually implement the INotifyPropertyChanged interface since in many use cases the binding only actually needs to happen the one time (like when rendering web pages).

I completely agree with a need for one-time/one-way binding. I didn't mean to get rid of that.

kruncher commented 9 years ago

I think that it might be worth considering having "sourcePropertyPath" instead of "sourcePropertyName" (and likewise for the target property) allowing it to behave as suggested in the following article:

SimplePropertyPath: A Poor Man's Binding

This would allow for binding like:

// Verbose syntax:
Bindings.Add(new PropertyBinding(ViewModel, "ComplexProperty.Foo", _textInputField, "Text"));

// Expression-tree powered syntax:
Bindings.AddProperty(() => ViewModel.ComlpexProperty.Foo, () => _textInputField.Text);
shmuelie commented 8 years ago

A thought to solve the reflection speed issue: Reflection Emit?

maxima120 commented 8 years ago

there are dreams of wanna be scientists and there are engineering tasks of practicioners. there is nothing wrong to have dynamic binding as it proven works. your debates can long for years only to produce a monster noone will use..

I say - give us WPF-style binding with inotifyproperychanged now. we (people who makes actual software) need it yesterday.

you can still build parallel strong typed binding separately and see how many people will use it..

birbilis commented 8 years ago

haven't followed the discussion, but maybe this code of mine is useful: https://github.com/Zoomicon/ClipFlair/blob/master/Client/Helpers/Utils/Source/Bindings/BindingUtils.cs see BindProperties method(s) implementation there

cosminstirbu commented 5 years ago

I'm also interested in this feature. Any updates on this?

Are there maybe any open source libraries that we could use for this?