-
Notifications
You must be signed in to change notification settings - Fork 0
#650 Add stereotype for occurrence constraints #675
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#650 Add stereotype for occurrence constraints #675
Conversation
PokaYokeProfileServices: add methods for occurrence constraint stereotype. PokaYokeUmlProfileUtil: add methods for occurrence constraint stereotype.
| } | ||
|
|
||
| Verify.verify(interval instanceof Interval, | ||
| String.format("Interval constraint '%s' must have an interval specification.", constraint.getName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if an exception is thrown in the UI when editing? Should it be robust in the service class, catching the exception and returning an empty string or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if an exception is thrown in the UI when editing?
An exception is thrown in the console, in the UI nothing happens.
Should it be robust in the service class, catching the exception and returning an empty string or so?
As far as I can tell, the service class only handles errors in setMinValue and setMaxValue with a try/catch. The rest of error handling is in this class. I am assuming that this error handling comment could be extended to several other methods in this class; perhaps it is worth to have a look for it all over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so it won't give error popups then. Then it is probably good enough for now. Although, I hop it won't leave the model in an unrepairable inconsistent/invalid state? Maybe we should create a new industrialization issue for this then, to fix that in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #678.
| public static void setIntervalConstraintMax(IntervalConstraint constraint, String maxValue) { | ||
| applyStereotype(constraint, getPokaYokeProfile(constraint).getOwnedStereotype(ST_OCCURRENCE)); | ||
|
|
||
| if (Strings.isNullOrEmpty(maxValue) && getIntervalConstraintMin(constraint).isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the constrained elements are not under control of the stereotype? It seems they are not on this new tab? Where are they specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct: the constrained elements still need to be specified in the 'Advanced' tab. I tried to create some selection widget to be included in the SynthML tab, but I could not get something decent to work, I am not too familiar with Sirius and related stuff. If you think this would be nice to have, I can spend some more time on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, let's not do that the. Could always do it later. This solves the most immediate need. Let's not close the issue then, move it to industrialization afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment in #650, see #650 (comment).
| applyStereotype(constraint, getPokaYokeProfile(constraint).getOwnedStereotype(ST_OCCURRENCE)); | ||
|
|
||
| if (Strings.isNullOrEmpty(maxValue) && getIntervalConstraintMin(constraint).isEmpty()) { | ||
| PokaYokeUmlProfileUtil.unapplyStereotype(constraint, OCCURRENCE_STEREOTYPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the stereotype is unapplied, it will next potentially create a new interval, and set the interval. That is not desired, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the stereotype is unapplied, there two options:
- the constraint already had a child
Intervalspecification, at which point the child remains but without any max/min bounds. The validator will pick up that this is not a valid model. I thought that if a user unsets both max and min, the user should also remove theIntervalConstraintas well, at which point the childintervalis removed as well. - the constraint does not have an
intervalchild, at which point a newIntervalis created but never assigned, so it never gets saved into the UML model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would then not modify the model, but only because the parsing of "" as int would throw an exception. I guess it works.
| : (Interval)constraint.getSpecification(); | ||
|
|
||
| // If the given string is empty, remove the interval's maximum. | ||
| if (maxValue.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above, it can also be null and then it would crash here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxValue can be an empty string, can never be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then why above do you use Strings.isNullOrEmpty(maxValue)? It seem inconsistent?
|
|
||
| // If the given string is empty, remove the interval's maximum. | ||
| if (maxValue.isEmpty()) { | ||
| if (interval != null && interval.getMax() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can interval be null, as we check that above and create a new one if it is null?
| return; | ||
| } | ||
|
|
||
| setIntervalMax(constraint, interval, Integer.parseInt(maxValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this crash the UI if it is not an integer or a too large integer, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, an exception in the console, nothing happens in the UI (for both cases). Will be picked up by the validator later on.
| return occurrenceConstraints.stream() | ||
| .filter(o -> o.getSpecification() instanceof Interval interval | ||
| && (Objects.equals(interval.getMax(), lit) || Objects.equals(interval.getMin(), lit))) | ||
| .map(o -> o.getSpecification()).map(Interval.class::cast).collect(Collectors.toSet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd maybe return a list. You go over the model and can never find duplicates, right? Or are these referred instead of contained here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list should be good indeed.
...hml.uml.profile.util/src/com/github/tno/synthml/uml/profile/util/PokaYokeUmlProfileUtil.java
Show resolved
Hide resolved
| return newLiteral; | ||
| } | ||
|
|
||
| private static List<LiteralInteger> getAllLiteralIntegersWithValue(Model model, int value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you only use one, you could make this getFirstLiteralIntegerWithValue, with on the stream findFirst, which is more efficient.
| LiteralInteger maxLiteral = getLiteralInteger(constraint, maxValue); | ||
|
|
||
| // Remove the old literal integer, if possible. | ||
| LiteralInteger currentIntegerBound = (LiteralInteger)interval.getMax(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this crash in the UI if it is not a LiteralInteger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous replies.
- move max/min methods close - `findAllIntervalsReferencing` returns a List - remove useless `interval != null` from `setIntervalConstraintMax` and `setIntervalConstraintMin`
`getFirstLiteralIntegersWithValue` and update return value accordingly
| : (Interval)constraint.getSpecification(); | ||
|
|
||
| // If the given string is empty, remove the interval's maximum. | ||
| if (maxValue.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then why above do you use Strings.isNullOrEmpty(maxValue)? It seem inconsistent?
| public static void setIntervalConstraintMax(IntervalConstraint constraint, String maxValue) { | ||
| applyStereotype(constraint, getPokaYokeProfile(constraint).getOwnedStereotype(ST_OCCURRENCE)); | ||
|
|
||
| if (Strings.isNullOrEmpty(maxValue) && getIntervalConstraintMin(constraint).isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, let's not do that the. Could always do it later. This solves the most immediate need. Let's not close the issue then, move it to industrialization afterwards.
| } | ||
|
|
||
| Verify.verify(interval instanceof Interval, | ||
| String.format("Interval constraint '%s' must have an interval specification.", constraint.getName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so it won't give error popups then. Then it is probably good enough for now. Although, I hop it won't leave the model in an unrepairable inconsistent/invalid state? Maybe we should create a new industrialization issue for this then, to fix that in general.
| constraint.setSpecification(interval); | ||
| } | ||
|
|
||
| private static void setIntervalMin(Constraint constraint, Interval interval, int minValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next time, if you move methods around, do that in a separate commit, and make no other changes in that commit. Now I have to see in the move what else you changed at the same time, which takes extra time.
| applyStereotype(constraint, getPokaYokeProfile(constraint).getOwnedStereotype(ST_OCCURRENCE)); | ||
|
|
||
| if (Strings.isNullOrEmpty(maxValue) && getIntervalConstraintMin(constraint).isEmpty()) { | ||
| PokaYokeUmlProfileUtil.unapplyStereotype(constraint, OCCURRENCE_STEREOTYPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would then not modify the model, but only because the parsing of "" as int would throw an exception. I guess it works.
|
Ready for review. |
|
I've updated the PR description to indicate that this addresses (rather than closes) the issue. Still, GitHub will close it, so we should re-open it after merging this PR. |
Addresses #650.
Adds a new SynthML page to
IntervalConstraintsso it is easier to specify occurrence constraints: it automatically creates anIntervalspecification and theLiteralIntegers bounds if necessary.In SynthML it looks like this:

