Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Make the NotifyChanged event part of each object #1419

Closed
Starwer opened this issue Mar 28, 2018 · 29 comments
Closed

Proposal: Make the NotifyChanged event part of each object #1419

Starwer opened this issue Mar 28, 2018 · 29 comments

Comments

@Starwer
Copy link

Starwer commented Mar 28, 2018

In my view, C#/WPF is the magic combo promoted by Microsoft. These are designed to work hand in hand and should support each-other optimally in a MVVM world. And I must say I'm pretty convinced by this association.
Now, whereas the learning in C# is a snap for an experienced programmer, I feel that learning WPF is quite a long way interrupted with road-blocks and paved with frustration. The main reason: The Binding can be done only on class properties that implement INotifyPropertyChanged interface. I think the experienced WPF developers are smiling now thinking: "Of course ! This is an easy one !"
But I think every newcomer in WPF has faced this problem, where he was trying to bind a non-observable property, or not even a property, or binding an ObservableCollection wishfully thinking that the changes on items on the collection would be detected... But also admit it, how much time did you spend to solve, implement or work-around this limitation or draw fancy dependency graphs to bind your properties correctly ?

This could have been avoided by having the change event implemented differently. Instead of having it patched over to the .NET library with the quick Implementation of the INotifyPropertyChanged interface, it could be handled by the compiler.

Think of an NotifyChanged event on the object base class, that would be handled directly by the C# Compiler... All the restriction to bind objects only if these are properties from an instance of object implementing an INotifyPropertyChanged would disappear, your Model could become your ViewModel, the binding would become simpler... and the performance would be way better: Less code size, faster response !

If this is too complicated for the compiler to figure out where the NotifyChanged should be added to the IL, we may consider to add a compiler attribute like [Observable]

Think of this as a new ViewModel:

[Observable]
public class Person
{
    [Observable]
    public string Name;
 
    [Observable]
    public Person[] Friends; 
}

That you can simply bind with:

<Border DataContext={Binding MainPersonInstance} >
    <TextBox Text={Binding} />
    <ItemsControl ItemsSource={Binding Friends}>
        <ItemsControl.ItemTemplate>
            <DataTemplate>
                <TextBox Text={Binding}/>
            </DataTemplate>
        </ItemsControl.ItemTemplate>
    /<ItemsControl>
</Border>

Wouldn't that make your day ?

@bondsbw
Copy link

bondsbw commented Mar 28, 2018

Doing this automatically for all objects would incur a major performance penalty when it isn't needed.

Using an attribute would be covered by code generation (#107).

@Starwer
Copy link
Author

Starwer commented Mar 28, 2018

Maybe I should add some example code to explain how you could handle this NotifyChanged event in the Model, in case my previous post was not already clear enough...

public class Example
{
    public void Register(Person pers)
    {
        // no null-check: remember that C# 8.0 makes value-types by default ;)
        pers.Name.NotifyChanged += OnNotifyChanged;
        pers.Friends.NotifyChanged += OnNotifyChanged;
    }

    private void OnNotifyChanged(object sender, NotifyChangedEventArgs e)
    {
        Console.WriteLine("Parent object is: {0}", e.Parent)
        if (sender is string str) {
            Console.WriteLine("The name of the person is now: {0}", str);
        } 
        else if (sender is Person[] friends)
        {
            if (e.Action == CollectionChangeAction.Add) 
            {
                var pers = e.Parent as Person;
                Console.WriteLine("{0} has a new friend !", pers );
            }
        }
    }

}

@Starwer
Copy link
Author

Starwer commented Mar 28, 2018

@basoundr : From what I understand, CodeGenerator would make it easier to write a standard INotifyPropertyChanged implementation, but that would be just cosmetic.
This would still have the limitations of the INotifyPropertyChanged:

  1. Only a property from an instance can be observed (bound).
    Not possible:
[Observable]
public string Name;
  1. To bind a property, the binding must know the source instance and the property path;

  2. To have the items of a collection observable, all object in the collection must be ViewModel class.
    Not possible:

[Observable]
public Person[] Friends; 
  1. If I want to be notified in the Model of change (any) in a list, the solution without the proposed change is quite expensive and complicated, involving creating a list of class containing observable string properties that we must all bind to their parent like in : https://gist.github.com/itajaja/7507120.
    A compiler could just detect (instrument) any access to an instance of Person.Friends sub items and generate the notification code only when needed.

@svick
Copy link
Contributor

svick commented Mar 28, 2018

@Starwer How does your proposal remove those limitations?

@jnm2
Copy link
Contributor

jnm2 commented Mar 28, 2018

In the meantime, this comment makes the case that there's a way to make life much easier in those objects which must implement or inherit from a class implementing INotifyPropertyChanged: #140 (comment)

@Starwer
Copy link
Author

Starwer commented Mar 28, 2018

@svick: As explained, the proposal is not to add just to add a some cosmetic [Observable] or to make the compiler handle the exact copy of INotifyPropertyChanged. My proposal is to enable any object, regardless whether it is a value-type, an instance, a property, a field, a local variable to be observable.
Only the compiler can do that because it has knowledge on the dependency graph.

On the following for example, it is quite obvious, at design time, for the Compiler that the collection is being changed:

[Observable]
string [] friends = new string[] { "John", "Ray", "Steeve" };
friends.NotifyChanged += OnFriendChanged;
friends[1] = "Jane";

Think about the amount of code you need to do that with INotifyPropertyChanged, and how it will (not) be optimized in that case.

Now if you have an call like that:

groupA.NotifyChanged += NeedToBeSaved;
groupA.SubGroup.Person = "Tom";

Pretty obvious for the compiler that groupA content changed... Imagine the gears required in INotifyPropertyChanged and all the LINQ involved.

Think also about applications like for debugging, if you could say: Track any change in groupA (wherever it is happening in the code, whatever sub-child it could be). That's the same idea.

It just strikes me that such a critical feature in VMMV is not natively handled by the compiler.
Looking on the amount of post of people puzzled about the INotifyPropertyChanged and its limitations, I don't understand why I get so little support here.

@svick
Copy link
Contributor

svick commented Mar 28, 2018

@Starwer

On the following for example, it is quite obvious, at design time, for the Compiler that the collection is being changed

Sure, but my question is: how do you handle the non-obvious cases? How exactly does the compiler track the "dependency graph", across multiple methods and even assemblies?

Consider e.g.:

void ChangeAFriend(ref string friend)
{
    friend = "Jane";
}

ChangeAFriend(ref friends[1]);

Or:

departments[0].NotifyChanged += FirstDepartmentChanged;
foreach (var employee in allEmployees)
{
    // how does the compiler know which employees belong to departments[0]?
    employee.Salary *= 1.05m;
}

@Logerfo
Copy link
Contributor

Logerfo commented Mar 28, 2018

This might help you.

@Starwer
Copy link
Author

Starwer commented Mar 28, 2018

@svick : Ok, now we start talking...
I had the impression, because of your -1 vote that you thought that the feature described was totally pointless. It now seems like you find it difficult to implement, which is already way better...

First of all, let me introduce myself: I spent 2 years full time in developing a C Compiler in Philips (although it was in 2004-6... I'm not getting younger...). So I do know it is a challenging thing. We make it easy for the user, but difficult for the compiler (but that's what a compiler is for ... IMHO). Precisely, I do ask it because it is difficult, and that's precisely the reason why this feature would make a difference on C#. This is where it can truly differentiate from C++ or Java... another way than in syntactical sweeteners. An universal change/event, that only exists in C, Tcl ... debuggers.

For your first example:

void ChangeAFriend(ref string friend)
{
    friend = "Jane";
}

ChangeAFriend(ref friends[1]);

We handle that the same way we handle polymorphism inheriting implicitly from an Observable class. If we know that friends[] is Observable. This class has in a sense an implicit = operator

class ObservableString : String{ <instrumentation of String, overload = > }
var friends = new ObservableString[]();
...
void ChangeAFriend(ref string friend)
{
    friend = "Jane"; // inside the pseudo class, delegate callback is handled
}

ChangeAFriend(ref friends[1]);

But wait ! We can not inherit from string ! Exact ! That's why we need the compiler to handle that, and to relax this restriction behind the scene. All the fancy libraries that generate some INotifyPropertyChanged can't do that.

Same for the second example.
As expensive as a INotifyPropertyChanged. At least it is way simpler for the user. And for the above-mentioned obvious case where the compiler can do it better, well this is a gain !

@Starwer
Copy link
Author

Starwer commented Mar 28, 2018

Note: I have now reached the 4x 👎 and a confused emoji...
So what is sure is that my proposal is not popular. What I don't understand is the discrepancies on the comments. Those oscillate between:

  • Too trivial: you can already do that with this library or that feature;
  • Too complicated: what you ask is impossible to do.

I'm willing to take a lesson on this total failure.... but from the contradictory comments I can't really get the take-away message...

@theunrepentantgeek
Copy link

As I see it, there are two ways to achieve your proposed idea.

#1 Implement the existing INotifyPropertyChanged on every object

This would have a significant negative performance impact on a very large amount of code. While there are many projects that use WPF style databinding where this would be useful, there are even more where INotifyPropertyChanged is never used at all.

Doing this instead by opt-in on specific objects is already easily done, whether by hand coding, aspect-oriented programming (e.g. PostSharp), code generation or other approaches.

So, this is trivial.

#2 Enhance the compiler to automagically modify just the right properties when needed

While the data flow analysis for this might (might!) be possible for trivial projects, I suggest its impossible for even moderately complex situations.

It seems you're wanting the compiler to consider every use of a class/property throughout the entire project when deciding how to compile an individual property.

Issues that arise from this ...

  • Assuming you've got a moderate sized project, say, 750kloc, you're asking the compiler to consider all of that source code, as it compiles each individual property. Since it doesn't know about most of the classes yet (remember, we're still doing the compilation), how does it search that code to find uses of the property without false positives?

  • When different parts of the project are compiled on different machines at different times (e.g. when part a library published as a nuget package on an internal feed), how does the compiler see through space and time to the point where the code is consumed?

  • When objects are deserialized using reflection (e.g. from an XML string when using WCF, or from JSON using JSON.NET), how would a compiler know that the object or a specific property was ever used at all?

  • When different parts of the project are compiled in different languages (say, F# or Visual Basic.NET, but there are others), how does the C# compiler know whether the code uses a specific property or not? Do we teach the C# compiler how to parse F#/VB as well?

  • When a templating language such as ASP.NET's Razor is in use, the same problem arises.

FWIW, these examples stem from code I've worked on just this week.

This isn't just complicated, it's impossible.

There are existing proposals that would make correct implementation of INotifyPropertyChanged a trivial matter - I particularly like one that combines code generation, partial classes, and a superceeds keyword. YMMV.

@jnm2
Copy link
Contributor

jnm2 commented Mar 29, 2018

@Starwer ❤️ It can be counter-instinctive to see your idea being downvoted without taking it personally, so I very much respect the constructive approach you are taking.

@bondsbw
Copy link

bondsbw commented Mar 29, 2018

Agreed with @jnm2.

FWIW I feel there is merit in language features built around push semantics. Keeping multiple objects in sync when changes are made to any of them is a clunky process, and I welcome proposals that can make this significantly better.

@ufcpp
Copy link

ufcpp commented Mar 29, 2018

@orthoxerox
Copy link

@Starwer something like Kotlin's delegated properties (with an option to delegate all properties of a type) would be a more controllable mechanism for what you're trying to achieve.

@Starwer
Copy link
Author

Starwer commented Mar 29, 2018

Thanks @theunrepentantgeek . I really value the time and effort (even more than a hypothetical thumb up) you spent to explain your view and show that I going too far into the concept.

I indeed didn't come up with all the solution and the implementation, but wanted to initiate an concept.
Maybe that's the problem: I've jumped too quick into the implementation detail. It's surely impossible for the compiler to figure out when an object is accessed. Something more impossible than getting a Garbage Collector working, which I also thought impossible the first time I've heard from that.

So, this is my last shot.

I'd really like to know (please!) if you're all against the very concept of this change, regardless of the implementation itself (the "What/why", before digging on the "how"):

What: (Specifications)

  1. You can bind any object, not only a INotifyPropertyChanged-property;
  2. the [Observable] attribute is inherited to child objects
  3. the child object automatically notify the parent object on change.

Why:

  1. Many people struggle with the INotifyPropertyChanged
  2. implementing the wiring to notify the parents in a complicated object tree is difficult, time-consuming and error-prone... but can be automated.
  3. This simplifies life of quite an important community of C#/VMMV developpers.

How:

  1. I'm ok to get the same overhead as with INotifyPropertyChanged (but remove it where possible)
  2. I'm ok to have a INotifyPropertyChanged-like mechanism behind the scene (but abstracted to the user)
  3. I'm ok if we need to help the compiler sometimes with hints (keywords) to handle this.

I believe there is something between the "trivial" and the "impossible" approach... to be discussed... if you're not already against the "What".

I could think that when somebody came up with the idea of enabling any method to be asynchronous automagically, he/she had the very similar objections:

  1. This is trivial:
  • we already have BackgroundWorker to do that !
  • we already have 3-4 libraries to make it easier
  • we can do a code generator to implement BackgroundWorker behind the scene !
  • This is only interesting to 50% of the users...
  1. This is impossible:
  • How can the compiler know that this sub-sub-method in an extrnal library should be called asynchronously;
  • Too expensive: I don't want to have the state-machine handling asynchronous calls to every method call !

But eventually, they extended the concept with the Task/async/await combo, and for me there's no doubt now that this is a real improvement for the C# language.

I wanted to know if you were open for a similar discussion here.

@Starwer
Copy link
Author

Starwer commented Mar 29, 2018

BTW, thanks @jnm2 to show some empathy...
thanks @bondsbw for your support, but the fact that you didn't thumb up the proposal make me think that you find it conceptually a real bad idea, even though you're not against pushing features around semantics...

@bondsbw
Copy link

bondsbw commented Mar 29, 2018

@Starwer Not the case at all. If I thought it was bad, I would have given it a thumbs down. I just don't feel the proposal is very compelling in its current form.

@HaloFour
Copy link
Contributor

I'm not seeing how much of this can be implemented. The C# compiler doesn't have that level of control over what it compiles. It certainly can't, say, "lift restrictions" around String or replace how it works in order to make it somehow observable. The C# compiler can only work with the surface that the CLR offers. The compiler would also have zero visibility into referencing assemblies, which may change values at will, especially ref values which are just pointers. The compiler has no visibility into what compiled types do.

I do understand that observing object changes is fairly important, particularly in the WinForm/WPF world. I could see enabling the compiler to automatically generate the boilerplate to invoke that event when a class is adorned with something like Observable or NotifyPropertyChanged attributes, either through direct compiler support or through the mythical source generator feature. But that would stop at the adorned class. If you wanted a type of a child property to be similarly observable you'd need to adorn that class. For collection types or arrays you'd be SOL as the compiler can't modify how existing containers work. You'd need to use some existing type that implements INotifyCollectionChanged.

@sylveon
Copy link

sylveon commented Mar 29, 2018

I'm surprised nobody mentionned Fody.PropertyChanged yet (https://github.com/Fody/PropertyChanged)

It works a lot like @Starwer's original idea:

[AddINotifyPropertyChangedInterface]
public class Person
{
    public string GivenNames { get; set; }
    public string FamilyName { get; set; }

    [DependsOn(nameof(GivenName), nameof(FamilyName))]
    public string FullName => $"{GivenNames} {FamilyName}";
}

@Starwer
Copy link
Author

Starwer commented Mar 29, 2018

@Logerfo, @ufcpp, @orthoxerox, @sylveon : I pretty much appreciate the effort put into all the libraries and code generators to simplify the use of INotifyPropertyChanged. However, the fact that there are so many different solutions to solve the same problem flourishing on the WEB is for me a sign that there is a hole to fill in the language itself. Also, this proves one thing: if a code-generator can do it, a compiler can also do it.

@HaloFour, @bondsbw, @theunrepentantgeek, @svick: the observations/reluctances you presented challenged me to deepen the concept. A more mature proposal starts to shape in my mind (similar to async/await model in fact). However, I'm afraid I've burnt down all my credibility on that topic. But if you think I should continue and come back we a more thorough plan (but less open for discussion de facto), I'd be glad to spend the required hours and come back with a solid proposal.

@HaloFour
Copy link
Contributor

@Starwer

Also, this proves one thing: if a code-generator can do it, a compiler can also do it.

Part of the mantra behind the source generators proposal is that if it can be accomplished relatively easily through annotations and auto-generated boilerplate source that it should be done that way rather than impacting the language proper. This is to both enable the community to iterate on ideas like this and ship product much faster than waiting on the compiler team to get around to implementing it directly in the compiler. Language changes are incredibly expensive in terms of time and resources and many good ideas have been sitting in a holding pattern since Roslyn first went open-source back in 2014.

That said, source generators themselves are quite expensive and have proven difficult to implement due to the tooling considerations, so it's hard to say whether deferring to them is really that appropriate.

similar to async/await model in fact

I think the comparison to async/await isn't quite appropriate. The motivation there was very different. It wasn't about trying to codify a way to implement background work. As you mentioned there are quite a few ways to accomplish that through various libraries. It was about trying to re-invert the inverted style of code that you must adopt when writing anything asynchronous due to nested callbacks. That could only be solved via a language feature of some kind. There's no amount of boilerplate code that could be possibly written that could fix the problem. You'll also note that async/await is incredibly unopinionated and you can quite literally take any of the existing libraries and get them to work with await by following a pattern that can be achieved through extension methods.

@Logerfo
Copy link
Contributor

Logerfo commented Mar 29, 2018

@Starwer

if a code-generator can do it, a compiler can also do it.

The discussion here is if the compiler should do it.

@svick
Copy link
Contributor

svick commented Mar 29, 2018

@Starwer

I think that most people here will agree that the current situation with INotifyPropertyChanged is not ideal. But I think that approaching it from this direction, you have a very steep hill to climb.

For me, for a solution to be considered, it has to satisfy the following points:

  1. It can't significantly affect performance of code that doesn't use this feature. That means that performance of programs that don't use this feature, and even parts of programs that don't use it, has to stay roughly the same, both on the "micro" level (e.g. a loop that reads a property) and on the "macro" level (the overall performance of a program).
  2. It has to work well for all kinds of object graphs. You mentioned "object trees" and "parent objects", but real object graphs are often not trees. What happens if the object graph contains a cycle? What happens if the object graph contains a huge strongly connected component? Notifying all those objects when any one object changes is likely not acceptable.
  3. It has to work on top of the existing CLR and standard library. You mentioned that "the compiler" can create a type that inherits from string. But the C# compiler can't do that. The CLR might be able to do it, but changing the CLR is even harder than changing just the C# compiler, so it's probably not acceptable. (And if you don't understand the relationship between the C# compiler, the CLR and the standard library well, it's probably worth learning about that before you make a proposal.)

@Starwer
Copy link
Author

Starwer commented Mar 29, 2018

@svick : Thanks for you constructive post.

Even though it seems contradictory to what I proposed, I acknowledge all you wrote. And I ensure you that I perfectly understand the difference between the relationship between the compiler, the CLR, the standard library, a code-generator and so on...
The String inheritance was an over-simplification of a potential solution that I have already dropped thanks to our discussions.

@HaloFour : I could only disagree with you on that thing: IMHO the comparison await/async is quite relevant. But on the others things you stated, I agree on every word.

There is indeed a steep hill to climb, and I have the impression I will have to climb it alone... which I just can't. Also, I didn't realize that people seem pretty satisfied about using workarounds code-generators...
I think I will have to lower my ambitions on C# future. After all, I don't ask this for me. I've been through the learning curve on INotifyPropertyChanged and can code it without effort now.

I was expecting enthusiasm about this proposal, but got exactly the opposite (the only encouragement I got is when I stated this proposal was a fail). I'll sleep on it, but I think I'll just give up the fight.

Don't get me wrong, despite my disappointment I really appreciated your dedication to drive the discussion in the right direction (to all).

@theunrepentantgeek
Copy link

theunrepentantgeek commented Mar 29, 2018

@Starwer Your motivations are in the right place and I'd encourage you to submit further proposals when you have them. Submitting your ideas to the scrutiny of critics is never easy, but it is worthwhile.

I've made a couple of proposals for C# features that I thought were really good ideas - one (#589) was met by a resounding meh and the other by disagreement (#1332).

The threshold for an idea to make it into the language is incredibly high - and that's a good thing because it means that those language features will be genuinely and widely useful.

As to your specific proposal …

I can think of ways (perhaps based on your [Observable] suggestion) for the implementation of INotifyPropertyChanged to be automatically generated by a tool. This might be the compiler, it might be a code generator, maybe by another approach.

However, not all objects need an implementation of INotifyPropertyChanged that behaves in the same ways. Harking back to my own WPF experience, I remember one application where I needed to explicitly not propagate change notifications up a tree of nodes because the performance of the screen suffered tremendously - tabbing out of one particular field would take 10-15 seconds because of the large number of WPF control updates triggered by the property write. I had to (carefully) break the rules to make the application usable.

And here we run into a paradox. If developers don't know how to correctly implement INotifyPropertyChanged in a simple way (because it's done automatically by a tool), they won't be able to correctly implement the complex cases when the automated approach lets them down.

In my opinion, automated approaches are a great idea, as long as they satisfy one of two conditions. Either they solve exactly 100% of the problem, or they provide override/injection/configuration options to allow the edge cases to be correctly handled. The async/await feature does exactly this.

@HaloFour
Copy link
Contributor

This is interesting, apparently the VB.NET team has a bit of interest in this problem:

dotnet/vblang#282

@Starwer

Maybe you're using the wrong language? 😉

@orthoxerox
Copy link

@HaloFour that looks very much like Kotlin's delegated properties. Decorated properties?

@Starwer
Copy link
Author

Starwer commented Mar 30, 2018

The discussion here is if the compiler should do it.

@Logerfo : Thanks for this exemplary truism.

@theunrepentantgeek : Thanks for your encouragements and (again) the effort spent to elaborate on the topic.

@HaloFour: Very interesting link. It indeed seems I'm in the wrong language. That's unfortunate that I dislike VB syntax in the first place. That's purely a matter of personal preference BTW, I don't mean VB is bad or ugly by nature... but I've quit the Basic languages in the 90's (Amiga) and there's no way I'm going back there.

You (all) convinced me that the idea was bad and/or premature. I'll then close this issue as it goes nowhere the way it was started. I'll maybe come back one day with a proper proposal on a built-in Observer Design Pattern, if I'm disappointed about the universal #107 (you triggered a very high expectation there).

@Starwer Starwer closed this as completed Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants