Catel SetValue and object equality

Topics: Feature requests
Nov 15, 2012 at 2:45 AM

Hi,

I have just noticed what I believe is an issue with the catel.ObjectHelper.AreEqual() method, which is used by The Catel.ViewModelBase.SetValue() method.

I have a model class (MyType) in which the bool Equals(object obj) method has been overriden to provide domain specific equality (non reference) comparisons. In the case of this class the test only involved a few (ie not all) of the objects attributes.

The equality (==) operator has not been overloaded and still performs referential equality.

I noticed that SetValue() did not update the underlying value when the 'new' object was equal to the 'old' object via a.Equals(b), even though a == b returns false (different objects in memory).

If SetValue() used == there would _not_ be an issue.

Is there a reason why SetValue does not use the == operator?

Coordinator
Nov 19, 2012 at 6:49 PM

The reason is that sometimes string are equal, but not the same reference. When using == on them as objects, they will not be marked as equal.

If 2 objects have the same reference, they cannot be different, not matter how you turn it.

If you provide unit tests that you think should succeed but do not at the moment, please submit them and we will take that into consideration.

Nov 19, 2012 at 7:29 PM

Hi, thanks for the response

When I call SetValue(new_obj), I don't care about 'value equality', I only care about 'reference equality'. If 'new_obj' refers to a different object than the 'current_obj' I _want_ new_obj to become the current value. The result of new_obj.Equals(current_obj) should have no impact on the behaviour of SetValue() .

IE I would expect that you would _only use_ Object.ReferenceEquals() inside SetValue(). After all, user classes can overide Equals() to provide whatever 'value equality' the class designer believes is appropriate. Surely, this should not affect the behaviour of SetValue().

In my case, where Equals() has been overridden, the VM property is sometimes _not_ the object that the code is expecting.

Once again, to me, it is _vital_ that the 'object' stored by SetValue() is the object that it was supplied.

If you still disagree, I am happy to provide a simple class implementation that (in my opinion) breaks SetValue().

Coordinator
Nov 19, 2012 at 7:43 PM

In case of string:

person.Name = "Geert";
person.Name = "Geert"';

You think this should provide NotifyPropertyChanged twice? Or do you want the NotifyPropertyChanged twice in case of reference && ! string?

Nov 19, 2012 at 9:13 PM

I believe that you are focusing on the wrong priority. To me, it is an example of 'premature optimisation'.

SetValue() has 2 main tasks

  1. set the backing value from the argument supplied
  2. update data bindings (NotifyPropertyChanged)

In my opinion, the task 1 must be the 1st priority and must always happen. Otherwise SetValue() is broken.

You cite the example where the backing type is string. String is an immutable type, whereas most user types are mutable. Also how often is your example likely to happen in real scenarios?

Which is the worse outcome after calling SetValue():

  1.  NotifyPropertyChanged() is called when it isn't absolutely necessary?
  2. The backing value is not the value the calling code expected.

Even if you are really upset by the 'extra' call to NotifyPropertyChanged(), why not provide an overload of SetValue() for strings?

In any case, when I supply an object to SetValue() , I require that object to be returned when I call GetValue().

 

As an aside, I tested the CSLA.SetProperty() [equivalent to Catel.SetValue()]. In every case SetProperty() set the backing value from the argument.

 

 

 

Nov 20, 2012 at 9:49 AM

hmm, just re read my last post. Reads somewhat harsh. I apologise for that, it wasnt my intention. I am very impressed with the Catel framework & the responsive of the Catel team. So I definitely don't want to be negative about it.

The vast majority of my apps are multi layer, Even simple operations like save result in objects travelling down and back up the layers. IE the saved object returned by the business layer has to be inserted in place of the object that was saved. So it is vital the correct object is being referenced by the VM properties.

I still believe that SetValue() is broken and that other  developers will be caught.

However, now that I know about the issue, I can work around it (eg first call SetValue(null) and then make the 'real' call) with any type that doesn't use reference equality in Equals().

 

As another aside, I have also done some testing with Microsoft's DependencyObject & DependencyProperty and their Setvalue() also uses reference comparisons.

 

Coordinator
Nov 20, 2012 at 2:59 PM

Don't worry too much, we prefer clear communications over politeness ;-)

I think you are right. Setting the value is good. However, what if you subscribed to a specific object with event, then you should also be able to unsubscribe. Thus, we need to both set the value AND notify about the change, although some users might not consider it a change.

Another option is to set a property to control the behavior, but the end users can do this as well. For now, we will enable the SetValue to always accept the value (we will keep the reference check though) and see if it doesn't break any of our unit tests.

Nov 20, 2012 at 6:52 PM

thanks.

Why not consider overloading SetValue() with a string version [SetValue(PropertyData property, string value)]. This version would do exactly what you are doing now.

The object version would use ReferenceEquals when deciding to set the backin value.

Coordinator
Nov 23, 2012 at 2:14 PM

I created this method (here are the docs only):

        /// <summary>
        ///   Checks whether the 2 specified objects are equal references. This method is better, simple because it also checks boxing so
        ///   2 integers with the same values that are boxed are equal.
        /// <para />
        ///   Two objects are considered equal if one of the following expressions returns true:
        /// <list type="bullet">
        ///   <item><description>Both values are <c>null</c>.</description></item>
        ///   <item><description>Both values have the same reference, checked by <see cref="object.ReferenceEquals"/>.</description></item>
        ///   <item><description>Both values are value types and have the same value.</description></item>
        ///   <item><description>Both values are string type and have the same value.</description></item>
        /// </list>
        /// </summary>
        /// <param name = "object1">The first object.</param>
        /// <param name = "object2">The second object.</param>
        /// <returns><c>true</c> if the objects are equal references; otherwise <c>false</c>.</returns>

Coordinator
Nov 23, 2012 at 2:31 PM

Implemented it, currently releasing a new version via nuget. Let me know if this is enough for you.

Nov 23, 2012 at 7:24 PM

sounds good...

I just tried updating via nuget gui, but it didn't appear to find any updates from (my current) 3.3.

Coordinator
Nov 23, 2012 at 8:54 PM
Edited Nov 24, 2012 at 10:52 AM
Make sure you use includeprerelease
Nov 27, 2012 at 9:20 PM

sorry for the late reply, been 'caught' in UAT phase of a project...

 

i guess I am showing my (lack of) nuget knowledge here...

I don't see that option in the nuget gui (VS2010 sp1).

Do I need to use the nuget cmd line?

If so, please give me the cmd to use.

Developer
Nov 27, 2012 at 10:40 PM

You have several options:

1) From VS package manager console you could write Update-Package [PackageName] -IncludePrerelease

2) The VS NuGet UI (since 2.0 or higher) comes with a new combobox with two value "Stable Only" and "Include Prerelease", select the second one.

3) From command line nuget.exe install [PackageName] -prerelease