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

Fix forced dark mode if dark mode on OS level #1686

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

IvoFPV
Copy link
Contributor

@IvoFPV IvoFPV commented Sep 24, 2019

Some users who run the chrome app reported dark mode switch in the options dialog is not working. This was because dark mode was forced if they used dark mode on OS level. I fixed this by having the dark mode automatically apply only if it has not been manually changed before and the dark mode is used on OS level. To me this works much better. @Docteh What do you think?

@mikeller mikeller added this to the 10.6.0 milestone Sep 24, 2019
@Docteh
Copy link
Contributor

Docteh commented Sep 24, 2019

I do like the effect of the betaflight configurator being one of the few apps that changes automagically when dark mode is turned on/off. Maybe we could do a select box with on/off/auto?

But detecting the OS dark mode on first launch does leave a good surprise. I forgot to suggest updating NWJS to 0.40.0+ so we'd actually get that feature out to the stand alone version.

I think I want to come back later and try and squash down the dark mode CSS into one file.

Dang, the more I think about this, the more I want on/off/auto.

@mikeller
Copy link
Member

On / off / auto sounds good. Re updating NW.js, we got bitten by some versions out there not properly working for us on a supported OS (IIRC), so we stuck to the latest working version.
Will have to check again if something newer is out that works for us after 10.6 is out.

@IvoFPV
Copy link
Contributor Author

IvoFPV commented Sep 25, 2019

Like I made it now it is basically auto mode until you make a manual change. Do we actually need auto mode at all because dark mode on OS level is not something that is changed often? If someone sets it manually it will use that. Standalone won't work this way unless updated so I would leave auto for when and if we update NW.js. For now this should work.
@Docteh I was looking to make dark mode .css files cleaner by using variables since most changes are only colors with exception being some borders which could be squashed into one .css file as you say.

@Docteh
Copy link
Contributor

Docteh commented Sep 25, 2019

Well darkTheme is set when a user opens the config options at the top right corner.

The nice thing about using Local Storage for the configuration (ConfigStorage) is that in the Application tab of DevTools you can delete things and then restart the app. I was going to type out where to find it, then remembered that I screenshots exist:
image
So if I need to Permanently enable Expert Mode, or otherwise just even check if its set, darkTheme is set. Yeah, It gets set when you open the options. Closing it, and clicking other items is fine. That section of code might need a look see.
Looks like we cant properly serialize that a variable is undefined.
image

I've been using dark mode manually on windows, but it looks like there is at least three third party macOS programs for scheduling it, this reddit thread mentions f.lux and Shift, and Nightowl.

NWJS:
Yeah, every time I think about it, I check their issue tracker and there is usually some issue mentioning trouble with newer versions on windows 7.
July: nwjs/nw.js#7116
This month: nwjs/nw.js#7159
I also saw a comment about the folder structure changing on macOS for 0.40.x nwjs/nw.js#7130

0.40.0 works fine for me on windows 10 and my macOS though.

@IvoFPV
Copy link
Contributor Author

IvoFPV commented Sep 25, 2019

Added the on/off/auto select. If undefined in ConfigStorage it will get set to Auto. Should be good now.

So if I need to Permanently enable Expert Mode, or otherwise just even check if its set, darkTheme is set. Yeah, It gets set when you open the options. Closing it, and clicking other items is fine. That section of code might need a look see.

@Docteh Noticed it changed expert mode as well when I was doing expert mode for sliders. Just clicking on the options menu shouldn't change the settings. This will need some care but after release.

src/js/DarkTheme.js Outdated Show resolved Hide resolved
@@ -14,6 +14,15 @@
"message": "No",
"description": "General No message to be used across the application"
},
"on": {
"message": "On"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have 'On', 'Off', and 'Auto' multiple times in the messages. We should replace these with the new generic messages as well.

Not sure if this should be done now, or if we want to wait and do it after 10.6 has been released, but it should be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are a lot of messages of this kind. Like save button, refresh are set for each tab individually and it would be best to convert all of them to a single generic message. Probably best to leave it for after 10.6 is out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a check at the same time that fails pull requests in CI if they attempt to add a message that is identical to an existing one...

// sets dark theme to auto if not manually changed
setDarkTheme(2);
} else {
setDarkTheme(result.darkTheme);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor niggle, but this will break on first load for users of 10.6.0-RC1, since result.darkTheme is true or false:

image

Copy link
Contributor Author

@IvoFPV IvoFPV Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to work around that and not do something like if it is true or false then clear it or set it to auto. But that seems like a wrong thing to do.

if (this.configEnabled) {
if (this.configEnabled === 0) {
this.applyDark();
} else if (this.configEnabled === 2 && window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (this.configEnabled === 0 || this.configEnabled === 2 && window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
    this.applyDark();
} else {
    this.applyNormal();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants