Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception - Expanding Collapsed Edges of a Collapsed Child #128

Open
sashokbg opened this issue Nov 6, 2021 · 9 comments
Open

Exception - Expanding Collapsed Edges of a Collapsed Child #128

sashokbg opened this issue Nov 6, 2021 · 9 comments

Comments

@sashokbg
Copy link

sashokbg commented Nov 6, 2021

Hello, I have stumbled upon two bugs (possible more) when I try to expand edges that are pointing to a child of a collapsed node:

I have created a small demo file to reproduce the issue - you can check out at https://github.com/sashokbg/cytoscape.js-expand-collapse.git branch bug/child_collapse_edges file: demo/demo-bug.html

Bug 1 Disappearing edges - Steps to reproduce:

  1. Starting graph
    image

  2. Click on "collapse edges" button

  3. Close node 1

  4. Close node a

  5. Click on expand all edges <-- exception

  6. Edges disappear

Stack:

Uncaught Error: Can not create edge `e1` with nonexistant source `1`
    Ee cytoscape.min.js:23
    restore cytoscape.min.js:29
    add cytoscape.min.js:29
    expandEdge expandCollapseUtilities.js:786
    expandEdges index.js:272
    forEach cytoscape.min.js:29
    expandEdges index.js:271
    expandAllEdges index.js:360
    <anonymous> demo-bug.html:151
    EventListener.handleEvent* demo-bug.html:150
    EventListener.handleEvent* demo-bug.html:16

Bug 2 - Expanded edges change source / destination

  1. Starting graph
    image

  2. Click on "collapse all"

  3. Click on "collapse edges"

  4. Click on "expand all" <-- bug
    Edges are not having as a source the parent node "a" instead of "1"
    image

  5. Click on "expand edges" <-- bug
    Bug persist after expanding edges, now original links are wrongly having node "a" as a source
    image

Observations

If we first collapse all nodes and then we expand them it seems that the exception issue is gone.
I have however still managed to reproduce the second bug in some scenarios when manually expanding and collapsing the child nodes and edges.

Any help or ideas are greatly appreciated !
Have a nice day!
Alex

@sashokbg
Copy link
Author

sashokbg commented Nov 6, 2021

I have debugged for a while and it seems both problems are coming from manipulating collapsed edges.

First issue
The function barrowEdgesOfcollapsedChildren.

In the case of the first bug, it turns out when collapsing a node, and it has edges that have already been collapsed, we are not properly creating the _cy-expand-collapse-collapsed-edge_s.

A quick fix for this issue is to just expand collapsed edges when collapsing a node. This way the meta edges are properly calculated.

Second issue
Same solution can be applied in the repairEdges function:

    node.connectedEdges('.cy-expand-collapse-collapsed-edge').forEach((edge) => this.expandEdge(edge));

@sashokbg
Copy link
Author

sashokbg commented Nov 8, 2021

A better fix is available at #131

@sashokbg
Copy link
Author

Can anyone of the maintainers please take a look ? @metincansiper @hasanbalci

@sashokbg
Copy link
Author

Anyone ?

@jonlev1n
Copy link

jonlev1n commented May 9, 2022

I came here to report this exact issue. Can we get any insight from the maintainers to respond or accept #131? Or alternatively explain why they cannot/will not use this fix?

@Revadike
Copy link

I came here to report this exact issue. Can we get any insight from the maintainers to respond or accept #131? Or alternatively explain why they cannot/will not use this fix?

+1

@canbax
Copy link
Member

canbax commented May 16, 2022

The first bug doesn't seem like a bug to me. It is a misuse of the API. You should not let the user expand a cy-expand-collapse-meta-edge It is a meta edge created after you collapsed a compound node. First, the compound should be expanded since the source/target doesn't exist.

For the second, I can also say this is not a bug if you first expand the collapsed edge. But this time, expanding all the collapsed edges connected to the compounds before it is going to be expanded might be a good use case.

@hasanbalci
Copy link
Contributor

"Collapse/expand edges" feature was added later. It seems that its interaction with the "collapse/expand nodes" feature wasn't considered comprehensively during addition of this new feature. Therefore, these side-effects/bugs are possible. However, expand-collapse is a rather complex extension and solving such bugs requires a detailed analysis and effort. We don't want to add bug-specific patches since they may cause other unexpected issues.

Unfortunately, we currently don't have enough sources to solve this issue. My suggestion for you to avoid these cases is either to use @sashokbg's branch that possibly has a fix (I didn't try it) or to find some workarounds while presenting these features to your users.

@sashokbg
Copy link
Author

The first bug doesn't seem like a bug to me. It is a misuse of the API. You should not let the user expand a cy-expand-collapse-meta-edge It is a meta edge created after you collapsed a compound node. First, the compound should be expanded since the source/target doesn't exist.

For the second, I can also say this is not a bug if you first expand the collapsed edge. But this time, expanding all the collapsed edges connected to the compounds before it is going to be expanded might be a good use case.

I am sorry to disagree but those are obviously bugs. Calling an API function that completely breaks the library, or wrongly reconstructs your edges afterwards is a definition of a bug.
Either the API should not allow such scenarios or if does they should not break anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants