Bug 513613 - [Tooling] Reinherit a redefined composite state back to a simple state does not work
Summary: [Tooling] Reinherit a redefined composite state back to a simple state does n...
Status: CLOSED FIXED
Alias: None
Product: Papyrus-rt
Classification: Modeling
Component: tool (show other bugs)
Version: 0.9.0   Edit
Hardware: PC Windows 7
: P3 normal
Target Milestone: 1.0.0   Edit
Assignee: Christian Damus CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-14 04:25 EDT by Peter Cigehn CLA
Modified: 2017-04-12 08:59 EDT (History)
2 users (show)

See Also:


Attachments
Example model with an inherited simple state with incoming/outgoing transitions (6.89 KB, application/x-zip-compressed)
2017-04-12 06:33 EDT, Peter Cigehn CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Cigehn CLA 2017-03-14 04:25:55 EDT
As identified as part of the verification of Bug 513195 when you reinherit a redefined composite state back to an inherited simple state, then this does not work as expected. Unfortunately the behavior is inconsistent, and sometimes an exception is thrown and nothing happens. Other times, the re-inherit partly works, but leaves the model in an inconsistent state which causes further issues when the model is closed and then reopened. 

This is somewhat related to the functionality of converting a composite state back to a simple state as tracked by Bug 512295, since the re-inherit operation on the redefined composite state should in practice to the same kind of "reversal" as described in Bug 512295, 

Steps to reproduce:

1) Create a UML-RT model based on the "UML-RT for C++" template
2) Create a superclass capsule with state-machine which have a simple state with some incoming and outgoing transitions
3) Create a subclass capsule that inherits superclass capsule and ensure it have a state-machine
4) Open the state-machine diagram of the subclass capsule
5) Double click on the inherited simple state and answer yes in the dialog to confirm the conversion to a composite state
6) Right click on the redefined composite state and select "Re-inherit Element"

Unfortunately what happens next is a bit inconsistent, and sometimes an exception is thrown, but most of the time the re-inherit seem to partly happen as expected, but leaves the incoming/outgoing transitions redefined. When closing and reopening the model causes further issues and the subclass state-machine diagram is not possible to open any longer.
Comment 1 Eclipse Genie CLA 2017-04-11 16:01:54 EDT
New Gerrit change created: https://git.eclipse.org/r/94865
Comment 3 Christian Damus CLA 2017-04-11 16:41:30 EDT
(In reply to comment #2)
> Gerrit change https://git.eclipse.org/r/94865 was merged to [master].
Comment 4 Peter Cigehn CLA 2017-04-12 06:28:32 EDT
I have tested this in the latest Papyrus-RT build (#560). And it now seem to work better, and no exceptions are thrown and the model is possible to close and reopen without issues.

The incoming/outgoing transitions though, are still left redefined. This kind of make sense, when you only re-inherit the state. But since the consequence of doing the re-inherit is that the transitions are being re-oriented back to have the simple state as its source/target, and thus there is really no need for having them redefined anymore. Sure, the user can now manually reinherit the transitions, but this on the other hand means more work (if there are many incoming/outgoing transitions). And it is kind of confusing also to have redefined elements that in practice does not redefine any of its properties.

When comparing the corresponding re-inherit scenario in the legacy tooling, the incoming/outgoing transitions also gets implicitly re-inherited when re-inheriting the state itself.

What do you say? As I see it, this is more of a general aspect: Whenever the last property of an redefining element is "unset", i.e. inherited from the redefined element in the superclass, then the redefining element can be removed, since it in practice now inherits everything from its redefined element in the superclass anyway. Do you think that such a general principle could be introduced into the framework, or is it something that needs to be done on a case-by-case basis? Should we track such an improvement in another bug, or is something that can be covered for the specific case of ensuring that the incoming/outgoing transitions of the state also gets re-inherited?
Comment 5 Peter Cigehn CLA 2017-04-12 06:33:54 EDT
Created attachment 267767 [details]
Example model with an inherited simple state with incoming/outgoing transitions

Steps to reproduce:

1) Open the attached model
2) Open the state-machine diagram in Capsule2
3) Double-click on State2 and answer yes in the dialog confirming that a composite state shall be created
4) Switch back to the top-level state-machine diagram
5) Right click on State2 and select "Re-inherit Element"
6) Observe how the incoming and two outgoing transitions still are indicated as redefined
7) Save the model and inspect the model file
8) Observe how the three transitions don't have any property values actually redefined, i.e. it simply inherits everything from the superclass and thus it is really no need for it to be redefined

          <transition xmi:type="uml:Transition" xmi:id="_-SYi_B9qEeeAaurnPzd64w" redefinedTransition="_QGmMcB9gEeeAaurnPzd64w"/>
          <transition xmi:type="uml:Transition" xmi:id="_-SYi_R9qEeeAaurnPzd64w" redefinedTransition="_QobHIB9gEeeAaurnPzd64w"/>
          <transition xmi:type="uml:Transition" xmi:id="_-SYi_h9qEeeAaurnPzd64w" redefinedTransition="_RqXsUB9gEeeAaurnPzd64w"/>
Comment 6 Peter Cigehn CLA 2017-04-12 06:43:43 EDT
I did some more testing in the legacy tooling and the behavior when re-inheriting the incoming/outgoing transitions seem to be bit too "stupid". If the transitions for example have redefined effect code, then the re-inherit of the state causes the incoming/outgoing transitions to be completely re-inherited as well, which in practice then causes the redefined effect to be lost.

