Cannot depend on Inherited member

[dellycowboy]

Maybe I am missing something very basic but I cannot see why the following does not work. Surely inherited members can be part of Fody?

public class StatusVM : BaseVM
{
        [DependsOn("Status")] //doesnt work with or without this
        public bool IsVisible
        {
            get { return Status != null; }
        }
}

public class BaseVM: INotifyPropertyChanged
{
        public bool Status { get; set; } 
}

[SimonCropp]

@dellycowboy that is because the implementation of DependsOn is to inject into the set of the property being depended on. it makes no sense to notify on IsVisible inside BaseVM.Status.

so sorry but this is simply not possible to support

[dellycowboy]

I understand why it happens but I disagree that "it makes no sense" and "it is simply impossible" to support.

Firstly it makes a lot of sense to have a consistent framework that works intuitively and does not require inside knowledge of PropertyChanged compilation.

It would be extremely useful to not have to worry about whether a property was in the subclass or not and just trust that it would fire correctly. So makes "no sense" is a pretty odd thing to say. The current implementation makes far less sense symantically.

Secondly it is not impossible to implement, you could emit an override of the property in the subclass and add the propertychange notification. Or you could add an override to the PropertyChanged method and do something in there. This could be an option that younrlect into and emit a build warning when you detect a depends on baseclass property. Clearly this is not impossible and would add hugely to this framework.

Finally if all that was geuinely not possible (it isnt) you could raise a compile warning so atleast the developer is aware propertychanege notifications are not firing where logic would expect them to.

I love this library by the way but this is a huge issue in building a largescale coherent hierarchical models. I have overrides everywhere with a dumb comment saying ///FODY Propertychange doesnt support baseclass notifications.

Fugly.

[SimonCropp]

sorry this is not going to happen.

I suggest instead of having a base VM with properties you have a shared interface.

if you dont like this i suggest you stop using Fody

[SimonCropp]

re

raise a compile warning

I would accept a PR for this

[distantcam]

@dellycowboy You do realize your suggestion would produce the following code?

public class BaseVM: INotifyPropertyChanged
{
    private bool status;
    public bool Status {
        get { return status; }
        set {
            if (status == value)
                return;
            status = value;
            OnPropertyChanged("Status");
            OnPropertyChanged("IsVisible") // <-------- This line is problematic
        }
    } 
}

BaseVM raising a PropertyChanged event for IsVisible makes no sense. This suggests to me that Status and IsVisible should be in the same class, since one depends on the other.

If I saw this code I would question why the base object is firing events for it's children.

[dellycowboy]

@distantcam no it wouldnt. re-read my suggestion.

[distantcam]

@dellycowboy Ok, the 2 classes in your original example, how would you want them to be rewritten by PropertyChanged?

[dellycowboy]

Simon there is no need to be rude.

"Stop using Fody" is totally uncalled for, it is a perfectly sensible suggestion which doesnt deserve ridiculing or becoming defensive over.

You have implemented all sorts of extensions that are useful for other situations but are less useful than this would be.

This is one that I may go and create for our project. Thank you for this great framework but there is no need to be offensive to suggestions.

Use a shared interface? This wouldnt solve anything, so you are suggestion that fody SHOULD never work with solutions that use base classes. Thats fine but I think you will find that many developers use base classes.

As I said the ugly solution is to override base class properties in subclasses, something that could be automated very easily. A reasonable suggestion I thought. Your suggestion of "no base classes in view models" is totally impractical to 99% of experienced developers.

The parallel is Dependency Properties in WPF, imagine that DPs could not be inherited, you would have to re implement in every child control. It would be farcical, then Microsofts response to "this is never gonna happen guys, if you dont like it piss off and use something else" :) Bizarre altogether.

Anyway as I said great work, but a weird response to a common use case, I will take a look at automating the boilerplate overrides myself.

[dellycowboy]

@distantcam please reread my suggestions. I offer 2 ways to do it and even state again how I have managed the problem. I am not going to state a 4th time.

[SimonCropp]

it is a perfectly sensible suggestion

It is not a sensible suggestion. changing a base property should not "also trigger" a property on the child class. This is why no one has come across this in the 4 years the project has been running.

You have implemented all sorts of extensions that are useful for other situations but are less useful than this would be.

It doesnt matter how useful you think it is. You should not be doing it

are suggestion that fody SHOULD never work with solutions that use base classes. Thats fine but I think you will find that many developers use base classes.

No i did not say that.

Use a shared interface? This wouldnt solve anything

yes it would. for starters why do u have a base class? if it is to "save you code and not have to implement properties in child classes" then you should rethink your strategy. flattening you hierarchy and duplicating some properties will save you much pain in the future. If you need to have a common (castable) base then this is where interfaces can help you. eg if you want to use the Status property in multiple places then have some interface IExposeStatus with the Status property.

