Bug 464387

Comment 1 Peter Cigehn 2015-05-19 10:43:38 EDT

I just checked the latest update of the UML-RT profile. The message for "All operations of a capsule are guarded" has indeed been corrected.

But the id of the same constraint, "All operations of a capsule are guarded", is still "allOperationsSequential", specified by the attribute "id" of the "ValidationRule" stereotype applied on this constraint.

This is of course minor compared to the incorrect message that has been fixed, but still I assume that this can cause some confusion. Since the id is also shown when checked the constraints under Preferences > Model Validation > Constraints, this incorrect id is shown to end users.

Comment 2 Ansgar Radermacher 2015-09-25 05:30:07 EDT

Status of 469923 changed.

Comment 3 Peter Cigehn 2015-09-25 07:08:59 EDT

Created attachment 256840 [details]
Screen shot showing the incorrect constraint id

I've checked this in the latest Papyrus-RT build and from what I can see the constraint id as described in Comment 1 is still incorrect. See the attached screen shot.

Comment 4 Peter Cigehn 2015-09-25 07:09:24 EDT

Reopening since this has still not be fixed.

Comment 5 Peter Cigehn 2015-09-25 07:22:47 EDT

When looking through a few more, there are more constraints which have confusingly named ids, e.g. the constraint "All unwired ports (SAPs and SPPs) must be behavior ports" has the id "hasProtectedVisibility" which seem to be completely unrelated to each other. 

A few more which seem to have unaligned names of its id are "All service ports must have public visibility and protected otherwise" (current id is 'publicPortIsService' which probably should be 'servicePortIsPublic' instead), "A relay port must be a service port" (current id is 'havePublicVisibility'), "only SPPs can publish their name" (currend it is 'noPublishName').

I am not sure who is responsible for updating what, but it seem pretty clear that whenever any of the constraints in the UML-RT profile has been changed, then its id pretty quickly gets out of sync.

