Comment 1 Camille Letavernier 2014-10-13 10:11:00 EDT

I think it's a little bit more complex than just presenting it in the UI. Papyrus uses a "Multiplicity Editor" to present/edit the multiplicity, but this widget can only handle a subset of values (i.e. only LiteralInteger/UnlimitedNatural, whereas UML supports any ValueSpecification). So, if you enter "UPPER_CARDINALITY", the Papyrus Multiplicity editor will not be able to understand it and won't be able to present it properly. This may lead to other issues

Comment 2 Patrik Nandorf 2014-10-13 11:01:50 EDT

"the Papyrus Multiplicity editor will not be able to understand it and won't be able to present it properly" - I know, thus the Bugzilla :-)

Could you elaborate on "This may lead to other issues"?

Comment 3 Camille Letavernier 2014-10-13 11:33:36 EDT

> Could you elaborate on "This may lead to other issues"?

The UML specification explains that "Anything which can be evaluated to an Integer" is a valid Lower/Upper bound. Rather vague and tool-dependent. Papyrus currently can't evaluate "UPPER_CARDINALITY" to an integer. I don't know how much impact this has; maybe none at all (Except on generic code generators, which will probably produce invalid code)

Moreover, the Multiplicity editor is a nice lightweight tool for editing multiplicities, so it shouldn't be completely removed. But since it doesn't support non-integer value specifications, something needs to be done here. Maybe the multiplicity editor should be disabled as soon as an unknown Upper/Lower bound is specified (Or have a magic value "<<Custom Multiplicity>>"). It may also be an "Advanced" button on the Multiplicity editor (Which allows edition of Lower/Upper bound directly)

Comment 4 Michael Golubev 2015-04-07 19:40:38 EDT

It is clear how to change the code to correctly display the StringLiteral's at the diagram and in the property sheet. 

But waht about editing mode? Blindly accepting whatever user entered and transforming it into the StringLiteral does not look great. 

