improve automated colour of label for colourselect button, especially when button colour includes alpha

[newville]

Fixes #2431

The calculation for 'average colour intensity' was

avg = functools.reduce(lambda a, b: a + b, self.colour.Get()) / 3

it could be improved by being

avg = functools.reduce(lambda a, b: a + b, self.colour.Get()[:3]) / 3

but even better would be to scale by the alpha value if available:

labcol =  self.colour.Get()
avg = (labcol[0] + labcol[1] + labcol[2])/3
if len(labcol) > 3: # alpha included
    avg *= labcol[3]/255.0

[jmoraleda]

I am not sure I agree with the logic of this change:

I see that avg = functools.reduce(lambda a, b: a + b, self.colour.Get()[:3]) / 3 makes sense, but it is less clear to me how to take into account alpha.

In particular, the proposed change avg *= labcol[3]/255.0 always moves avg to a smaller number, that is towards darker, but with transparency the color will move towards whatever the background color is, which could be darker or lighter than the color itself.

[newville]

@jmoraleda Well, if alpha is less than 255, that effectively darkens the color and so the text color ought to tend toward WHITE. With only a black and white choice, it will always be possible to end up with not-perfect colour contrast between coloured buttons and text, but I think the change here improves things.

With example code of

#!/usr/bin/env python
import wx
import wx.lib.colourselect as  csel

class ColourFrame(wx.Frame):
    def __init__(self):
        wx.Frame.__init__(self, None, -1)
        sizer = wx.GridBagSizer(4, 4)
        irow, icol = 0, 0
        for r in (64, 128, 196):
            for g in (64, 128, 196):
                for b in (64, 128, 196):
                    for a in (30, 150, 230):
                        label = f"{r, g, b, a}"
                        col = wx.Colour((r, g, b, a))
                        btn = csel.ColourSelect(self, label=label, colour=col, size=(150, -1))
                        irow += 1
                        sizer.Add(btn,  (icol, irow), (1,1), wx.LEFT, 2)
                        if irow == 3:
                            icol += 1
                            irow = 0
        self.SetSize((700, 600))
        self.SetSizer(sizer)
        sizer.Fit(self)
        self.Show()
        self.Raise()

if __name__ == '__main__':
    app = wx.App()
    app.SetTopWindow(ColourFrame())
    app.MainLoop()

current master branch gives:

So even a value of alpha as high as 230 gives a text colour that is often too dark.

Using avg = functools.reduce(lambda a, b: a + b, self.colour.Get()[:3]) / 3 and ignoring alpha gives:

does work better when alpha is high but still does not work very well at lower alpha.

Scaling by alpha using this PR gives:

So, yes, when alpha is lowered from 255, the text colour should tend towards white.

[jmoraleda]

@newville Thank you for your sample code and your screenshots. I think they really help to anchor the discussion.

To me, the screenshots suggest that the problem with the appearance of the ColourSelect buttons is not with the color of the label, but with the color of the background. In particular, I would expect that as transparency increases, the button background should be converging to the default button background, not to black.

The way I see it, transparent colors are not currently supported by the ColourSelect control and this PR is about adding such support. But I can't make a compelling case to add support for transparency for ColourSelect because the ColorChooser control that this buttons invoke does not handle transparency. So I can't see a use case to pass a transparent color, so I would not make any changes to the current code. But maybe others feel differently.

---

If a compelling use case to handle transparency in this control could be made, I think we would want to fix the background computation, not the foreground.

A starting point for this change could be the following code:

