FR : some private members of ComboBox class should be protected


#1

Hello to the JUCE team !

I have a feature request. In a recent attempt of using the virtual function showPopup() from the class ComboBox in my own custom class, I realized that it is not possible to copy and paste the code from the original function in the new one, because the variables being used are all private.

Would it be possible to make the members menuActive, currentMenu, label and noChoicesMessage protected instead of private ?

Thanks in advance !
Ivan


#2

Would it be possible to make that change @jules, @ed95 or @tom ?

Thanks in advance :wink:


#3

Hi Ivan - I just saw your suggested change but nope, no way: changing internal implementations to be public is one of the big no-noes of library design. An absolute golden rule is to never give users access to internal data because that means we can never change the way it’s implemented.

What are you actually trying to achieve? There’s probably some other method we could expose that makes more sense.


#4

Wait, I don’t want it to be public at all, I want it to be protected, so I can just copy and paste the content of the function showPopup() in my own inherited class. Would it be a problem to do so ?

The original code in this function is this :

void ComboBox::showPopup()
{
    if (! menuActive)
        menuActive = true;

    auto menu = currentMenu;

    if (menu.getNumItems() > 0)
    {
        auto selectedId = getSelectedId();

        for (PopupMenu::MenuItemIterator iterator (menu, true); iterator.next();)
        {
            auto& item = iterator.getItem();

            if (item.itemID != 0)
                item.isTicked = (item.itemID == selectedId);
        }
    }
    else
    {
        menu.addItem (1, noChoicesMessage, false, false);
    }

    auto& lf = getLookAndFeel();

    menu.setLookAndFeel (&lf);
    menu.showMenuAsync (lf.getOptionsForComboBoxPopupMenu (*this, *label),
                        ModalCallbackFunction::forComponent (comboBoxPopupMenuFinishedCallback, this));
}

The reason I needed this is because I want the combobox to display a popup menu with a fixed size / font size which is not depending on the combobox size. With the current implementation, that function uses lf.getOptionsForComboBoxPopupMenu with the label as an argument, and I have not found another way to get the wanted result other from customizing the options provided there, hence that topic.


#5

Protected code is public!

If members of the public can write code which depends on a variable, then as far as the library is concerned, that variable is now public! I don’t want you to write code that messes with or relies on those variables, regardless of what kind of class you use to do it.

It sounds like what you’re trying to do belongs in a L+F class? I’d be happy to add a method if you could suggest one that does what you’re looking for.


#6

Ok fair point, I thought that it would make sense to make these variables protected, since you are using them in a function declared as virtual, but I guess I should find a way to do that with a LookAndFeel method.

Let me think about it, I’ll tell you when I find the new L+F function that will make me happy :wink:


#7

These Frankenstein controls (ComboBox being essentially a wrapper around a Button and a PopupMenu pair) are all a bit of a nightmare to L&F. The L&F for the popup devolves to whatever you’ve set your popup L&F to be, IIRC; there’s no intermediary specifically for ComboBox, best I can tell.

If you want a fixed or scaling font size for the button itself, something like this L&F method worked well for me:

Font getComboBoxFont(ComboBox& box) override
{
     return { box.getHeight() * 0.56f };
}

In that case, I’m scaling the box in resized(); I empirically determined the 0.56 value to keep the font in line with other controls.


#8

OK so finally I have been able to get the result I wanted using LookAndFeel:: getOptionsForComboBoxPopupMenu() and LookAndFeel::getComboBoxFont() :dunce cap:

I agree that the ComboBox component specifically is quite difficult to customize with the LookAndFeel classes because you have to change a lot of different functions and variables to get what you want, but at least it was enough as it is designed to achieve what I wanted. Thanks for the help guys, and @jules don’t forget to look into my merge request when you have some time :wink: