Add extra attributes for notification and method calling

[wf-soft]

Overview

Add two new API

API breakdown

[NotifyCanExecuteChangedFrom(nameof(T))]
[PropertyCallMethod(name(T))]

Usage example

We often encounter a scene, I think better way is "NotifyCanExecuteChangedFrom" rather than "NotifyCanExecuteChangedFor", because we can't change notice from the base class

public partial class TestBase : ObservableObject
    {
        [ObservableProperty]
        private string? name = "A";
    }

    public partial class Test : TestBase
    {
        [NotifyCanExecuteChangedFrom(nameof(Name))]
        [RelayCommand(CanExecute = nameof(CanOk))]
        public void Ok() { }
        public bool CanOk => true;
    }

Another point is that the method called by attribute change is currently overwritten by overwriting the OnChanged method. However, we often use multiple attribute changes to call the same target method, which leads to the need to write many override methods, or value parameters are often not needed.

public partial class Test : TestBase
    {
        [PropertyCallMethod(nameof(Ok))]
        [ObservableProperty]
        private string? name1 = "A";

        [PropertyCallMethod(nameof(Ok))]
        [ObservableProperty]
        private string? name2 = "B";

        public void Ok() { }
    }

Breaking change?

No

Alternatives

None

Additional context

No response

Help us help you

No, just wanted to propose this

[michael-hawker]

@wf-soft the subclass wouldn't be able to change the generation of the base class, there's no connection there, so I'm a bit confused by your NotifyCanExecuteChangedFrom scenario. Typically, you'd have to register to listen for the property change in the subclass to the base class property, in that new handler you'd refresh your command's can execute. There's no place for an attribute to easily hook-in to do that though as it couldn't modify the constructor.

For PropertyCallMethod is that supposed to happen before or after the property is changed? That's why we have the OnChanging/Changed methods to use. It makes it clear and easy to adapt code. If you need to do something more specific, then don't use the attribute and just write a traditional property, they can be mixed and matched.

(Also, it's best if you have two different independent requests to file two separate issues, otherwise it's hard to discuss and/or work out what's going with each one.)

[wf-soft]

@michael-hawker I think MvvmGen does a good job of making it clear what properties your code depends on

[CommandInvalidate(nameof(FirstName), nameof(LastName))]
[Command(CanExecuteMethod = nameof(CanSave))]
private void Save() { }

private bool CanSave()
{
  return !string.IsNullOrEmpty(_firstName)
    && !string.IsNullOrEmpty(_lastName);
}

[ViewModel]
public partial class EmployeeViewModel
{
  [Property] private string _firstName;
  [Property] private string _lastName;

  [PropertyInvalidate(nameof(FirstName))]
  [PropertyInvalidate(nameof(LastName))]
  public string FullName => $"{FirstName} {LastName}";
}

[MisinformedDNA]

I would love to see PropertyInvalidate or DependsOn (as named in the linked issue) or NotifyOnPropertyChange (whatever we call it).

I have a base class with ObservablePropertys and the derived class could make good use of this functionality.

public partial class PersonViewModel : ObservableObject
{
    [ObservableProperty] private string _firstName;
    [ObservableProperty] private string _lastName;
}

public partial class EmployeeViewModel : PersonViewModel
{
    [NotifyOnPropertyChanged(nameof(FirstName)]
    [NotifyOnPropertyChanged(nameof(LastName)]
    public string FullName => $"{FirstName} {LastName}";
}

@kaivol is willing to help and has already created an implemention for it. Can the team chime in on whether this is something we can bring into the library?

[Sergio0694]

I feel like there's some misunderstanding here, the code snippet you shared could never work. There's no way that eg. FirstName would know about having to notify FullName in a completely unrelated type that inherits from its containing type. Even if we added an attribute like the one proposed here, this scenario would still not work anyway.

That said, the proposed attributes are just not something we want to pursue in the MVVM Toolkit, at least for now. We specifically decided to keep the reference flow unidirectional, so that each property would indicate what it would notify, but not vice versa. It lets the generators be more efficient (which is not a secondary point), and the code easier to follow, as there's just a single way to express things, and not two to choose from. Closing this as not planned 🙂

[MisinformedDNA]

Right, the way it is currently done, my code snippet would never work, but it could work if it was done the opposite direction.

I also disagree that the existing way makes "the code easier to follow". Between option 1 and option 2 (below), option 2 is much easier to follow. In option 2, FirstName doesn't need to know about what properties might rely on it, but instead the attributes are right next to the logic that actually uses them.

// Option 1
public partial class PersonViewModel : ObservableObject
{
    [ObservableProperty]
    [NotifyPropertyChangedFor(nameof(FullName))]
    private string _firstName;

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

    public string FullName => $"{FirstName} {LastName}";
}

// Option 2
public partial class PersonViewModel : ObservableObject
{
    [ObservableProperty]
    private string _firstName;

    [ObservableProperty]
    private string _lastName;

    [DependsOn(nameof(FirstName), nameof(LastName))]
    public string FullName => $"{FirstName} {LastName}";
}

Imaging FullName changes to public string FullName => $"{FirstName} {LastName}";. We can immediately see that we don't need one of the attributes. With option 1, it's very possible that it become dead code.

In the above case, you could argue speed vs simplicity, but in my scenario, only one way would work. And without that, I'm back to writing boilerplate code for something that could easily be simplified. What is most disappointing is that I was taking a bet on this library, but you all seem less interested in the community and more interested in your internal decisions and us doing things how you want us to instead of giving us needed flexibility. :(

[Sergio0694]

"the way it is currently done, my code snippet would never work, but it could work if it was done the opposite direction"

No, that's not what I'm saying. Even if we had [DependsnOn], your code snippet would just never work regardless. When compiling PersonViewModel, there's no way for the generator to know that there is some type inheriting from it that happens to use [DependsOn] on one of the properties there, so it couldn't generate the necessary code to notify that property. And even if it could, it'd end up just notifying properties that might not even exist at all in case that type was then inherited by another one without that property, so that would just become completely unnecessary overhead.

"What is most disappointing is that I was taking a bet on this library, but you all seem less interested in the community and more interested in your internal decisions and us doing things how you want us to instead of giving us needed flexibility."

I have to push back on this, as it's just not true. I've started writing the MVVM Toolkit before I was even at Microsoft at all, so this is quite literally a community born project. And even now, we're constantly asking for feedbacks from the community. You can just go back to the thread about source generators (#8) to see just how many iterations we did over time, and how many features we modified and added after feedback from the community. And there's been plenty like that one. We are listening. Just as an example, take #449. That completely started from community requests, such as #208 and #365.

Now, that said, being community driven doesn't mean that we can just accept all suggestions, of course. With respect to this one in particular, there's just several reasons why, at least right now, this is not something we're interested in supporting, but that doesn't mean that we're not listening to the community. I've explained why this feature is problematic:

• Unlike the current approach, the proposed one would not work in all cases. For instance, it'd be completely unusable in case of inherited types, like mentioned above, which would be fairly unintuitive for users ("why does this not work??").
• It'd introduce a different way to do exactly the same, which adds further confusion.
• It breaks the established pattern of generators immediately affecting annotating code, not indirectly.
• It makes the generators less efficient, as they'd now have to have extra logic to look at all annotated properties.

[MisinformedDNA]

I'm not going to belabor the request, but just to clarify how it could be solved (and maybe I'm still wrong) would be to implement an event handler that handled the inverse of the current process:

private void ViewModel_PropertyChanged(object? sender, PropertyChangedEventArgs e)
{
    switch (e.PropertyName)
    {
        case nameof(FirstName):
            OnPropertyChanged(nameof(FullName));
            break;
        case nameof(LastName):
            OnPropertyChanged(nameof(FullName));
            break;
    }
}

This is what I have to manually code anyway, so that's what I would have envisioned the generator doing. I didn't envision it all getting nicely wrapped like it currently is. Just functional. But I don't want to argue the point anymore. Cheers.

[CodingOctocat]

For this, PropertyChanged/Fody does a good job, but beyond that, sometimes I need CalculatedProperties to solve the "nested (child-properties)" notification problem, I think it would be perfect if MVVMToolkit was smart enough to easily solve any property notification related problems!