Code review

tomy

It's automatically done by the layouts I've used below.

mrjj

@tomy
Hi
Looks good. Good naming. using new connect syntax, tr macro for the text.

Unexpected behavior

You mean it goes in here

 if(!saved && isTextChanged)
    {
        QMessageBox::StandardButton btn = QMessageBox::question(this, "Save Files?",
                                                                "Your file has been changed, would you like to save it?");
        if(btn == QMessageBox::StandardButton::Yes) on_SaveBtn();
    }

even isTextChanged is still false ?

PS: The fourth connection is suspicious to me, however.
This ?
connect(save, &QPushButton::clicked, this, &TestIcon::on_SaveBtn);

what is the suspicious part ? :)

Pablo J. Rogina

@tomy said in Code review:

It's automatically done by the layouts I've used below.

could you please share that code snippet?

tomy

@mrjj

even isTextChanged is still false ?

Yup! :|

what is the suspicious part ? :)

Sorry I meant the fifth one, because the argument for the signal and slot doesn't match!

@Pablo-J-Rogina

could you please share that code snippet?

You mean the layout used? It's in the code above. You mean the reason why parenting is done through layouts? It must be documented on Docs quite possibly.

Pablo J. Rogina

@tomy said in Code review:

You mean the reason why parenting is done through layouts?

I asked because you're using layouts, which is good, but I cannot see you're handling proper parenting, i.e. that one widget has another widget as its "parent". From documentation:

QObjects organize themselves in object trees. When you create a QObject with another object as parent, the object will automatically add itself to the parent's children() list. The parent takes ownership of the object; i.e., it will automatically delete its children in its destructor. You can look for an object by name and optionally type using findChild() or findChildren().

So for example your widgets

name = new QLabel(tr("Name"));
line = new QLineEdit;

aren't assigned a parent at constructor (as @guerinoni pointed out) and adding those widgets to a layout seem not to fix missing parent...

hrzLayout->addWidget(name);
hrzLayout->addWidget(line);

since addWidget() method doesn't mess with widget parents:

void QBoxLayout::addWidget(QWidget *widget, int stretch = 0, Qt::Alignment alignment = Qt::Alignment())

fcarney

@Pablo-J-Rogina You rotated your avatar. Freaked me out man! ;-)

tomy

@Pablo-J-Rogina

I think you're wrong, with complete respect to you. The addWidget function, as the name suggests, adds a widget to the layout. When added to the layout, Qt adds it to the tree as the child of the layout.

Pablo J. Rogina

@tomy said in Code review:

When added to the layout, Qt adds it to the tree as the child of the layout.

It looks like you're confusing "layout tree" (GUI) with "object parenting tree" (memory).

Small test.
Case A: not using parents (mostly based on your code)

Dialog::Dialog(QWidget *parent)
    : QDialog(parent)
{
    qDebug() << "'dialog' object created: " << this << ". Parent:" << this->parent();

    name = new QLabel(tr("Name"));
    line = new QLineEdit;
    qDebug() << "'name' object created: " << name << ". Parent:" << name->parent();
    qDebug() << "'line' object created: " << line << ". Parent:" << line->parent();

    auto hrzLayout = new QHBoxLayout;
    qDebug() << "'hrzLayout' object created: " << hrzLayout << ". Parent:" << hrzLayout->parent();

    hrzLayout->addWidget(name);
    hrzLayout->addWidget(line);
    qDebug() << "After adding to layout. Parent of 'name':" << name->parent();
    qDebug() << "After adding to layout. Parent of 'line':" << line->parent();
}

Case A console output:

'dialog' object created:  Dialog(0x7ffed5b08fd0) . Parent: QObject(0x0)
'name' object created:  QLabel(0x556425a32f10) . Parent: QObject(0x0)
'line' object created:  QLineEdit(0x556425a3c3e0) . Parent: QObject(0x0)
'hrzLayout' object created:  QHBoxLayout(0x556425affbb0) . Parent: QObject(0x0)
After adding to layout. Parent of 'name': QObject(0x0)
After adding to layout. Parent of 'line': QObject(0x0)

Case B: using object parents

