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

XELK Transformer Restructure #23

Closed
3 of 6 tasks
dfreeman06 opened this issue Oct 21, 2020 · 5 comments · Fixed by #51
Closed
3 of 6 tasks

XELK Transformer Restructure #23

dfreeman06 opened this issue Oct 21, 2020 · 5 comments · Fixed by #51

Comments

@dfreeman06
Copy link
Contributor

dfreeman06 commented Oct 21, 2020

  • Refactor XELK to use a flatter approach to ElkNode generation rather than recursive.
  • Avoid None to mark the XELK root node. Consider adding a Sentinel singleton in case None is a node in the incoming Networkx graph.
  • Investigate moving dynamic visibility functionality from XELK to more general ElkTransformer
  • Improve the to_dict and from_dict. Issue with a port dict that has an ElkLabel instance instead of a dict that looks like an ElkLabel
  • Simplify transformer.py by pulling out logic into smaller more testable pieces
  • discuss how get_properties get_layout and get_css should work. Remove them?
@dfreeman06
Copy link
Contributor Author

Avoid None to mark the XELK root node. Consider adding a Sentinel singleton in case None is a node in the incoming Networkx graph.

@nrbgt
Copy link
Contributor

nrbgt commented Nov 12, 2020

A further refactor for testability would be to extract all of the "guts" of the transformer algorithm to not be aware of widgets or even traitlets. This would help the typeability (#25) / testability (#39) of the system. The output of this would be schema-valdiateable (#29).

@dfreeman06
Copy link
Contributor Author

dfreeman06 commented Nov 14, 2020

Scratch pad for thoughts:

  • with Normalize and test transformer handling of custom properties #47, is it worth keeping css_classes dictionary?
    • original intent was to have a mechanism for only updating css classes of some marks (probably better handled with additional commands to sprotty like the current fit and zoom commands in ElkDiagram.
  • standardizing generating elk ids - currently mapping networkx nodes to strings for predictable identifiers between updates. better ways to accomplish using internal maps?
  • ElkPort creation happens in two places - simplify

Thoughts to help unit testing

  • currently awkward to unit test test the ElkTransformer without a browser since it relies on the ElkTextSizer to bounce messages through the frontend. Current approach to test is to set the trasformer's text_sizer to None

@nrbgt
Copy link
Contributor

nrbgt commented Nov 16, 2020

standardizing generating elk ids

i started mangling them, couldn't handle the > in them anymore. they are funny as all get out once emoji start showing up. what did you have in mind?

@dfreeman06
Copy link
Contributor Author

We can even uuid them if we keep a mapping of the original item to the new uuid. The problem i see is specifying the ports on an edge. I havent worked out how we would concisely specify port data on an edge, right now:
g.add_edge("a", "b", sourcePort="x", targetPort="y")

Programatically things work out nicely as well if "x" and "y" are hashable things we can still work out mappings but I dont know what it looks like yet for round tripping from networkx json. There might be something obvious but not clear yet in my brain.

@dfreeman06 dfreeman06 linked a pull request Dec 2, 2020 that will close this issue
3 tasks
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

Successfully merging a pull request may close this issue.

2 participants