Code review
-
Hello,
Will you take a look at my code here. Is it completely correct to you?
dialogTest.h:
#ifndef DIALOGTEST_H #define DIALOGTEST_H #include <QDialog> class QPushButton; class QDialogButtonBox; class QLabel; class dialogTest : public QDialog { Q_OBJECT public: dialogTest(QWidget *parent = nullptr); private: QPushButton* one, *two, *three; QLabel* label, *btnLabel; QDialogButtonBox* dbBox; }; #endif // DIALOGTEST_H
dialogTest.cpp:
#include "dialogtest.h" #include <QPushButton> #include <QLabel> #include <QDialogButtonBox> #include <QHBoxLayout> #include <QVBoxLayout> #include <QMessageBox> dialogTest::dialogTest(QWidget *parent) : QDialog(parent) { one = new QPushButton(tr("&One")); two = new QPushButton(tr("&Two")); three = new QPushButton(tr("T&hree")); dbBox = new QDialogButtonBox(QDialogButtonBox::Ok); label = new QLabel(tr("Button Clicked:\n\r")); btnLabel = new QLabel; QHBoxLayout* hLayout = new QHBoxLayout; hLayout->addWidget(one); hLayout->addWidget(two); hLayout->addWidget(three); QVBoxLayout* vLayout = new QVBoxLayout; vLayout->addLayout(hLayout); vLayout->addWidget(label); vLayout->addWidget(btnLabel); vLayout->addWidget(dbBox); setLayout(vLayout); connect(dbBox, &QDialogButtonBox::clicked, this, &dialogTest::accept); connect(one, &QPushButton::clicked, [this]() { btnLabel->setText("One"); }); connect(two, &QPushButton::clicked, [this]() { btnLabel->setText("Two"); }); connect(three, &QPushButton::clicked, [this]() { btnLabel->setText("Three"); }); }
main.cpp:
#include "dialogtest.h" #include <QApplication> int main(int argc, char *argv[]) { QApplication a(argc, argv); dialogTest w; w.show(); return a.exec(); }
-
- lowercase class name - not a bug, but unorthodox
- multiple pointers not initialized with
nullptr
in header file - connect statements contain a lambda but no control object (see https://github.com/KDE/clazy/blob/master/docs/checks/README-connect-3arg-lambda.md)
- user-visible strings not enclosed in
tr()
calls (in lambdas) - code duplication in lambdas. Better use something like:
auto notify = [this]() { auto button = qobject_cast<QPushButton*>(sender()); btnLabel->setText(button->text()); };
-
@qcoderpro said in Code review:
@sierdzio
Thank you.auto notify = this {
auto button = qobject_cast<QPushButton*>(sender());
btnLabel->setText(button->text());
};This doesn't show anything!
Possibly it needs to be done in a proper slot and not a lambda function.
-
@qcoderpro said in Code review:
Possibly it needs to be done in a proper slot and not a lambda function.
And that slot is gonna be called each time any of those buttons is clicked. So once again three repetitive connections!
But only single implementation ;-) Now you have 3.
-
@sierdzio
Alright.Thanks for the link. What does "a context object" there completely mean please? I know in my example it mean the receiver, "btnLabel", but does it mean the object which receives the signal? Then why "a context object" and not "the receiver object"?
-
Whatever object is meaningful in the context. This is used to disconnect the slot when necessary. In your case, code is executed in
this
(your dialog), so I'd use that as context object. -
I initialized all pointers in the header with "nullptr" and created a private slot:
private slots: void printButton();
Then changed the implementation to this:
dialogTest::dialogTest(QWidget *parent) : QDialog(parent) { one = new QPushButton(tr("&One")); two = new QPushButton(tr("&Two")); three = new QPushButton(tr("T&hree")); dbBox = new QDialogButtonBox(QDialogButtonBox::Ok); label = new QLabel(tr("Button Clicked:\n\r")); btnLabel = new QLabel; QHBoxLayout* hLayout = new QHBoxLayout; hLayout->addWidget(one); hLayout->addWidget(two); hLayout->addWidget(three); QVBoxLayout* vLayout = new QVBoxLayout; vLayout->addLayout(hLayout); vLayout->addWidget(label); vLayout->addWidget(btnLabel); vLayout->addWidget(dbBox); setLayout(vLayout); connect(dbBox, &QDialogButtonBox::clicked, this, &dialogTest::accept); connect(one, &QPushButton::clicked, this, &dialogTest::printButton); connect(two, &QPushButton::clicked, this, &dialogTest::printButton); connect(three, &QPushButton::clicked, this, &dialogTest::printButton); } void dialogTest::printButton() { auto button = qobject_cast<QPushButton*>(sender()); btnLabel->setText(button->text()); }
- Is it all OK now, please?
- When the button "one" is clicked, the string "&One" is shown, including the ampersand!
- The destructor is removed because all widgets are children of the top-level widget which is created on the stack, so there's nothing to be deleted manually, hence no memory leak.
- If we explain what the line below in the "printButton()" slot means in simple language, we would say:
auto button = qobject_cast<QPushButton*>(sender());
Here "sender" returns a pointer to the object which called the slot (here a QPushButton), then that pointer is cast to <QPushButton*> and finally used to initialize "button". A shade vague!
-
In addition you could rename your class, so that it stars with an upper case ( @sierdzio said that at the beginning as well)
It's not wrong, but code convention: class names start with upper case, then camel case. Variable names start with lower case letter. -
@qcoderpro said in Code review:
- Is it all OK now, please?
Looks fine to me.
- When the button "one" is clicked, the string "&One" is shown, including the ampersand!
Ah, that's bad. Well, then you can probably go back to your original solution :-( Or, there is a chance that
sender()->property("text").toString()
will return it without ampersand (and no need for a cast!), but I don't know.- The destructor is removed because all widgets are children of the top-level widget which is created on the stack, so there's nothing to be deleted manually, hence no memory leak.
Fine.
- If we explain what the line below in the "printButton()" slot means in simple language, we would say:
auto button = qobject_cast<QPushButton*>(sender());
Here "sender" returns a pointer to the object which called the slot (here a QPushButton), then that pointer is cast to <QPushButton*> and finally used to initialize "button". A shade vague!
It might be unclear to people who do not know Qt, yes. The
sender()
method is a very special kind of method. But to anybody who has used Qt for a while, this is perfectly understandable. -
there is a chance that sender()->property("text").toString() will return it without ampersand (and no need for a cast!), but I don't know.
With or without a cast, the error below turns up:
error: calling 'property' with incomplete return type 'QVariant'
Optimistically, there's a way to solve it and not ruin what we've done from the first place where I used three implementations! :|It might be unclear to people who do not know Qt, yes. The sender() method is a very special kind of method. But to anybody who has used Qt for a while, this is perfectly understandable.
Do you mean that is the exact "sender" as the first parameter in the Q_OBJECT::connection!? Yes, I think!
-
personally I would prefer multiple lambdas over the use of sender()
-
@qcoderpro said in Code review:
error: calling 'property' with incomplete return type 'QVariant'
#include <QVariant>
-
Probably you mean this:
void dialogTest::printButton() { btnLabel->setText(sender()->property("text").toString()); }
Still with the ampersands!
personally I would prefer multiple lambdas over the use of sender()
I used both versions below:
connect(one, &QPushButton::clicked, this, [this]() { btnLabel->setText("&One"); }); connect(two, &QPushButton::clicked, this, [this]() { btnLabel->setText(two->text()); });
Still with the ampersands!
-
@qcoderpro
Hi
Its not a bug. When the text contains a &, a shortcut is made for it but its not removed and not
shown. So when you ask for its text() , its still included. -
@qcoderpro said in Code review:
It can clearly be a bug I think
The button text contains the
&
to create the shortcut. So why shouldbtn->text()
not return the full text with the ampersand?
btn->text()
just returns theQString
which is the displayed button text (without any pre-processing). OnlyQPushButton
(i.e. its base classQAbtractButton
) converts&
into the shortcut functionality and for this you need the ampersand in your button text. Thetext
-getter does not know about this, so the actual text with ampersand is returned.Another example:
If you inspect HTML with a texteditor, you will see <br> and no line break, because the text editor doesn't interpret this as line break, it just shows the raw UTF8 (or whatever) encoded text. -
With this we have two options: either "not to use shortcuts" or "have the text including the ampersand"! Neither is wanted to me.
To me, the code behind "setText" should've been designed so that it could process the text and omit the ampersands when showing.Do you have a better way?