iVis-at-Bilkent / cytoscape.js-undo-redo

A Cytoscape.js extension to provide an undo-redo framework
MIT License
46 stars 9 forks source link

Undoing a target change on an edge using move command #13

Closed Linkmetal closed 4 years ago

Linkmetal commented 5 years ago

Hi!

I'm having problems undoing a move command to change the target property of an edge.

I was making some tests with the lib and I noticed that when I do a move action on an edge and try to undo it, it returns a "Cannot read property [0] of 'undefined'". I debugged the source code and noticed that or I'm doing something in the wrong way or there is something wrong on the code:

                // cytoscape-undo-redo.js Line 488
                undo_: function (eles) {
                        var newEles = cy.collection();
                        var location = {};
                        if (eles.newNodes.length > 0) {
                            location.parent = eles.newNodes[0].parent();
                            for (var i = 0; i < eles.newNodes.length; i++) {
                                var newNode = eles.newNodes[i].move({
                                    parent: eles.oldNodes[i].parent()
                                });
                                newEles.union(newNode);
                            }
                        } else {
                            location.source = location.newEdges[0].source();
                            location.target = location.newEdges[0].target();
                            for (var i = 0; i < eles.newEdges.length; i++) {
                                var newEdge = eles.newEdges[i].move({
                                    source: eles.oldEdges[i].source(),
                                    target: eles.oldEdges[i].target()
                                });
                                newEles.union(newEdge);
                            }
                            console.log("newEles -->", newEles);
                            console.log("location -->", location);
                        }
                        return {
                            eles: newEles,
                            location: location
                        };

When I call the move command on the edge to change the target property, the newNodes array is empty so it goes to the else part of the function. On this part there is this two lines:

         location.source = location.newEdges[0].source();
         location.target = location.newEdges[0].target();

The problem is that the location object is always going to be void because is declared at the start of the function like that and never gets modified.

I tried to modify it a little bit to make it works but I also facing a problem, I modified the previous two lines like this:

         location.source = eles.newEdges[0].source();
         location.target = eles.newEdges[0].target();

With that changes I solved the problem of the undefined property, but when the plugin saves the action on the Undo Stack with the unmodified edge, there is some point where the "OldEdges" property gets modified and gets modified with the new value. I tried to debug it and locate the line where it gets modified but I got no clue where it is happening.

I don't know if I'm changing the edge's target in the proper way, but I didn't saw another way to do it. I attach you the code that I have for the move command:

 this.ur.do("move", {
        eles: `#${this.graph.$(`#${this.selectedEdge}`).id()}`,
        location: {
          target: newId.toString(),
        }
 });
hasanbalci commented 4 years ago

@Linkmetal Sorry for the late response. Firstly, your correction is correct, it should be eles instead of location. Secondly, I think eles.move() operation in Cytoscape.js has been done in-place after v3.4.0, therefore "oldEdges" are updated with the new values and we lose the original "oldEdges" information. I fixed the problem by using element ids instead of elements themselves while transferring information to the undo operation. If possible, can you please check that again and verify the problem is solved?