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.

Sign up for GitHub

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

Jump to bottom

Wrong "TAIL" edge label layout #905

Closed
lredor opened this issue Dec 23, 2022 · 9 comments
Closed

Wrong "TAIL" edge label layout #905

lredor opened this issue Dec 23, 2022 · 9 comments
Milestone
Release 0.9.0

Comments

@lredor
Copy link
Contributor

lredor commented Dec 23, 2022

It seems that the location of an edge label with property "edgeLabels.placement: TAIL" is non deterministic between several layouts.
Here is an example, each diagram is the result of the previous layout, you can see that the "begin" label move at each layout. The property "noLayout" is set to true to see the state before layout:

The corresponding successive layout results are like this:
successiveLayoutResults
@lredor
Copy link
Contributor Author

lredor commented Dec 23, 2022

And for information, the problem does not exist without container and without border nodes: sample in ELK live.

@lredor
Copy link
Contributor Author

lredor commented Dec 23, 2022

I looked a bit at the code level to see if there was anything "obvious" between a different treatment for org.eclipse.elk.core.options.EdgeLabelPlacement.TAIL and for org.eclipse.elk.core. options.EdgeLabelPlacement.HEAD but I saw nothing.

@lredor
Copy link
Contributor Author

lredor commented Dec 23, 2022 •
edited

Not sure about all consequences, but this change fixes the problem:

diff --git a/plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/intermediate/EndLabelPostprocessor.java b/plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/intermediate/EndLabelPostprocessor.java
index d6f7826..ad6d48b 100644
--- a/plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/intermediate/EndLabelPostprocessor.java
+++ b/plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/intermediate/EndLabelPostprocessor.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2012, 2020 Kiel University and others.
+ * Copyright (c) 2012, 2022 Kiel University and others.
  * 
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License 2.0 which is available at
@@ -51,7 +51,7 @@
         // We iterate over each node's label cells and offset and place them
         layeredGraph.getLayers().stream()
                 .flatMap(layer -> layer.getNodes().stream())
-                .filter(node -> node.getType() == NodeType.NORMAL && node.hasProperty(InternalProperties.END_LABELS))
+                .filter(node -> ((node.getType() == NodeType.NORMAL || node.getType() == NodeType.EXTERNAL_PORT) && node.hasProperty(InternalProperties.END_LABELS)))
                 .forEach(node -> processNode(node));
         
         monitor.done();

lredor added a commit to lredor/elk that referenced this issue Dec 23, 2022
@lredor
[DRAFT] Wrong tail edge label (eclipse#905)
b26fe00 
Signed-off-by: Laurent Redor <laurent.redor@obeo.fr>
@lredor
Copy link
Contributor Author

lredor commented Dec 23, 2022

I've done a corresponding pull request #906

soerendomroes pushed a commit that referenced this issue Mar 9, 2023
@lredor
[DRAFT] Wrong tail edge label (#905) (#906)
b85ad1c 
Signed-off-by: Laurent Redor <laurent.redor@obeo.fr>
@lredor
Copy link
Contributor Author

lredor commented Mar 9, 2023

As the corresponding commit has been merged, I think that this issue can be closed, with milestone 0.9.0, except if some additional tests are planned to be done.

@soerendomroes
Copy link
Contributor

A test would be nice. Thank you for your effort @lredor

lredor added a commit to lredor/elk that referenced this issue Mar 9, 2023
@lredor
Layered: Add tests for eclipse#905
a996430 
Signed-off-by: Laurent Redor <laurent.redor@obeo.fr>
@lredor
Copy link
Contributor Author

lredor commented Mar 9, 2023

@soerendomroes I have added a test with #913 and associated data in eclipse/elk-models#17.

@lredor
Copy link
Contributor Author

lredor commented Mar 9, 2023

The test also covers center and head labels. Even if only tail label is problematic in #905. But it could help to detect a possible regression later...

soerendomroes pushed a commit that referenced this issue Mar 15, 2023
@lredor
Layered: Add tests for #905 (#913)
2eb73d4 
Signed-off-by: Laurent Redor <laurent.redor@obeo.fr>
@lredor
Copy link
Contributor Author

lredor commented Mar 15, 2023

The fix and tests have been merged on master branch so I consider this issue as solved. I think it can be added to 0.9.0 milestone

@lredor lredor closed this as completed Mar 15, 2023
@soerendomroes soerendomroes added this to the Release 0.9.0 milestone Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 0.9.0
Development

No branches or pull requests
2 participants
@lredor @soerendomroes