Bug 509070 - Make ELK layouts work on Sirius diagrams
Summary: Make ELK layouts work on Sirius diagrams
Status: CLOSED FIXED
Alias: None
Product: Sirius
Classification: Modeling
Component: Diagram (show other bugs)
Version: 4.1.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 6.0.0   Edit
Assignee: Florian Barbin CLA
QA Contact: Julien Dupont CLA
URL:
Whiteboard:
Keywords: triaged
Depends on:
Blocks:
 
Reported: 2016-12-12 07:30 EST by Pierre-Charles David CLA
Modified: 2018-06-27 11:55 EDT (History)
7 users (show)

See Also:


Attachments
Ecore sample ELK (4.82 KB, application/zip)
2018-04-23 08:15 EDT, Florian Barbin CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre-Charles David CLA 2016-12-12 07:30:24 EST
See https://polarsys.org/forums/index.php/mv/msg/307/1066/#msg_1066 for some context.

In summary, ELK mostly works "out of the box" on Sirius diagrams, giving much better results that the default "Arrange All" algorithms in many cases. However, because ELK works at the generic GMF level, it has no knowledge of some Sirius-specific constraints. The resulting layout can thus cause problems with the rest of Sirius, which expects these constraints to always hold.

One such point concerns border nodes ("ports"), whose position after an ELK layout do not match what Sirius expect in normal operation. The mismatch then causes some Sirius features to be broken/disabled.

The scope of this ticket is to identify all such issues, discuss and analyse them, and if possible fix them (in Sirius and/or in ELK, depending on the details). This is not about deep integration of ELK in Sirius (which would be nice, but is another kind of work). The goal here is to be able to run ELK algorithms on Sirius diagram as an external tool to get the improved layout, but still get a fully functional Sirius diagram afterwards.
Comment 1 Pierre-Charles David CLA 2016-12-13 08:40:24 EST
One known issue is that after an ELK layout, the "Straighten to" command on edges with ports can not be executed anymore. It seems that ELK shifts the GMF-level positions of the ports in a way that StraightenToCommand.canExecute() does not expect, which makes canExecute() return false for these. The problem is not visible immediatly because even though the GMF-level coordinates are broken (from the point of view of Sirius), the DBorderItemLocator which works at the Draw2D level "fixes" them and displays the bordered nodes at the expected location.
Comment 2 Laurent Redor CLA 2016-12-13 10:53:48 EST
I created a post [1] an ELK forum about port location aspect.

