TreeView demo bug

General JUCE discussion
1

I found a bug in the ValueTrees part of the demo. If you try to drag a node down within the same parent it gets put one position lower than it should. For example if you drag "You can drag around the nodes to rearrange them" between "...and press 'delete' to delete them" and "Then, you can use the undo/redo buttons to undo these changes" it will render after the last node not between.

This seems to fix it. Add these lines at line # 69. Basically if the new parent is the same as the old parent and the new index is greater than the old index you need to decrement the new index by 1.

if ((v.getParent() == newParent) && (v.getParent().indexOf(v) < insertIndex))
        insertIndex--;
 

 

2

Ah good point, thanks!

3

Thanks for the bugfix! I was using that code as a starting point and had already seen the weird behaviour. So this thread has probably saved me an hour of bug hunting because I thought I had done something wrong.

However when testing it all out with the fixed JUCE tip I found another problem and it's not just in the demo, it's a bug in TreeView. If you have a TreeView that does not fill it's vertical space and drag an item down into the free space below, there is no visible drag indicator and it gets moved to the top of the rootItem. This can easily be reproduced in the JuceDemo by collapsing a few items with subitems so there is space at the bottom.

IMHO the ideal thing to happen would be for the item to be moved to the bottom of the rootItem instead and ideally the drag indicator would also show just that during the drag. I achieved this by adding the following code near the bottom of the TreeView::InsertPoint constructor (line 928 of juce_TreeView.cpp in current git tip.

 else // (item == nullptr)
        {
            item = view.getRootItem();
            insertIndex = item->getNumSubItems();
            Rectangle<int> itemRect;
            if (insertIndex > 0) {
                itemRect = item->getSubItem(insertIndex - 1)->getItemPosition(true);
            } else {
                itemRect = item->getItemPosition(true);
            }
            pos.setXY(itemRect.getX(), itemRect.getBottom());
        }

 I'm not sure my solution is sound though, the idea is to return the correct insert spot at the visual bottom for dragging into empty space assuming empty space is always at the bottom. And sorry about the thread takeover.
 

4

Btw.: A reasonable case where my suggested behaviour is needed is this: Imagine an empty TreeView and you want the user to be able to populate the tree by dragging item/files onto it. 

 

 

5

Good call, thanks, I'll have a look at that today!

6

Great! thanks for the very quick fix using half as much code as I needed ;). I didn't realize the root itemposition already includes subitems. 

However it's not perfect yet and mine was slightly better. When I tested it I found a problem I've already seen when I tried to fix it. If the TreeView area is left during a drag and then reentered at the bottom, the x-coordinate of the insert point is off. The same thing can happen for fast drags. Or when dragging a file to the TreeView.

I guess the issue is that getItemPosition(true) for the root item does not include the correct inset for a 1st order child. The reason is sometimes works is because when dragging down (slowly) over the tree, the indicator just stays at the last (correct) position it got from the child and after that the indicator drawing doesn't update/redraw because the insert point does no longer change, so the position from the new code is not really used in that case. However it is used when leaving and reentering as that changes the insert pos.

Turns out all that's need IMHO is the line 

pos.x += view.getIndentSize();

at the end of the blob.

 

7

Ok, I see - thanks, I'll sort that out.

8

I saw the second commit but I'm afraid it's not working for me, the indicator is still at the wrong x position for the same cases as before (dragging out and back in). You added:

pos.x += root->getIndentX();

I believe getIndentX() is always 0 for root, so this does nothing. 

pos.x += view.getIndentSize();

would be better and it would be the same thing you do above on line 900 of the same file for dragging an item "into" another item.

9

I tested it when I made the change, and it all seemed correctly positioned.. Can you suggest some code where my fix shows this problem?

10

Ok sorry. I see it now works as it should in the JuceDemo. However it does not in my code I derived from the JuceDemo example. I'll post here again once I figure out why that is - maybe I do something stupid. 

11

Ok i see the difference now. My TreeView has rootItemVisible = false and openCloseButtonsVisible also = false. 

I dumped the values for root->getIndentX() and view.getIndentSize() for all cases in JuceDemo.

For the original settings I get:
root->getIndentX() => 20  view.getIndentSize() => 20

for rootItemVisible = false and openCloseButtonsVisible = true I get:
root->getIndentX() => 0  view.getIndentSize() => 20

for rootItemVisible = true and openCloseButtonsVisible = false I get:
root->getIndentX() => 0  view.getIndentSize() => 20

for rootItemVisible = false and openCloseButtonsVisible = false I get:
root->getIndentX() => -20  view.getIndentSize() => 20

IMHO the needed offset is 20 in all cases (for JuceDemo) and root->getItemPosition(true) does already use getIndentX(), so I still think view.getIndentSize() is the right one to use. Or maybe things are more complicated than they seem to me.

So if you want to reproduce the problem I am still seeing, add this to the ValueTreesDemo() constructor in JuceDemo:

        tree.setRootItemVisible(false);
        tree.setOpenCloseButtonsVisible(false);


 

12

Thanks - should be ok now.

13

Yes thank you! It works perfectly for me now.