Code review

qcoderpro

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();
}

sierdzio

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

@sierdzio
Thank you.

auto notify = this {
auto button = qobject_cast<QPushButton*>(sender());
btnLabel->setText(button->text());
};

This doesn't show anything!

sierdzio

@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

@sierdzio

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!

sierdzio

@qcoderpro said in Code review:

@sierdzio

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.

qcoderpro

@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"?

sierdzio

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.

qcoderpro

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!

Pl45m4

@qcoderpro

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.

sierdzio

@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.

qcoderpro

@sierdzio

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!

J.Hilk

personally I would prefer multiple lambdas over the use of sender()

sierdzio

@qcoderpro said in Code review:

error: calling 'property' with incomplete return type 'QVariant'

#include <QVariant>

qcoderpro

@sierdzio

Probably you mean this:

void dialogTest::printButton()
{
     btnLabel->setText(sender()->property("text").toString());
}

Still with the ampersands!

@J-Hilk

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

Not a bug?
If it is, how to report that?

mrjj

@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

@mrjj
Hi,

and not shown. So when you ask for its text() , its still included.

It's what the programmer needs, to be included but "not shown". In the example above, it's shown, while it mustn't! It can clearly be a bug I think.

Pl45m4

@qcoderpro said in Code review:

It can clearly be a bug I think

The button text contains the & to create the shortcut. So why should btn->text() not return the full text with the ampersand?
btn->text() just returns the QString which is the displayed button text (without any pre-processing). Only QPushButton (i.e. its base class QAbtractButton) converts & into the shortcut functionality and for this you need the ampersand in your button text. The text-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.

qcoderpro

@Pl45m4

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?