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

fix: allow HierarchicalLayout to manage circular dependencies #233

Merged

Conversation

Franz-Ritter
Copy link

@Franz-Ritter Franz-Ritter commented Aug 10, 2023

Summary
Solved bug creating a graph with a circular dependency using the HiearchlLayout.ts algorithm.

Check this issue for more detailed information including an example to reproduce the bug

Solution:

Uncomment the isAncestor method GraphHierachyNode, which is used by HiearchlLayout. The uncommented implementation makes sure, that a circular dependency is detected and resolved. Using the default isAncestor does not work at this point, because the parent nodes of the child nodes in the graph are not set.

Closes #231

@tbouffard
Copy link
Member

Hi @Franz-Ritter did you try to add automatic tests as I suggested in the issue?

This would reproduce the initial problem and show that is now fixed.

@tbouffard tbouffard added the bug Something isn't working label Aug 28, 2023
@Franz-Ritter
Copy link
Author

Hi @tbouffard, Sorry I oversaw your comment. I try to find time to check how you structure your tests, and add one to the PR as requested.

@Franz-Ritter
Copy link
Author

@tbouffard I added a test which creates a cyclic Graph and a non-cyclic graph.
Please consider that I am not a native typescript developer...
Anyway, both graph used to work using mxGraph. When you do and checkout the code and execute the test without the change in GraphHierarchyNode one of the tests fails. However, my change let both test pass.

If you have any questions or need more input, please let me know. Applying that fix on the code base would help me a lot. Atm i am forced to package and link the change locally.

@Franz-Ritter
Copy link
Author

@tbouffard Do you need some additional input?

@tbouffard
Copy link
Member

Hi @Franz-Ritter
I have been busy at work and I haven't had time to have a look at this Pull Request.
I will check it in a few days.

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Franz-Ritter It's all good to me. Thanks

  • ✔️ reproduce the issue with newly introduced tests (prior the fix is applied)
  • ℹ️ I push directly some lint fixes in the branch of this PR

@tbouffard tbouffard changed the title Uncommit isAncestor method to resolve circular dependndies during gra… fix: allow HierarchicalLayout to manage circular dependencies Sep 14, 2023
@tbouffard tbouffard merged commit efb5b4b into maxGraph:development Sep 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting error on circular dependency using HierarchicalLayout
2 participants