Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renable dark mode on macOS #1144

Merged
merged 1 commit into from Dec 19, 2018

Conversation

Projects
None yet
3 participants
@scribblemaniac
Copy link
Member

scribblemaniac commented Dec 15, 2018

Compiles and runs just fine on macOS 10.13, still needs to be tested on macOS 10.14 with and without dark mode. Based on code from Chromium.

@scribblemaniac scribblemaniac added the ux label Dec 15, 2018

@scribblemaniac scribblemaniac requested a review from CandyFace Dec 15, 2018

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Dec 15, 2018

it works on Mac OS 10.14 but what about the build server which runs on 10.12?

../../core_lib/src/external/macosx/macosxnative.mm:38:46: warning: instance method '-bestMatchFromAppearancesWithNames:' not found (return type defaults to 'id') [-Wobjc-method-access]�

                [[NSApp effectiveAppearance] bestMatchFromAppearancesWithNames:@[

although that's just a warning, I think we should have it verified on a machine that actually runs 10.12 to make sure it works.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 15, 2018

Can't we just wrap all the dark mode code with the 10.14 #define? The dark mode is only available in Mojave. All other mac os versions before Mojave don't even need it.

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Dec 16, 2018

The MAC_OS_X_VERSION_10_14 define is not enough by itself, since it already exists in 10.13 SDK. It would have to be something like the define this commit adds.
I'm not sure it works as intended though, the MAC_OS_X_VERSION_MIN_REQUIRED flag is ignored unless a target or min mac os version is set.

Given that Scribble added a string to contain the value though, it'll probably work either way now.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 16, 2018

Ok, I see. We need a runtime mac os version check, not compile time check.

@chchwy chchwy merged commit 82dc3e3 into pencil2d:master Dec 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@scribblemaniac

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Dec 20, 2018

Do we have any way to actually test this on 10.12? Because that is still something we should do.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 20, 2018

I can test it. I have a macbook which runs 10.12

@chchwy chchwy added this to the 0.6.3 milestone Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.