Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

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

  • Moderators

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


  • @sierdzio
    Thank you.

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

    This doesn't show anything!


  • Moderators

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



  • @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!


  • Moderators

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



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


  • Moderators

    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!



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


  • Moderators

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



  • @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!


  • Moderators

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

    e5abd79e-e6aa-4e2b-9941-864d545d9865-image.png


  • Moderators

    @qcoderpro said in Code review:

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

    #include <QVariant>



  • @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!



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


  • Lifetime Qt Champion

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



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



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



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


  • Lifetime Qt Champion

    Hi,

    You have here the text property. You set a text, you retrieve the same text. If you want something else, then it's up to you to a step that does transform the output the way you want it.

    When setting a property, I am expecting the getter to return the exact same value. The special ampersand handling falls in that case. Since I want to use a shortcut then I will handle that fact in my code. If you want to not have to do that then use a normal text and create a QShortCut by hand.



  • @SGaist

    create a QShortCut by hand

    I found it hard for my example.
    Instead, used a slot:

    .h :

    ...
    private slots:
        QString removeAmpersand(QString);
    ...
    

    .cpp:

    ...
    connect(one, &QPushButton::clicked, this, [this]() {
           btnLabel->setText(removeAmpersand(one->text()));
       });
    
       connect(two, &QPushButton::clicked, this, [this]() {
           btnLabel->setText(removeAmpersand(two->text()));
       });
       connect(three, &QPushButton::clicked, this, [this]() {
           btnLabel->setText(removeAmpersand(three->text())); });
    }
    
    QString DialogTest::removeAmpersand(QString text)
    {
        QString res = "";
        for(auto t : text)
         if(t != '&')
             res += t;
        return res;
    }
    ...
    

    Fine?


  • Lifetime Qt Champion

    That's not a slot just a normal method. QString::remove will make your code shorter.



  • @SGaist

    Yes, since it's not used in a connection, it's not a slot. Thanks. Solved.


Log in to reply