Bug 467545 - [UML-RT] PapyrusRT shall provide a UML specific implementation to support redefinition
Summary: [UML-RT] PapyrusRT shall provide a UML specific implementation to support red...
Status: CLOSED FIXED
Alias: None
Product: Papyrus-rt
Classification: Modeling
Component: core (show other bugs)
Version: 0.7.1   Edit
Hardware: All All
: P2 major
Target Milestone: 0.9.0   Edit
Assignee: Christian Damus CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 468030 507618 508404 508629 509383
Blocks: 411009 507552 508102 510188 510189 510315
  Show dependency tree
 
Reported: 2015-05-19 03:53 EDT by Remi Schnekenburger CLA
Modified: 2017-03-23 14:45 EDT (History)
7 users (show)

See Also:


Attachments
Visualization of inherited and redefined elements in legacy tooling (268.82 KB, image/png)
2016-11-25 03:59 EST, Peter Cigehn CLA
no flags Details
Example legacy models with redefinition with different combinations of moving (13.27 KB, application/x-zip-compressed)
2016-11-25 04:35 EST, Peter Cigehn CLA
no flags Details
Screen shot showing the icon for the re-inherit menu choice (1.43 KB, image/png)
2016-12-01 10:00 EST, Peter Cigehn CLA
no flags Details
Example model which shows ports not following its superclass (6.42 KB, application/x-zip-compressed)
2016-12-22 05:46 EST, Peter Cigehn CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Remi Schnekenburger CLA 2015-05-19 03:53:30 EDT
Papyrus shall provide a specific UML implementation to support redefinition and specific UML-RT concepts. This UML implementation should not pollute standard UML implementation and shall only be used when editing a UML-RT profiled model.
Comment 1 Remi Schnekenburger CLA 2015-12-07 17:27:08 EST
Moving to Papyrus-RT project, setting target value to 1.0.0, and revert status to new
Comment 2 Charles Rivet CLA 2016-09-01 09:28:32 EDT
Moving version to 0.7.1, given the date of the comments, and target milestone to 0.9/MVP2 (potential 1.0), to reflect current plan.
Comment 3 Christian Damus CLA 2016-11-23 16:02:24 EST
This feature, as well as several other bugs dealing with capsule structure and protocol inheritance, is being developed on a feature branch regular builds, here:

    https://hudson.eclipse.org/papyrus-rt/job/Papyrus-RT-Feature-Inheritance/

A demonstration of the first stages of capsule structure diagram inheritance is presented here:

    http://youtu.be/AG-lizXP5rE
Comment 4 Peter Cigehn CLA 2016-11-24 05:03:03 EST
(In reply to Christian W. Damus from comment #3)
> A demonstration of the first stages of capsule structure diagram inheritance
> is presented here:
> 
>     http://youtu.be/AG-lizXP5rE

I have looked at the video and it looks like a really great start! Good work!

A few comments:

1) I would not have expected that the change of the position/size of the capsule part to trigger a redefinition in the semantic model. Since it is only a layout "redefinition" (not sure if it even should be considered a redefinition per se), the semantic model should not have been updated in that situation. This is also how the legacy tooling behaves, and I guess we need to consider the model migration scenario here as well. Only when you actually redefine the capsule part (semantically), e.g. redefining its typing capsule, the redefinition should be persisted in the semantic model.

2) I see that the "virtual" nodes in the model explorer for the inherited (but not yet redefined) elements (ports and capsule parts) still have the RTRedefinedElement stereotype applied. As I have indicated before, I find this a bit confusing (maybe the planned annotation for inherited, redefined and excluded elements, as proposed in Bug 507529, will make this a bit more clear), and as I tried to explain in the off-line mail thread, when considering multi-level inheritance we need to handle this in a more consistent way. Here is a quote of what I wrote off-line: 

"On the other hand, if you have multi-level inheritance, e.g. for example C1 <- C2 <- C3, where C1 have a port p which is redefined in the context of C2, then I would expect the port shown for C3, will indeed be the redefined port in C2 with the RTRedefinedElement applied since it has been redefined in that context and is now inherited (as-is) to C3. But if it was *not* redefined in C2, then the port that you see in C3 *is* the port inherited from C1 (without the RTRedefinedElement stereotype applied).

To make this consistent, the RTDefinedElement stereotype shall not be shown until the element actually is redefined, either in the current context itself, or a earlier context in the inheritance chain. In this scenarios, you will also see the different between the value of the rootFragment property of the RTRedefinedElement stereotype vs. the standard UML property redefinedElement, since the former will reference the root in C1, whereas the latter will reference the "previous" redefinition in C2."

But maybe, you have not looked into these aspect more yet.

3) I guess that you have not yet looked into making the inherited views in a lighter border (and fill) color to make them visibly different than views directly owned by the specialized capsule.
Comment 5 Christian Damus CLA 2016-11-24 19:44:25 EST
(In reply to Peter Cigehn from comment #4)

Does this address some of your concerns?

    https://youtu.be/5kFtDFxx-V4

Note that, for now, I just have inherited ports and parts painted in somewhat washed out tones.  I would prefer some kind of hatching pattern in the alpha channel or something more distinctive than just a lighter hue, but that is beyond my current knowledge of GEF.

Eliminating the «RTRedefinedElement» stereotype on the inherited elements in the explorer is on the to-do list for next week as a part of exclusion support.
Comment 6 Peter Cigehn CLA 2016-11-25 03:59:24 EST
Created attachment 265578 [details]
Visualization of inherited and redefined elements in legacy tooling

(In reply to Christian W. Damus from comment #5)
> (In reply to Peter Cigehn from comment #4)
> 
> Does this address some of your concerns?
> 
>     https://youtu.be/5kFtDFxx-V4
> 
> Note that, for now, I just have inherited ports and parts painted in
> somewhat washed out tones.  I would prefer some kind of hatching pattern in
> the alpha channel or something more distinctive than just a lighter hue, but
> that is beyond my current knowledge of GEF.
> 
> Eliminating the «RTRedefinedElement» stereotype on the inherited elements in
> the explorer is on the to-do list for next week as a part of exclusion
> support.

I've checked the video and yes indeed, it looks better now! A few comments though... :)

Regarding the visualization of inherited, and redefined, elements I have provided an attachment which should show how it looks like in the legacy tooling. We have an inheritance chain C1 <- C2 <- C3 where C2 adds some own ports and capsule parts, and C3 redefines two the capsule parts addd in C2. Observe how the label of the capsule parts is in bold and the redefinition annotation in the top right corner of the capsule part to clearly indicate that the part, and its typing capsule, is redefined.

One thing to consider if we are going to make the visualization more distinctive, we must consider that we already have a hatching pattern for optional (and plugin) capsule parts, and the dashed border for plugin capsule parts (keep in mind that we have a slightly different notation for plugin capsule parts compared to the legacy tooling, more aligned with the notation in SysML for shared aggregation). So the way we then make it more distinctive, of course must work well for optional and plugin capsule parts as well.

In the video we could see that when you redefined the element, it turned back to look like a locally defined element. It should still keep the lighter color, but get the little redefinition annotations (as indicated in  Bug 507529). Only truly local element should have their original color, whereas both inherited but not yet redefined, and inherited and redefined, should have the lighter color.

Regarding the bug that you made an annotation about on the video, I do no necessarily think that it was. If you only redefine the element in the semantic model without moving/resizing it in the notation model, then there should not be a need to persist anything in the notation model. I checked this in the legacy tooling, and there is nothing persisted in the notation model if you *only* redefine in the semantic model. It is not until you also move/resize it, that it needs to persisted in the notation model. I can see some inconsistent(?) behavior though in the legacy tooling regarding this. I can attach an example model to show how it behaves in the legacy tooling.
Comment 7 Peter Cigehn CLA 2016-11-25 04:35:58 EST
Created attachment 265579 [details]
Example legacy models with redefinition with different combinations of moving

In the attached project there are three models

1) RedefinitionWithoutMoving
2) RedefinitionWithMoving
3) RedefinitionBothWithAndWithoutMoving

As can be seen in 1) there is nothing persisted in the notation model when the part is being redefined. In 2) one can see that the redefined and moved part is persisted in the notation model. Note however that the element referenced is the inherited part, not the redefined part. In 3) one case see that *both* parts actually gets persisted in the notation model, even though only one of them is moved. Not fully sure why this happens, but I guess there are some reason for this behavior. The drawback with this is if we have the scenario where the superclass capsule is stored in a separate model, e.g. a library, and you change the layout of the superclass capsule "off-line" from any of the subclass capsules, then the notation model for the subclass capsule needs to be updated the next time the model containing the subclass capsule is saved. To be able to keep track of if the position shall be inherited from the superclass capsules or not, there seem to be a special style used:

                <styles xmi:type="rtnotation:RedefinableViewStyle" xmi:id="_ecEGwLLvEeagpqSp2vxxcw" viewPropertiesFromSuperClass="false"/>

I guess the inconsistent(?) behavior that the part that is *not* moved, gets persisted in the notation model just because another part is moved, could be questioned if that is what you want. But I guess we need to be able to handle it when importing legacy models. So I guess all cases needs to be supported, i.e. a) the position is inherited from the superclass view and nothing is persisted, b) the position is inherited from the superclass view but the position have been persisted and "duplicated", i.e. the case in model 3), c) the position has been "overridden" and thus of course needs to be persisted in the notation model, the case the basic case in model 2)