``` import wx import wx.lib.buttons import functools #---------------------------------------------------------------------------- wxEVT_COMMAND_COLOURSELECT = wx.NewEventType() class ColourSelectEvent(wx.PyCommandEvent): """ :class:`wx.ColourSelectEvent` is a special subclassing of :class:`wx.CommandEvent` and it provides for a custom event sent every time the user chooses a colour. """ def __init__(self, id, value): """ Default class constructor. :param integer `id`: the event identifier; :param wx.Colour `value`: the colour currently selected. """ wx.PyCommandEvent.__init__(self, id = id) self.SetEventType(wxEVT_COMMAND_COLOURSELECT) self.value = value def GetValue(self): """ Returns the currently selected colour. :rtype: :class:`wx.Colour` """ return self.value EVT_COLOURSELECT = wx.PyEventBinder(wxEVT_COMMAND_COLOURSELECT, 1) #---------------------------------------------------------------------------- class CustomColourData(object): """ A simple container for tracking custom colours to be shown in the colour dialog, and which facilitates reuse of this collection across multiple instances or multiple invocations of the :class:`ColourSelect` button. """ COUNT = 16 def __init__(self): self._customColours = [None] * self.COUNT @property def Colours(self): return self._customColours @Colours.setter def Colours(self, value): # slice it into the current list to keep the same list instance if isinstance(value, CustomColourData): value = value.Colours self._customColours[:] = value #---------------------------------------------------------------------------- class ColourSelect(wx.Button): """ A subclass of :class:`wx.Button` that, when clicked, will display a colour selection dialog. """ def __init__(self, parent, id=wx.ID_ANY, label="", colour=wx.BLACK, pos=wx.DefaultPosition, size=wx.DefaultSize, callback=None, style=0): """ Default class constructor. :param wx.Window `parent`: parent window. Must not be ``None``; :param integer `id`: window identifier. A value of -1 indicates a default value; :param string `label`: the button text label; :param wx.Colour: a valid :class:`wx.Colour` instance, which will be the default initial colour for this button; :type `colour`: :class:`wx.Colour` or tuple :param `pos`: the control position. A value of (-1, -1) indicates a default position, chosen by either the windowing system or wxPython, depending on platform; :type `pos`: tuple or :class:`wx.Point` :param `size`: the control size. A value of (-1, -1) indicates a default size, chosen by either the windowing system or wxPython, depending on platform; :type `size`: tuple or :class:`wx.Size` :param PyObject `callback`: a callable method/function that will be called every time the user chooses a new colour; :param integer `style`: the button style. """ super().__init__(parent, id, pos=pos, size=size, style=style, name='ColourSelect') self.SetBackgroundColour(colour) self.colour = colour self.SetLabel(label) self.callback = callback self.customColours = None parent.Bind(wx.EVT_BUTTON, self.OnClick, self) def GetColour(self): """ Returns the current colour set for the :class:`ColourSelect`. :rtype: :class:`wx.Colour` """ return self.colour def GetValue(self): """ Returns the current colour set for the :class:`ColourSelect`. Same as :meth:`~ColourSelect.GetColour`. :rtype: :class:`wx.Colour` """ return self.colour def SetValue(self, colour): """ Sets the current colour for :class:`ColourSelect`. Same as :meth:`~ColourSelect.SetColour`. :param `colour`: the new colour for :class:`ColourSelect`. :type `colour`: tuple or string or :class:`wx.Colour` """ self.SetColour(colour) def SetColour(self, colour): """ Sets the current colour for :class:`ColourSelect`. :param `colour`: the new colour for :class:`ColourSelect`. :type `colour`: tuple or string or :class:`wx.Colour` """ self.colour = wx.Colour(colour) # use the typmap or copy an existing colour object self.SetBackgroundColour(colour) def GetCustomColours(self): """ Returns the current set of custom colour values to be shown in the colour dialog, if supported. :rtype: :class:`CustomColourData` """ return self.customColours def SetCustomColours(self, colours): """ Sets the list of custom colour values to be shown in colour dialog, if supported. :param `colours`: An instance of :class:`CustomColourData` or a 16 element list of ``None`` or :class:`wx.Colour` values. """ if isinstance(colours, CustomColourData): colours = colours.Colours if self.customColours is None: self.customColours = CustomColourData() self.customColours.Colours = colours Colour = property(GetColour, SetColour) Value = property(GetValue, SetValue) CustomColours = property(GetCustomColours, SetCustomColours) def OnChange(self): """ Fires the ``EVT_COLOURSELECT`` event, as the user has changed the current colour. """ evt = ColourSelectEvent(self.GetId(), self.GetValue()) evt.SetEventObject(self) wx.PostEvent(self, evt) if self.callback is not None: self.callback() def OnClick(self, event): """ Handles the ``wx.EVT_BUTTON`` event for :class:`ColourSelect`. :param `event`: a :class:`wx.CommandEvent` event to be processed. """ data = wx.ColourData() data.SetChooseFull(True) data.SetColour(self.colour) if self.customColours: for idx, clr in enumerate(self.customColours.Colours): if clr is not None: data.SetCustomColour(idx, clr) dlg = wx.ColourDialog(wx.GetTopLevelParent(self), data) changed = dlg.ShowModal() == wx.ID_OK if changed: data = dlg.GetColourData() self.SetColour(data.GetColour()) if self.customColours: self.customColours.Colours = \ [data.GetCustomColour(idx) for idx in range(0, 16)] dlg.Destroy() # moved after dlg.Destroy, since who knows what the callback will do... if changed: self.OnChange() ```

