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

[Xuetian Weng]

> On Oct. 2, 2015, 8:46 p.m., David Faure wrote:
> > Please check if the unittests in frameworkintegration still pass for you.
> > 
> > https://build.kde.org/job/frameworkintegration%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/20/testReport/junit/(root)/TestSuite/frameworkintegration_kfiledialog_unittest/ seems to indicate that your commit broke the tests, now the test program gets stuck and never terminates.
> 
> Xuetian Weng wrote:
>     It works here localily. Maybe for some reason that QTest::qWaitForWindowExposed will not work on test machine?

Ok now I get it. I uninstall kinit on my machine to make sure my test can't launch a kdeinit5 just like the server, and in Xephyr I saw that a dialog with content "Could not start process. Cannot talk to klauncher: The name org.kde.klauncher5 was not provided by and .service files." waiting for user to click on it. http://imgur.com/vCRMfkm


- Xuetian


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


On Sept. 30, 2015, 6:04 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125208/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2015, 6:04 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Jean-Baptiste Mardelle.
> 
> 
> Bugs: 350758
>     https://bugs.kde.org/show_bug.cgi?id=350758
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> -------
> 
> 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/20151002/e6da682f/attachment.html>