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).
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.
