Caliburn-Micro / Caliburn.Micro

A small, yet powerful framework, designed for building applications across all XAML platforms. Its strong support for MV* patterns will enable you to build your solution quickly, without the need to sacrifice code quality or testability.
http://caliburnmicro.com/
MIT License
2.8k stars 776 forks source link

Property Notification Chaining #125

Closed johnds1974 closed 9 years ago

johnds1974 commented 9 years ago

I would like to propose some 'enhancement' functionality if I may: 'Notification chaining'. I love Caliburn.Micro so far, in the short time I have been using it, and this area is one where I think there could be some benefit.

So, what is 'Notification Chaining' I hear you ask? Well, in many cases, where you have a PropertyChangedBase derived ViewModel, where you have several setters, and one or more calls to NotifyOfPropertyChange() in each setter, sometimes you need to also Notify of OTHER properties that are related to/connected with the one that has just changed, for instance:

// What's our Name? public bool Name { get { return _name; } set { _name = value; NotifyOfPropertyChange(); // Let he view know that Name has changed. NotifyOfPropertyChange(() => HasName); // let any UI element that may want to know that Name has changed NotifiyOfPropertyChange(() => CanSavePerson); }

// Do we have Name value? public bool HasName { get { return !string.IsNulOrEmpty(Name); } }

// Can we save this Person? Enable/Disable the 'SavePerson' button public bool CanSavePerson { get { return HasName; } }

This is an extremely simple example, but sometimes you may have a ViewModel where many similar Notification dependencies need to be coded into each property setter, with individual NotifyOf... calls.

What I propose is to have a mechanism where you can either 1) in the setter method, have a overload to NotifyOf... where you can specify a collection of Property names/expressions have have the internal implementation then notify on each of the supplied properties 2) Have another setup mechanism, which you could possibly call from the ViewModel constructor where you can inform the viewmodel that if Name changes, then you need x,y,z other properties to be notified too.

Perhaps something like...

public class PersonViewModel : PropertyChangedBase { public PersonViewModel() { OnNofityOf(() => Name).NotifyAll(() => HasName, () => CanSavePerson); }

public string Name { get { return _name; } set { _name = value; NotifyOfPropertyChange(); // This tells the base to then notify all dependent properties of a change. } } ... }

Either way, the intention is to keep Setters minimal and tidy, and allow such dependency Notifications to be setup elsewhere.

Thoughts anyone? I know I would like to experiment in my own Caliburn FORK to hava play.

John.

mwpowellhtx commented 9 years ago

Couple of observations when I've approached this type of scenario. Overall, I'm not sure it's clever, but over complicating things a bit.

1) Do nothing. Keep calling additional NotifyOfPropertyChanged as it ever was.

2) Are you making it too complicated? With your proposal, you are suggesting that there be some sort of hash, dictionary, or other lookup, to coordinate the triggered update. Presumably; I could be wrong. Domain model and/or view model objects would be really heavy weight then, wouldn't they? What happens with cloning? Be careful not to transfer notifications from one instance to the next.

3) Inspecting the base class in the version of CM that I am working from, why not simply virtualize the methods and override them, then trap for names and such there?

//This one was already virtual, and in fact may be sufficient
public virtual void NotifyOfPropertyChange(string propertyName = null) {}

Override this method and use a simple Switch statement to delegate the appropriate response.

Or call NotifyOfPropertyChange additional times as you see fit.

That's my two cents.

johnds1974 commented 9 years ago

No no, fair enough mate. I did not actually consider the possible notion of making it any more 'heavy weight'.

mwpowellhtx commented 9 years ago

That is not to say you shouldn't experiment. I do that all the time. I take my lessons from thought leaders in the industry though; i.e. keep it simple, 'sometimes a notification is just a notification'. Patterns like delayed notifications could be interesting, things of this nature. Food for thought.

nigel-sampson commented 9 years ago

It's certainly an interesting idea, feels very much like what you see in Reactive Programming

These days I use Fody.PropertyChanged. Which gives you code like the following.

public string Name
{
    get; set;
}

[DependsOn("Name")]
public bool HasName
{
    get
    {
         return !String.IsNulOrEmpty(Name);
    }
}

The only thing I don't like about this solution is the property name in a string which makes it easy to break with refactors. This should be solved in C# 6 with the nameof operator.

You could potentially build it in a way that doesn't involve modifying the Caliburn.Micro base classes on top of INotifyPropertyChanged so it could work on any MVVM framework and a separate nuget package.

johnds1974 commented 9 years ago

@nigel-sampson - Hmmm, I like that attribute markup idea, but yeah I agree, 'magic strings' are generally a bad idea. I suppose I could look into your suggestion of some sort of general purpose 'Dependency-Notification' addon package that could supplement Caliburn.

My only concern, and I could be wrong, I'm thinking of this as I am writing this response, is designing it in a way that would not 'replace' the Caliburn notification mechanism, because Caliburn may have some sort of specialization code baked into it's notification implementation that should always be called etc etc. Know what I mean?

nigel-sampson commented 9 years ago

Yeah, strings are definitely bad here, thankfully C# 6 nameof should let us do

public string Name
{
    get; set;
}

[DependsOn(nameof(Name))]
public bool HasName
{
    get
    {
         return !String.IsNulOrEmpty(Name);
    }
}

You're correct that Caliburn.Micro does add some specialisation, PropertyChangedBase adds the ability to disable notifications through IsNotifiying and also ensures the notification is on the UI thread.

I still like the idea of building this in a way that doesn't involve adding a lot of code to what is currently a very simple but important building block of Caliburn.Micro.

I'd suggest specialising this to Caliburn.Micro if that's what you're working with 90% of the time, it should be able to be done with a combination of extension methods and method chaining in quite an elegant way.

mwpowellhtx commented 9 years ago

Now I am jealous: nameof is a long time coming IMO. :+1:

nigel-sampson commented 9 years ago

Closing this off as it's better as a separate project / package.