Code review
-
from PySide2 import QtCore, QtWidgets class UiModelCheck(QtWidgets.QWidget): def __init__(self): super().__init__(parent=None) self.check_asset_pushbutton = QtWidgets.QPushButton("Check Asset") self.info_plaintextedit = QtWidgets.QPlainTextEdit() self.info_clear_pushbutton = QtWidgets.QPushButton("Clear logs") self.checks_scrollarea = QtWidgets.QScrollArea() self.constraint_splitter = QtWidgets.QSplitter(QtCore.Qt.Horizontal) self.constraint_checkbox = QtWidgets.QCheckBox("No Constraints") self.constraint_button = QtWidgets.QPushButton("Delete All Constraints") self.constraint_color_button = QtWidgets.QPushButton() self.constraint_splitter.addWidget(self.constraint_checkbox) self.constraint_splitter.addWidget(self.constraint_button) self.constraint_splitter.addWidget(self.constraint_color_button) self.constraint_splitter.setSizes([195, 265, 37]) self.pivot_points_splitter = QtWidgets.QSplitter(QtCore.Qt.Horizontal) self.center_pivot_points_check = QtWidgets.QCheckBox( "Check Pivot points are centered" ) self.center_pivot_points_button = QtWidgets.QPushButton( "Center Pivots on all objects" ) self.center_pivot_points_float_precision_button = QtWidgets.QPushButton( "Float Precision fix" ) self.center_pivot_color_button = QtWidgets.QPushButton() self.pivot_points_splitter.addWidget(self.center_pivot_points_check) self.pivot_points_splitter.addWidget(self.center_pivot_points_button) self.pivot_points_splitter.addWidget(self.center_pivot_points_float_precision_button) self.pivot_points_splitter.addWidget(self.center_pivot_color_button) # Create a widget to contain the splitter inside the scroll area self.scrollarea_widget = QtWidgets.QWidget() self.scrollarea_layout = QtWidgets.QVBoxLayout(self.scrollarea_widget) self.scrollarea_layout.addWidget(self.constraint_splitter) self.scrollarea_layout.addWidget(self.pivot_points_splitter) self.scrollarea_layout.addStretch() # Set the scroll area widget self.checks_scrollarea.setWidget(self.scrollarea_widget) self.checks_scrollarea.setWidgetResizable( True ) # Make the scroll area resizable self.fix_pushbutton = QtWidgets.QPushButton("Fix All Issues") self.info_verticallayout = QtWidgets.QVBoxLayout() self.info_verticallayout.addWidget(self.info_plaintextedit) self.info_verticallayout.addWidget(self.info_clear_pushbutton) self.checks_info_horizontallayout = QtWidgets.QHBoxLayout() self.checks_info_horizontallayout.addWidget(self.checks_scrollarea) self.checks_info_horizontallayout.addLayout(self.info_verticallayout) self.settings = QtCore.QSettings("Trial", "PushButton") self.layout = QtWidgets.QVBoxLayout() self.layout.addWidget(self.check_asset_pushbutton) self.layout.addLayout(self.checks_info_horizontallayout) self.layout.addWidget(self.fix_pushbutton) self.setLayout(self.layout) self.resize(1200, 50) def closeEvent(self, event): self.save_splitter_state() print(self.constraint_splitter.sizes()) event.accept() def save_splitter_state(self): self.settings.setValue("splitterSizes", self.constraint_splitter.saveState()) def restore_splitter_state(self): splitter_sizes = self.settings.value("splitterSizes") if splitter_sizes is not None: self.splitter.restoreState(splitter_sizes) w = UiModelCheck() w.show()
can you review the code?
- My general queries- I am launching this inside of autodesk maya - I will be saving it in a folder that is part of maya environment and then using. is this a good approach to launch the UI or is there a better way?
import module name win = module name.class name() win.show()
- i first print the splitter value while closing the tool and i copy paste the values as default,is there a better way?
- I have around 20 more splitters to add(containing checkbox, button, color button), so should I create a separate splitter class and store splitter class in a variable of
UiModelCheck
and then keep using them for 20 such splitters?
any other details if you spot please do let me know. do provide a code snippet, example or document page. I want to improve my code as much as possible.
thank you.
-
@blossomsg said in Code review:
I have around 20 more splitters to add(containing checkbox, button, color button), so should I create a separate splitter class and store splitter class in a variable of UiModelCheck and then keep using them for 20 such splitters?
What? :D
Have you triedQWidget
withQLayout
?!I don't see a reason to use that much
QSplitters
?!
What should this program do anyway? -
C Christian Ehrlicher moved this topic from General and Desktop on
-
Hello @Pl45m4 ,
This tool is a teccheck tool, this will help artist to check their scene file for issues that needs to be fixed.
The reason why i am using splitters, because the size of buttons don't align in one line. this was the earlier version. With splitters all look clean. let me know if it makes sense. -
from PySide2 import QtWidgets class UiModelCheck(QtWidgets.QWidget): def __init__(self): super().__init__(parent=None) self.check_asset_pushbutton = QtWidgets.QPushButton("Check Asset") self.info_plaintextedit = QtWidgets.QPlainTextEdit() self.info_clear_pushbutton = QtWidgets.QPushButton("Clear logs") self.checks_scrollarea = QtWidgets.QScrollArea() self.constraint_checkbox = QtWidgets.QCheckBox("No Constraints") self.constraint_button = QtWidgets.QPushButton("Delete All Constraints") self.constraint_color_button = QtWidgets.QPushButton() self.constraint_color_button.setSizePolicy( QtWidgets.QSizePolicy.Fixed, QtWidgets.QSizePolicy.Fixed ) self.center_pivot_points_checkbox = QtWidgets.QCheckBox( "Check Pivot points are centered" ) self.center_pivot_points_button = QtWidgets.QPushButton( "Center Pivots on all objects" ) self.center_pivot_points_float_precision_button = QtWidgets.QPushButton( "Float Precision fix" ) self.center_pivot_color_button = QtWidgets.QPushButton() self.center_pivot_color_button.setSizePolicy( QtWidgets.QSizePolicy.Fixed, QtWidgets.QSizePolicy.Fixed ) # Create a widget to maintain the items in scrollarea. self.scrollarea_widget = QtWidgets.QWidget() self.scrollarea_layout = QtWidgets.QGridLayout(self.scrollarea_widget) self.scrollarea_layout.addWidget(self.constraint_checkbox, 1, 1) self.scrollarea_layout.addWidget(self.constraint_button, 1, 1, 2, 3) self.scrollarea_layout.addWidget(self.constraint_color_button, 1, 4) self.scrollarea_layout.addWidget(self.center_pivot_points_checkbox, 2, 1) self.scrollarea_layout.addWidget(self.center_pivot_points_button, 2, 2) self.scrollarea_layout.addWidget( self.center_pivot_points_float_precision_button, 2, 3 ) self.scrollarea_layout.addWidget(self.center_pivot_color_button, 2, 4) # Set the scroll area widget self.checks_scrollarea.setWidget(self.scrollarea_widget) self.checks_scrollarea.setWidgetResizable( True ) # Make the scroll area resizable self.fix_pushbutton = QtWidgets.QPushButton("Fix All Issues") self.info_verticallayout = QtWidgets.QVBoxLayout() self.info_verticallayout.addWidget(self.info_plaintextedit) self.info_verticallayout.addWidget(self.info_clear_pushbutton) self.checks_info_horizontallayout = QtWidgets.QHBoxLayout() self.checks_info_horizontallayout.addWidget(self.checks_scrollarea) self.checks_info_horizontallayout.addLayout(self.info_verticallayout) self.layout = QtWidgets.QVBoxLayout() self.layout.addWidget(self.check_asset_pushbutton) self.layout.addLayout(self.checks_info_horizontallayout) self.layout.addWidget(self.fix_pushbutton) self.setLayout(self.layout) def closeEvent(self, event): event.accept()
I guessed you would like the updated code, let me know if i can refactor the code in a better way
and how should i inherit this in a new class?- good practice
-
@blossomsg said in Code review:
and how should i inherit this in a new class?- good practice
What does this mean? You have a class
UiModelCheck
which inherits/derives fromQtWidgets.QWidget
. If you want to inherit fromUiModelCheck
(to create a "grandchild" class,NewClass <- UiModelCheck <- QWidget
) then you may do so. -
Okay that is one of the approaches, actually, currently in my experience
for eg:
approach you providedclass UiModelCheck(QtWidgets.QWidget): pass class NewClass(UiModelCheck) pass
Approach where certain reviewers have suggested me to keep the logic of UI and functions separate
class UiModelCheck(QtWidgets.QWidget): pass class NewClass(object) self.mod_ui = UiModelCheck pass
I have mostly used the first approach, but I have been really confused with inheriting approaches, alot of people are telling me second one or some other random that i have not seen in any other place eg: qtforum or stack, so thought of clearning my doubt over here
-
@blossomsg It really depends. If you want NewClass to be a widget then inherit UiModelCheck (first approach).
-
@blossomsg said in Code review:
any more suggestions?
Don't eat yellow snow :)
It's your app after all, so we dont't know when you are happy with the outcome...
If it works for you now and it behaves like expected, it's fine I guess. -
-
@blossomsg said in Code review:
I have been told many a times my codes are not well written
A variant (don't know if improvement or not) is to use a UI file and create your form design there.
Same choice when writing GUIs in C++...Then you can see what you're about to get.
Design with QtDesigner or the integrated Designer in QtCreator and then use the UI converter tool to convert your UI to a GUI .py class... or don't compile and useQUiLoader
to load and create your widget at runtime.As I'm not the most experienced Qt Python (PySide/PyQt) user, I can't tell what approach is the best... I've heard some Python geeks condemning the UI-to-py-compiling approach, even though it's claimed to be the "standard", as it's not clean "Python style" and violates some principles of this language (use of external compilation tools, time consuming, annoying blah blah)... but I don't know :)
Anything Qt related for Python is provided by "wrappers" as Qt's "main" language is C++... so can't tell what is good practice when used with Python.
I can imagine the wholeQObject
/ MOC mechanism is design-wise in conflict with pure script/interpreted programming languages... but it is how it works and otherwise you wouldn't be able to use Qt in Python.As always, people will argue about that (and everything else), and in the end, it's a matter of taste what you use and how you do stuff to reach your goal.
-
@blossomsg
The first approach --- subclassing --- is appropriate ifNewClass
IS aUiModelCheck
widget.
The second approach --- encapsulating, to some extent --- is appropriate ifNewClass
wants to use aUiModelCheck
while doing whatever it does, but is not itself a widget.
Neither one is suitable (IMHO) for keeping "logic" and "UI" separate. If you really want to keep (back-end) logic separate from (front-end) UI then the back-end should not include or know anything about the front-end. It is however OK for the front-end to make calls to the back-end, but really not vice versa.
Qt's signals and slots mechanism is one way to help the back-end keep quite separate from the front-end. It allows them to "communicate" with each side knowing little about the other and (if written correctly) without the back-end even knowing whether there is or is not any front-end. -
Both the feedbacks in the end are really helpful, i have tried @Pl45m4 approach of .ui and .ui to .py, I am not sure since i have not explored with designer/creator if we can create a delegate and add model data, etc in it, but only used it for general layouting and rest i code it later. But none the less Thanks for taking out time for the reply.
@JonB
Thanks for the brevity information, i was hoping to get some clarity on this back-end and front-end logic, and you gave me exactly that. I will definitely study more about signals and slots, and try to implement in my work to keep ui and backend logic separate. Thanks for the wisdom.Thanks alot all of you in the thread.