Skip to content

Commit

Permalink
Using NSColor System Colours
Browse files Browse the repository at this point in the history 
Removing the old HITheme Constants for the last two wx system colours, first step for Dark Mode support, see https://trac.wxwidgets.org/ticket/18146
  • Loading branch information
@csomor
csomor committed Jun 11, 2018
1 parent 93edcae commit 71bb680
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions src/osx/cocoa/settings.mm
View file
Expand Up @@ -74,9 +74,14 @@ static int wxOSXGetUserDefault(NSString* key, int defaultValue)
sysColor = [NSColor windowFrameColor];
break;
case wxSYS_COLOUR_WINDOW:
return wxColour(wxMacCreateCGColorFromHITheme( 15 /* kThemeBrushDocumentWindowBackground */ )) ;
sysColor = [NSColor windowBackgroundColor];

This comment has been minimized.

Sign in to view

Copy link
@dkulp

dkulp Jun 12, 2018

Contributor

I believe this one needs to be controlBackgroundColor, not windowBackgroundColor. windowBackgroundColor in 'light' mode is 0xE7E7E7. However, a bunch of controls (TreeView, wxPropertyGrid, etc...) use the wxSYS_COLOUR_WINDOW for their background which results in grey instead of the older white. (kThemeBrushDocumentWindowBackground resolved to 0xFFFFFF).

This comment has been minimized.

Sign in to view

Copy link
@csomor

csomor Jun 12, 2018

Author Contributor

You are right, could not see it at first in a tree control, and actually windowBackgroundColor is a pattern, not just a color, but in the propGrid sample, it is clearly visible. Thanks.
break;
case wxSYS_COLOUR_BTNFACE:
return wxColour(wxMacCreateCGColorFromHITheme( 3 /* kThemeBrushDialogBackgroundActive */));
if ( wxPlatformInfo::Get().CheckOSVersion(10, 14 ) )
sysColor = [NSColor windowBackgroundColor];
else
sysColor = [NSColor controlColor];

This comment has been minimized.

Sign in to view

Copy link
@dkulp

dkulp Oct 30, 2018

Contributor

This isn't working on Sierra/High Sierra. On those machines, controlColor is coming back as black. Most likely, this should be controlBackgroundColor

This comment has been minimized.

Sign in to view

Copy link
@dkulp

dkulp Oct 30, 2018

Contributor

Actually, controlBackgroundColor returns white. :( According to Apple docs:
https://developer.apple.com/documentation/appkit/nscolor/1524856-controlcolor?language=objc
the controlColor is a pattern color and you cannot assume that it is a solid color. Instead, they say to use lightGrayColor

This comment has been minimized.

Sign in to view

Copy link
@csomor

csomor Oct 30, 2018

Author Contributor

Could you provide a sample that shows an incorrect rendering ? wxColour can be a pattern, at least that worked previously, when I was using CGColorRefs directly, and it should be drawn correctly. I don't know whether we'd have to support getting a 'compatible' RGB color.

This comment has been minimized.

Sign in to view

Copy link
@dkulp

dkulp Oct 30, 2018

Contributor

Well, I need to have RGB codes as I use the colors to make parts of OpenGL contexts drawn to something close to system colors. Right now, I just get black which is definitely not usable.

This comment has been minimized.

Sign in to view

Copy link
@dkulp

dkulp Oct 30, 2018

Contributor

At the very least, we would need some way to determine if wxColor::Red() and company will return usable results. If we can at least determine that, then we can do something smart. Right now, if I call wxSYS_COLOUR_BTNFACE, I get usable results on Mojave, but not on High Sierra.

This comment has been minimized.

Sign in to view

Copy link
@csomor

csomor Oct 30, 2018

Author Contributor

ok, I think I should be able to find out:
when colorUsingColorSpaceName:NSCalibratedRGBColorSpace returns nil, right now I return 0 in this case, could you try to debug to find out whether this conversion is successful, or whether the default 0 is returned, it's all in color.mm 61ff

This comment has been minimized.

Sign in to view

Copy link
@dkulp

dkulp Oct 30, 2018

Contributor

The default of "return 0.0;" is being hit.

This comment has been minimized.

Sign in to view

Copy link
@csomor

csomor Oct 30, 2018

Author Contributor

perfect, thanks, @vadz what would you think could be a good method name on a wxColourto inquire whether the wxColour is indeed a pattern or whether it has valid RGB values ? IsRGB ? HasRGB ? IsTexture/Pattern

This comment has been minimized.

Sign in to view

Copy link
@vadz

vadz Oct 30, 2018

Contributor

To be honest, I don't really that much about the name of the method. I'd probably call it IsSolid() but the other names are fine too.

The real problem is that almost none of the class methods really make any sense for patterns which means that we really should have 2 different classes here...

This comment has been minimized.

Sign in to view

Copy link
@csomor

csomor Oct 30, 2018

Author Contributor

Thanks. I agree that something eg like wxBrush would be a more versatile instance, but our API as it is is asking for colors, and while I could have different ref data classes easily, having multiple wxColour subclasses seems incompatible with our current API. How would you design such a change ?

This comment has been minimized.

Sign in to view

Copy link
@csomor

csomor Oct 30, 2018

Author Contributor

could you test with 9a05410, it has a IsSolid

This comment has been minimized.

Sign in to view

Copy link
@vadz

vadz Oct 30, 2018

Contributor

This is not really a wxBrush neither as, if I understand correctly the previous discussions about it, its value can also change on its own if the system colours/theme change. So I think we really need some new class, wxSystemPattern or something like that, which could be converted to wxColour when possible (and this would also probably return a fixed value, which wouldn't change later, avoiding any surprises).

The question is, of course, about compatibility, but I think it could be made reasonably compatible if we provided implicit conversion from wxColour to this new class for the setters. Getters will need to change however, e.g. we'd need to have GetBackgroundPattern() instead of GetBackgroundColour(), but this seems better/unavoidable anyhow if there is no solid background colour to return in any case.
break;
case wxSYS_COLOUR_LISTBOX:
sysColor = [NSColor controlBackgroundColor];
break;
Expand Down

0 comments on commit 71bb680

Please sign in to comment.