[1] https://www.eclipse.org/forums/index.php/m/1749900/#msg_1749900
Comment 3 Eclipse Genie CLA 2018-02-19 12:32:24 EST
New Gerrit change created: https://git.eclipse.org/r/117685
Comment 4 Eclipse Genie CLA 2018-02-20 09:41:55 EST
New Gerrit change created: https://git.eclipse.org/r/117775
Comment 5 Eclipse Genie CLA 2018-02-22 11:40:54 EST
New Gerrit change created: https://git.eclipse.org/r/117964
Comment 6 Eclipse Genie CLA 2018-02-22 11:40:57 EST
New Gerrit change created: https://git.eclipse.org/r/117963
Comment 12 Eclipse Genie CLA 2018-03-05 05:27:50 EST
New Gerrit change created: https://git.eclipse.org/r/118650
Comment 14 Eclipse Genie CLA 2018-03-06 03:37:20 EST
New Gerrit change created: https://git.eclipse.org/r/118759
Comment 15 Pierre-Charles David CLA 2018-03-09 05:19:27 EST
Note that the bulk of the integration work is already here and will be testable in M6, but this is still an early version with known bugs/limitations and many possible improvements. Work on this will continue after M6.
Comment 16 Eclipse Genie CLA 2018-03-09 11:45:15 EST
New Gerrit change created: https://git.eclipse.org/r/119095
Comment 17 Eclipse Genie CLA 2018-03-13 10:24:23 EDT
New Gerrit change created: https://git.eclipse.org/r/119333
Comment 20 Eclipse Genie CLA 2018-03-27 07:58:16 EDT
New Gerrit change created: https://git.eclipse.org/r/120254
Comment 21 Pierre-Charles David CLA 2018-03-30 03:48:35 EDT
We should probably include the ELK documentation in our update-site, or at least the part of it that it is needed for specifiers to understand the different algorithms provided by ELK and how the configuration parameters impact their behavior. It looks like https://www.eclipse.org/elk/reference.html is a good entry point, but I'm not sure the web site is packaged as an Eclipse Help plug-in. Maybe just a clear pointer to that website is the best we can do.
Comment 22 Christoph Daniel Schulze CLA 2018-03-30 06:54:22 EDT
(In reply to Pierre-Charles David from comment #21)
> It looks like
> https://www.eclipse.org/elk/reference.html is a good entry point, but I'm
> not sure the web site is packaged as an Eclipse Help plug-in. Maybe just a
> clear pointer to that website is the best we can do.

Nope, we do not package it as an Eclipse Help plug-in. It is a simple website generated using Hugo. Feel free to point to the website.

Since release 0.3.0, we publish a ZIP archive that contains the website for each release. Depending on whether you depend on releases or on our nightly build, that might be something to look at?
Comment 24 Eclipse Genie CLA 2018-04-18 07:52:57 EDT
New Gerrit change created: https://git.eclipse.org/r/121331
Comment 26 Florian Barbin CLA 2018-04-23 08:15:22 EDT
Created attachment 273732 [details]
Ecore sample ELK

* Import the given project
* Open the representation
* Perform an arrange all
* The EPackage1 and EPackage2 keep their original size => KO.

The ELK new bounds are overridden later during the arrange all.
Comment 27 Laurent Redor CLA 2018-04-26 03:53:39 EDT
There is a NPE if I launch the above steps to reproduce without the plugin org.eclipse.sirius.diagram.elk activated. The corresponding stack, when launching the Arrange All is:

org.eclipse.core.commands.ExecutionException: While executing the operation, an exception occurred
	at org.eclipse.core.commands.operations.DefaultOperationHistory.execute(DefaultOperationHistory.java:496)
	at org.eclipse.sirius.diagram.ui.tools.internal.editor.DDiagramCommandStack.execute(DDiagramCommandStack.java:71)
...
	at 
org.eclipse.gmf.runtime.diagram.ui.actions.internal.ArrangeAction.doRun(ArrangeAction.java:278)
...
Caused by: java.lang.NullPointerException
	at org.eclipse.sirius.diagram.ui.internal.layout.GenericLayoutProvider.getGenericLayoutProvider(GenericLayoutProvider.java:89)
	at org.eclipse.sirius.diagram.ui.internal.layout.GenericLayoutProvider.provides(GenericLayoutProvider.java:46)
	at org.eclipse.sirius.diagram.ui.tools.internal.layout.provider.LayoutService.getProvider(LayoutService.java:95)
Comment 29 Pierre-Charles David CLA 2018-05-02 10:47:34 EDT
Beside the NPE mentioned above and the related enhancement in bug #533553, there is still work to do on the documentation, notably for the newly introduced extension point.
Comment 30 Eclipse Genie CLA 2018-05-09 09:05:00 EDT
New Gerrit change created: https://git.eclipse.org/r/122353
Comment 31 Pierre-Charles David CLA 2018-05-11 10:20:16 EDT
It's a minor issue, but the description fields of the ELK layout and layout option elements, which can contain significant amount of text, are serialized in the VSM. It would be nice to avoid this. If it requires too much work to be done before 6.0 please open a separate enhancement request to handle this in 6.1.
Comment 32 Eclipse Genie CLA 2018-05-14 11:41:33 EDT
New Gerrit change created: https://git.eclipse.org/r/122596
Comment 35 Pierre-Charles David CLA 2018-05-24 02:44:53 EDT
Closing as the basics of the integration are here, even if it's still experimental. Remaining issues and enhancement requests should be treated by separate tickets.
Comment 36 Florian Barbin CLA 2018-05-24 04:01:21 EDT
Known issue:
With the ecore sample, Their is still an issue. The padding in containers (EPackage) is not enough and the sub node (EClass) can overlap the container label.
Comment 37 Florian Barbin CLA 2018-05-24 04:02:48 EDT
(In reply to Florian Barbin from comment #36)
> Known issue:
> With the ecore sample, Their is still an issue. The padding in containers
> (EPackage) is not enough and the sub node (EClass) can overlap the container
> label.

* there is
Comment 38 Laurent Redor CLA 2018-06-27 11:55:02 EDT
Available in Sirius 6.0.0, see https://wiki.eclipse.org/Sirius/6.0.0 for details