Code review
-
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. -
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 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 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()
-
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!
@qcoderpro said in Code review:
error: calling 'property' with incomplete return type 'QVariant'
#include <QVariant> -
@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!
@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
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
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 shouldbtn->text()not return the full text with the ampersand?
btn->text()just returns theQStringwhich 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. -
@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 theQStringwhich 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?
-
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.
-
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.
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?
-
That's not a slot just a normal method. QString::remove will make your code shorter.
-
That's not a slot just a normal method. QString::remove will make your code shorter.