Consider e.g "0...3" (parsed as {lover=<int>0; upper=<string>".3"}, and then shown back to user again as "0...3" thus difficult to spot. 

Should not we only accept ALL_CAPS_STRING's or may be even just a preconfigured set of strings (via project level preferences -- note that it would allow to just add known fixed set of choices into the prop sheet combo)? 
Should we allow multiple words? 

(And what about the text containing specifically ']' char? can we assume it indicates a user mistake?)

Comment 5 Michael Golubev 2015-04-07 19:56:10 EDT

Note BTW that preference with mapping of UPPER_CARDINALITY -> 42 would also allow Papyrus to "evaluate "UPPER_CARDINALITY" to an integer" in some sense. 

Also in this case OpaqueExpression would be better than a StringLiteral. But this replacement may be done by the import. What do you think?

Comment 6 Patrik Nandorf 2015-04-08 00:45:57 EDT

My comments below is based on the needs spawning the bugzilla.

(In reply to Michael Golubev from comment #4)
> It is clear how to change the code to correctly display the StringLiteral's
> at the diagram and in the property sheet. 
> 
> But waht about editing mode? Blindly accepting whatever user entered and
> transforming it into the StringLiteral does not look great. 
> 

I'm probably missing something but a straight mapping to a StringLiteral would be fine unless it matches the X..Y pattern in which case it should be mapped to upper and lower cardinalities.


> Consider e.g "0...3" (parsed as {lover=<int>0; upper=<string>".3"}, and then
> shown back to user again as "0...3" thus difficult to spot. 
> 

Some naive ideas could be to add spaces around the '..' ("0 .. .3") and using colors to differentiate between the cardinatilies and the '..'.

> Should not we only accept ALL_CAPS_STRING's or may be even just a
> preconfigured set of strings (via project level preferences -- note that it
> would allow to just add known fixed set of choices into the prop sheet
> combo)? 
> Should we allow multiple words? 
> (And what about the text containing specifically ']' char? can we assume it
> indicates a user mistake?)

Since we write 'expressions' (as strings) here we need more than just a predefined set of choices.

Comment 7 Camille Letavernier 2015-04-08 04:30:32 EDT

Hi,

There are (at least) two distinct tasks here

Nicolas is already working on the properties view (Edition/Display), with a specific editor to type different kinds of value specifications.

What remains to be done is the display in the diagram (And maybe in several other locations; I don't know exactly where these multiplicities are displayed in a structured way)

Comment 8 Nicolas FAUVERGUE 2015-04-09 06:21:14 EDT

Hi,

To introduce in some words what i am doing :

All the actual 'StringCombo' used for the multiplicity edition will be replaced by two 'modes' of edition with one button which allow to switch of mode:
  - The 'simple' mode : Actual edition ('0..3' for example) which create LiteralInteger for the lower value and LiteralUnlimitedNatural for the upper value

  - The 'advanced' mode : Two editors of ValueSpecification (one for lower value and one for upper value). Those two editors are basic ValueSpecification editors but can be extended by the XText editor of ValueSpecification (will be done for all when the bug #463677 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=463677) is closed ?)

The XText ValueSpecification editors allow to create the value specifically depending on lower value or upper value edition.
For example : If '5' is filled in the lower value, a LiteralInteger with value '5' will be created for the lower value.
If '*' is filled in the upper value, a LiteralUnlimitedNatural with value '*' will be created for the upper value.

The Xtext ValueSpecification editor used the created grammar corresponding to the bug #459747 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=459747). The file managing the ValueSpecification parsing grammar is 'UmlValueSpecification.xtext'.

Comment 9 Eclipse Genie 2015-04-09 09:00:26 EDT

New Gerrit change created: https://git.eclipse.org/r/45542

WARNING: this patchset contains 4199 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire

Comment 10 Eclipse Genie 2015-04-09 13:21:39 EDT

New Gerrit change created: https://git.eclipse.org/r/45583

Comment 11 Eclipse Genie 2015-04-21 11:58:47 EDT

Gerrit change https://git.eclipse.org/r/45583 was merged to [master].
Commit: http://git.eclipse.org/c/papyrus/org.eclipse.papyrus.git/commit/?id=f7f60cd2835dc93bf6eb22af002e1ec4d1e018a8

Comment 12 Camille Letavernier 2015-04-21 11:59:35 EDT

> Gerrit change https://git.eclipse.org/r/45583 was merged to [master].

This covers the display of exotic multiplicities in the Diagrams

Comment 13 Patrik Nandorf 2015-04-22 03:33:24 EDT

(In reply to Camille Letavernier from comment #12)
> > Gerrit change https://git.eclipse.org/r/45583 was merged to [master].
> 
> This covers the display of exotic multiplicities in the Diagrams

So the current status is that displaying i diagrams is implemented but editing in the properties view is not yet done?

Comment 14 Nicolas FAUVERGUE 2015-04-22 03:44:03 EDT

(In reply to Patrik Nandorf from comment #13)
> So the current status is that displaying i diagrams is implemented but
> editing in the properties view is not yet done?

Hi, concerning the property view contribution, this one is 'finished' but may be reviewed before merged.

Comment 15 Patrik Nandorf 2015-04-22 03:52:39 EDT

(In reply to Nicolas FAUVERGUE from comment #14)
> (In reply to Patrik Nandorf from comment #13)
> > So the current status is that displaying i diagrams is implemented but
> > editing in the properties view is not yet done?
> 
> Hi, concerning the property view contribution, this one is 'finished' but
> may be reviewed before merged.

Do you have a link to the gerrit change?

Comment 16 Nicolas FAUVERGUE 2015-04-22 04:09:04 EDT

(In reply to Patrik Nandorf from comment #15)
> (In reply to Nicolas FAUVERGUE from comment #14)
> > (In reply to Patrik Nandorf from comment #13)
> > > So the current status is that displaying i diagrams is implemented but
> > > editing in the properties view is not yet done?
> > 
> > Hi, concerning the property view contribution, this one is 'finished' but
> > may be reviewed before merged.
> 
> Do you have a link to the gerrit change?

https://git.eclipse.org/r/#/c/45542/

Comment 17 Camille Letavernier 2015-04-22 04:32:31 EDT

Another step will be to update the Property/Port Advanced Editor (XText grammar) to support the new ValueSpecification Literals

Do you need something similar for e.g. Parameters? (And other MultiplicityElements?) Currently it has only been done for Properties

Comment 18 Patrik Nandorf 2015-04-22 09:37:04 EDT

No, I think it currently is only needed for properties.

(In reply to Camille Letavernier from comment #17)
> Another step will be to update the Property/Port Advanced Editor (XText
> grammar) to support the new ValueSpecification Literals
> 
> Do you need something similar for e.g. Parameters? (And other
> MultiplicityElements?) Currently it has only been done for Properties

Comment 19 Eclipse Genie 2015-04-27 08:12:54 EDT

Gerrit change https://git.eclipse.org/r/45542 was merged to [master].
Commit: http://git.eclipse.org/c/papyrus/org.eclipse.papyrus.git/commit/?id=f5d0993829fc8d5302fe932a1d69bc0ec7e15ce3

Comment 20 Camille Letavernier 2015-04-27 08:15:06 EDT

> Gerrit change https://git.eclipse.org/r/45542 was merged to [master].

This contribution provides the new editor for multiplicities. It is possible to switch from Simple to Advanced mode either from the Papyrus Preferences, or using the new button next to the multiplicity editor directly

This contribution still contains some minor issues:

- The validation is still enabled for exotic multiplicities and reports warnings (e.g. when typing 2..MAX, you will get a validation warning)
- When typing "*" in the Upper bound, using the XText Multiplicity editor, the editor displays "null=*" (It may also happen for other values, but it doesn't seem to be consistent)
- The Simple Multiplicity Editor doesn't properly refresh its read-only state. If you switch from a Property [1..2] to a property [1..MAX], the editor remains writable. Switching in the other direction, the editor remains read-only

Comment 21 Eclipse Genie 2015-04-28 04:19:36 EDT

New Gerrit change created: https://git.eclipse.org/r/46642

Comment 22 Eclipse Genie 2015-04-30 10:41:51 EDT

Gerrit change https://git.eclipse.org/r/46642 was merged to [master].
Commit: http://git.eclipse.org/c/papyrus/org.eclipse.papyrus.git/commit/?id=d1ad71e44d97bd56fd171e36bbb1f2ef12b74958

Comment 23 Camille Letavernier 2015-04-30 10:45:22 EDT

> Gerrit change https://git.eclipse.org/r/46642 was merged to [master].

Improves integration on the Propertie's view multiplicity editor

Some exceptions are still happening when changing selection from a property to another:

!ENTRY org.eclipse.core.databinding 4 0 2015-04-30 16:40:04.266
!MESSAGE Unhandled exception: assertion failed: Getter called on disposed observable org.eclipse.papyrus.uml.tools.databinding.PapyrusObservableValue@7622af01
!STACK 0
org.eclipse.core.runtime.AssertionFailedException: assertion failed: Getter called on disposed observable org.eclipse.papyrus.uml.tools.databinding.PapyrusObservableValue@7622af01
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:110)
	at org.eclipse.core.databinding.observable.ObservableTracker.getterCalled(ObservableTracker.java:252)
	at org.eclipse.core.databinding.observable.value.AbstractObservableValue.getterCalled(AbstractObservableValue.java:92)
	at org.eclipse.core.databinding.observable.value.AbstractObservableValue.getValue(AbstractObservableValue.java:79)
	at org.eclipse.papyrus.infra.widgets.editors.ReferenceDialog.getValue(ReferenceDialog.java:370)
	at org.eclipse.papyrus.uml.properties.widgets.ExtendedMultiplicityDialog.canEditStringCombo(ExtendedMultiplicityDialog.java:60)
	at org.eclipse.papyrus.infra.widgets.editors.MultiplicityDialog.setReadOnly(MultiplicityDialog.java:472)
	at org.eclipse.papyrus.views.properties.widgets.AbstractPropertyEditor.applyReadOnly(AbstractPropertyEditor.java:240)
	at org.eclipse.papyrus.views.properties.widgets.AbstractPropertyEditor$2$2.run(AbstractPropertyEditor.java:632)
	at org.eclipse.core.databinding.observable.Realm$1.run(Realm.java:150)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.core.databinding.observable.Realm.safeRun(Realm.java:154)
	[...]

This doesn't cause any behavioral issue (None I could notice at least) but indicates that some observables are not properly disposed or are disposed but still used, which may lead to unexpected side effects or memory leaks, and will spam the user's error log

Comment 24 Eclipse Genie 2015-04-30 11:17:09 EDT

New Gerrit change created: https://git.eclipse.org/r/46900

Comment 25 Eclipse Genie 2015-05-18 08:09:09 EDT

Gerrit change https://git.eclipse.org/r/46900 was merged to [master].
Commit: http://git.eclipse.org/c/papyrus/org.eclipse.papyrus.git/commit/?id=653ff51f82c58f73dbd9523202e1c147e1160137

Comment 26 Patrik Nandorf 2015-05-29 04:22:59 EDT

Created attachment 253920 [details]
Screenshot of faulty multiplicity values in the property view

Comment 27 Patrik Nandorf 2015-05-29 04:23:52 EDT

Some comments on this one using RC1

When changing the multiplicity of a property in the property view from the default to '1..2' multiplicity is changed in the diagram and in the model but it is displayed as '1..0' in the property view. See attached screen shot

Comment 28 Patrik Nandorf 2015-05-29 04:39:26 EDT

I'd like to be able to in the simple mode just write e.g. 'MAX..MIN' and get two LiteralStrings auto-created with MIN resp MAX as values.

This would greatly simplify creating these exotic cardinalities.

My point is that anything not being LiteralInteger/LiteralUnlimitedNatural should be regarded as ListeralString.

I think though it is ok that editing is done through the advanced editor even though it would be great if the values of LiteralStrings could be edited also in the simple mode.

Comment 29 Camille Letavernier 2015-05-29 04:55:07 EDT

There is an additional component to use an xtext editor instead of the current ad-hoc text parser to support complex multiplicities in text mode. However this would rely on some properties view improvements (Especially widget substitution) which didn't make it into Mars

On your screenshot, however, I would expect the simple multiplicity editor to be disabled (grayed), because it is not a simple multiplicity. Maybe a Linux issue?

Comment 30 Patrik Nandorf 2015-05-29 05:03:38 EDT

(In reply to Camille Letavernier from comment #29)
> There is an additional component to use an xtext editor instead of the
> current ad-hoc text parser to support complex multiplicities in text mode.
> However this would rely on some properties view improvements (Especially
> widget substitution) which didn't make it into Mars
> 

Ok, 

> On your screenshot, however, I would expect the simple multiplicity editor
> to be disabled (grayed), because it is not a simple multiplicity. Maybe a
> Linux issue?

Sorry, I don't follow. Why would the simple editor be disabled when I enter e.g. 1..2?

Comment 31 Camille Letavernier 2015-05-29 05:05:09 EDT

> Sorry, I don't follow. Why would the simple editor be disabled when I enter e.g. 1..2?

My mistake, I read too fast... I thought it was a String-based multiplicity.

Definitely a bug

Comment 32 Patrik Nandorf 2015-05-29 06:09:05 EDT

I guess for this one a new bugzilla should be created?

(In reply to Patrik Nandorf from comment #30)
> (In reply to Camille Letavernier from comment #29)
> > There is an additional component to use an xtext editor instead of the
> > current ad-hoc text parser to support complex multiplicities in text mode.
> > However this would rely on some properties view improvements (Especially
> > widget substitution) which didn't make it into Mars
> > 
> 
> Ok,

Comment 33 Ansgar Radermacher 2015-10-28 03:34:42 EDT

The fix for the bug seems to be the cause of bug 479331 (double validation of MM constraints), since the new class DelegatingConstraintProviderWithOverride adds constraints for all validation methods of class UMLValidator. This implies that these constraints are evaluated twice.

Comment 34 Ansgar Radermacher 2015-10-28 11:12:05 EDT

(In reply to Ansgar Radermacher from comment #33)
> The fix for the bug seems to be the cause of bug 479331 (double validation
> of MM constraints), since the new class
> DelegatingConstraintProviderWithOverride adds constraints for all validation
> methods of class UMLValidator. This implies that these constraints are
> evaluated twice.

In order to avoid the double evaluation, we need to decide whether we remove the invocation of the UMLValidator.validate(...) method from the Papyrus validator adapter or the reification of the UML MM constraints.

While it is nice to see the constraints explicitly on the UI level (and it could eventually be useful of being able to de-active some), the main question is whether these explicit constraints cover all constraints that are currently called.

Another question is why the new MM validation constraints have been added in the context of this bug (with gerrit https://git.eclipse.org/r/#/c/46642/). I see that we explicitly add a constraint "UpperGeLower", but we could have done that without adding other MM constraints.

For the first question: the code within DelegatingConstraintProviderWithOverride creates a new constraint for each method of the UMLValidator class that complies with the naming schema validate<AB>_validate<XY> (and the right number/types of arguments). This covers validation leafs within the UMLValidator, e.g. validateClass itself is not covered but most of the methods it calls in turn (like validateElement_validateHasOwner). However, validateClass also calls methods from the superclass EObjectValidator which do not correspond to the naming schema (e.g. validate_EveryMultiplicityConforms) and are therefore not explicitly included as constraints. The question is whether the EObjectValidator constraints are not relevant in the UML context or already checked otherwise.

Comment 35 Christian Damus 2015-10-28 14:32:46 EDT

(In reply to Ansgar Radermacher from comment #34)

GMF (and, by extension, Papyrus) provides applications with two distinct validation mechanisms:  one built into EMF, which generates validation constraints in the model code, and one in (what I like to think of as) the GMF Run-time, which lets applications add constraints to third-party models.  Personally, I don't think it's a good idea to let these mechanisms overlap.  If Papyrus wants to continue using the extrinsic (GMF) framework, then I recommend that

* all UML metamodel constraints defined by UML2 should be incorporated by simple delegation to the UML2Validator.  Also, this ensures that basic constraints like EStructuralFeature multiplicity implemented by Ecore are taken into account (it sounds like they aren't, now?)

* the UML metamodel constraints of UML2 are what UML specifies and should not be suppressed or replaced.  I don't think they should be presented in the preferences as user-selectable constraints.  If we need these to be customizable, then whatever enhancements are required to make that happen should be done in EMF/UML2 for more general applicability

* Papyrus in any case remains free to add constraints of its own for the UML domain, and DSMLs are free to define their own additional constraints (in profiles or otherwise).  Both frameworks support this, in very different ways

So, I would suggest revisiting the new constraint provider implemented in this bug.  In particular, the problem is that the UML2 implementation of the validation rule for 'uppper_ge_lower' in the spec is too literal:  it doesn't account for the fact that bounds may be arbitrary ValueSpecifications (even expressions).  The spirit of the OCL "upperBound() >= lowerBound()" makes sense, but may not be evaluatable in all cases because the ValueSpecification::integerValue() query is ill-defined:  it gives a "single Integer value when one can be computed".  In the case that it cannot be computed, the MultiplicityElement must not have a value for the corresponding lowerBound/upperBound query.  Defaulting those to 1 in these cases is simply incorrect.

I would rather see the validation problem in this case fixed in UML2 and the validation provider changes in review 46642 reverted.

Comment 36 Ansgar Radermacher 2015-10-29 04:52:30 EDT

https://git.eclipse.org/r/#/c/59186/ removes validation provider and constraint from oep.uml.tools, since they duplicate the constraint evaluation.
Even the "UpperGeLower" constraint got evaluated twice in case of a multiplicity mistake. The purpose of the modification of UpperGeLower is to avoid false warnings in case of value specifications that are not literal integers. In case of the latter, the original UML2 constraint is not invoked. However, the modified constraint is already in the Papyrus EValidatorAdaptor and the explicit constraint is thus not necessary as well (the modified constraint is in oep.infra.services.validation - it has apparently been moved from the OCLEValidatorAdaptor by gerrit 46642. I don't think this move was a good idea, since it adds UML specific methods to the infra plugin, but that's another issue).

Another issue with the existing code: an undefined (null) lower or upper value corresponds to the default value 1. However, the current test skips validation and does therefore for example not detect the case [3..1] (with an undefined upper). This has been corrected in the gerrit above.

(In reply to Christian W. Damus from comment #35)
* all UML metamodel constraints defined by UML2 should be incorporated by simple delegation to the UML2Validator.  Also, this ensures that basic constraints like EStructuralFeature multiplicity implemented by Ecore are taken into account (it sounds like they aren't, now?)

No, they are currently taken into account, since the validate method of UMLValidator takes care of it. But this might not be the case, if we would opt for not calling this method any more.

Comment 37 Ansgar Radermacher 2015-11-09 10:10:46 EST

The removal of the additional constraint provider has been integrated into the build with
commit e638c413ede22d94939ac1528de9f17b5da0e5e7 for master and
commit b2bd8b20a7581e1b5ae6f2a6bad0a1c1fa81b4c9 for 1.1-maintenance.

This provider caused bug 479331, a double evaluation of UML MM constraints.

Apart from this validation issue, the original fault is no longer reproducible, e.g. if I enter 1..2 either with an xtext editor or the property view, the multiplicity is correctly assigned and shown in the property view.
The additional request to support arbitrary string constants for lower and upper (e.g. MIN..MAX) is a quite different issue and should be treated in a new bug report. Thus, I propose to create a new bug for non-integer literals and close this bug.

(In reply to Ansgar Radermacher from comment #36)
> https://git.eclipse.org/r/#/c/59186/ removes validation provider and
> constraint from oep.uml.tools, since they duplicate the constraint
> evaluation.

Comment 38 Ansgar Radermacher 2015-11-09 14:34:12 EST

(In reply to Ansgar Radermacher from comment #37)
: [...]
> The additional request to support arbitrary string constants for lower and
> upper (e.g. MIN..MAX) is a quite different issue and should be treated in a
> new bug report. Thus, I propose to create a new bug for non-integer literals
> and close this bug.

I should have read the original report more thoroughly. It does refer to non-integer literals (I was confused by the attached screenshot that showed integer-only issues).

In addition, I just discovered the following wrong behavior (slightly) within the context of bug (only on neon): the first multiplicity change from 1 to another value (e.g. 0..1) results in "0" in the property view display. A refresh of this view shows the right value, i.e. this is an update issue.

The error can be reproduced by resetting the lower/upper values to the default (=undefined) via the Advanced tab.