CommunityToolkit / dotnet

.NET Community Toolkit is a collection of helpers and APIs that work for all .NET developers and are agnostic of any specific UI platform. The toolkit is maintained and published by Microsoft, and part of the .NET Foundation.
https://docs.microsoft.com/dotnet/communitytoolkit/?WT.mc_id=dotnet-0000-bramin
Other
3.07k stars 299 forks source link

OnPropertyChanging not called for generated property #711

Closed marhja closed 1 year ago

marhja commented 1 year ago

Describe the bug

OnPropertyChanging is not called for a generated property when using the NotifyPropertyChangedFor attribute.

Steps to reproduce

Using this class:

public partial class MyObject : ObservableObject
{
  [ObservableProperty]
  [NotifyPropertyChangedFor(nameof(FullName))]
  private string name;

  [ObservableProperty]
  [NotifyPropertyChangedFor(nameof(FullName))]
  private string surname;

  public string FullName => $"{Name} {Surname}";

  protected override void OnPropertyChanging(PropertyChangingEventArgs e)
  {
    base.OnPropertyChanging(e);

    // This is never called for FullName
    Debug.Assert(e.PropertyName != nameof(FullName)); 
  }
}

The following code should cause OnPropertyChanging to be called for property FullName:

var o = new MyObject();
o.Name = "Jack";

Expected behavior

I expected OnPropertyChanging to be called also for property FulleName when Name is assigned a new value.

IDE and version

VS 2022

IDE version

17.2.12

Nuget packages

Nuget package version(s)

8.2.0

Help us help you

No, just wanted to report this

Sergio0694 commented 1 year ago

Oooh good catch! Thank you for reporting this! 😄

EDIT: actually on second thought, this was by design. The rationale is that OnPropertyChanging is only raised for cases where an actual property is being set, right before assigning the field. That is not the case for dependent properties. Those are only notified as having changed due to some external event, such as in this case, a dependent property having changed 🤔

marhja commented 1 year ago

OK, I can understand that this is by design, but then the following paragraph should probably be deleted from the doc comment for NotifyPropertyChangedForAttribute, because that was what made me believe OnPropertyChanging() would be called:

If the containing type also implements the INotifyPropertyChanging interface and exposes a method with the same signature as ObservableObject.OnPropertyChanging(string), then this method will be invoked as well by the property setter.

Sergio0694 commented 1 year ago

Thinking about this more, I think it makes sense to fix this. I've implemented it, but changed target to 9.0 instead of 8.2.1, as this is arguably a small breaking change, functionally speaking. Will want to validate this more with a preview release 🙂