Moving from a ValueTree

General JUCE discussion
1

So, iiuc, copying a ValueTree means that the copy will refer to the same underlying data as the original ValueTree, but it will have no listeners (so the original ValueTree's listeners are not copied). That’s maybe a bit unintuitive at first, but OK – it’s a useful property.

But here’s something really confusing: what are the move semantics of a ValueTree? If I move-from a ValueTree, will the new tree also have no listeners? I can’t figure that out by looking at the JUCE code.

This is crucial because I have a class – let’s call it Widget – that has an internal ValueTree member, and is listening to that. (for example, CachedValue is doing the same thing, I think it’s a reasonably common pattern).

Now I need to have a std::vector of Widgets. Therefore, Widget needs to have a non-throwing move constructor.

How do I correctly implement this for Widget, without producing Widgets that are broken because they don’t listen to their internal ValueTree anymore? (I noticed that CachedValue has an implicitly declared move constructor, isn’t that broken, too?)

Note also that ValueTree::addListener isn’t noexcept, therefore it can’t be used in a move constructor.

7 Likes
2

Did you ever figure this out? I’m struggling with the same thing.

3

I am not sure, if that is properly solvable, since when a class adds itself as listener to a ValueTree, and the ValueTree is copied including the listeners, how would the listening class know, it has to deregister from two ValueTrees on destruction?

I think a silent copying of listeners can be just as unintuitive as not copying it.

4

imho the solution would be to implement a proper move constructor for ValueTree, which moves the tree’s listener list over into the new tree instance. Since this does not require any listeners to be added to or removed from a listener list, it can be made noexcept.

This would leave the moved-from ValueTree without any listeners, which is fine.

3 Likes
5

I’m curious. Why do questions like this get zero attention from the JUCE team? Is it because being able to move from value trees is just something they think you shouldn’t do, or just something they never had to do, and thus didn’t think it would need to be implemented?

@timur can you show what a proper move constructor for ValueTree would look like?

1 Like
6

OK, so the current ValueTree move constructor looks like this:

ValueTree::ValueTree (ValueTree&& other) noexcept
    : object (std::move (other.object))
{
    if (object != nullptr)
        object->valueTreesWithListeners.removeValue (&other);
}

This does not provide what I would consider correct move semantics, for the reason explained above: the new ValueTree does not have the Listeners of the moved-from object. What I think this should do is simply:

ValueTree::ValueTree (ValueTree&& other) noexcept
    : object (std::move (other.object)),
      listeners (std::move (other.listeners))
{}

provided that ListenerList has a correct nothrow-move-ctor (which I do not have the time to check right now).

2 Likes
7

I mean, that’s not 100% clean either, because then moving a ValueTree will give the new tree the listeners of the old, but copying a ValueTree will not give the new tree the listeners of the old. It almost feels like the semantics of copying a ValueTree inherently do something else than copying (reminds me a bit of std::auto_ptr), and that makes_moving_ a ValueTree more odd.

My guess is that the JUCE team simply does not want to mess with the semantics of such a widely-used class :wink:

3 Likes
8

why can’t there be a move assign operator that copies correctly if you do something like this?

ValueTree otherTree ("SourceTree");
ValueTree tree ("CopyTree");
tree = std:move (otherTree);

?

@jules @ed95 @t0m Why does the ValueTree class have

/** Move constructor */
    ValueTree (ValueTree&&) noexcept;

but no Move Assign operator?

you guys didn’t follow the rule of 3/5/0 here…

Rule of five
Because the presence of a user-defined destructor, copy-constructor, or copy-assignment 
operator prevents implicit definition of the move constructor and the move assignment
operator, any class for which move semantics are desirable, has to declare all 
five special member functions:

https://en.cppreference.com/w/cpp/language/rule_of_three

3 Likes
9

There’s certainly an element of that; we’ll need to spend some time thinking over the best way to do things. No one has commented on this as there’s not much to say - it’s clearly something we could do better, and looking at it has gone on the backlog.

3 Likes
10

Just spent an hour to figure out what’s going on… before to understand that the move constructor of a ValueTree does NOT preserve the listeners. :dizzy_face:

11

Yeah, it got me pretty bad too. Tried to store a ValueTree in a std::vector and it went to hell pretty damn fast. :smile:

1 Like
12

This has tripped me up countless times but eventually you will just remember that copying a ValueTree won’t copy the listeners.

Personally I’d say that copying a ValueTree should not copy the listeners, as this would lead to cases when listeners would recieve callbacks from multiple trees pointing to the same data so anytime a property changed you’d recieve 2 callbacks, which would rarely be useful.

For moving, I can see the benefit of also transferring over the listeners, however that leaves you in a situation where the listeners don’t necessarily know what opbject they’re listening too. I very often have some sort of assertion in listener callbacks to check that the object that triggered the callback is infact the one I expected. Maybe that could be solved with an additional valueTreeMoved() callback for the listeners.

My preference would be to make this very verbose by not defining a move constructor or move assignment operator, but instead having moveTo() and/or moveFrom() methods. This would also allow you to explicitly state what behaviour you want for moving listeners, e.g.:

juce::ValueTree treeOne {"One"};

juce::ValueTree treeTwo;
treeOne.moveTo (treeTwo);
treeTwo.addListener (this);

juce::ValueTree treeThree;
treeTwo.moveTo (treeThree, juce::ValueTree::moveWithListeners);
// or
treeTwo.moveTo (treeThree, juce::ValueTree::moveWithoutListeners);
2 Likes
13

I was typing something similar to @ImJimmi. As a long time VT user I obviously have it’s behaviors pretty well understood, and I certainly went through all the growing pains. Including misunderstanding that the callbacks themselves were not part of the underlying data, ie. take a copy of a VT, add a listener, and not having it be part of the original VT.

Since Jules isn’t around I am not sure how easy it would be to understand why the callbacks weren’t originally moved… but very possibly because move ctors weren’t around when VT’s were implemented? From an engineering perspective it seems important to understand why it was implemented how it was, before making the change.

There will certainly be issues with moving a VT from one class to another, if the callbacks refer to the original class, but this is already true for all classes that are moveable with listeners (Components?, etc?). So it seems that isn’t a good argument against it. @ImJimmi 's explicit moving of listeners offers some protection against that.

I just looked at the docs for the VT move ctor and it does not mention anything about the listeners. I think this could be the first fix applied, ie. document the behavior. As precedent, the docs clearly specify that taking a copy of a VT does not copy the data, and that you need to use createCopy to do that. if it weren’t documented, people would certainly complain about it as unexpected.

6 Likes
14

:+1: