Skip to content

Enable other classes that already implement the INotifyPropertyChanged to leverage Source Generator Attributes #620

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

Open
hawkerm opened this issue Feb 23, 2023 · 4 comments
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit

Comments

@hawkerm
Copy link

hawkerm commented Feb 23, 2023

Overview

For instance, I already have a class I'm using which inherits from INotifyPropertyChanged. In my scenario, it's ObservableCollection, so I can't modify it as it's in the BCL.

public class ObservableCollection<T> : Collection<T>, INotifyCollectionChanged, INotifyPropertyChanged

It already has an OnPropertyChanged helper and PropertyChanged event:

        protected virtual event PropertyChangedEventHandler? PropertyChanged;

        protected virtual void OnPropertyChanged(PropertyChangedEventArgs e)
        {
            PropertyChanged?.Invoke(this, e);
        }

It would be great if I could still use some or all of the MVVM attributes for source generators like [ObservableProperty] when inheriting from these classes in my own class.

API breakdown

N/A uses existing attributes.

Usage example

For instance, I have a class already inheriting from this:

public partial class MyClass : ObservableCollection<T>
{
     // Since my parent class already inherits from INotifyPropertyChanged (and already has a OnPropertyChanged method), just use those...
     [ObservableProperty]
     private bool _isModified;
}

Worst case, let me have to re-specify that I want whatever's needed to polyfill, either with INotifyPropertyChanged or ObservableObject:

[INotifyPropertyChanged]
public partial class MyClass : ObservableCollection<T>
{
     [ObservableProperty]
     private bool _isModified;

     partial OnIsModifiedChanged(bool value)
     {
         // Yay, I could also do this now...
     }
}

Basically the existing generator in this case would spit out all the same stuff except what's already implemented by the type (in the case of ObservableCollection it'd be the following:

    partial class MyClass : global::System.ComponentModel.INotifyPropertyChanged
    {
        //// The Event and the event args OnPropertyChanged are already implemented, so skip adding them

        /// <summary>
        /// Raises the <see cref = "PropertyChanged"/> event.
        /// </summary>
        /// <param name = "propertyName">(optional) The name of the property that changed.</param>
        [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.INotifyPropertyChangedGenerator", "8.1.0.0")]
        [global::System.Diagnostics.DebuggerNonUserCode]
        [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
        protected void OnPropertyChanged([global::System.Runtime.CompilerServices.CallerMemberName] string? propertyName = null)
        {
            OnPropertyChanged(new global::System.ComponentModel.PropertyChangedEventArgs(propertyName));
        }

        //// All the SetProperty helpers, TaskNotifier, etc...

If there's any conflict for some particular type, then raise an analytic warning/error.

Breaking change?

No

Alternatives

Can't use MVVM Toolkit generators or helpers? 😥

Additional context

Don't know if I know enough to help here. Had a hard time trying to understand how the existing code was generated with the new fanciness...

Help us help you

Yes, if someone can help (I'm not sure I understand where the current INotifyPropertyChanged generator is and how it works. Nor do I know if I know how to detect if something inherits or has the required methods to skip outputting them... but beyond that, I'd be willing to assist giving it a go if pointed in the right directions.

@hawkerm hawkerm added the feature request 📬 A request for new changes to improve functionality label Feb 23, 2023
@Sergio0694 Sergio0694 added the mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit label Mar 14, 2023
@eeevans
Copy link

eeevans commented Apr 18, 2023

It would be nice for integrating with Caliburn.Micro base classes for screens and conductors.

@varyamereon
Copy link

I have a similar use case for writing custom controls in Maui, they inherit already from 'ContentView' which implements INotifyPropertyChanged. Have some properties that I would like to add an ObservableProperty attribute to but it's not currently possible.

@hawkerm
Copy link
Author

hawkerm commented Nov 23, 2024

As mentioned above and other duplicates/discussions, this is a problem for some MAUI Components due to BindableObject in MAUI implementing INotifyPropertyChanged.

I'd be curious to know why the UI layer in MAUI provides this implementation vs. just listening to it and using DependencyObjects like WPF/WinUI do?

Oddly, Avalonia does something similar with its AvaloniaObject

In these cases, you could sometimes just abstract things into a ViewModel that your page/view contains, but that's not always as immediately obvious for folks; many folks find it easier to use INPC over DependencyProperties...

In any case, providing better interop with other systems which may be providing some base level of implementation could be interesting still. In my original case, I was trying to simplify how I could interact with my ViewModel by making it based off of the collection type itself, I believe I just made it contain the collection instead and provided an indexer implementation.

@ComtelJeremy
Copy link

Just wondering if there is any traction on this. We also have a use case where we use ReactiveUI and would like to use the SourceGenerators instead of rolling our own. ReaciveObject already implements INotifyPropertyChanged.

I assumed this would just work, as the docs seem to indicate this: See the first note in the docs for ObservableProperty attribute. But it throws and error, as mentioned above. These docs here also seem to indicate this.

Could the generator for INotifyPropertyChanged or ObservableObject accept a bool that tells it to skip certain parts, or not re-implement the parts that are already there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit
Projects
None yet
Development

No branches or pull requests

5 participants