microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.18k stars 667 forks source link

Proposal: Add INotifyValueChanged #4984

Open omikhailov opened 3 years ago

omikhailov commented 3 years ago

Proposal: Incapsulate the logic of binding source into classes

Summary

I think everyone at least once wondered why we have to bind to properties, while it would be more logical to bind to objects:

    // How classic binding source looks today

    public class ProductViewModel : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;

        private string _name;

        public string Name
        {
            get
            {
                return _name;
            }
            set
            {
                if (_name != value)
                {
                    _name = value;

                    PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
                }
            }
        }
    }

    // How it should work in the ideal world

    public class ProductViewModel
    {
        public BindableProperty<string> Name { get; } = new ();
    }

Most likely, authors of WPF didn't add generic classes for source properties just because they started to develop it when they had only .Net Framework 2.0 on hand and generics were not yet available or popular.

Now it is possible to create such classes, but binding path will be longer, e.g. ViewModel.Name.Value. So, it makes sense to add some special interface as an alternative to INotifyPropertyChanged that would be recognized as binding source in the markup.

Rationale

JaiganeshKumaran commented 3 years ago

Technically the binding target (BindableProperty class in your case) could implement INotifyPropertyChanged however this will lead too many PropertyChangedEventHandler registrations. In my library, there's a type named ObservableProperty which stores a weak reference to the parent view model class to raise property changed. I had to make the Raise method in the parent view model public to let this happen which isn't safe and use reflection to populate the weak reference inside the property to avoid manually setting it.

omikhailov commented 3 years ago

Yes, there are plenty tricks to make INPC code shorter and more readable:

  1. Extension methods with PropertyChangedEventHandler parameter
  2. Base classes with virtual methods
  3. Friend classes using Reflection to fire that event
  4. Aspect programming tools like PostSharp
  5. Source generators
  6. Custom code snippets to type less code
  7. Property classes implementing INPC

Obviously, the interface does not fully meet the needs of developers if they continue to demonstrate their skills just to make some object know that some value changed. So, I propose to add new interface and close this problem:

  public interface INotifyValueChanged<T>
  {
      T Value { get; set; }

      event EventHandler ValueChanged;
  }

As for registrations count, this is definitely much better than one handler matching property name by the list of strings to find what it needs to do.

chrisglein commented 3 years ago

One of the requirements for the binding system is that it works with a wide(ish) spectrum of objects without imposing too many requirements (for example, requiring a common base class or too onerous of an interface implementation). Additionally while C# constructs may help here it also needs to work with C++ code, which may not have the same utilities. Those baselines needs to be maintained. So anything here would need to be additive/optional syntactic sugar or class utilities.

It sounds like what you propose may be possible but with not the ideal binding path syntax, is that correct? Is what your seeking here an extension to the binding path evaluation?

omikhailov commented 3 years ago

Yes, this is about expanding the binding system, not reworking it. Now we have interfaces for binding to collection or to an object keeping multiple values. It would be logical to add a third specialized interface for binding to one value.

It is possible to use INPC for that third case, but binding will not "know" that there is only one value and it will be required to specify full path. Adding new interface really looks like syntax sugar, but also, this would result in better performance due to the lack of property name checks.

If we focus on the syntax part only, alternative way to extend it would be some shortcut for binding path and simple naming convention would work. But IMO, interface would be better way. In UWP, System.ComponentModel.INotifyPropertyChanged from .Net and Windows.UI.Xaml.Data.INotifyPropertyChanged from WinRT are just mapped to each other. Seems like WinUI 3 and Reunion do the same for Microsoft.UI.Xaml.Data.INotifyPropertyChanged, so technically this would be 100% possible for new interface too.

omikhailov commented 3 years ago

Updated the title to make subject of proposal more clear.

JaiganeshKumaran commented 2 years ago

@omikhailov Can you take a look the ObservableProperty attribute from Microsoft.Toolkit.Mvvm? It doesn't require an extra object or unnecessary allocations however is cleaner and uses source generator to generate the exact same code for you. https://github.com/CommunityToolkit/WindowsCommunityToolkit/issues/3872