CalculatedProperties icon indicating copy to clipboard operation
CalculatedProperties copied to clipboard

Calculated properties break property changed semantic

Open roubachof opened this issue 7 years ago • 8 comments

Let say we have this:

bool IsHorseVisible { get ; set }  // trigger property

bool IsCatVisible { get ; set }  // trigger property

bool IsAnimalVisible => Calculated(() => IsHorseVisible || IsCatVisible);

Initially we have this situation

IsHorseVisible = true;
IsCatVisible = true;

// IsAnimalVisible is true

Then we set:

IsHorseVisible = false;

// IsAnimalVisible will be raised cause source property value has changed

And so the property changed semantic is broken cause even if IsAnimalVisible value hasn't changed, the property is raised.

roubachof avatar Sep 01 '17 08:09 roubachof

I don't think it's an issue. Side effects of firing/re-reading IsAnimalVisible are: reading the property and rewiring dependencies/triggers for calculated property. When Calculated Properties fire PropertyChanged for IsAnimalVisible it makes view invoke the property getter which in turn recalculates list of trigger properties - and it has changed. Note that initially IsCatVisible was never registered as trigger - because it was short-circuited by "OR" operator when IsHorseVisible was true.

By the way, I never met any simple implementations which would check that derived value hasn't changed before firing it. Rx could do it with DistinctUntilChanged() but it comes at a cost of extra boilerplate code

DunetsNM avatar Sep 03 '17 21:09 DunetsNM

I understand your point. The logic is:

  1. Raise calculated property any time a trigger property has changed
  2. Then let the view evaluate the calculated value

But couldn't we have:

  1. Evaluate calculate property any time a trigger property has changed
  2. Raise the property only if the calculated value has changed

?

roubachof avatar Sep 12 '17 09:09 roubachof

The main problem with change detection is that it's difficult for a general-purpose library to determine what "changed" means for a particular type.

StephenCleary avatar Sep 12 '17 09:09 StephenCleary

I think we could say CalculatedValues are great for text concatenation but as soon we have some conditional operations in the calculated property func, behavior becomes hard to follow.

roubachof avatar Sep 12 '17 11:09 roubachof

Calculated properties work with conditional operators just as well as with string concatenation, any level of nesting or complexity. There's simply no way short-circuited dependency could have changed calculated value so no bugs here. Yes library internals are tricky and do rely on certain behavior of consumer (view) i.e. probably not intended to be used outside of MVVM.

But overall in my opinion it's cheapest Excel-like formula engine for your XAML apps out there, with zero learning curve. All you need to make sure is:

  1. getters of trigger & derived properties are side-effect free (apart from dependency graph rebuilding logic which is internal to the property)
  2. all getters and setters invoked in the same thread context (UI thread, makes parallel unit testing impossible)
  3. there's a view/consumer which reacts to property notification and reads it as soon as it fires (which is the case in XAML app)

DunetsNM avatar Oct 09 '17 03:10 DunetsNM

Parallel unit testing works fine if your code is synchronous; all the unit tests for this library run in parallel. If you want to do parallel asynchronous unit testing, you can use AsyncContext. It was originally designed for precisely that reason.

StephenCleary avatar Oct 16 '17 00:10 StephenCleary

object.Equals(a ,b) does a pretty good job of detecting whether there was a change, IMHO. It gives the user some control because they can implement IEquatable if they wish. There could be some boxing inefficiencies.

FYI, I made a somewhat similar class based on property notification. It does have the drawback of firing the action each time any one of the properties changes; there is no way to defer them. An example usage looks like this:

 private BoundAction HighlightBrushBoundAction;

 this.HighlightBrushBoundAction = BoundAction.Create(
        (isValid, separatorBrush) => {
            this.HighlightBrush = (isValid) ? separatorBrush : (Brush)Application.Current.Resources["ErrorForegroundBrush"];
        },
        this,
        @this => @this.ItemsSource.DetailEntity.EntityIsValid, // binding path for isValid
        @this => @this.SeparatorBrush, // binding path for separatorBrush
        true, // fallback value for isValid
        null  // fallback value for separatorBrush
    );

This way it looks to the programmer like a more or less natural extension of property binding. However you can bind to private properties and to properties that are not DependencyProperty's (i.e. which either implement INotifyPropertyChanged or which never change).

sjb-sjb avatar Feb 12 '18 01:02 sjb-sjb

Even if we allow user overriding of IEquatable (or more generally, IEqualityComparer<T>), there's an additional aspect to the user-specified equatable calculation: it must ensure that all computed property delegates don't change.

For example, if someone has a UserEntity that defines equality based on a key userEntity.Id, but if a computed property uses userEntity.Name, then that equality semantic would break the computed property.

StephenCleary avatar Mar 16 '18 14:03 StephenCleary