[FR] ValueTree::addChild without specifying index


#1

I'm not sure I understand why ValueTree::addChild() requires you to specify an index...

Why are we not able to arbitrarily add children, simply expecting such to be in the order they were added?

e.g.:

void addChild (const ValueTree& child, UndoManager* undoManager);​

void ValueTree::addChild (const ValueTree& child, UndoManager* const undoManager)
{
    addChild (child, getNumChildren(), undoManager);
}

#2

The ValueTree is an ordered array, not an unordered set. Its children are analogous to XML sub-elements, and their order will in many cases have some intrinsic meaning.

That's why it's important to be able to specify the index, and why I think it's a good idea to make people aware of where the child will go when it's added. 

It's a valid feature request though, as appending rather than inserting is probably the most common way that it's used. Maybe a new method called "appendChild" would be good?


#3

appendChild sounds sensible.


#4

After all this time, could still use appendChild :slight_smile:


#5

Wouldn’t this just call ValueTree::addChild() with an index of -1 ?


#6

Probably. As it is, the API is goofy. If it was really to be similar to XML, specifically XmlElement::createNewChildElement, it should follow suit with something like:

auto child = myVT.appendChild (childIdentifier, &undoManager);

#7

I’m not sure I like this. It overcomplicates the API. People new to the class will have to determine the difference between “appending” and “adding” a child.

If anything, this should be a free function:

void appendChild (ValueTree& parent, const ValueTree& child, UndoManager* um)
{
    parent.addChild (child, -1, um);
}

#8

I’d argue that having that extra bit of code floating around points to a missing piece of the overall puzzle.

From what I can tell so far, the typical ValueTree managing scenario goes like this:

  • create an Identifier
  • create a parent ValueTree with said Identifier
  • create another Identifier
  • create a new ValueTree with this said Identifier
  • add child to the parent

I’ve seen fewer cases that require an index than the other way around, so am foregoing that part.

I think it’s dreadfully long winded - all of that can be encompassed with an appending method. I don’t understand how you see the suggestion as confusing or overcomplicated. It’s an extension or overall simplification aimed towards the typical use case, basically helping do away with the typical boilerplate C++ nonsense.

Basically, as Jules mentioned above:


#9

Yeah, a quick grep of our repo shows 25 places where there’s an addChild (…, -1), and where appendChild would make the code more understandable, so I think that’s good enough for me! Will push something shortly…


#10

Great! I believe in my code I have 99% addChild(…, -1) and I will be happy if I can replace all those calls with a more descriptive method.


#11

I’m not saying that it’s not a valid addition but I think placement has an important role to play here.

Firstly though, your 6 step example isn’t usually how you would create and add a child to a parent. There’s no reason you can’t just do this (assuming all your Identifiers are static which they should be for performance reasons).

ValueTree parent (IDs::TRACK);
parent.addChild (ValueTree (IDs::CLIP), -1, nullptr);

So it’s really only two steps. (Removing explicit from the ValueTree constructor would also remove the need to specify that in the second line but maybe there’s a reason for this I’m missing…).


However there are good reasons to prefer non-member non-friend functions to member functions as I previously stated. For a start these are two good articles:

http://www.gotw.ca/gotw/084.htm

What I worry most about however is that providing functionality only as member functions has some serious implications on how we reason about a class.
Firstly, keeping only the minimum amount of member functions keeps the API small so it’s easy to see what a class “can do”. This is the base level for abstractions we’re building. It provides a small, single place to quickly ascertain the core functionality of a class.

ValueTree is actually a really good example where this is important because it’s useful to understand its transactional nature (you can set a property, add/remove/reorder children and re-assign a tree). Each of these things has a corresponding listener callback. The more methods the class has, the more unclear this transactional nature becomes.

To me, it’s a lot more conceptually clean to think of what a class “can do” and then provide a number of non-member functions to aid common tasks. This is effectively a second layer of abstraction and tells us not what a ValueTree “can do” but what “we can do with” a ValueTree.


My real reason for disliking adding member functions is a bit simpler than all that though. If things in JUCE are only ever provided as member functions it’s easy for users to only ever use the APIs provided and miss that they can add their own layers of abstractions with non-member functions. This seems to be an increasing issue over the past year or so with a slew of requests that often end in “why don’t you write a free function?”. If common use cases are provided as free functions in JUCE, it’s not so much of a jump for users to realise they can write code in a similar way.

Also, things are far more likely to be added as non-member functions rather than bloat the core class API. There’s at least 3 versions of appendChild I can think of which could be useful:

/** Returns the parent so you can chain multiple children additions easily. */
ValueTree appendChild (ValueTree& parent, const ValueTree& child, UndoManager*);

/** Returns the child so you can act on that*/
ValueTree appendChild (ValueTree& parent, const ValueTree& child, UndoManager*);

/** Returns the new child. */
ValueTree appendChild (ValueTree& parent, const Identifier& childID, UndoManager*);

And while we’re at it, it’s probably useful to have versions either without UndoManager parameters or have them default to nullptr in the interest of terseness.
Plus we could have versions that insert at the front, similar to push_front.


I appreciate that I’ve derailed this thread a bit and I don’t really have a problem with adding the method you’ve suggested but I think it does start a larger conversation about the best ways of adding things and the implications they have on how users view a class.

I think it is a benefit of JUCE that it does force you to think about the functionality of a class by forcing the index and UndoManager when adding children. Similarly I think that having all the listener callbacks pure virtual is useful (again it’s very easy to write named abstractions if you need only a single callback. I know I’ve personally avoided bugs by being forced to consider these things.

One final thing is that if we do ever get unified call syntax the use of these two methods would be identical (although judging by recent standard meetings it looks unlikely we’ll ever get that due to overloading additions that can break code…).

Once we make the initial break of insisting everything has to be a member function it really does open up a lot more doors and abstraction possibilities… :grin:


#12

Just a little extra on this point, there’s a talk from CppCon this year which outlines a lot of the arguments for free functions: https://youtu.be/WLDT1lDOsb4
Worth a watch.


#13

I’m certainly open to the mindset, and do understand where it’s coming from.

My main apprehension in adopting this way of thinking for everything is discoverability: with all sorts of free functions for special cases, how do you get a user to use the free function over them writing their own that does the same thing? (eg: Having a user always dig in the class’ header or Doxygen to find out if what they want to do is already there?)


#14

I think the standard thing to do is just put it directly under the class declaration. Then you’ve only got the private section in the way and it’s fairly obvious to just scroll down a bit. All the free functions should still have documentation etc. so they won’t look like internal gubbins.

Personally I’d always look in the library for a function that does what I want first. if it’s not with the class declaration then I’d start to write my own. The workflow is the same as looking in the class header for a function, just a little more scrolling.

I do agree tooling could do a little more here. I guess it’s hard to do auto-complete when you start by typing the name of the function as there’s no object to do a lookup on but to be honest I’ve never really noticed that missing. Putting functions in appropriate namespaces gets you most of the way there with code completion.