Inconsistent API: FlowScene::selectedNodes() and FlowScene::nodes()

[DexterMagnific]

Hi,

The returned types of these two methods is not the same, as a consequence it is hard to work with the API.

My use case is that I want to do some work on the selected nodes. If no nodes are selected, then I do it on all the nodes. With this API, I cannot use the code below:

selection = scene.selectedNodes();
if ( selection.size() == 0 ) {
    selection = scene.nodes()
}

Please make a consistent API, whether I all the nodes or the selected ones.

Thanks

[Daguerreo]

You can just take the Nodes from the nodes() method.

[DexterMagnific]

I don't understand. nodes() and selectedNodes() do not return the same type. I'd like to use the same variable for either storing the selection or all the nodes.

[paceholder]

selectedNodes() can't return the same map with unique_ptr<Node> values, because such values are non-copyable.

On the other hand, nodes() is a simple getter returning exactly what is stored inside the class.

There are standard C++ tools to take iterate the map and extract saved values from pair.

It would cost you a couple of lines of code that could be stored on your side and used throughout your code.

[DexterMagnific]

I'd like you to understand my problem regardless of technical implementation details.

My problem is that I want to apply some processing on the selection. If there is no selection, I'd like to apply it on all the nodes. My processing is aligning nodes together.

With the API you provide, I cannot use a single variable to store either the selection or all the nodes. I must write two identical codes that do the same thing but on two different types.

In addition, I think that exposing "map of unique_ptr" is exposing implementation details. The one who uses your library has never inserted a unique_ptr into the scene.

The FlowScene::createNode() returns a &Node so I consider that the thing that is inserted is a Node. So why can't I get a list of Node when trying to get all the scene nodes ? Whether you internally store the Nodes as a map of unique_ptr is not something I (the programmer) must handle.

Do you follow the reasoning ?

[paceholder]

I am convinced

Added:

std::vector<Node*> allNodes() const;

[DexterMagnific]

Fantastic !