Conversation
|
What exactly is the issue you want to fix? Ideally, the theme / file would always be valid if we call set_theme(). When would the string be empty or None? When the user closed the dialog? |
|
Yup, so when the dialog is cancelled and empty string/None is returned, then these help fix those cases. Well, a small part of this PR does. Should I reduce the amount of diffs so it's a bit more focused? |
| message_dark = self.tr('Open dark wallpaper') | ||
| file_name, _ = QFileDialog.getOpenFileName( | ||
| self, message_dark if dark else message_light, | ||
| self, self.tr(f'Open {"dark" if dark else "light"} wallpaper'), |
There was a problem hiding this comment.
self.tr() gets translations for that message. Since different languages use different, grammar, we shouldn't do that
|
|
||
| @property | ||
| def theme_light(self) -> str: | ||
| def theme_light(self) -> str | None: |
There was a problem hiding this comment.
This should never be None, I'd rather return an empty string here
|
|
||
| theme = self.theme_dark if dark else self.theme_light | ||
| self.set_theme(theme) | ||
| if (theme := self.theme_dark if dark else self.theme_light): |
There was a problem hiding this comment.
Sorry, I don't really like that syntax. I prefer to have one statement per line, that's less confusing to me.
| inputs_wallpaper = group_wallpaper.findChildren(QtWidgets.QLineEdit) | ||
| i = 1 if dark else 0 | ||
| inputs_wallpaper[i].setText(file_name) | ||
| if file_name: |
There was a problem hiding this comment.
Yeah, we should definitely handle empty strings better. I would say we should disable the plugin if no valid input was provided.
For example:
- add a
is_valid() -> boolmethod to the plugin base class - Call that in
_write_config() - If false, don't write and disable the plugin.
Other plugins with text input will have the same issue, so implementing it there makes sense. If available_themes() returns a non-empty dict, we can assume True (we use drop-down menus in that case). If you don't want to implement that for every plugin, just return true in the base class and I will do the rest after merge.
There was a problem hiding this comment.
I just pushed an implementation for that to master. If any plugin has any theme with an empty string and is enabled, the user will see an error when saving.
2 participants
i.e. when
theme/value/fileNameis not None/empty stringJust a minor quality of life for the code.