There is a reason people regularly say "use composition over inheritance", it works. and VMs are one of the most common abusers of inheritance.

Your suggestion of "no base classes in view models" is totally impractical to 99% of experienced developers.

Again i didnt say this. and "99% of developers"... please

"Stop using Fody" is totally uncalled for, it is a perfectly sensible suggestion which doesnt deserve ridiculing or becoming defensive over.

i was not being rude or defensive. PropertyChanged.Fody is an opinionated approach to injecting INPC. if you dont like those opinions it is best you dont use it.

[dellycowboy]

So now you are telling me not to use inheritance!

You are one of those developers, fine I get it, its more obvious with every rude and close minded post you make. Giving me a lecture about using interfaces and composition, haha :) that really is patronising, I actually use composition all over the place.

You are telling me you never use inheritance in any projects that use PropertyChange, really?

Inheritance is extremely useful, specialized view models can contain extensive logic on handling a variety of things that really do not make sense to compose and re-implement for every type, but I do not need to explain the benefits of inheritance surely?

The example I provided was simplistic but wow I don't like your IExposeStatus route either, yuk! Your code strucutre would have 1000s of interfaces all with the same Property implemented, massive code duplication instead of a clear base class? Listen, both routes have issues, a sensible code structure makes use of many strategies, Inheritance is one of them. Over interfacing is becoming very common place too. Every piece of code is in a separate interface and a separate class. Methods are now Objects. Don't get me started.

It doesnt matter how useful you think it is. You should not be doing it

You still haven't provided any good reasoning for this statement other than stating it as a fact and waffling about composition. Sorry, still not a reason not to inherit properties.

I am using inheritance like 99% of other developers and you do too, so get over it.

The example I proposed was a simple one but forget a base class which you are correct should be kept simple, imagine a ListVM that provides logic for filtering items or a CompareVM that provides a way to compare 2 items, there are a million non 'baseclass is bad' examples of inheritance that are perfectly acceptable ways to build View Models where composition is a terrible bloated option.

Don't forget your framework is not just for ViewModels right? If it isn't perhaps you should rename it to Fody.PropertyChangedButOnlyForViewModelsAndPropertiesMustBeInTheSameType

I maintain a PropertyChange Framework of any kind that claims to be able to inject PropertyChanged notifications and especially one that provides a DependsAttribute should support:

[DependsOn("Count")]
public bool RequiresRefresh { get; set; }

There is just simply no logical reason that this shouldn't work other than, your emit strategy prevents this working. That is implementation detail.

You can keep stating this shouldn't be done till you are blue in the face, fact is you are wrong. It is completely reasonable and I find it astounding that as someone who seems to be a fairly accomplished developer cannot see past his own nose on this.

It is a very poor implementation detail and design that FORCES this not to work against 'expected behaviour', it should be up to the developer to decide whether this is good design or bad *not you * because you prefer Composition.

I understand you won't make this change (especially not now) but at the very least you should raise compile time warnings to detail that your framework has type level limitations and doesn't support basic functionality.

It should be a paragraph on your how to page, but I suggest leaving out your arrogant waffle about composition over inheritance, looks a bit junior developer.

I will continue to use this framework for now, but I explain this bug in Fody.PropertyChanged in comments so that other unsuspecting developers are aware of this gotcha. I suggest you do the same.

[GeertvanHorrik]

I think there is an easy solution for this, although I think it will only be considered as a PR. There is still a catch if you are using this with 3rd party libs, but for libs itself this should work just fine. Fody will add a method to each class implementing INotifyPropertyChanged:

protected virtual void OnPropertyChanged_Fody(string propertyName);

If you use a DependsOn attribute, that class will override the base like this:

protected virtual void OnPropertyChanged_Fody(string propertyName)
{
    base.OnPropertyChanged_Fody(propertyName);

    if (string.IsNullOrEmpty(propertyName) || string.Equals(propertyName, "MyDependsOnProperty"))
    {
        RaisePropertyChanged("MyDependsOnProperty");
    }
}

I know it's not perfect, but I guess this is something this guy is looking for. But I think the only way to get this implemented is by forking and releasing a custom version.

[dellycowboy]

Thanks Gert for constructive input. This is exactly what I ended up doing, originally hardcoded.
In my latest version I created my own custom attribute the VM examines at startup and registers automatically the raise property changed in the base OnPropertyChanged. Not ideal but it works, thanks again.

[ylatuya]

I have run into a simliar problem with Fody, where I need the properties of a base class to be weaved so that OnPropertyChanged is also called. I understand the reason why it's not possible to inject it in the setter of the property of the base class and for now I have defined those properties as virtual and overriden them in the super class, the one implementing INPC. The problem with this approach is that it's hard to maintain since I need to remember to do the override each time it happens.

I do think it's something Fody could do on it's own, for example by listing in a type implementing INPC all the virtual properties of the base class that are not overrinden in the INPC type, and injecting in this type an override of the setter. This override would just call the base setter and inject the OnPropertyChanged event as usual.

I could try to do it, but I would need some guidance as I have no idea on how I could override the virtual property setter.

[ylatuya]

To explain my self a little bit an following the examples of the Fody documentation, this is how it should look before weaving:

public class Person:
{        
    public virtual string Name { get; set; }
}

[ImplementPropertyChanged]
public class Teacher: Person
{        
    public string ID { get; set; }
}

And after weaving:

public class Teacher
{        
    public string ID {
      set {
        // Weaved Code
     }
   }

   public override string Name {
      set {
        base.Name = value;
        // Weaved Code
     }
   }
}

[dellycowboy]

We have another troll khellang?

ylatuya this is what I thought about but I gave up pretty quickly and stuck to the PropertyChanged hack posted by Gert. Yours would probably be a better solution to this problem but sadly it won't ever be included now that battle lines have been drawn (no one should ever ever use inheritance with PropertyChanged, no one wants this and if you don't like it get lost) so 'too much face would be lost'. A great shame. I would like to use your forked version if you get it working but it just creates too many maintainability headaches going forward but good luck!

[distantcam]

@ylatuya Is it possible for you to add the ImplementPropertyChanged attribute to the Person class?

[ghost]

I'm completely with @dellycowboy on this and I'm disappointed in how Simon handled the suggestion. He did seem very opinionated, patronizing and aggressive. I believe Fody.PropertyChanged should be able to handle dependent properties, too.

Anyways, I'm stuck using Fody.PropertyChanged as my client is (not my choice), and I've recently done a lot of refactoring for reusability, which drastically made the code base smaller and easier to maintain.

However, doing so by leveraging inheritance, I have ran into this problem. It's a show stopper. and best part is, since there are no warnings or exceptions being thrown when attempting to do this, It wasn't easy to figure out what was failing.

The virtual/override hack might be the workaround I have to use, but it's silly. Fody could easily handle this itself. It's a shame the community isn't being heard here. If anyone gets a fork put together with a working fix, it'd be much obliged. If I have any personal time to learn fody weaving, maybe I'll try to fork it myself.

Also, @SimonCropp, you should probably include this behavior somewhere in your documentation. Inheritance is pretty common, as @dellycowboy stated.

At the very least, do this:

Finally if all that was geuinely not possible (it isnt) you could raise a compile warning so atleast the developer is aware propertychanege notifications are not firing where logic would expect them to.

Or some sorry soul will refactor for reusability like I did and not find out they broke anything until much further down the line...

[distantcam]

He did seem very opinionated, patronizing and aggressive.

It's a shame the community isn't being heard here.

you should probably include this behavior somewhere

At the very least, do this

No one owes you anything. This is a free project.

[ghost]

You're right, and that's totally fair.

All I can do is support other community members, and hope someone forks before I do. After reading the comments, I wanted to address what I read. You're right, no one owes us anything. And certainly not ridicule.

delly was shutdown for using inheritance to clean up his code and create a reusable framework, which isn't all uncommon. I felt his suggestion could have been handled with more of an open mind, so I wanted to support him. Again, you're right. No one has to change this and maybe no one will. But maybe my support will change something. It's worth that maybe.

I understand your guy's arguments about how the MDIC compiled code doesn't make sense and would be a bad code smell to see a base class raise a property on a child class. However, I can think of other arguments in support of delly, too. I'm willing to elaborate, if interested.

I'll close with saying the project does seem like a great project, over all. Well done. I've worked with reducing time on implementing INotifyPropertyChanged in different ways, but doing it with Fody is a neat way. I'm only using it on this one client project, and it's been nice to work with up until this speedbump.

[GeertvanHorrik]

@kriscoleman-springthrough There are several reasons why this hasn't been implemented:

