Add color theme toggle to pilot#788
Conversation
There was a problem hiding this comment.
- Use
Qt.ColorSchemeenum. - Balance
if/elif/else. - Fix a bug: Both dark and system are assigned number 1 and treat the same way.
- Keep
button_groupas a member data, otherwise it will be garbage collected after the end ofinit_ui()function. -
_Controller.appearance_openis not used. Remove the dead code. - Discuss: Why duplicating construction of
AppearanceDialog? - Rename
AppearanceDialog.ok()toon_ok()like the other functions in the class. - Discuss: Is it intended to create
AppearanceDialogwithout a parent?
@tigercosmos please help take a look.
Screenshots. Pretty nice.
| dark_mode_button.setFocusPolicy(Qt.NoFocus) | ||
| system_mode_button.setFocusPolicy(Qt.NoFocus) | ||
|
|
||
| color_scheme_name = str(self.qApp.styleHints().colorScheme()) |
There was a problem hiding this comment.
self.qApp.styleHints().colorScheme() already returns Qt.ColorScheme. See https://doc.qt.io/qt-6/qt.html#ColorScheme-enum , also available in PySide https://doc.qt.io/qtforpython-6/PySide6/QtCore/Qt.html#PySide6.QtCore.Qt.ColorScheme
Code to compare the enum will be more robust:
scheme = self.qapp.styleHints().colorScheme()
if scheme == Qt.ColorScheme.Light:
light_mode_button.setChecked(True)
elif scheme == Qt.ColorScheme.Dark:
dark_mode_button.setChecked(True)
else: # Qt.ColorScheme.Unknown -> "System"
system_mode_button.setChecked(True)You forgot Unknown (System). Please add it back.
We should also use balanced if/elif/else most of the time.
|
|
||
| button_group = QButtonGroup() | ||
| button_group.addButton(light_mode_button, 0) | ||
| button_group.addButton(dark_mode_button, 1) |
There was a problem hiding this comment.
A bug: Both dark and system are assigned number 1 and treat the same way. A fix is like:
button_group.addButton(dark_mode_button, 1)
button_group.addButton(system_mode_button, 2)| if color_scheme_name == "ColorScheme.Dark": | ||
| dark_mode_button.setChecked(True) | ||
|
|
||
| button_group = QButtonGroup() |
There was a problem hiding this comment.
Keep button_group as a member data, otherwise it will be garbage collected after the end of this init_ui() function (because it is not added to another widget to keep the live).
| self.canvas = None | ||
| self.openprofiledata = None | ||
| self.runprofiling = None | ||
| self.appearance_open = False |
There was a problem hiding this comment.
appearance_open is not used. Remove the dead code.
| self.openprofiledata.populate_menu() | ||
| self.runprofiling.populate_menu() | ||
|
|
||
| self.appearance_dialog = AppearanceDialog() |
There was a problem hiding this comment.
Why do you construct AppearanceDialog at two places, here and at line 168?
| self.qApp = QApplication.instance() | ||
| self.init_ui() | ||
|
|
||
| def ok(self): |
There was a problem hiding this comment.
How about naming it on_ok() like the rest of the functions?
| def on_open_appearance(self): | ||
| self.appearance_open = True | ||
| if not self.appearance_dialog: | ||
| self.appearance_dialog = AppearanceDialog() |
There was a problem hiding this comment.
AppearanceDialog is created without a parent, and won't be owned by the main window. Is it intented?
2 participants
Ref issue #510
Last PR #678
After some cleaning in my fork, the last PR was force closed, so I created this new one.
It's the same function and usage as presented in the issue, while the codebase is up-to-date and the change has passed CI runs in my fork.