Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
1.72k stars 70 forks source link

Property Binding #161

Closed michaelstonis closed 4 months ago

michaelstonis commented 4 months ago

This is just a quick proof of concept for consideration and to get feedback on a feature like this.

In ReactiveUI and ReactiveMarbles there is a very helpful and cross-platform concept of data binding. Example and details here and here. What is great about this binding is that it can be applied consistently across platforms. For MVVM-supported platforms, this makes the transition across these various platforms very easy as you do not need to know the intricacies of each platform's binding language.

An implementation of this for R3 could be powerful and offer a nice way to bind components together. It feels like it would be a decent candidate for an opt-in/additional library like R3.Binding or similar. A binding like this would best be driven by source generators and could internally be backed by the ObservePropertyChanged and EveryValueChanged methods to allow for universal coverage of binding of objects that implement INotifyPropertyChanged and for plain .NET objects.

Any thoughts on having support for a feature like this added as an extension library?

neuecc commented 4 months ago

Thank you, I understand that it is useful.

However, this is close to being a framework, and I don't think it should be included in R3 Core, even if the package is separated. (In that regard, I also have concerns about the fact that ReactiveCommand is currently included as standard) For example, I thought that collection-related functionality should be separated into a completely different project called ObservableCollections, and should not be included in the standard.

eder-em commented 4 months ago

Also please be careful about adopting source generators to automatically generating INotifyPropertyChanged properties. MSFT has opted in for that on https://learn.microsoft.com/en-us/dotnet/communitytoolkit/mvvm/ and IMO that's a poor implementation and I'll try to summarize the drawbacks of that:

1) It sounds like a complete "black magic" to have declared only a private field on your VM like, private string name;

and then on your View you bind to a property that you've never defined on that VM like: <Label Text={Binding Name} />

2) Because it always automatically generates a new *.g.cs file for each individual property and command of your VM, It completely pollutes the seach experience of your IDE.

3) Also when you're editing your XAML files and that you put the cursor over the name of a given bindable property and press F12 the IDE will navigate to that dam .g.cs file instead of pointing you straight to the definition of that field on your VM.

This is one of the main reason I prefer ReactiveProperty, what you have declared on your VM is what you'll get to bind on your View.

In short what I would say is "stay away" from c# source generators to facilitate binding. What we have from https://github.com/runceel/ReactiveProperty is more then enough and way more elegant;

michaelstonis commented 4 months ago

Thank you, I understand that it is useful.

However, this is close to being a framework, and I don't think it should be included in R3 Core, even if the package is separated. (In that regard, I also have concerns about the fact that ReactiveCommand is currently included as standard) For example, I thought that collection-related functionality should be separated into a completely different project called ObservableCollections, and should not be included in the standard.

I can't disagree with you on any of that. It is a difficult thing to get right without solving too much. I may make a separate repository with some MVVM-related framework components and kick that out there.