Redo Command leads to NullPointerExceptions

eckig commented 9 years ago

I debugged this a lot but I can not find the source of this issue.. In your demo application everything works fine, in my application not.

Reproduction:

creating a new GNode (with 2 or more GConnectors)
creating 2 or more connections between them
undo all changes
redo the changes

Redoing the last change (last connection added) produces the following:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    at de.tesis.dynaware.grapheditor.core.skins.defaults.connection.SimpleConnectionSkin.restrictFirstAndLastJoints(SimpleConnectionSkin.java:208)
    at de.tesis.dynaware.grapheditor.core.skins.defaults.connection.SimpleConnectionSkin.addRectangularConstraints(SimpleConnectionSkin.java:186)
    at de.tesis.dynaware.grapheditor.core.skins.defaults.connection.SimpleConnectionSkin.setJointSkins(SimpleConnectionSkin.java:112)
    at de.tesis.dynaware.grapheditor.core.skins.defaults.DefaultConnectionSkin.setJointSkins(DefaultConnectionSkin.java:70)
    at de.tesis.dynaware.grapheditor.core.skins.SkinManager.addJoints(SkinManager.java:358)
    at de.tesis.dynaware.grapheditor.core.skins.SkinManager.addConnections(SkinManager.java:204)
    at de.tesis.dynaware.grapheditor.core.GraphEditorController.updateSkinManager(GraphEditorController.java:229)
    at de.tesis.dynaware.grapheditor.core.GraphEditorController.reloadView(GraphEditorController.java:182)
    at de.tesis.dynaware.grapheditor.core.GraphEditorController.initializeAll(GraphEditorController.java:150)
    at de.tesis.dynaware.grapheditor.core.GraphEditorController.lambda$createCommandStackListener$47(GraphEditorController.java:169)
    at de.tesis.dynaware.grapheditor.core.GraphEditorController$$Lambda$625/334772896.commandStackChanged(Unknown Source)
    at org.eclipse.emf.common.command.BasicCommandStack.notifyListeners(BasicCommandStack.java:270)
    at org.eclipse.emf.common.command.BasicCommandStack.redo(BasicCommandStack.java:194)
    at de.tesis.dynaware.grapheditor.Commands.redo(Commands.java:289)

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    at de.tesis.dynaware.grapheditor.core.skins.defaults.connection.SimpleConnectionSkin.alignJoint(SimpleConnectionSkin.java:241)
    at de.tesis.dynaware.grapheditor.core.skins.defaults.connection.SimpleConnectionSkin.checkFirstAndLastJoints(SimpleConnectionSkin.java:222)
    at de.tesis.dynaware.grapheditor.core.skins.defaults.connection.SimpleConnectionSkin.applyConstraints(SimpleConnectionSkin.java:118)
    at de.tesis.dynaware.grapheditor.core.view.ConnectionLayouter.redraw(ConnectionLayouter.java:62)
    at de.tesis.dynaware.grapheditor.core.view.GraphEditorView$GraphEditorViewLayer.layoutChildren(GraphEditorView.java:295)
    at javafx.scene.Parent.layout(Parent.java:1074)

I can see why this is happening: The eContainer of the target of the connection is null. But I do not know why. When the connection is created everything is fine, if I undo and redo the connection the eContainer is gone. For testing purposes I unhooked my domain model but that does not change anything..

rmfisher commented 9 years ago

Interesting, I will look into this and get back to you asap.

rmfisher commented 9 years ago

Can you link the code where you create the new GConnection objects and add them to the GModel?

eckig commented 9 years ago

That is not easy, but I tried to extract it:

GConnection connection = GraphUtils.createConnection(CHILD_CONNECTION, pOutputConnector, childInput,
  new Point2D(getNode().getX() + getNode().getWidth(), getNode().getY()),
  new Point2D(node.getX(), node.getY()));

EMFUtils.addCommand(model, pNewNode)
  .append(EMFUtils.addCommand((EProcessModel) getStep().eContainer(), pNewStep))
  .append(EMFUtils.addCommand(model, pConnection))
  .append(EMFUtils.addCommand(model, pConnection.getSource(), pConnection))
  .append(EMFUtils.addCommand(model, pConnection.getTarget(), pConnection));
command.execute();

pOutputConnector is the given output GConnector where the new node is to be attached to. childInput is the new GConnector of the new GNode. EMFUtils is my utiltiy class helping with the EMF commands (shorter to write, builder pattern, ..).

And the mentioned method:

public static GConnection createConnection(String pConnectionType, GConnector pSource, GConnector pTarget, Point2D pSourcePoint,
            Point2D pTargetPoint)
    {
        Point2D middle = pSourcePoint.midpoint(pTargetPoint);

        GConnection connection = GraphFactory.eINSTANCE.createGConnection();
        connection.setType(pConnectionType);
        connection.setSource(pSource);
        connection.setTarget(pTarget);

        connection.getJoints().add(createJoint(middle.getX(), pSourcePoint.getY()));
        connection.getJoints().add(createJoint(middle.getX(), pTargetPoint.getY()));

        return connection;
    }

rmfisher commented 9 years ago

Looking at your code I assume that the eContainer of the target of the connection should be 'pNewNode'. Can you make sure you are calling

pNewNode.getConnectors().add(childInput)

before the command is executed?

eckig commented 9 years ago

I thought EMF does that?

Edit: I already do that. Edit#2: It is not the programmatically added connection that causes the NPE. The connection I created via Drag and Drop afterwards does.

rmfisher commented 9 years ago

We use EMF 'EOpposites' in the simple cases, for example if you call

pNewNode.getConnectors().add(childInput)

then 'childInput' will automatically have it's parent set to 'pNewNode', as if you had also called

childInput.setParent(pNewNode)

You just need to call either one, and it needs to be called before you execute your command.

Unfortunately for slightly more complicated relationships we could not use this mechanism so you have to specify everything, e.g.:

newConnection.setSource(newSource);
newConnection.setTarget(newTarget);
newSource.getConnections().add(newConnection);
newTarget.getConnections().add(newConnection);

rmfisher commented 9 years ago

I also just noticed, make sure you are calling

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

rather than

command.execute();

directly. The former will trigger the command stack listener that updates the view.

eckig commented 9 years ago

Ok I finally found the problem: The first GConnector was added via EMF and thus working fine with undo and redo. The second connector was not added via EMF commands and that caused the problems.

rmfisher commented 9 years ago

Ok great. I added some additional logging whenever the model state changes that will hopefully make it easier to detect errors like this in the future.

eckig commented 9 years ago

Ok. Thanks anyway and always good to have additional logging.

tesis-dynaware / graph-editor

Redo Command leads to NullPointerExceptions #10