I guess it might be good to already now start to consider the import of legacy models, so that we know that we can handle the way the legacy tooling handles the layout inheritance and do go off in a completely different direction which might make it hard to import models.
Comment 8 Christian Damus CLA 2016-11-28 11:38:35 EST
(In reply to Peter Cigehn from comment #7)

The reason why in RSA sometimes you see more views persisted in the model is that the GMF run-time is quite aggressive about persisting views.  Specifically, when any view in a diagram needs to be persisted for some reason (such as tweaking its position or size) then *all* of the children of that view's parent view are persisted, even if otherwise they would not need to be.  The GMF run-time does this to avoid failure of undo/redo due to inconsistencies in the ChangeDescriptions recorded by EMF as the model is edited (the notation model uses a transient containment reference that confuses things).

I have seen these undo/redo failures in my work on diagram inheritance, too, so I have in fact had to just let the inherited views be persisted.  This actually happens to be more consistent with Papyrus diagrams anyways because whereas in RSA the canonical diagrams don't persist views by default (although persistence spreads virally as you have observed), in Papyrus they always do persist their views.

In consequence, I have implemented a style to record whether a view has overridden its inherited layout, thus separating the visual inheritance from the semantic inheritance as we wanted it.  Also, now all views of inherited elements, whether they are (semantically) redefined in the inheriting capsule or not, reference the root element of the redefinition chain to ensure that a "real" element is always referenced by the serialized XMI.

Persistence of views notwithstanding, I still am suppressing the hidden applied-stereotype comment view and its link edge for all inherited elements that are not (semantically) redefined in the inheriting capsule.

You can see these changes in the latest branch build (#13).  That build also has a change to leave the «rtRedefinedElement» stereotype off of inherited elements that are not redefined, unless the inherited element itself has that stereotype applied because it is a redefinition.
Comment 9 Peter Cigehn CLA 2016-11-29 08:42:10 EST
(In reply to Christian W. Damus from comment #8)

Thanks for the explanation! Yes, I have seen this persistence "spreading virally" in other situations as well, and this gives an explanation why this happens. I started to realize that we probably have some issues related to this when importing legacy models, which I have started to look at now when the core issue in Bug 503075 have been solved. Exactly as you say, canonical diagrams (with complete auto-layout) does not persist anything, but as soon as you move something, then a lot of other (seemingly unrelated) stuff get persisted. But still not everything is persisted and in some cases, e.g. the position information (for the not moved stuff) is not persisted byt still being auto-layout. This in its turn causes issues when importing these models. I will probably elaborate regarding this in a separate bug.

Regarding the builds of the branch, how is that supposed to be "consumed"? I guess I still have to checkout the branch in my development environment and test in a run-time workbench as usual? There is no p2 repo being produced from which I can install, I guess?
Comment 10 Christian Damus CLA 2016-11-29 11:04:42 EST
(In reply to Peter Cigehn from comment #9)
> 
> Regarding the builds of the branch, how is that supposed to be "consumed"? I
> guess I still have to checkout the branch in my development environment and
> test in a run-time workbench as usual? There is no p2 repo being produced
> from which I can install, I guess?

Good point.  Now that I am also working on the tooling plug-ins, I have updated the branch build to build those also and to archive an update site.  You can install from

    https://hudson.eclipse.org/papyrus-rt/job/Papyrus-RT-Feature-Inheritance/lastSuccessfulBuild/artifact/updates/

Just be warned that there are many bugs (especially in undo/redo) that it is not worth reporting, yet, because this is work in progress.  It is also exposing latent bugs in Papyrus, which always complicates matters.
Comment 11 Peter Cigehn CLA 2016-11-30 07:43:35 EST
(In reply to Peter Cigehn from comment #9)
> Thanks for the explanation! Yes, I have seen this persistence "spreading
> virally" in other situations as well, and this gives an explanation why this
> happens. I started to realize that we probably have some issues related to
> this when importing legacy models, which I have started to look at now when
> the core issue in Bug 503075 have been solved. Exactly as you say, canonical
> diagrams (with complete auto-layout) does not persist anything, but as soon
> as you move something, then a lot of other (seemingly unrelated) stuff get
> persisted. But still not everything is persisted and in some cases, e.g. the
> position information (for the not moved stuff) is not persisted byt still
> being auto-layout. This in its turn causes issues when importing these
> models. I will probably elaborate regarding this in a separate bug.

I wrote Bug 508438 related to this (import) issue. As can be seen from the model attached to that bug, there is only partial position information persistently stored for the information that "spreads virally", causing confusion(?) for the auto-layout in Papyrus-RT.
Comment 12 Christian Damus CLA 2016-11-30 18:01:39 EST
Here's a first look at exclusion of inherited elements in the Capsule Structure Diagram:  https://youtu.be/enu2sya-3j4   Just about 2¼ minutes long
Comment 13 Peter Cigehn CLA 2016-12-01 04:13:43 EST
(In reply to Christian W. Damus from comment #12)
> Here's a first look at exclusion of inherited elements in the Capsule
> Structure Diagram:  https://youtu.be/enu2sya-3j4   Just about 2¼ minutes long

I checked the video and it looks like a good start. Now when you have the exclusion annotation in place (as tracked by Bug 507529), have you also added the annotation for inherited and redefined elements as well? I think it would be good to get that in place also to make things in general more clear.

On thing that I noted though was when you used "Re-inherit element" the model explorer still showed the RTRedefinedElement stereotype being applied on the (what I assume) now is the inherited element (the inherited annotation would have been good to have to clearly see this). Not sure if that was just a refresh glitch or if the RTRedefinedElement stereotype was not "unapplied" for that virtual node now showing the inherited element.

One thing to be noted also: If you would have had the visual inheritance of the connector in place, i.e. it would have been visualized also in the specializing capsule with washed out color, then excluding the port as you did, should have removed the visualization of the connector in the diagram, but not excluded the connector itself (in the semantic model).
Comment 14 Peter Cigehn CLA 2016-12-01 04:41:54 EST
(In reply to Peter Cigehn from comment #13)
> On thing that I noted though was when you used "Re-inherit element" the
> model explorer still showed the RTRedefinedElement stereotype being applied
> on the (what I assume) now is the inherited element (the inherited
> annotation would have been good to have to clearly see this). Not sure if
> that was just a refresh glitch or if the RTRedefinedElement stereotype was
> not "unapplied" for that virtual node now showing the inherited element.

One more comment regarding the "Re-inherit element" menu choice. It would be good if the menu choice had an icon (compare with the red cross on the Delete menu choice). The legacy tooling have a bent inheritance arrow for this menu choice.
Comment 15 Christian Damus CLA 2016-12-01 08:33:16 EST
(In reply to Peter Cigehn from comment #13)
> (In reply to Christian W. Damus from comment #12)
> > Here's a first look at exclusion of inherited elements in the Capsule
> > Structure Diagram:  https://youtu.be/enu2sya-3j4   Just about 2¼ minutes long
> 
> I checked the video and it looks like a good start. Now when you have the
> exclusion annotation in place (as tracked by Bug 507529), have you also
> added the annotation for inherited and redefined elements as well? I think
> it would be good to get that in place also to make things in general more
> clear.

Indeed, the exclusion annotation was a necessity for demonstration but the inherited and redefined annotations will easily follow the same pattern.


> On thing that I noted though was when you used "Re-inherit element" the
> model explorer still showed the RTRedefinedElement stereotype being applied
> on the (what I assume) now is the inherited element (the inherited
> annotation would have been good to have to clearly see this). Not sure if
> that was just a refresh glitch or if the RTRedefinedElement stereotype was
> not "unapplied" for that virtual node now showing the inherited element.

Yes, it's on my to-do list to completely strip the element when it is excluded and then fix up the re-inherit to simply delete the «rtRedefinedElement» stereotype application and make the element purely inherited.  Currently, it still actually exists in the model.

I also have a problem in which undoing a re-inherit action properly excludes the element (as shown in the explorer) but it remains on the diagram.  So many details!


> One thing to be noted also: If you would have had the visual inheritance of
> the connector in place, i.e. it would have been visualized also in the
> specializing capsule with washed out color, then excluding the port as you
> did, should have removed the visualization of the connector in the diagram,
> but not excluded the connector itself (in the semantic model).

I have been putting off the connectors because they are more complex than parts and ports.  It will be time, soon, to tackle those.  One complication will be that a connector that attaches to a port may be in a model that is not open at the time when that port is excluded.  I shall have to think how to account for that.

Which leads to another question:  I was thinking that perhaps the exclusion and re-inheritance actions should not be available on ports on parts, because they are not elements actually inherited by the capsule that is the context of the diagram.  It might be confusing that this actually edits some other capsule, with consequences that are not immediately to be seen.  However, we are able to delete and add non-inherited ports on parts, which likewise has hidden consequences, so maybe we should be consistent with that.


(In reply to Peter Cigehn from comment #14)
> 
> One more comment regarding the "Re-inherit element" menu choice. It would be
> good if the menu choice had an icon (compare with the red cross on the
> Delete menu choice). The legacy tooling have a bent inheritance arrow for
> this menu choice.

Interesting.  I agree that the menu actions need icons, and I thought maybe an inheritance arrow (like the decoration in the Model Explorer) would be good for the re-inherit action and the same arrow with a slash through it for the exclusion action.  Bent arrows are commonly used to indicate undo actions.  What do you think?
Comment 16 Peter Cigehn CLA 2016-12-01 10:00:57 EST
Created attachment 265677 [details]
Screen shot showing the icon for the re-inherit menu choice

(In reply to Christian W. Damus from comment #15)
> (In reply to Peter Cigehn from comment #13)
> I have been putting off the connectors because they are more complex than
> parts and ports.  It will be time, soon, to tackle those.  One complication
> will be that a connector that attaches to a port may be in a model that is
> not open at the time when that port is excluded.  I shall have to think how
> to account for that.

I guess that in general there will be lot more cases where you can exclude, or delete, stuff that "sweeps away the feet" for models not loaded into memory. I wrote some general comment about possible "reconcilation" (not sure if that is the correct term for it) support in Bug 507552 regarding this.

> 
> Which leads to another question:  I was thinking that perhaps the exclusion
> and re-inheritance actions should not be available on ports on parts,
> because they are not elements actually inherited by the capsule that is the
> context of the diagram.  It might be confusing that this actually edits some
> other capsule, with consequences that are not immediately to be seen. 

I feel that this is related to exactly what we are debating on the developer maling list. The legacy tooling in RSARTE do allow that you to exclude a port in the context of a capsule part (you cannot re-inherit it though since the excluded port is only visible/avilable in the context of the capsule), in the corresponding way as you also can redefine it, e.g. by simply selecting the port and then choose a new protocol via the properties view. I am not really sure how to proceed with these discussions since we apparently have so fundamentally different views regarding this. Yes, if we would have had the meta-model of RoseRT, I would completely agree since then it would be the port role you saw on the outside (and you never exclude port roles, but only ports). 

Currently you can also drag-and-drop a protocol onto a capsule part to create a port on the typing capsule (legacy tooling also supports this), and you can right click on a part in the model explorer and create a new port (on the typing capsule), which then does not make sense either since you are actually changing the capsule and not the capsule part. All this boils down to what "mental model" we should have regarding ports on capsule parts. If we are going to change the principles to limit what you can do with ports from the "outside", then we should really take a complete grip of this, since we are going to change a lot of principles from the legacy tooling.

> However, we are able to delete and add non-inherited ports on parts, which
> likewise has hidden consequences, so maybe we should be consistent with that.
> 

Exactly my point. If we are going to be consistent, then we need to be consistent for all aspects of the ports on parts, and probably also have a different properties view, with a limited set of (read-only) properties exactly as it works in RoseRT. I am not sure that this is really the direction we want to go, since it had some pains that you had to go "inside" to edit some aspects of a port (however it did exist a short cut to bring up the properties dialog for a port directly from the port role, as you also could bring up the protocol properties dialog). The transition to RSARTE made several editing scenarios easier, when the port actually was available also on the "outside". Sure, it could cause confusion, but if you just make it clear that it *is* the same port shown on the outside as on the inside and consistently always treat them the same, and people are educated that this is how it is supposed to work, then I don't see this as a problem. But sure, as long as we try to envision ourselves the meta-model of RoseRT, when it is fact not, then this will continue to be confusing.

> 
> (In reply to Peter Cigehn from comment #14)
> > 
> > One more comment regarding the "Re-inherit element" menu choice. It would be
> > good if the menu choice had an icon (compare with the red cross on the
> > Delete menu choice). The legacy tooling have a bent inheritance arrow for
> > this menu choice.
> 
> Interesting.  I agree that the menu actions need icons, and I thought maybe
> an inheritance arrow (like the decoration in the Model Explorer) would be
> good for the re-inherit action and the same arrow with a slash through it
> for the exclusion action.  Bent arrows are commonly used to indicate undo
> actions.  What do you think?

Regarding the symbol for exclusion, the legacy tooling (two earlier generations) often combines the act of deleting and excluding to the same menu choice or button, and thus only use the (same) red cross for both cases (I mention this in Bug 507529, bullet 3). Often it makes sense to see deletion and exclusion being the same kind of thing.

Regarding the bent arrow used for undo, e.g. as seen on the Edit menu in the Eclipse top menu, is more "rounded". The re-inherit arrow is a 90 degree angle as can be seen from the attached screen shot. So hopefully this make a clear distinction and not cause any confusion.
Comment 17 Peter Cigehn CLA 2016-12-01 10:05:08 EST
Adding Ernesto on Cc since I know, based on the discussion on the developer maling list, that he will have opinions about what I wrote in Comment 16.
Comment 18 Christian Damus CLA 2016-12-01 12:43:00 EST
A small update on iconography etc. in the UI:

    http://youtu.be/B2-ct5zZ1cU

I, too, would prefer to re-target the Delete action for inherited elements to just exclude them.  However, the Model Explorer hijacks the Delete command to make it impossible to override its handler.  I need to look into the reasons for this and what I can do (in Neon.x) to fix it.  Note that it is not sufficient just to use edit-helper advice to replace the default deletion behaviour to do an exclude, because other advice registered on the DeleteElementRequest expects deletion semantics.  So, I really do need to override the delete handler.  In the mean-time, I am therefore maintaining a distinct "Exclude Element" menu action.  And I should probably disabled deletion of inherited elements, which I can do with advice.
Comment 19 Ernesto Posse CLA 2016-12-01 13:26:56 EST
(In reply to Peter Cigehn from comment #16)
> Created attachment 265677 [details]
> Screen shot showing the icon for the re-inherit menu choice
> 
> (In reply to Christian W. Damus from comment #15)
> > (In reply to Peter Cigehn from comment #13)
> > I have been putting off the connectors because they are more complex than
> > parts and ports.  It will be time, soon, to tackle those.  One complication
> > will be that a connector that attaches to a port may be in a model that is
> > not open at the time when that port is excluded.  I shall have to think how
> > to account for that.
> 
> I guess that in general there will be lot more cases where you can exclude,
> or delete, stuff that "sweeps away the feet" for models not loaded into
> memory. I wrote some general comment about possible "reconcilation" (not
> sure if that is the correct term for it) support in Bug 507552 regarding
> this.
> 
> > 
> > Which leads to another question:  I was thinking that perhaps the exclusion
> > and re-inheritance actions should not be available on ports on parts,
> > because they are not elements actually inherited by the capsule that is the
> > context of the diagram.  It might be confusing that this actually edits some
> > other capsule, with consequences that are not immediately to be seen. 

I'm not sure this is just a tooling question, but a language question, and I think Bran should have a say on whether this should or should not be allowed.  Personally I don't like allowing the exclusion of ports-on-a-part because as you say, it entails modifying another capsule. In fact, I don't like the possibility of editing a port "on the outside" at all. It might result in invalidating other capsules that use the capsule whose port is edited. If it was up to me, I wouldn't allow editing a port "on the outside" at all. 


> I feel that this is related to exactly what we are debating on the developer
> maling list. The legacy tooling in RSARTE do allow that you to exclude a
> port in the context of a capsule part (you cannot re-inherit it though since
> the excluded port is only visible/avilable in the context of the capsule),

And what does it mean to exclude a port-on-a-part (as opposed to the port itself in its defining capsule)? Does it mean that if I have a capsule A with a part b:B where B has a port p and we exclude A.b.p then any capsule C which is a subclass of A will inherit A.b but A.b.p will not be visible in C? Does it mean that if there was a connector in A to A.b.p, the connector is also excluded?

What I mentioned in the mailing list is that in the context of a part, you don't get the port itself. Yes, you can edit it right there, but what really exists in the model is a ConnectorEnd, which acts as a *proxy* for the actual port. So allowing to exclude a port-on-a-part would entail the exclusion of a ConnectorEnd, otherwise, you are excluding the port itself, which results in excluding it from all clients of the port's owner.

So if we are talking about excluding a port-on-a-part and not the port itself, we are talking about excluding a ConnectorEnd, and according to the UML-RT profile spec document, the RTRedefinedElement needs to be applied to the UML element to be excluded, and the document has an explicit rule describing to which elements this stereotype can be applied. ConnectorEnd is not one of them. If this needs to change, then maybe we should have Bran involved in this discussion.


> in the corresponding way as you also can redefine it, e.g. by simply
> selecting the port and then choose a new protocol via the properties view. I
> am not really sure how to proceed with these discussions since we apparently
> have so fundamentally different views regarding this. Yes, if we would have

Do we really have such fundamental different views? It looks to me that it is a matter of terminology and whether we are talking from the perspective of a user (which I think is your view, cf. "they are the 'same' port") or from the perspective of the underlying data-structures involved (which is what I have been talking about,  cf. "they are different objects, one being a proxy that points to the actual port").

In any case, you can proceed in any way you wish. The issue seems to be about tooling. My area is codegen and textual. As long as the data-structures involved are consistent with what the UML-RT spec document and the profile prescribe, and as long as you give me a precise definition of what does it mean, for example, to exclude a "port on the outside" and whether it differs from excluding a "port on the inside", I can use it. As for the tooling/visualization issues, I am just giving my opinion as a user, which is what was asked in the original message.


> had the meta-model of RoseRT, I would completely agree since then it would
> be the port role you saw on the outside (and you never exclude port roles,
> but only ports). 
> 
> Currently you can also drag-and-drop a protocol onto a capsule part to
> create a port on the typing capsule (legacy tooling also supports this), and

I also dislike this, for the same reason that I mentioned above, about editing something in a context that might invalidate it in other contexts. Well, adding ports may not invalidate the capsule, but changing their protocols, or removing them might.

> you can right click on a part in the model explorer and create a new port
> (on the typing capsule), which then does not make sense either since you are
> actually changing the capsule and not the capsule part. All this boils down
> to what "mental model" we should have regarding ports on capsule parts. If
> we are going to change the principles to limit what you can do with ports
> from the "outside", then we should really take a complete grip of this,
> since we are going to change a lot of principles from the legacy tooling.
> 
> > However, we are able to delete and add non-inherited ports on parts, which
> > likewise has hidden consequences, so maybe we should be consistent with that.
> > 
> 
> Exactly my point. If we are going to be consistent, then we need to be
> consistent for all aspects of the ports on parts, and probably also have a
> different properties view, with a limited set of (read-only) properties
> exactly as it works in RoseRT. I am not sure that this is really the
> direction we want to go, since it had some pains that you had to go "inside"
> to edit some aspects of a port (however it did exist a short cut to bring up
> the properties dialog for a port directly from the port role, as you also
> could bring up the protocol properties dialog). The transition to RSARTE
> made several editing scenarios easier, when the port actually was available
> also on the "outside". Sure, it could cause confusion, but if you just make
> it clear that it *is* the same port shown on the outside as on the inside
> and consistently always treat them the same, and people are educated that
> this is how it is supposed to work, then I don't see this as a problem. But
> sure, as long as we try to envision ourselves the meta-model of RoseRT, when
> it is fact not, then this will continue to be confusing.

Again, only my opinion as a user, but I do not see how recognizing that the port-on-a-part is a proxy of the port, rather than the port itself, is being inconsistent, or proposing a different meta-model (which is not what I'm proposing). Yes, it is convenient to be able to edit a port "on the outside", but personally I think that is quite error prone. As a user, if I'm allowed to change the port "on the outside", which changes the port itself, then I am likely to break a bunch of clients of the port in other contexts, and this would be an even bigger problem if those clients are in other models which are not loaded. Perhaps, some users might prefer to be allowed to edit all objects via their proxies, giving the tool a more "laissez-faire" feeling, and that's fine, but if we want consistency there, then that rule should be applied to every object in the model that acts as a proxy for another object. For example, it should be possible to directly edit a capsule C by editing any part elsewhere which is typed by C. But again, it's only my opinion. If it is decided that allowing editing on the outside is the way to go (or continue), then yes, user documentation, even if no-one ever reads it, should make it quite clear what is going to happen, and what are the dangers of editing something on the outside.
Comment 20 Charles Rivet CLA 2016-12-01 15:16:00 EST
(In reply to Ernesto Posse from comment #19)

<...snip...>

> Again, only my opinion as a user, but I do not see how recognizing that the
> port-on-a-part is a proxy of the port, rather than the port itself, is being
> inconsistent, or proposing a different meta-model (which is not what I'm
> proposing). Yes, it is convenient to be able to edit a port "on the
> outside", but personally I think that is quite error prone. As a user, if
> I'm allowed to change the port "on the outside", which changes the port
> itself, then I am likely to break a bunch of clients of the port in other
> contexts, and this would be an even bigger problem if those clients are in
> other models which are not loaded. Perhaps, some users might prefer to be
> allowed to edit all objects via their proxies, giving the tool a more
> "laissez-faire" feeling, and that's fine, but if we want consistency there,
> then that rule should be applied to every object in the model that acts as a
> proxy for another object. For example, it should be possible to directly
> edit a capsule C by editing any part elsewhere which is typed by C. But
> again, it's only my opinion. If it is decided that allowing editing on the
> outside is the way to go (or continue), then yes, user documentation, even
> if no-one ever reads it, should make it quite clear what is going to happen,
> and what are the dangers of editing something on the outside.

I'm with Ernesto on this one. Trying to modify a property (not a property's value) from within an instance is confusing and potentially dangerous with side effects that can spread rapidly. Imagine what that would do for a capsule that is used in other places and/or that has subclasses?

Inheritance/redefinition (and exclusion, which is an arguably (un)necessary evil), should be done at the level of the "class"/capsule, not capsule part so that these actions are explicit. Would you want to modify a C++ class from a class instance (and yes, I do know that this is feasible...with the right knowledge...)?

It just seems to me that we are breaking down some important architectural and historical barriers (e.g. OO principles) that should be left intact.

I can (somewhat) understand what the legacy tool developers wanted, and it was probably based on a request from a customer, but I find that decision rather confusing.

And, like Ernesto, and for what it's worth, it's just my opinion.
Comment 21 Simon Redding CLA 2016-12-05 10:35:13 EST
I do not believe we should support editing the part but the user should have to go to the definition to make changes. Besides the change in workflow, does this have any other implications? Peter seemed to say  that if we adopt this approach, it will have knock on effects that will cause a number of other changes. If this straight forward, then this is how I think we should proceed.
Comment 22 Christian Damus CLA 2016-12-05 14:45:50 EST
(In reply to Simon Redding from comment #21)
> I do not believe we should support editing the part but the user should have
> to go to the definition to make changes. Besides the change in workflow,
> does this have any other implications? Peter seemed to say  that if we adopt
> this approach, it will have knock on effects that will cause a number of
> other changes. If this straight forward, then this is how I think we should
> proceed.

We would need to disable context-menu/toolbar actions on ports-on-parts such as "Delete from Model" and "Exclude Element".  Also the in-line editor (if any) for port names.

In the Properties View, I suppose all of the properties of a port-on-part would have to be disabled except for those that deal only with the notation (e.g., visual styling and applied stereotype appearance).  We may even want to suppress some property tabs altogether, such as Advanced and UML, to further highlight the restricted access to these elements in the containing capsule context.

The Model Explorer does not present ports on parts, so there should be nothing to do in that view.

A question to ask is whether we want to continue to support creation of ports on parts by dropping protocols onto them or via the tool palette.  Although it would seem innocuous (because it is an additive edit), I would suggest not because it would be an incongruous user experience to find that the element just created is not further editable in the same context.
Comment 23 Simon Redding CLA 2016-12-05 15:01:29 EST
I did not realize this was already supported. My mistake. Since it is, then I suggest we leave it as is for now and re-visit in a later release. I do believe that this feature is error prone, but do not feel we will be able to come to agreement in the sort term, so the best approach may be to just leave it as is. In the future, this could be changed to just support opening the definition from the part.
Comment 24 Christian Damus CLA 2016-12-05 15:09:06 EST
(In reply to Simon Redding from comment #23)
> I did not realize this was already supported. My mistake. Since it is, then
> I suggest we leave it as is for now and re-visit in a later release. I do
> believe that this feature is error prone, but do not feel we will be able to
> come to agreement in the sort term, so the best approach may be to just
> leave it as is. In the future, this could be changed to just support opening
> the definition from the part.

Editing ports on parts is supported only by deliberate inaction.  It isn't that the team made an effort to add editing support for ports on parts, only that we did not make an effort to disable editing because it seemed we wanted to be able to edit these things.

However, if we want to continue being able to edit ports on parts, then we still have to decide how we want to visualize inherited ports on parts, because what any given editing gesture actually does depends on whether the port is inherited by the capsule type of the part or not.  In order not to surprise the user, we would have to clearly show that a port is inherited (or not) by the capsule type of the part, regardless of whether the part is inherited by the capsule that is the context of the diagram.  This is the visualization that Ernesto, Charles, and I argued against, but which is what RSARTE implements and Peter argues for.

Agreed?
Comment 25 Simon Redding CLA 2016-12-05 16:33:38 EST
If it is now just down to a visual queue that the port is inherited, why not stick an I on it and be done with it?
Comment 26 Christian Damus CLA 2016-12-05 17:00:54 EST
(In reply to Simon Redding from comment #25)
> If it is now just down to a visual queue that the port is inherited, why not
> stick an I on it and be done with it?

It's not the kind of visual indicator that is at issue; we already have implemented the "washed out" colour as is used for the capsule's own ports and parts.  The question is do we show the inheritance indicator for ports on parts according to whether the part they are on is inherited by the containing capsule (as in RoseRT) or according to whether they are inherited by the capsule that is the part's type (as in RSARTE).

Since it is now decided to continue letting ports on parts be edited in the context of the containing capsule, I think the only way not to confuse the user is to go with the RSARTE convention.  So, let's just do that.
Comment 27 Simon Redding CLA 2016-12-05 17:14:46 EST
Agreed.
Comment 28 Christian Damus CLA 2016-12-05 20:25:41 EST
(In reply to Simon Redding from comment #27)
> Agreed.

Okay, next on my to-do list is to revert from the RoseRT-style inheritance presentation to the RSA-RTE style.  Which I can do now that I have a decent first run of the use cases for connector inheritance:

   https://youtu.be/DvMUY_t7hsk
Comment 29 Simon Redding CLA 2016-12-05 21:15:34 EST
Peter - Can you comment in whether this is what you want? I am having a hard time following this. This is comment 29 on the bug. We need a better way to resolve these. 

Christian - Let's wait until Peter comments before you continue working on it.
Comment 30 Peter Cigehn CLA 2016-12-06 03:03:14 EST
(In reply to Simon Redding from comment #29)
> Peter - Can you comment in whether this is what you want? I am having a hard
> time following this. This is comment 29 on the bug. We need a better way to
> resolve these. 
> 
> Christian - Let's wait until Peter comments before you continue working on
> it.

Yes, what Christian have stated is exactly what I have argued for all along. We shall allow editing of ports in the context of a capsule part, including visualizing their inherited/redefined state in the same way both on the inside of the capsule as well as on the outside of the capsule on the capsule part, in the same way as it is done in RSARTE.

I do want to make a correction though to the statement in Comment 24: We actually have made several very deliberate choices from before, that ports on capsule parts shall be editable in this way. I have already guided us and the project in that direction with respect to a lot of usage scenarios, since that is what I had envisioned all along. Thus this discussion, for me personally, have been extremely frustrating since we all of a sudden got in a situation where a lot of this potentially had to be reverted (apart from also from making the tool harder to use).

I have actually refrained from commenting on this bug several times, to avoid "cluttering" this bug any further, but I am very happy that you finally came to the right conclusion on your own.
Comment 31 Peter Cigehn CLA 2016-12-06 03:43:28 EST
(In reply to Christian W. Damus from comment #28)
> Okay, next on my to-do list is to revert from the RoseRT-style inheritance
> presentation to the RSA-RTE style.  Which I can do now that I have a decent
> first run of the use cases for connector inheritance:
> 
>    https://youtu.be/DvMUY_t7hsk

I checked the video and it looks like a good start!

Regarding the usage scenario for redefinition of the connector (where you used the example of changing the multiplicity), I don't expect this to be something that is used in practice. The only "redefinition" of a connector that is done is to exclude it. 

Keep in mind also that in the legacy tooling the inherited connector is not shown in the model explorer (and not in any table in a properties view either). It is only visible in the diagram (washed out). Not until the connector is excluded, you see the excluded connector in the model explorer (since you now also have a model element created in the semantic model). What should be noted is that in this situation, the connector does not even have any connector ends (they are not shown in the model explorer). It is only the connector itself that is "duplicated" in the semantic model (with the RTRedefinedElement stereitype applied). I am not sure how the alignment with legacy models should be handled. Either the tooling needs to handle the exclusion case where only the connector exist (without connector ends), or the import tool have to "duplicate" the connector ends. I have a feeling that it is better to align with the legacy models, and create a connector without duplication of the connector ends, and possibly let them "inherit" the connector ends, in the corresponding way as I guess we need to handle the ValueSpecifications for inheriting multiplicity. But since it is excluded that is not really necessary either. The only issue here I assume is the constraint in the UML meta-model that a connector *must* have 2 (or more) connector ends (something that I do know that the legacy tooling "violates" in several other places as well, e.g. single ended connectors in interactions). I assume that this something that can be handled with our specific implementation of the UML meta-model (which this bug is all about really).
Comment 32 Christian Damus CLA 2016-12-06 07:58:00 EST
(In reply to Peter Cigehn from comment #31)
> (In reply to Christian W. Damus from comment #28)
> > Okay, next on my to-do list is to revert from the RoseRT-style inheritance
> > presentation to the RSA-RTE style.  Which I can do now that I have a decent
> > first run of the use cases for connector inheritance:
> > 
> >    https://youtu.be/DvMUY_t7hsk
> 
> I checked the video and it looks like a good start!
> 
> Regarding the usage scenario for redefinition of the connector (where you
> used the example of changing the multiplicity), I don't expect this to be
> something that is used in practice. The only "redefinition" of a connector
> that is done is to exclude it.

Yes, I just needed something that could cause a connector to be redefined as in the cases of ports and parts to test the metamodel implementation.  It is not something that will be supported in the UI.
 

> Keep in mind also that in the legacy tooling the inherited connector is not
> shown in the model explorer (and not in any table in a properties view
> either). It is only visible in the diagram (washed out). Not until the

But so is the case also for other inherited elements, correct?  So far, I am using the Model Explorer to provide access to stuff for testing (it takes a while to get selections and such working in the diagrams).


> connector is excluded, you see the excluded connector in the model explorer
> (since you now also have a model element created in the semantic model).
> What should be noted is that in this situation, the connector does not even
> have any connector ends (they are not shown in the model explorer). It is

I am working on making the UML representation as complete as it needs to be in order to be correctly structured and also to behave plausibly when it is manipulated by code that works with the UML API directly.  So, connector ends could have been inherited as they are from the redefined connector (they would be shared elements), but then any code that attempts to work with them would be confused:  then ends of a connector would end up reporting a different connector as their owner and making changes to them would edit the redefined connector.  In the implementation that I have now, the redefining connector automatically maintains ends that shadow (effectively redefine) the ends of the redefined connector.  Any change to these ends causes the entire connector to be redefined and added into the model (no longer being a proxy for the inherited connector).


> only the connector itself that is "duplicated" in the semantic model (with
> the RTRedefinedElement stereitype applied). I am not sure how the alignment
> with legacy models should be handled. Either the tooling needs to handle the
> exclusion case where only the connector exist (without connector ends), or
> the import tool have to "duplicate" the connector ends. I have a feeling
> that it is better to align with the legacy models, and create a connector
> without duplication of the connector ends, and possibly let them "inherit"
> the connector ends, in the corresponding way as I guess we need to handle
> the ValueSpecifications for inheriting multiplicity. But since it is

Now that I have worked out the shadowing of connector ends, I am actually planning to do the same for the value-specifications for multiplicity bounds.  This provides the most consistent experience for code written to the UML API.


> excluded that is not really necessary either. The only issue here I assume
> is the constraint in the UML meta-model that a connector *must* have 2 (or
> more) connector ends (something that I do know that the legacy tooling

That might be an issue, yes, but it is more about meeting the expectations that code (especially extenders and other UML plug-ins) has about the contract of the UML API.  Sharing elements that are meant to be owned is extremely surprising to code that tries to work with them.  So, for me, it's a matter of safety and resilience in the face of UML tools more than validation rules.


> "violates" in several other places as well, e.g. single ended connectors in
> interactions). I assume that this something that can be handled with our
> specific implementation of the UML meta-model (which this bug is all about
> really).

Exactly.  It is handled internally by our specific implementation in a way that is still consistent with some basic UML structural constraints.
Comment 33 Peter Cigehn CLA 2016-12-06 08:18:15 EST
(In reply to Christian W. Damus from comment #32)
> (In reply to Peter Cigehn from comment #31)
> > Keep in mind also that in the legacy tooling the inherited connector is not
> > shown in the model explorer (and not in any table in a properties view
> > either). It is only visible in the diagram (washed out). Not until the
> 
> But so is the case also for other inherited elements, correct?  So far, I am
> using the Model Explorer to provide access to stuff for testing (it takes a
> while to get selections and such working in the diagrams).

Yes, as we discussed before when you introduced those "virtual" nodes for inherited elements, none of those are available in the legacy tooling. You explained that they simply are a facet customization, and hence could easily be removed, I was happy with that. There is a "clutter" aspect in the model explorer, and I am not sure that everyone want to see the inherited stuff in the model explorer (especially not if you are used to way that the legacy tooling works).

> > connector is excluded, you see the excluded connector in the model explorer
> > (since you now also have a model element created in the semantic model).
> > What should be noted is that in this situation, the connector does not even
> > have any connector ends (they are not shown in the model explorer). It is
> 
> I am working on making the UML representation as complete as it needs to be
> in order to be correctly structured and also to behave plausibly when it is
> manipulated by code that works with the UML API directly.  So, connector
> ends could have been inherited as they are from the redefined connector
> (they would be shared elements), but then any code that attempts to work
> with them would be confused:  then ends of a connector would end up
> reporting a different connector as their owner and making changes to them
> would edit the redefined connector.  In the implementation that I have now,
> the redefining connector automatically maintains ends that shadow
> (effectively redefine) the ends of the redefined connector.  Any change to
> these ends causes the entire connector to be redefined and added into the
> model (no longer being a proxy for the inherited connector).
> 
> 
> > only the connector itself that is "duplicated" in the semantic model (with
> > the RTRedefinedElement stereitype applied). I am not sure how the alignment
> > with legacy models should be handled. Either the tooling needs to handle the
> > exclusion case where only the connector exist (without connector ends), or
> > the import tool have to "duplicate" the connector ends. I have a feeling
> > that it is better to align with the legacy models, and create a connector
> > without duplication of the connector ends, and possibly let them "inherit"
> > the connector ends, in the corresponding way as I guess we need to handle
> > the ValueSpecifications for inheriting multiplicity. But since it is
> 
> Now that I have worked out the shadowing of connector ends, I am actually
> planning to do the same for the value-specifications for multiplicity
> bounds.  This provides the most consistent experience for code written to
> the UML API.
> 
> 
> > excluded that is not really necessary either. The only issue here I assume
> > is the constraint in the UML meta-model that a connector *must* have 2 (or
> > more) connector ends (something that I do know that the legacy tooling
> 
> That might be an issue, yes, but it is more about meeting the expectations
> that code (especially extenders and other UML plug-ins) has about the
> contract of the UML API.  Sharing elements that are meant to be owned is
> extremely surprising to code that tries to work with them.  So, for me, it's
> a matter of safety and resilience in the face of UML tools more than
> validation rules.
> 

I am not sure that I understand the implications of what you are trying to explain here. What are your plans with respect to legacy models? If you are going in another direction that what the legacy tooling and models are doing, then I assume that we also need to do something when importing legacy models, e.g. this about excluded connectors which will not have any connector ends in an imported model.

I am not sure either about these "shadowing" regarding ValueSpecifications for multiplicity. Will that imply that whenever changing the replication factor that "shadow" in the subclass capsule(s) needs to be updated (potentially in other model files)?

Maybe I need to see this more in action to understand what you mean and what implications it will have (I have not yet been able to get an installation in place for the builds on the branch). I just get a bad feeling that we diverting a bit too much compared to the legacy tooling and how legacy models look like, which then will cause issues when importing them (which we need to adjust to in the import tool).
Comment 34 Christian Damus CLA 2016-12-06 09:32:28 EST
Tweaks to the presentation of inherited elements:

    https://youtu.be/GtaCwUXTOSg

Shows inherited elements not shown by default in the Model Explorer, ends of inherited connectors not shown, and in the diagram ports on parts showing inheritance according to the part's capsule type, not the part itself.
Comment 35 Peter Cigehn CLA 2016-12-06 10:10:06 EST
(In reply to Christian W. Damus from comment #34)
> Tweaks to the presentation of inherited elements:
> 
>     https://youtu.be/GtaCwUXTOSg
> 
> Shows inherited elements not shown by default in the Model Explorer, ends of
> inherited connectors not shown, and in the diagram ports on parts showing
> inheritance according to the part's capsule type, not the part itself.

I have checked the video, and it looks nice! We are heading in the right direction... :)

But what I still don't understand regarding the connector ends (which now was not shown in the model explorer similar to the legacy tooling), is whether they still actually exist in the semantic model (persistently stored in the model file) or not, i.e. only filtered from the model explorer. What I meant with not being shown in the model explorer in the legacy tooling is that they are not persisted in the model file either. 

Basically this is all that is in the model file for the an excluded connector (I included a snippet with the applied stereotypes also just for the sake of it):

      <ownedConnector xmi:id="_rCRn8LvEEeaPcqj1NswNTA" name="Connector" redefinedConnector="_DUS2wLemEeah37pYXgeMBw">
        <eAnnotations xmi:id="_rCbY8rvEEeaPcqj1NswNTA" source="http://www.eclipse.org/uml2/2.0.0/UML">
          <details xmi:id="_rCbY87vEEeaPcqj1NswNTA" key="excluded"/>
        </eAnnotations>
      </ownedConnector>
.
.
.
  <UMLRealTime:RTConnector xmi:id="_rCbY8LvEEeaPcqj1NswNTA" base_Connector="_rCRn8LvEEeaPcqj1NswNTA"/>
  <UMLRealTime:RTRedefinableElement xmi:id="_rCbY8bvEEeaPcqj1NswNTA" base_RedefinableElement="_rCRn8LvEEeaPcqj1NswNTA"/>

As you can see there are no connector ends persistently stored. All you have is the "excluded" keyword applied (which the legacy tooling consistently applies on all excluded elements, not only connectors, to make it even more clear in the model explorer when an element is excluded, but I have not (yet) bothered to bring up whether this should be done or not, since there are so many discussion in general regarding the use of keywords in Papyrus which I did not want to bring into this discussion as well).
Comment 36 Peter Cigehn CLA 2016-12-07 09:29:31 EST
(In reply to Christian W. Damus from comment #10)
> You can install from
>    
> https://hudson.eclipse.org/papyrus-rt/job/Papyrus-RT-Feature-Inheritance/
> lastSuccessfulBuild/artifact/updates/
> 

I tried making an installation from this p2 repo, by simply tweaking the current tester.setup file and simply adding this repo to the list of repos. But since the nightly build was newer than the build for the inheritance branch, it still seemed to pick everything from the ordinary nightly repo.

I guess we bump into the same issues that we have discussed related to the custom builds of EGit/EMF Compare and they never can "lag behind", i.e. the build on the inheritance branch always needs to be newer than the latest nightly build.

I was thinking that I simply could trigger a new build for the inheritance branch to ensure that this build was newer, but I don't have access rights to do so. Maybe you can give me the permission to do so, so that whenever the inheritance branch builds "lags behind" the nightly builds, I can manually trigger a new build?
Comment 37 Remi Schnekenburger CLA 2016-12-07 09:56:51 EST
Peter, I updated the configuration for the hudson project. Are you now able to launch the job?
Comment 38 Peter Cigehn CLA 2016-12-07 10:01:01 EST
(In reply to Remi Schnekenburger from comment #37)
> Peter, I updated the configuration for the hudson project. Are you now able
> to launch the job?

Yes, I triggered a build a few moments ago. Let's see if that does the trick in getting an installation with the inheritance stuff incorporated...
Comment 39 Peter Cigehn CLA 2016-12-07 11:24:44 EST
(In reply to Peter Cigehn from comment #38)
> (In reply to Remi Schnekenburger from comment #37)
> > Peter, I updated the configuration for the hudson project. Are you now able
> > to launch the job?
> 
> Yes, I triggered a build a few moments ago. Let's see if that does the trick
> in getting an installation with the inheritance stuff incorporated...

I was now able to get an installation, which seem to have the inheritance stuff from the branch. I had to tweak the tester.setup file a bit more, and install the individual features contained in the top level Papyrus-RT feature (common, common UI, core, profile, tooling) to ensure that they were picked up from the branch repo and not from the nightly repo. But after doing that it seemed to have installed correctly. I will see if I can do some more testing now when I seem to have it all up and running...
Comment 40 Peter Cigehn CLA 2016-12-09 05:24:12 EST
I have tested the latest build based on the inheritance branch, and here is some feedback (I assume that lots of the things you are already aware of, but I put down what I can see anyway, in the hope of providing feedback on something that you have not seen or considered yet):

* The inherited connector seem to be always be attached to the top left corner of a port on the capsule. For ports on capsule parts they seem to attach at the same location as in the super-class context. This causes the connector to be routed differently int he sub-class capsule.

* Moving a port on a capsule part in the superclass capsule does not seem to be reflected in the subclass context. Moving a port on the capsule itself, and moving the capsule part, in the superclass capsule works as expected.

* Related to this, the size (and position) of the capsule frame itself is not updated in the subclass capsule. In general I do not really like this that the frame of the capsule can be moved around on the diagram, but it should be more seen as a, kind of "fixed", frame for the capsule structure diagram itself. But this is really not related to inheritance, so as long as we have this principle the the capsule frame also can be moved around (apart from being resized), that both the size and the position of the capsule frame itself shall both be inherited to the subclass capsule.

* When trying to redefine the type of a port and capsule part, I realized that we lack the ... select button for the protocol and capsule properties in the properties view for port respectively capsule part. We only have the green + button for creating a new protocol respectively capsule, and the pen edit button for editing the already associated protocol respectively capsule. But we lack the ... selection button (which is the natural way to redefine the type of a port respectively capsule part). I can write a separate bug for tracking this, since it is a more general issue (but strongly related to the act of redefining the type).

* I guess that you have not added that yet, but it would be nice to have the menu choice for re-inheriting the size and position of shapes and edges in the diagram. In the legacy tooling there is a menu-choice on the Format context-menu named "Position from Superclass View". Is is a toggle menu-choice with the standard "check-mark" to indicate if it is enabled or not. Something similar would be nice. I do know that you have discussed before that size and position could be re-inherited separately, but I think that it is sufficient with only re-inheriting both size and position, as this menu choice works in the legacy tooling.

* If I do redefine something, and I get the redefined element in the model explorer, I would expect it to be possible to delete the redefined element in the model explorer (which in practice would re-inherit it, "reverting" the redefinition), using the ordinary delete menu choice. But it is not possible to delete, nothing happens. You however have an exclude menu choice, but that I feel is a bit error-prone, and possibly confusing, that you can exclude something that you have redefined. I would expect it to only be possible to exclude something that is inherited "as-is", but not excluding something that already have been redefined. Maybe the explicit re-inherit menu choice (that you get for excluded elements) should be available also for redefined elements?

* I also checked this case that we discussed earlier regarding the exclusion of a connector. And as you explained, I can see that the connector ends indeed are (partly) "mirrored" for the excluded element and also persisted in the model file

    <packagedElement xmi:type="uml:Class" xmi:id="_t8xwML3zEeaV6eqQY7mgBw" name="Capsule3" isActive="true">
      <generalization xmi:type="uml:Generalization" xmi:id="_xZSTUL3zEeaV6eqQY7mgBw" general="_JR7S0LyIEeajqZxzS8DrZA"/>
      <ownedConnector xmi:type="uml:Connector" xmi:id="_kFIjZL34EeaV6eqQY7mgBw" redefinedConnector="_URSjYLyIEeajqZxzS8DrZA">
        <end xmi:type="uml:ConnectorEnd" xmi:id="_kFOqAL34EeaV6eqQY7mgBw"/>
        <end xmi:type="uml:ConnectorEnd" xmi:id="_kFOqAb34EeaV6eqQY7mgBw"/>
      </ownedConnector>
    </packagedElement>

I guess that you ensure that role and partWithPort attributes are implicitly inherited. But if we do like this, how are we supposed to handle legacy models, which does not have the connector ends. Is your idea that this should be fixed during import, or? Also this about that the connector ends are "hidden" from the model explorer, which I guess was something you made based my comment earlier that they are not shown in the legacy tooling for an excluded connector. But that was also based on the fact that the excluded connector did not have any connector ends, so there is nothing to show in the semantic model. But here we hide them (not show them), even though they are there in the semantic model. Not sure in which direction we should be taking this, especially with respect to importing legacy models.

I think that this is enough for now. I will continue testing a bit more, but I am not sure if you find this kind of feedback to be good, or if it is too premature and just slows things down, and that I should wait with testing and providing feedback until things are a bit more ready.
Comment 41 Christian Damus CLA 2016-12-09 07:32:25 EST
(In reply to Peter Cigehn from comment #40)
> I have tested the latest build based on the inheritance branch, and here is
> some feedback (I assume that lots of the things you are already aware of,
> but I put down what I can see anyway, in the hope of providing feedback on
> something that you have not seen or considered yet):

Thanks, Peter.  Your feed-back is always helpful.  Even if I already have an issue on my radar, your reporting it validates its importance.


> * The inherited connector seem to be always be attached to the top left
> corner of a port on the capsule. For ports on capsule parts they seem to
> attach at the same location as in the super-class context. This causes the
> connector to be routed differently int he sub-class capsule.

Yes.  This is a general problem in attachment of edges created by Canonical Edit Policy.  For now, I have to defer to a specialist in GEF/GMF layout.


> * Moving a port on a capsule part in the superclass capsule does not seem to
> be reflected in the subclass context. Moving a port on the capsule itself,
> and moving the capsule part, in the superclass capsule works as expected.

Hmm, I think I haven't tried this recently (been working more on semantics), but I think this worked at one point.  I really should start adding diagram JUnit tests to catch regressions like that as I go.


> * Related to this, the size (and position) of the capsule frame itself is
> not updated in the subclass capsule. In general I do not really like this
> that the frame of the capsule can be moved around on the diagram, but it
> should be more seen as a, kind of "fixed", frame for the capsule structure
> diagram itself. But this is really not related to inheritance, so as long as

I agree about the behaviour of the frame; I find it surprising, too.  But the actual size and shape of the capsule frame isn't something I had thought about, yet.  So far I had only looked at the stuff in it.  I shall add it to the list.


> * When trying to redefine the type of a port and capsule part, I realized
> that we lack the ... select button for the protocol and capsule properties
> in the properties view for port respectively capsule part. We only have the
> green + button for creating a new protocol respectively capsule, and the pen
> edit button for editing the already associated protocol respectively
> capsule. But we lack the ... selection button (which is the natural way to
> redefine the type of a port respectively capsule part). I can write a
> separate bug for tracking this, since it is a more general issue (but
> strongly related to the act of redefining the type).

Yes, please do.  It's possible that the ellipsis button was explicitly suppressed because we typically create ports and parts by dropping types onto the diagram, but for redefinition, we need to be able to select especially sub-types of the inherited port/part type, right?


> * I guess that you have not added that yet, but it would be nice to have the
> menu choice for re-inheriting the size and position of shapes and edges in
> the diagram. In the legacy tooling there is a menu-choice on the Format
> context-menu named "Position from Superclass View". Is is a toggle
> menu-choice with the standard "check-mark" to indicate if it is enabled or
> not. Something similar would be nice. I do know that you have discussed
> before that size and position could be re-inherited separately, but I think
> that it is sufficient with only re-inheriting both size and position, as
> this menu choice works in the legacy tooling.

I agree that it should be just one menu choice.  For edges, I think bendpoints and anchors should also be taken together ("routing" generally).  Indeed, I haven't looked at this yet, but it's on the to-do list now.


> * If I do redefine something, and I get the redefined element in the model
> explorer, I would expect it to be possible to delete the redefined element
> in the model explorer (which in practice would re-inherit it, "reverting"
> the redefinition), using the ordinary delete menu choice. But it is not

Yes, I mentioned in comment 18 that I cannot re-target the Delete action behaviour until we fix the Model Explorer in Papyrus, and now it's too late for Neon.2.  Until the, we have to have a separate exclusion action.


> possible to delete, nothing happens. You however have an exclude menu
> choice, but that I feel is a bit error-prone, and possibly confusing, that
> you can exclude something that you have redefined. I would expect it to only
> be possible to exclude something that is inherited "as-is", but not
> excluding something that already have been redefined. Maybe the explicit
> re-inherit menu choice (that you get for excluded elements) should be
> available also for redefined elements?

That is an interesting point.  Re-inherit to revert a redefinition to be purely inherited.  I like it.  So far, it is necessary to first exclude a redefined element before it can be re-inherited.

Should the exclude action then not be available on redefined elements but only on purely inherited ones? (which by default are not shown in the explorer)  That is easy to do.


> * I also checked this case that we discussed earlier regarding the exclusion
> of a connector. And as you explained, I can see that the connector ends
> indeed are (partly) "mirrored" for the excluded element and also persisted
> in the model file

Yes, I think it would not be difficult to have import add the inherited ends, but I think it would probably happen automatically, anyways:  this UML-RT implementation of the UML Connector implicitly creates and manages these "shadows" of inherited connector ends.  It will have to be tested.

As far as excluded elements are concerned, I think it would be tidy in the explorer never to show any contents of an excluded element.  The user only really needs to see that the element is excluded.  Perhaps even it should show no properties in the Properties View apart from the name.  It just so happens that, so far, the only excludable elements that ever would show children in the explorer are connectors.
Comment 42 Remi Schnekenburger CLA 2016-12-09 07:56:45 EST
> * Related to this, the size (and position) of the capsule frame itself is
> not updated in the subclass capsule. In general I do not really like this
> that the frame of the capsule can be moved around on the diagram, but it
> should be more seen as a, kind of "fixed", frame for the capsule structure
> diagram itself. But this is really not related to inheritance, so as long as

I agree about the behaviour of the frame; I find it surprising, too.  But the actual size and shape of the capsule frame isn't something I had thought about, yet.  So far I had only looked at the stuff in it.  I shall add it to the list.


> * When trying to redefine the type of a port and capsule part, I realized
> that we lack the ... select button for the protocol and capsule properties
> in the properties view for port respectively capsule part. We only have the
> green + button for creating a new protocol respectively capsule, and the pen
> edit button for editing the already associated protocol respectively
> capsule. But we lack the ... selection button (which is the natural way to
> redefine the type of a port respectively capsule part). I can write a
> separate bug for tracking this, since it is a more general issue (but
> strongly related to the act of redefining the type).

Yes, please do.  It's possible that the ellipsis button was explicitly suppressed because we typically create ports and parts by dropping types onto the diagram, but for redefinition, we need to be able to select especially sub-types of the inherited port/part type, right?

Some very short comment on this:
- for the ellipsis, it seems that the method org.eclipse.papyrusrt.umlrt.tooling.ui.modelelement.UMLRTExtModelElement.getDirectCreation(String) is a bit too restrictive, it should allow also the ellipsis on the RTPort. I think it may be interesting to select an existing protocol from the property view.
- for the capsule frame, the layout is a plain X/Y layout like the other nodes. It may be interesting for such 'frames' shapes to have some more constrained layout. It may be possible to implement it inside Papyrus-RT, and then propose this feature in Papyrus.
Comment 43 Christian Damus CLA 2016-12-09 08:03:12 EST
(In reply to Remi Schnekenburger from comment #42)
> 
> Some very short comment on this:
> - for the ellipsis, it seems that the method
> org.eclipse.papyrusrt.umlrt.tooling.ui.modelelement.UMLRTExtModelElement.
> getDirectCreation(String) is a bit too restrictive, it should allow also the
> ellipsis on the RTPort. I think it may be interesting to select an existing
> protocol from the property view.

Glad to know it's not intended.


> - for the capsule frame, the layout is a plain X/Y layout like the other
> nodes. It may be interesting for such 'frames' shapes to have some more
> constrained layout. It may be possible to implement it inside Papyrus-RT,
> and then propose this feature in Papyrus.

+1
Comment 44 Peter Cigehn CLA 2016-12-09 08:39:38 EST
(In reply to Christian W. Damus from comment #41)
> (In reply to Peter Cigehn from comment #40)
> > * When trying to redefine the type of a port and capsule part, I realized
> > that we lack the ... select button for the protocol and capsule properties
> > in the properties view for port respectively capsule part. We only have the
> > green + button for creating a new protocol respectively capsule, and the pen
> > edit button for editing the already associated protocol respectively
> > capsule. But we lack the ... selection button (which is the natural way to
> > redefine the type of a port respectively capsule part). I can write a
> > separate bug for tracking this, since it is a more general issue (but
> > strongly related to the act of redefining the type).
> 
> Yes, please do.  It's possible that the ellipsis button was explicitly
> suppressed because we typically create ports and parts by dropping types
> onto the diagram, but for redefinition, we need to be able to select
> especially sub-types of the inherited port/part type, right?

Yes, we need to be able to select new types, normally sub-types, of the inherited port/part type (not sure though if the selection dialog itself should make the sub-type filtering, or we only should rely on the validation and what the isConsistentWith() in our specialized implementation returns). So we need to be able to select an existing type from the selection dialog. It could possibly be me that overlooked this when providing feedback earlier regarding that it should only be the green + button and then pen button. But I will write a separate bug for this.

> > * If I do redefine something, and I get the redefined element in the model
> > explorer, I would expect it to be possible to delete the redefined element
> > in the model explorer (which in practice would re-inherit it, "reverting"
> > the redefinition), using the ordinary delete menu choice. But it is not
> 
> Yes, I mentioned in comment 18 that I cannot re-target the Delete action
> behaviour until we fix the Model Explorer in Papyrus, and now it's too late
> for Neon.2.  Until the, we have to have a separate exclusion action.

Yes, and I can just comment that this is actually what the legacy tooling also have. For the specific case of right clicking in a diagram, there is a separate "Exclude" menu choice. This that the Delete and Exclude are combined is only done in some other places, like buttons in relation to tables/lists in the properties view, e.g. similar to what I describe in Bug 507530. In those cases you normally have a button with a tool/tip that can inform you that the red x button is a combined delete/exclude.

> > possible to delete, nothing happens. You however have an exclude menu
> > choice, but that I feel is a bit error-prone, and possibly confusing, that
> > you can exclude something that you have redefined. I would expect it to only
> > be possible to exclude something that is inherited "as-is", but not
> > excluding something that already have been redefined. Maybe the explicit
> > re-inherit menu choice (that you get for excluded elements) should be
> > available also for redefined elements?
> 
> That is an interesting point.  Re-inherit to revert a redefinition to be
> purely inherited.  I like it.  So far, it is necessary to first exclude a
> redefined element before it can be re-inherited.
> 
> Should the exclude action then not be available on redefined elements but
> only on purely inherited ones? (which by default are not shown in the
> explorer)  That is easy to do.

Yes, I would really not expect that you first have to exclude a redefined element, before you can "re-inherit" it again. 

In the legacy tooling the only way (at least that I know of) to "re-inherit" a redefined port/capsule part is to remove the redefined element. I find this a bit confusing, and have not found any other way of doing it. So having the explicit choice of re-inherit a redefined port/capsule part I feel would make this a bit more clear. Still it should work also to delete the redefined element (which in practice would be the same thing as "re-inherit" it).

And yes, any exclude action I would only expect to be available on purely inherited elements.

> > * I also checked this case that we discussed earlier regarding the exclusion
> > of a connector. And as you explained, I can see that the connector ends
> > indeed are (partly) "mirrored" for the excluded element and also persisted
> > in the model file
> 
> Yes, I think it would not be difficult to have import add the inherited
> ends, but I think it would probably happen automatically, anyways:  this
> UML-RT implementation of the UML Connector implicitly creates and manages
> these "shadows" of inherited connector ends.  It will have to be tested.

Yes, we need to check that then. I might test some import of model with inheritance just for the sake of it. I guess we will need to make some adjustments to the import tool anyway, to start supporting models with inheritance.

> 
> As far as excluded elements are concerned, I think it would be tidy in the
> explorer never to show any contents of an excluded element.  The user only
> really needs to see that the element is excluded.  Perhaps even it should
> show no properties in the Properties View apart from the name.  It just so
> happens that, so far, the only excludable elements that ever would show
> children in the explorer are connectors.

Yes, I agree that from a user perspective, it would make it more clean to not see any contents of excluded elements. Now that I understand a bit more what you meant by these "shadows" of the inherited connector ends, it makes sense to show only the connector (as it is in practice is done in the legacy tooling, but then due to the reason that the connector ends does not even exist since there are no similar "shadows").

Regarding this about *only* showing the name of an excluded element in the properties view I am not sure that I agree, especially not for connectors which normally don't have any meaningful name and are never explicitly named by the user, and instead are (implicitly) identified by their connector ends. In the legacy tooling there is a very convenient table that shows the ports, parts and protocols at each end of a connector, i.e. showing the role and partWithPort properties from the connector ends (with clickable hyper links which can be used to navigate to the port, part and protocol). 

But I just realized that when checking this table in the legacy tooling(which is a rather new feature), it is empty for an excluded connector. So you really do not know which excluded connector is which (in case you have several of them), if you want to re-inherit them. I guess that these "shadow" connectors that you have introduced, would make it possible to show the port and parts also for the excluded connector if we would have a similar table in the properties view for the connector. Or?
Comment 45 Christian Damus CLA 2016-12-09 09:02:57 EST
(In reply to Peter Cigehn from comment #44)

> ...
> 
> Yes, we need to be able to select new types, normally sub-types, of the
> ...
> I will write a separate bug for this.

Thanks!


> ...
> 
> Yes, I would really not expect that you first have to exclude a redefined
> element, before you can "re-inherit" it again. 

Very good.  I'll make it so.

> ...
> 
> And yes, any exclude action I would only expect to be available on purely
> inherited elements.

Will do.

> ...
>
> Regarding this about *only* showing the name of an excluded element in the
> properties view I am not sure that I agree, especially not for connectors
> which normally don't have any meaningful name and are never explicitly named
> by the user, and instead are (implicitly) identified by their connector
> ends. In the legacy tooling there is a very convenient table that shows the
> ports, parts and protocols at each end of a connector, i.e. showing the role
> and partWithPort properties from the connector ends (with clickable hyper
> links which can be used to navigate to the port, part and protocol).

That's a good point.  The inherited details of an excluded element can still be useful to distinguish it from others and inform the user of what actually is excluded.


> But I just realized that when checking this table in the legacy
> tooling(which is a rather new feature), it is empty for an excluded
> connector. So you really do not know which excluded connector is which (in
> case you have several of them), if you want to re-inherit them. I guess that
> these "shadow" connectors that you have introduced, would make it possible
> to show the port and parts also for the excluded connector if we would have
> a similar table in the properties view for the connector. Or?

Yes, I agree that this would be very useful.  And your surmise is correct:  I would expect, whenever such table is implemented, that it would show exactly what you're looking for on excluded connectors, because those will have have ends that inherit the role and part-with-port.  Moreover, the UMLRTConnector object in the façade API (which such table should use) resolves the ends' roles and parts-with-ports to the redefinitions (if any) of those elements in the context of the capsule that excludes the connector.
Comment 46 Peter Cigehn CLA 2016-12-09 09:16:53 EST
(In reply to Christian W. Damus from comment #45)
> (In reply to Peter Cigehn from comment #44)
> > Yes, we need to be able to select new types, normally sub-types, of the
> > ...
> > I will write a separate bug for this.
> 
> Thanks!

I wrote Bug 507552 to track this. I made it block Bug 507552 which requires it to be able to redefine the protocol of a port and the capsule of a capsule part.
Comment 47 Peter Cigehn CLA 2016-12-09 09:17:59 EST
(In reply to Peter Cigehn from comment #46)
> (In reply to Christian W. Damus from comment #45)
> > (In reply to Peter Cigehn from comment #44)
> > > Yes, we need to be able to select new types, normally sub-types, of the
> > > ...
> > > I will write a separate bug for this.
> > 
> > Thanks!
> 
> I wrote Bug 507552 to track this. I made it block Bug 507552 which requires
> it to be able to redefine the protocol of a port and the capsule of a
> capsule part.

Hmmm... that should of course be Bug 508991 that I wrote to track this.
Comment 48 Christian Damus CLA 2016-12-10 13:36:22 EST
The latest branch build (#36) has several new fixes,

    https://youtu.be/6VRODnYA9Ds

including

* new "Follow Superclass View" in the format menu of shapes
* tracking of ports on parts from the superclass diagram
* undo/redo not reverting changes to port and part replication
* several performance optimizations, especially for elements that are not inherited or redefining other elements
* new JUnit test fixtures that ensure correct installation of the UML-RT metamodel implementation
* port kind radio button enablement now works for inherited ports
* merged fixes from master, including Asma's fix for refresh on undo of port movement
Comment 49 Peter Cigehn CLA 2016-12-12 08:01:39 EST
(In reply to Christian W. Damus from comment #48)
> The latest branch build (#36) has several new fixes,
> 
>     https://youtu.be/6VRODnYA9Ds
> 
> including
> 
> * new "Follow Superclass View" in the format menu of shapes

I've checked the video, and this looks good! I also checked the latest build with the new tool bara button, which I really like! This one (without really knowing it) I have missed a lot in the legacy tooling. It has been rather cumbersome to have to open up a context sub-menu to check the "state" of graphical inheritance to know if it was really following the superclass view or not. Now you can with a quick glance on the tool bar button directly see the "state". Great!

Some possible improvements in this area:

* Make it possible to do multi-select and mark all of them as "follow superclass view". I guess should work for both the tool bar button as well as the context menu choice. I guess the state of the tool bara button and the menu choice should be that if at least one selected element does not follow, then the aggregated state should be "non-follow". Only if all selected elements are following, then the aggregated state should indicate "follow". This seem to be how it works in the legacy tooling. Right now you only seem to be able to select a single element and let that one follow.

* If the multi-select works as above, then it would be nice also to be able to do a "select all" (Ctrl+A) and then mark everything to follow the super-class view (in case you for some reason want to completely revert all redefined position/sizes/bendpoints in the subclass view, and let them all follow the superclass view again). This of course means that non-inherited elements should be possible to be selected (since Ctrl+A selects everything), but still the "Follow Superclass View" should still be possible to be used. I guess any non-inherited elements (when you have a multi-select) simply can be ignored. This seem to be how it is working in the legacy tooling.

> * tracking of ports on parts from the superclass diagram

When looking at this a bit, I realized that the port label position is not tracked. Actually when checking this in the legacy tooling there seem to be a bug where the position of the label for ports on capsule border itself is not tracked, the label for internal ports and ports on capsule parts are tracked. So in general labels on all ports, both service ports on the border as well non-service ports on the inside, as well as (service) ports on the border of a capsule part, should have their label position tracked from the superclass view.

> * undo/redo not reverting changes to port and part replication
> * several performance optimizations, especially for elements that are not
> inherited or redefining other elements
> * new JUnit test fixtures that ensure correct installation of the UML-RT
> metamodel implementation
> * port kind radio button enablement now works for inherited ports

I was a bit confused regarding this. From what I can see, this is not related to what shall be enabled regarding which redefinitions are allowed. As listed ín Bug 507552 Comment 0, only Replication, Type (Protocol), isNotification, and isBehavior, should be possible to be redefined, and thus only those should be enabled in the properties view for an inherited port. But I guess it was not this enablement (or really disablement of all other properties) that was meant here.

> * merged fixes from master, including Asma's fix for refresh on undo of port
> movement

I will try to see if I can play around a bit more with the latest build and provide some additional feedback.
Comment 50 Christian Damus CLA 2016-12-12 08:23:50 EST
(In reply to Peter Cigehn from comment #49)
> 
> I've checked the video, and this looks good! I also checked the latest build
> with the new tool bara button, which I really like! This one (without really
> knowing it) I have missed a lot in the legacy tooling. It has been rather
> cumbersome to have to open up a context sub-menu to check the "state" of
> graphical inheritance to know if it was really following the superclass view
> or not. Now you can with a quick glance on the tool bar button directly see
> the "state". Great!

I'm glad that Eclipse makes it so easy.  It's the exact same thing contributed to the tool bar as to the context menu.  It's all in the plugin.xml.

> Some possible improvements in this area:
> 
> * Make it possible to do multi-select and mark all of them as "follow
> superclass view". I guess should work for both the tool bar button as well

This is supposed to work, but I think I must have done something wrong in the enablement condition in the XML.  So far, I had been trying to enable multi-selection where all selected elements are of the same disposition, either all following their superclass view or all not.

But, of course, that's ridiculous.  Multi-selection should be enabled if at least one selected element is a view inherited or redefined from the superclass diagram and then the button should toggle follow ON for all views that can do it if any view is not following its superclass view, and OFF if all selected views that are inherited/redefined are following their superclass view.

To put it another way, I think multiple selection should be allowed if any selected view supports inheritance and should bias towards the follow ON state in case of heterogeneous selection.

Or, do you think it makes more sense for real use cases to bias towards turning off following the superclass view?

> 
> > * tracking of ports on parts from the superclass diagram
> 
> When looking at this a bit, I realized that the port label position is not
> tracked. Actually when checking this in the legacy tooling there seem to be
> a bug where the position of the label for ports on capsule border itself is
> not tracked, the label for internal ports and ports on capsule parts are
> tracked. So in general labels on all ports, both service ports on the border
> as well non-service ports on the inside, as well as (service) ports on the
> border of a capsule part, should have their label position tracked from the
> superclass view.

Okay, I was wondering about this.  It certainly makes sense.  Should not be difficult to do.


> > * port kind radio button enablement now works for inherited ports
> 
> I was a bit confused regarding this. From what I can see, this is not
> related to what shall be enabled regarding which redefinitions are allowed.

Right, this was something that made me realize that the utility methods used by this UI to determine what kinds of inside and outside connections ports have needed to be updated to account for inheritance, because they are more generally applicable than just this use case.  This was easy using the new façade API, but it also meant updating all of the JUnit tests to introduce new test fixtures that provide the full UML-RT metamodel in their editing environment, which is a good thing anyways.

More focused changes in the Properties view are still ahead in the to-do list.
Comment 51 Peter Cigehn CLA 2016-12-12 08:38:13 EST
(In reply to Christian W. Damus from comment #50)
> (In reply to Peter Cigehn from comment #49)
> > Some possible improvements in this area:
> > 
> > * Make it possible to do multi-select and mark all of them as "follow
> > superclass view". I guess should work for both the tool bar button as well
> 
> This is supposed to work, but I think I must have done something wrong in
> the enablement condition in the XML.  So far, I had been trying to enable
> multi-selection where all selected elements are of the same disposition,
> either all following their superclass view or all not.
> 
> But, of course, that's ridiculous.  Multi-selection should be enabled if at
> least one selected element is a view inherited or redefined from the
> superclass diagram and then the button should toggle follow ON for all views
> that can do it if any view is not following its superclass view, and OFF if
> all selected views that are inherited/redefined are following their
> superclass view.
> 
> To put it another way, I think multiple selection should be allowed if any
> selected view supports inheritance and should bias towards the follow ON
> state in case of heterogeneous selection.
> 
> Or, do you think it makes more sense for real use cases to bias towards
> turning off following the superclass view?

Maybe both you and I are a bit unclear here. But if I understand you correctly, and what I tried to explain before, is that if at least one of the selected elements does not follow the superclass view, then the state of the button/menu choice should be off, i.e. if you press the button or select the menu choice, then it should ensure that all elements now follow the superclass view. Only if *all* selected elements follows the superclass view, the state of the button/menu choice should be on, i.e. the action will now be to mark all elements to not follow the superclass view anymore.

This is how the legacy tooling, and how I personally also wants it to work. The normal use case for using this feature is that if you have "by mistake" moved/resize an element and you want ensure that it follows the superclass view again. I would say that you rarely (if ever), use the action to ensure that an element "unfollows" the superclass view (but still keeping it at its original position), since that action is is most cases done by simply moving it a bit.
Comment 52 Christian Damus CLA 2016-12-12 08:41:10 EST
(In reply to Peter Cigehn from comment #51)

Good, we are agreed, then.  That is exactly what I meant when I said that the toggle button should bias towards the follow-superclass-view state.
Comment 53 Peter Cigehn CLA 2016-12-13 08:10:06 EST
I have tested the latest build from the inheritance branch and have some feedback:

* The multi-select for "Follow Superclass View" seem to be working fine for the case when only inherited elements are selected. Also the Ctrl+A case seem to working as expected. But for the case when non-inherited elements are selected, the behavior feels a bit strange. It is as if also non-inherited elements seem to keep track if it follows the superclass view or not (even though there is nothing to follow). At least the in-memory representation (I cannot see anything being persistently stored in the .notation-file) and closing an re-opening the model causes the follow state for non-inherited elements to be "forgotten". If I only select a non-inherited element (or several non-inherited elements) then the follow button is disabled, which is expected. But if I select both a number of inherited elements (which already follows the superclass view) and (at least) one non-inherited element, then the button is enabled, but in the off state. That felt confusing, since all the inherited elements already follows and the non-inherited element does not make sense that it should follow. If I now press the follow button, it feels like the non-inherited element is "marked" (in-memory at least) to follow some superclass view (even it there is nothing to follow). Selecting the same set of elements now causes the follow button to end up in the on state, which indicates that the non-inherited element now also seem to be following "something". As I indicated in Comment 49, it have a feeling that any non-inherited elements selected (in a multi-select scenario where other inherited elements are selected) should simply be ignored when computing the state of the follow button/menu choice. Only if only non-inherited element(s) are selected, the button/menu choice should be disabled respectively not shown. Apart from this corner-case, the multi-select seem to work as expected.

* The port label now follow as expected for all ports kinds. Nice.

* The restriction on what can be redefined seem to work as expected for ports. The redefinition of isBehavior needs to be double-checked by Ernesto that it works as expected in the code-generator and the run-time. We have had some discussion about it from before, and it was some discussion back-and-forth if this could be supported in the code-generator/run-time or not, and I have feeling that this needs to verified, since this is a new feature and not something that is supported in the legacy tooling (and thus I assume not in the legacy code-generator and run-time either).

* When redefining isBehavior for a port, the behavior adornment should of course be updated in the subclass view. As it is now it seem still to follow the behavior adornment for the superclass view, and the fact that it has been redefined is ignored in the diagram.

* The restriction for capsule part though seem to be still allowing to redefine into a plugin capsule part. Maybe Bug 507552 Comment 0 was a bit unclear, but only if the capsule part is fixed or optional in the superclass, then it can be redefined to optional respectively fixed in the subclass. It can never be redefined to plugin capsule part. So to be more clear:

Superclass: Fixed -> Subclass: Fixed and Optional enabled. Plugin disabled.
Superclass: Optional -> Subclass: Fixed and Optional enabled. Plugin disabled.
Superclass: Plugin -> Subclass: Fixed and Optional disabled. Plugin enabled.

* I think it would be good timing to look at Bug 508991 also to get the support for redefining the type, with an existing type, in place.

* When redefining an element (port and capsule part), the redefinition annotation should be shown on the port and the capsule part in the upper right corner. For ports on capsule parts, it could be questioned if we shall do that since the port is so small and it might be cluttering. The legacy tooling does not show this redefinition annotation on ports on capsule parts.

* Personally I find the redefinition (and inherited) annotations in the model explorer to be a bit hard to see. Not sure if they can be made slightly larger, and maybe have a darker border, or possibly a darker fill color as well. I find light blue to not "stand out" enough compared to the icon it is annotating. It is easier to see this in the legacy tooling. See the attachment attached to Comment 6 for example of this annotation on two capsule parts for Capsule3.

* Redefining the type of a port and capsule part should show the port/capsule part name label to be in bold. See attachment in Comment 6 for capsule parts with redefined type in Capsule3. The same goes also for ports which have their type (protocol) redefined.

* When deleting an element from the superclass, the subclass view seem to be updated as expected, i.e. the element is removed from the subclass diagram as well. But strange enough some resizing of capsule parts seem to happen, in the subclass view. Pressing F5 and refreshing restores the size of the capsule part. Actually the same seem to happen if you add a port to a superclass. The subclass diagram is indeed updated, but the port position seem to be on the right border (where autolayout places them by default), and the capsule part gets resized. Pressing F5 to refresh the subclass diagram gets the port located correctly following the superclass view and the size of the capsule part is also restored.

* When redefining, e.g. the type, of a port or capsule part, I see that you now persistently store the "shadow" ValueSpecifications for upper and lower (in the corresponding way as for the ConnectorEnds of the Connector). I wonder though if this is such a good idea. Since the core intention with the incremental redefinition is to store as little information as possible in the subclass, and let as much as possible to inherited from the superclass to make sure that any updates to the superclass does not need to be persistently stored and updated in the subclass. Take the scenario where the superclass originally have a replication of e.g. 2. Then a subclass now have "shadows" of the two ValueSpecification (but their actual values are of course inherited). But if you now change the replication to "None (1)", i.e. upper and lower are unset and there are not ValueSpecifications in the superclass. What should then happen to the subclass(es) which have "shadows" persistently stored? The next time a subclass is updated I guess the "shadows" also needs to be removed, but this might cause confusing changes when comparing the model, e.g. using EMF Compare. So I am still a bit reluctant regarding making such a change compared to how it works in the legacy tooling (which don't have these shadows). But maybe you already have considered this case, as you try to explain regarding the ConnectorEnds, e.g. with respect to importing legacy models which do not have these "shadows". But there is one difference between the ValueSpecifications and gthe ConnectorEnds and that is this special case, where the superclass not necessarily always have the ValueSpecifications, whereas the ConnectorEnds always should be there. In general we will have more similar aspects to consider when we come to state-machine inheritance were we have even more cases of where we should mirror as little as possible in the subclass state-machine.

I guess this feedback should be sufficient for now.... I continue testing more as we go along! :)
Comment 54 Christian Damus CLA 2016-12-14 08:54:55 EST
(In reply to Peter Cigehn from comment #53)
> 
> * When redefining, e.g. the type, of a port or capsule part, I see that you
> now persistently store the "shadow" ValueSpecifications for upper and lower
> (in the corresponding way as for the ConnectorEnds of the Connector). I
> wonder though if this is such a good idea. Since the core intention with the

These value-specifications need to exist, at least in memory, because clients of the UML API need to be able to modify them by setting their values (or bodies/languages, in the case of OpaqueExpressions).  In our context, setting the value of one of these multiplicity-bounds then causes its containing port/property to be a redefinition of the inherited port/property.

So far, the persistence of these inherited elements is a transitional thing.  I am still working on a way to make them transient that scales:  there are a lot more of these little objects than there are of capsules, so I am wary of applying the same extension pattern to them that so far I have used for capsules to have transient "shadows" of inherited ports, parts, and connectors.  My goal is to make these inherited values transient, also, until such time as they override the inherited values, and probably the inherited features of capsules will end up taking the same shape (and inherited connector-ends, too).
Comment 55 Peter Cigehn CLA 2016-12-14 09:11:38 EST
(In reply to Christian W. Damus from comment #54)
> (In reply to Peter Cigehn from comment #53)
> > 
> > * When redefining, e.g. the type, of a port or capsule part, I see that you
> > now persistently store the "shadow" ValueSpecifications for upper and lower
> > (in the corresponding way as for the ConnectorEnds of the Connector). I
> > wonder though if this is such a good idea. Since the core intention with the
> 
> These value-specifications need to exist, at least in memory, because
> clients of the UML API need to be able to modify them by setting their
> values (or bodies/languages, in the case of OpaqueExpressions).  In our
> context, setting the value of one of these multiplicity-bounds then causes
> its containing port/property to be a redefinition of the inherited
> port/property.

Yes, you are probably right that something needs to exist in memory for the value-specifications (I am not really sure how this is handled in the legacy tooling, and have not had the time to dig that deep). My main concern here was more related to what we persistently store in the model files, and what implication this have on version control, diffing models, multi-model management (including sub-models), and so on.

> 
> So far, the persistence of these inherited elements is a transitional thing.
> I am still working on a way to make them transient that scales:  there are a
> lot more of these little objects than there are of capsules, so I am wary of
> applying the same extension pattern to them that so far I have used for
> capsules to have transient "shadows" of inherited ports, parts, and
> connectors.  My goal is to make these inherited values transient, also,
> until such time as they override the inherited values, and probably the
> inherited features of capsules will end up taking the same shape (and
> inherited connector-ends, too).

Okay, not sure I understand all the details here, but it sounds good that you are looking into making them transient, to ensure that we persistently store only what is absolutely needed.
Comment 56 Peter Cigehn CLA 2016-12-14 11:58:46 EST
I did a quick test of the latest build from the inheritance branch, and you made quite some progress with the feedback I provided in Comment 53!

The multi-select, including non-inherited elements, now seem to behave as expected. The behavior adornment is shown correctly when redefining isBehavior for a port. The redefinition of capsule part now have the same kind of restrictions as in the legacy, i.e. fixed/optional can only be redefined to optional/fixed, and plugin cannot be redefined to any other kind. Port and capsule part labels are in bold when their type is redefined. The redefinition annotation is shown on port and capsule parts in the diagram. And the inheritance and redefinition annotation icons have a higher contrast making them easier to see.

I still would like them just a little bit bigger though! ;-) When I compare with the legacy tool they feel just slightly bigger, making them a bit easier to see. Is it possible to make them bigger? We are only talking about one, maybe two, pixels bigger. Or are they their maximum size (not sure how big such an annotation icon can be)?

It starts to look really good! I need to test a bit more, to identify some corner cases. When "fiddling" around a bit, I see some exceptions and inconsistent behavior, and refresh issue, which I assume is expected considering the "work in progress". But I can test a bit more and provide feedback...
Comment 57 Peter Cigehn CLA 2016-12-16 05:13:16 EST
I did some testing with the latest build from the feature branch. Here is some feedback. I write down what I notice, even if we have discussed it before (as you say, this probably indicates that it is important)

* Right after the generalization is established, the subclass diagram is not refreshed, and the layout does not follow the superclass. Pressing F5 do get the diagram refreshed and its layout to be up to date (this I assume you have on your radar since you mention it every time in you demonstration videos!)

* The size (and the position) of the superclass capsule frame itself is not inherited to the subclass.

* The inheritance of connectors seem to have become broken in the latest build. I started with a new fresh workspace, and created a new model. But I don't get the connectors to be show up as inherited, neither in the diagram, nor in the model explorer (when you have switched on the UML-RT Inherited Structure customization). In the old workspace with the existing model where I had it working, it still seem to be working, at least for the existing connector. Adding a new connector does not seem to work there either. If I close and re-open the model editor, then the inherited connector appear. So it seem to be something when it initially is created, which does not get updated in the subclass.

* I tried a case of multi-level inheritance (Capsule1 <- Capsule2 <- Capsule3) and it seem to work. I had some initial refresh issues when redefining in the context of Capsule2 and the diagram in Capsule3 did not, at least not initially, get updated. 

* Adding a new port in the superclass causes the subclass diagram to only partly be refreshed. One can see the port be shown in the top left corner in the subclass diagram, then after a brief moment the port gets located at the default auto-layout position. At the same time the capsule parts gets resized to be smaller. When pressing F5 in the subclass diagram, now refreshes and the ports is located at the position of the superclass and the size of the capsule is restored.

* If you use the "Link with Editor"-button (the double-arrows) in the model explorer, and you do *not* have the UML-RT Inherited Structure customization activated, i.e. aligned with the legacy tooling, I would have expected the element in the superclass to be "linked" in the model explorer, when selecting an inherited element in the subclass diagram. If you the customization enabled, i.e. that we show the inherited elements as "virtual" nodes in the subclass, then it makes sense that it is this "virtual" node that is selected. But I could very well see that the user *also* wants to the see the true semantic element that has been inherited, e.g. in the case of multi-level inheritance and the user really want to see from which of the superclasses that a specific element is inherited from. In the legacy tooling these "virtual" nodes do not even exist, and when navigating to the model explorer from a inherited element in a diagram, you navigate to the actual position of the semantic element, i.e. in the superclass from where the element is being inherited. Currently nothing gets selected in the model explorer if you have customization switched off. I guess we need to come up with a good scheme here, both how the "Link with Editor" should behave, and possibly also complementing with some new menu choices on the Navigate menu.

* I also tried the more trickier case where the superclass capsule is stored in a separate model from the subclass capsule. If I then edited the model with superclass capsule, e.g. removing a capsule part, "off-line" from the model with the subclass capsule, and then opening the structure diagram in the model with the subclass capsule, an exception occurs (which I kind of expected something like that to happen):

java.lang.NullPointerException
	at org.eclipse.papyrus.infra.gmfdiag.css.dom.GMFElementAdapter.getLocalName(GMFElementAdapter.java:396)
	at org.eclipse.papyrus.uml.diagram.css.dom.GMFUMLElementAdapter.getLocalName(GMFUMLElementAdapter.java:316)
	at org.eclipse.e4.ui.css.core.dom.ElementAdapter.getNodeName(ElementAdapter.java:204)
	at org.eclipse.e4.ui.css.core.impl.sac.CSSElementSelectorImpl.match(CSSElementSelectorImpl.java:60)
	at org.eclipse.e4.ui.css.core.impl.sac.CSSConditionalSelectorImpl.match(CSSConditionalSelectorImpl.java:81)
	at org.eclipse.papyrus.infra.gmfdiag.css.engine.enginecopy.ExtendedViewCSSImpl.getStyleWrappers(ExtendedViewCSSImpl.java:115)
	at org.eclipse.papyrus.infra.gmfdiag.css.engine.enginecopy.ExtendedViewCSSImpl.getComputedStyle(ExtendedViewCSSImpl.java:65)
	at org.eclipse.papyrus.infra.gmfdiag.css.engine.ExtendedCSSEngineImpl.getStyleDeclaration(ExtendedCSSEngineImpl.java:162)
	at org.eclipse.papyrus.infra.gmfdiag.css.engine.ExtendedCSSEngineImpl.getStyleDeclaration(ExtendedCSSEngineImpl.java:153)
	at org.eclipse.papyrus.infra.gmfdiag.css.engine.ExtendedCSSEngineImpl.retrievePropertyValue(ExtendedCSSEngineImpl.java:147)
	at org.eclipse.papyrus.infra.gmfdiag.css.style.impl.CSSViewDelegate.isCSSVisible(CSSViewDelegate.java:50)
	at org.eclipse.papyrus.infra.gmfdiag.css.CSSShapeImpl.isCSSVisible(CSSShapeImpl.java:164)
	at org.eclipse.papyrus.infra.gmfdiag.css.CSSShapeImpl.isVisible(CSSShapeImpl.java:333)
	at org.eclipse.gmf.runtime.notation.impl.BasicDecorationNodeImpl.getVisibleChildren(BasicDecorationNodeImpl.java:470)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.getModelChildren(GraphicalEditPart.java:699)
	at org.eclipse.gef.editparts.AbstractEditPart.refreshChildren(AbstractEditPart.java:762)
	at org.eclipse.gef.editparts.AbstractEditPart.refresh(AbstractEditPart.java:726)
	at org.eclipse.gef.editparts.AbstractGraphicalEditPart.refresh(AbstractGraphicalEditPart.java:644)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.access$3(GraphicalEditPart.java:1)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart$3.run(GraphicalEditPart.java:861)
	at org.eclipse.papyrus.infra.emf.readonly.PapyrusROTransactionalEditingDomain.runExclusive(PapyrusROTransactionalEditingDomain.java:271)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.refresh(GraphicalEditPart.java:851)
	at org.eclipse.gef.editparts.AbstractEditPart.addNotify(AbstractEditPart.java:253)
	at org.eclipse.gef.editparts.AbstractGraphicalEditPart.addNotify(AbstractGraphicalEditPart.java:223)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.ShapeCompartmentEditPart.addNotify(ShapeCompartmentEditPart.java:783)
	at org.eclipse.gmf.tooling.runtime.linklf.LinkLFShapeCompartmentEditPart.addNotify(LinkLFShapeCompartmentEditPart.java:74)
	at org.eclipse.gef.editparts.AbstractEditPart.addChild(AbstractEditPart.java:212)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.addChild(GraphicalEditPart.java:1319)
	at org.eclipse.gef.editparts.AbstractEditPart.refreshChildren(AbstractEditPart.java:781)
	at org.eclipse.gef.editparts.AbstractEditPart.refresh(AbstractEditPart.java:726)
	at org.eclipse.gef.editparts.AbstractGraphicalEditPart.refresh(AbstractGraphicalEditPart.java:644)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.access$3(GraphicalEditPart.java:1)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart$3.run(GraphicalEditPart.java:861)
	at org.eclipse.papyrus.infra.emf.readonly.PapyrusROTransactionalEditingDomain.runExclusive(PapyrusROTransactionalEditingDomain.java:271)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.refresh(GraphicalEditPart.java:851)
	at org.eclipse.papyrus.uml.diagram.common.editparts.UMLNodeEditPart.refresh(UMLNodeEditPart.java:99)
	at org.eclipse.papyrus.uml.diagram.common.editparts.NamedElementEditPart.refresh(NamedElementEditPart.java:108)
	at org.eclipse.gef.editparts.AbstractEditPart.addNotify(AbstractEditPart.java:253)
	at org.eclipse.gef.editparts.AbstractGraphicalEditPart.addNotify(AbstractGraphicalEditPart.java:223)
	at org.eclipse.gef.editparts.AbstractEditPart.addChild(AbstractEditPart.java:212)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.addChild(GraphicalEditPart.java:1319)
	at org.eclipse.gef.editparts.AbstractEditPart.refreshChildren(AbstractEditPart.java:781)
	at org.eclipse.gef.editparts.AbstractEditPart.refresh(AbstractEditPart.java:726)
	at org.eclipse.gef.editparts.AbstractGraphicalEditPart.refresh(AbstractGraphicalEditPart.java:644)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.access$3(GraphicalEditPart.java:1)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart$3.run(GraphicalEditPart.java:861)
	at org.eclipse.papyrus.infra.emf.readonly.PapyrusROTransactionalEditingDomain.runExclusive(PapyrusROTransactionalEditingDomain.java:271)
	at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.refresh(GraphicalEditPart.java:851)
	at org.eclipse.papyrus.infra.gmfdiag.common.editpart.PapyrusDiagramEditPart.refresh(PapyrusDiagramEditPart.java:93)
	at org.eclipse.gef.editparts.AbstractEditPart.addNotify(AbstractEditPart.java:253)
	at org.eclipse.gef.editparts.AbstractGraphicalEditPart.addNotify(AbstractGraphicalEditPart.java:223)
	at org.eclipse.gef.editparts.AbstractEditPart.addChild(AbstractEditPart.java:212)
	at org.eclipse.gef.editparts.SimpleRootEditPart.setContents(SimpleRootEditPart.java:105)
	at org.eclipse.gef.ui.parts.AbstractEditPartViewer.setContents(AbstractEditPartViewer.java:617)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramGraphicalViewer.setContents(DiagramGraphicalViewer.java:352)
	at org.eclipse.gef.ui.parts.AbstractEditPartViewer.setContents(AbstractEditPartViewer.java:626)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramEditor.initializeGraphicalViewerContents(DiagramEditor.java:872)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramEditor.initializeGraphicalViewer(DiagramEditor.java:865)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramEditorWithFlyOutPalette.initializeGraphicalViewer(DiagramEditorWithFlyOutPalette.java:116)
	at org.eclipse.gmf.runtime.diagram.ui.resources.editor.parts.DiagramDocumentEditor.initializeGraphicalViewer(DiagramDocumentEditor.java:174)
	at org.eclipse.papyrus.infra.gmfdiag.common.SynchronizableGmfDiagramEditor.initializeGraphicalViewer(SynchronizableGmfDiagramEditor.java:350)
	at org.eclipse.papyrus.uml.diagram.composite.part.UMLDiagramEditor.initializeGraphicalViewer(UMLDiagramEditor.java:485)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramEditor.createGraphicalViewer(DiagramEditor.java:807)
	at org.eclipse.papyrus.uml.diagram.composite.UmlCompositeDiagramForMultiEditor.createGraphicalViewer(UmlCompositeDiagramForMultiEditor.java:120)
	at org.eclipse.gef.ui.parts.GraphicalEditor.createPartControl(GraphicalEditor.java:171)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramEditor.createPartControl(DiagramEditor.java:1580)
	at org.eclipse.gmf.runtime.diagram.ui.parts.DiagramEditorWithFlyOutPalette.createPartControl(DiagramEditorWithFlyOutPalette.java:328)
	at org.eclipse.gmf.runtime.diagram.ui.resources.editor.parts.DiagramDocumentEditor.createPartControl(DiagramDocumentEditor.java:1514)
	at org.eclipse.papyrus.uml.diagram.common.part.UmlGmfDiagramEditor.createPartControl(UmlGmfDiagramEditor.java:245)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.EditorPart.createEditorPartControl(EditorPart.java:297)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.EditorPart.createPartControl(EditorPart.java:199)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.TabFolderPart.createChildPart(TabFolderPart.java:1069)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.TabFolderPart.createTabItem(TabFolderPart.java:990)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.TabFolderPart.synchronize2(TabFolderPart.java:904)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.RootPart.synchronize2(RootPart.java:139)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.SashWindowsContainer.refreshTabsInternal(SashWindowsContainer.java:715)
	at org.eclipse.papyrus.infra.core.sasheditor.internal.SashWindowsContainer.refreshTabs(SashWindowsContainer.java:669)
	at org.eclipse.papyrus.infra.core.sasheditor.editor.AbstractMultiPageSashEditor.doRefreshTabs(AbstractMultiPageSashEditor.java:248)
	at org.eclipse.papyrus.infra.core.sasheditor.editor.AbstractMultiPageSashEditor.access$1(AbstractMultiPageSashEditor.java:246)
	at org.eclipse.papyrus.infra.core.sasheditor.editor.AbstractMultiPageSashEditor$2.run(AbstractMultiPageSashEditor.java:241)
	at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:233)
	at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:145)
	at org.eclipse.swt.widgets.Display.syncExec(Display.java:4813)
	at org.eclipse.papyrus.infra.core.sasheditor.editor.AbstractMultiPageSashEditor.refreshTabs(AbstractMultiPageSashEditor.java:237)
	at org.eclipse.papyrus.infra.ui.editor.CoreMultiDiagramEditor.refreshTabs(CoreMultiDiagramEditor.java:1183)
	at org.eclipse.papyrus.infra.ui.editor.CoreMultiDiagramEditor$6.run(CoreMultiDiagramEditor.java:1172)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4203)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3819)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1121)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1022)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:150)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:687)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:604)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:138)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:673)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:610)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1519)

As I have indicated before, there is probably a need for some reasonable "reconcilation" (don't know what word to use) to try to align the subclass view with the superclass view in cases like this where the superclass has been updated "off-line" from the subclasses. At least it should not crash, and the user could be required to do some manual cleanup, but I guess also that certain kinds of changes should just propagate automatically.

I guess this is sufficient for now, I will see if I continue to do some more testing later.
Comment 58 Christian Damus CLA 2016-12-16 16:52:25 EST
Build #45 implements the persistence of multiplicity bounds for ports, parts, and connector-ends only where they are redefined from the inherited port/part/end.  This is as the demo at today's status call showed (but in addition the bug in which an overridden multiplicity was not persisted is now fixed).

Connector-ends are still persisted whenever the connector is.  Tackling that is the last of these kinds of incremental redefinition problems, and is up next.

As discussed on the call, explorer selection linking for inherited elements would require work in the Model Explorer in Papyrus to make the linking extensible.  I doubt this can be done for Papyrus-RT 1.0.

Refresh on edits is a general problem in Papyrus's canonical synchronization framework.  I am not sure what can be done about that in the near term; almost certainly not for Papyrus-RT 1.0, unless somebody else wants to look into it.

I am overhauling the accounting for inheritance in the canonical synchronization of connectors.  My primary use case is having a new connector added to a superclass capsule show up in the subclass, which it doesn't now (I don't know whether this ever worked).  I expect this to address also the regression noted in comment #57.
Comment 59 Peter Cigehn CLA 2016-12-19 04:07:36 EST
(In reply to Christian W. Damus from comment #58)
> As discussed on the call, explorer selection linking for inherited elements
> would require work in the Model Explorer in Papyrus to make the linking
> extensible.  I doubt this can be done for Papyrus-RT 1.0.

No, I guess this area is something that could be a candidate for improvement post-1.0. I have been planning on writing a bug on Papyrus related to the selection linking for the case when you select an element in the model explorer which is visualized multiple times in the same diagram, in which case only one (random?) visualization of that element is selected. This is for example related to the visualization of ports on capsule parts where you have multiple capsule parts typed by the same capsule, and the same port is visualized once for each capsule part. In this case, when a port is selected in the model explorer, then only the port on one (random?) capsule parts in the diagram gets selected. This issue is applicable also for other diagram, e.g. a class diagram where the same class is visualized multiple times. 

> 
> Refresh on edits is a general problem in Papyrus's canonical synchronization
> framework.  I am not sure what can be done about that in the near term;
> almost certainly not for Papyrus-RT 1.0, unless somebody else wants to look
> into it.

I guess we need to see how acceptable the refresh issues are when you do "normal" modeling. In the normal cases, you don't have the superclass and subclass diagrams up at the same time, especially not side-by-side. So maybe some if the refresh issues are in practice not that noticeable. But since I know how confusing these refresh issues can be, I feel that it would be good if this could be looked into in base Papyrus. I guess this is also related to the question if we need an additional release of base Papyrus "in between" Neon.2 and Neon.3.
Comment 60 Peter Cigehn CLA 2016-12-21 08:35:34 EST
I tested the latest build for the inheritance branch looking at protocol inheritance. Here is some feedback:

* I guess the customization of the model explorer, showing the inherited protocol messages, should be made optional in the same as way as for the structure inheritance, with the same reasoning of avoiding clutter.

* The ordering of the inherited elements, both in the model explorer and in the protocol messages list in the properties view for a protocol seem to be that they come after the owned protocol messages. In the legacy tooling the opposite ordering is used, i.e. the inherited stuff comes first and then the owned stuff. I cannot really say which is the most natural, but maybe it is better to align with legacy.

* I see that you now integrated the exclude action with the red x delete button (as we have discussed before). I think that it would be good if the tool tip that shows when hovering the button is updated to "Remove/exclude selected element" or something similar (as the tool tip shows in the legacy tooling). This makes it more clear that you can "remove" also inherited protocol messages.

* As indicated Bug 507529 there is now also a need for a re-inherit button above the protocol message lists. It should also be considered to have a some filter button which can be toggled to either show or hide the excluded elements. This filter button should by default be on, so that excluding a protocol message should also remove it from the protocol message list in the properties view, i.e. it will behave very similar to the delete button. In case you want to re-inherit what you have excluded you either have to do it in the model explorer (where the excluded element always is shown), or if you first toggle the filter button to show excluded elements so that you then can re-inherit it.

* I tried a bit with re-inherit (in the model explorer) and this does not seem to be working fully yet. When re-inheriting an excluded protocol message, it became a redefined protocol message. And when trying to re-inherit a redefined protocol message, nothing happened. When I then tried to delete the redefined protocol message (which in practice also should re-inherit it again), then I got some exception and the protocol message was seemingly gone from the model explorer. When closing and re-opning the model, the inherited protocol message was back again (as expected).

* I tested the automatic naming of new protocol messages, and it seem to work as expected for the subclass protocol, i.e. the next available number is selected including considering any of the inherited protocol messages. This is nice! But what preferably also is needed is that whenever adding new protocol messages to the superclass protocol, then the next available number should be considered also based on the subclass protocol(s). The legacy tooling can handle this situation, and it is probably a good thing if Papyrus-RT also can handle it, to avoid name clashes.

* Considering the previous comment, I tested to explicitly cause a name clash, and the warning indicator in the properties view for the name field does not seem to work for protocol messages (not sure if that ever have worked). But if we would have had a working warning indicator for name clashes for protocol messages, then I assume that it should indicate name clashes also for inherited elements, including warning on the name for a protocol message in a superclass protocol when it clashes with a name of a protocol message in subclass protocol. In the legacy tooling there is actually a live validation (with error severity) which even makes it impossible to cause a name clash between a protocol message both within the same protocol or between protocol messages in a superclass protocol and a subclass protocol (disregarding if the renaming is made in the superclass or the subclass protocol). If you try to rename, the live validation dialog pops up and indicates that the integrity of the model is violated and the transation is rolled-back and nothing is changed.

* I checked a bit in the legacy tooling, and the same kind of live validation is, as expected, made for naming of ports and capsule parts as well, where it is checked for name clashes between superclass and subclass capsules. I guess this is also something to be considered for Papyrus-RT.

Otherwise it looks like a very good start. I checked a bit regarding the persisted model, and from what I could see it looks good. Looks like a good start with the handling when redefining protocol messages. In the legacy tooling, which basically only supports redefining the (single) type (of the hard-code parameter named 'data'), the parameter name is duplicated for the redefined operation (which does not matter since it is hard-coded). So the approach where you leave the name unset and inherit instead looks like a reasonable approach here. The generalization is also shown as expected for the subclass protocol (from what I recall that did not work before).
Comment 61 Charles Rivet CLA 2016-12-21 08:41:05 EST
(In reply to Peter Cigehn from comment #60)

<...snip...>

> * The ordering of the inherited elements, both in the model explorer and in
> the protocol messages list in the properties view for a protocol seem to be
> that they come after the owned protocol messages. In the legacy tooling the
> opposite ordering is used, i.e. the inherited stuff comes first and then the
> owned stuff. I cannot really say which is the most natural, but maybe it is
> better to align with legacy.
<cr>

> 
> * I see that you now integrated the exclude action with the red x delete
> button (as we have discussed before). I think that it would be good if the
> tool tip that shows when hovering the button is updated to "Remove/exclude
> selected element" or something similar (as the tool tip shows in the legacy
> tooling). This makes it more clear that you can "remove" also inherited
> protocol messages.
> 
> * As indicated Bug 507529 there is now also a need for a re-inherit button
> above the protocol message lists. It should also be considered to have a
> some filter button which can be toggled to either show or hide the excluded
> elements. This filter button should by default be on, so that excluding a
> protocol message should also remove it from the protocol message list in the
> properties view, i.e. it will behave very similar to the delete button. In
> case you want to re-inherit what you have excluded you either have to do it
> in the model explorer (where the excluded element always is shown), or if
> you first toggle the filter button to show excluded elements so that you
> then can re-inherit it.
> 
> * I tried a bit with re-inherit (in the model explorer) and this does not
> seem to be working fully yet. When re-inheriting an excluded protocol
> message, it became a redefined protocol message. And when trying to
> re-inherit a redefined protocol message, nothing happened. When I then tried
> to delete the redefined protocol message (which in practice also should
> re-inherit it again), then I got some exception and the protocol message was
> seemingly gone from the model explorer. When closing and re-opning the
> model, the inherited protocol message was back again (as expected).
> 
> * I tested the automatic naming of new protocol messages, and it seem to
> work as expected for the subclass protocol, i.e. the next available number
> is selected including considering any of the inherited protocol messages.
> This is nice! But what preferably also is needed is that whenever adding new
> protocol messages to the superclass protocol, then the next available number
> should be considered also based on the subclass protocol(s). The legacy
> tooling can handle this situation, and it is probably a good thing if
> Papyrus-RT also can handle it, to avoid name clashes.
> 
> * Considering the previous comment, I tested to explicitly cause a name
> clash, and the warning indicator in the properties view for the name field
> does not seem to work for protocol messages (not sure if that ever have
> worked). But if we would have had a working warning indicator for name
> clashes for protocol messages, then I assume that it should indicate name
> clashes also for inherited elements, including warning on the name for a
> protocol message in a superclass protocol when it clashes with a name of a
> protocol message in subclass protocol. In the legacy tooling there is
> actually a live validation (with error severity) which even makes it
> impossible to cause a name clash between a protocol message both within the
> same protocol or between protocol messages in a superclass protocol and a
> subclass protocol (disregarding if the renaming is made in the superclass or
> the subclass protocol). If you try to rename, the live validation dialog
> pops up and indicates that the integrity of the model is violated and the
> transation is rolled-back and nothing is changed.
> 
> * I checked a bit in the legacy tooling, and the same kind of live
> validation is, as expected, made for naming of ports and capsule parts as
> well, where it is checked for name clashes between superclass and subclass
> capsules. I guess this is also something to be considered for Papyrus-RT.
> 
> Otherwise it looks like a very good start. I checked a bit regarding the
> persisted model, and from what I could see it looks good. Looks like a good
> start with the handling when redefining protocol messages. In the legacy
> tooling, which basically only supports redefining the (single) type (of the
> hard-code parameter named 'data'), the parameter name is duplicated for the
> redefined operation (which does not matter since it is hard-coded). So the
> approach where you leave the name unset and inherit instead looks like a
> reasonable approach here. The generalization is also shown as expected for
> the subclass protocol (from what I recall that did not work before).
Comment 62 Charles Rivet CLA 2016-12-21 08:44:04 EST
Please ignore my previous, obviously incomplete comment...keyboard macro conflict with the way Bugzilla works...

Let's try again!

(In reply to Peter Cigehn from comment #60)

<...snip...>

> * The ordering of the inherited elements, both in the model explorer and in
> the protocol messages list in the properties view for a protocol seem to be
> that they come after the owned protocol messages. In the legacy tooling the
> opposite ordering is used, i.e. the inherited stuff comes first and then the
> owned stuff. I cannot really say which is the most natural, but maybe it is
> better to align with legacy.

From a product management point of view: when in doubt, align with legacy.

<...snip...>
Comment 63 Christian Damus CLA 2016-12-21 10:05:51 EST
(In reply to Peter Cigehn from comment #60)
> I tested the latest build for the inheritance branch looking at protocol
> inheritance. Here is some feedback:
> 
> * I guess the customization of the model explorer, showing the inherited
> protocol messages, should be made optional in the same as way as for the
> structure inheritance, with the same reasoning of avoiding clutter.

Actually, the model explorer was already coded to show all inherited messages in a protocol.  But, they didn't show any adornments and they actually *were* the inherited messages, so editing them didn't create redefinitions but instead modified the original inherited elements (impacting all protocols that inherited them).

On the assumption that this was a deliberate design, I maintained the same presentation, only now showing the unique "shadow" for each inherited message.

I can easily change this to align with the inheritance of capsule features.  That is, if there wasn't a reason why we wanted always to show inherited protocol messages.


> * The ordering of the inherited elements, both in the model explorer and in
> the protocol messages list in the properties view for a protocol seem to be
> that they come after the owned protocol messages. In the legacy tooling the
> opposite ordering is used, i.e. the inherited stuff comes first and then the
> owned stuff. I cannot really say which is the most natural, but maybe it is
> better to align with legacy.

That's easily done.


> * I see that you now integrated the exclude action with the red x delete
> button (as we have discussed before). I think that it would be good if the
> tool tip that shows when hovering the button is updated to "Remove/exclude
> selected element" or something similar (as the tool tip shows in the legacy
> tooling). This makes it more clear that you can "remove" also inherited
> protocol messages.

I hope this doesn't need changes in Papyrus to support modification of the tooltip.


> * As indicated Bug 507529 there is now also a need for a re-inherit button
> above the protocol message lists. It should also be considered to have a
> some filter button which can be toggled to either show or hide the excluded
> elements. This filter button should by default be on, so that excluding a

I hope this doesn't need changes in Papyrus to support modification of the tooltip.


> * I tried a bit with re-inherit (in the model explorer) and this does not
> seem to be working fully yet. When re-inheriting an excluded protocol

I shall investigate.

> * I tested the automatic naming of new protocol messages, and it seem to
> work as expected for the subclass protocol, i.e. the next available number
> is selected including considering any of the inherited protocol messages.

That is a benefit of my expansion of the Namespace::ownedMember supersets for capsule classes and protocol message-set interfaces.  :-)


> This is nice! But what preferably also is needed is that whenever adding new
> protocol messages to the superclass protocol, then the next available number
> should be considered also based on the subclass protocol(s). The legacy
> tooling can handle this situation, and it is probably a good thing if
> Papyrus-RT also can handle it, to avoid name clashes.

This is new.  Perhaps it should be tracked in a new bugzilla, preferably together with requirements for UML-RT customization of UML metamodel constraints.  That also is a subject that I have not yet tackled.


> * Considering the previous comment, I tested to explicitly cause a name
> clash, and the warning indicator in the properties view for the name field
> does not seem to work for protocol messages (not sure if that ever have
> worked). But if we would have had a working warning indicator for name

I doubt that ever worked.  Messages of each kind are in distinct namespaces (different message-set interfaces) so UML is perfectly happy that they have the same name.

There should be a constraint for this pan-protocol-container uniqueness of message names in the Protocol stereotype in the profile.  Attempting to address the issue by overriding the member distinguishability queries in the UML metamodel does not make sense.


> reasonable approach here. The generalization is also shown as expected for
> the subclass protocol (from what I recall that did not work before).

Yes, I had to add the generalization into the Protocol facet in the Model Explorer.
Comment 64 Peter Cigehn CLA 2016-12-21 10:56:35 EST
(In reply to Christian W. Damus from comment #63)
> (In reply to Peter Cigehn from comment #60)
> > I tested the latest build for the inheritance branch looking at protocol
> > inheritance. Here is some feedback:
> > 
> > * I guess the customization of the model explorer, showing the inherited
> > protocol messages, should be made optional in the same as way as for the
> > structure inheritance, with the same reasoning of avoiding clutter.
> 
> Actually, the model explorer was already coded to show all inherited
> messages in a protocol.  But, they didn't show any adornments and they
> actually *were* the inherited messages, so editing them didn't create
> redefinitions but instead modified the original inherited elements
> (impacting all protocols that inherited them).
> 
> On the assumption that this was a deliberate design, I maintained the same
> presentation, only now showing the unique "shadow" for each inherited
> message.
> 
> I can easily change this to align with the inheritance of capsule features. 
> That is, if there wasn't a reason why we wanted always to show inherited
> protocol messages.
> 

Not sure how deliberate that could have been since we have not had support for protocol inheritance before. I certainly have not provided any input to this, and have never tested it before. Considering also that the legacy tooling do not show them inherited protocol messages in the model explorer (only in the protocol message list in the properties view), so we still have the same kind of aspect regarding cluttering the model explorer as for port/capsule parts/connectors for the structure inheritance. I definitively think that we should align with the legacy tooling and by default not show the inherited protocol messages. Not until they are redefined/excluded they will be shown. Then we can have an optional customization for users that want them to be shown (in the same was as we have for the structure inheritance).

> > This is nice! But what preferably also is needed is that whenever adding new
> > protocol messages to the superclass protocol, then the next available number
> > should be considered also based on the subclass protocol(s). The legacy
> > tooling can handle this situation, and it is probably a good thing if
> > Papyrus-RT also can handle it, to avoid name clashes.
> 
> This is new.  Perhaps it should be tracked in a new bugzilla, preferably
> together with requirements for UML-RT customization of UML metamodel
> constraints.  That also is a subject that I have not yet tackled.

Yes, this about the name clash between the superclass and any of its subclass(es) is probably something to be tracked separately. It is probably also something that can come as improvement later. I can see if I write a separate one regarding this.

> > * Considering the previous comment, I tested to explicitly cause a name
> > clash, and the warning indicator in the properties view for the name field
> > does not seem to work for protocol messages (not sure if that ever have
> > worked). But if we would have had a working warning indicator for name
> 
> I doubt that ever worked.  Messages of each kind are in distinct namespaces
> (different message-set interfaces) so UML is perfectly happy that they have
> the same name.

I was probably a bit unclear. The name clash aspect for protocol messages is only for the protocol messages in the same direction, i.e. they do belong to the same name space, i.e. within the same <<RTMessageSet>> Interface. 

Actually, now when I test it again, it seem to, partly, be working. But it seem to be some refresh issue. If you rename the protocol message, via the name field in the properties view for the protocol message, then the warning marker does not immediately appear. If you however switches to another protocol message and then back again, then the warning indicator is shown.

So it was apparently more a refresh issue that made me draw the conclusion that it did not work. So it do work as expected also for the subclass case, i.e. if a I give a protocol message the same name as an inherited protocol message (of course in the same direction), then I do get the warning marker. But I have to switch back-and-forth to get the properties view to refresh to show the warning marker.

> 
> There should be a constraint for this pan-protocol-container uniqueness of
> message names in the Protocol stereotype in the profile.  Attempting to
> address the issue by overriding the member distinguishability queries in the
> UML metamodel does not make sense.

As indicated above, I think I was unclear. The uniqueness is only applicable in the same direction. Actually the legacy way of defining symmetrical protocols *is* the name the protocol message in both in the in and out direction exactly the same. In Papyrus-RT we have introduced the "third interface" with direction inOut for that specific purpose to not having to do like that. But I guess we still need to support legacy protocol which do have symmetrical protocol messages defined in both in and out direction.
Comment 65 Peter Cigehn CLA 2016-12-21 11:04:19 EST
One more quick feedback for today: I tested a bit more with the structure inheritance and I am not sure if the refresh issues that we have has gone worse, or if it is a new phenomenon I am seeing.

If I create a capsule with some structure (ports, connectors, capsule parts). And then establish a generalization from another capsule. Then the layout of the structure diagram does not really follow the superclass capsule, at least not the port position. Pressing F5 does not really help either (so it does not seem to be a refresh issue).

Actually, if I now check the (really great) tool bar button, I can see that none of the ports indicates that they are following the superclass view. If I now do Ctrl+A selecting everything, and then press the follow button, then the layout becomes correct.

So it seem like the ports does not follow its superclass view directly after you have established a generalization.
Comment 66 Peter Cigehn CLA 2016-12-22 05:46:30 EST
Created attachment 266026 [details]
Example model which shows ports not following its superclass

(In reply to Peter Cigehn from comment #65)
> One more quick feedback for today: I tested a bit more with the structure
> inheritance and I am not sure if the refresh issues that we have has gone
> worse, or if it is a new phenomenon I am seeing.
> 
> If I create a capsule with some structure (ports, connectors, capsule
> parts). And then establish a generalization from another capsule. Then the
> layout of the structure diagram does not really follow the superclass
> capsule, at least not the port position. Pressing F5 does not really help
> either (so it does not seem to be a refresh issue).
> 
> Actually, if I now check the (really great) tool bar button, I can see that
> none of the ports indicates that they are following the superclass view. If
> I now do Ctrl+A selecting everything, and then press the follow button, then
> the layout becomes correct.
> 
> So it seem like the ports does not follow its superclass view directly after
> you have established a generalization.

Followup to this: From what I can see, this seem to happen only in the case you have at least two connectors of which one goes to an internal behavior port. The attached model shows how Capsule3 and its ports does not follow its superclass. Try creating a new Capsule4 in this and establish a generalization to Capsule1 and you will see the same issue also for Capsule4.
Comment 67 Peter Cigehn CLA 2016-12-22 06:14:21 EST
I tested the latest build from the inheritance branch and looked a bit on the updates to the properties view for a protocol. I see that you have changed from the three column layout to a two column. This I feel is highly related to Bug 495137, where the need for improvements have been raised from the perspective of having multiple parameters (which makes the lists to be become wider). In that bug there are two suggestion: Either a one column layout, with the lists simply on top of each other, or possibly having sub-tabs for each of the direction and only have one list per such sub-tab (this is similar to how it looks like in the legacy tooling). I am not fully sure that the current 2 column approach solves the issue raised in Bug 495137 (and the layout feels a bit non-symmetrical). Anyway, I think that we probably should consider Bug 495137 at the same time if we have a need for changing the layout due to the added buttons, and maybe start with testing a single column layout instead, with the three lists "stacked on top of each other".

It was nice to see though that it was possible to make this customization with the new re-inherit button and the filter toggle button.

I am not sure though if I was unclear regarding the filter button, or if you felt that there was a need for also filtering the inherited protocol messages. As I tried to explain, the need I saw for this filter button was to filter excluded elements. The inherited elements could very well always be shown (as they are in the legacy tooling). The scenario to be supported with the filter button was more related to exclusion, i.e. when you exclude something, they should by default disappear from the list (as if they were deleted). But to be able to re-inherit them, there should be a toggle to also see any excluded elements in the list (in the legacy tooling there are some inconsistencies regarding this where excluded elements gets removed from the list, but you are unable to "get hold of them" if you want to re-inherit them). As the filter button works now, it actually only filters inherited elements, but the excluded elements are always shown (disregarding how the filter is toggled). Sure, it maybe could be useful also to easily filter inherited protocol messages, but having two filter buttons maybe is taking this a bit too far. Could you elaborate a bit how you reasoned about this?

I tested the re-inherit also for protocol messages, and it seem to work a bit better. But still there seem to some issues with re-inheriting a redefined protocol message. I re-defined the parameter type for a protocol message with one single parameter. The redefinition seemed to work fine. But when re-inheriting, it looks like the protocol message was re-inherited, e.g. the RTRedefinedElement, but it kept its parameter list and its redefined type. So it was only partly "re-inherited".
Comment 68 Peter Cigehn CLA 2016-12-22 09:48:36 EST
I just tested the latest build and the one column layout looks better. And the toggle filter button works exactly as I had expected. The red x delete button now behaves as a "delete" button, but you are still able to re-inherit excluded elements if you want to, by first toggling the show excluded filter button and then use the re-inherit button! Nice!

There is still one aspect left with the single column layout though, but that should probably be tracked with Bug 495137 instead. If you have a protocol message parameter with lots of parameters, then when the list becomes wider, then the buttons are right aligned with the width of the list, which causes the buttons to go outside the frame of the properties view, and you have to use the horizontal scrollbar to reach the buttons. It would be nice if the buttons always were visible, disregarding if the list was widened due to a long parameter list. This could of course be solved in different ways, e.g. by shortening/cutting off the parameter list with a ... notation or something similar. Of if there is some way to control the layout of the list and the alignment of the buttons. Don't spend any time on this unless you have some quick solution up your sleeve, and we track this specific issue with Bug 495137 as originally intended.

I see that the name field of the protocol is still disabled, and don't even display the name of the protocol. I hope that you have this on your todo list also. This also impact the dialog when creating new protocols, e.g. when you create a new port and select to type it with a new protocol. You can't name the protocol in that dialog either (I assume they are both issues have the same root cause).
Comment 69 Peter Cigehn CLA 2016-12-23 10:17:43 EST
(In reply to Peter Cigehn from comment #68)
> I see that the name field of the protocol is still disabled, and don't even
> display the name of the protocol. I hope that you have this on your todo
> list also. This also impact the dialog when creating new protocols, e.g.
> when you create a new port and select to type it with a new protocol. You
> can't name the protocol in that dialog either (I assume they are both issues
> have the same root cause).

This issue now seem to be fixed. The name is not disabled, and the name is now shown. There is however some regression related to the green flashing of the name field, whenever you switch from another kind of model element to a protocol. You do not see the green flash when switching from one protocol to another protocol.

I do know that we have had refresh issues, and similar problems like this, before, especially when switching between model elements of the same kind, versus switching between model elements of different kinds.

Steps to reproduce:

1) Create a UML-RT model
2) Create two protocols in this model
3) Select the root element of the model
4) Keep the properties view open with the UML-RT tab visible
5) Now select the first protocol 
6) Observe how the name field flashes green
7) Select the second protocol
8) The name field does not flash green

The same flashing can actually be seen when the protocol is created as well. This cannot be seen with the latest build on the master branch.
Comment 70 Eclipse Genie CLA 2017-01-04 09:19:57 EST
New Gerrit change created: https://git.eclipse.org/r/88008
Comment 71 Eclipse Genie CLA 2017-01-04 10:15:06 EST
New Gerrit change created: https://git.eclipse.org/r/88010
Comment 72 Eclipse Genie CLA 2017-01-04 10:15:21 EST
New Gerrit change created: https://git.eclipse.org/r/88011
Comment 74 Eclipse Genie CLA 2017-01-08 12:38:19 EST
New Gerrit change created: https://git.eclipse.org/r/88224
Comment 76 Eclipse Genie CLA 2017-01-10 12:33:01 EST
New Gerrit change created: https://git.eclipse.org/r/88393
Comment 80 Eclipse Genie CLA 2017-01-10 17:22:43 EST
New Gerrit change created: https://git.eclipse.org/r/88421
Comment 82 Christian Damus CLA 2017-01-11 16:18:37 EST
The UML metamodel implementation for UML-RT semantics of inheritance and redefinition in Capsule Structure and Protocols is merged to the master branch for the 0.9 release.

Bug 510315 is raised for the enhancement to extend this into state machines.  That would obviously be based on the foundation implemented here, including the integrations in the Core and Tooling components.
Comment 83 Eclipse Genie CLA 2017-01-15 08:13:30 EST
New Gerrit change created: https://git.eclipse.org/r/88724
Comment 85 smaoui asma CLA 2017-01-18 05:11:48 EST
Hello, 
while fixing Bug 509720, I discovered a regression in the replication field caused by this gerrit : https://git.eclipse.org/r/#/c/88011/10 

to reproduce the regression:

1) create a port or a capsule part in the capsule diagram
2) set the replication field to a string (abc for example)
3) reset it to an integer : 5 for example
4) reset it to a string abcd : see now that this new value is not taken into account, the multiplicity still 5, (in the diagram and in the uml property view, muliplicity = 5 and not abcd.

This is due to changes occuring in the PropertyReplicationObservableValue API implementation that is responsible to observe and set the new value of the replication field.

Christian, any idea before I start to analyze the changes ?

Asma
Comment 86 Peter Cigehn CLA 2017-01-18 07:08:37 EST
In reply to smaoui asma from comment #85)
> 1) create a port or a capsule part in the capsule diagram
> 2) set the replication field to a string (abc for example)
> 3) reset it to an integer : 5 for example
> 4) reset it to a string abcd : see now that this new value is not taken into
> account, the multiplicity still 5, (in the diagram and in the uml property
> view, muliplicity = 5 and not abcd.

I can add that this seem to a general issue with changing the replication to a string from the state when when it is explicitly set to some integer value. If you follow the steps to reproduce above and instead select the unset case, i.e. selecting "None (1)" in between step 3 and 4, then it works. It only seem to be a case when going from lower/upper being explicitly set with a LiteralInteger/UnlimitedNatural to OpaqueExpressions (which is used for storing a string entered in the replication field). I reopen this bug to track this issue. We have also had similar refresh issues being discussed a tracked by Bug 507552.
Comment 87 Christian Damus CLA 2017-01-18 13:43:55 EST
(In reply to smaoui asma from comment #85)
> 
> Christian, any idea before I start to analyze the changes ?

The problem is that the CreateEditBasedElementCommand that would create the OpaqueExpression in the multiplicity-element bounds reports that it is not executable because there is already a ValueSpecification set in the bounds reference.

Unfortunately, there is no way to tell this command that it is okay to execute because the previous command in a composite will have destroyed that existing ValueSpecification.
Comment 88 Eclipse Genie CLA 2017-01-18 16:52:09 EST
New Gerrit change created: https://git.eclipse.org/r/89051
Comment 90 Eclipse Genie CLA 2017-01-19 14:40:33 EST
New Gerrit change created: https://git.eclipse.org/r/89137
Comment 92 Christian Damus CLA 2017-01-19 16:35:07 EST
(In reply to Eclipse Genie from comment #89)
> Gerrit change https://git.eclipse.org/r/89051 was merged to [master].

Fixes.

(In reply to Eclipse Genie from comment #91)
> Gerrit change https://git.eclipse.org/r/89137 was merged to [master].

Tests.
Comment 93 Charles Rivet CLA 2017-03-23 14:45:32 EDT
Closing