refactor: Categorical NodeRegistry and updated QML Graph menu - #2582
Conversation
| QHash<int, QByteArray> EnumOptionsModel::roleNames() const | ||
| { | ||
| QHash<int, QByteArray> roles; | ||
| roles[Qt::DisplayRole] = "option"; | ||
| return roles; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a reason to rename the display role as "option" instead of just using "display" in the QML?
There was a problem hiding this comment.
I didn't know you could do that, but it sounds sensible and would make this cleaner (no need for a custom roleNames override)
| graphModel: graphRoot.rootGraphModel | ||
| posx: Math.round(ctxMenuCatcher.mouseX) | ||
| posy: Math.round(ctxMenuCatcher.mouseY) | ||
| Instantiator { |
There was a problem hiding this comment.
I'm a bit more inclined to use a Repeater here. The Instantiator is useful when you're adding and removing nodes at runtime. Since the number of nodes and categories are fixed, we can just use a repeater. As an added bonus, we can drop the onObjectAdded and onObjectRemoved
There was a problem hiding this comment.
Good point👍
| } | ||
|
|
||
| // Static Singletons | ||
| const std::map<NodeRegistry::Category, ProducerMap> NodeRegistry::categoricalProducers_{ |
There was a problem hiding this comment.
I'll admit that I had considered a slightly different architecture. Specifically, I had though that we would add a virtual category method to the Node class which returns a Node::Category (since this would require that the Category enum be moved out of NodeRegistry into Node). This map would then just be a vector and the NodeRegistryModel would filter the list by the category of each node type to ensure that everything was listed in the proper categories.
Honestly, though, I'm not sure that my architecture is any better, but rather just different. The main advantage would be that it would be impossible to forget to assign a category to a new Node class, since the unimplemented virtual method would be a compiler error. However, it would still be possible to forget to add the node to the registry vector, which gets us the very same problem. The only real advantage is that Nodes would directly know their own category, but I'm not sure that will every be useful.
This PR puts the responsibility on the C++
NodeRegistryto store Node categories (which had previously been hardcoded into a QML menu) by an enum type. ThiscategoricalProducers_map is then flattened for more general use throughout the application, for retrieving any node. The categories are used to generate the context menu that appears when right-clicking in the Graph, and nodes can be created in that way.