fix: SG-43079: Optimize time it takes to display thumbnails when we re-display them - #1298
fix: SG-43079: Optimize time it takes to display thumbnails when we re-display them#1298deltag0 wants to merge 12 commits into
Conversation
32242df to
f9fe721
Compare
| // source and input each get their own render queue/timer/state | ||
| ThumbnailManager _source; | ||
| ThumbnailManager _input; | ||
| QDockWidget _dockWidget; |
There was a problem hiding this comment.
Please try to align the members whenever possible
| QMenu _createMenu; | ||
| QMenu _folderMenu; | ||
|
|
||
| ThumbnailRenderer thumbnailRenderer; |
There was a problem hiding this comment.
To be consistent, if it's a member, then it should be prepended with an underscore:
_thumbnailRenderer
| thumbnailRenderer._input.renderQueue = string[](); | ||
| thumbnailRenderer._input.progressiveRenderDeferred = string[](); | ||
| thumbnailRenderer._input.progressiveRenderPreviews = SourcePreviewWidget[](); | ||
| thumbnailRenderer._input.renderIdx = 0; |
There was a problem hiding this comment.
The exact same sequence is repeated on line 2198. So it is a good candidate to encapsulate it into a dedicated method of the "ThumbnailManager" class.
| } | ||
| } | ||
|
|
||
| class: ThumbnailManager |
There was a problem hiding this comment.
Naming things is hard but I am not sure it is the best name for this class. Does it actually manage thumbnails ?
There was a problem hiding this comment.
Renamed to PreviewRenderQueue
| // source and input each get their own render queue/timer/state | ||
| ThumbnailManager _source; | ||
| ThumbnailManager _input; | ||
| QDockWidget _dockWidget; |
There was a problem hiding this comment.
I don't think the ThumbnailRender should own the _dockWidget. My understanding is that the ThumbnailRender should be optional - It should only be used when _previewsEnabled is true.
I would pass _dockWidget as an argument to the ThumbnailRenderer's constructor.
What do you think @eloisebrosseau ?
There was a problem hiding this comment.
I agree with you. The Session Manager dock widget should not be inside the ThumbnailRenderer class and only pass as an argument to use it when needed
There was a problem hiding this comment.
That's fine, but it seemed like the SessionManagerMode class is a God class, it basically owns every single object. If that's fine, we can keep it that way.
| // source and input each get their own render queue/timer/state | ||
| ThumbnailManager _source; | ||
| ThumbnailManager _input; | ||
| QDockWidget _dockWidget; |
There was a problem hiding this comment.
I agree with you. The Session Manager dock widget should not be inside the ThumbnailRenderer class and only pass as an argument to use it when needed
| if (_thumbnailRenderer._source.renderIdx < _thumbnailRenderer._source.renderQueue.size()) | ||
| { | ||
| _thumbnailRenderer._source.renderTimer.start(0); | ||
| } |
There was a problem hiding this comment.
itemOfNode can return nil so you should validate its content like it's done in the rest of the code before assigning item in _viewTreeView.setIndexWidget(_viewModel.indexFromItem(item), w);
| } | ||
| } | ||
|
|
||
| method: resetPreviews (void;) |
There was a problem hiding this comment.
resetPreviews is not called once, you should remove it
There was a problem hiding this comment.
FYI, I still see this method in your latest changes
There was a problem hiding this comment.
I removed it from the generated file, fixed now
fc4be0a to
5a85405
Compare
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <ioan.1931@gmail.com> Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <ioan.1931@gmail.com> Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <ioan.1931@gmail.com> Signed-off-by: deltag0 <victor.terme@autodesk.com>
Signed-off-by: deltag0 <ioan.1931@gmail.com> Signed-off-by: deltag0 <victor.terme@autodesk.com>
5a85405 to
92bed4c
Compare
3 participants
fix: Optimize time it takes to display thumbnails when we re-display them
Summarize your change.
Instead of blocking the main thread and building the entire session manager from scratch with all thumbnails loaded if it's closed/updated, load thumbnails progressively, letting the main thread handle other events.
Describe the reason for the change.
When loading many files in RV, and closing/opening the session manager, it would freeze the application for an unreasonable amount of time.
Describe what you have tested and on which operating system.
Mac CY2025
List of changes
The goal was that when we open the session manager after all thumbnails are closed, we display the placeholder first, add the task to load the thumbnails in a queue, to run them alongside the main thread. We could also load the first 10 or so thumbnails before opening the session manager to avoid showing a placeholder.