OntoUML / ontouml-vp-plugin

A plugin for Visual Paradigm to add support for OntoUML modeling and model intelligence services
Apache License 2.0
38 stars 15 forks source link

Language Updates #64

Closed tgoprince closed 4 years ago

tgoprince commented 4 years ago

This PR introduces a new development version of the plugin, with minor improvements.

Bugs fixed

New features:

Changes:

Observations:

claudenirmf commented 4 years ago

Does it make sense to enable isExtensional to be changed for any class whose restriction is collective?

I believe that once a collective class is extensional, this must be the case for subclasses as well. However, it is weird to consider the other way around.

claudenirmf commented 4 years ago

I am happy with this PR, just give me your feedback on the comments above so I can generate a new release.

tgoprince commented 4 years ago

Does it make sense to enable isExtensional to be changed for any class whose restriction is collective?

It definitely does. I changed the condition on the contextual menu to enabled it in such cases. However, it doesn't work because other stereotypes don't have the isExtensional meta-property. Could you fix that @claudenirmf?

I believe that once a collective class is extensional, this must be the case for subclasses as well. However, it is weird to consider the other way around.

I'm not sure. Depends on how we interpret isExtensional when it is false...

tgoprince commented 4 years ago

Double-check constraints

Are you asking me to double-check the constraints or are you telling me you did it? :)

claudenirmf commented 4 years ago

Double-check constraints

Are you asking me to double-check the constraints or are you telling me you did it? :)

I was double-checking the constraints and GitHub required some message to submit my comments. Sorry for the bad naming, no need for you to do anything.

claudenirmf commented 4 years ago

Does it make sense to enable isExtensional to be changed for any class whose restriction is collective?

It definitely does. I changed the condition on the contextual menu to enabled it in such cases. However, it doesn't work because other stereotypes don't have the isExtensional meta-property. Could you fix that @claudenirmf?

Working on it.

I believe that once a collective class is extensional, this must be the case for subclasses as well. However, it is weird to consider the other way around.

I'm not sure. Depends on how we interpret isExtensional when it is false...

Let's think about this constraint, but we may need an enumeration instead of a boolean here to account for three possible value (or we just use restrictedTo again)... maybe even for powertypes... 🤔

claudenirmf commented 4 years ago

I have updated the code so any stereotypes whose value of restrictedTo may be collective have the isExtensional tagged value. The value of isExtensional may be set to true iff collective is the unique value of restrictedTo.

Now updates to restrictedTo trigger a verification of isExtensional, which is set to false in case the class doesn't apply to collectives only. Also, manual changes to the tagged value are prohibited while smart modeling is enabled.

This update accommodates old projects that lacked this tagged value in their stereotypes.

claudenirmf commented 4 years ago

Btw, are we using «characterization» between intrinsic moments and events? I understand if they could be used as read-only but wanted to check with you.

tgoprince commented 4 years ago

Btw, are we using «characterization» between intrinsic moments and events? I understand if they could be used as read-only but wanted to check with you.

Yes, this is allowed by the theory.

See the paper below by Giancarlo and João. In pager 4, a Trope inheres in an Individual, which can be an Event, a Situation or an Endurant.

https://www.researchgate.net/publication/336848806_Events_as_Entities_in_Ontology-Driven_Conceptual_Modeling

Also see the object property gufo:inheresIn defined in gUFO:

http://purl.org/nemo/gufo

tgoprince commented 4 years ago

I have updated the code so any stereotypes whose value of restrictedTo may be collective have the isExtensional tagged value. The value of isExtensional may be set to true iff collective is the unique value of restrictedTo.

Now updates to restrictedTo trigger a verification of isExtensional, which is set to false in case the class doesn't apply to collectives only. Also, manual changes to the tagged value are prohibited while smart modeling is enabled.

This update accommodates old projects that lacked this tagged value in their stereotypes.

Great solution!

tgoprince commented 4 years ago

I just noticed 2 things:

tgoprince commented 4 years ago

I fixed the aforementioned issues in my last commit (https://github.com/OntoUML/ontouml-vp-plugin/pull/64/commits/e18f72863e892255d18bb896b71a18bdfccde557).

I think we are ready to merge now.

tgoprince commented 4 years ago

Please remember that we have to update the syntax verification on ontouml-js to accept situations and abstracts.

claudenirmf commented 4 years ago

What do you think about creating a script to use GitHub CLI and automate release generation? We could run something like mvn release -DpathToReleaseFile=<path> and create a release with default title and body. I would only need you to create the Maven part and put the plugin version as an environment variable at ./src/main/resources/.properties (see this).

I believe I can write the shell script that makes the actual release and patches the version. This would be mainly for pre-releases of pull requests.

claudenirmf commented 4 years ago

I noticed that we have different code styles between Eclipse and IntelliJ. Let's use Google's style instead. I have installed it on Eclipse and enabled auto-formatting on save (see this), you should be able to do the same on IntelliJ.

claudenirmf commented 4 years ago

@tgoprince, could you add the formatter to Maven as shows this tutorial?

I have integrated it to my installation of Eclipse but, beyond this being limited to my computer, it has some annoying side-effects on Eclipse.

tgoprince commented 4 years ago

Just to register our decision.

  1. I implemented a Maven plugin that automatically formats the code according to Google's Java Format Guidelines upon compiling the code (see https://github.com/OntoUML/ontouml-vp-plugin/pull/64/commits/1c82bbb7ec42a16beda5469cc98d0464d89f1c53)
  2. I added a Github action that automatically formats the code, again according to Google's Java Format Guidelines, upon any push to the repository (see https://github.com/OntoUML/ontouml-vp-plugin/pull/64/commits/4864ecdd60a44b84c9d1326149096f5c2bfb5497)

1 is to assist developers in getting the final style while coding 2 is to make sure no unstyled code is committed to the repository

Notice that by pushing commit 2 to Github, I triggered an automatic restyling of all our plugin code (see https://github.com/OntoUML/ontouml-vp-plugin/pull/64/commits/b1e744e5f4debbe50e93c2abf492aa6a02de7eea)

claudenirmf commented 4 years ago

After struggling with some issues when deleting associations from diagrams, I have implemented the following strategy for inverting associations (from the example below):

Screen Shot 2020-09-27 at 22 04 07

If you invert an association by making a change for a suggested stereotype, the association ends are kept.

Screen Shot 2020-09-27 at 22 04 26

Screen Shot 2020-09-27 at 22 04 42

If you invert the association through the "Reverse Association" action on the menu, everything is inverted.

Screen Shot 2020-09-27 at 22 05 08

Screen Shot 2020-09-27 at 22 05 30

The code is ugly because I tried to be safe at each step. Please have a look at Association.invertAssociation().

claudenirmf commented 4 years ago

Now, SmartModelingController is responsible for keeping the consistency on navigability as well as multiplicity, read-only properties, and aggregation kind. The code now prevents navigability from appearing alongside aggregation kind in the same end (when involving ontouml classes on both sides).

However, SmartModeling runs only when the user changes the stereotype through the context menu. The user is still able to "mess up the navigability" manually through other menus. To prevent this we need to use model listeners as well.

claudenirmf commented 4 years ago

There is a known bug in the code, but it is likely to be caused by Visual Paradigm itself and it doesn't seem to break anything.

When hovering the meta-properties menu of an association's source or target, the following exception is thrown if any of the properties are set (i.e., isReadOnly, isDerived, or isOrdered):

java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location

I believe we can tolerate this exception.

tgoprince commented 4 years ago

Now, SmartModelingController is responsible for keeping the consistency on navigability as well as multiplicity, read-only properties, and aggregation kind. The code now prevents navigability from appearing alongside aggregation kind in the same end (when involving ontouml classes on both sides).

However, SmartModeling runs only when the user changes the stereotype through the context menu. The user is still able to "mess up the navigability" manually through other menus. To prevent this we need to use model listeners as well.

Sounds like a good feature to consider in the future. I suggest we create an issue for it and merge this PR as it is.

tgoprince commented 4 years ago

There is a known bug in the code, but it is likely to be caused by Visual Paradigm itself and it doesn't seem to break anything.

When hovering the meta-properties menu of an association's source or target, the following exception is thrown if any of the properties are set (i.e., isReadOnly, isDerived, or isOrdered):

java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location

I believe we can tolerate this exception.

Were you able to track down which of our methods throws this exception? And if there is any consequence in terms of additional memory or cpu consumption?

claudenirmf commented 4 years ago

Sounds like a good feature to consider in the future. I suggest we create an issue for it and merge this PR as it is.

I have created the issue

Were you able to track down which of our methods throws this exception? And if there is any consequence in terms of additional memory or cpu consumption?

It happens whenever we hover on these "internal" context menus for the source and target ends meta-properties, regardless with we anything or not. My bet is on some buggy listener on the tool.

I have seen no impact on processor time or memory allocation.

claudenirmf commented 4 years ago

I believe that whenever we invert associations we should keep the association ends where they are, along with their properties (including multiplicity). One important example is when someone defines a parthood relation by hand in the wrong direction and uses the suggested stereotypes menu to set the stereotype:

  1. Before applying stereotype: Screen Shot 2020-10-01 at 02 10 47

  2. After applying stereotype: Screen Shot 2020-10-01 at 02 11 44

Notice that it is interesting to keep even the multiplicity in place as it carries some information the user made an effort to put there. Keeping the multiplicity in place may be an issue when replacing a «mediation» for a «characterization», for example, but we already have the policy of not overriding user-defined multiplicities.

The main thing we may change from this example is keeping the original reading direction because it must make sense with the name of the association. This will not be in conflict with the real direction of the association, as this is identified by the navigability or the aggregation symbols.

Given everything I said, I am considering removing the dialog warning for inverting associations and the menu button for inverting associations without changing stereotypes.