Skip to content

Conversation

@tatatupi
Copy link
Collaborator

@tatatupi tatatupi commented Jun 22, 2025

The idea is to have a NodeValidationState member object in the NodeDelegateModel class, 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

  • Introduces NodeValidationState struct class.
  • Getter and setter in NodeDelegateModel for validation.
  • DataFlowGraphModel handles NodeRole::ValidationState updates.
  • Painting logic reflects invalid states in DefaultNodePainter.
  • Tooltip shows validation message in NodeGraphicsObject.
  • Calculator example demonstrates usage.

image

Solves #226
Solves #75

P.S.: some changes happened automatically when saving files, due to autoformatting using nodeeditor clang format style.

@tatatupi
Copy link
Collaborator Author

I also changed the linux-gcc qt version to 6, so that QtGui/QUndoCommand include could work

@tatatupi
Copy link
Collaborator Author

I will change the coloring scheme to color only the node border.

@Llcoolsouder
Copy link
Collaborator

I also changed the linux-gcc qt version to 6, so that QtGui/QUndoCommand include could work

You shouldn't need to change the Qt version for this. You can support both by changing the include to just #include <QUndoCommand> like I've done in this PR https://github.com/paceholder/nodeeditor/pull/476/files

Cool feature by the way!

Copy link
Collaborator

@Llcoolsouder Llcoolsouder left a 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.

@tatatupi
Copy link
Collaborator Author

tatatupi commented Sep 2, 2025

Thanks for the inputs, @Llcoolsouder. I have made the changes you requested.
I also changed the UI, as I mentioned in a conversation I had with @paceholder. It now looks like this:

image

@tatatupi tatatupi merged commit ff23d00 into paceholder:master Sep 2, 2025
3 checks passed

QIcon icon(":/info-tooltip.svg");
QSize iconSize(16, 16);
QPixmap pixmap = icon.pixmap(iconSize);
Copy link
Owner

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"};
Copy link
Owner

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>()) {
Copy link
Owner

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.

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