Skip to content
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

Add entity inheritances in the diagram representation #90

Merged
merged 6 commits into from
Feb 4, 2025

Conversation

tortmayr
Copy link
Collaborator

@tortmayr tortmayr commented Jan 8, 2025

  • Render entity inheritances as entity edges
  • Extend edge creation tool to enable creation of new inheritances
  • Add inheritance creation tool to tool box
  • Add operation/command handlers on server side for inheritance creation & deletion

Also:

  • Update tsconfig of vscode extension to fix ts errors related to inversify property injection
  • Fix wrong entity initialization in create operation handler

@tortmayr tortmayr force-pushed the graphical-inheritance branch from 2209f4c to ee4b989 Compare January 24, 2025 11:08
@tortmayr tortmayr marked this pull request as ready for review January 24, 2025 11:09
@harmen-xb
Copy link
Contributor

harmen-xb commented Jan 24, 2025

@martin-fleck-at @tortmayr

Thanks for the update!

I updated the inheritance system diagram to include the existing inheritances (using the 'Create inheritance' tool and that worked good 👍 ).

I tested the workings and I have some findings:

  • When I create an inheritance in the system diagram between the same two nodes twice, it results in 2 inheritance edges representing the same inheritance. This should not be possible, inheritance edge can only exist once between 2 nodes.
  • When I create an inheritance in the opposite direction of an existing inheritance, I am allowed to do it in the system diagram. So in the example there is already an inheritance from DigitalProduct to Product. When I then drag from Product to DigitalProduct it actually creates the edge and updates the Product entity. I think we should block this in the diagram by showing this is not possible.
  • When you open the ProductInheritance system diagram in the ExampleDWH model (in the mapping-example workspace) and create an inheritance from SomeProduct to Product it doesn't seem to do anything. The other way around works. I first didn't understand why, but now I see SomeProduct resides in another model. So it tries to add an inheritance to SomeProduct referencing a model which is not in the dependencies list of that model yet. In this case ExampleDWH should be added to the list of dependencies of ExampleOtherModel. This is generic behaviour which also goes wrong when for example creating a diagram in the ExampleOtherModel model and dragging the ExampleDWH.Product entity on the new diagram. This leads to an unresolved node, because the dependency is missing.

@martin-fleck-at
Copy link
Collaborator

  • When I create an inheritance in the opposite direction of an existing inheritance, I am allowed to do it in the system diagram. So in the example there is already an inheritance from DigitalProduct to Product. When I then drag from Product to DigitalProduct it actually creates the edge and updates the Product entity. I think we should block this in the diagram by showing this is not possible.

I think we discussed this at some point and argued that we do not want to restrict the user in advance but much rather indicate an error through validation. In this example, the user would create an intermediate erroneous state that they need to fix afterwards. If this were not be possible, the user would need to delete the other inheritance first before creating the new one. What do you think? If we do want to restrict the user, we need to figure out a good way to indicate to the user why something is not allowed because simply a "forbidden" cursor might not be enough information, especially if cycles can be very complex or not all nodes in the cycle are present in the diagram.

@harmen-xb
Copy link
Contributor

  • When I create an inheritance in the opposite direction of an existing inheritance, I am allowed to do it in the system diagram. So in the example there is already an inheritance from DigitalProduct to Product. When I then drag from Product to DigitalProduct it actually creates the edge and updates the Product entity. I think we should block this in the diagram by showing this is not possible.

I think we discussed this at some point and argued that we do not want to restrict the user in advance but much rather indicate an error through validation. In this example, the user would create an intermediate erroneous state that they need to fix afterwards. If this were not be possible, the user would need to delete the other inheritance first before creating the new one. What do you think? If we do want to restrict the user, we need to figure out a good way to indicate to the user why something is not allowed because simply a "forbidden" cursor might not be enough information, especially if cycles can be very complex or not all nodes in the cycle are present in the diagram.

Yes I remember. At that point I was thinking about indirect cycles, so via another node. But this is a direct cycle. Because the edges are overlapping you can't select 1 and remove it.

So either we need to block it or the routing algorithm should change a bit so the lines are not overlapping.

Maybe the routing algorithm change would be best indeed.

