-
Notifications
You must be signed in to change notification settings - Fork 937
Adds validation state feedback to nodes #475
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
Conversation
Integrate upstream
|
I also changed the linux-gcc qt version to 6, so that QtGui/QUndoCommand include could work |
|
I will change the coloring scheme to color only the node border. |
You shouldn't need to change the Qt version for this. You can support both by changing the include to just Cool feature by the way! |
Llcoolsouder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything appears to be building and working for me.
Just that one note above about the QtGui/X.h includes. Doesn't look like you've added any of those includes here, so this should build with or without the version change. If anything I'd recommend adding both major Qt versions back to the build matrix, but not strictly necessary.
9a48404 to
2e91091
Compare
|
Thanks for the inputs, @Llcoolsouder. I have made the changes you requested.
|
|
|
||
| QIcon icon(":/info-tooltip.svg"); | ||
| QSize iconSize(16, 16); | ||
| QPixmap pixmap = icon.pixmap(iconSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would eat CPU resources (maybe very insignificantly) for converting the icon in each rendering pass.
Maybe we could have used QGraphicsSvgItem that has caching and it also could render the icon smoothly when zoomed-in. What do you think?
| void drawValidationIcon(QPainter *painter, NodeGraphicsObject &ngo) const; | ||
|
|
||
| private: | ||
| QIcon _toolTipIcon{"://info-tooltip.svg"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this one is not needed in the header file, as we instantiate the icon one more time in the rendering function in *.cpp. Or vice versa
| : nodeStyle.NormalBoundaryColor; | ||
|
|
||
| auto validationState = NodeValidationState::State::Valid; | ||
| if (var.canConvert<NodeValidationState>()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we shouldn't check if we can convert it everywhere.
If we call model.nodeData<NodeValidationState>(...) it will return the converted value from QVariant or the defaul-constructed value for this type. Which is "valid + empty message" for NodeValidationState.

The idea is to have a
NodeValidationStatemember object in theNodeDelegateModelclass, that defaults to a valid and empty state message. The API allows for updates to be made accordingly, and the UI will respond automatically.UI will show a node red (error) or yellow (warning). A tooltip is added with the state message. And a small icon for information is displayed as a badge, to help the user access the tooltip. I added a screenshot of the calculator example to help discussions. in the example, the tooltip for the Warning state is displayed.
Key Changes
NodeValidationStatestruct class.DataFlowGraphModelhandlesNodeRole::ValidationStateupdates.DefaultNodePainter.NodeGraphicsObject.Solves #226
Solves #75
P.S.: some changes happened automatically when saving files, due to autoformatting using nodeeditor clang format style.