Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • Users
  • Groups
  • Search
  • Get Qt Extensions
  • Unsolved
Collapse
Brand Logo
  1. Home
  2. Qt Development
  3. General and Desktop
  4. Code review
Forum Updated to NodeBB v4.3 + New Features

Code review

Scheduled Pinned Locked Moved Solved General and Desktop
24 Posts 6 Posters 3.0k Views 5 Watching
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • sierdzioS Offline
    sierdzioS Offline
    sierdzio
    Moderators
    wrote on last edited by
    #8

    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.

    (Z(:^

    1 Reply Last reply
    2
    • Q Offline
      Q Offline
      qcoderpro
      wrote on last edited by qcoderpro
      #9

      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!

      Pl45m4P sierdzioS 2 Replies Last reply
      0
      • Q 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!

        Pl45m4P Offline
        Pl45m4P Offline
        Pl45m4
        wrote on last edited by Pl45m4
        #10

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


        If debugging is the process of removing software bugs, then programming must be the process of putting them in.

        ~E. W. Dijkstra

        1 Reply Last reply
        0
        • Q 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!

          sierdzioS Offline
          sierdzioS Offline
          sierdzio
          Moderators
          wrote on last edited by sierdzio
          #11

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

          (Z(:^

          Q 1 Reply Last reply
          0
          • sierdzioS 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.

            Q Offline
            Q Offline
            qcoderpro
            wrote on last edited by
            #12

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

            sierdzioS 1 Reply Last reply
            0
            • J.HilkJ Offline
              J.HilkJ Offline
              J.Hilk
              Moderators
              wrote on last edited by
              #13

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

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


              Be aware of the Qt Code of Conduct, when posting : https://forum.qt.io/topic/113070/qt-code-of-conduct


              Q: What's that?
              A: It's blue light.
              Q: What does it do?
              A: It turns blue.

              1 Reply Last reply
              1
              • Q 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!

                sierdzioS Offline
                sierdzioS Offline
                sierdzio
                Moderators
                wrote on last edited by
                #14

                @qcoderpro said in Code review:

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

                #include <QVariant>

                (Z(:^

                Q 1 Reply Last reply
                1
                • sierdzioS sierdzio

                  @qcoderpro said in Code review:

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

                  #include <QVariant>

                  Q Offline
                  Q Offline
                  qcoderpro
                  wrote on last edited by qcoderpro
                  #15

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

                  1 Reply Last reply
                  0
                  • Q Offline
                    Q Offline
                    qcoderpro
                    wrote on last edited by qcoderpro
                    #16

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

                    mrjjM 1 Reply Last reply
                    0
                    • Q qcoderpro

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

                      mrjjM Offline
                      mrjjM Offline
                      mrjj
                      Lifetime Qt Champion
                      wrote on last edited by
                      #17

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

                      Q 1 Reply Last reply
                      1
                      • mrjjM 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.

                        Q Offline
                        Q Offline
                        qcoderpro
                        wrote on last edited by
                        #18

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

                        Pl45m4P 1 Reply Last reply
                        0
                        • Q 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.

                          Pl45m4P Offline
                          Pl45m4P Offline
                          Pl45m4
                          wrote on last edited by Pl45m4
                          #19

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


                          If debugging is the process of removing software bugs, then programming must be the process of putting them in.

                          ~E. W. Dijkstra

                          Q 1 Reply Last reply
                          2
                          • Pl45m4P 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.

                            Q Offline
                            Q Offline
                            qcoderpro
                            wrote on last edited by
                            #20

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

                            1 Reply Last reply
                            0
                            • SGaistS Offline
                              SGaistS Offline
                              SGaist
                              Lifetime Qt Champion
                              wrote on last edited by
                              #21

                              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.

                              Interested in AI ? www.idiap.ch
                              Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

                              Q 1 Reply Last reply
                              1
                              • SGaistS SGaist

                                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.

                                Q Offline
                                Q Offline
                                qcoderpro
                                wrote on last edited by
                                #22

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

                                1 Reply Last reply
                                0
                                • SGaistS Offline
                                  SGaistS Offline
                                  SGaist
                                  Lifetime Qt Champion
                                  wrote on last edited by
                                  #23

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

                                  Interested in AI ? www.idiap.ch
                                  Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

                                  Q 1 Reply Last reply
                                  1
                                  • SGaistS SGaist

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

                                    Q Offline
                                    Q Offline
                                    qcoderpro
                                    wrote on last edited by qcoderpro
                                    #24

                                    @SGaist

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

                                    1 Reply Last reply
                                    0

                                    • Login

                                    • Login or register to search.
                                    • First post
                                      Last post
                                    0
                                    • Categories
                                    • Recent
                                    • Tags
                                    • Popular
                                    • Users
                                    • Groups
                                    • Search
                                    • Get Qt Extensions
                                    • Unsolved