Review Request 125208: Workaround bug 350758

[Xuetian Weng]

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

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


Repository: frameworkintegration


Description
-------

1. The reason of 350758: https://bugreports.qt.io/browse/QTBUG-48248 , I just tried to reproduce it with pure Qt code. If a dialog called against show, hide, and exec one immediately after another, the dialog may not show up.

2. Why hide() is needed in the first place?
If one runs dialog.show() and dialog.exec(), it usually works. So there is something happened in between 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 first check, and QDialog will actually show an invisible qdialog with it. I assume https://git.reviewboard.kde.org/r/123335/ tries to solve this issue by "steal" the focus from that invisible dialog by hide it first.

It makes some sense for Qt with "real" native dialog, because this invisble dialog helps Qt to aware that there's a modal dialog, but doesn't work for our case. 

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. So here I used a trick with QTimer to delegate the show to next event when calling to KDEPlatformFileDialogHelper::show().

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 on m_dialog 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/20150912/b49389a7/attachment-0001.html>