Setting ComboBox's PopupMenu colors

Trying to set the colors for a ComboBox’s PopupMenu. I’m setting them in the constructor of a class derived from ComboBox, which works for other components (like setting the caret color in a TextEditor), but isn’t working here…

ThemedComboBox::ThemedComboBox()
{
    // Working
    setColour (ComboBox::backgroundColourId, Theme::mainBackground);
    setColour (ComboBox::textColourId, Theme::text);
    setColour (ComboBox::buttonColourId, Theme::mainBackground);
    setColour (ComboBox::arrowColourId, Theme::text);
    setColour (ComboBox::outlineColourId, Theme::border);
    setColour (ComboBox::focusedOutlineColourId, Theme::buttonHighlighted);  

    // Not working  
    setColour (PopupMenu::backgroundColourId, Theme::mainBackground);
    setColour (PopupMenu::textColourId, Theme::text);
    setColour (PopupMenu::headerTextColourId, Theme::text);
    setColour (PopupMenu::highlightedBackgroundColourId, Theme::buttonHighlighted);
    setColour (PopupMenu::highlightedTextColourId, Theme::text);
}

PopupMenu is not a component, so you cannot do:
popupMenu.setColour (PopupMenu::backgroundColourId, Colours::grey);
calling setColour (PopupMenu::backgroundColourId, ..); on your ComboBox won’t work either because the ComboBox does not handle the popupMenu painting and does not use the PopupMenu::backgroundColourId.

The only way to have those setColour (PopupMenu::backgroundColourId, ..) working is to call them on the popupMenu’s lookAndFeel.
In your case you have to call them on the ComboBox’s lookAndFeel (unless you have set a parent Component to your ComboBox’s PopupMenu, in which case the popupMenu will use that parent’s lookAndFeel).

1 Like

Reviving this thread, since I have a usecase which cannot be solved using the LookAndFeel:

I have a generic GUI engine, and the colours are controlled per Component in a hierarchical manner. There is a set of LookAndFeels, but creating a lookAndFeel for each Component is not an option.

It would be great, if the findColour() in the lookAndFeel could use the PopupMenu::Options::parentComponent* instead. This would lead to the correct behaviour.

EDIT: targetComponent is actually set, just the draw methods would need a const PopupMenu::Options& as argument.

Little bump, explanatory code:

Colour LookAndFeel::findPopupMenuColour (PopupMenu::ColourIDs colourId, const PopupMenu::Options& options)
{
    if (auto* target = options.getTargetComponent())
        return target->findColour (colourId);

    return findColour (colourId);
}

void LookAndFeel_V2::drawPopupMenuBackground (Graphics& g, int width, int height, const PopupMenu::Options& options)
{
    auto background = findPopupMenuColour (PopupMenu::backgroundColourId, options);
    // ...

needs a change of the PopupMenu::LookAndFeelMethods though, but now that we have the PopupMenu::Options that seems like the perfect use for it

1 Like

Any chance to get that in juce6 please?

Only users who have overridden the drawPopupMenu will have to add that argument to their function, they don’t even need to use it (but I guess, they will want to)

And it fixes the wrong behaviour of ComboBox not to use the defined colours for its PopupMenu.

EDIT: I took the liberty to edit this into a Feature Request, please vote :slight_smile:

EDIT 2:
Good morning to a fresh new week :slight_smile:

Any chance?

P.S. I am not allowed to post before anyone else says something. But I can edit :wink:

EDIT 3: Here is a PR to allow setting the PopupMenu colours on the ComboBox directly:

2 Likes

Why not passing the whole popupMenu::options to the popupMenu painting methods rather than just the target component?

Is a new LookAndFeel::findColour() method needed?
Couldn’t you use just use targetComponent->findColour() in your lnf ?

1 Like

That was my first thought as well. When I learned the PopupMenu won’t keep the PopupMenu::Options, I refactored to use only the targetComponent to limit the information exposed.

TBH. later I learned, that the MenuWindow stores a copy of the PopupMenu::Options, but I decided against changing it back to limit the exposed information and maybe future Components might use the same approach, which are not a PopupMenu.
But that is totally up for discussion.

Yes, that is possible. But if you look, I switched more than 10 times the findColour to use the targetComponent if available, so DRY justifies a second method IMHO.
I could have settled for a default argument, but that would impose a runtime cost on all other occasions, and only a few LookAndFeelMethods actually need to deflect to a targetComponent (so far only PopupMenu).

Little bump, can the PopupMenu::LookAndFeelMethods please make either the PopupMenu::Options or more generically the targetComponent* available?

Currently it is not possible to use the colours from the targetComponent, which I would argue is expected behaviour.

The PR above would make that possible, if it needs changing, please let me know.

N.B. the major version upgrade is a good time to change the PopupMenu::LookAndFeelMethods signature, which needs to happen eventually…

2 Likes

Still no feedback?

We’ll be adding this, along with some other PopupMenu features (manual columnbreaks, themed column separators) shortly after the JUCE 6 release.

7 Likes

I just tested this, seems there was no change. Is that still to do?
Or do I have to change something to make the PopupMenu reacting to the colours defined in ComboBox?

One other thing that would be really nice to have: for the LookAndFeel method getPopupMenuBorderSize(), could this return BorderSize instead of just int? That’d provide more flexibility, so you could have extra padding at the top and bottom, but not at the sides, or vice versa.

Good idea @pmbk. Do you mind creating a new topic for that?
What usually happens when people add their own issues to an existing thread, the easier one is addressed and the rest is forgotten.

Thanks!

Erm… cough… bump? :wink:

If the themed fancy things are too time consuming, I don’t mind if it’s just my PR…

Sorry for the delay. I’ve just pushed these changes to the develop branch:

Thank you @reuk for the hard work.

I looked through the change log, and I am currently recompiling to test, but at first glance: does this resolve the original issue, that you cannot change the colours of a PopupMenu as used in the ComboBox (without changing the colours for the whole LookAndFeel)?

I think it should, yes. It’s now possible to query both the parent and target component for a particular PopupMenu, so you could (for example) call findColour on either of those components and then use the resulting colour to fill the background/border/separator/etc. of the PopupMenu. The ‘Menus’ tab in the WidgetDemo has an example of this in action.

Ok, but that requires writing new LookAndFeel classes, am I right? The existing ones remain “broken”?

Yes, this intentionally tries to avoid changing the behaviour of existing LookAndFeel classes. The existing behaviour doesn’t feel “broken” enough to change it, and modifying the colour lookup mechanism might silently change the styling of peoples’ apps which I’d prefer not to do without a strong reason.

For now this feels like a good middle-ground between allowing more flexible theming without requiring users to test and rework too much code. Maybe I’ve missed a better solution that would work for more users though, in which case please let me know!

I understand that you don’t want to change existing behaviour. But:

  • the original behaviour is wrong as you can see this from the posts from several even experienced juce users (myself included). Everybody who posted in this and the sibling thread expected that setting a colour on a ComboBox would propagate to the PopupMenu
  • Since the API needs to change to propagate the PopupMenu::Options, there cannot happen a silent change, it is a breaking change on the LookAndFeel API
  • The only behaviour that changes is, if someone set a PopupMenu::ColourId on a ComboBox, this would suddenly have an effect. That person did that most likely intentionally, but was disappointed that it had no effect before

The solution that would work IMHO is in the PR from 23. April, linked above: