Upgrade Frameworks and Improve Tools and Toolbox in Diagrams

CrossBreezeNL / crossmodel

An open-source logical data modeler to support the model driven data engineering approach.

GNU Affero General Public License v3.0

8 stars 3 forks source link

Upgrade Frameworks and Improve Tools and Toolbox in Diagrams #60

Closed martin-fleck-at closed 2 months ago

martin-fleck-at commented 4 months ago

Upgrade Frameworks:

Update to Theia 1.49.1 - the latest community release
Update to GLSP 2.2.0 - the latest GLSP version
Update to Node 20 - only affects CI workflows

Improve Tools and Toolbox in Diagrams:

Wrap tool functionality up into their own tool modules
Define default tool as select tool
Create new tool for creating an entity in the system diagram
Ensure newly created files are put into the correct sub-folder
Render toolbox to explicitly list tools and give toolbox better style

github-actions[bot] commented 4 months ago

Unit Test Results

3 files ±0 30 suites ±0 2m 13s :stopwatch: -46s 71 tests ±0 71 :white_check_mark: ±0 0 :zzz: ±0 0 :x: ±0 216 runs ±0 216 :white_check_mark: ±0 0 :zzz: ±0 0 :x: ±0

Results for commit 5a3a73f8. ± Comparison against base commit 353fc84a.

:recycle: This comment has been updated with latest results.

martin-fleck-at commented 3 months ago

@harmen-xb I have no idea why the Windows build keeps failing. Could you please check if that also happens locally on your Windows machine?

harmen-xb commented 3 months ago

@martin-fleck-at I noticed there was an error in de Windows build output about some missing node module files.

I checked on my laptop and I had the same issue in Windows (which I usually don't use since I usually work in a dev container).

Anyway, because of the error message I thought there was something off with with yarn prepare, built and test command for the e2e-test package. So I aligned it more with how it was setup within Theia and that seems to have solved it.

It doesn't actually execute playwright in Windows, but it never did before anyway.

I did have some issues with the GitHub workflow executing successfully. It failed on some very unrelated things. After 3 executions it now does succeed. So a bit strange, but it looks like it's ok now.

martin-fleck-at commented 3 months ago

@harmen-xb Great, thank you very much for looking into that! If the Windows CI is happy, so am I ;-) Please feel free to have a look at the PR once you some time.

harmen-xb commented 3 months ago

@martin-fleck-at

I tested everything after the update and everything seems very responsive and fast. Startup is also noticable faster.

Some issues I found:

When making a relationship using a system diagram the ID of the edge becomes the same as the ID of the relationship itself. This lways leads to a validation error. I think the ID of the relationship should be suffixed with Edge or something.
In the mapping diagram when adding a source object the tool should be blocked when hovering over existing nodes. You already implemented this in the system diagram for new entities (you see a forbidden sign when hovering over existing entities).
In the mapping diagram when creating a source object the dropdown with entities is openened in the left top corner. We think it's better to open it at the position of the mouse cursor. I know the position of the node will dynamically be defined, but for the user it makes more sense if the dropdown is at the mouse cursor location.

martin-fleck-at commented 2 months ago

@harmen-xb Great, thank you for your feedback! I merged the current master into the PR and fixed all the issues that you mentioned if you want to have another look.

harmen-xb commented 2 months ago

@martin-fleck-at

Thanks for the update. The things I noticed last time are indeed all working correct now.

I do see something I didn't notice the previous time (I think it happened after the merge from main). When I now open the CRM source system diagram I don't see the attributes of the Address entity. When I then resize the node I do see them, but the customProperties which are defined on the node are now removed. So I guess something goes wrong there when updating the diagram from the diagram editor versus the text editor.

martin-fleck-at commented 2 months ago

@harmen-xb Good catch! I'll have a look at this later today and hopefully it is just a minor issue that happened during the merge.

martin-fleck-at commented 2 months ago

@harmen-xb There was indeed a problem with the serialization as we already use 'name' and 'value' in other contexts. And the document for the 'Address' node was not correctly identified. I pushed some commits that should fix those problems. Thank you for catching that!

harmen-xb commented 2 months ago

@martin-fleck-at

The scenario I described works good now. I did find another scenario which isn't working.

If you change an attribute on which customProperties are defined via de property widget, the customProperties are also removed.

Take for example the ExampleCRM.Address entity. If you change the datatype or identifier property of the CountryCode attribute, the customProperties are gone.

martin-fleck-at commented 2 months ago

@harmen-xb Another great catch, thank you! Indeed, we manually selected the fields to update which did not include the newly created customProperties. I changed it in a way that we hopefully no longer have to manually specify the fields. I also notices that some of our examples were using datatype "Text" which was not part of the form dropdown. I added it so that we get a nicer UX.