[ui] Refactor the access to the list of recent project files #2637
Conversation
The property itself only accesses a list instead of reading the QSettings every single time it is called. It is set once during the application's launch with a full reading, and is then updated without performing extra reading operations every time a file is added to or removed from the list.
When the list of recent project files is updated, there is no attempt to retrieve its thumbnail as the update is said to be "minimal". This minimal update is justified by the lack of use for the thumbnails in the Application part of Meshroom. Thumbnails are only useful when displaying the Homepage, hence their retrieval during Meshroom's initialization. There is only a need to update them once we want to display the Homepage again. The Homepage thus requests an update of the thumbnails before setting its model. If there have been some updates to the list of recent project files, the reading of the QSettings is performed again and thumbnails are retrieved whenever it is possible. Otherwise, nothing happens to prevent useless readings.
Instead of doing it directly within the `_getRecentProjectFiles` method, add a function that attempts to retrieve a thumbnail corresponding to an input project file path.
…jects There is no need to read the QSettings again as the content of `self._recentProjectFiles` reflects their content accurately and they do not store any thumbnail-related information. QSettings will now only be read once, upon Meshroom's start.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2637 +/- ##
========================================
Coverage 69.93% 69.93%
========================================
Files 121 121
Lines 7088 7088
========================================
Hits 4957 4957
Misses 2131 2131 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Great improvement over the existing code.
In addition to the review comments, it could be great to;
- Move all the code modifying the list of recent projects to the Python side - this should not be the responsibility of the UI, but rather of the code that loads projects.
And few extra notes (could be for later PRs):
- Recent project file management is a good candidate for having its own class, rather than living within the MeshroomApp class. That would improve the separation of responsibilities.
- Projects could also be presented by a dataclass rather than a raw dictionary for improved code readability.
|
|
||
| return thumbnail | ||
|
|
||
| def _getRecentProjectFiles(self) -> list[dict[str, str]]: |
There was a problem hiding this comment.
For clarity, I would rename this one _getRecentProjectFilesFromSettings.
| thumbnail. | ||
|
|
||
| Returns: | ||
| list[dict[str, str]]: The list containing dictionaries of the form {"path": "/path/to/project/file", |
There was a problem hiding this comment.
As a general rule of thumbs, let's keep the typing information only as annotation typings of the function and not in docstrings (to avoid the double maintenance).
| with open(filepath) as file: | ||
| fileData = json.load(file) | ||
| # Find the first CameraInit node | ||
| fileData = fileData["graph"] |
There was a problem hiding this comment.
This code could be impacted by project file format changes.
As we're accessing dictionary keys, we should be cautious of error handling here.
Either by adding exception types to the except statement below, or safely get data from the dictionary.
Also, try to minimize the duplicated code to access dictionary items to improve readability and simplify maintenance.
graphData = fileData.get("graph", {})
for node in graphData.values():
if node.get("nodeType") != "CameraInit":
continue
if viewpoints := node.get("inputs", {}).get("viewpoints"):
return viewpoints[0].get("path", "")
return ""(code snippet not tested, but for reference)
| if len(fileData[node]["inputs"]["viewpoints"]) > 0: | ||
| thumbnail = fileData[node]["inputs"]["viewpoints"][0]["path"] | ||
| break | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
We need to handle more failure cases here, as we don't want this function to ever raise.
For instance, if the Meshroom project file got corrupted, json.load can fail too.
| model: { | ||
| // Request latest thumbnail paths | ||
| if (mainStack.currentItem instanceof Homepage) | ||
| MeshroomApp.updateRecentProjectFilesThumbnails() |
There was a problem hiding this comment.
This component should not access mainStack directly.
We should have the updateRecentProjectFilesThumbnails called when the Homepage component is created/made visible (if it stays alive even when not shown), but not part of this property binding.
| # Flag set to True if, for all the project files in the list, thumbnails have been retrieved when they | ||
| # are available. If set to False, then all the paths in the list are accurate, but some thumbnails might | ||
| # be retrievable | ||
| self._updatedRecentProjectFilesThumbnails = True |
There was a problem hiding this comment.
[per-project thumbnail status]
Instead of having this flag, what do you think of a special state in the data model for recent projects (eg: "thumbnail": None) to indicate that we have not tried to query the thumbnail yet?
That would allow to only query them on a per project file basis later on, rather than considering that state global for all of them.
| # keep only the 40 first elements | ||
| projects = projects[0:40] | ||
| # Only keep the first 40 projects | ||
| if len(projects) > 40: |
There was a problem hiding this comment.
This value should be stored in a variable.
| def _updateRecentProjectFilesThumbnails(self) -> None: | ||
| projects = self._recentProjectFiles | ||
| for project in projects: | ||
| path = project["path"] |
There was a problem hiding this comment.
[per-project thumbnail status]
In the per-project thumbnail status approach, we could just iterate over projects and have a test like:
if project["thumbail"] is None:| self._updatedRecentProjectFilesThumbnails = True | ||
|
|
||
| def _updateRecentProjectFilesThumbnails(self) -> None: | ||
| projects = self._recentProjectFiles |
There was a problem hiding this comment.
I'm not sure to get why we have this projects variable (instead of working directly with the class member).
| del projects[idx] # If so, delete its entry | ||
|
|
||
| # Insert the newest entry at the top of the list | ||
| projects.insert(0, {"path": projectFileNorm, "thumbnail": ""}) |
There was a problem hiding this comment.
[per-project thumbnail status]
Here, we could set "thumbnail" to None to indicate that the thumbnail has not been queried yet.
Description
This pull request addresses some performance issues that were detected when interacting with the list of recent project files.
It mostly focuses on refactoring the access to the list of recent project files, which involved reading the QSettings every single time the content of the list was to be accessed, and accessing it far too many times.
It also decouples the need for thumbnails between the homepage and the application: while the homepage needs access to thumbnails (if available) to set up the list of recent project files, the application has no use for them. When the list of project files is edited, there is no need to retrieve the corresponding thumbnail (if it exists) unless the current view is the homepage's.
Finally, it prevents the homepage from performing any such request for thumbnails unless it is actually being displayed.
Performance Summary
QSettings used to be read every single time
recentProjectFileswas called:=> QSettings are now only read once throughout the lifetime of the Meshroom instance.
Features list
Implementation remarks
Prior to this PR, a call to the
recentProjectFilesproperty necessarily involved a full reading of the QSettings, which provides for each entry both a path to the project file itself and a path to its thumbnail (if it exists). Upon the launch of Meshroom alone, if there were 40 project files in the QSettings, these QSettings were fully accessed and read 40 times just to set the homepage properly.recentProjectFilesis now only an accessor to a list that is filled and edited in other calls, and which is initialized with a full reading of the QSettings when Meshroom is started, and never again during the lifetime of the instance. QSettings are thus only read once.From there, any project file that is added or deleted leads to an update of the QSettings, but these settings are not read again to update the content of
recentProjectFiles:recentProjectFilesis updated directly within these methods, ensuring it is always accurately reflecting the content of the QSettings.When a project file is added, only the path to the project is added to the QSettings; there is no attempt to retrieve the thumbnail. However, a flag indicating that there may be some thumbnail paths to retrieve will be set.
When the homepage will become the current view again, it will request an update of the thumbnails. If the flag is set (meaning at least a file has been added to the list), all the project files in
recentProjectFileswill be parsed in an attempt to retrieve their thumbnails. Otherwise (meaning that files have either been deleted or nothing has happened), nothing will happen.