1

I see the accept() somewhat similar to a return, so I've been putting it a the end of my slots with no code afterwards. That is, the accept() "finishes" the execution of the dialog.

Nevertheless, I came across the need to close a dialog and open a new one from a slot in the first one. Therefore, what I thought was moving the accept() to the beginning of the slot and initializing the second dialog after it. Something like the following:

void FirstDialog:slotFirstDialog()
{
  accept();
  // Setup second dialog arguments
  // ...
  SecondDialog *sd = new SecondDialog();
  sd->exec();
}

Is this use of accept() valid? Is it good practice?

Alechan
  • 817
  • 1
  • 10
  • 24

2 Answers2

2

I'd avoid it. Calling accept() can trigger a delayed deletion of FirstDialog (say, if it has the Qt::WA_DeleteOnClose flag set)1; in that case, it would be deleted in one of the first events dispatched by the nested event loop (sd->exec()), which would lead to go on executing code in a method of an instance that has been deleted. This is just a sample problem on the top of my head, I'm sure others can be found.

I'd probably just hide the dialog before calling exec() on the other, and call accept() after the end of the nested event loop.

void FirstDialog:slotFirstDialog()
{
    // Setup second dialog arguments
    // ...
    SecondDialog *sd = new SecondDialog();
    hide();
    sd->exec();
    accept();
    // NB are we leaking sd? 
}

By the way:

SecondDialog *sd = new SecondDialog();
sd->exec();

here you are allocating on the heap a dialog without a parent, so either you set the Qt::WA_DeleteOnClose or explicitly call this->deleteLater() inside its code, or you are leaking the dialog instance.

Notes:

  1. and it is explicitly remarked in the documentation

    As with QWidget::close(), done() deletes the dialog if the Qt::WA_DeleteOnClose flag is set.

Community
  • 1
  • 1
Matteo Italia
  • 123,740
  • 17
  • 206
  • 299
  • 1
    I moved the "accept()" to the end of the function and added a "hide()" at the beginning. Is this what you meant? – Alechan Sep 10 '18 at 16:33
  • Yes, exactly like that. Check out also the lifetime of the second dialog: who is responsible to delete it in your code? – Matteo Italia Sep 10 '18 at 16:42
  • Thanks, that seems to work. Can you edit your answer to add a code excerpt with my original code but "fixed" with hide() so the answer is more complete? – Alechan Sep 10 '18 at 16:43
  • You were right, I was leaking memory. So it should be "SecondDialog *sd = new SecondDialog(this);". Also, I would put the "hide();" at the beginning of the function to be more "faithful" to the [wrapper design pattern](https://stackoverflow.com/questions/889160/what-is-a-wrapper-class) with the "second dialog initialization" being the wrapped behaviour. – Alechan Sep 10 '18 at 17:31
0

QDialog::accept calls QDialog::done with a dialog code Accepted. Here is how QDialog::done looks like:

void QDialog::done(int r)
{
    Q_D(QDialog);
    setResult(r);
    hide();
    d->close_helper(QWidgetPrivate::CloseNoEvent);
    d->resetModalitySetByOpen();
    emit finished(r);
    if (r == Accepted)
        emit accepted();
    else if (r == Rejected)
        emit rejected();
}

which, according to the documentation:

Hides the modal dialog and sets the result code to Accepted.

With this in mind, I think this is not a question of a good practice, but of what your application logic requires.

scopchanov
  • 7,966
  • 10
  • 40
  • 68
  • My application logic requires that one dialog opens another dialog. It's not clear to me with the code excerpt from your answer if what I did is valid or not – Alechan Sep 10 '18 at 16:34
  • @Alechan, as I have already mentioned: _I think this is not a question of a good practice, but of what your application logic requires_. So I have shown you what `done` does in order to be able to decide, based on your specific application, if you need this executed in the beginning or in the end of your slot. – scopchanov Sep 24 '18 at 17:08