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?
I just tested this, seems there was no change. Is that still to do?
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.
Erm… cough… bump?
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:
I agree with the other two points, but I’m not convinced about this one. If users are inheriting their LnF classes from LnF_V2, their code won’t be broken by this change, and I think this probably means that the behaviour shouldn’t change either. It looks like your PR is also silently changing the colour lookup mechanism of the default LnF implementations.
Admittedly, I can’t imagine this “breaking change” affecting that many users - I think it’d just be users who have set custom PopupMenu colours directly on the LnF, but have also set up different PopupMenu colours on a ComboBox or other target component. However, for those users the change in behaviour would not have an obvious cause…
It’s a tricky one. I’ll see what the rest of the team thinks.
I think it would, because it changes the signature of the PopupMenu::LookAndFeelMethods (it adds the
PopupMenu::Options&). So the override will issue a warning (hopefully, I know override is not mandatory).
Thank you for having another look!
I’m not sure about that. Imagine a user is using the built-in LnF implementations directly, or is overriding a handful of other methods but leaving the PopupMenu-related methods alone. In these cases, changing the behaviour of the colour lookup will be a silent breaking change.
I am not sure, maybe I am missunderstanding something.
It will change the behaviour, which is the intention:
- findColour will use the targetComponent first and look for colours from PopupMenu::ColourIDs
- only if a colour from that enum range was set to the targetComponent, it will be found (which would seem like intentionally, like said before. Remember PopupMenu is not a Component!)
- Since it is not found, it will fall back to the colours from the LookAndFeel, which is the original behaviour
I discussed this with the team, and our feeling was that if we could implement this to just modify the colour lookup behaviour without affecting any other aspect of users’ code, that would be a useful change. However, when I tried to implement this behaviour, it turned out that it would be more disruptive than expected.
At the moment, the LnF classes implement both the old “without-options” and the new “with-options” methods, and the with-options methods just forward to the without-options methods. This means that if users have derived from any of the
LookAndFeel_V* classes, their old without-options overrides will continue to function as expected. If we were to add the new colour lookup behaviour, we’d no longer be able to forward to the old without-options methods, and users’ custom popup drawing routines would no longer be called. This would be quite a large change in behaviour, and one we’d prefer not to force on users all in one go.
As I said earlier, the current state feels like a good compromise - current code won’t change in behaviour, but if people wish to implement their own LnF classes which do more sophisticated colour lookup, that’s now possible.
In the longer term, we may deprecate the old without-options LnF methods, and we can take another look at modifying the colour lookup behaviour once we’ve given users a chance to switch to the new API.
That is a sad decission, as it renders the existing LookAndFeels useless, if you want to have different colours for different ComboBoxes. I would have thought that most users will want the behaivour fixed for the existing LookAndFeels as well.
I repeat myself when I say, the user written LookAndFeel methods will break with the update and they only have to add the PopupMenu::Options or the targetComponent*, depending of which route you choose. Nothing else changes for their methods, because setting those colours to a Component never had any effect.
Anyway, time to let go…
Btw. What happens to feature requests that you decide not to fix despite being the fifth most voted feature request?
Since the issue is fixed for future LookAndFeels, this can probably be closed, so the votes become free?
Alright, will do. Thanks!