1. No one that uses this plugin and needed this feature has taken time to fork + write this feature (basically the maintainers of this plugin + any contributors didn't find the need to implement this)
2. Note that even a PR written by a contributor probably needs to be maintained by the project owner. This means that if person X would PR this, and create 4 bugs, the project owner is the one needing to fix those bugs and will probably get flamed if something else breaks due to the lack of unit tests for the PR'd feature. Therefore project owners are (and should be!) very conservative when it comes to accepting complex features since it can break the plugin for 98% of the users that don't use inheritance to the same level as some do.

The cool thing about Fody is that it's very extendible. I think there is some need by some developers / companies. I think you are in a great position to ask your boss / client to give you time to write PropertyChangedWithInheritance.Fody.

[dellycowboy]

@gert not quite. The reasons that have been given are:

1. Its a stupid suggestion
2. You shouldnt be doing inheritance with Fody, use facades or interfaces
3. No one owes us anything

Lol

Also Gert for the record Simon did accept a PR for a warning.

The big problem is not that this project should support this although I think it should and it can be done, its that it takes quite a bit of knowledge of inside the black box to realise this doesnt work! Thats fine early on but project handovers or seperate team bug fixing become a nightmare.

Simons curt and quite frankly typical emotional developer response to something they built (weve all been there too) was a shame but he and the contributers (who you correctly state didnt see a need to fix this) should know that this "bug" or lets say "unexpected side affect of weaving" will be causing subtle and serious bugs in enterprise software for years to come.

At the very very very least this lmitation needs to explicit in the help and documentation.

When Ive passed on the 5 Fody projects ive built to new developers I heavily comment this so the are aware that they SHOULD never use or rely on DependsOn either explicit or implicit because it doesnt work with base class properties due to how weaving works.

In my projects I resorted to creating an auto generated RefreshBindings method and a [Notify] attribute on properties such that I can refresh a subset of object properties at any time and then call RefreshBindings after key events. This has its own pitfalls and performance penalties.

I think in future projects I may go back to a non weaved world and simply hand write everything to avoid confusion. A plugin for VS with a preview would also be very useful and help devs see whats happening, not sure how hard that would be.

yLatuya's suggestion seems promising and Gert your idea also of a weaved propertychange even handler would also make this possible, I tried that in code and it works fine but in the end I had to ditch it due to other issues.

So who knows maybe someone will create PropertychangeWithoutInheritancePitfalls.Fody

[ghost]

The big problem is not that this project should support this although I think it should and it can be done, its that it takes quite a bit of knowledge of inside the black box to realise this doesnt work! Thats fine early on but project handovers or seperate team bug fixing become a nightmare.

That was the frustration I had when I finally found this thread. I spent probably a day and a half before realizing what the problem was. So much time and energy wasted when a simple explanation in documentation could have saved me a lot of trouble. Does anyone owe me that? Nope. But damn, it sure would have been nice.

At the very very very least this lmitation needs to explicit in the help and documentation.

That would certainly help us developers from making assumptions on what Fody.PropertyChanged can do. One paragraph in a markdown readme file.

Personally, I'm only using this project because my client is. I personally don't like the black box paradigm that Fody exhibits, and prefer to stick to plain old property changed implementation. I use custom Resharper code inspections to implement PropertyChanged automatically, and that way I get all of the property changed logic in my code base. This allows me to debug the propertychanged logic more naturally and without surprises, and offers a lot more flexibility for things like inheritance.

I just finished our contract with the client on this project, so I might not have to use Fody.PropertyChanged again. But if the contract is renewed I might try to fork it myself if noone else has.

@dellycowboy, your RefreshBindings() idea seems decent. Better than making VM properties virtual, overriding in subclass, and etc. Thanks for sharing.

[CADbloke]

I found this because I Googled Fody: The typebBlahBlah already implements INotifyPropertyChanged so [ImplementPropertyChanged] is redundant.

I had the problem with a class that inherited a base class (that doesn't do INPC) just to save me typing the same method in half a dozen classes.

This comment:

for starters why do u have a base class? if it is to "save you code and not have to implement properties in child classes" then you should rethink your strategy. flattening you hierarchy and duplicating some properties will save you much pain in the future. If you need to have a common (castable) base then this is where interfaces can help you. eg if you want to use the Status property in multiple places then have some interface IExposeStatus with the Status property.

There is a reason people regularly say "use composition over inheritance", it works. and VMs are one of the most common abusers of inheritance.

Yup, fair enough. That's how this works and if I don't like it I am free to fork it and play God with it. I'm also free to take guidance from some who knows that the ... he is doing. I don't mean that as an insult to anyone's abilities but my own, ok.

[mcd8604]

I am really grateful for @SimonCropp's advice on this issue:

yes it would. for starters why do u have a base class? if it is to "save you code and not have to implement properties in child classes" then you should rethink your strategy. flattening you hierarchy and duplicating some properties will save you much pain in the future. If you need to have a common (castable) base then this is where interfaces can help you. eg if you want to use the Status property in multiple places then have some interface IExposeStatus with the Status property.

I've been struggling with trying to figure out why the inherited properties weren't firing the PropertyChanged events. I was using a simple base class to implement the interface for just a small set of common properties for the derived classes. I removed that base class impl and copied the few lines of code to the derived classes and voila, the events are fired properly. Granted, if the interface and base impl had many more properties, it might seem painful to make redundant via inheritance.. but I could just compose those into a new object instead.