-
Notifications
You must be signed in to change notification settings - Fork 529
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
Use system theme via matchMedia for auto dark mode option #372
Comments
@shgysk8zer0 thanks for the feedback. The option I've had my eye on for a while is Electron's https://www.electronjs.org/docs/api/native-theme#nativethemeshouldusedarkcolors-readonly This is now supported in recent versions of macOS and Windows, and Desktop itself has some detection available: desktop/app/src/ui/lib/dark-theme.ts Lines 7 to 18 in 236402d
It'd be interesting to see what sort of support is there for Linux users in Chromium itself before we try out alternative approaches, as it may work already. |
@shiftkey I think that either the media query option or
The only thing I've not seen is detecting if system theme is even an option on an OS. Linux has too many distros for the UA sniffing route to be feasible. |
With this change and running the app locally: diff --git a/app/src/ui/lib/dark-theme.ts b/app/src/ui/lib/dark-theme.ts
index 31451ec9f..5c898852b 100644
--- a/app/src/ui/lib/dark-theme.ts
+++ b/app/src/ui/lib/dark-theme.ts
@@ -12,6 +12,9 @@ export function supportsDarkMode() {
// was released October 2nd, 2018 that the feature can just be "attained" by upgrading
// See https://github.com/desktop/desktop/issues/9015 for more
return isWindows10And1809Preview17666OrLater()
+ } else if (__LINUX__) {
+ // TODO: let's see if this works at all
+ return true
}
return false I see these changes:
desktop/app/src/ui/lib/theme-change-monitor.ts Lines 25 to 32 in 5f7bf50
So it's close to how the other platforms work, but maybe there's no change monitor support in Chromium for Linux to signal this (or it's not working how I expect). It's also not clear to me how broadly this is supported. |
@shiftkey This is consistent with my experience in Fedora, though my dark/light themes are Adwaita instead or Yaru. I expect similar results for anything on Gnome and hopefully beyond. Based on my understanding and assumptions, I think we could gauge support by having other run And I suspect that the lack of the change event being emitted stems from themes themselves being unofficial and kinda hacks in any distro I'm aware of. |
I had a play around with this and it seems to work as expected, but I wanted to add an event listener like this to monitor when it changes:
But it doesn't seem to fire when I change the theme in the OS. Without that, using |
It is my understanding per Electron documentation of nativeTheme.themeSource = 'light';
matchMedia('(prefers-color-scheme: dark)').matches // false
nativeTheme.themeSource = 'dark';
matchMedia('(prefers-color-scheme: dark)').matches // true
nativeTheme.themeSource = 'system';
matchMedia('(prefers-color-scheme: dark)').matches // depends on OS theme Is the current implementation using |
It is not, but even when I enabled this and ran the app locally I wasn't seeing the change event fire (e.g. switching between |
In desktop#11629 I moved Electron to use |
@say25 thanks for the update. We're up to date with the Electron 11 upgrade so this was something I was hoping to revisit. |
I've re-tested this behaviour with Electron 11 but the lack of the The workarounds like polling for theme changes feel unsatisfactory here, so I'm going to leave this open in the meantime. |
@shiftkey May I request that startup light/dark mode be regarded as a separate issue from updating theme post-launch? Switching themes while Desktop is open is pretty minor, and I'd personally rather re-open it if I change system themes and want Desktop's theme to reflect that than manually change anything in preferences every time. |
The main project has updated to Electron 16 and 17 last week, so I have a build out that should have proper support for this - |
Please test out the |
Describe the feature or problem you’d like to solve
GitHub Desktop (at least on Mac) supports system preferences for dark/light mode (see desktop#5037). This seems to rely on some
systemPreferences
APIs, and this does not seem to be a cross-platform solution, and is definitely not working for me on Fedora. Conversations surrounding the issue and feature seem exclusive to macOS despite dark/light being something that exists in some form on nearly every modern OS.I'm not sure if this is an issue for Desktop itself, Electron, or this fork. It's a feature request here, something already implemented on GitHub Desktop, and seemingly a bug in Electron.
Using GitHub Desktop 2.5.7 on Fedora 29 (I know... A bit outdated).
Proposed solution
Instead of relying on
systemPreferences
, favormatchMedia('(prefers-color-scheme: dark)')
for implementing system preferences.How will it benefit Desktop and its users?
Using a standard API instead of an Electron one should improve compatibility and maintainability.
Additional context
I've done some experimenting with this approach.
query.matches
works (mostly) correctly, butquery.addListener()
does not behave as expected.matchMedia
seems to be fixed at launch time, such thatquery.matches
is determined by system theme when first launched.matchMedia('(prefers-color-scheme: dark)').matches
is falsematchMedia('(prefers-color-scheme: dark)').matches
is still false and event is not dispatchedmatchMedia('(prefers-color-scheme: dark)').matches
is true nowmatchMedia('(prefers-color-scheme: dark)').matches
is still true and event is not dispatchedAdd any other context like screenshots or mockups are helpful, if applicable.
The text was updated successfully, but these errors were encountered: