-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Loader with aggregation and visual loading state #555
Changes from all commits
77a3559
703c6eb
f5ee56d
fc5e108
77a8676
2d7e0af
8dfb66d
5bac974
ae85b42
4b89898
6f530a6
2de45f8
4de3464
79957b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,10 @@ def is_filtering_recursible(): | |
|
||
|
||
class SubsetsModel(TreeModel): | ||
|
||
doc_fetched = QtCore.Signal() | ||
refreshed = QtCore.Signal(bool) | ||
|
||
Columns = [ | ||
"subset", | ||
"family", | ||
|
@@ -43,14 +47,19 @@ def __init__(self, grouping=True, parent=None): | |
self._icons = { | ||
"subset": qtawesome.icon("fa.file-o", color=style.colors.default) | ||
} | ||
self._doc_fetching_thread = None | ||
self._doc_fetching_stop = False | ||
self._doc_payload = list() | ||
|
||
self.doc_fetched.connect(self.on_doc_fetched) | ||
|
||
def set_asset(self, asset_id): | ||
self._asset_id = asset_id | ||
self.refresh() | ||
|
||
def set_grouping(self, state): | ||
self._grouping = state | ||
self.refresh() | ||
self.on_doc_fetched() | ||
|
||
def setData(self, index, value, role=QtCore.Qt.EditRole): | ||
|
||
|
@@ -141,17 +150,72 @@ def set_version(self, index, version): | |
"step": version_data.get("step", None) | ||
}) | ||
|
||
def refresh(self): | ||
def fetch_subset_and_version(self): | ||
"""Query all subsets and latest versions from aggregation | ||
|
||
(NOTE) The returned version documents are NOT the real version | ||
document, it's generated from the MongoDB's aggregation so | ||
some of the first level field may not be presented. | ||
|
||
""" | ||
def _fetch(): | ||
_subsets = list() | ||
_ids = list() | ||
for subset in io.find({"type": "subset", | ||
"parent": self._asset_id}): | ||
if self._doc_fetching_stop: | ||
return | ||
_subsets.append(subset) | ||
_ids.append(subset["_id"]) | ||
|
||
_pipeline = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be great if there is (e.g. in avalon.lib) function for this pipeline to be usable in multiple places.
But it is only nice to have, because it is not used in other places yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it could be in the |
||
# Find all versions of those subsets | ||
{"$match": {"type": "version", "parent": {"$in": _ids}}}, | ||
# Sorting versions all together | ||
{"$sort": {"name": 1}}, | ||
# Group them by "parent", but only take the last | ||
{"$group": {"_id": "$parent", | ||
"_version_id": {"$last": "$_id"}, | ||
"name": {"$last": "$name"}, | ||
"data": {"$last": "$data"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add "type" key here, but don't have explanation why :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure :D |
||
"locations": {"$last": "$locations"}, | ||
"schema": {"$last": "$schema"}}}, | ||
] | ||
versions = dict() | ||
for doc in io.aggregate(_pipeline): | ||
if self._doc_fetching_stop: | ||
return | ||
doc["parent"] = doc["_id"] | ||
doc["_id"] = doc.pop("_version_id") | ||
versions[doc["parent"]] = doc | ||
|
||
self._doc_payload[:] = [(subset, versions.get(subset["_id"])) | ||
for subset in _subsets] | ||
self.doc_fetched.emit() | ||
|
||
self._doc_payload[:] = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am curious why this is used EDITED: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is basically an in-place assignment. It would be the same as doing: my_list = ["a", "b", "c"]
my_list.clear() And this replaces the list in place: a = ["a", "b"]
b = a
# Show mutability
a.append("c")
print(b)
# ["a", "b", "c"]
# Assign new value to a does not change value of b, because it's assigning a new list
a = ["aa", "bb"]
print(b)
# ["a", "b", "c"]
# But when using in place assignment it would, because it's changing the same mutable list.
a = ["a", "b"]
b = a
a[:] = ["aa", "bb"]
print(b)
# ["aa", "bb"] Edit: added example above of the in-place assignment that is not just clearing it I believe the reason to do explicit in-place assignments is due to the "mutable list" potentially being shared to another thread and otherwise that one working on an older copy of a list that isn't actually the same list as There should be some informational topics online on replacing lists in-place with regards to its mutability if you're looking for more info. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it's true that there's no need to use in-place assignment, just re-using the same object for no special reason. Will change it for good. |
||
self._doc_fetching_stop = False | ||
self._doc_fetching_thread = lib.create_qthread(_fetch) | ||
self._doc_fetching_thread.start() | ||
|
||
def stop_fetch_thread(self): | ||
if self._doc_fetching_thread is not None: | ||
self._doc_fetching_stop = True | ||
while self._doc_fetching_thread.isRunning(): | ||
pass | ||
|
||
def refresh(self): | ||
self.stop_fetch_thread() | ||
self.clear() | ||
self.beginResetModel() | ||
if not self._asset_id: | ||
self.endResetModel() | ||
return | ||
self.fetch_subset_and_version() | ||
|
||
asset_id = self._asset_id | ||
def on_doc_fetched(self): | ||
self.clear() | ||
self.beginResetModel() | ||
|
||
active_groups = lib.get_active_group_config(asset_id) | ||
active_groups = lib.get_active_group_config(self._asset_id) | ||
|
||
# Generate subset group nodes | ||
group_items = dict() | ||
|
@@ -166,15 +230,10 @@ def refresh(self): | |
group_items[name] = group | ||
self.add_child(group) | ||
|
||
filter = {"type": "subset", "parent": asset_id} | ||
|
||
# Process subsets | ||
row = len(group_items) | ||
for subset in io.find(filter): | ||
|
||
last_version = io.find_one({"type": "version", | ||
"parent": subset["_id"]}, | ||
sort=[("name", -1)]) | ||
has_item = False | ||
for subset, last_version in self._doc_payload: | ||
if not last_version: | ||
# No published version for the subset | ||
continue | ||
|
@@ -203,8 +262,10 @@ def refresh(self): | |
# Set the version information | ||
index = self.index(row_, 0, parent=parent_index) | ||
self.set_version(index, last_version) | ||
has_item = True | ||
|
||
self.endResetModel() | ||
self.refreshed.emit(has_item) | ||
|
||
def data(self, index, role): | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
import os | ||
import datetime | ||
import pprint | ||
import inspect | ||
|
||
from ...vendor.Qt import QtWidgets, QtCore, QtCompat | ||
from ...vendor.Qt import QtWidgets, QtCore, QtGui, QtSvg | ||
from ...vendor import qtawesome | ||
from ... import io | ||
from ... import api | ||
from ... import pipeline | ||
from ... import style | ||
|
||
from .. import lib as tools_lib | ||
from ..delegates import VersionDelegate | ||
|
@@ -44,7 +46,7 @@ def __init__(self, enable_grouping=True, parent=None): | |
top_bar_layout.addWidget(filter) | ||
top_bar_layout.addWidget(groupable) | ||
|
||
view = QtWidgets.QTreeView() | ||
view = SubsetTreeView() | ||
view.setIndentation(20) | ||
view.setStyleSheet(""" | ||
QTreeView::item{ | ||
|
@@ -98,10 +100,15 @@ def __init__(self, enable_grouping=True, parent=None): | |
self.view.setModel(self.family_proxy) | ||
self.view.customContextMenuRequested.connect(self.on_context_menu) | ||
|
||
header = self.view.header() | ||
# Enforce the columns to fit the data (purely cosmetic) | ||
QtCompat.setSectionResizeMode( | ||
header, QtWidgets.QHeaderView.ResizeToContents) | ||
view.setColumnWidth(0, 240) # subset | ||
view.setColumnWidth(1, 120) # family | ||
view.setColumnWidth(2, 100) # version | ||
view.setColumnWidth(3, 120) # time | ||
view.setColumnWidth(4, 100) # author | ||
view.setColumnWidth(5, 80) # frames | ||
view.setColumnWidth(6, 60) # duration | ||
view.setColumnWidth(7, 50) # handles | ||
view.setColumnWidth(8, 50) # step | ||
|
||
selection = view.selectionModel() | ||
selection.selectionChanged.connect(self.active_changed) | ||
|
@@ -126,6 +133,18 @@ def set_grouping(self, state): | |
current_index=False): | ||
self.model.set_grouping(state) | ||
|
||
def set_loading_state(self, loading, empty): | ||
view = self.view | ||
|
||
if view.is_loading != loading: | ||
if loading: | ||
view.spinner.repaintNeeded.connect(view.viewport().update) | ||
else: | ||
view.spinner.repaintNeeded.disconnect() | ||
|
||
view.is_loading = loading | ||
view.is_empty = empty | ||
|
||
def on_context_menu(self, point): | ||
|
||
point_index = self.view.indexAt(point) | ||
|
@@ -309,6 +328,44 @@ def echo(self, message): | |
print(message) | ||
|
||
|
||
class SubsetTreeView(QtWidgets.QTreeView): | ||
|
||
def __init__(self, parent=None): | ||
super(SubsetTreeView, self).__init__(parent=parent) | ||
loading_gif = os.path.dirname(style.__file__) + "/svg/spinner-200.svg" | ||
spinner = QtSvg.QSvgRenderer(loading_gif) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My experience with svg in Qt is that it is slow in comparison to png or gif. Recommendation is to implement rotating QLabel with png, on the other side I guess that one svg won't affect Qt much. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, there's only one, and it's animation playback will be stopped once the loading is completed. So yeah, I think we are safe :P |
||
|
||
self.spinner = spinner | ||
self.is_loading = False | ||
self.is_empty = True | ||
|
||
def paint_loading(self, event): | ||
size = 160 | ||
rect = event.rect() | ||
rect = QtCore.QRectF(rect.topLeft(), rect.bottomRight()) | ||
rect.moveTo(rect.x() + rect.width() / 2 - size / 2, | ||
rect.y() + rect.height() / 2 - size / 2) | ||
rect.setSize(QtCore.QSizeF(size, size)) | ||
painter = QtGui.QPainter(self.viewport()) | ||
self.spinner.render(painter, rect) | ||
|
||
def paint_empty(self, event): | ||
painter = QtGui.QPainter(self.viewport()) | ||
rect = event.rect() | ||
rect = QtCore.QRectF(rect.topLeft(), rect.bottomRight()) | ||
qtext_opt = QtGui.QTextOption(QtCore.Qt.AlignHCenter | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. line break after binary operator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either before or after line break, Hound barks. 😠 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use line break before so operators are on new line (both won't work with hound if hound is not set ;) ). I would personally in this case use:
|
||
QtCore.Qt.AlignVCenter) | ||
painter.drawText(rect, "No Data", qtext_opt) | ||
|
||
def paintEvent(self, event): | ||
if self.is_loading: | ||
self.paint_loading(event) | ||
elif self.is_empty: | ||
self.paint_empty(event) | ||
else: | ||
super(SubsetTreeView, self).paintEvent(event) | ||
|
||
|
||
class VersionTextEdit(QtWidgets.QTextEdit): | ||
"""QTextEdit that displays version specific information. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_fetch
should be class method, there is not much reason to have it inside.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's there because it acts more like a local lambda written as a function.
Also, if it were put on the class it shouldn't be a classmethod right? Because it uses variables/functions of the instance of the class inside of it (not how it refers to self). So if put on the class it would be a regular method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant as object method
def _fetch(self):
, not as class method, my fault.I see advantage of lamda but there's no advantage of that here, all
_fetch
need isself
. Methodfetch_subset_and_version
has 4 lines in fact. All what these lines do, is preparation and launching another method in thread, but because the definition of launched method is inside it's harder to read (for me) what's really happening inside.This is "formatting" comment of my preferences. I have no ambition to change mindset because I know it's hard to change mine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right to me. Nesting a function tells me that it is only useful to the parent function, which seems to be the case here. If you put it in the class or instance, I would expect it to be useful standalone and by others, and require its own tests. The smaller the interface the better. Also, if Python did support multiline lambdas, this would be a good place for it.