Subtle issue with TreeView::restoreOpenness and threads


#1

I tracked down an interesting issue with my usage of TreeView that made restoreOpenness not work quite right and leads me to propose a minor feature enhancement.

My TreeViewItems correspond to directories, files and other system resources - which is a very common use case - so of course when you open a Node in the GUI thread, I populate the TreeViewItem in a worker thread rather than block the GUI (which can be really bad if I e g. open my itunes directory…!)

Unfortunately, that means that if I just call TreeView::restoreOpenness(), I only get the top level items opened - because opening the top level items starts reading their contents in the other thread, which doesn’t complete till a tiny amount of time later.

I feel this issue might crop up in any similar scheme and it seems to be the most common individual use case (where your boxes correspond to files or things like files).

I have two possible fixes - one in Juce and one in my code.

The Juce fix is to add a new method, virtual void TreeViewItem::prepareToOpen() that’s called by restoreOpenness() just before it actually changes the openness(). You override TreeView::prepareToOpen contain your code that might block because it’s hitting the file system. You don’t call prepareToOpen when opennessChanged() is called from the GUI…

The fix in my code is to look at whether we’re in the message thread or not, and only run in the background if we are. That’s of course what I’m going with for now.

But wait, it won’t work. :frowning: After the first level of callback from setOpenness to opennessChanged(), I’ll be back on the message thread again and lose.

So I have another workaround, not too bad, that involves me reading the opennness file myself, but I think the Juce solution is loads better…


#2

When the item gets opened, it’ll call itemOpennessChanged (true), so can’t you block in there and wait for the background thread to become ready?


#3

I hadn’t tried that in specific but I didn’t really want to block for what might be a long time - and the one-threaded code was tiny:void restoreOpenness(Node* node, const XmlElement& xml) { node->computeChildren(); node->setOpen(true); forEachXmlChildElement(xml, child) { if (child->getTagName() == "OPEN") { const String& id = child->getStringAttribute("id"); int i = 0, n = node->getNumSubItems(); for (; i < n && node->getSubItem(i)->getUniqueName() != id; ++i); if (i < n) restoreOpenness(dynamic_cast<Node*>(node->getSubItem(i)), *child); else LOG(ERROR) << "Didn't find id " << id; } } }

The reason the change suggested might be good is that it will prevent some future programmer from not even thinking of this and falling into the same trap I did…