-
Notifications
You must be signed in to change notification settings - Fork 3.2k
qt: use system theme as default theme and add 'System' theme option #9684
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
base: master
Are you sure you want to change the base?
Conversation
colortheme_combo.addItem(_('Light'), 'light') | ||
colortheme_combo.addItem(_('Dark'), 'dark') | ||
colortheme_combo.addItem(_('System'), 'default') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running on a system that has a dark theme, what exactly happens when the user sets GUI_QT_COLOR_THEME to "light"? Presumably the user would then expect Electrum to run in a light theme - but that's not what happens, is it? I suppose it is only a few colours here-and-there that gets changed.
What I am getting at is that the cornerstone of the "dark" theme is the qdarkstyle package, and the "system" ("default") theme is just whatever qt does by default. -- and on top of all that we have some hardcoded colors that depend on GUI_QT_COLOR_THEME
(see util.ColorScheme
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the user sets GUI_QT_COLOR_THEME
to light and has its system theme in dark mode electrum will start in the regular light theme (because use_dark_theme
if False
) respecting the configuration (nothing changes to master), at least on my machine (wayland, kde), is this behaviour different on other plattforms?
The only place i see that is dependent on the value of GUI_QT_COLOR_THEME
is reload_app_stylesheet()
:
use_dark_theme = self.config.GUI_QT_COLOR_THEME == 'dark' \
or self.config.GUI_QT_COLOR_THEME == 'default' and is_system_dark_mode()
if use_dark_theme:
try:
import qdarkstyle
self.app.setStyleSheet(qdarkstyle.load_stylesheet_pyqt6())
except BaseException as e:
use_dark_theme = False
self.logger.warning(f'Error setting dark theme: {repr(e)}')
else:
self.app.setStyleSheet(self._default_qtstylesheet)
The only thing that changes in this pr is that use_dark_theme
gets an additional condition, if GUI_QT_COLOR_THEME
is set to default ('System' setting in GUI) and if the system theme is dark (determined by is_system_dark_mode()
) then also use the dark mode, else just go the same path as if the configuration is Light
.
I see that ColorScheme is also dependent on the system theme (background color) but the behavior stays the same here as if Light
is selected on master.
def is_system_dark_mode() -> bool: | ||
"""Returns True if the system theme is in dark mode, False otherwise.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation for this PR? Is it that when running on a dark system, you want an option to run electrum with the system qt theming but with util.ColorScheme
selecting the dark colors? If so, there is already some related logic here that seems to duplicate your is_system_dark_mode()
function (perhaps that logic is flawed and yours is better -- but maybe then it should be fixed instead :) ):
electrum/electrum/gui/qt/__init__.py
Lines 195 to 198 in 1559129
# Even if we ourselves don't set the dark theme, | |
# the OS/window manager/etc might set *a dark theme*. | |
# Hence, try to choose colors accordingly: | |
ColorScheme.update_from_widget(QWidget(), force_dark=use_dark_theme) |
electrum/electrum/gui/qt/util.py
Lines 1144 to 1150 in 1559129
def has_dark_background(widget): | |
brightness = sum(widget.palette().color(QPalette.ColorRole.Window).getRgb()[0:3]) | |
return brightness < (255*3/2) | |
@staticmethod | |
def update_from_widget(widget, force_dark=False): | |
ColorScheme.dark_scheme = bool(force_dark or ColorScheme.has_dark_background(widget)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation for this PR is that its annoying having to set the theme manually in every application instead of just switching the system theme and have all applications follow it, especially when switching often due to lighting conditions. So this seemed like a small useful change to allow electrum automatically switch between the Light
and Dark
setting depending on the current system theme it is starting in. Am i missing some point here?
Oh yeah i didn't see has_dark_background()
, seems like it does the same thing. Will try to unify them.
Just noticed this doesn't work as expected on gnome (always detects system theme as light), will change the approach of detection... |
(somewhat related: #9690) |
Adds a theme setting to qt gui to use the system theme color instead of a fixed theme, also uses it as default config.
Edit: see #9690, will wait for the qt function to handle this properly