I’m suprised that ValueTree doesn’t have a hasListener method.
Is there something obvious i didn’t see?
It seems to me that the ValueTrees are designed so that you hold the ValueTree as a member variable and then add the listener during construction, and you don’t really need to know whether the same node has any other listeners (at least I’ve never found myself in a position where I needed to know). What’s your use case here?
2 Likes
No much I can add to that - other than that the ListenerList is checking if the listener instance is already added to the list. So if you are in deed adding and removing listeners for some reason (I literally never removed a listener form a value tree), you are protected against having your listener called twice on the same change.
1 Like
The problem comes from that listeners are not preserved while moving ValueTree.
In complex case (e.g. a filtering synchronizer) a way to check if a listener has already be binded would have make my initialization process easier.
For now i rely that duplicate listeners are checked when added/removed.
But IMHO it would make my code more clear with an explicit check.
Can you wrap your class in a unique_ptr and then move that instead? That way the ValueTree move constructor wouldn’t get called in the first place.
1 Like
I guess i could. But add an extra layer of indirection (and memory allocation) is something i would like to avoid at first if not required. But yes it’s a valid approach. 
1 Like
I know that feeling, and I’ve definitely spent lots of time fretting about how much memory allocation is going on in my code. But if it makes you feel any better, moving a unique_ptr is just a single pointer swap, whereas moving a stack allocated object can involve a lot more work (as you’ve encountered). So if you’re creating the object infrequently and then moving / swapping it frequently, using unique_ptr could possibly be more efficient. Of course I don’t know whether or not this is what is happening in your code, but it’s something to think about.
OT: this is only ok, if the listener is definitely destroyed after the ValueTree, e.g. the ValueTree is a member of the Listener.
I agree. I would rather move the object containing the ValueTree around than a heap allocated ValueTree. But that is personal preference, just because I prefer to encapsulate stuff and don’t pass around generic structures.
Besides ValueTrees as member variables, what other options are there? Either you allocated the ValueTree on the heap (which is a Bad Idea). Or it’s on the stack and about to go out of scope, in which case adding a listener is useless (unless you’re using multiple threads, which is another Bad Idea).
A member variable of a different class:
struct A
{
juce::ValueTree tree;
};
struct B : juce::ValueTree::Listener
{
};
A a;
B b;
a.tree.addListener (&b);
B can go out of scope and a.tree would call a dangling pointer.
That’s why ideally B should have a reference to the ValueTree and deregisters itself in it’s destructor.
Otherwise you need to somehow guard the lifetime of B manually.
And if your app is heavilly based onto ValueTree (as shown in Dave Rowland’s talk) there’s design like @daniel example above!
And note that if you std::move the owner of struct A and struct B the binding is removed.
Why not just give B a copy of the ValueTree and let it listen to that?
I’m not so sure about it. Can you maybe supply a better example how you actually run into that pit? My main problem is, that ValueTree listeners are scrubbed when using a std::vector to store them. E.i. one object subscribing to a variable number of value trees with an uncommon parent is not easily possible right now. Only with the unique_ptr hack.
But I don’t think @Daniel example is in itself very good. He already pointed out, that B should carry his own instance of the value tree it is interested in. I think this is kind of the one main design principle behind the ValueTree and Value. Low cost when copying so every one can get a piece of it and decide what to do when stuff changes.
That of course comes with some challenges. E.g. the execution order of the listeners is often undefined. But I got very comfortable with that and designed my applications in a way, that it doesn’t require a specific order of execution for the listeners.
I think I didn’t get my point across.
B cannot have an instance of the ValueTree. It could instead have a reference to the ValueTree that keeps the Listeners in order to deregister itself in the destructor.
In any situation where the Listener and the one owning the ListenerList are not somehow related in terms of lifetime, you need to manually make sure a listener is removed before it goes out of scope.
Since you mention vectors, let’s expand:
struct A
{
juce::ValueTree tree;
std::vector<std::unique_ptr<B>> bees;
};
struct B : juce::ValueTree::Listener
{
};
A a;
auto b = std::make_unique<B>();
a.tree.addListener (b.get());
a.bees.pushBack (std::move (b));
a.tree.addChild (...); // is fine
a.bees.clear(); // b is gone, a.tree contains a dangling pointer to the b instance
a.tree.addChild (...); // the listenerList calls a dangling pointer to b
It is your obligation to make sure b is removed from the ListenerList.
That is indeed what I usually do. The local ValueTree gets the same events like the one it shares the data.
You can’t add the same listener multiple times, so if you wanted to ensure a listener is listening to a certain tree you could simply call addListener() again.
IMO, it’s bad practice for different listeners to know about each other. Only the listener itself should ever know what it is listening to, and should manage what objects it does or doesn’t listen to itself. For this reason I always make sure to inherit from Listener classes privately, and an object’s listener list should always be kept private with the only listener-related methods on its interface being add/remove.
Thinking about it a little more, maybe an alternative solution would be to instead expand the interface on the listener to be able to check if it listens to a particular object, e.g.:
// Instead of
tree.hasListener (&object);
// Prefer
object.listensTo (tree);
This might expose fewer details about the object being observed, and is possibly closer to what you actually want to know in this situation which is whether or not one object is currently observing another.
I don’t think we get any further in this discussion with using these abstract examples. In my opinion, the examples violates the design principle of the ValueTree in a sense that B should carry his own ValueTree pointing to the same underlying data as you wrote.
My problem with a the ValueTree is that std::vector<ValueTree> is not working. And therefore, as a side affect, std::vector<A> is also working, if A has a member of a ValueTree.
I’m not saying, that there isn’t one example where one runs into this issues of listener live time, I just can’t think of one, where it would make not only sense in my code but also makes it better either in design or in readability. For me, you (all be it very basic example) exposes multiple issues that makes me avoid that in it’s entirety:
- A is exposing the ValueTree to the public. I use the ValueTree in my objects to store all the data everything I’d normally do with members, so I automatically achieve the observer design pattern with all its benefits. But if I make my ValueTree public, that’s like making all my members public (which I’d normally not do when not using a valuetree) but it’s even worse – now I’m not even sure if my “members” exist or have the type I expect them to have. With the ValueTree, I’m basically getting rid of all strong type guarantees C++ makes. So first thing is I write a wrapper class, that brings that functionality back although behaving the same as a ValueTree (you can add listeners to it and copy it freely around without worrying about live time)
- B is exposing the fact, that it is a ValueTree::Listener. The B instance has no control over the object it is subscribing, the caller can do what every they want with it with no safety measurements what so ever. Even when B would inherit from my wrapper listener (e.g. A::Listener), that ensures all my types etc. are in line, I try as much as possible to make that a private “attribute” of B. There might be some valid use-cases for inheriting a listener class public, but I found that writing the code in a way that there is no need for it, turns out to be better (longer durable, easier to read, easier to test and – if it all goes south – easier to debug).
I’m not sure you really want to read all the code necessary to illustrate the case. 
Note that it is required for an editable inspector (objects can be changed/deleted at the same time).
You can’t add the same listener multiple times, so if you wanted to ensure a listener is listening to a certain tree you could simply call
addListener()again.
By the way that is what i have finally done.
You mean by that, that an object is capable of destroying itself (/remove itself from its parent collection)? Let me just show you how I’m doing it. So the scenario being: we have one collection object (in this case ProgramInlayCollection) and an object being “collected” (that will be ProgramInlay). The ProgramInlay will have attributes (any kind of let’s say “primitive” attributes – ints and stuff). So for a possible Inspector component, it just holds a ref to ProgramInlay and can modify by user input and register a ProgramInlay::Listener to it so the inspector component gets updated when the ProgramInlay gets changed from anywhere else in the application. And last but not least, the inspector should present a button “delete” and by that, the ProgramInlay needs to be able to delete itself so the inspector component doesn’t need a ref to ProgramInlayCollection, which would hijack all of our beautiful design work.
So here it goes:
class ProgramInlay
: private ValueTree::Listener
{
public:
class Listener
{
public:
virtual ~Listener() noexcept = default;
virtual void attachingCtrValChanged(juce::TCtrVal);
/* and other methods for members */
};
ProgramInlay();
explicit ProgramInlay(const ValueTree& vt);
ProgramInlay(const ProgramInlay& other);
juce::TCtrVal getAttachingCtrVal() const;
/* and more getters */
void setAttachingCtrVal(juce::TCtrVal value, UndoManager* undo = nullptr);
/* and more setters */
void deleteSelf(UndoManager* undo = nullptr);
void addListener(Listener& listener);
void removeListener(Listener& listener);
private:
void valueTreePropertyChanged(ValueTree&, const Identifier& property) override;
void valueTreePropertyChanged_attachingCtrVal();
ValueTree valueTree;
ListenerList<Listener> listeners;
CachedValue<int> attachingCtrVal;
/* and more CachedValues for more members */
friend class ProgramInlayCollection; // this is necessary, so the collection can mess with the internal valueTree. We take that price, as both classes go and in hand and when we are not exposing anything in ProgramInlayCollection (which we don't), there is no "harm" in doing it.
};
And here are the important implementations:
ProgramInlay::ProgramInlay()
: ProgramInlay(ValueTree(IDs::PHRASE_PROGRAM_INLAY))
{}
ProgramInlay::ProgramInlay(const ProgramInlay& other)
: ProgramInlay(other.valueTree)
{}
ProgramInlay::ProgramInlay(const ValueTree& vt)
: valueTree(vt) /* hook up CachedValues */
{
jassert(valueTree.hasType(IDs::PHRASE_PROGRAM_INLAY));
valueTree.addListener(this);
}
void ProgramInlay::valueTreePropertyChanged(ValueTree&, const Identifier& property)
{
if (property == IDs::attachingNoteNr)
/* call listeners registered to this node */
else
jassertfalse;
}
void ProgramInlay::deleteSelf(UndoManager* undoManager)
{
jassert(valueTree.getParent().isValid()); // Can't delete an inlay that's not in any collection
valueTree.getParent().removeChild(valueTree, undoManager);
}
And then we also get the collection class:
class ProgramInlayCollection
: private ValueTree::Listener
{
public:
class Listener
{
public:
virtual ~Listener() noexcept = default;
virtual void inlayAdded(const ProgramInlay& inlay);
virtual void inlayRemoved(const ProgramInlay& inlay, int index);
virtual void inlayChanged(const ProgramInlay& inlay); // That is just for convenience and a side product. If we really want to now what goes on, we should subscribe to a ProgramInlay directly.
};
ProgramInlayCollection();
explicit ProgramInlayCollection(const ValueTree& vt);
ProgramInlayCollection(const ProgramInlayCollection& other);
ProgramInlay createNewInlay(UndoManager* undoManager = nullptr);
void deleteInlay(const ProgramInlay& inlay, UndoManager* undoManager = nullptr);
void addListener(Listener& listener);
void removeListener(Listener& listener);
private:
void valueTreePropertyChanged(ValueTree& vt, const Identifier& property) override;
void valueTreeChildAdded(ValueTree&, ValueTree& child) override;
void valueTreeChildRemoved(ValueTree&, ValueTree& child, int ixChild) override;
ValueTree valueTree;
ListenerList<Listener> listeners;
};
ProgramInlayCollection::ProgramInlayCollection()
: ProgramInlayCollection(ValueTree(IDs::PHRASE_PROGRAM_INLAY_COLLECTION))
{}
ProgramInlayCollection::ProgramInlayCollection(const ProgramInlayCollection& other)
: ProgramInlayCollection(other.valueTree)
{}
ProgramInlayCollection::ProgramInlayCollection(const ValueTree& vt)
: valueTree(vt)
{
jassert(valueTree.hasType(IDs::PHRASE_PROGRAM_INLAY_COLLECTION));
valueTree.addListener(this);
}
ProgramInlay ProgramInlayCollection::createNewInlay(UndoManager* undoManager)
{
ProgramInlay inlay;
valueTree.addChild(inlay.valueTree, valueTree_appendChild /* -1 as index */, undoManager);
return inlay;
}
// This method is not really needed, as deleting the inlay via its own method doesn't enable that assert to even be possible. This is an example on how the collection is capable of removing elements from itself.
void ProgramInlayCollection::deleteInlay(const ProgramInlay& inlay, UndoManager* undoManager)
{
jassert(inlay.valueTree.getParent() == valueTree);
valueTree.removeChild(inlay.valueTree, undoManager);
}
void ProgramInlayCollection::valueTreeChildAdded(ValueTree&, ValueTree& child)
{
jassert(child.hasType(IDs::PHRASE_PROGRAM_INLAY));
listeners.call([inlay = ProgramInlay(child)](auto& listener) { listener.inlayAdded(inlay); });
}
void ProgramInlayCollection::valueTreeChildRemoved(ValueTree&, ValueTree& child, int ixChild)
{
jassert(child.hasType(IDs::PHRASE_PROGRAM_INLAY));
listeners.call([inlay = ProgramInlay(child), ixChild](auto& listener) { listener.inlayRemoved(inlay, ixChild); });
}
void ProgramInlayCollection::valueTreePropertyChanged(ValueTree& vt, const Identifier& property)
{
if (vt.hasType(IDs::PHRASE_PROGRAM_INLAY))
listeners.call([inlay = ProgramInlay(vt)](auto& listener) { listener.inlayChanged(inlay); });
}
I also included the three tests, that mark the behaviour that ProgramInlay::deleteSelf is promising:
- (void) test_deleteSelf
{
ProgramInlayCollection collection;
ProgramInlay inlay = collection.createNewInlay();
inlay.deleteSelf();
XCAssertEqual(collection.indexOfInlay(inlay), -1);
XCAssertEqual(collection.getNumInlays(), 0);
}
- (void) test_deleteSelf_working_undo
{
ProgramInlayCollection collection; UndoManager undo;
ProgramInlay inlay = collection.createNewInlay();
inlay.deleteSelf(&undo);
undo.undo();
XCAssertEqual(collection.indexOfInlay(inlay), 0);
XCAssertEqual(collection.getNumInlays(), 1);
}
- (void) test_deleteSelf_calls_parent_listeners
{
DECLARE_Spy_Listener_Explicit(ProgramInlayCollection::Listener, inlayRemoved) listener;
ProgramInlayCollection collection;
ProgramInlay inlay = collection.createNewInlay();
collection.addListener(listener);
inlay.deleteSelf();
XCAssertTrue(listener.__methodWasCalled);
XCAssertEqual(listener.__param<1>(), 0); // That's prev index of the inlay being removed.
}