Review Request 130162: Why QDialogButtonBox::Close could not emit closeEvent?

[Harald Sitter]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130162/#review103343
-----------------------------------------------------------



There are a bunch of things to note here.

Firstly, please always use the new Qt 5 by-reference connection syntax when adding new connections. The old string syntax `SIGNAL()` and `SLOT()` are evaluated at runtime making them a substantial point of failure further down the road. https://wiki.qt.io/New_Signal_Slot_Syntax

Secondly, with QDialogButtonBox you want to connect the box, not the individual buttons, as seen earlier in the constructor.

Thirdly, your change is introducing excessive code. As Anthony said, closeEvent is called when the underlying QWidget gets closed.
i.e. `connect(buttonBox, &QDialogButtonBox::rejected, this, &QDialog::close);` without a new slot or anything would be sufficient to get exactly the same result as what you've done. That's not really the correct fix though...

Lastly, reading the QDialog documentation tells us why closeEvent is not called properly. The button `Close` has a reject role in the [QDialogButtonBox](http://doc.qt.io/qt-5/qdialogbuttonbox.html#StandardButton-enum) meaning it will emit `rejected` which we connect to `QDialog::reject` which is [documented](http://doc.qt.io/qt-5/qdialog.html#reject) as "Hides the modal dialog and sets the result code to Rejected.". Simply put QDialogs practically do not get closed, they get "hidden".

The way to fix this properly is to reimplemnt [QDialog::done](http://doc.qt.io/qt-5/qdialog.html#done) instead of the closeEvent. `done` is internally backing both accepted and rejected so it is always run. Change `closeEvent(QCloseEvent*)` to `done(int)` and this should work without any other changes.

- Harald Sitter


On June 19, 2017, 4:18 a.m., Leslie Zhai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130162/
> -----------------------------------------------------------
> 
> (Updated June 19, 2017, 4:18 a.m.)
> 
> 
> Review request for KDE Multimedia, Albert Astals Cid and Anthony Fieroni.
> 
> 
> Bugs: 381368
>     http://bugs.kde.org/show_bug.cgi?id=381368
> 
> 
> Repository: k3b
> 
> 
> Description
> -------
> 
> Dear,
> 
> As Dr. Chaptian reported "do not show again" from system configuration problems dialog is not remembered, so I simply added `slotClose` to write the entry for K3b's KConfigGroup. but I have no idea why QDialogButtonBox::Close could not emit closeEvent, not need to connect, perhaps old Qt v4.x was able to work?
> 
> Regards,
> Leslie Zhai
> 
> 
> Diffs
> -----
> 
>   src/k3bsystemproblemdialog.h b45f2f80c 
>   src/k3bsystemproblemdialog.cpp 9dfc50c1d 
> 
> Diff: https://git.reviewboard.kde.org/r/130162/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Leslie Zhai
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20170619/6f8856d9/attachment.htm>