-
-
Notifications
You must be signed in to change notification settings - Fork 412
PICARD-2300: Add qtdbus
dark theme detect
#2692
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?
PICARD-2300: Add qtdbus
dark theme detect
#2692
Conversation
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.
This looks fine to me, other than a few instances of catching the general Exception
, which would be nice to tighten up a bit if possible.
picard/ui/theme_detect.py
Outdated
# Try D-Bus first (secure method) | ||
try: | ||
detector = get_dbus_detector() | ||
result = detector.detect_freedesktop_portal_color_scheme() | ||
if result is not None: | ||
return result | ||
except Exception: # noqa: BLE001 | ||
log.debug("Unable to detect `freedesktop` color scheme with dbus.") |
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.
This largely duplicates the code from Lines 60-67. If there was a third test using similar code, I would suggest some refactoring, but it's likely not worth it in this case.
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.
Refactored for DRY and removed blind exception catch: e6c287e
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.
This is great. Using dbus will allow to check the theme from inside e.g. flatpak.
Some minor comments for discussion only.
@@ -201,6 +242,10 @@ def detect_lxqt_dark_wrapper() -> bool: | |||
def get_linux_dark_mode_strategies() -> list: | |||
"""Return the list of dark mode detection strategies in order of priority.""" | |||
return [ | |||
# Pure D-Bus methods (will gracefully fail if D-Bus unavailable) | |||
detect_freedesktop_color_scheme_dbus, | |||
detect_gnome_color_scheme_dbus, |
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.
One thing we could optimize is that we don't need to initialize DBusThemeDetector
twice. It handles both interfaces already.
So either we add a single dbus strategy, that checks both the freedesktop and gnome interfaces, or we initialize DBusThemeDetector
first and have the detect methods accept it as a parameter (and use partial to apply it here).
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.
So actually, this pattern already uses a singleton. DBusThemeDetector
is only ever instantiated once.
picard\ui\theme_detect_qtdbus.py:160-169
# Global D-Bus detector instance
_dbus_detector = None
def get_dbus_detector() -> DBusThemeDetector:
"""Get or create the global D-Bus theme detector instance."""
global _dbus_detector
if _dbus_detector is None:
_dbus_detector = DBusThemeDetector()
return _dbus_detector
Summary
Problem
Existing linux dark theme detection uses
gsettings
, calling insecure subprocesses and reading files/services that may not exist. Added support forqtdbus
, which should be more secure, with fallback to legacy strategies.Note: Couldn't implement
dbus-python
, as that requires system tools to be installed.Solution
Dark Theme Detection Implementation:
DBusThemeDetector
class that communicates with freedesktop portal and GNOME dconf servicesComprehensive Test Coverage:
test_theme_detect_qtdbus.py
with 734 lines of test coverageTechnical Features:
Action
Additional actions required: