Autosave and Performance Settings #154
Conversation
| self.updateSettings() | ||
|
|
||
| # Give settings to classes that need it. | ||
| ThumbRenderer.settings = self.settings |
There was a problem hiding this comment.
it would be much cleaner to pass the existing QSettings instance as a parameter to constructor, or pass the whole QTDriver which has the .settings property similarly as PreviewPanel does it:
TagStudio/tagstudio/src/qt/ts_qt.py
Line 443 in b8d72a6
so it would be something like:
# self refers to `QtDriver`
self.renderer = ThumbRenderer(self)
# then in ThumbRenderer:
def __init__(self, driver):
self.driver = driver
# and then...
if self.driver.settings.contains("render_thumbnails") and self.driver.settings.value("render_thumbnails") == "true":
|
It feels like this PR is named improperly - the main part here are changes in functionality (autosave timer, rendering thumbnails switch). The settings window is nice -but complementary- thing in comparison. As such, it would be better to create separate PR for each introduced feature (timer, rendering), so it can be tested in isolation. Regarding the thumbnails rendering option - what is the use case to disable thumbnails rendering in -media library-? I mentioned previously that I dont like having all CPU cores busy with thumbnails rendering when the application starts, but this doesnt solve my use-case. I want to see the thumbnails, but only when I actually select the library I want. The more reasonable option I can imagine here would be option for number of threads which will be used for thumbnails rendering. |
A thread limit is amazing idea, I will add it. Also, I added "Do not Render Thumbnails" not because of your issue, but an issue I have been having with internet and storage performance. You see, some of my files on my computer are in the cloud because of OneDrive, and because TagStudio is opening them to get the thumbnails, all of those files get downloaded to my computer. This is just a band-aid for my issue, at least until we have a solution for online files. |
… file and program.
|
|
||
| # Settings Menu ======================================================== | ||
| open_settings_action = QAction("&Settings", menu_bar) | ||
| open_settings_action.triggered.connect(lambda: self.openSettings()) |
There was a problem hiding this comment.
| open_settings_action.triggered.connect(lambda: self.openSettings()) | |
| open_settings_action.triggered.connect(self.openSettings) |
| @@ -164,16 +166,17 @@ class QtDriver(QObject): | |||
| """A Qt GUI frontend driver for TagStudio.""" | |||
|
|
|||
| SIGTERM = Signal() | |||
| settings_window = None | |||
| autosave_timer = QTimer() | |||
There was a problem hiding this comment.
The instance of timer should be created in constructor instead of here. Moreover you're creating another one later (line 519, so it creates second instance) which would overwrite this existing instance, so that's extra work for nothing.
| # self.main_window = None | ||
| # self.main_window = Ui_MainWindow() | ||
|
|
||
| # self.autosave_timer.setTimerType(QtCore.Qt.TimerType) |
There was a problem hiding this comment.
why is this commented out?
| max_threads = os.cpu_count() | ||
| if self.settings.value("limit_threads", False, bool): | ||
| max_threads = self.settings.value("max_threads", os.cpu_count(), int) | ||
| else: | ||
| max_threads = os.cpu_count() |
There was a problem hiding this comment.
as mentioned previously please make one Pull Request per feature. So this one should be either for the timer, or for the threads count, not both.
| app.exec_() | ||
|
|
||
| self.shutdown() | ||
|
|
||
| def openSettings(self): |
There was a problem hiding this comment.
I know it's difficult to pick the style since PySide is using camelCase, but please try to follow PEP-8 wherever possible as mentioned in README.
| def openSettings(self): | |
| def open_settings(self): |
| def openSettings(self): | ||
| self.settings_window = Ui_Settings(self.main_window) | ||
| self.settings_window.setSettings(self.settings) | ||
| self.settings_window.accepted.connect(lambda: self.closeSettings()) |
There was a problem hiding this comment.
| self.settings_window.accepted.connect(lambda: self.closeSettings()) | |
| self.settings_window.accepted.connect(self.close_settings) |
| @@ -1321,6 +1364,11 @@ def open_library(self, path): | |||
| self.cur_query: str = "" | |||
| self.selected.clear() | |||
| self.preview_panel.update_widgets() | |||
| if ( | |||
| self.settings.contains("autosave") | |||
| and self.settings.value("autosave") == True | |||
There was a problem hiding this comment.
I'm suspecting this wont work, because by default the value you get from settings is string, but you're comparing it to bool
| @@ -0,0 +1,330 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
add this file into pyproject.toml -> ruff.tool.exclude beside the other autogenerated files
| # Use as static variable. | ||
| settings = QSettings() |
There was a problem hiding this comment.
dont use a static variable, see my previous comment: #154 (comment)
Or is this some paradigm I am not aware of? Honest question, I'm not that familiar with PySide guidelines.
| @@ -112,156 +116,172 @@ def render( | |||
| ), | |||
| math.floor(12 * ThumbRenderer.font_pixel_ratio), | |||
| ) | |||
|
|
|||
| if isLoading or filepath: | |||
| if ThumbRenderer.settings.value("render_thumbnails", False, bool): | |||
There was a problem hiding this comment.
this variable is called render_thumbnails but when it's true it doesnt render thumnails. So either this condition should be flipped, or the variable name should be changed.
I was adding auto-saves to TagStudio when I realized, "We do not have any settings window!" So I created one.
It is very simple and only contains three settings, one for auto-saving and minutes between each auto-save. I also added a Performance tab and added the option to not render thumbnails in the main view.
Hope you like it :)