Border nodes appear inside of their container

[Gelio]

• I have checked that this bug has not yet been reported by someone else
• I have checked that this bug appears on Chrome
• I have specified the version :

• 2021.12.7

• I have specified my environment :

• Ubuntu 20.04 LTS

Screenshots

Note that the border nodes are inside of the container

Whereas in Sirius Desktop they are correctly rendered right outside the border of the container

Interestingly, the border nodes are positioned correctly after I force rearrangement of elements on the diagram, but they go back into the container after I try to move them

Peek.2022-01-12.19-20.mp4

Steps to reproduce

1. Open a model that has border nodes

Expected behavior

Border nodes appear on the edge of their container, just like in Sirius Desktop

Actual behavior

Border nodes appear inside of their container, not making them border nodes anymore.

I thought #359 was supposed to fix it, but it seems I was wrong. The problem is also visible in the screenshots in that issue, though, so it seems like a regression.

[sbegaudeau]

Small comment for myself and the rest of the team: I'm pretty sure that on the frontend side at least, the solution should involve a better support for SPort which should have a different move strategy.

[pcdavid]

I'm not so sure. Sprotty's SPorts are not SNodes. It's a different type of element, which in particular does not have children. It seems designed more as "anchor points" for incoming/outgoing edges, but in Sirius border nodes have all the capabilities of other nodes (they can have their own children, and even their own border nodes). The only thing that makes them special is that their location is constrained to the (rectangular) bounding box of their parent.

Sprotty's type hierarchy seems to match ELK's metamodel:

• SConnectableElement <-> ElkConnectableShape
• SNode extends SConnectableElement <-> ElkNode extends ElkConnectableShape
• SPort extends SConnectableElement <-> ElkPort extends ElkConnectableShape

ELK has the same limitation.

So it seems there are two options:

1. Use the SPort mechanism anyway, and accept that Sirius Components will only support a subset of what Sirius border nodes can do (granted, it's the most common use case, but our APIs are more generic than that: {oes}.components.diagrams.Node.getBorderNodes() is a generic List<Node>, which can themselves have children & border nodes). It might not be that simple as SNode does not extend SPort (so, code duplication or dynamic type checks to handle both cases with the same code).
2. Continue to use SNode to represent border nodes, but make the code (frontend, ELK, incremental layout) aware that their position is constrained compared to plain child nodes (e.g. extending SiriusDragAndDropMouseListener#getValidPosition() for the front-end/interactive move part).

[sbegaudeau]

Yes that's a core part of the issue. I wonder if it's worth trying to support border nodes as nodes. We could simply convert border nodes to ElkPort on the backend and SPort on the frontend and while the diagram API could theoretically support border nodes with border nodes or child nodes, in practice it would not be supported for now.

Have you encountered, over the years, that many Sirius RCP diagrams with border nodes containing child nodes or border nodes?

[pcdavid]

Have you encountered, over the years, that many Sirius RCP diagrams with border nodes containing child nodes or border nodes?

Technically the Sirius Desktop support for sequence diagrams is almost entirely based on that capacity (lifelines are border nodes of the participant nodes, and all executions on a lifeline are border nodes of the lifeline or of their parent execution). But that is a very special case.

I don't have other concrete examples, so I guess we can decide to only support the common case.

There's still the issue that SPorts are not SNode, so going this route, while being more in line with how Sprotty and ELK work, may require some duplication or refactorings to get the same behaviors we have on nodes available on ports (styling, tool application, direct edit, etc.). We'll see when I start concretely (soon).

[sbegaudeau]

On the backend, we already convert border nodes to ElkPort in ELKDiagramConverter#convertBorderNode so I think that this part is already ok (which is why the arrange all correctly put the border nodes on the border of their container). I think that the main issue is that convertDiagram on the frontend does not convert our border nodes to our Port type but instead to our Node type. You should start looking there to see what are the consequences of such change.

[pcdavid]

I think that the main issue is that convertDiagram on the frontend does not convert our border nodes to our Port type but instead to our Node type.

Already started on that, but currently stuck with GraphFactory creating plain Nodes for my BorderNodes/Ports. I'm not very familiar with the part of Sprotty, but I'm investigating. I'll keep updating here with what I find.

Note that one limitation of SPort is that they can not have children, which (from what I understand) means no labels. Not supporting the complex "border nodes on border nodes with nodes inside them" case is OK, but not supporting labels not so much. Sure it's probably possible to add it, but not sure how much work that would involve.

[pcdavid]

Actually, SPort extends SConnectableElement extends SShapeElement extends SChildElement extends SParentElement... which has a children property, but defined as readonly:

readonly children: ReadonlyArray<SChildElement> = [];

SNode redefines a children property as mutable array: children: SChildElement[];

It seems we're supposed to use SParentElement.add/remove to add/remove children, the methods ensuring the index is kept up to date.