With the above code, in gtk I get this with a light theme:

and this with a dark theme:

I have not looked into why the label background is different from the rest of the button with this code and I am not enamored with the two-tone way the label is rendered in some of the cases. Both are things we might want to further improve if we went down this path. But as I mentioned earlier I can't make a compelling case for adding transparency support. Thank you.

[newville]

@jmoraleda

To me, the screenshots suggest that the problem with the appearance of the ColourSelect buttons is not with the color of the label, but with the color of the background.

There definitely is a problem with the color of the label.

If you want to propose changing the meaning of the 4th value for the Colour created with the ColourSelect dialog, that seems like a different request to me.

I am not proposing to change the behavior of ColourSelect for selecting the color itself. I am proposing that the automated selection of the color of the text written on top of that color be changed to reflect the coloring of the button.

With a choice between only black or white, having it perfect text color for all cases (especially near "#888888") is not easy. but I would also call the results you get somewhat worse, even ignoring the two-toned button.

I don't disagree with you that the handling of transparency is confusing, or maybe the behavior is that the back of the button is black and the button is coloring over that: transparency becomes black and opaque goes to the saturated color.

But for now, I would say, let's first fix the bad color of the text and then maybe figure out if ColourSelect needs even more changing.

[jmoraleda]

@newville If I understand things correctly, if we pass a color without transparency to the ColourSelect button, then avg = functools.reduce(lambda a, b: a + b, self.colour.Get()[:3]) / 3 will produce the correct result regardless of whether the color has alpha or not. This seems to me like a good change.

But, given that the underlying ColorDialog doesn't support transparency, the return value will be a color without transparency, regardless of whether there was transparency in the initial value. So what is the use case for supporting passing a color with transparency as the initial color? If there is no good use case, then I would suggest we don't make the code longer than it needs to be and we don't make the change:

labcol =  self.colour.Get()
avg = (labcol[0] + labcol[1] + labcol[2])/3
if len(labcol) > 3: # alpha included
    avg *= labcol[3]/255.0

Just my opinion.

[newville]

If the fourth number is 255, the two results are the same. If the fourth value is not 255, scaling by it is better: see the images above. If the colourdialog does not expose the fourth value, there is no difference. On systems where the fourth value is exposed in the colourdialog, including macos, the scaled value is never worse.

Your objection to the obviously better solution confuses me.

[jmoraleda]

On systems where the fourth value is exposed in the colourdialog, including macos ...

I do not have access to MacOS and https://docs.wxpython.org/wx.ColourDialog.html does not show the appearance for MacOS. I gather from this comment that ColourDialog in MacOS does allow selecting colors with transparency? If so, that definitely would be a valid reason to add support for transparency in the ColourSelect button.

the scaled value is never worse.

Agreed. But if it makes sense to add support for colors with transparency, then personally I would prefer a PR that fixes the button background so that it shows the correct color, rather than a PR that computes the correct intensity of the wrong color. This is just my personal opinion.

[newville]

@jmoraleda if you have a PR that sets the color of the button as you think it should be and also gives a readable color for the text, I will happily close this PR. And I promise not to question if your PR works as intended until I have actually tried it ;).

Until then, I think I'll leave this PR open as it gives a simple solution to the identified issue of the color automatically selected for text on a ColourSelect button.