Dialog::Dialog(QWidget *parent)
    : QDialog(parent)
{
    qDebug() << "'dialog' object created: " << this << ". Parent:" << this->parent();

    name = new QLabel(tr("Name"), this);
    line = new QLineEdit(this);
    qDebug() << "'name' object created: " << name << ". Parent:" << name->parent();
    qDebug() << "'line' object created: " << line << ". Parent:" << line->parent();

    auto hrzLayout = new QHBoxLayout(this);
    qDebug() << "'hrzLayout' object created: " << hrzLayout << ". Parent:" << hrzLayout->parent();

    hrzLayout->addWidget(name);
    hrzLayout->addWidget(line);
    qDebug() << "After adding to layout. Parent of 'name':" << name->parent();
    qDebug() << "After adding to layout. Parent of 'line':" << line->parent();
}

Case B console output:

'dialog' object created:  Dialog(0x7ffda6e94b80) . Parent: QObject(0x0)
'name' object created:  QLabel(0x5637c9e34f00) . Parent: Dialog(0x7ffda6e94b80)
'line' object created:  QLineEdit(0x5637c9e3e2a0) . Parent: Dialog(0x7ffda6e94b80)
'hrzLayout' object created:  QHBoxLayout(0x5637c9f01230) . Parent: Dialog(0x7ffda6e94b80)
After adding to layout. Parent of 'name': Dialog(0x7ffda6e94b80)
After adding to layout. Parent of 'line': Dialog(0x7ffda6e94b80)

Clearly not adding parents to objects make a difference... and layouts don't fix object parenting.

tomy

@Pablo-J-Rogina

By 'tree' I meant parent-children tree for memory management.

I think layouts do the job (parenting) when when they're set to the widget as in:

setLayout(mainLayout);

And it's pretty straightforward from my point of view. For instance, assume a layout is a container or an empty box. You put some things into it and define it as the child of a main widget. This way the layout plus its contents all become the children of the main widget.

PS: I've always used dynamically allocated objects with layouts this way in my projects and posted them many many times here for bugs, reviews, etc. No admin, moderator, Qt Champion has said that explicit parenting is necessary for those objects when adding them to a layout and using them the way I did above, yet!

JonB

@Pablo-J-Rogina , @tomy

You are both confusing me! I'm not sure whether you are in agreement with each other or not. But I read @tomy as correct.

@Pablo-J-Rogina wrote:

since addWidget() method doesn't mess with widget parents:

Oh, but yes it does! That's where you are making an assumption which is incorrect. QBoxLayout::addWidget() does set parentage, and so does QWidget::setLayout():

#include <QApplication>
#include <QVBoxLayout>
#include <QWidget>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    QWidget *grandparent = new QWidget;
    Q_ASSERT(grandparent->parent() == nullptr);

    // create `grandparentLayout`, and immediately add it
    QVBoxLayout *grandparentLayout = new QVBoxLayout;
    Q_ASSERT(grandparentLayout->parent() == nullptr);
    grandparent->setLayout(grandparentLayout);
    Q_ASSERT(grandparentLayout->parent() == grandparent);

    QWidget *parent = new QWidget;
    Q_ASSERT(parent->parent() == nullptr);
    grandparentLayout->addWidget(parent);
    // as soon as `parent` is added onto `grandparentLayout` it gets parented, because `grandparentLayout` is parented
    Q_ASSERT(parent->parent() == grandparent);

    // create `parentLayout`, but don't add it yet, leave it "floating"
    QVBoxLayout *parentLayout = new QVBoxLayout;
    Q_ASSERT(parentLayout->parent() == nullptr);

    QWidget *grandchild = new QWidget;
    Q_ASSERT(grandchild->parent() == nullptr);
    // `grandchild` does not get parented yet, because `parentLayout` is not yet parented...
    parentLayout->addWidget(grandchild);
    Q_ASSERT(grandchild->parent() == nullptr);

    parent->setLayout(parentLayout);
    Q_ASSERT(parentLayout->parent() == parent);
    // ...`grandchild` gets parented now, because `parentLayout` is now added
    Q_ASSERT(grandchild->parent() == parent);

    grandparent->show();
    return a.exec();
}

The long and the short: it is absolutely true that if you just create a "floating" layout (i.e. not attached anywhere), and add a parentless widget to it, then the widget (and the layout) will "leak". But so long as you add your layouts & widgets into the tree you do not need to use the parent to either layouts or widgets, as I show here, they get parented during setLayout() or addWidget() as necessary.

Like @tomy, I tend not to bother specifying parents when creating widgets/layouts, so long as I add them onto the tree later on. But if I created a widget/layout which might not be added onto the tree (for whatever reason), I would specify a parent to prevent leak.

VRonin

Parenting is fine, the parent argument in your case is optional.
I wouldn't use manual booleans to manage making the document dity, you should use https://doc.qt.io/qt-5/qwidget.html#windowModified-prop which also deals with the window title automatically
void TestIcon::closeEvent(QCloseEvent *event) is a mess. Clicking the cancel button or closing the dialog window will trigger both an accept (called from closeEvent) and a reject called from the slot. I wouldn't be able to tell you which one takes priority without digging into the dark of Qt sources. Get rid of this method and of the connect(btnBox, &QDialogButtonBox::accepted, this, &TestIcon::accept);, create a new slot connected to the QDialogButtonBox::accepted signal and handle the unsaved changes there

tomy

@VRonin

I wouldn't use manual booleans to manage making the document dity, you should use https://doc.qt.io/qt-5/qwidget.html#windowModified-prop which also deals with the window title automatically

It says, "A modified window is a window whose content has changed but has not been saved to disk.", and I use:

if(isWindowModified())

But the QMessageBox is not called even if I put some more content into the line edit and close the dialog without saving!

void TestIcon::closeEvent(QCloseEvent *event) is a mess. Clicking the cancel button or closing the dialog window will trigger both an accept (called from closeEvent) and a reject called from the slot.

What slot, please? I didn't understand this part completely.

Get rid of this method and of the connect(btnBox, &QDialogButtonBox::accepted, this, &TestIcon::accept);, create a new slot connected to the QDialogButtonBox::accepted signal and handle the unsaved changes there.

But I want to provide with the user an option to save the changes when he/she closes (hit the X button of the dialog) without saving those changes, so I need to re-implement the closeEvent I guess.

VRonin

@tomy said in Code review:

But I want to provide with the user an option to save the changes when he/she closes (hit the X button of the dialog) without saving those changes, so I need to re-implement the closeEvent I guess.

What is the difference between pressing the Ok or Cancel button in your dialog?

tomy

@VRonin

What is the difference between pressing the Ok or Cancel button in your dialog?

They're connected to QDialog's Accepted/Rejected slots which hide the dialog and set the so-called result code to Accepted/Rejected!

I know that they're awkward since they do almost nothing and by even no having them the program can work properly. But I put them there just to have the dialog look nicer and more normal than without them.

The more important issue is that the isWindowModified() method doesn't work as expected! If it works, we can get rid of the booleans.

tomy

I solved the problem this way, using a bool and line->isModified():

testicon.h:

#ifndef TESTICON_H
#define TESTICON_H

#include <QDialog>

class QLabel;
class QLineEdit;
class QPushButton;

class TestIcon : public QDialog
{
    Q_OBJECT

public:
    TestIcon(QWidget *parent = nullptr);
    void on_SaveBtn();
    void load();

protected:
    void closeEvent(QCloseEvent *event) override;

private:
    QLabel* name = nullptr;
    QLineEdit* line = nullptr;
    QPushButton* clear = nullptr;
    QPushButton* save = nullptr;
    bool saved = false;
};
#endif // TESTICON_H

testicon.cpp:

#include "testicon.h"
#include <QLabel>
#include <QPushButton>
#include <QSettings>
#include <QMessageBox>
#include <QLineEdit>
#include <QHBoxLayout>

TestIcon::TestIcon(QWidget *parent)
    : QDialog(parent)
{
    name = new QLabel(tr("Name"));
    line = new QLineEdit;
    clear = new QPushButton(QIcon(":/files/Files/clear.png"), tr("Clear"));
    save = new QPushButton(QIcon(":/files/Files/save.png"),tr("Save"));

    auto mLayout = new QHBoxLayout;
    mLayout->addWidget(name);
    mLayout->addWidget(line);
    mLayout->addWidget(clear);
    mLayout->addWidget(save);

    setLayout(mLayout);

    connect(clear, &QPushButton::clicked, line, &QLineEdit::clear);
    connect(save, &QPushButton::clicked, this, &TestIcon::on_SaveBtn);
    load();
}

void TestIcon::on_SaveBtn()
{
  QSettings settings ("Icon", "Test");
  settings.setValue("name", line->text());
  QMessageBox::information(this, "Saved", "Your new name is saved now!");
  saved = true;
}

void TestIcon::load()
{
    QSettings settings ("Icon", "Test");
    line->setText(settings.value("name").toString());
}

void TestIcon::closeEvent(QCloseEvent *event)
{
    if(line->isModified() && !saved)
    {
        QMessageBox::StandardButton btn = QMessageBox::question(this, "Save Files?",
                           "Your file has been changed, would you like to save it?");
        if(btn == QMessageBox::StandardButton::Yes) on_SaveBtn();
    }
}

Do you have a better solution, please?