-
Notifications
You must be signed in to change notification settings - Fork 297
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
Add extra attributes for notification and method calling #367
Comments
@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 For (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.) |
@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}";
} |
I would love to see I have a base class with 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? |
I feel like there's some misunderstanding here, the code snippet you shared could never work. There's no way that eg. 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 🙂 |
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, // 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 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. :( |
No, that's not what I'm saying. Even if we had
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:
|
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. |
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! |
Overview
Add two new API
API breakdown
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
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.
Breaking change?
No
Alternatives
None
Additional context
No response
Help us help you
No, just wanted to propose this
The text was updated successfully, but these errors were encountered: