Feature proposal: Source generator that implements INotifyPropertyChanged

[tom-englert]

For classes that are partial and decorated with the [AddINotifyPropertyChangedInterface] attribute the source generator could do the following:

• Add another partial part of the class that derives from INotifiyPropertyChanged and implements the event.

Fody then will just ignore the [AddINotifyPropertyChangedInterface], since INotifiyPropertyChanged is already implemented, and will continue to do the plumbing of the properties.

Advantage: INotifiyPropertyChanged is then already implemented at compile time, so other code is aware of this.

For non-C# code everything will stay the same as now.

[ltrzesniewski]

The generator could even inject the implementaiton if a class is partial and implements INotifiyPropertyChanged but does not have a PropertyChanged event (essentially, don't require [AddINotifyPropertyChangedInterface] for this to work). What do you think?

I'd inject the following implementation:

public event PropertyChangedEventHandler? PropertyChanged;

protected void NotifyPropertyChanged(PropertyChangedEventArgs eventArgs)
    => PropertyChanged?.Invoke(this, eventArgs);

protected void NotifyPropertyChanged([CallerMemberName] string? propertyName = null)
    => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));

[SimonCropp]

no objections from me

[ltrzesniewski]

FYI, I noticed the following issue after working on another source generator this week: https://youtrack.jetbrains.com/issue/RIDER-79659

Having several type parts (even if only one is written manually) makes code navigation harder, at least in Rider.

[tom-englert]

Did not see this in VS.

[ltrzesniewski]

Do you use ReSharper? I've seen inconsistent behavior in VS+R#, but haven't played with it much yet.

[tom-englert]

Yes, I'm using R#, just looking at the LoggerMessage generated files.
They start with // <auto-generated/>, maybe that makes a difference?
The only way to show the generated part in VS is via solution explorer in the projects Dependencies -> Analyzers -> YourAnalyzer -> File.g.cs

[ltrzesniewski]

Oh right! That may be it. I've tried VS+R# with a generator that emits <auto-generated> and another one that doesn't. That may explain the inconsistency I've seen.
I'll be AFK for a few weeks though, I'll test it after I'm back.

[ltrzesniewski]

Maybe we should bump the version to v4.0.1 with this feature?

v4.0.0 has over 100k downloads and is picked up with Version="*".

See #867.

[virzak]

This works great for classes. Can you also ensure it works for records?

[tom-englert]

@virzak records were introduced to simplify writing of read only classes, so implementing change events on a read-only class does not feel like a major use case.

[virzak]

@tom-englert In my case, the record is not read only, and the reason a class is not used is to avoid writing my own IEquatable. since there are about 15 properties. My project would definitely benefit from this feature.

[SimonCropp]

@virzak why would you need IEquatable on a binding model?

[virzak]

@SimonCropp,

If any model property changes from its original, an asterisk is displayed to notify the user. It stays until the user hits the save button.

Here is the code:

[AddINotifyPropertyChangedInterface]
public partial record MyRecord
{
    public double Prop1 { set; get; }
    ....
    public double PropN { set; get; }
}

((INotifyPropertyChanged)MyRecord).PropertyChanged += MyRecord_PropertyChanged;

private async void MyRecord_PropertyChanged(object? sender, PropertyChangedEventArgs e)
{
    CanSave = MyRecord != MyRecordOriginal;
}

// controls the visibility of *
public double CanSave { set; get; }

[tom-englert]

@virzak there are also some source generators that generate IEquatable on classes

[ltrzesniewski]

I like the way he uses records for this, we could add support for them.

Though you don't really need the source generator for that @virzak, you can inherit from a base record which implements INPC for instance.

[virzak]

Thanks for implementing this, but that pattern did not work in my particular issue. This isn't an issue with this library, but with C# records. To make it work I removed the partial keyword from the record to rely on weaving. The problem is that PropertyChanged event gets included in the record equality when using source generator or implementing by hand. Weaving does not modify the public virtual bool Equals(T? other) method of the record, as it shouldn't since Equals could already be custom by the time weaver processes the record.

The other issue would be: if one wanted to use partial record for any other reason, opting out of source generator on a record/class doesn't seem possible, correct? Should [AddINotifyPropertyChangedInterface(PreferWeaving=true)] be considered?

Here are the scenarios:

[AddINotifyPropertyChangedInterface]
partial record MyRecordWithSourceGenerator { public int X { get; set; } public int Y { get; set; } }

partial record MyRecordWithout { public int X { get; set; } public int Y { get; set; } }

record MyRecordWithExplicit : INotifyPropertyChanged
{
    public int X { get; set; }
    public int Y { get; set; }
    public event PropertyChangedEventHandler? PropertyChanged;
}

[AddINotifyPropertyChangedInterface]
record MyRecordWithWeaving { public int X { get; set; } public int Y { get; set; } }

The testing code is:

MyRecordWithSourceGenerator a = new() { X = 1, Y = 2 }, b = new() { X = 1, Y = 2 };
MyRecordWithout c = new() { X = 1, Y = 2 }, d = new() { X = 1, Y = 2 };
MyRecordWithExplicit e = new() { X = 1, Y = 2 }, f = new() { X = 1, Y = 2 };
MyRecordWithWeaving g = new() { X = 1, Y = 2 }, h = new() { X = 1, Y = 2 };

a.PropertyChanged += (s, e) => { };
e.PropertyChanged += (s, e) => { };
((INotifyPropertyChanged)g).PropertyChanged += (s, e) => { };

var equalWith = a == b;         // False
var equalWithout = c == d;      // True, but doesn't have INotifyPropertyChanged
var equalWithExplicit = e == f; // False
var equalWithWeaving = g == h;  // True and works

If I was to use this source generator, I'd have to exclude PropertyChanged from equality either by hand or through another code generator that allows to specify excluded members in a class attribute.

Thanks again, and nothing urgent.

[tom-englert]

It was my gut feeling from the beginning that enabling the source generator for records is not a good idea

[ltrzesniewski]

Damn it 🤦

Including events in the equality comparison really doesn't seem like a good idea... To the point I wonder if that shouldn't be reported as an issue.

Thanks for notifying us.

[virzak]

Reported.