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

[WIP] Macro nodes #196

Draft
wants to merge 73 commits into
base: master
Choose a base branch
from
Draft

Conversation

ales-erjavec
Copy link
Collaborator

Issue

Need macro node support

Changes

Add macro node functionality - hierarchical sub-workflows.

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2021

Codecov Report

Attention: Patch coverage is 83.72361% with 424 lines in your changes missing coverage. Please review.

Project coverage is 76.83%. Comparing base (834ae0b) to head (0d2d8e9).

Files Patch % Lines
orangecanvas/scheme/readwrite.py 80.85% 90 Missing ⚠️
orangecanvas/document/schemeedit.py 77.28% 82 Missing ⚠️
orangecanvas/scheme/signalmanager.py 58.27% 63 Missing ⚠️
orangecanvas/gui/scene.py 72.72% 39 Missing ⚠️
orangecanvas/document/interactions.py 47.69% 34 Missing ⚠️
orangecanvas/scheme/metanode.py 91.08% 32 Missing ⚠️
orangecanvas/canvas/scene.py 88.95% 19 Missing ⚠️
orangecanvas/canvas/items/nodeitem.py 80.00% 14 Missing ⚠️
orangecanvas/scheme/scheme.py 89.10% 11 Missing ⚠️
orangecanvas/scheme/events.py 80.55% 7 Missing ⚠️
... and 11 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
+ Coverage   76.62%   76.83%   +0.21%     
==========================================
  Files          99      106       +7     
  Lines       21155    22667    +1512     
==========================================
+ Hits        16209    17416    +1207     
- Misses       4946     5251     +305     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@borondics
Copy link
Member

@ales-erjavec, this is a very nice addition, can't wait to be available in the released version - my colleagues build brutally large workflows sometimes.

One thing to keep in mind could be to limit the depth of levels a user can create so that it doesn't get out of hand like LabView. :)

@borondics
Copy link
Member

A suggestion: the parent canvas display should show if in the child canvas the input or output are disconnected.
image
Here we can only see that the last widget doesn't receive data, but no indication of why.

And the corresponding child node:
image

I suggest putting the little exclamation sign either at the input shield (input not connected inside the node), on top of the Macro node widget/icon (problem somewhere inside) or the output shield (output not connected inside).

@borondics
Copy link
Member

Another observation about output connections and labeling:
This workflow can be added to a Macro node.
image

Resulting in this:
image

Inside the node:
image

Notice that in the expanded workflow there are 3 connections, which are nicely preserved after creating the Macro node but once opening the node one connection gets lost.

For naming: Could the data channel labels say something about the outside/inside world? What I mean, instead of Data (1) on the inside could we indicate the name and channel of the widget that channel represents? In this case it could be "File/Data". Same would be nice when exiting the node, which in this case would be "Data Table / Selected Data".

Finally, it seems that currently, after creating the node, the node inputs/outputs can't be changed, so for example if I open the node and try to connect "Data Table (2)" to "Selected Data (1)" the new connection replaces the original "Data Table"->"Selected Data (1)" channel instead of adding a new one. This should be dependent on whether the widget on the outside of the node can receive multiple channels and then the question should be which input the new channel should be connected to.

I see that this must be difficult to handle, just giving my thoughts here.

Should be deprecated and removed.
Due to deprecated nodes/links/annotations properties
Does not conform to standard guidelines, wastes space and the implementation is ugly.
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.

3 participants