[PATCH] [ATLAS] Colour choice buttons are ugly on OS X

[historic_bruno]

We use these in Atlas for selecting player colour and various settings on the environment panel. It's a custom control comprising of a button whose colour is changed by the result of a derived ​wxColourDialog. On OS X the buttons have a fixed texture and the background is a barely visible part of the button.

• A ​wxBitmapButton might work better assuming we create a new bitmap when choosing a colour, but then we lose the colour label. This may be a suitable trade-off, or we could make the implementation specific to OS X using preprocessor macros, if that's not too messy.
• There's always ​wxColourPickerCtrl which combines the button and the colour dialog in one control, in a platform dependent fashion. The problem is it's ugly on wxGTK (a plain button with a small coloured patch in the middle - which is also how it looks on OS X), and we lose automatically notifying the environment setting observers. On Windows, it looks identical to what we have now.

[historic_bruno]

The same problem occurs with certain themes (e.g. New Wave) on Ubuntu.

[Mitchell K]

I'll try to fix this with a wxColourPickerCtrl.

[Mitchell K]

I've implemented wxColourPickerCtrl in the Player and Environment sections. It looks much better on OS X than it did before. Apparently wxColourPickerCtrl looks good on wxWidgets 2.9 but bad on 2.8. It should look no different than before on Windows.

Here's how it looks on OS X:

much better than

It would be nice if a linux user could test this and see how it looks on gtk. If it's really bad, it would be possible to use #if OS_LINUX macros to leave it as it was before on Linux, but would probably be a bit messy.

I've also removed the hex codes which used to be on the Environment section buttons. There actually is an option for this very thing in wxColourPickerCtrl, but it doesn't look that great as it simply inverts against the button (thus at 50% grey it is invisible) and I don't really see why it would be that useful anyway.

I also replaced a few #includes in header files with forward class declarations.

This is my first patch, I hope it's useful :).

[Mitchell K]

Forgot the "g_EnvironmentSettings.NotifyObserversExcept(m_Conn);" line in the original patch, fixed in v2.

[Mitchell K]

Cleaned up the patch in v3, there were a couple headers edited for no reason and this time I diffed specific directories rather than edited out unrelated changes in the diff after. So yea review v3 please :).

[historic_bruno]

Here's how your patch looks on Ubuntu with default theme and wxGTK 2.8:

Current style:

So I would consider that a regression and a lot of our users are on Linux w/ wxGTK 2.8. I'm seriously tempted to try Philip's idea of having a custom control with two separate items: a color box (perhaps a ​wxStaticBitmap) with a button next to it that opens the color picker - assuming the padding between controls doesn't make that look ugly too. (I think going as far as a ​property grid is a bit overkill)

It's probably best to avoid the messiness of platform-specific code paths for something like this.

[Mitchell K]

Yes, it does look worse on GTK than it did before… not as much worse as it was for OS X compared to now, but still not an optimal solution. I think something more custom that fits all platforms is a good idea.

[Mitchell K]

One alternative I can think of is to make it platform specific but in a non-messy way : use a custom class, and just give one definition for Mac (typdef it to wxColourPickerCtrl, or subclass if necessary) and the other for GTK/Windows (the custom class that exists now, VariableColourBox). All other code should work unchanged. This should be better because I think the button looks really good the way it already is on GTK. I'll try this later today.

[Mitchell K]

Hmmm, I had forgotten that Environment and Player do this differently, and it actually wouldn't be as non-messy as I thought it was… I guess the button with the color well beside it would work better.

[Stan]

Is that still relevant ?

[historic_bruno]

Yes.

[historic_bruno]

Current look on Mojave (light theme), wxWidgets 3.0.3.1: