Is the behavior of Component::addChildComponent incorrect when attempting to insert an "always on top" component below its *not* always on top siblings?

General JUCE discussion
1

Here’s the relevant code from juce_Component.cpp. The comments were added by me:

void Component::addChildComponent (Component& child, int zOrder)
{    
    // ...

    if (! child.isAlwaysOnTop())
    { 
        if (zOrder < 0 || zOrder > childComponentList.size())
            zOrder = childComponentList.size();

        while (zOrder > 0) // a *not* always on top component can't be inserted inside the block of its always on top siblings
                           // so if the requested insertion index falls inside this block, iterate backwards until an appropriate spot is found (or the beginning of the list is reached)
        {
            if (! childComponentList.getUnchecked (zOrder - 1)->isAlwaysOnTop())
                break;
            --zOrder;
        }
    }
    // no else clause, so no special action is taken if the child *is* always on top


    childComponentList.insert (zOrder, &child); // so just insert it wherever the user asked (incorrect?)
    // ...
}

Because an always on top child is inserted at whatever index the user requests, without any adjustments based on which of its siblings are/aren’t always on top, it’s possible to insert an always on top component behind components that are not always on top.
So for example, if you have a list of components like this:

{notAlwaysOnTop, notAlwaysOnTop}
 ^               ^
 [0]             [1]

and then you insert an always on top component at index zero, because addChildComponent doesn’t attempt to adjust the insertion index of always on top child components, it will get placed below its not always on top siblings in the child list of its parent

{alwaysOnTop, notAlwaysOnTop, notAlwaysOnTop}
 ^ !!!!!!!!!  ^               ^ 
 [0]          [1]             [2]

I wrote a simple test program, and confirmed that this situation does occur
Is this the intended behavior? It seems incorrect…

If you rewrite the earlier snippet like this:

void Component::addChildComponent (Component& child, int zOrder)
{
    // ..

    if (zOrder < 0 || zOrder > childComponentList.size())
        zOrder = childComponentList.size();

    if (child.isAlwaysOnTop())
    {
        while (zOrder < childComponentList.size()) // basically the same idea, but in reverse
        {                                          // search the list front to back instead of back to front
            if (childComponentList[zOrder]->isAlwaysOnTop())
                break;

            ++zOrder;
        }
    }
    else
    {
        while (zOrder > 0)
        {
            if (! childComponentList.getUnchecked (zOrder - 1)->isAlwaysOnTop())
                break;

            --zOrder;
        }
    }

    childComponentList.insert (zOrder, &child);        

    // ...
}

then always on top components never end up below not always on top components, according to my (very brief) testing

So is this a bug? Should the implementation of addChildComponent be changed?

2

You might be right, I can see the logic that always on top components should probably only be adjusted for zorder with other always on top components. That being said calling addChildComponent with a non default value for zorder while also marking the component as always on top feels like it’s bordering into user error territory. It might be helpful to understand the use case?

3

Thanks for the response Anthony

I agree, using addChildComponent like this does feel like a rather strange thing to do. Calling setAlwaysOnTop after all children have been added seems more natural imo.
But at the same time, addChildComponent clearly cares to enforce these ordering rules in the case where the child-component-to-be is not always on top, so it doesn’t feel like a huge leap to me to apply those same rules to the other case as well
Though if it does get decided that this usage of addChildComponent is erroneous, then the documentation should probably be updated to reflect that

As for my use case, there isn’t one haha. I was just studying the codebase :P. So this isn’t blocking anything important on my end

Thanks again

1 Like