Bug 340587

Comment 1 Rimvydas 2011-03-21 12:57:44 EDT

Created attachment 191624 [details]
Test case.

Comment 2 Ed Merks 2011-03-21 13:59:21 EDT

I'm not sure we should change this behavior.  The Javadoc is pretty explicit about what calling eUnset does:

If the feature is many-valued, the value must be an EList and that list is cleared. Otherwise, the value of the feature of the object is set to the feature's default value or the meta class's default value, as appropriate. If the feature is unsettable, the modeled state becomes unset. In any case, the feature will no longer be considered set. 

So then the question is what should setting a container feature to null do?  

The generated and dynamic patterns, as you know, look like this:

  public void setRefC_A(C newRefC_A)
  {
    if (newRefC_A != eInternalContainer() || (eContainerFeatureID() != TestPackage.A__REF_CA && newRefC_A != null))
    {

One could certainly argue reasonably that we should have implemented this differently.  But given that it's very long established behavior, is it reasonable at this point to change it?  

My sense is that it's not a good idea and that we're as likely to make existing clients unhappy as we are to make you happy.

How is this issue important to you that it justifies the risk of a behavior change adversely impacting existing clients?

Comment 3 Rimvydas 2011-03-21 16:10:49 EDT

I agree with you that after unset the feature should be no longer considered as set but unset of one feature should not unset another feature. The feature that was unset had not been set before so I think that unset of the feature should not change objects.

We are working on UML metamodel implementation. In UML an object can be contained by objects of different classes. E.g. UML class can be contained by a package or another class. So if class A is contained by a package and we unset "class" reference (opposite of nestedClassifier) of the class A then the class A will be removed from the containing package. I do not think that many people would expect such behavior.

On set of a container feature to null it is required to check if a new container feature ID is equal to current container feature ID.

Comment 4 Ed Merks 2011-03-21 16:25:46 EDT

As I said, you can reasonably argue your case, and, if we were back in 2002, I'd likely be inclined to agree. But now, if we change long established behavior, someone can reasonably object that it's worked that way for years and now we've broken them.  I.e., "when we set the container, whether to null or otherwise, it will/should always remove it from its current container." 

So at this point in the evolution, I need some strong argument that you're not just facing an academic problem but a fundamentally important one.  I.e., why are you unsetting references that don't need to be unset in the first place?  After all, you'd end up with a pointless notification, so why do you choose to incur that expense?

Comment 5 Rimvydas 2011-03-22 04:17:22 EDT

So your suggested solution for the problem is to check whether the feature is set before unsetting it. Am I right ?

Comment 6 Ed Merks 2011-03-22 13:38:06 EDT

I'm not actually sure what your problem is.  I.e., how are you currently deciding which features to call unset on?  I assume you're not simply calling unset on all features so there must be some logic already and that logic may as well avoid calling any methods that are pointless.  So yes, if eIsSet is false, there's no point in calling anything, which avoids notifications going out.

Comment 7 Ed Merks 2011-03-27 13:22:59 EDT

Given any associated problems are easily avoided and considering in the fact that changing long-established behavior could adversely affect existing clients, I think it's better not to change anything.