What I would have expected, was that the, implicit, re-inherit of the incoming/outgoing transitions *only* should have been made for the case when there should not be any need for redefinition, i.e. when its last child element was deleted and the last property of the element itself became "unset".

So if we are going to support the implicit re-inherit of the transitions, then we should do it better than the behavior in the legacy tooling.
Comment 7 Christian Damus CLA 2017-04-12 07:56:49 EDT
It can be discussed whether the tooling should be smart about re-inheriting elements that match their parent definitions:  for example, should this also include the case where a property is explicitly set to the same value as is inherited to avoid inheriting further changes from up the chain?  By all means, raise an enhancement request for the tooling.

But I do not think that this behaviour should be implemented in the UML-RT metamodel because it is not a matter of structural/data integrity.  The tooling needs to decide how to handle situations like these.
Comment 8 Peter Cigehn CLA 2017-04-12 08:20:28 EDT
(In reply to Christian W. Damus from comment #7)
> It can be discussed whether the tooling should be smart about re-inheriting
> elements that match their parent definitions:  for example, should this also
> include the case where a property is explicitly set to the same value as is
> inherited to avoid inheriting further changes from up the chain?  By all
> means, raise an enhancement request for the tooling.

No, if a property is explicitly set in the subclass, then it should be seen as a redefinition (even if it has the same value as in the superclass). I was only referring to the case when it becomes "unset" as in this case. But maybe what you are saying that it cannot be distinguished between these two different cases?

I just felt that for this specific case, it should have been possible to "detect" that the transition no longer needed to be redefined, since it did not persist any additional properties (all of them were "unset")

> 
> But I do not think that this behaviour should be implemented in the UML-RT
> metamodel because it is not a matter of structural/data integrity.  The
> tooling needs to decide how to handle situations like these.

Okay, so this will then be on a case-by-case basis in the tooling. I will write a separate bug for the enhancement to also re-inherit the transitions in this specific case (to match, but still improve, the behavior in the legacy tooling).
Comment 9 Christian Damus CLA 2017-04-12 08:26:59 EDT
(In reply to Peter Cigehn from comment #8)
> 
> No, if a property is explicitly set in the subclass, then it should be seen
> as a redefinition (even if it has the same value as in the superclass). I

Okay, good, we agree on that.

> was only referring to the case when it becomes "unset" as in this case. But
> maybe what you are saying that it cannot be distinguished between these two
> different cases?

Actually, that is a problem, and it is why the transition vertices actually appear to become unset in this case.  The deletion of the connection points of the (formerly) composite state sets the transitions' source/target references to null, which is indistinguishable from the unset state because in the Ecore representation of these properties they are not "unsettable".


> Okay, so this will then be on a case-by-case basis in the tooling. I will
> write a separate bug for the enhancement to also re-inherit the transitions
> in this specific case (to match, but still improve, the behavior in the
> legacy tooling).

It may not have to be a case-by-base thing.  The tooling may be able to implement a generic algorithm, if that is what we want.  But it may be that we only want it applied in certain cases?  I don't know.
Comment 10 Peter Cigehn CLA 2017-04-12 08:40:54 EDT
(In reply to Christian W. Damus from comment #9)
> (In reply to Peter Cigehn from comment #8)
> > was only referring to the case when it becomes "unset" as in this case. But
> > maybe what you are saying that it cannot be distinguished between these two
> > different cases?
> 
> Actually, that is a problem, and it is why the transition vertices actually
> appear to become unset in this case.  The deletion of the connection points
> of the (formerly) composite state sets the transitions' source/target
> references to null, which is indistinguishable from the unset state because
> in the Ecore representation of these properties they are not "unsettable".
> 

But what you are saying is that this then worked "by luck", i.e. the fact the transition now "automagically" inheriting its new (or former) source/target in form of the now simple state. If I compare with the corresponding feature (which we currently still lack) tracked by Bug 512295, where the re-orientation of the transitions back to the simple state I guess would have to be made a bit more explicit (in that case you cannot rely on the setting the source/target to null when the entry/exit points are being removed).

> 
> > Okay, so this will then be on a case-by-case basis in the tooling. I will
> > write a separate bug for the enhancement to also re-inherit the transitions
> > in this specific case (to match, but still improve, the behavior in the
> > legacy tooling).
> 
> It may not have to be a case-by-base thing.  The tooling may be able to
> implement a generic algorithm, if that is what we want.  But it may be that
> we only want it applied in certain cases?  I don't know.

I don't know either. I will write a bug tracking this specific case with the transitions at least. If we then solve with a generic algorithm or not can be decided later.
Comment 11 Peter Cigehn CLA 2017-04-12 08:53:17 EDT
(In reply to Peter Cigehn from comment #10)
> I don't know either. I will write a bug tracking this specific case with the
> transitions at least. If we then solve with a generic algorithm or not can
> be decided later.

I wrote Bug 515179 to track the implicit re-inherit of the incoming/outgoing transitions.
Comment 12 Peter Cigehn CLA 2017-04-12 08:59:13 EDT
Verified to be fixed in the latest Papyrus-RT build (#560). You are now able to re-inherit a composite state back to be a simple state. 

The incoming/outgoing transitions are left re-inherited though and the improvement to implicitly inherit those as well is tracked separately with Bug 515179.
Comment 13 Peter Cigehn CLA 2017-04-12 08:59:26 EDT
Closing as verified fixed.