Create GConnection via EMF may throw Exception

[eckig]

If I create a GConnection programmatically and add it via EMF commands just like in the Tree Example:

final GConnection connection = GraphFactory.eINSTANCE.createGConnection();

connection.setType(BusinessProcessConstants.CHILD_CONNECTION);
connection.setSource(source);
connection.setTarget(input);

EditingDomain editingDomain = AdapterFactoryEditingDomain.getEditingDomainFor(model);
CompoundCommand command = new CompoundCommand();

command.append(AddCommand.create(editingDomain, model, GraphPackage.Literals.GMODEL__NODES, childNode));
command.append(AddCommand.create(editingDomain, model, GraphPackage.Literals.GMODEL__CONNECTIONS, connection));
command.append(AddCommand.create(editingDomain, source, GraphPackage.Literals.GCONNECTOR__CONNECTIONS, connection));

if (command.canExecute())
{
    editingDomain.getCommandStack().execute(command);
}

This will not work properly using the default skins. (looks somewhat weird and the arrow visuals are lost when one of the two connected nodes gets dragged around). Additionally trying to detach this connection causes an Exception:

Exception in thread "JavaFX Application Thread" java.lang.ArrayIndexOutOfBoundsException: -1
    at java.util.ArrayList.elementData(ArrayList.java:418)
    at java.util.ArrayList.get(ArrayList.java:431)
    at de.tesis.dynaware.grapheditor.core.skins.defaults.DefaultTailSkin.draw(DefaultTailSkin.java:125)
    at de.tesis.dynaware.grapheditor.core.connections.TailManager.createFromConnection(TailManager.java:88)
    at de.tesis.dynaware.grapheditor.core.connections.ConnectorDragManager.detachConnection(ConnectorDragManager.java:498)
    at de.tesis.dynaware.grapheditor.core.connections.ConnectorDragManager.handleMouseDragged(ConnectorDragManager.java:335)

I looked into the ConnectorDragManager and I can see the reason why: When a connection is added with mouse gestures, the Joints are calculated and added to the connection. Since the joint positions are calculated based on the skin, why not apply the same logic to connections which have been added programmatically?

[rmfisher]

In ConnectorDragManager, when a new connection is created its joint positions come from GTailSkin.allocateJointPositions(). The user has dragged a tail somewhere to get to this point, and so the tail knows where the new joints should be placed based on its own position.

If you are creating a connection programmatically, you need to add the joints yourself, e.g.

final GConnection connection = GraphFactory.eINSTANCE.createGConnection();

connection.setType(BusinessProcessConstants.CHILD_CONNECTION);
connection.setSource(source);
connection.setTarget(input);

final GJoint firstJoint = GraphFactory.eINSTANCE.createGJoint();
firstJoint.setX(123);
firstJoint.setY(456);

final GJoint secondJoint = GraphFactory.eINSTANCE.createGJoint();
secondJoint .setX(123);
secondJoint .setY(456);

connection.getJoints().add(firstJoint);
connection.getJoints().add(secondJoint);

If you have not registered a custom skin for your type 'BusinessProcessConstants.CHILD_CONNECTION', the default connection skin will be used. This currently expects:

• At least 2 joints
• An even number of joints

The exception handling is very poor, you should not get an ArrayOutOfBoundsException. I will try to improve this as soon as possible.

[eckig]

My point is, that it should not be necessary to add the correct number of joints but the framework (graph-editor) should do that if necessary?

[rmfisher]

Well the graph editor framework cannot do it, it doesn't know anything about the restrictions that a particular skin might have.

The issue is the default connection skin. Perhaps it could create 2 joints if none are present. But what positions should it choose? Something simple based on the positions of the nodes it's connecting I guess.

If you want to provide a solution for this I will try to integrate it.

[eckig]

Really? :-)

If I look into ConnectorDragManager the framework is already doing this. Just for the GUI side of creating connections... Would'nt it be possible to move addConnection(GConnector,GConnector) method out of the ConnectorDragManager. So the ConnectorDragManager only adds the connection to the EMF model and when the model changes the connection is added with the extracted method?

[rmfisher]

The ConnectorDragManager does not decide how many joints will be added to a new connection, and what their positions will be. It calls GTailSkin.allocateJointPositions(). The DefaultTailSkin class allocates 2 or 4 new joints, depending on its shape.

Maybe I don't understand what you mean exactly. Where would you like to move the addConnection method to?

[eckig]

Or maybe I am confusing things, but I will try to clear this up..

Currently the user interfaces goes as follows: User Mouse gesture connects two connectors, if the validator successfully validates them a connection is created, added to the model and to the GUI (with the correctly allocated joint positions).

If the user "only" adds the connection to the model by programmatically connecting two connectors, the GUI of that connection may result in an "incomplete state" because the joint positions are not correctly allocated.

From my simple point of view it should be pretty easy to refactor both behaviors into a single one: User Mouse gesture connects two connectors, if the validator successfully validates them a connection is created and added to the model. You dont allocate the joint positions here, but after the EMF listener notifies you about that new connection. So both mouse gesture connections and manual connections end up being handled the same way: EMF notification about a new connection > Retrieve skin lookup > allocate joint positions > add joints to the connection.

[rmfisher]

Firstly, lets make it clear that the position-allocation logic is skin-specific (i.e. customizable) and should not be done by the framework. Where would you like this logic to take place? In the GConnectionSkin implementation when it initializes?

The reason the tail skin instance currently does it, is that the tail skin knows where the user has been dragging it. The information about where the joints should go is already there to be used.

Secondly let's consider how EMF commands work. If we execute one command to create a connection, then execute a second command to add joints, there will be 2 new commands on the command stack. Therefore you will need to press undo twice to remove the connection.

It is possible to use the "CompoundCommand#appendAndExecute" method for adding the joints, so they are appended to the existing compound command that added the connection. One could in principle use this approach to create a 'tolerant' connection skin that would allocate joints itself if it was initialized with no joints. I do not think we will implement this ourselves in the near future.

We have never had a use-case yet to add connections programmatically, and the documentation & exception-handling for this is unclear. I'll try to improve it soon.

[eckig]

Thanks for clearing that up.

So I have to figure something out on my own :-)

[rmfisher]

No problem.

Could you give me a complete code sample that leads to the exception? Then I'll try and figure out what's going on.

[eckig]

Just use the code in the tree-demo used for creating a new direct child node but without the custom connection skin (use the default arrow skin). When you create a child node that way will look weird and it can not be dragged / detached.

[rmfisher]

I'm not sure what you mean by "default arrow skin", the default connection skin is not an arrow.

If I comment out line 266 of the TreeNodeSkin class, i.e.

// connection.setType(TreeSkinConstants.TREE_CONNECTION);

this will definitely lead to strange behaviour. The tree node skins are not intended to be mixed with the default connection skins. As I said, the default connection skins expect at least 2 joints. They also currently expect that their start and end connectors are attached to the sides of nodes.

The custom skins are there to show what is in principle possible. They are not intended to be mixed together. If your custom node skin has connectors on the top or bottom, you will have to create a custom connection skin that can deal with this.

[eckig]

Ok, then I was not clear enough :-)

When I said "use the code in the tree-demo used for creating a new direct child node" I meant just that part of the tree skin code. I used that part of the code to create a new connection between two nodes. The skin for that node puts the connectors on the left or right.

And as you explained earlier, I now understand how to use the default connection skin. I am adding two joints to the connection and then it works. It is not a perfect solution, but it works for now.