Skip to content

Allow use of local flow nodes#82

Merged
reddigari merged 15 commits intomasterfrom
dev/samir
May 7, 2020
Merged

Allow use of local flow nodes#82
reddigari merged 15 commits intomasterfrom
dev/samir

Conversation

@reddigari
Copy link
Collaborator

  • Renders an input flow port on all nodes
  • Fetches global and (connected) local flow nodes when config form is opened
  • Fixes a couple tiny server-side bugs relating to treatment of flow nodes
  • Uses CSS flex to layout the icons inside the node widget so the gear is centered when shown alone
  • Alters the link style to match KNIME (but same color for flow control links for now)

Rather than set up a new custom class to represent flow ports (which involves some react-diagrams headaches), I just used the existing VPPortModel and indicate the flow port by name. The CustomNodeWidget finds it and renders/styles it separately from the other input ports. Should be fine since there is exactly one per node.

Copy link
Member

@reelmatt reelmatt left a comment

Choose a reason for hiding this comment

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

I think this looks great. All "normal" combinations I tried worked flawlessly, so should be good for a demo.

There were a few bugs I noticed when trying "non-normal" combinations. I had some ideas that seemed easy at first, but got more complicated the more I looked into it—react-diagrams headaches is an understatement! Given the time remaining, I'd probably classify these as "nice-to-haves":

  • Flow node output ports should also be purple circles. KNIME also has different shapes/colors for other kinds of ports we're not dealing with. The idea is you can more easily tell what hooks up with what. This seemed the easiest where you could just add make the port's name flow-out, much like you did with the flow-in ports, but at first glance it looks like adding a lot of if/else conditions to test what's what.
  • Flow nodes should only be allowed to connect to flow-in ports. This also seemed easy at first—you can just modify the canLinkToPort in the VPPortModel—but it doesn't look like the port knows its parent node which makes it tricky.
    • Easier temp fix??: the server throws an error if too many edges try to attach to in ports (e.g. try to connect a flow node to a flow port AND a regular port). Can react-diagrams delete, or not render, the failed edge? This way the user gets a clearer idea the edge failed than just looking at the console (it says failed, but I see an edge).

@reelmatt reelmatt linked an issue May 4, 2020 that may be closed by this pull request
@reddigari
Copy link
Collaborator Author

I did mean to implement the canLinkToPort method but as you said it gets a little complex. The port does know its node (port.getNode()), but all the combos of in/out/flow/data gets messy.

I think I can wrangle the output flow ports to match the input flow (purple circle on the right), but I'm not positive.

@reddigari
Copy link
Collaborator Author

The rendering issues are definitely included in my react-diagrams headaches. Even if canLinkToPort returns false, you're left with a dead link somewhere near the port, and I have had no success figuring out how to "cancel" the rendering of the link. I know @cesaragv fixed a lot of this, e.g. when you click a port, you no longer get a random link up to the corner. However if you look closely, clicking a port does result in the END of a link in the top left corner, so we have a lot of link-rendering bugs that could ideally be cleaned up (but we almost definitely don't have time for).

@reddigari
Copy link
Collaborator Author

reddigari commented May 6, 2020

  • Render flow out ports similar to flow in ports (purple circles). Flow nodes no longer have any data ports.
  • Fix canLinkToPort so only flow-in-to-flow-out and data-in-to-data-out are allowed.

I have no idea how to prevent the rendering of invalid links, so we might just want to call this done.

Screen Shot 2020-05-06 at 5 11 27 PM

@reddigari reddigari merged commit 3903abf into master May 7, 2020
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 this pull request may close these issues.

UI: Flow variable ports/Node configuration

2 participants