Dagre: d.eachNode iterates over removed objects.

[nebril]

Hi,

I am creating a graph app using dagre layout. I want this graph to be interactive, so I am using two cytoscape instances as described in this answer: http://stackoverflow.com/questions/28319490/how-to-manipulate-cytoscape-js-graph-data-before-rendering

Now sometimes I want to scratch my graph completely and I call cy.remove('*') to remove all nodes, then I am adding filtered nodes from allcy to it, running the layout, voila.

However, I was getting an error in this line of code: https://github.com/cytoscape/cytoscape.js/commit/0fa45e03e11a0a4fcd001a832c1d5cbc5a77d786#diff-62b92f7ccee2858aeb394a10bd1e0114L82

It seems like it was iterating over objects I removed, as I wasn't getting any scratch objects from the cy. I am patched it locally by adding an if watcher, to check whether retrieved element exists, but I guess it should be fixed somewhere else. Here is my quick fix:

  d.eachNode(function(id, n) {
    var dagreElement = cy.getElementById(id);
    if(dagreElement.length > 0) dagreElement.scratch().dagre = n;
  });

I will try to find the time to fix this properly via PR, but I cannot guarantee any timeline so I am filing this issue make the bug public.

[maxkfranz]

Scratchpad data is temporary, and so it is never serialised in ele.json(). If it's not serialised, then you can't transfer that data with elements that are transferred from one graph to another one.

If you don't want to render some elements in the graph, it would be better to set display: none to those elements using a class. That will probably sidestep your issue.

I'm not sure exactly why your workflow causes the error. Maybe an example or a numbered list of steps to reproduce would help. I will experiment and try to reproduce the error as soon as I get the chance.

Thanks!

[nebril]

I will give you an example:

I am loading all my graph data to the variable I called allcy:

var allcy = cytoscape({
    headless: true,
});

Then I am creating normal "working" cy var

var cy = cytoscape({
    container: document.getElementById('thegraph'),
    //stylesheets, dagre layout options, hideEdgesOnViewport etc.
});

I load all my data to allcy

allcy.load(response); //response is json graph data I got from xmlhttp request

I add roots and their neighbourhood to cy and reload the layout.

cy.add(allcy.nodes().roots().closedNeighborhood());
cy.layout(layout);
cy.load( cy.elements('*').jsons());

Then I want to start my graph exploration in another place, so I do the following:

cy.remove(cy.elements());
cy.add(allcy.nodes("[funcName *= '" + funcName + "']").closedNeighborhood());
cy.layout(layout);
cy.load( cy.elements('*').jsons(), function() { cy.center(node)} );

And it is when it fails without fix proposed by me.

[maxkfranz]

You can work around this by not calling cy.load(). That's the problem. Without load(), it's not reproducible -- at least for me on the debug page.

[maxkfranz]

Try the above commit on this snapshot : http://js.cytoscape.org/download/cytoscape.js-snapshot-6e4647c514-1423771528077.zip

It may work around your issue, but it slightly changes backwards compatibility with old cy.layout() behaviour. Since that's deprecated and would work the same for 95%+ of people anyway, I think we should just go ahead with the workaround if it works for you. If not, I'll revert.

[nebril]

I'll let you know how it is tomorrow morning european time. Thanks for taking the time to fix this!

[nebril]

Applying layout after adding nodes without calling load breaks node positions (they appear in upper left corner). While using load - the issue remains. I will stay with my workaround for now.

[maxkfranz]

I can't reproduce this after the change I made. Would you please create a either a testcase under /test or a demo that I could use to debug the issue? Thanks

[maxkfranz]

Eliding from 2.3.11 until reproducible

[maxkfranz]

@nebril Do you find this reproducible with the latest Dagre and the latest Cytoscape?

[maxkfranz]

Closing for now

Dagre is moved to its own repo now too : https://github.com/cytoscape/cytoscape.js-dagre