@tortmayr tortmayr force-pushed the graphical-inheritance branch from 0c6287a to ee4b989 Compare January 29, 2025 09:54
- Render entity inheritances as entity edges
- Extend edge creation tool to enable creation of new inheritances
- Add inheritance creation tool to tool box
- Add operation/command handlers on server side for inheritance creation & deletion

Also:
- Update tsconfig of vscode extension to fix ts errors related to inversify property injection
- Fix wrong entity initialization in create operation handler

Co-authored-by: Harmen Wessels <[email protected]>
@tortmayr tortmayr force-pushed the graphical-inheritance branch from ee4b989 to 16b5fa0 Compare January 29, 2025 13:40
@tortmayr
Copy link
Collaborator Author

@harmen-xb Thanks for the detailed review. I have updated the PR as follows:

When I create an inheritance in the system diagram between the same two nodes twice, it results in 2 inheritance edges representing the same inheritance. This should not be possible, inheritance edge can only exist once between 2 nodes.

This is now fixed. A check is in place that prevents the creation of an additional inheritance edge if there is already an edge between the two nodes representing the same inheritance.

Yes I remember. At that point I was thinking about indirect cycles, so via another node. But this is a direct cycle. Because the edges are overlapping you can't select 1 and remove it.

So either we need to block it or the routing algorithm should change a bit so the lines are not overlapping.

Maybe the routing algorithm change would be best indeed.

Unfortunately a routing algorithm change alone would not be enough. The diagram becomes readonly if there is a validation error. So even if we fix the routing so that the edges don't overlap the error cannot be resolved in the diagram (readonly) and could only be fixed in the textual representation.
Therefore, I opted for blocking the operation if it would result in a direct inheritance cycle. In addition, a warning message is displayed to the user.

When you open the ProductInheritance system diagram in the ExampleDWH model (in the mapping-example workspace) and create an inheritance from SomeProduct to Product it doesn't seem to do anything. The other way around works. I first didn't understand why, but now I see SomeProduct resides in another model. So it tries to add an inheritance to SomeProduct referencing a model which is not in the dependencies list of that model yet. In this case ExampleDWH should be added to the list of dependencies of ExampleOtherModel. This is generic behaviour which also goes wrong when for example creating a diagram in the ExampleOtherModel model and dragging the ExampleDWH.Product entity on the new diagram. This leads to an unresolved node, because the dependency is missing.

I would suggest to tackle this issue in a follow-up task. As you already mentioned this more of a generic issue that also affects other aspects of diagram editing (like entities). Identifying a missing dependency and the corresponding model that needs to be added is not a trivial task. We should come up with a clean generic solution that works for all cases.
Another thing that we have to consider here are cyclic dependencies.
For instance, looking at the mapping example:
The ExampleDWH model already has a dependency to ExampleOtherModel. By adding an inheritance from SomeProduct to Product we would also need a dependency for ExampleOtherModel to ExampleDWH.

@harmen-xb
Copy link
Contributor

harmen-xb commented Jan 30, 2025

Hi @tortmayr ,

Thanks for the update!

For both not allowing to create an inheritance edge twice and not allowing a direct cyclic inheritance, can we use show it visually in the diagram by showing the stop-sign icon when we or hovering on a target which is not allowed?

I think this is more user friendly, than not showing anything (when creating the inheritance twice) or showing a warning afterwards.

This behaviour should be equal to how it's implemented in the mapping diagram when using the 'Create Mapping' tool. The stop-sign icon disappears when we are on a target where it is allowed to let go.

image

@harmen-xb
Copy link
Contributor

@tortmayr I just had a chat with @martin-fleck-at and the inheritance action where you create an inheritance on a remote entity in the diagram is the only action we have where we need to added the dependencies list.

For now I would rather block the action where the child side of the inheritance is a remote entity. It should block just like with creating the same inheritance twice.

- Implement `SystemEdgeCreationChecker` on server-side that disallows creation of inheritance edges if
 - a similar directed inheritance already exists
 - the new inheritance would result in a direct circular inheritance
 - the inheritance would be create to an external model element that is not a dependency of the source model

 -Refactor and improve `SystemEdgeCreationTool`
     -  Add support for dynamic server-side edge checking
     -  Greatly improve performance overhead by ensuring that we only update the feedback and invoke the `this.canConnect` method if the current connection context actually changes
@tortmayr
Copy link
Collaborator Author

tortmayr commented Feb 4, 2025

@harmen-xb I have updated the PR as discussed and added visual validation to the SystemEdgeCreationTool.
The following cases are now disallowed

  • Create an inheritance edge if there is already an existing edge representing the same inheritance
  • Creation of inheritance edges that would introduce a direct cycle
  • Creation of inheritance edges where the child side of the inheritance is and unresolved remote entity
demo.mp4

I also discovered a significant performance issue with the SystemEdgeCreationTool. Previously during the creation process the tool did continously send feedback update actions and checked if the connection is allowed whenever the mouse was moved. This resulted in a lot of unnecessary overhead as we only need to do these checks and feedback updates when the potential connection candidate changes (i.e. whenever the element under the mouse changes).
This is now fixed but required a refactoring of the SystemEdgeCreationTool. Could you please look out for any potential regression issues during testing i.e. make sure that the tool still behaves as expected.

@harmen-xb harmen-xb closed this Feb 4, 2025
@harmen-xb harmen-xb deleted the graphical-inheritance branch February 4, 2025 11:06
@harmen-xb harmen-xb restored the graphical-inheritance branch February 4, 2025 11:15
@harmen-xb harmen-xb reopened this Feb 4, 2025
@harmen-xb
Copy link
Contributor

@tortmayr

Thanks for the update! The behavior of the inheritance tool looks good now. I tested a bit and I don't see strange things in the UI. Did you implement the optimization genericly for all edge tools? So also for the create mapping tool in the mapping diagram?

Also, if you create branches could you prefix them with 'feature/' cause the the CI/CD pipeline is automatically ran at every commit. I noticed renaming doesn't work well in GitHub, but a unit test is failing: https://github.com/CrossBreezeNL/crossmodel/actions/runs/13134077562/job/36645443260

Copy link

github-actions bot commented Feb 4, 2025

Unit Test Results

  3 files  ±0   27 suites  ±0   2m 23s ⏱️ -25s
 42 tests +1   42 ✅ +1  0 💤 ±0  0 ❌ ±0 
126 runs  +3  126 ✅ +3  0 💤 ±0  0 ❌ ±0 

Results for commit 83fb215. ± Comparison against base commit 3c642f0.

♻️ This comment has been updated with latest results.

- Fix diagram unit tests for relationship edges
- Add a test for inheritance edges
@tortmayr tortmayr closed this Feb 4, 2025
@tortmayr tortmayr reopened this Feb 4, 2025
@tortmayr tortmayr closed this Feb 4, 2025
@tortmayr tortmayr deleted the graphical-inheritance branch February 4, 2025 18:04
@tortmayr tortmayr restored the graphical-inheritance branch February 4, 2025 18:04
@tortmayr tortmayr reopened this Feb 4, 2025
@tortmayr
Copy link
Collaborator Author

tortmayr commented Feb 4, 2025

Thanks for the update! The behavior of the inheritance tool looks good now. I tested a bit and I don't see strange things in the UI. Did you implement the optimization genericly for all edge tools? So also for the create mapping tool in the mapping diagram?

No, I only optimized the system edge creation tool. For the create mapping tool this is not necessary. The difference is that the system edge creation tool is a complete custom tool, while the create mapping tool extends the default GLSP EdgeCreationTool (which already has this performance tweaks in place).

Also, if you create branches could you prefix them with 'feature/' cause the the CI/CD pipeline is automatically ran at every commit. I noticed renaming doesn't work well in GitHub, but a unit test is failing:

I was not aware of the naming convention. Thank you, I will keep it in mind for future PRs.
I pushed an update that should fix the failing unit test and also adds an additional test for the diagram inheritance edges.

Updated PlayWright cm-app to use relative path from script file, i.s.o. relative path from working directory.
Updated PlayWright cm-app to use relative path from script file, i.s.o. relative path from working directory.
@harmen-xb harmen-xb added the enhancement New feature or request label Feb 4, 2025
Copy link
Contributor

@harmen-xb harmen-xb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good to me.

@harmen-xb harmen-xb merged commit c926afa into main Feb 4, 2025
5 checks passed
@harmen-xb harmen-xb deleted the graphical-inheritance branch February 4, 2025 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants