Issue #24 - Use ELK layout to position SVG tasks + links by LudoBroche · Pull Request #30 · ewoks-kit/ewoksdraw

LudoBroche · 2026-08-28T15:22:58Z

PR summary

From the graph/tasks geometry ELK representation, we now used ELK to compute the tasks' optimal positions and link routing. The computed ELK layout is used to position the tasks and draw the links.

AI Disclosure

Claude used to pre-review the PR
Claude used to give advice on the pytest coverage plan.
Claude used to render the svg collage of all the workflow svgs in the PR comment

codecov · 2026-08-28T15:25:11Z

Codecov Report

❌ Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/ewoksdraw/layout/elk_link_group_builder.py	95.45%	1 Missing ⚠️
src/ewoksdraw/svg/svg_task_group.py	88.88%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

LudoBroche · 2026-08-28T16:24:03Z

When applying ELK layout routing computation:

elk_graph = ELK().layout(elk_graph)

The ElkGraph is modified in place, adding metadata about positioning, links, routing ...
In this version of the code, I'm creating two types:

the "uncomputed" graph : ElkGraph.

the "computed" graph: LaidOutElkGraph

To have something explicit like that:

elk_graph: ElkGraph = convert_ewoks_to_elk_graph( ... ) laid_out_graph: LaidOutElkGraph = ELK().layout(elk_graph)

But the definition here is a little verbose.

Another solution is to go for a single type with NotRequired elements:

class ElkChild(TypedDict): x: NotRequired[float] y: NotRequired[float]

elk_graph: ElkGraph = convert_ewoks_to_elk_graph( ... ) laid_out_graph: elk_graph = ELK().layout(elk_graph)

What do you think ?

I like the two definitions. Sure it is more verbose but it is very clear in terms with what we expect. Since we are enforcing typing with mypy, I think it will help in the long run.

LudoBroche · 2026-08-28T16:26:55Z

The __init__.py is getting crowded with various SVG group builders.
I'm moving these helper functions to ./layout/.

I'll even suggest to move everything from __init__.py in another PR. Personally, I am so used to have nothing in these files but I don't always have to idea to check it.

Moving functions in a new module also creates the opportunity to describe a bit more what the functions are for, though the module name.

LudoBroche · 2026-08-28T16:30:30Z

+            if len(points) < 2:
+                continue
+
+            cubic_bezier_path = CubicBezierPath.from_points(


This is the mathematical representation of the Bezier Path.
We make the orthogonal control points into smooth curved curves.

LudoBroche · 2026-08-28T16:36:51Z

+@pytest.fixture(params=graph_names(), ids=graph_names())
+def ewoks_graph(request: pytest.FixtureRequest) -> TaskGraph:
+    graph_description, _ = get_graph(request.param)
+    return load_graph(graph_description)


So far I was only testing on a single graph.
Why not test all of them?

LudoBroche · 2026-08-28T16:38:58Z

+        task_group.extract_input_positions(),
+        task_group.extract_output_positions(),
+    )
+    laid_out_graph: LaidOutElkGraph = ELK().layout(elk_graph)


The layout routing is happening here.

LudoBroche · 2026-08-28T16:40:23Z

+    task_positions = extract_task_positions_from_elk_graph(laid_out_graph)
+    task_group.set_task_positions(task_positions)


I want to keep the ELK graph representation separated from the SVG structure.

LudoBroche · 2026-08-28T16:57:58Z

Tested with ewoks examples:

for graph in acyclic1 acyclic2 acyclic3 cyclic1 demo empty self_trigger triangle1
do
    pixi run graph-to-svg --test "$graph"
done

Seems to be working nicely.

loichuder · 2026-09-01T11:46:31Z

+class ElkChildBase(TypedDict):
    id: str
    width: float
    height: float
    layoutOptions: dict[str, Any]
    ports: list[ElkPort]


-class ElkEdge(TypedDict):
+class ElkChild(ElkChildBase):
+    """An ELK child before layout."""
+
+
+class LaidOutElkChild(ElkChildBase):
+    """An ELK child with coordinates computed by ELK."""
+
+    x: float
+    y: float


Two comments:

If the base is the same as one of the class, I don't think we need to keep both.

Since the first-class citizen is the laid-out graph, I would propose to rename elements before layout to ElementBeforeLayout or UnpositionnedElement and keep the "proper" name for the laid out element Element.

Here how it would pan out for ElkChild:

Suggested change

class ElkChildBase(TypedDict):

id: str

width: float

height: float

layoutOptions: dict[str, Any]

ports: list[ElkPort]

class ElkEdge(TypedDict):

class ElkChild(ElkChildBase):

"""An ELK child before layout."""

class LaidOutElkChild(ElkChildBase):

"""An ELK child with coordinates computed by ELK."""

