Can't use NotifyProperty attribute selectively anymore?

[marcuswhit]

For various reasons, I don't want all my class properties to fire a change event, so previously used the NotifyProperty attribute selectively as needed. It seems this doesn't exist in the new library, and it's an all-or-nothing approach? Any plans to reintroduce the option? Thanks.

[SimonCropp]

@marcuswhit firstly where is the MTB photo from?

This project makes the assumption that if you implement INPC then you would most likely want to notify for most, if not all, properties. If you wish to exclude them, which should be the exception not the rule, then you can add a [DoNotNotifyAttribute] to a class or a property. see here for more details https://github.com/Fody/PropertyChanged/wiki/Attributes

Does this answer your question?

[marcuswhit]

Photo is from the Pyrenees - cycled across a couple years ago.

Makes sense. Would perhaps like to be able to stipulate the default as we could in NPW, but not a big issue I guess.

Thanks.

[SimonCropp]

I did consider it. The problem was that a global switch like that is actually very complex to implement. I know it seems like it should be simple but trust me it isn't. So I went with the simple opinionated approach. Hope it doent cause too many issues for you.

[marcuswhit]

No problem. Would also be nice to have details on how perf is better than NPW - I'm assuming compile-time only, but how? Great work regardless, thanks!

[marcuswhit]

Having given this a bit more thought, I think this is still an issue when introducing Fody.PropertyChanged into an existing project (that doesn't already use NPW). I have just that task to do now, and I'm not comfortable doing so - with the current default, it means that all NPC classes (a lot) will immediately have ALL their properties weaved once I add the PropertyChanged package. Without trawling through every NPC class, there's no way of knowing what unintended side-effects this may cause. If I was able to explicitly set NotifyProperty on properties as I go through a class and update it, it would mitigate that risk.

[SimonCropp]

can you give me an example of how this would cause an unintended side-effect?

[marcuswhit]

Other classes could be subscribed to the PropertyChanged event. If they start receiving notifications for many properties they were previously unaware of, then it could cause performance issues, or worst case they'll handle the new notifications incorrectly. It's possible perf issues could also be seen with grid bindings - some grids refresh the whole row whenever it receives any PropertyChanged event on a row - it we're rapidly raising events on properties irrelevant to the current grid view, then it may cause perf issues.

I realise such issues are edge cases, and not perhaps relevant to small vanilla projects, but in a large legacy project I think it may occasionally cause issues. Some of this may be more a sign of a brittle code base, but that's not something we always have control over...

[SimonCropp]

"Other classes subscribed to the PropertyChanged event" this is something that is easy checked. You can do a find usages of that event and ensure proper usage. If these handlers already handle events I find it hard to believe how adding a few more property notification could significantly impact them. If it did then it is a bug that should be fixed in your code since the same problem could manifest by a developer manually adding in a few more properties.

Re grids and perf issues. Really. How many items are you loading into a grid that you think it will cause issues. Can you share some code to illustrate this?

As for "rapidly changing events" causing perf problems. If this is the case you prob have problems with your architecture. There should be very w cases where you need rapidly changing objects that are bound to. I have never seen any. If this is a valid use case you should try using something like reactiveui and throttle events.

If you have a large existing code base then add a [DoNotNotify] attribute to all your INPC classes and leave the manual OnPropChanged calls in the sets. Then as you add or refactor classes+screens remove the attribute and then test the smaller surface area. This should significantly decrease the risk.

I am not trying to be difficult. It is just that I am trying to target the 80 percent of use cases. Bing friendly to large brittle legacy applications does not make for a clean nor performant implementation.

[marcuswhit]

Fair enough. I'll probably go with the class-level [DoNotNotify] approach initially. Thanks.