Review Request 125208: Fixes file dialog randomly not showing up with frameworkintegration

[Xuetian Weng]

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

(Updated Sept. 13, 2015, 12:18 a.m.)


Review request for KDE Frameworks, David Faure and Jean-Baptiste Mardelle.


Summary (updated)
-----------------

Fixes file dialog randomly not showing up with frameworkintegration


Bugs: 350758
    https://bugs.kde.org/show_bug.cgi?id=350758


Repository: frameworkintegration


Description (updated)
-------

1. The directly reason of 350758 is https://bugreports.qt.io/browse/QTBUG-48248, which seems to be a Qt problem. See the code example. One possible to avoid this is to remove the m_dialog->hide().

2. Why hide() is needed in the first place?
If one runs dialog.show() and dialog.exec(), it should works. So there is something happened in between the calls to KDEPlatformFileDialogHelper::show() and KDEPlatformFileDialogHelper::exec().

The real reason is the platform dialog helper doesn't expect that user may use "Qt" dialog to implement it.
https://github.com/qtproject/qtbase/blob/5bfac9d653357c906946563b9494d7ae69cdad92/src/widgets/dialogs/qfiledialog.cpp#L845
here it sets Qt::WA_DontShowOnScreen on the actual dialog.

https://github.com/qtproject/qtbase/blob/5bfac9d653357c906946563b9494d7ae69cdad92/src/widgets/dialogs/qfiledialog.cpp#L868

and call QDialog::setVisible() here

https://github.com/qtproject/qtbase/blob/17d6b2d1f02e5f679008d97036befd713025a0f2/src/widgets/dialogs/qdialog.cpp#L713

Thus it will by pass the check, and QDialog will actually show an invisible qdialog. I assume https://git.reviewboard.kde.org/r/123335/ tries to solve this issue by "steal" the focus from that invisible dialog by hide our dialog and show it again.

It makes some sense for Qt with "real" native dialog which is not controlled by Qt, because this invisble dialog helps Qt to aware that there's a modal dialog, but doesn't work for our case since our dialog is also a QDialog.

3. To avoid calling hide() in exec()
I'd rather to see that KDEPlatformFileDialogHelper::show() is not called in QDialog::exec(), but that's not gonna happen anytime soon. So here I used a trick with QTimer to delegate the call to show() to next event.

I uses QTimer instead of QMetaObject::invoke here, because we can discard the "m_dialog->show()" if hide() or exec() is called immediately. (Actually we don't need to discard the one in exec(), but we can save a call to m_dialog->show() if we discard it.)

Before this change, the calling sequence is 

m_dialog->show();
The dummy dialog setVisible(true);
m_dialog->hide();
m_dialog->exec();

After this change, it becomes
The dummy dialog setVisible(true);
m_dialog->exec();

4. Required changes to autotest
QTest::qWaitForWindowExposed(fw->window()) is added to some necessary places.


Diffs
-----

  autotests/kfiledialog_unittest.cpp 0d4c816 
  autotests/kfiledialogqml_unittest.cpp f805ef2 
  src/platformtheme/kdeplatformfiledialogbase.cpp 8e696bd 
  src/platformtheme/kdeplatformfiledialogbase_p.h 5936dfb 
  src/platformtheme/kdeplatformfiledialoghelper.h dfbbed1 
  src/platformtheme/kdeplatformfiledialoghelper.cpp 94f2059 

Diff: https://git.reviewboard.kde.org/r/125208/diff/


Testing
-------

not able to reproduce the bug.


Thanks,

Xuetian Weng

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150913/c764fd61/attachment.html>