[newville]

Any chance of getting thoughts on this PR?

[swt2c]

Any chance of getting thoughts on this PR?

I was hoping that you and @jmoraleda would come to a consensus about this fix. :-) Personally, I'm not great at reviewing changes that are color-related because I'm partially color blind.

[newville]

@swt2c actually, I think that does not reduce your ability to review this. In fact, this does not have much to do with color, but with how to choose between white and black text on a colored button. Please compare the readability for you of the three options (1=current, 2=ignore alpha, 3=this PR, including alpha) at https://github.com/wxWidgets/Phoenix/pull/2432#issuecomment-1631792049

I believe that @jmoraleda was hoping for a more comprehensive change to the coloring of these widgets. I am not sure whether there is a proposal in the works for that or not. In the meantime, I would hope that merging this rather simple fix for a definite problem would be separate from a larger change.

Please allow simple solutions to simple problems without requiring drawn-out processes.

[jmoraleda]

In the platforms that I can access: GTK and MSW, the color chooser does not support transparency, so I cannot justify a PR to add supporting transparency as the initial color passed to the color chooser.

It has been mentioned that the wx colour dialog in OSX does support transparency, but I cannot test this myself. So someone with OSX should look into this PR. In particular, I think the person evaluating the PR should look into the following two questions:

1. Confirm that the dialog that is currently shown when the colourselect button is pressed does support transparency (otherwise it makes no sense to add support to pass a transparent color as the initial color).

2. Test the proposed PR using both a light and a dark theme. By definition, a transparent color will let through some of the color of the underlying widget. So how should a transparent color be displayed as a button background? I believe this PR has not taken the issue of light vs dark themes into consideration.

[newville]

@jmoraleda Thanks.

1. Confirm that the dialog that is currently shown when the colourselect button is pressed does support . transparency (otherwise it makes no sense to add support to pass a transparent color as the initial color).

On MacOS, there is an Opacity slider on the color select dialog and it does adjust the color by setting the 4th value ("A) in the Colour tuple. With a default value of 255 (described "100% opacity"), the colors are fully saturated. At very low "opacity" the colors become black. I agree that may not be the "most obvious reading" of how these values should work, I am only describing how they do, in fact, work.

2 Test the proposed PR using both a light and a dark theme. I don't know of a "theme setting" to adjust that "black" to be "transparent". I just have no idea what "setting a theme" means for wxPython applications. Is that a thing?

By definition, a transparent color will let through some of the color of the underlying widget.

I think that may not be correct.

So how should a transparent color be displayed as a button background? I believe this PR has not taken the issue of light vs dark themes into consideration.

I could be wrong, but I believe wxPython does not have a light and dark theme. Please point to documentation for that.

Back to your point 1: I would not agree that whether the ColourSelect dialog has this setting is what should influence whether this fix is applied. What should matter is whether setting that value either with the ColourSelect dialog OR programmatically actually changes the displayed color of the button. On MacOS, changing the value of "A" does change the color of the button, as shown above.

Windows and Linux (at least the Linuxes I test on) do not support or respond to changing "A". In principle, this PR would then not affect those (more below). But "A" does have an effect on MacOS.

The current calculation of the text color is still bad on Windows and Linux. At https://github.com/wxWidgets/Phoenix/pull/2432#issuecomment-1631792049 you can just look at the right-most column (A=230 is very close to A=255). Several choices for colors of buttons, notably (64, 64, 64, 255) get text that is black and too hard to read. This PR fixes that.

Let me put it this way:

A colourselect button with color (64, 64, 64, 255) and even as dark as (15, 15, 15, 255) will get text that is black on all systems. The current calculation:

            avg = functools.reduce(lambda a, b: a + b, self.colour.Get()) / 3
            fcolour = avg > 128 and wx.BLACK or wx.WHITE

is simply mistaken. This code assumes self.colour.Get() is a tuple with three values. It is not. This PR fixes that. It also fixes the text of the color to be visible on systems that allow setting A<255.

I am somewhat flabbergasted and alarmed at the pushback here. The problem is simple, the fix is simple, and the PR explains the problem and fixes the behavior. The comments from @jmoraleda are poorly informed and actively discouraging. Why is that happening?

If the goal is to discourage small improvements from community members, the response to this PR is perfect.

Please merge this now.

[jmoraleda]

I am somewhat flabbergasted and alarmed at the pushback here. The problem is simple, the fix is simple, and the PR explains the problem and fixes the behavior. The comments from @jmoraleda are poorly informed and actively discouraging. Why is that happening?

@newville I am sorry you feel this way. It was never my intention to discourage your contributions. I apologize if I have erred. I am just another volunteer with no special privileges on the project. At this point I will recuse myself from further comments on this discussion and will let other people with greater expertise on colors and themes, in particular on MACOS, evaluate the PR.

[newville]

@jmoraleda Well, your first response to this PR was literally "I am not sure I agree with the logic of this change".

Currently, the text drawn on a color select button with color=(20, 20, 20, 255) is black. This PR fixes that bug.

[newville]

Hi all, This PR is now more than 2 months old. It's a pretty simple fix to a pretty clear bug that assumes colours are RGB and cannot be RGBA. I've pinged developers on this a few times.

I certainly understand over-busy schedules and thankless demands on open-source developers. OTOH, Ignoring simple patches like this suggests that outside contributions are not valued.

As it turns out, I've now monkey-patched my own downstream code that uses ColourSelect.

Please merge or comment on this. I will delete my branch and close this in one week.

[newville]

@Metallicow I have no idea how that is relevant to the simple, clear, and short PR here.

This was a simple fix to a simple problem. @jmoraleda went out of their way to misunderstand the problem and repeatedly tried to change the goal of the PR. No other developer offered any meaningful insight. I gave up trying to help in disgust and came up with a workaround.

Now, months after the PR was closed, you post some ravings. Honestly, I have no clue what you are talking about.

And I am very worried about the development of wxPython.

[Metallicow]

@newville Sorry if I came across as a bit blind, I have since removed those posts. Try this. I think you are on the right path, but I do see that your readability is a bit hard here. Look at this and imagine a function that does all that but makes it easy to read the damned text on the button just because of its color.

[]

... NOT everyone thinks in RGB or GBRAKMYC for example. Put a little shinyness in your work and we will reconsider readability.

def GetLuminance(rgb):
    """
    https://www.w3.org/TR/2008/REC-WCAG20-20081211/#relativeluminancedef
    """
    r, g, b = rgb
    RsRGB = r / 255.0
    GsRGB = g / 255.0
    BsRGB = b / 255.0

    if RsRGB <= 0.03928:
        R = RsRGB / 12.92
    else:
        R = ((RsRGB + 0.055) / 1.055) ** 2.4
    if GsRGB <= 0.03928:
        G = GsRGB / 12.92
    else:
        G = ((GsRGB + 0.055) / 1.055) ** 2.4
    if BsRGB <= 0.03928:
        B = BsRGB / 12.92
    else:
        B = ((BsRGB + 0.055) / 1.055) ** 2.4
    return (0.2126 * R) + (0.7152 * G) + (0.0722 * B)

... and if you are using a newer version of wxPython, wxWidgets was awesome enough to implement all that as a function at the C/C++ level.

Good luck.

[newville]

@Metallicow Please stop responding to this PR I closed several months ago, and please stop sending messages to me.

This was a simple solution to a simple problem. The color of text on a button is automatically selected assuming that the color of the button has 3 values for R, G, and B. Colors now have 4 values, R, G, B, and A and the current math is just plain wrong and easy to fix. Sure, maybe there's a fancier way to do it. The very simple fix proposed here 7 months ago appears to have been too confusing for the current developers, so I doubt anything more elaborate is worth proposing. I cannot help a project where the developers cannot recognize a simple algebraic error when it is pointed out to them.

[Metallicow]

Fair enough. I'd like a link to your sample demo and repository, if this isn't another 3-way into oblivion color scheme type of widget fix. I guess it never hurt to listen or be interested in something such as x, y, and z. Thank you for your time. I never said you had an algebraic error anywhere.