Code review

JonB

@blossomsg
Makes no sense to me using splitters. Use a layout, that's what they are there for. Maybe a 3-column QGridLayout. Having said that don't know what your third column of empty boxes is for.

blossomsg

The empty boxes in the rhs will update with a color green/red after running the fix - status

blossomsg

I will try layout on my end by tonight
Just as a query - will we be able to maintain the spacing height on each line similar to the first image with splitters? the intention to get all in one line with proper height and spacing

blossomsg

grid layout worked well, had to add rowSpan and colSpan incases of multiple buttons in 2 and 3 column. thank you.
can you suggest any more modifications?

blossomsg

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

JonB

@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 from QtWidgets.QWidget. If you want to inherit from UiModelCheck (to create a "grandchild" class, NewClass <- UiModelCheck <- QWidget) then you may do so.

blossomsg

Okay that is one of the approaches, actually, currently in my experience

for eg:
approach you provided

class 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

jsulm

@blossomsg It really depends. If you want NewClass to be a widget then inherit UiModelCheck (first approach).

blossomsg

noted, Thank you.
any more suggestions?

Pl45m4

@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

haha, i wish there was even snow in my region. but i'll avoid yellow snow for sure.
I have been told many a times my codes are not well written, so i just though of getting a feedback.
thanks.
I guess its good to close then

Pl45m4

@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 use QUiLoader 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 whole QObject / 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.

JonB

@blossomsg
The first approach --- subclassing --- is appropriate if NewClass IS a UiModelCheck widget.
The second approach --- encapsulating, to some extent --- is appropriate if NewClass wants to use a UiModelCheck 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.

blossomsg

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.