cytoscape / cytoscape.js-edgehandles

Edge creation UI extension for Cytoscape.js
MIT License
167 stars 87 forks source link

edgeType callback unexpected behaviour #157

Closed azable closed 3 years ago

azable commented 3 years ago

I am using the edgeType callback to prevent edges from being created under certain circumstances. Here is a specific example, which prevents users from creating duplicate edges between the same two nodes:

edgeType: (sourceNode, targetNode) => {
  if (sourceNode.edgesTo(targetNode).length !== 0) {
    return undefined;
  }
  return 'flat';
}

This works great for preventing invalid (duplicate) edges from being created. However, there is odd behaviour when a valid edge is created. Firstly, complete is not called for the edge. Secondly, the edge cannot be selected. I suspect this may be some sort of order bug whereby the actual edge is prevented from being created, since the preview edge is "counted" as an already existing edge. I've attempted to try to identify the preview edge and exclude it from the count, but to no avail thus far.

There is a related discussion that doesn't have a resolution at the end of https://github.com/cytoscape/cytoscape.js-edgehandles/issues/39.

If this is intended behaviour, is there a better way to solve this problem?

daemontus commented 3 years ago

If anyone is interested in a workaround: The reason why this keeps happening seems to be that edgeType is called again, even after the edge is created. This means edgesTo will return undefined which breaks something and creates an inconsistent state where complete is never called. To avoid this situation, we have to return flat even for this freshly created edge.

To do so, I propose:

1) Add some kind of marker data to the new edge using edgeParams:

edgeParams: function() {
    return { data: { fresh: true }};
},

2) In edgeType, return undefined only if you see an edge without this marker:

let edges = sourceNode.edgesTo(targetNode); 
if (edges.length > 0) {
    let is_fresh = edges[0].data().fresh === true;
    if (is_fresh) { return 'flat'; } else { return null; }            
} else { return 'flat'; }

3) In complete, remove the fresh marker from the edge. Thus it won't be created again next time edgeType is called.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale, because it has not had activity within the past 30 days. It will be closed if no further activity occurs within the next 30 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

maxkfranz commented 3 years ago

If anyone is open to making a PR to cache the call to edgeType(), I'd be open to reviewing it and merging it in for a release.

The original design of the extension considered edgeType() to be idempotent

daemontus commented 3 years ago

Thanks! I might be able to get to it later this month when I have more time to actually work on the project where we use this. But until then it is probably fine to close this as stale.