x: float

y: float

class ElkChildBeforeLayout(TypedDict):

id: str

width: float

height: float

layoutOptions: dict[str, Any]

ports: list[ElkPort]

class ElkChild(ElkChildBeforeLayout):

"""An ELK child with coordinates computed by ELK."""

x: float

y: float

loichuder · 2026-09-01T11:47:28Z

+
+
+class ElkGraph(ElkGraphBase):
+    """An ELK graph before layout."""


I'd suggest to separate the types in two parts: one for raw, unpositionned elements and the other for laid out elements.

loichuder · 2026-09-01T11:48:43Z

I like the two definitions. Sure it is more verbose but it is very clear in terms with what we expect. Since we are enforcing typing with mypy, I think it will help in the long run.

loichuder · 2026-09-01T11:50:09Z

I'll even suggest to move everything from __init__.py in another PR. Personally, I am so used to have nothing in these files but I don't always have to idea to check it.

Moving functions in a new module also creates the opportunity to describe a bit more what the functions are for, though the module name.

loichuder · 2026-09-01T11:51:19Z

@@ -0,0 +1,56 @@
+from ewoksdraw.config.constants import LINK_TURN_RADIUS


Suggested change

from ewoksdraw.config.constants import LINK_TURN_RADIUS

from .config.constants import LINK_TURN_RADIUS

loichuder · 2026-09-01T11:56:13Z

+            if len(points) < 2:
+                continue


By design, points length is always >=2 no?

loichuder · 2026-09-01T12:00:02Z

Not sure this is the best place nor the best name for this module.

Should it not be in svg ? Or even put the function in svg_task_group?

loichuder · 2026-09-01T12:19:53Z

 from .svg_task import TaskSize

 TaskSizes = dict[str, TaskSize]
+TaskPositions = list[TaskPosition]


Why about making this a dict[str, TaskPosition] ? Then, we ensure by design that the name is used as id.

loichuder · 2026-09-01T12:22:53Z

+    transform = group.get("transform")
+    assert transform is not None


If transform is needed, we may as well let the KeyError be raised:

Suggested change

transform = group.get("transform")

assert transform is not None

transform = group["transform"]

loichuder · 2026-09-01T12:32:07Z

+        # Going through the source/target couples in ewoks graph edges
+        # checking for each the computed ELK positions
+        # gives source on the left of target
+        assert positions[source][0] < positions[target][0]


Already part of the docstring

Suggested change

# Going through the source/target couples in ewoks graph edges

# checking for each the computed ELK positions

# gives source on the left of target

assert positions[source][0] < positions[target][0]

assert positions[source][0] < positions[target][0]

LudoBroche linked an issue Aug 28, 2026 that may be closed by this pull request

[Feature]: Use ELK layout to position SVG tasks #24

Open

LudoBroche changed the title ~~Issue #24 - First implementation - tasks positioning + links display~~ Issue #24 - Use ELK layout to position SVG tasks + links Aug 28, 2026

LudoBroche added 2 commits August 28, 2026 17:44

Issue #24 - First implementation - tasks positioning + links display

99fec1b

Issue #24 - Refactoring and clean Up

dd4db3c

LudoBroche force-pushed the 24-feature-use-elk-layout-to-position-svg-tasks branch from 036190d to dd4db3c Compare August 28, 2026 15:44

Issue #24 - Instead of testing one ewoks workflow lets do all of them

c31f898

LudoBroche commented Aug 28, 2026

View reviewed changes

LudoBroche added this to the ewoksdraw 1.0 release milestone Aug 28, 2026

LudoBroche requested a review from loichuder August 28, 2026 17:01

loichuder reviewed Sep 1, 2026

View reviewed changes

loichuder mentioned this pull request Sep 2, 2026

Make internal packages private #14

Open

		task_positions = extract_task_positions_from_elk_graph(laid_out_graph)
		task_group.set_task_positions(task_positions)



		class ElkGraph(ElkGraphBase):
		"""An ELK graph before layout."""

		@@ -0,0 +1,56 @@
		from ewoksdraw.config.constants import LINK_TURN_RADIUS

	from ewoksdraw.config.constants import LINK_TURN_RADIUS
	from .config.constants import LINK_TURN_RADIUS

		transform = group.get("transform")
		assert transform is not None

	transform = group.get("transform")
	assert transform is not None
	transform = group["transform"]

Uh oh!

Conversation

LudoBroche commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR summary

AI Disclosure

Uh oh!

codecov Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche commented Aug 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LudoBroche commented Aug 28, 2026 •

edited

Loading

codecov Bot commented Aug 28, 2026 •

edited

Loading

LudoBroche Aug 28, 2026 •

edited

Loading

LudoBroche Aug 28, 2026 •

edited

Loading