Please check how this will look like for the end-user by opening Preferences > Modeling Validations > Constraints and select org.eclipse.papyrusrt.core.validation.umlrealtime (which also could be questioned why the category don't have a more user-friendly name instead of this internal id).

Comment 6 Ansgar Radermacher 2015-09-25 07:26:56 EDT

Hi Peter,

thanks for checking. I re-generated the validation plugin from the new profile that , thinking that it was only a re-generation issue. But, ID and name were simply inconsistent in the profile.
I'll post another comment on 469923

Comment 7 Ansgar Radermacher 2015-09-25 07:39:02 EDT

IDs do not become inconsistent due to editing in other elements. But -of course- if you change the name of a constraint (or the body), you have to make sure that the ValidationRule stereotype might need an update as well. This might not be the case for a user that focus on the semantic issues.
Therefore, my proposition in 469923 to remove the manual ID entries.

(In reply to Peter Cigéhn from comment #5)
> ...
> 
> I am not sure who is responsible for updating what, but it seem pretty clear
> that whenever any of the constraints in the UML-RT profile has been changed,
> then its id pretty quickly gets out of sync.
> 
> ...

Comment 8 Peter Cigehn 2015-09-25 07:50:16 EDT

(In reply to Ansgar Radermacher from comment #7)
> IDs do not become inconsistent due to editing in other elements. But -of
> course- if you change the name of a constraint (or the body), you have to
> make sure that the ValidationRule stereotype might need an update as well.
> This might not be the case for a user that focus on the semantic issues.
> Therefore, my proposition in 469923 to remove the manual ID entries.

That's why I ask. I guess that Bran has simply renamed and changed the OCL expression for a number existing constraints, without considering that the constraint has additional information in the properties of the applied DSML validation stereotype <<ValidationRule>>. That's why I asked who is responsible for updating what? Can you please synchronize with Bran regarding how such a change really should be made, i.e. if a constraint and its semantics (name and possibly its OCL body) is changed, then how should that change really be made? By completely removing the existing constraint, create a new one, apply the <<ValidationRule>> stereotype and fill in the properties (again). Or change the existing constraints (update name and possibly OCL expression) and *also* updating and ensuring consistenty with the properties of the <<ValidationRule>> stereotype.

Who have applied the <<ValidationRule>> stereotype and filled in its properties? Has Bran done that, or is someone else that has done that after Bran has created the constraints in the profile?

This in general is not only an issue regarding the id, but also a problem with the "message" property of the <<ValidationRule>> stereotype, since the "message" also quickly can get out of sync of you change the name/semantic of an existing constraint (which has happened once before as we have already iterated once in Comment 1).

So I am not sure that removing the manual id really solves the actual process issue, and responsiblity issue, of who is doing what when updating the constraints, since you still have the same inconsistency issue with 'message'. 

Personally I would also prefer if the "Description" property had some meaningful information in them as well, so that users can get more information about what the constraints is all about. But that is another question, and is out of the scope of this Bugzilla.

Comment 9 Ansgar Radermacher 2015-09-25 08:09:28 EDT

You are right. There are two different issues.
(1) Ease to keep things in sync
I agree that the ID is not the only thing that must manually be adapted after a change (if set). But it is an additional data field to synchronize hat does not have any added value. Therefore, it is not part of the "validation rule definition" tab. I don't remember who filled it out initially.

In addition, we could show some attributes of the ValidationRule stereotype in the diagram (e.g. message & severity). This would help avoiding inconsistencies in the first place.

(2) Establish a good process.
The gerrit review is an option to check model consistency when an update is requested. I should have been my task to verify these issues (in particular, since you already pointed out an inconsistency in the old model)

Comment 10 Bran Selic 2015-09-27 09:22:05 EDT

I thought I had changed both parts in all the constraints - although, clearly, I missed at least one (apologies about that).

Regarding who owns the profile, perhaps the best solution is for me to own it, but for someone at CEA (Ansgar) to do the formal submission via Git. The latter is because I've had all kinds of problems with these processes since I am not a regular user. Along the way, my Eclipse image got corrupted and now I have to relearn how to do it all over again.

Comment 11 Ansgar Radermacher 2015-09-28 03:47:30 EDT

(In reply to Bran Selic from comment #10)
> I thought I had changed both parts in all the constraints - although,
> clearly, I missed at least one (apologies about that).
> 
> Regarding who owns the profile, perhaps the best solution is for me to own
> it, but for someone at CEA (Ansgar) to do the formal submission via Git. The
> latter is because I've had all kinds of problems with these processes since
> I am not a regular user. Along the way, my Eclipse image got corrupted and
> now I have to relearn how to do it all over again.

Hi Bran, Peter

I agree with respect to that process (Bran owns it, I verify it and take care of technical aspects).
The profile right now in gerrit corresponds to Bran's latest commit, except for resetting the ValidationRule attributes "message" and "id". It also contains some layout modifications. There is also a style sheet that sets the visibility of all ValidationRule attributes to false (by default) except message. This facilitates showing the "message" attribute in the diagram, when it is not empty (you do not have to disable all other attributes manually). Currently, it has no effect, since we do not show the stereotype content for any of the constraints.

A new change that I've not commited yet is a modification of the style sheet which could be useful in the future: it will automatically set the color of the constraint to red, if the "severity" attribute of the stereotype is set to error and to orange, if it is a warning. The usefulness is currently limited, as all constraints have error severity, but if we will have constraints of mixed severity in the future, it would provide an immediate visual feedback about the severity. Should I commit that to gerrit?

Comment 12 Peter Cigehn 2015-09-28 04:54:34 EDT

I think Bran should give his view regarding the layout and automatic coloring of constraints in the diagram, since I assume those diagrams are being used "as-is" in the profile document. It sounds like a useful feature, even though I am not sure though, how many constraints with severity warning we will actually add to the profile itself in the future. I guess that constraints with severity warning is much more often added by individual users/teams depending on their specific use of the UML-RT profile.

Comment 13 Bran Selic 2015-09-28 15:42:14 EDT

Regarding the coloring proposal, which sounds pretty cool: One problem with it is that, I may want to dynamically change what is an error and what is a warning, depending on when I am doing the validation. Although I suspect this is not possible with the current implementation, I definitely see it as an important future requirement. So, cool as it sounds, I think it will be better to hold off on it.

Comment 14 Peter Cigehn 2015-10-02 05:00:24 EDT

I have verified the updated profile in the latest Papyrus-RT build. And now when the explicit setting of id and message have been removed, they are by definition consistent with each other.

One consequence though (which I hadn't anticipated) is that the message that was automatically derived from the name states:

'<constraint name> not valid'

At first this was extremely confusing, since I though that the constraint itself was not valid, e.g. due to incorrect OCL or something. But then I realized that was the message really meant was 

'<constraint name> is violated'

But since this I guess is part of the DSML validation plugin tooling, and how it choose to automatically derive a message, that derived message can be tweaked if this is found to be confusing. 

As I understood it earlier, I though that the message simply would have been generated as 

'<constraint name>' 

with no additional text after, which maybe had been better. But maybe this is only me that gets confused and "normal" users will maybe don't even notice this 'not valid' part.

I'm setting this into resolved fixed. Any changes to the DSML validation tooling I guess should be part of another Bugzilla anyway.

Comment 15 Peter Cigehn 2015-10-02 05:00:47 EDT

Verified to be fixed in the latest Papyrus-RT build.